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

["Mian M. Hamayun" <m.hamayun@virtualopensystems.com>]

On 09/08/2013 15:24, Peter Maydell wrote:
> On 23 July 2013 10:33, Mian M. Hamayun <m.hamayun@virtualopensystems.com> wrote:
>> From: "Mian M. Hamayun" <m.hamayun@virtualopensystems.com>
>>
>> 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 <m.hamayun@virtualopensystems.com>
>>
>> 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