Re: [Qemu-devel] [PATCH 3/6] Added Aarch64 CPU Initialization, Get and Put Registers Support.

[Mian M. Hamayun]

On 06/28/2013 02:43 PM, Alexander Graf wrote:
> On 28.06.2013, at 14:11, Mian M. Hamayun wrote:
>
> > 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
> This should be part of your header update patch.
Just to make sure that we understand it correctly, do you mean that this 
macro definition should be moved to the arm64 specific header that we 
include in another patch series ? or keep this change in the same file 
and do it along with the header inclusion in a single patch ?
> > +#ifdef TARGET_AARCH64
> > +int kvm_arch_init_vcpu(CPUState *cs)
> > +{
> > +    struct kvm_vcpu_init init;
> > +    int ret;
> > +
> > +    /* Try initializing with Foundation Models */
> > +    init.target = KVM_ARM_TARGET_FOUNDATION_V8;
> > +    memset(init.features, 0, sizeof(init.features));
> > +    ret = kvm_vcpu_ioctl(cs, KVM_ARM_VCPU_INIT, &init);
> > +    if (ret) {
> > +        /* Retry initializing with Fast Models */
> > +        init.target = KVM_ARM_TARGET_AEM_V8;
> > +        ret = kvm_vcpu_ioctl(cs, KVM_ARM_VCPU_INIT, &init);
> Not sure I understand this part. Do we have different CPU types for the 
> different models? If so, they'd be different -cpu parameters, with -cpu host 
> picking the same as the host's.
KVM tool does a very similar thing, using a loop and tries to initialize 
supported processors types and gets done when it succeeds with anyone of 
them. We could include a similar implementation in the next revision.
> > +{
> > +    struct kvm_one_reg reg;
> > +    int i;
> > +    int ret;
> > +
> > +    ARMCPU *cpu = ARM_CPU(cs);
> > +    CPUARMState *env = &cpu->env;
> > +
> > +    for (i = 0; i < 31; i++) {
> s/31/ARRAY_SIZE(...)/
Agreed, we should avoid hard-coding as much as possible.
> > +        reg.id = AARCH64_CORE_REG(regs.regs[i]);
> > +        reg.addr = (uintptr_t)(env) + offsetof(CPUARMState, xregs[i]);
> reg.addr = (uintptr_t)&env->xregs[i];
Our implementation was influenced by the existing 
kvm_arch_get_registers() implementation for the A15 CPU, with slight 
differences. Anyways, we will include the proposed change in the next 
revision.

--
Hamayun