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

[Peter Maydell]

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.

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.

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

> 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.

> +            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.

>          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)?.

> +
>      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.

> +        }
> +    }
> +
>      /* 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.

thanks
-- PMM