Re: [Qemu-devel] [PATCH 4/5] target-arm: Add AArch32 guest support to KVM64

[Greg Bellows]

On Tue, Jan 20, 2015 at 10:57 AM, Alex Bennée <address@hidden> wrote:
>
> Greg Bellows <address@hidden> writes:
>
>> Add 32-bit to/from 64-bit register synchronization on register gets and puts.
>> Set EL1_32BIT feature flag passed to KVM
>>
>> Signed-off-by: Greg Bellows <address@hidden>
>> ---
>>  target-arm/kvm64.c | 21 +++++++++++++++++----
>>  1 file changed, 17 insertions(+), 4 deletions(-)
>>
>> diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c
>> index ba16821..0061099 100644
>> --- a/target-arm/kvm64.c
>> +++ b/target-arm/kvm64.c
>> @@ -81,8 +81,7 @@ int kvm_arch_init_vcpu(CPUState *cs)
>>      int ret;
>>      ARMCPU *cpu = ARM_CPU(cs);
>>
>> -    if (cpu->kvm_target == QEMU_KVM_ARM_TARGET_NONE ||
>> -        !arm_feature(&cpu->env, ARM_FEATURE_AARCH64)) {
>> +    if (cpu->kvm_target == QEMU_KVM_ARM_TARGET_NONE) {
>>          fprintf(stderr, "KVM is not supported for this guest CPU type\n");
>>          return -EINVAL;
>>      }
>> @@ -96,6 +95,9 @@ int kvm_arch_init_vcpu(CPUState *cs)
>>          cpu->psci_version = 2;
>>          cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_PSCI_0_2;
>>      }
>> +    if (!arm_feature(&cpu->env, ARM_FEATURE_AARCH64)) {
>> +        cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_EL1_32BIT;
>> +    }
>>
>>      /* Do KVM_ARM_VCPU_INIT ioctl */
>>      ret = kvm_arm_vcpu_init(cs);
>> @@ -133,6 +135,7 @@ int kvm_arch_put_registers(CPUState *cs, int level)
>>      ARMCPU *cpu = ARM_CPU(cs);
>>      CPUARMState *env = &cpu->env;
>>
>> +    aarch64_sync_32_to_64(env);
>>      for (i = 0; i < 31; i++) {
>>          reg.id = AARCH64_CORE_REG(regs.regs[i]);
>>          reg.addr = (uintptr_t) &env->xregs[i];
>> @@ -162,7 +165,11 @@ int kvm_arch_put_registers(CPUState *cs, int level)
>>      }
>>
>>      /* Note that KVM thinks pstate is 64 bit but we use a uint32_t */
>> -    val = pstate_read(env);
>> +    if (is_a64(env)) {
>> +        val = pstate_read(env);
>> +    } else {
>> +        val = cpsr_read(env);
>> +    }
>
> I know why we do this (especially given where my attempt ended up) but
> perhaps we could at list have a single state aware accessor so we don't
> end up duplicating this test all over the place?

I'd happily add an accessor function, but I only found 1 other
location that does this conditional so I'm not sure it is warranted.

>
> --
> Alex Bennée