Re: [Qemu-devel] [PATCH] target-arm: ARM64: Adding EL1 AARCH32 guest support for KVM.

[Pranavkumar Sawargaonkar]

Hi PMM,

On 2 December 2014 at 21:29, Peter Maydell <address@hidden> wrote:
> On 28 November 2014 at 13:06, Pranavkumar Sawargaonkar
> <address@hidden> wrote:
>> In KVM ARM64 one can choose to run guest in 32bit mode i.e EL1 in AARCH32 
>> mode.
>> This patch adds qemu support for running guest EL1 in AARCH32 mode with
>> virt as a machine model.
>
> Thanks for sending this patch.
>
>> This patch also adds a support to run Image (along with zImage) for arm32.
>
Thanks for reviewing this patch.

> I'm a bit confused by this -- we already support running Images
> and zImages on 32 bit. We shouldn't need any extra "is this a zImage"
> detection code to handle this, I don't think.

Yes so I have tried booting Image with arm-soffmmu in emulation mode
but it does not boot but zImage was booting. Then realized that it is
not putting Image address aligned with 0x8000 as zImage does it during
compression.
Hence for Image booting I have added a code to put that at 0x8000
aligned address.

>
> In any case, if we do need something extra here it should probably
> be in its own patch.

Sure so I will create a separate patch for this.

>
>> One can specify about 32bit kernel Image by using -cpu host,el1_aarch32 
>> argument.
>>
>> e.g.
>> "./qemu/aarch64-softmmu/qemu-system-aarch64  -nographic -display none \
>>  -serial stdio -kernel ./Image  -m 512 -M virt -cpu host,el1_aarch32 \
>>  -initrd rootfs.img  -append "console=ttyAMA0 root=/dev/ram" -enable-kvm"
>>
>> Signed-off-by: Pranavkumar Sawargaonkar <address@hidden>
>> ---
>>  hw/arm/boot.c      | 44 ++++++++++++++++++++++++++++----
>>  hw/arm/virt.c      | 30 +++++++++++++++++++++-
>>  target-arm/cpu.c   |  5 ++--
>>  target-arm/cpu.h   |  2 ++
>>  target-arm/kvm64.c | 73 
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  5 files changed, 146 insertions(+), 8 deletions(-)
>>
>> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
>> index 0014c34..da8cdc8 100644
>> --- a/hw/arm/boot.c
>> +++ b/hw/arm/boot.c
>> @@ -476,6 +476,32 @@ static void do_cpu_reset(void *opaque)
>>      }
>>  }
>>
>> +static int check_load_zimage(const char *filename)
>> +{
>> +    int fd;
>> +    uint8_t buf[40];
>> +    uint32_t *p = (uint32_t *) &buf[36];
>> +
>> +    fd = open(filename, O_RDONLY | O_BINARY);
>> +    if (fd < 0) {
>> +        perror(filename);
>> +        return -1;
>> +    }
>> +
>> +    memset(buf, 0, sizeof(buf));
>> +    if (read(fd, buf, sizeof(buf)) != sizeof(buf)) {
>> +        close(fd);
>> +        return -1;
>> +    }
>> +
>> +    /* Check for zImage magic number */
>> +    if (*p == 0x016F2818) {
>> +        return 1;
>> +    }
>> +
>> +   return 0;
>> +}
>> +
>>  void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
>>  {
>>      CPUState *cs;
>> @@ -515,15 +541,23 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info 
>> *info)
>>          return;
>>      }
>>
>> -    if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64)) {
>> +    if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64) &&
>> +                    (!cpu->env.el1_aarch32)) {
>>          primary_loader = bootloader_aarch64;
>>          kernel_load_offset = KERNEL64_LOAD_ADDR;
>>          elf_machine = EM_AARCH64;
>>      } else {
>> -        primary_loader = bootloader;
>> -        kernel_load_offset = KERNEL_LOAD_ADDR;
>> -        elf_machine = EM_ARM;
>> -    }
>> +        if (check_load_zimage(info->kernel_filename)) {
>> +            primary_loader = bootloader;
>> +            kernel_load_offset = KERNEL_LOAD_ADDR;
>> +            elf_machine = EM_ARM;
>> +        } else {
>> +            primary_loader = bootloader;
>> +            /* Assuming we are loading Image hence aligning it to 0x8000 */
>> +            kernel_load_offset = KERNEL_LOAD_ADDR - 0x8000;
>> +            elf_machine = EM_ARM;
>> +        }
>> +   }
>>
>>      info->dtb_filename = qemu_opt_get(qemu_get_machine_opts(), "dtb");
>>
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index 314e55b..64213e6 100644
>> --- a/hw/arm/virt.c
>> +++ b/hw/arm/virt.c
>> @@ -204,7 +204,8 @@ static void fdt_add_psci_node(const VirtBoardInfo *vbi)
>>          qemu_fdt_setprop(fdt, "/psci", "compatible", comp, sizeof(comp));
>>
>>          cpu_off_fn = QEMU_PSCI_0_2_FN_CPU_OFF;
>> -        if (arm_feature(&armcpu->env, ARM_FEATURE_AARCH64)) {
>> +        if (arm_feature(&armcpu->env, ARM_FEATURE_AARCH64) &&
>> +                                    (!armcpu->env.el1_aarch32)) {
>>              cpu_suspend_fn = QEMU_PSCI_0_2_FN64_CPU_SUSPEND;
>>              cpu_on_fn = QEMU_PSCI_0_2_FN64_CPU_ON;
>>              migrate_fn = QEMU_PSCI_0_2_FN64_MIGRATE;
>> @@ -527,6 +528,24 @@ static void *machvirt_dtb(const struct arm_boot_info 
>> *binfo, int *fdt_size)
>>      return board->fdt;
>>  }
>>
>> +#if defined(TARGET_AARCH64) && !defined(CONFIG_USER_ONLY)
>> +static void check_special_cpu_model_flags(const char *cpu_model,
>> +                                          Object *cpuobj)
>> +{
>> +        ARMCPU *cpu = ARM_CPU(cpuobj);
>> +
>> +        if (!cpu) {
>> +            return;
>> +        }
>> +
>> +        if (strcmp(cpu_model, "host,el1_aarch32") == 0) {
>
> This looks wrong -- we should support the "32 bit EL1" flag
> for all 64 bit CPU types, not just "host". It also should
> not be in the virt board model, but in target-arm/ code
> somewhere.

Yes but I could not find correct way do read command line value passed
for cpu_model there to determine this.
Hence I have kept this option with virt for now.

>
>> +            cpu->env.el1_aarch32 = 1;
>> +        } else {
>> +            cpu->env.el1_aarch32 = 0;
>> +        }
>> +}
>> +#endif
>> +
>>  static void machvirt_init(MachineState *machine)
>>  {
>>      qemu_irq pic[NUM_IRQS];
>> @@ -540,6 +559,12 @@ static void machvirt_init(MachineState *machine)
>>          cpu_model = "cortex-a15";
>>      }
>>
>> +#if defined(TARGET_AARCH64) && !defined(CONFIG_USER_ONLY)
>> +    if (strcmp(cpu_model, "host,el1_aarch32") == 0) {
>> +        cpu_model = "host";
>> +    }
>> +#endif
>> +
>>      vbi = find_machine_info(cpu_model);
>>
>>      if (!vbi) {
>> @@ -578,6 +603,9 @@ static void machvirt_init(MachineState *machine)
>>              object_property_set_int(cpuobj, 
>> vbi->memmap[VIRT_CPUPERIPHS].base,
>>                                      "reset-cbar", &error_abort);
>>          }
>> +#if defined(TARGET_AARCH64) && !defined(CONFIG_USER_ONLY)
>> +        check_special_cpu_model_flags(machine->cpu_model, cpuobj);
>> +#endif
>>
>>          object_property_set_bool(cpuobj, true, "realized", NULL);
>>      }
>> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
>> index 5ce7350..37dfe30 100644
>> --- a/target-arm/cpu.c
>> +++ b/target-arm/cpu.c
>> @@ -103,7 +103,7 @@ static void arm_cpu_reset(CPUState *s)
>>          env->iwmmxt.cregs[ARM_IWMMXT_wCID] = 0x69051000 | 'Q';
>>      }
>>
>> -    if (arm_feature(env, ARM_FEATURE_AARCH64)) {
>> +    if (arm_feature(env, ARM_FEATURE_AARCH64) && (!env->el1_aarch32)) {
>>          /* 64 bit CPUs always start in 64 bit mode */
>>          env->aarch64 = 1;
>>  #if defined(CONFIG_USER_ONLY)
>> @@ -394,7 +394,8 @@ static void arm_cpu_post_init(Object *obj)
>>                                   &error_abort);
>>      }
>>
>> -    if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64)) {
>> +    if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64) &&
>> +                                    (!cpu->env.el1_aarch32)) {
>
> Given how often this conditional turns up that would suggest it
> should be encapsulated in a suitable helper function.

Sure I will add this.

>
>>          qdev_property_add_static(DEVICE(obj), &arm_cpu_rvbar_property,
>>                                   &error_abort);
>>      }
>> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
>> index 7f80090..d2f8eda 100644
>> --- a/target-arm/cpu.h
>> +++ b/target-arm/cpu.h
>> @@ -343,6 +343,8 @@ typedef struct CPUARMState {
>>      /* Internal CPU feature flags.  */
>>      uint64_t features;
>>
>> +    uint32_t el1_aarch32; /* 1 If CPU is in aarch64 state and el1 is 
>> aarch32 */
>
> I need to think about how this should fit in with the support
> for proper emulated EL2/EL3 (should we have "highest EL is 32 bit"
> rather than specifically "EL1 is 32 bit"? should it be "boot
> the kernel in 32 bit at whatever is the highest EL we can boot
> the kernel in" (which might mean 32-bit EL2)?.

Yeah this is interesting when we have EL3.

>
>> +
>>      void *nvic;
>>      const struct arm_boot_info *boot_info;
>>  } CPUARMState;
>> diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c
>> index c615286..03e6dcc 100644
>> --- a/target-arm/kvm64.c
>> +++ b/target-arm/kvm64.c
>> @@ -97,6 +97,15 @@ int kvm_arch_init_vcpu(CPUState *cs)
>>          cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_PSCI_0_2;
>>      }
>>
>> +    if ((cpu->env.el1_aarch32)) {
>> +        if ((kvm_check_extension(cs->kvm_state, KVM_CAP_ARM_EL1_32BIT))) {
>> +            cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_EL1_32BIT;
>> +        } else {
>> +            fprintf(stderr, "KVM is not supporting EL1 AARCH32 mode\n");
>
> "KVM does not support booting this guest CPU in AArch32 state"
>
>> +            return -1;
>
> This function returns negative-errnos; compare the error-exit code
> already present.

I will fix this in V2.

>
>> +        }
>> +    }
>> +
>>      /* Do KVM_ARM_VCPU_INIT ioctl */
>>      ret = kvm_arm_vcpu_init(cs);
>>      if (ret) {
>> @@ -111,6 +120,35 @@ int kvm_arch_init_vcpu(CPUState *cs)
>>  #define AARCH64_CORE_REG(x)   (KVM_REG_ARM64 | KVM_REG_SIZE_U64 | \
>>                   KVM_REG_ARM_CORE | KVM_REG_ARM_CORE_REG(x))
>>
>> +
>> +static int kvm_arch32_put_registers(CPUState *cs, int level)
>> +{
>> +    ARMCPU *cpu = ARM_CPU(cs);
>> +    CPUARMState *env = &cpu->env;
>> +    struct kvm_one_reg reg;
>> +    int ret;
>> +    int i;
>> +
>> +    for (i = 0; i < 16; i++) {
>> +        reg.id = AARCH64_CORE_REG(regs.regs[i]);
>> +        reg.addr = (uintptr_t) &env->regs[i];
>> +        ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
>> +        if (ret) {
>> +            return ret;
>> +        }
>> +    }
>> +
>> +    /* PC will be set in env->regs[15] */
>> +    reg.addr = (uintptr_t) &env->regs[15];
>> +    reg.id = AARCH64_CORE_REG(regs.pc);
>> +    ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
>> +    if (ret) {
>> +        return ret;
>> +    }
>
> This doesn't look right. The design of the 64 bit virtualization
> extensions is such that the hypervisor shouldn't need to care
> about whether the guest is in 32 bit or 64 bit mode. It would
> be better to use the existing 64-bit get/put code, and just have
> a small bit of code that says "if we're in 32 bit mode then
> sync the 32-bit state to/from the 64-bit state". This is
> going to be pretty similar to the code TCG needs for exception
> entry/return.

Ok then do I need to put the value in env->xreg only by adjusting pc
value which gets set in env->regs[15] ?

>
> thanks
> -- PMM
>
Thanks,
Pranav