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

[Qemu-devel] [PATCH v3 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

v2 -> v3
- Fix KVM64/AArch64 hang by conditionalizing register sync
- Conditionalize 64-bit interrupt handler setting of aarch64

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 |  9 +++--
 target-arm/internals.h  | 89 +++++++++++++++++++++++++++++++++++++++++++++++++
 target-arm/kvm64.c      | 33 +++++++++++++++---
 target-arm/op_helper.c  |  6 ++--
 7 files changed, 175 insertions(+), 17 deletions(-)

-- 
1.8.3.2

[Qemu-devel] [PATCH v3 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 v3 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 v3 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>

---

v2 -> v3
- Conditionalize interrupt handler update of aarch64.
---
 target-arm/helper-a64.c |  9 +++--
 target-arm/internals.h  | 89 +++++++++++++++++++++++++++++++++++++++++++++++++
 target-arm/op_helper.c  |  6 ++--
 3 files changed, 95 insertions(+), 9 deletions(-)

diff --git a/target-arm/helper-a64.c b/target-arm/helper-a64.c
index 81066ca..8c6a100 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,15 +511,15 @@ 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;
     }
 
     pstate_write(env, PSTATE_DAIF | new_mode);
-    env->aarch64 = 1;
+    if (arm_feature(env, ARM_FEATURE_AARCH64)) {
+        env->aarch64 = 1;
+    }
     aarch64_restore_sp(env, new_el);
 
     env->pc = addr;
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 v3 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>

---

v2 -> v3
- Conditionalize sync of 32-bit and 64-bit registers
---
 target-arm/kvm64.c | 33 +++++++++++++++++++++++++++++----
 1 file changed, 29 insertions(+), 4 deletions(-)

diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c
index ba16821..924b423 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,13 @@ int kvm_arch_put_registers(CPUState *cs, int level)
     ARMCPU *cpu = ARM_CPU(cs);
     CPUARMState *env = &cpu->env;
 
+    /* If we are in AArch32 mode then we need to sync the AArch64 regs with the
+     * AArch32 regs before pushing them out 64-bit KVM.
+     */
+    if (!is_a64(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 +171,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);
@@ -219,6 +232,13 @@ int kvm_arch_get_registers(CPUState *cs)
         }
     }
 
+    /* If we are in AArch32 mode then we need to sync the AArch32 regs with the
+     * incoming AArch64 regs received from 64-bit KVM.
+     */
+    if (!is_a64(env)) {
+        aarch64_sync_64_to_32(env);
+    }
+
     reg.id = AARCH64_CORE_REG(regs.sp);
     reg.addr = (uintptr_t) &env->sp_el[0];
     ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
@@ -239,7 +259,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 v3 0/4] target-arm: ARM64: Adding EL1 AARCH32 guest support

[Christoffer Dall]

On Tue, Jan 27, 2015 at 05:58:33PM -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 ...
> 
Tested-by: Christoffer Dall <christoffer.dall@linaro.org>

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

[Peter Maydell]

On 27 January 2015 at 23:58, Greg Bellows <greg.bellows@linaro.org> 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>
>
> ---
>
> v2 -> v3
> - Conditionalize interrupt handler update of aarch64.
> ---
>  target-arm/helper-a64.c |  9 +++--
>  target-arm/internals.h  | 89 +++++++++++++++++++++++++++++++++++++++++++++++++
>  target-arm/op_helper.c  |  6 ++--
>  3 files changed, 95 insertions(+), 9 deletions(-)
>
> diff --git a/target-arm/helper-a64.c b/target-arm/helper-a64.c
> index 81066ca..8c6a100 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,15 +511,15 @@ 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;
>      }
>
>      pstate_write(env, PSTATE_DAIF | new_mode);
> -    env->aarch64 = 1;
> +    if (arm_feature(env, ARM_FEATURE_AARCH64)) {
> +        env->aarch64 = 1;
> +    }

This doesn't look correct. If this CPU doesn't have the AArch64
feature then pretty much all of what this function does is wrong,
because it is the "take exception to an EL that is using AArch64"
function. We need to ensure that CPUs without the feature
bit never call this, either by setting the CPU class method
pointer correctly (which is what happens at the moment for
the traditional 32-bit-only CPUs) or else we need to arrange that
we dynamically call the right function depending on the register
width of the EL we're about to take the exception to. We'll
definitely need to handle that latter case for 64-bit EL3 or EL2
support (since suddenly EL1 can be 32-bit even in a CPU with
64-bit support enabled).

>      aarch64_restore_sp(env, new_el);
>
>      env->pc = addr;
> 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)

These functions are pretty big; I would just put them in a
suitable .c file rather than have them be static inline in
a .h file. A few lines of doc comment describing the purpose
of the functions would be nice too.

> +{
> +    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)];

This isn't going to give the right answers if we happen to be in
one of these modes. For instance if we're in SVC mode then SVC's
r13 (which needs to be copied into  xregs[18]) is going to be
in env->regs[13], not in the banked_r13 slot.

> +
> +    for (i = 0; i < 5; i++) {
> +        env->xregs[24+i] = env->fiq_regs[i];

Similarly if we're in FIQ mode then the remainder of the FIQ
registers are in env->regs[8] to [14], not in the fiq_regs[] or
banked_r13[] or banked_r14[].

PS: spaces round operators like '+', please.

> +    }
> +    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) {

use the CPSR_M mask, not 0x1f.

> +        for (i = 8; i < 15; i++) {
> +            env->regs[i] = env->xregs[i];
> +        }
> +    }

This is missing the update of regs[8..14] for modes other than USR
(which is sort of the inverse of the problem in the preceding function).

> +
> +    /* 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;

This feels a bit out of place -- I'd put it up with the rest
of the updates of env->regs[].

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

thanks
-- PMM

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

[Peter Maydell]

On 27 January 2015 at 23:58, Greg Bellows <greg.bellows@linaro.org> 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>
>
> ---
>
> v2 -> v3
> - Conditionalize sync of 32-bit and 64-bit registers
> ---
>  target-arm/kvm64.c | 33 +++++++++++++++++++++++++++++----
>  1 file changed, 29 insertions(+), 4 deletions(-)
>
> diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c
> index ba16821..924b423 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");

This change will let you try to run with KVM enabled and
-cpu cortex-a15 on a 64-bit host kernel, which is not right.
Worse, because the kernel's KVM_ARM_TARGET_CORTEX_A15 and
KVM_ARM_TARGET_AEM_V8 are the same value the VCPU init ioctl
may succeed but instantiate the wrong kind of CPU.
It probably needs to turn into a check whether cpu is a
subclass of TYPE_AARCH64_CPU.

>          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,13 @@ int kvm_arch_put_registers(CPUState *cs, int level)
>      ARMCPU *cpu = ARM_CPU(cs);
>      CPUARMState *env = &cpu->env;
>
> +    /* If we are in AArch32 mode then we need to sync the AArch64 regs with the
> +     * AArch32 regs before pushing them out 64-bit KVM.
> +     */
> +    if (!is_a64(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 +171,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);
> @@ -219,6 +232,13 @@ int kvm_arch_get_registers(CPUState *cs)
>          }
>      }
>
> +    /* If we are in AArch32 mode then we need to sync the AArch32 regs with the
> +     * incoming AArch64 regs received from 64-bit KVM.
> +     */
> +    if (!is_a64(env)) {
> +        aarch64_sync_64_to_32(env);
> +    }

This is happening too early, for two reasons:
 (1) this is_a64() call needs to operate on the state we read from
 the kernel, and we don't read the PSTATE from the kernel til later
 (2) aarch64_sync_64_to_32() reads env->pc, which again we haven't
 read from the kernel yet

> +
>      reg.id = AARCH64_CORE_REG(regs.sp);
>      reg.addr = (uintptr_t) &env->sp_el[0];
>      ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
> @@ -239,7 +259,12 @@ int kvm_arch_get_registers(CPUState *cs)
>      if (ret) {
>          return ret;
>      }
> -    pstate_write(env, val);
> +    if (is_a64(env)) {

The value currently in env->aarch64 is junk left over from the last
time execution stopped. What you need to do first is
   env->aarch64 = ((val & PSTATE_nRW) == 0);

(compare target-arm/machine.c:get_cpsr()).

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

thanks
-- PMM

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

[Peter Maydell]

On 27 January 2015 at 23:58, Greg Bellows <greg.bellows@linaro.org> wrote:
> Added machvirt parsing of feature keywords added to the -cpu command line
> option.  Parsing occurs during machine initialization.

> -    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]);

It seems a shame we have to do this kind of manual string
fiddling in every board model, but that is definitely not a
problem to address in this patchset.

> @@ -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 */

"specified"

> +        cc->parse_features(CPU(cpuobj), cpustr[1], &err);
> +        if (err) {
> +            error_report("%s", error_get_pretty(err));
> +            exit(1);
> +        }

Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

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

[Peter Maydell]

On 27 January 2015 at 23:58, Greg Bellows <greg.bellows@linaro.org> wrote:
> 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, ",");

strtok isn't thread safe. Let's just use g_strsplit like we did
in virt.c and then we don't have to worry about whether or not
multiple threads could ever end up here at the same time.

> +    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);
>  }

This all looks OK code-wise. Still need to think about whether we
can manage to end up with a nicer interface to the user than
cpuname,-aarch64, though.

-- PMM

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

[Peter Maydell]

On 3 February 2015 at 19:14, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 27 January 2015 at 23:58, Greg Bellows <greg.bellows@linaro.org> wrote:
>>  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);
>>  }
>
> This all looks OK code-wise. Still need to think about whether we
> can manage to end up with a nicer interface to the user than
> cpuname,-aarch64, though.

[I meant "-cpu cpuname,aarch64=off", hadn't processed that we've
updated to the new style syntax.]

I had a think about this as I was cycling home, and I (re)convinced
myself that modelling this as "remove the AArch64 feature flag" is
the right way to do it, which then just leaves us with "what's the
best user-facing UI we can manage to expose that?".

Suppose that we had two properties/feature flag switches that do
the same thing but have inverse sense:

  aarch64=off
  aarch32-only=on

Would that help, or just be more confusing?

-- PMM

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

[Christoffer Dall]

On Tue, Feb 3, 2015 at 10:15 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 3 February 2015 at 19:14, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 27 January 2015 at 23:58, Greg Bellows <greg.bellows@linaro.org> wrote:
>>>  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);
>>>  }
>>
>> This all looks OK code-wise. Still need to think about whether we
>> can manage to end up with a nicer interface to the user than
>> cpuname,-aarch64, though.
>
> [I meant "-cpu cpuname,aarch64=off", hadn't processed that we've
> updated to the new style syntax.]
>
> I had a think about this as I was cycling home, and I (re)convinced
> myself that modelling this as "remove the AArch64 feature flag" is
> the right way to do it, which then just leaves us with "what's the
> best user-facing UI we can manage to expose that?".
>
> Suppose that we had two properties/feature flag switches that do
> the same thing but have inverse sense:
>
>   aarch64=off
>   aarch32-only=on
>
> Would that help, or just be more confusing?
>
that would help, imho.

-Christoffer

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

[Greg Bellows]

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

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

> On Tue, Feb 3, 2015 at 10:15 PM, Peter Maydell <peter.maydell@linaro.org>
> wrote:
> > On 3 February 2015 at 19:14, Peter Maydell <peter.maydell@linaro.org>
> wrote:
> >> On 27 January 2015 at 23:58, Greg Bellows <greg.bellows@linaro.org>
> wrote:
> >>>  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);
> >>>  }
> >>
> >> This all looks OK code-wise. Still need to think about whether we
> >> can manage to end up with a nicer interface to the user than
> >> cpuname,-aarch64, though.
> >
> > [I meant "-cpu cpuname,aarch64=off", hadn't processed that we've
> > updated to the new style syntax.]
> >
> > I had a think about this as I was cycling home, and I (re)convinced
> > myself that modelling this as "remove the AArch64 feature flag" is
> > the right way to do it, which then just leaves us with "what's the
> > best user-facing UI we can manage to expose that?".
> >
> > Suppose that we had two properties/feature flag switches that do
> > the same thing but have inverse sense:
> >
> >   aarch64=off
> >   aarch32-only=on
> >
> > Would that help, or just be more confusing?
> >
> that would help, imho.
>
>
​Hmmm..., while it is trivial to add the second option I think it would be
confusing for users and begs for issues of having both specified.  If we
prefer the second name lets go with it otherwise my vote is against having
two.  If the behavior of the flag is "remove the AArch64 flag" it seems
most natural to stick with what we have as it conveys just that.  The other
option seems counter-intuitive.



> -Christoffer
>

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

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

[Greg Bellows]

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

On Tue, Feb 3, 2015 at 1:14 PM, Peter Maydell <peter.maydell@linaro.org>
wrote:

> On 27 January 2015 at 23:58, Greg Bellows <greg.bellows@linaro.org> wrote:
> > 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, ",");
>
> strtok isn't thread safe. Let's just use g_strsplit like we did
> in virt.c and then we don't have to worry about whether or not
> multiple threads could ever end up here at the same time.
>
>
​Sure, I'll change it.​



> > +    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);
> >  }
>
> This all looks OK code-wise. Still need to think about whether we
> can manage to end up with a nicer interface to the user than
> cpuname,-aarch64, though.
>
> -- PMM
>

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

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

[Greg Bellows]

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

On Tue, Feb 3, 2015 at 12:54 PM, Peter Maydell <peter.maydell@linaro.org>
wrote:

> On 27 January 2015 at 23:58, Greg Bellows <greg.bellows@linaro.org> 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>
> >
> > ---
> >
> > v2 -> v3
> > - Conditionalize interrupt handler update of aarch64.
> > ---
> >  target-arm/helper-a64.c |  9 +++--
> >  target-arm/internals.h  | 89
> +++++++++++++++++++++++++++++++++++++++++++++++++
> >  target-arm/op_helper.c  |  6 ++--
> >  3 files changed, 95 insertions(+), 9 deletions(-)
> >
> > diff --git a/target-arm/helper-a64.c b/target-arm/helper-a64.c
> > index 81066ca..8c6a100 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,15 +511,15 @@ 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;
> >      }
> >
> >      pstate_write(env, PSTATE_DAIF | new_mode);
> > -    env->aarch64 = 1;
> > +    if (arm_feature(env, ARM_FEATURE_AARCH64)) {
> > +        env->aarch64 = 1;
> > +    }
>
> This doesn't look correct. If this CPU doesn't have the AArch64
> feature then pretty much all of what this function does is wrong,
> because it is the "take exception to an EL that is using AArch64"
> function. We need to ensure that CPUs without the feature
> bit never call this, either by setting the CPU class method
> pointer correctly (which is what happens at the moment for
> the traditional 32-bit-only CPUs) or else we need to arrange that
> we dynamically call the right function depending on the register
> width of the EL we're about to take the exception to. We'll
> definitely need to handle that latter case for 64-bit EL3 or EL2
> support (since suddenly EL1 can be 32-bit even in a CPU with
> 64-bit support enabled).
>
> >      aarch64_restore_sp(env, new_el);
> >
> >      env->pc = addr;
> > 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)
>
> These functions are pretty big; I would just put them in a
> suitable .c file rather than have them be static inline in
> a .h file. A few lines of doc comment describing the purpose
> of the functions would be nice too.
>

​Moved and commented in v4
​


>
> > +{
> > +    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)];
>
> This isn't going to give the right answers if we happen to be in
> one of these modes. For instance if we're in SVC mode then SVC's
> r13 (which needs to be copied into  xregs[18]) is going to be
> in env->regs[13], not in the banked_r13 slot.
>

​Yes, you are correct and I dealt with this for USR mode but for some
reason failed to do so for the other modes.  Fixed in v4.​


>
> > +
> > +    for (i = 0; i < 5; i++) {
> > +        env->xregs[24+i] = env->fiq_regs[i];
>
> Similarly if we're in FIQ mode then the remainder of the FIQ
> registers are in env->regs[8] to [14], not in the fiq_regs[] or
> banked_r13[] or banked_r14[].
>
> PS: spaces round operators like '+', please.
>

​Fixed in v4


>
> > +    }
> > +    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) {
>
> use the CPSR_M mask, not 0x1f.
>
> > +        for (i = 8; i < 15; i++) {
> > +            env->regs[i] = env->xregs[i];
> > +        }
> > +    }
>
> This is missing the update of regs[8..14] for modes other than USR
> (which is sort of the inverse of the problem in the preceding function).
>

Yes, this is due to the same omission you pointed out above.  I should also
point out that r8:r12 only get sync'd when in USR or FIQ mode.


>
> > +
> > +    /* 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;
>
> This feels a bit out of place -- I'd put it up with the rest
> of the updates of env->regs[].
>

​I originally put this at the end because regs[15] was the last register
numerically to be updated.  I think the end makes even more sense given my
v4 changes.  Let me know if you agree.​


>
> > +    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 {
>
> thanks
> -- PMM
>

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

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

[Greg Bellows]

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

On Tue, Feb 3, 2015 at 12:54 PM, Peter Maydell <peter.maydell@linaro.org>
wrote:

> On 27 January 2015 at 23:58, Greg Bellows <greg.bellows@linaro.org> 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>
> >
> > ---
> >
> > v2 -> v3
> > - Conditionalize interrupt handler update of aarch64.
> > ---
> >  target-arm/helper-a64.c |  9 +++--
> >  target-arm/internals.h  | 89
> +++++++++++++++++++++++++++++++++++++++++++++++++
> >  target-arm/op_helper.c  |  6 ++--
> >  3 files changed, 95 insertions(+), 9 deletions(-)
> >
> > diff --git a/target-arm/helper-a64.c b/target-arm/helper-a64.c
> > index 81066ca..8c6a100 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,15 +511,15 @@ 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;
> >      }
> >
> >      pstate_write(env, PSTATE_DAIF | new_mode);
> > -    env->aarch64 = 1;
> > +    if (arm_feature(env, ARM_FEATURE_AARCH64)) {
> > +        env->aarch64 = 1;
> > +    }
>
> This doesn't look correct. If this CPU doesn't have the AArch64
> feature then pretty much all of what this function does is wrong,
> because it is the "take exception to an EL that is using AArch64"
> function. We need to ensure that CPUs without the feature
> bit never call this, either by setting the CPU class method
> pointer correctly (which is what happens at the moment for
> the traditional 32-bit-only CPUs) or else we need to arrange that
> we dynamically call the right function depending on the register
> width of the EL we're about to take the exception to. We'll
> definitely need to handle that latter case for 64-bit EL3 or EL2
> support (since suddenly EL1 can be 32-bit even in a CPU with
> 64-bit support enabled).
>

​​I think this is deeper than the one routine.  Essentially, we have to
convert the AArch64 CPU object into an ARM CPU object on the fly. This
would have to happen after class instantiation because we don't have the
properties until after.  From looking at the code, I'm not convinced it is
safe to override the fields, but I need to look a bit more.

I wanted to understand more, so I ran without my changes to see if aarch64
ever gets cleared and sure enough it does.  Is this expected?

I wasn't convinced this should occur, so I investigated further. The only
place I see us explicitly set it to 0 is in exception return when spsr has
nRW set. So I set a watch point on the spsr, and sure enough I see an
update occur setting nRW which means aarch64 gets set to 0.

I realize it is possible for the PSR to get set but are we prepared to
handle it?


> >      aarch64_restore_sp(env, new_el);
> >
> >      env->pc = addr;
> > 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)
>
> These functions are pretty big; I would just put them in a
> suitable .c file rather than have them be static inline in
> a .h file. A few lines of doc comment describing the purpose
> of the functions would be nice too.
>
> > +{
> > +    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)];
>
> This isn't going to give the right answers if we happen to be in
> one of these modes. For instance if we're in SVC mode then SVC's
> r13 (which needs to be copied into  xregs[18]) is going to be
> in env->regs[13], not in the banked_r13 slot.
>
> > +
> > +    for (i = 0; i < 5; i++) {
> > +        env->xregs[24+i] = env->fiq_regs[i];
>
> Similarly if we're in FIQ mode then the remainder of the FIQ
> registers are in env->regs[8] to [14], not in the fiq_regs[] or
> banked_r13[] or banked_r14[].
>
> PS: spaces round operators like '+', please.
>
> > +    }
> > +    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) {
>
> use the CPSR_M mask, not 0x1f.
>
> > +        for (i = 8; i < 15; i++) {
> > +            env->regs[i] = env->xregs[i];
> > +        }
> > +    }
>
> This is missing the update of regs[8..14] for modes other than USR
> (which is sort of the inverse of the problem in the preceding function).
>
> > +
> > +    /* 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;
>
> This feels a bit out of place -- I'd put it up with the rest
> of the updates of env->regs[].
>
> > +    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 {
>
> thanks
> -- PMM
>

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

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

[Peter Maydell]

On 4 February 2015 at 23:37, Greg Bellows <greg.bellows@linaro.org> wrote:
> Peter Maydell wrote:
>> This doesn't look correct. If this CPU doesn't have the AArch64
>> feature then pretty much all of what this function does is wrong,
>> because it is the "take exception to an EL that is using AArch64"
>> function.

> I think this is deeper than the one routine.  Essentially, we have to
> convert the AArch64 CPU object into an ARM CPU object on the fly. This would
> have to happen after class instantiation because we don't have the
> properties until after.  From looking at the code, I'm not convinced it is
> safe to override the fields, but I need to look a bit more.

I think we probably want the other approach -- leave the object being
the class it is, but the functions have to cope with the CPU possibly
being in 32 bit mode. We need that anyway for interworking between
64 bit EL3 and 32 bit EL1.

> I wanted to understand more, so I ran without my changes to see if aarch64
> ever gets cleared and sure enough it does.  Is this expected?

I'm not sure what you mean here.

> I wasn't convinced this should occur, so I investigated further. The only
> place I see us explicitly set it to 0 is in exception return when spsr has
> nRW set. So I set a watch point on the spsr, and sure enough I see an update
> occur setting nRW which means aarch64 gets set to 0.

This sounds like "exception return to a 32-bit EL0", which obviously
clears the aarch64 flag.

> I realize it is possible for the PSR to get set but are we prepared to
> handle it?

To handle what? At the moment the code assumes that if we're a 64
bit CPU then we can only be in 32 bit mode for an AArch32 EL0.
This obviously needs to be fixed to support both 32-bit EL1
under 64-bit EL3 and also in this case to support a 64-bit CPU
that only has 32-bit EL1. My original point is that the change
to aarch64_cpu_do_interrupt() as you have it in this patch can't
possibly work, because it is not actually doing what you need
to do to handle taking exceptions to a 32 bit EL.

-- PMM