[Qemu-devel] [PATCH v2 0/4] target-arm: ARM64: Adding EL1 AARCH32 guest support

[Qemu-devel] [PATCH v2 0/4] target-arm: ARM64: Adding EL1 AARCH32 guest support

[Greg Bellows]

Added support for running an AArch32 guest on a AArch64 KVM host.  Support has
only been added to the QEMU machvirt machine.  The addition of CPU properties
specifiable from the command line were added to allow disablement of AArch64
execution state hereby forcing EL1 to be AArch32.  The new CPU command line
property is "aarch64=on/off" that is specified as follows:

    aarch64-softmmu/qemu-system-aarch64 -M virt -cpu cortex-a57,aarch64=off ...

---

v1 -> v2
- Replaced custom property parsing with use of generic CPU property parser
- Added CPU property registration
- Fixed mulitple property handling in virt.c
- Removed unnecessary kernel load changes

Greg Bellows (4):
  target-arm: Add CPU property to disable AArch64
  target-arm: Add feature parsing to virt
  target-arm: Add 32/64-bit register sync
  target-arm: Add AArch32 guest support to KVM64

 hw/arm/virt.c           | 20 +++++++++--
 target-arm/cpu.c        |  6 +++-
 target-arm/cpu64.c      | 29 ++++++++++++++++
 target-arm/helper-a64.c |  5 +--
 target-arm/internals.h  | 89 +++++++++++++++++++++++++++++++++++++++++++++++++
 target-arm/kvm64.c      | 21 +++++++++---
 target-arm/op_helper.c  |  6 ++--
 7 files changed, 160 insertions(+), 16 deletions(-)

--
1.8.3.2

[Qemu-devel] [PATCH v2 1/4] target-arm: Add CPU property to disable AArch64

[Greg Bellows]

Adds registration and get/set functions for enabling/disabling the AArch64
execution state on AArch64 CPUs.  By default AArch64 execution state is enabled
on AArch64 CPUs, setting the property to off, will disable the execution state.
The below QEMU invocation would have AArch64 execution state disabled.

    $ ./qemu-system-aarch64 -machine virt -cpu cortex-a57,aarch64=off

Also adds stripping of features from CPU model string in acquiring the ARM CPU
by name.

Signed-off-by: Greg Bellows <greg.bellows@linaro.org>

---

v1 -> v2
- Scrap the custom CPU feature parsing in favor of using the default CPU
  parsing.
- Add registration of CPU AArch64 property to disable/enable the AArch64
  feature.
---
 target-arm/cpu.c   |  6 +++++-
 target-arm/cpu64.c | 29 +++++++++++++++++++++++++++++
 2 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/target-arm/cpu.c b/target-arm/cpu.c
index 285947f..29ed691 100644
--- a/target-arm/cpu.c
+++ b/target-arm/cpu.c
@@ -514,13 +514,17 @@ static ObjectClass *arm_cpu_class_by_name(const char *cpu_model)
 {
     ObjectClass *oc;
     char *typename;
+    char *cpuname;
 
     if (!cpu_model) {
         return NULL;
     }
 
-    typename = g_strdup_printf("%s-" TYPE_ARM_CPU, cpu_model);
+    cpuname = g_strdup(cpu_model);
+    cpuname = strtok(cpuname, ",");
+    typename = g_strdup_printf("%s-" TYPE_ARM_CPU, cpuname);
     oc = object_class_by_name(typename);
+    g_free(cpuname);
     g_free(typename);
     if (!oc || !object_class_dynamic_cast(oc, TYPE_ARM_CPU) ||
         object_class_is_abstract(oc)) {
diff --git a/target-arm/cpu64.c b/target-arm/cpu64.c
index bb778b3..5a59280 100644
--- a/target-arm/cpu64.c
+++ b/target-arm/cpu64.c
@@ -32,6 +32,11 @@ static inline void set_feature(CPUARMState *env, int feature)
     env->features |= 1ULL << feature;
 }
 
+static inline void unset_feature(CPUARMState *env, int feature)
+{
+    env->features &= ~(1ULL << feature);
+}
+
 #ifndef CONFIG_USER_ONLY
 static uint64_t a57_l2ctlr_read(CPUARMState *env, const ARMCPRegInfo *ri)
 {
@@ -170,8 +175,32 @@ static const ARMCPUInfo aarch64_cpus[] = {
     { .name = NULL }
 };
 
+static bool aarch64_cpu_get_aarch64(Object *obj, Error **errp)
+{
+    ARMCPU *cpu = ARM_CPU(obj);
+
+    return arm_feature(&cpu->env, ARM_FEATURE_AARCH64);
+}
+
+static void aarch64_cpu_set_aarch64(Object *obj, bool value, Error **errp)
+{
+    ARMCPU *cpu = ARM_CPU(obj);
+
+    if (value == false) {
+        unset_feature(&cpu->env, ARM_FEATURE_AARCH64);
+    } else {
+        set_feature(&cpu->env, ARM_FEATURE_AARCH64);
+    }
+}
+
 static void aarch64_cpu_initfn(Object *obj)
 {
+    object_property_add_bool(obj, "aarch64", aarch64_cpu_get_aarch64,
+                             aarch64_cpu_set_aarch64, NULL);
+    object_property_set_description(obj, "aarch64",
+                                    "Set on/off to enable/disable aarch64 "
+                                    "execution state ",
+                                    NULL);
 }
 
 static void aarch64_cpu_finalizefn(Object *obj)
-- 
1.8.3.2

[Qemu-devel] [PATCH v2 2/4] target-arm: Add feature parsing to virt

[Greg Bellows]

Added machvirt parsing of feature keywords added to the -cpu command line
option.  Parsing occurs during machine initialization.

Signed-off-by: Greg Bellows <greg.bellows@linaro.org>

---

v1 -> v2
- Fix multiple property handling
---
 hw/arm/virt.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 2353440..9b34e94 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -573,15 +573,19 @@ static void machvirt_init(MachineState *machine)
     MemoryRegion *ram = g_new(MemoryRegion, 1);
     const char *cpu_model = machine->cpu_model;
     VirtBoardInfo *vbi;
+    char **cpustr;
 
     if (!cpu_model) {
         cpu_model = "cortex-a15";
     }
 
-    vbi = find_machine_info(cpu_model);
+    /* Separate the actual CPU model name from any appended features */
+    cpustr = g_strsplit(cpu_model, ",", 2);
+
+    vbi = find_machine_info(cpustr[0]);
 
     if (!vbi) {
-        error_report("mach-virt: CPU %s not supported", cpu_model);
+        error_report("mach-virt: CPU %s not supported", cpustr[0]);
         exit(1);
     }
 
@@ -595,8 +599,10 @@ static void machvirt_init(MachineState *machine)
     create_fdt(vbi);
 
     for (n = 0; n < smp_cpus; n++) {
-        ObjectClass *oc = cpu_class_by_name(TYPE_ARM_CPU, cpu_model);
+        ObjectClass *oc = cpu_class_by_name(TYPE_ARM_CPU, cpustr[0]);
+        CPUClass *cc = CPU_CLASS(oc);
         Object *cpuobj;
+        Error *err = NULL;
 
         if (!oc) {
             fprintf(stderr, "Unable to find CPU definition\n");
@@ -604,6 +610,13 @@ static void machvirt_init(MachineState *machine)
         }
         cpuobj = object_new(object_class_get_name(oc));
 
+        /* Handle any CPU options specfied by the user */
+        cc->parse_features(CPU(cpuobj), cpustr[1], &err);
+        if (err) {
+            error_report("%s", error_get_pretty(err));
+            exit(1);
+        }
+
         if (!vms->secure) {
             object_property_set_bool(cpuobj, false, "has_el3", NULL);
         }
@@ -623,6 +636,7 @@ static void machvirt_init(MachineState *machine)
 
         object_property_set_bool(cpuobj, true, "realized", NULL);
     }
+    g_strfreev(cpustr);
     fdt_add_timer_nodes(vbi);
     fdt_add_cpu_nodes(vbi);
     fdt_add_psci_node(vbi);
-- 
1.8.3.2

[Qemu-devel] [PATCH v2 3/4] target-arm: Add 32/64-bit register sync

[Greg Bellows]

Add AArch32 to AArch64 register sychronization functions.
Replace manual register synchronization with new functions in
aarch64_cpu_do_interrupt() and HELPER(exception_return)().

Signed-off-by: Greg Bellows <greg.bellows@linaro.org>
---
 target-arm/helper-a64.c |  5 +--
 target-arm/internals.h  | 89 +++++++++++++++++++++++++++++++++++++++++++++++++
 target-arm/op_helper.c  |  6 ++--
 3 files changed, 92 insertions(+), 8 deletions(-)

diff --git a/target-arm/helper-a64.c b/target-arm/helper-a64.c
index 81066ca..2b5a668 100644
--- a/target-arm/helper-a64.c
+++ b/target-arm/helper-a64.c
@@ -448,7 +448,6 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
     unsigned int new_el = arm_excp_target_el(cs, cs->exception_index);
     target_ulong addr = env->cp15.vbar_el[new_el];
     unsigned int new_mode = aarch64_pstate_mode(new_el, true);
-    int i;
 
     if (arm_current_el(env) < new_el) {
         if (env->aarch64) {
@@ -512,9 +511,7 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
         }
         env->elr_el[new_el] = env->regs[15];
 
-        for (i = 0; i < 15; i++) {
-            env->xregs[i] = env->regs[i];
-        }
+        aarch64_sync_32_to_64(env);
 
         env->condexec_bits = 0;
     }
diff --git a/target-arm/internals.h b/target-arm/internals.h
index bb171a7..626ea7d 100644
--- a/target-arm/internals.h
+++ b/target-arm/internals.h
@@ -128,6 +128,95 @@ static inline void aarch64_restore_sp(CPUARMState *env, int el)
     }
 }
 
+static inline void aarch64_sync_32_to_64(CPUARMState *env)
+{
+    int i;
+
+    /* We can blanket copy R[0:7] to X[0:7] */
+    for (i = 0; i < 8; i++) {
+        env->xregs[i] = env->regs[i];
+    }
+
+    /* If we are in USR mode then we just want to complete the above blanket
+     * copy so we get the accurate register values.  If not, then we have to go
+     * to the saved and banked user regs.
+     */
+    if ((env->uncached_cpsr & 0x1f) == ARM_CPU_MODE_USR) {
+        for (i = 8; i < 15; i++) {
+            env->xregs[i] = env->regs[i];
+        }
+    } else {
+        for (i = 8; i < 13; i++) {
+            env->xregs[i] = env->usr_regs[i-8];
+        }
+        env->xregs[13] = env->banked_r13[bank_number(ARM_CPU_MODE_USR)];
+        env->xregs[14] = env->banked_r14[bank_number(ARM_CPU_MODE_USR)];
+    }
+    env->pc = env->regs[15];
+
+    env->xregs[15] = env->banked_r13[bank_number(ARM_CPU_MODE_HYP)];
+    env->xregs[16] = env->banked_r13[bank_number(ARM_CPU_MODE_IRQ)];
+    env->xregs[17] = env->banked_r14[bank_number(ARM_CPU_MODE_IRQ)];
+    env->xregs[18] = env->banked_r13[bank_number(ARM_CPU_MODE_SVC)];
+    env->xregs[19] = env->banked_r14[bank_number(ARM_CPU_MODE_SVC)];
+    env->xregs[20] = env->banked_r13[bank_number(ARM_CPU_MODE_ABT)];
+    env->xregs[21] = env->banked_r14[bank_number(ARM_CPU_MODE_ABT)];
+    env->xregs[22] = env->banked_r13[bank_number(ARM_CPU_MODE_UND)];
+    env->xregs[23] = env->banked_r14[bank_number(ARM_CPU_MODE_UND)];
+
+    for (i = 0; i < 5; i++) {
+        env->xregs[24+i] = env->fiq_regs[i];
+    }
+    env->xregs[29] = env->banked_r13[bank_number(ARM_CPU_MODE_FIQ)];
+    env->xregs[30] = env->banked_r14[bank_number(ARM_CPU_MODE_FIQ)];
+}
+
+static inline void aarch64_sync_64_to_32(CPUARMState *env)
+{
+    int i;
+
+    /* We can blanket copy R[0:7] to X[0:7] */
+    for (i = 0; i < 8; i++) {
+        env->regs[i] = env->xregs[i];
+    }
+
+    /* If we are in USR mode then we want to complete the above blanket
+     * copy as the XREGs will contain the most recent value.
+     */
+    if ((env->uncached_cpsr & 0x1f) == ARM_CPU_MODE_USR) {
+        for (i = 8; i < 15; i++) {
+            env->regs[i] = env->xregs[i];
+        }
+    }
+
+    /* Update the user copies and banked registers so they are also up to
+     * date.
+     */
+    for (i = 8; i < 13; i++) {
+        env->usr_regs[i-8] = env->xregs[i];
+    }
+    env->banked_r13[bank_number(ARM_CPU_MODE_USR)] = env->xregs[13];
+    env->banked_r14[bank_number(ARM_CPU_MODE_USR)] = env->xregs[14];
+
+    env->regs[15] = env->pc;
+
+    env->banked_r13[bank_number(ARM_CPU_MODE_HYP)] = env->xregs[15];
+    env->banked_r13[bank_number(ARM_CPU_MODE_IRQ)] = env->xregs[16];
+    env->banked_r14[bank_number(ARM_CPU_MODE_IRQ)] = env->xregs[17];
+    env->banked_r13[bank_number(ARM_CPU_MODE_SVC)] = env->xregs[18];
+    env->banked_r14[bank_number(ARM_CPU_MODE_SVC)] = env->xregs[19];
+    env->banked_r13[bank_number(ARM_CPU_MODE_ABT)] = env->xregs[20];
+    env->banked_r14[bank_number(ARM_CPU_MODE_ABT)] = env->xregs[21];
+    env->banked_r13[bank_number(ARM_CPU_MODE_UND)] = env->xregs[22];
+    env->banked_r14[bank_number(ARM_CPU_MODE_UND)] = env->xregs[23];
+
+    for (i = 0; i < 5; i++) {
+        env->fiq_regs[i] = env->xregs[24+i];
+    }
+    env->banked_r13[bank_number(ARM_CPU_MODE_FIQ)] = env->xregs[29];
+    env->banked_r14[bank_number(ARM_CPU_MODE_FIQ)] = env->xregs[30];
+}
+
 static inline void update_spsel(CPUARMState *env, uint32_t imm)
 {
     unsigned int cur_el = arm_current_el(env);
diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
index 2bed914..7713022 100644
--- a/target-arm/op_helper.c
+++ b/target-arm/op_helper.c
@@ -465,7 +465,7 @@ void HELPER(exception_return)(CPUARMState *env)
     int cur_el = arm_current_el(env);
     unsigned int spsr_idx = aarch64_banked_spsr_index(cur_el);
     uint32_t spsr = env->banked_spsr[spsr_idx];
-    int new_el, i;
+    int new_el;
 
     aarch64_save_sp(env, cur_el);
 
@@ -491,9 +491,7 @@ void HELPER(exception_return)(CPUARMState *env)
         if (!arm_singlestep_active(env)) {
             env->uncached_cpsr &= ~PSTATE_SS;
         }
-        for (i = 0; i < 15; i++) {
-            env->regs[i] = env->xregs[i];
-        }
+        aarch64_sync_64_to_32(env);
 
         env->regs[15] = env->elr_el[1] & ~0x1;
     } else {
-- 
1.8.3.2

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

[Greg Bellows]

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 <greg.bellows@linaro.org>
---
 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);
+    }
     reg.id = AARCH64_CORE_REG(regs.pstate);
     reg.addr = (uintptr_t) &val;
     ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
@@ -218,6 +225,7 @@ int kvm_arch_get_registers(CPUState *cs)
             return ret;
         }
     }
+    aarch64_sync_64_to_32(env);
 
     reg.id = AARCH64_CORE_REG(regs.sp);
     reg.addr = (uintptr_t) &env->sp_el[0];
@@ -239,7 +247,12 @@ int kvm_arch_get_registers(CPUState *cs)
     if (ret) {
         return ret;
     }
-    pstate_write(env, val);
+    if (is_a64(env)) {
+        pstate_write(env, val);
+    } else {
+        env->uncached_cpsr = val & CPSR_M;
+        cpsr_write(env, val, 0xffffffff);
+    }
 
     /* KVM puts SP_EL0 in regs.sp and SP_EL1 in regs.sp_el1. On the
      * QEMU side we keep the current SP in xregs[31] as well.
-- 
1.8.3.2

Re: [Qemu-devel] [PATCH v2 0/4] target-arm: ARM64: Adding EL1 AARCH32 guest support

[Christoffer Dall]

On Wed, Jan 21, 2015 at 12:49:49PM -0600, Greg Bellows wrote:
> Added support for running an AArch32 guest on a AArch64 KVM host.  Support has
> only been added to the QEMU machvirt machine.  The addition of CPU properties
> specifiable from the command line were added to allow disablement of AArch64
> execution state hereby forcing EL1 to be AArch32.  The new CPU command line
> property is "aarch64=on/off" that is specified as follows:
> 
>     aarch64-softmmu/qemu-system-aarch64 -M virt -cpu cortex-a57,aarch64=off ...
> 

This patch set seems to break standard aarch64 KVM guest functionality,
I never see my guest progressing....

-Christoffer

Re: [Qemu-devel] [PATCH v2 0/4] target-arm: ARM64: Adding EL1 AARCH32 guest support

[Greg Bellows]

[-- Attachment #1: Type: text/plain, Size: 893 bytes --]

On Mon, Jan 26, 2015 at 5:03 AM, Christoffer Dall <
christoffer.dall@linaro.org> wrote:

> On Wed, Jan 21, 2015 at 12:49:49PM -0600, Greg Bellows wrote:
> > Added support for running an AArch32 guest on a AArch64 KVM host.
> Support has
> > only been added to the QEMU machvirt machine.  The addition of CPU
> properties
> > specifiable from the command line were added to allow disablement of
> AArch64
> > execution state hereby forcing EL1 to be AArch32.  The new CPU command
> line
> > property is "aarch64=on/off" that is specified as follows:
> >
> >     aarch64-softmmu/qemu-system-aarch64 -M virt -cpu
> cortex-a57,aarch64=off ...
> >
>
> This patch set seems to break standard aarch64 KVM guest functionality,
> I never see my guest progressing....
>
> -Christoffer
>

​Hmm... sounds like something I need to debug.  I'll work with you on the
environment.

[-- Attachment #2: Type: text/html, Size: 1524 bytes --]

Re: [Qemu-devel] [PATCH v2 3/4] target-arm: Add 32/64-bit register sync

[Christoffer Dall]

On Wed, Jan 21, 2015 at 12:49:52PM -0600, Greg Bellows wrote:
> Add AArch32 to AArch64 register sychronization functions.
> Replace manual register synchronization with new functions in
> aarch64_cpu_do_interrupt() and HELPER(exception_return)().
> 
> Signed-off-by: Greg Bellows <greg.bellows@linaro.org>
> ---
>  target-arm/helper-a64.c |  5 +--
>  target-arm/internals.h  | 89 +++++++++++++++++++++++++++++++++++++++++++++++++
>  target-arm/op_helper.c  |  6 ++--
>  3 files changed, 92 insertions(+), 8 deletions(-)
> 
> diff --git a/target-arm/helper-a64.c b/target-arm/helper-a64.c
> index 81066ca..2b5a668 100644
> --- a/target-arm/helper-a64.c
> +++ b/target-arm/helper-a64.c
> @@ -448,7 +448,6 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
>      unsigned int new_el = arm_excp_target_el(cs, cs->exception_index);
>      target_ulong addr = env->cp15.vbar_el[new_el];
>      unsigned int new_mode = aarch64_pstate_mode(new_el, true);
> -    int i;
>  
>      if (arm_current_el(env) < new_el) {
>          if (env->aarch64) {
> @@ -512,9 +511,7 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
>          }
>          env->elr_el[new_el] = env->regs[15];
>  
> -        for (i = 0; i < 15; i++) {
> -            env->xregs[i] = env->regs[i];
> -        }
> +        aarch64_sync_32_to_64(env);
>  
>          env->condexec_bits = 0;
>      }
> diff --git a/target-arm/internals.h b/target-arm/internals.h
> index bb171a7..626ea7d 100644
> --- a/target-arm/internals.h
> +++ b/target-arm/internals.h
> @@ -128,6 +128,95 @@ static inline void aarch64_restore_sp(CPUARMState *env, int el)
>      }
>  }
>  
> +static inline void aarch64_sync_32_to_64(CPUARMState *env)
> +{
> +    int i;
> +
> +    /* We can blanket copy R[0:7] to X[0:7] */
> +    for (i = 0; i < 8; i++) {
> +        env->xregs[i] = env->regs[i];
> +    }
> +
> +    /* If we are in USR mode then we just want to complete the above blanket
> +     * copy so we get the accurate register values.  If not, then we have to go
> +     * to the saved and banked user regs.
> +     */
> +    if ((env->uncached_cpsr & 0x1f) == ARM_CPU_MODE_USR) {
> +        for (i = 8; i < 15; i++) {
> +            env->xregs[i] = env->regs[i];
> +        }
> +    } else {
> +        for (i = 8; i < 13; i++) {
> +            env->xregs[i] = env->usr_regs[i-8];
> +        }
> +        env->xregs[13] = env->banked_r13[bank_number(ARM_CPU_MODE_USR)];
> +        env->xregs[14] = env->banked_r14[bank_number(ARM_CPU_MODE_USR)];
> +    }
> +    env->pc = env->regs[15];
> +
> +    env->xregs[15] = env->banked_r13[bank_number(ARM_CPU_MODE_HYP)];
> +    env->xregs[16] = env->banked_r13[bank_number(ARM_CPU_MODE_IRQ)];
> +    env->xregs[17] = env->banked_r14[bank_number(ARM_CPU_MODE_IRQ)];
> +    env->xregs[18] = env->banked_r13[bank_number(ARM_CPU_MODE_SVC)];
> +    env->xregs[19] = env->banked_r14[bank_number(ARM_CPU_MODE_SVC)];
> +    env->xregs[20] = env->banked_r13[bank_number(ARM_CPU_MODE_ABT)];
> +    env->xregs[21] = env->banked_r14[bank_number(ARM_CPU_MODE_ABT)];
> +    env->xregs[22] = env->banked_r13[bank_number(ARM_CPU_MODE_UND)];
> +    env->xregs[23] = env->banked_r14[bank_number(ARM_CPU_MODE_UND)];

some of these numebers look dodgy when I compare to LookUpRIndex pseudo
code in the ARM ARM in Section G1.8.  Can you explain why they seem
one-off?

Thanks,
-Christoffer

> +
> +    for (i = 0; i < 5; i++) {
> +        env->xregs[24+i] = env->fiq_regs[i];
> +    }
> +    env->xregs[29] = env->banked_r13[bank_number(ARM_CPU_MODE_FIQ)];
> +    env->xregs[30] = env->banked_r14[bank_number(ARM_CPU_MODE_FIQ)];
> +}
> +
> +static inline void aarch64_sync_64_to_32(CPUARMState *env)
> +{
> +    int i;
> +
> +    /* We can blanket copy R[0:7] to X[0:7] */
> +    for (i = 0; i < 8; i++) {
> +        env->regs[i] = env->xregs[i];
> +    }
> +
> +    /* If we are in USR mode then we want to complete the above blanket
> +     * copy as the XREGs will contain the most recent value.
> +     */
> +    if ((env->uncached_cpsr & 0x1f) == ARM_CPU_MODE_USR) {
> +        for (i = 8; i < 15; i++) {
> +            env->regs[i] = env->xregs[i];
> +        }
> +    }
> +
> +    /* Update the user copies and banked registers so they are also up to
> +     * date.
> +     */
> +    for (i = 8; i < 13; i++) {
> +        env->usr_regs[i-8] = env->xregs[i];
> +    }
> +    env->banked_r13[bank_number(ARM_CPU_MODE_USR)] = env->xregs[13];
> +    env->banked_r14[bank_number(ARM_CPU_MODE_USR)] = env->xregs[14];
> +
> +    env->regs[15] = env->pc;
> +
> +    env->banked_r13[bank_number(ARM_CPU_MODE_HYP)] = env->xregs[15];
> +    env->banked_r13[bank_number(ARM_CPU_MODE_IRQ)] = env->xregs[16];
> +    env->banked_r14[bank_number(ARM_CPU_MODE_IRQ)] = env->xregs[17];
> +    env->banked_r13[bank_number(ARM_CPU_MODE_SVC)] = env->xregs[18];
> +    env->banked_r14[bank_number(ARM_CPU_MODE_SVC)] = env->xregs[19];
> +    env->banked_r13[bank_number(ARM_CPU_MODE_ABT)] = env->xregs[20];
> +    env->banked_r14[bank_number(ARM_CPU_MODE_ABT)] = env->xregs[21];
> +    env->banked_r13[bank_number(ARM_CPU_MODE_UND)] = env->xregs[22];
> +    env->banked_r14[bank_number(ARM_CPU_MODE_UND)] = env->xregs[23];
> +
> +    for (i = 0; i < 5; i++) {
> +        env->fiq_regs[i] = env->xregs[24+i];
> +    }
> +    env->banked_r13[bank_number(ARM_CPU_MODE_FIQ)] = env->xregs[29];
> +    env->banked_r14[bank_number(ARM_CPU_MODE_FIQ)] = env->xregs[30];
> +}
> +
>  static inline void update_spsel(CPUARMState *env, uint32_t imm)
>  {
>      unsigned int cur_el = arm_current_el(env);
> diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
> index 2bed914..7713022 100644
> --- a/target-arm/op_helper.c
> +++ b/target-arm/op_helper.c
> @@ -465,7 +465,7 @@ void HELPER(exception_return)(CPUARMState *env)
>      int cur_el = arm_current_el(env);
>      unsigned int spsr_idx = aarch64_banked_spsr_index(cur_el);
>      uint32_t spsr = env->banked_spsr[spsr_idx];
> -    int new_el, i;
> +    int new_el;
>  
>      aarch64_save_sp(env, cur_el);
>  
> @@ -491,9 +491,7 @@ void HELPER(exception_return)(CPUARMState *env)
>          if (!arm_singlestep_active(env)) {
>              env->uncached_cpsr &= ~PSTATE_SS;
>          }
> -        for (i = 0; i < 15; i++) {
> -            env->regs[i] = env->xregs[i];
> -        }
> +        aarch64_sync_64_to_32(env);
>  
>          env->regs[15] = env->elr_el[1] & ~0x1;
>      } else {
> -- 
> 1.8.3.2
> 

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

[Christoffer Dall]

On Wed, Jan 21, 2015 at 12:49:53PM -0600, Greg Bellows wrote:
> 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 <greg.bellows@linaro.org>
> ---
>  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);

How can you call this unconditionally?  Don't you have to check if the
AARCH64 feature is disabled?

-Christoffer

>      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);
> +    }
>      reg.id = AARCH64_CORE_REG(regs.pstate);
>      reg.addr = (uintptr_t) &val;
>      ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
> @@ -218,6 +225,7 @@ int kvm_arch_get_registers(CPUState *cs)
>              return ret;
>          }
>      }
> +    aarch64_sync_64_to_32(env);
>  
>      reg.id = AARCH64_CORE_REG(regs.sp);
>      reg.addr = (uintptr_t) &env->sp_el[0];
> @@ -239,7 +247,12 @@ int kvm_arch_get_registers(CPUState *cs)
>      if (ret) {
>          return ret;
>      }
> -    pstate_write(env, val);
> +    if (is_a64(env)) {
> +        pstate_write(env, val);
> +    } else {
> +        env->uncached_cpsr = val & CPSR_M;
> +        cpsr_write(env, val, 0xffffffff);
> +    }
>  
>      /* KVM puts SP_EL0 in regs.sp and SP_EL1 in regs.sp_el1. On the
>       * QEMU side we keep the current SP in xregs[31] as well.
> -- 
> 1.8.3.2
> 

Re: [Qemu-devel] [PATCH v2 3/4] target-arm: Add 32/64-bit register sync

[Greg Bellows]

[-- Attachment #1: Type: text/plain, Size: 7390 bytes --]

On Tue, Jan 27, 2015 at 3:20 PM, Christoffer Dall <
christoffer.dall@linaro.org> wrote:

> On Wed, Jan 21, 2015 at 12:49:52PM -0600, Greg Bellows wrote:
> > Add AArch32 to AArch64 register sychronization functions.
> > Replace manual register synchronization with new functions in
> > aarch64_cpu_do_interrupt() and HELPER(exception_return)().
> >
> > Signed-off-by: Greg Bellows <greg.bellows@linaro.org>
> > ---
> >  target-arm/helper-a64.c |  5 +--
> >  target-arm/internals.h  | 89
> +++++++++++++++++++++++++++++++++++++++++++++++++
> >  target-arm/op_helper.c  |  6 ++--
> >  3 files changed, 92 insertions(+), 8 deletions(-)
> >
> > diff --git a/target-arm/helper-a64.c b/target-arm/helper-a64.c
> > index 81066ca..2b5a668 100644
> > --- a/target-arm/helper-a64.c
> > +++ b/target-arm/helper-a64.c
> > @@ -448,7 +448,6 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
> >      unsigned int new_el = arm_excp_target_el(cs, cs->exception_index);
> >      target_ulong addr = env->cp15.vbar_el[new_el];
> >      unsigned int new_mode = aarch64_pstate_mode(new_el, true);
> > -    int i;
> >
> >      if (arm_current_el(env) < new_el) {
> >          if (env->aarch64) {
> > @@ -512,9 +511,7 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
> >          }
> >          env->elr_el[new_el] = env->regs[15];
> >
> > -        for (i = 0; i < 15; i++) {
> > -            env->xregs[i] = env->regs[i];
> > -        }
> > +        aarch64_sync_32_to_64(env);
> >
> >          env->condexec_bits = 0;
> >      }
> > diff --git a/target-arm/internals.h b/target-arm/internals.h
> > index bb171a7..626ea7d 100644
> > --- a/target-arm/internals.h
> > +++ b/target-arm/internals.h
> > @@ -128,6 +128,95 @@ static inline void aarch64_restore_sp(CPUARMState
> *env, int el)
> >      }
> >  }
> >
> > +static inline void aarch64_sync_32_to_64(CPUARMState *env)
> > +{
> > +    int i;
> > +
> > +    /* We can blanket copy R[0:7] to X[0:7] */
> > +    for (i = 0; i < 8; i++) {
> > +        env->xregs[i] = env->regs[i];
> > +    }
> > +
> > +    /* If we are in USR mode then we just want to complete the above
> blanket
> > +     * copy so we get the accurate register values.  If not, then we
> have to go
> > +     * to the saved and banked user regs.
> > +     */
> > +    if ((env->uncached_cpsr & 0x1f) == ARM_CPU_MODE_USR) {
> > +        for (i = 8; i < 15; i++) {
> > +            env->xregs[i] = env->regs[i];
> > +        }
> > +    } else {
> > +        for (i = 8; i < 13; i++) {
> > +            env->xregs[i] = env->usr_regs[i-8];
> > +        }
> > +        env->xregs[13] = env->banked_r13[bank_number(ARM_CPU_MODE_USR)];
> > +        env->xregs[14] = env->banked_r14[bank_number(ARM_CPU_MODE_USR)];
> > +    }
> > +    env->pc = env->regs[15];
> > +
> > +    env->xregs[15] = env->banked_r13[bank_number(ARM_CPU_MODE_HYP)];
> > +    env->xregs[16] = env->banked_r13[bank_number(ARM_CPU_MODE_IRQ)];
> > +    env->xregs[17] = env->banked_r14[bank_number(ARM_CPU_MODE_IRQ)];
> > +    env->xregs[18] = env->banked_r13[bank_number(ARM_CPU_MODE_SVC)];
> > +    env->xregs[19] = env->banked_r14[bank_number(ARM_CPU_MODE_SVC)];
> > +    env->xregs[20] = env->banked_r13[bank_number(ARM_CPU_MODE_ABT)];
> > +    env->xregs[21] = env->banked_r14[bank_number(ARM_CPU_MODE_ABT)];
> > +    env->xregs[22] = env->banked_r13[bank_number(ARM_CPU_MODE_UND)];
> > +    env->xregs[23] = env->banked_r14[bank_number(ARM_CPU_MODE_UND)];
>
> some of these numebers look dodgy when I compare to LookUpRIndex pseudo
> code in the ARM ARM in Section G1.8.  Can you explain why they seem
> one-off?
>

​The ​LookUpRIndex is pseudo code for looking up the R bank used when
looking  up a register in AArch32 mode. The number are off because this is
not mapping from AArch64 X reg to R reg.  The table the code was written
from is I table D1-77 which describes the mapping between AArch32 R
registers to AArch64 X register.  I believe the correct mappings are being
used.


>
> Thanks,
> -Christoffer
>
> > +
> > +    for (i = 0; i < 5; i++) {
> > +        env->xregs[24+i] = env->fiq_regs[i];
> > +    }
> > +    env->xregs[29] = env->banked_r13[bank_number(ARM_CPU_MODE_FIQ)];
> > +    env->xregs[30] = env->banked_r14[bank_number(ARM_CPU_MODE_FIQ)];
> > +}
> > +
> > +static inline void aarch64_sync_64_to_32(CPUARMState *env)
> > +{
> > +    int i;
> > +
> > +    /* We can blanket copy R[0:7] to X[0:7] */
> > +    for (i = 0; i < 8; i++) {
> > +        env->regs[i] = env->xregs[i];
> > +    }
> > +
> > +    /* If we are in USR mode then we want to complete the above blanket
> > +     * copy as the XREGs will contain the most recent value.
> > +     */
> > +    if ((env->uncached_cpsr & 0x1f) == ARM_CPU_MODE_USR) {
> > +        for (i = 8; i < 15; i++) {
> > +            env->regs[i] = env->xregs[i];
> > +        }
> > +    }
> > +
> > +    /* Update the user copies and banked registers so they are also up
> to
> > +     * date.
> > +     */
> > +    for (i = 8; i < 13; i++) {
> > +        env->usr_regs[i-8] = env->xregs[i];
> > +    }
> > +    env->banked_r13[bank_number(ARM_CPU_MODE_USR)] = env->xregs[13];
> > +    env->banked_r14[bank_number(ARM_CPU_MODE_USR)] = env->xregs[14];
> > +
> > +    env->regs[15] = env->pc;
> > +
> > +    env->banked_r13[bank_number(ARM_CPU_MODE_HYP)] = env->xregs[15];
> > +    env->banked_r13[bank_number(ARM_CPU_MODE_IRQ)] = env->xregs[16];
> > +    env->banked_r14[bank_number(ARM_CPU_MODE_IRQ)] = env->xregs[17];
> > +    env->banked_r13[bank_number(ARM_CPU_MODE_SVC)] = env->xregs[18];
> > +    env->banked_r14[bank_number(ARM_CPU_MODE_SVC)] = env->xregs[19];
> > +    env->banked_r13[bank_number(ARM_CPU_MODE_ABT)] = env->xregs[20];
> > +    env->banked_r14[bank_number(ARM_CPU_MODE_ABT)] = env->xregs[21];
> > +    env->banked_r13[bank_number(ARM_CPU_MODE_UND)] = env->xregs[22];
> > +    env->banked_r14[bank_number(ARM_CPU_MODE_UND)] = env->xregs[23];
> > +
> > +    for (i = 0; i < 5; i++) {
> > +        env->fiq_regs[i] = env->xregs[24+i];
> > +    }
> > +    env->banked_r13[bank_number(ARM_CPU_MODE_FIQ)] = env->xregs[29];
> > +    env->banked_r14[bank_number(ARM_CPU_MODE_FIQ)] = env->xregs[30];
> > +}
> > +
> >  static inline void update_spsel(CPUARMState *env, uint32_t imm)
> >  {
> >      unsigned int cur_el = arm_current_el(env);
> > diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
> > index 2bed914..7713022 100644
> > --- a/target-arm/op_helper.c
> > +++ b/target-arm/op_helper.c
> > @@ -465,7 +465,7 @@ void HELPER(exception_return)(CPUARMState *env)
> >      int cur_el = arm_current_el(env);
> >      unsigned int spsr_idx = aarch64_banked_spsr_index(cur_el);
> >      uint32_t spsr = env->banked_spsr[spsr_idx];
> > -    int new_el, i;
> > +    int new_el;
> >
> >      aarch64_save_sp(env, cur_el);
> >
> > @@ -491,9 +491,7 @@ void HELPER(exception_return)(CPUARMState *env)
> >          if (!arm_singlestep_active(env)) {
> >              env->uncached_cpsr &= ~PSTATE_SS;
> >          }
> > -        for (i = 0; i < 15; i++) {
> > -            env->regs[i] = env->xregs[i];
> > -        }
> > +        aarch64_sync_64_to_32(env);
> >
> >          env->regs[15] = env->elr_el[1] & ~0x1;
> >      } else {
> > --
> > 1.8.3.2
> >
>

[-- Attachment #2: Type: text/html, Size: 9583 bytes --]

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

[Greg Bellows]

[-- Attachment #1: Type: text/plain, Size: 3371 bytes --]

On Tue, Jan 27, 2015 at 3:21 PM, Christoffer Dall <
christoffer.dall@linaro.org> wrote:

> On Wed, Jan 21, 2015 at 12:49:53PM -0600, Greg Bellows wrote:
> > 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 <greg.bellows@linaro.org>
> > ---
> >  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);
>
> How can you call this unconditionally?  Don't you have to check if the
> AARCH64 feature is disabled?
>
>
​That is indeed an issue.  I thought it was benign to do it
unconditionally, but as it turns out, this is incorrect.  Fixed in V3.​



> -Christoffer
>
> >      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);
> > +    }
> >      reg.id = AARCH64_CORE_REG(regs.pstate);
> >      reg.addr = (uintptr_t) &val;
> >      ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
> > @@ -218,6 +225,7 @@ int kvm_arch_get_registers(CPUState *cs)
> >              return ret;
> >          }
> >      }
> > +    aarch64_sync_64_to_32(env);
> >
> >      reg.id = AARCH64_CORE_REG(regs.sp);
> >      reg.addr = (uintptr_t) &env->sp_el[0];
> > @@ -239,7 +247,12 @@ int kvm_arch_get_registers(CPUState *cs)
> >      if (ret) {
> >          return ret;
> >      }
> > -    pstate_write(env, val);
> > +    if (is_a64(env)) {
> > +        pstate_write(env, val);
> > +    } else {
> > +        env->uncached_cpsr = val & CPSR_M;
> > +        cpsr_write(env, val, 0xffffffff);
> > +    }
> >
> >      /* KVM puts SP_EL0 in regs.sp and SP_EL1 in regs.sp_el1. On the
> >       * QEMU side we keep the current SP in xregs[31] as well.
> > --
> > 1.8.3.2
> >
>

[-- Attachment #2: Type: text/html, Size: 5038 bytes --]