Re: [Qemu-devel] [PATCH v3 6/9] target-mips: kvm: Add main KVM support for MIPS

[Andreas Färber]

Am 06.03.2014 18:09, schrieb James Hogan:
> From: Sanjay Lal <address@hidden>
> 
> Implement the main KVM arch API for MIPS.
> 
> Signed-off-by: Sanjay Lal <address@hidden>
> Signed-off-by: James Hogan <address@hidden>
> Cc: Aurelien Jarno <address@hidden>
> Cc: Gleb Natapov <address@hidden>
> Cc: Paolo Bonzini <address@hidden>
> Cc: Andreas Färber <address@hidden>
> Cc: Peter Maydell <address@hidden>
> ---
> Changes in v3:
>  - s/dprintf/DPRINTF/ (Andreas Färber).
>  - Use "cs" rather than "cpu" or "env" for CPUState variable names
>    (Andreas Färber).
>  - Use CPUMIPSState rather than CPUArchState (Andreas Färber).
>  - Pass MIPSCPU to cpu_mips_io_interrupts_pending() rather than
>    CPUMIPSState (Andreas Färber).
>  - Remove spurious parentheses around cpu_mips_io_interrupts_pending()
>    call (Andreas Färber).
>  - Pass MIPSCPU to kvm_mips_set_[ipi_]interrupt (Andreas Färber).
>  - Make use of error_report (Andreas Färber) and clean up error messages
>    a little to include __func__.
>  - Remove inline kvm_mips_{put,get}_one_[ul]reg() declarations from
>    kvm_mips.h. They're only used in target-mips/kvm.c anyway.
>  - Make kvm_arch_{put,get}_registers static within target-mips/kvm.c and
>    remove from kvm_mips.h.
>  - Set sigmask length to 16 from kvm_arch_init() since MIPS Linux has
>    128 signals. This is better than cluttering kvm_all.c with TARGET_*
>    ifdefs (Peter Maydell).
> 
> Changes in v2:
>  - Expand commit message
>  - Checkpatch cleanups.
>  - Some interrupt bug fixes from Yann Le Du <address@hidden>
>  - Add get/set register functionality from Yann Le Du <address@hidden>
>  - Use new 64 bit compatible ABI from Cavium from Sanjay Lal
>    <address@hidden>
>  - Add dummy kvm_arch_init_irq_routing()
>    The common KVM code insists on calling kvm_arch_init_irq_routing() as
>    soon as it sees kernel header support for it (regardless of whether
>    QEMU supports it). Provide a dummy function to satisfy this.
>  - Remove request_interrupt_window code (Peter Maydell)
> ---
>  target-mips/kvm.c      | 472 
> +++++++++++++++++++++++++++++++++++++++++++++++++
>  target-mips/kvm_mips.h |  19 ++
>  2 files changed, 491 insertions(+)
>  create mode 100644 target-mips/kvm.c
>  create mode 100644 target-mips/kvm_mips.h
> 
> diff --git a/target-mips/kvm.c b/target-mips/kvm.c
> new file mode 100644
> index 0000000..0ec343d
> --- /dev/null
> +++ b/target-mips/kvm.c
[...]
> +static inline int kvm_mips_put_one_reg(CPUState *cs, int reg_id, int32 *addr)

Did you mean int32_t?

> +{
> +    __u64 val64 = (__u64)*addr;
> +    struct kvm_one_reg cp0reg = {
> +        .id = reg_id,
> +        .addr = (__u64)((target_ulong)&val64)
> +    };
> +
> +    return kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &cp0reg);
> +}
> +
> +static inline int kvm_mips_put_one_ulreg(CPUState *cs, int reg_id,
> +                                         target_ulong *addr)
> +{
> +    __u64 val64 = (__u64)*addr;
> +    struct kvm_one_reg cp0reg = {
> +        .id = reg_id,
> +        .addr = (__u64)((target_ulong)&val64)
> +    };
> +
> +    return kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &cp0reg);
> +}
> +
> +static inline int kvm_mips_get_one_reg(CPUState *cs, int reg_id, int32 *addr)

int32_t?

> +{
> +    int ret;
> +    __u64 val64 = 0;
> +    struct kvm_one_reg cp0reg = {
> +        .id = reg_id,
> +        .addr = (__u64)((target_ulong)&val64)
> +    };
> +
> +    ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &cp0reg);
> +    if (ret < 0) {
> +        return ret;
> +    }
> +
> +    *addr = (int32)val64;

int32_t?

> +    return ret;
> +}
[snip]

int32 is a type used in softfloat that has weird at-least-as-wide
semantics and bit us in the past.

I'm not sure if we have a policy about __u64 etc. in KVM code. Since
it'll be Linux-only I don't see problems currently; for cross-platform
parts we prefer uint64_t. Suggest to leave as is unless told otherwise.

Otherwise looking good now, thanks for the CPU cleanups! We just had
another round of CPU refactorings go in today, but I don't spot a
conflict in this patch. Please rebase your local branch to verify.

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg