Re: [Qemu-devel] [PATCH v2 3/7] AARCH64: Add aarch64 CPU initialization, get and put registers support

[Mian M. Hamayun]

On 09/08/2013 15:24, Peter Maydell wrote:
> On 23 July 2013 10:33, Mian M. Hamayun <address@hidden> wrote:
> > From: "Mian M. Hamayun" <address@hidden>
> >
> > The cpu init function tries to initialize with all possible cpu types, as
> > KVM does not provide a means to detect the real cpu type and simply refuses
> > to initialize on cpu type mis-match. By using the loop based init function,
> > we avoid the need to modify code if the underlying platform is different,
> > such as Fast Models instead of Foundation Models.
> >
> > Get and Put Registers deal with the basic state of Aarch64 CPUs for the moment.
> >
> > Signed-off-by: Mian M. Hamayun <address@hidden>
> >
> > Conflicts:
> >
> >          target-arm/kvm.c
> >
> > Conflicts:
> >
> >          target-arm/cpu.c
> This sort of Conflicts note shouldn't be in a commit message.
Agreed, will remove it in the next revision.
> > ---
> >   linux-headers/linux/kvm.h |    1 +
> >   target-arm/kvm.c          |  125 +++++++++++++++++++++++++++++++++++++++++++++
> >   2 files changed, 126 insertions(+)
> >
> > diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h
> > index c614070..4df5292 100644
> > --- a/linux-headers/linux/kvm.h
> > +++ b/linux-headers/linux/kvm.h
> > @@ -783,6 +783,7 @@ struct kvm_dirty_tlb {
> >   #define KVM_REG_IA64           0x3000000000000000ULL
> >   #define KVM_REG_ARM            0x4000000000000000ULL
> >   #define KVM_REG_S390           0x5000000000000000ULL
> > +#define KVM_REG_ARM64          0x6000000000000000ULL
> >
> >   #define KVM_REG_SIZE_SHIFT     52
> >   #define KVM_REG_SIZE_MASK      0x00f0000000000000ULL
> Updates to the linux-headers/ files need to:
>   * be a separate patch
>   * be the result of running scripts/update-linux-headers.sh
>     on an upstream mainline kernel or kvm-next kernel tree
>   * include the kernel tree/commit hash in the commit message
Agreed, will do it like this.
> > diff --git a/target-arm/kvm.c b/target-arm/kvm.c
> > index b92e00d..c96b871 100644
> > --- a/target-arm/kvm.c
> > +++ b/target-arm/kvm.c
> > @@ -32,6 +32,11 @@
> >   #error mismatch between cpu.h and KVM header definitions
> >   #endif
> >
> > +#ifdef TARGET_AARCH64
> > +#define AARCH64_CORE_REG(x)   (KVM_REG_ARM64 | KVM_REG_SIZE_U64 | \
> > +                 KVM_REG_ARM_CORE | KVM_REG_ARM_CORE_REG(x))
> > +#endif
> > +
> >   const KVMCapabilityInfo kvm_arch_required_capabilities[] = {
> >       KVM_CAP_LAST_INFO
> >   };
> > @@ -50,6 +55,33 @@ unsigned long kvm_arch_vcpu_id(CPUState *cpu)
> >       return cpu->cpu_index;
> >   }
> >
> > +#ifdef TARGET_AARCH64
> This set of #ifdefs is pretty messy. I suggest splitting kvm.c
> into three:
>    target-arm/kvm.c   -- anything common to both 32 and 64 bit
>    target-arm/kvm32.c -- non-TARGET_AARCH64 functions
>    target-arm/kvm64.c -- TARGET_AARCH64 functions
>
> and have target-arm/Makefile.objs build only one of kvm32.c
> or kvm64.c depending on whether TARGET_AARCH64 is set.
My initial intuition was to separate 32 and 64 bit versions, but as
many functions are common, so I decided to use #ifdefs instead.
btw, I have a similar situation in hw/arm/boot.c as well.
> > +    /* Find an appropriate target CPU type.
> > +     * KVM does not provide means to detect the host CPU type on aarch64,
> > +     * and simply refuses to initialize, if the CPU type mis-matches;
> > +     * so we try each possible CPU type on aarch64 before giving up! */
> > +    for (i = 0; i < KVM_ARM_NUM_TARGETS; ++i) {
> > +        init.target = kvm_arm_targets[i];
> > +        ret = kvm_vcpu_ioctl(cs, KVM_ARM_VCPU_INIT, &init);
> > +        if (!ret)
> > +            break;
> > +    }
> We definitely don't want to do this -- see my notes on
> '-cpu host' in another email thread.  (We should make sure
> we coordinate who of you or Linaro is going to do the
> -cpu host support, incidentally.)
I tend to agree with this proposition as well. But the point that I don't
understand is how something is acceptable in kvmtool and not in the
qemu. If this implementation lets us use the 64-bit architecture in the
current state of affairs, then why not use it. We can always replace
this particular part, when the -cpu host support becomes available, right ?

Hamayun