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

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

---
v3 -> v4
- Move and fix sync functions to properly handle current EL/PL
- Replace use of strtok
- Add disablement of aarch64 option if KVM disabled
- Revert and update AArch64 check on vcpu init to not check for AARch64 feature
  but rather AArch64 family type.
- Relocate register sync on get
- add missing env->aarch64 refresh after pstate fetch

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

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        |   5 +-
 target-arm/cpu.h        |   2 +
 target-arm/cpu64.c      |  39 +++++++++++
 target-arm/helper-a64.c |   5 +-
 target-arm/helper.c     | 181 ++++++++++++++++++++++++++++++++++++++++++++++++
 target-arm/kvm64.c      |  38 ++++++++--
 target-arm/op_helper.c  |   6 +-
 8 files changed, 280 insertions(+), 16 deletions(-)

--
1.8.3.2

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

---

v3 -> v4
- Switch from using strtok to g_strsplit
- Add disablement of aarch64 option if KVM is not enabled.

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   |  5 ++++-
 target-arm/cpu64.c | 39 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+), 1 deletion(-)

diff --git a/target-arm/cpu.c b/target-arm/cpu.c
index d38af74..986f04c 100644
--- a/target-arm/cpu.c
+++ b/target-arm/cpu.c
@@ -544,13 +544,16 @@ 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_strsplit(cpu_model, ",", 1);
+    typename = g_strdup_printf("%s-" TYPE_ARM_CPU, cpuname[0]);
     oc = object_class_by_name(typename);
+    g_strfreev(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..d03f203 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,42 @@ 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);
+
+    /* At this time, this property is only allowed if KVM is enabled.  This
+     * restriction allows us to avoid fixing up functionality that assumes a
+     * uniform execution state like do_interrupt.
+     */
+    if (!kvm_enabled()) {
+        error_setg(errp,
+                   "'aarch64' option only supported with '-enable-kvm'");
+        return;
+    }
+
+    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 v4 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>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

---

v3 -> v4
- Fix misspelling

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 34d9379..3c37a5e 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -602,15 +602,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);
     }
 
@@ -624,8 +628,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");
@@ -633,6 +639,13 @@ static void machvirt_init(MachineState *machine)
         }
         cpuobj = object_new(object_class_get_name(oc));
 
+        /* Handle any CPU options specified 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);
         }
@@ -652,6 +665,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 v4 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>

---

v3 -> v4
- Rework sync routines to cover various exception levels
- Move sync routines to helper.c

v2 -> v3
- Conditionalize interrupt handler update of aarch64.
---
 target-arm/cpu.h        |   2 +
 target-arm/helper-a64.c |   5 +-
 target-arm/helper.c     | 181 ++++++++++++++++++++++++++++++++++++++++++++++++
 target-arm/op_helper.c  |   6 +-
 4 files changed, 186 insertions(+), 8 deletions(-)

diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index 1830a12..11845a6 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -495,6 +495,8 @@ typedef struct CPUARMState {
 ARMCPU *cpu_arm_init(const char *cpu_model);
 int cpu_arm_exec(CPUARMState *s);
 uint32_t do_arm_semihosting(CPUARMState *env);
+void aarch64_sync_32_to_64(CPUARMState *env);
+void aarch64_sync_64_to_32(CPUARMState *env);
 
 static inline bool is_a64(CPUARMState *env)
 {
diff --git a/target-arm/helper-a64.c b/target-arm/helper-a64.c
index 8aa40e9..7e0d038 100644
--- a/target-arm/helper-a64.c
+++ b/target-arm/helper-a64.c
@@ -466,7 +466,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) {
@@ -530,9 +529,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/helper.c b/target-arm/helper.c
index 1a1a005..c1d6764 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -4096,6 +4096,15 @@ unsigned int arm_excp_target_el(CPUState *cs, unsigned int excp_idx)
     return 1;
 }
 
+void aarch64_sync_64_to_32(CPUARMState *env)
+{
+    int i;
+
+    for (i = 0; i < 15; i++) {
+        env->regs[i] = env->xregs[i];
+    }
+}
+
 #else
 
 /* Map CPU modes onto saved register banks.  */
@@ -4425,6 +4434,178 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
     env->thumb = addr & 1;
 }
 
+/* Function used to synchronize QEMU's AArch64 register set with AArch32
+ * register set.  This is necessary when switching between AArch32 and AArch64
+ * execution state.
+ */
+void aarch64_sync_32_to_64(CPUARMState *env)
+{
+    int i;
+    uint32_t mode = env->uncached_cpsr & CPSR_M;
+
+    /* We can blanket copy R[0:7] to X[0:7] */
+    for (i = 0; i < 8; i++) {
+        env->xregs[i] = env->regs[i];
+    }
+
+    /* The latest copy of some registers depend on the current executing mode.
+     * The general purpose copy is used when the current mode corresponds to
+     * the mapping for the given register.  Otherwise, the banked copy
+     * corresponding to the register mapping is used.
+     */
+    if (mode == 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)];
+    }
+
+    if (mode == ARM_CPU_MODE_HYP) {
+        env->xregs[15] = env->regs[13];
+    } else {
+        env->xregs[15] = env->banked_r13[bank_number(ARM_CPU_MODE_HYP)];
+    }
+
+    if (mode == ARM_CPU_MODE_IRQ) {
+        env->xregs[16] = env->regs[13];
+        env->xregs[17] = env->regs[14];
+    } else {
+        env->xregs[16] = env->banked_r13[bank_number(ARM_CPU_MODE_IRQ)];
+        env->xregs[17] = env->banked_r14[bank_number(ARM_CPU_MODE_IRQ)];
+    }
+
+    if (mode == ARM_CPU_MODE_SVC) {
+        env->xregs[18] = env->regs[13];
+        env->xregs[19] = env->regs[14];
+    } else {
+        env->xregs[18] = env->banked_r13[bank_number(ARM_CPU_MODE_SVC)];
+        env->xregs[19] = env->banked_r14[bank_number(ARM_CPU_MODE_SVC)];
+    }
+
+    if (mode == ARM_CPU_MODE_ABT) {
+        env->xregs[20] = env->regs[13];
+        env->xregs[21] = env->regs[14];
+    } else {
+        env->xregs[20] = env->banked_r13[bank_number(ARM_CPU_MODE_ABT)];
+        env->xregs[21] = env->banked_r14[bank_number(ARM_CPU_MODE_ABT)];
+    }
+
+    if (mode == ARM_CPU_MODE_UND) {
+        env->xregs[22] = env->regs[13];
+        env->xregs[23] = env->regs[14];
+    } else {
+        env->xregs[22] = env->banked_r13[bank_number(ARM_CPU_MODE_UND)];
+        env->xregs[23] = env->banked_r14[bank_number(ARM_CPU_MODE_UND)];
+    }
+
+    if (mode == ARM_CPU_MODE_FIQ) {
+        for (i = 24; i < 31; i++) {
+            env->xregs[i] = env->regs[i - 16];   /* X[24:30] -> R[8:14] */
+        }
+    } else {
+        for (i = 24; i < 29; i++) {
+            env->xregs[i] = env->fiq_regs[i - 24];
+        }
+        env->xregs[29] = env->banked_r13[bank_number(ARM_CPU_MODE_FIQ)];
+        env->xregs[30] = env->banked_r14[bank_number(ARM_CPU_MODE_FIQ)];
+    }
+
+    env->pc = env->regs[15];
+}
+
+/* Function used to synchronize QEMU's AArch32 register set with AArch64
+ * register set.  This is necessary when switching between AArch32 and AArch64
+ * execution state.
+ */
+void aarch64_sync_64_to_32(CPUARMState *env)
+{
+    int i;
+    uint32_t mode = env->uncached_cpsr & CPSR_M;
+
+    /* We can blanket copy X[0:7] to R[0:7] */
+    for (i = 0; i < 8; i++) {
+        env->regs[i] = env->xregs[i];
+    }
+
+    /* The destination of some registers depend on the current executing mode.
+     * The AArch32 registers 8-12 are only sync'd when we are in USR or FIQ
+     * mode as they are the only modes where AArch64 registers map to these
+     * registers.  In all other cases, the respective USR and FIQ banks are
+     * sync'd.
+     * The AArch32 registers 13 & 14 are sync'd depending on the execution mode
+     * we are in.  If we are not in a given mode, the bank corresponding to the
+     * AArch64 register is sync'd.
+     */
+    if (mode == ARM_CPU_MODE_USR) {
+        for (i = 8; i < 15; i++) {
+            env->regs[i] = env->xregs[i];
+        }
+    } else {
+        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];
+    }
+
+    if (mode == ARM_CPU_MODE_HYP) {
+        env->regs[13] = env->xregs[15];
+    } else {
+        env->banked_r13[bank_number(ARM_CPU_MODE_HYP)] = env->xregs[15];
+    }
+
+    if (mode == ARM_CPU_MODE_IRQ) {
+        env->regs[13] = env->xregs[16];
+        env->regs[14] = env->xregs[17];
+    } else {
+        env->banked_r13[bank_number(ARM_CPU_MODE_IRQ)] = env->xregs[16];
+        env->banked_r14[bank_number(ARM_CPU_MODE_IRQ)] = env->xregs[17];
+    }
+
+    if (mode == ARM_CPU_MODE_SVC) {
+        env->regs[13] = env->xregs[18];
+        env->regs[14] = env->xregs[19];
+    } else {
+        env->banked_r13[bank_number(ARM_CPU_MODE_SVC)] = env->xregs[18];
+        env->banked_r14[bank_number(ARM_CPU_MODE_SVC)] = env->xregs[19];
+    }
+
+    if (mode == ARM_CPU_MODE_ABT) {
+        env->regs[13] = env->xregs[20];
+        env->regs[14] = env->xregs[21];
+    } else {
+        env->banked_r13[bank_number(ARM_CPU_MODE_ABT)] = env->xregs[20];
+        env->banked_r14[bank_number(ARM_CPU_MODE_ABT)] = env->xregs[21];
+    }
+
+    if (mode == ARM_CPU_MODE_UND) {
+        env->regs[13] = env->xregs[22];
+        env->regs[14] = env->xregs[23];
+    } else {
+        env->banked_r13[bank_number(ARM_CPU_MODE_UND)] = env->xregs[22];
+        env->banked_r14[bank_number(ARM_CPU_MODE_UND)] = env->xregs[23];
+    }
+
+    if (mode == ARM_CPU_MODE_FIQ) {
+        for (i = 8; i < 15; i++) {
+            env->regs[i] = env->xregs[i + 16];   /* X[24:30] -> R[8:14] */
+        }
+    } else {
+        for (i = 0; i < 5; i++) {
+            env->fiq_regs[i] = env->xregs[i + 24];
+        }
+        env->banked_r13[bank_number(ARM_CPU_MODE_FIQ)] = env->xregs[29];
+        env->banked_r14[bank_number(ARM_CPU_MODE_FIQ)] = env->xregs[30];
+    }
+
+    env->regs[15] = env->pc;
+}
+
 /* Handle a CPU exception.  */
 void arm_cpu_do_interrupt(CPUState *cs)
 {
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 v4 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>

---

v3 -> v4
- Add check that to make sure KVM64 is only being used on AArch64 family of
  machines.
- Relocate register sync to follow register fetches.
- Refresh env->aarch64 prior to use.

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

diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c
index 033babf..789933e 100644
--- a/target-arm/kvm64.c
+++ b/target-arm/kvm64.c
@@ -81,8 +81,8 @@ 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 &&
+        object_dynamic_cast(cpu, TYPE_AARCH64_CPU)) {
         fprintf(stderr, "KVM is not supported for this guest CPU type\n");
         return -EINVAL;
     }
@@ -96,6 +96,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 +136,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 +172,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);
@@ -242,7 +256,14 @@ int kvm_arch_get_registers(CPUState *cs)
     if (ret) {
         return ret;
     }
-    pstate_write(env, val);
+
+    env->aarch64 = ((val & PSTATE_nRW) == 0);
+    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.
@@ -256,6 +277,15 @@ int kvm_arch_get_registers(CPUState *cs)
         return ret;
     }
 
+    /* If we are in AArch32 mode then we need to sync the AArch32 regs with the
+     * incoming AArch64 regs received from 64-bit KVM.
+     * We must perform this after all of the registers have been acquired from
+     * the kernel.
+     */
+    if (!is_a64(env)) {
+        aarch64_sync_64_to_32(env);
+    }
+
     reg.id = AARCH64_CORE_REG(elr_el1);
     reg.addr = (uintptr_t) &env->elr_el[1];
     ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
-- 
1.8.3.2

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

[Peter Maydell]

On 10 February 2015 at 10:50, 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>
>
> ---
>
> v3 -> v4
> - Switch from using strtok to g_strsplit
> - Add disablement of aarch64 option if KVM is not enabled.
>
> 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   |  5 ++++-
>  target-arm/cpu64.c | 39 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 43 insertions(+), 1 deletion(-)
>
> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
> index d38af74..986f04c 100644
> --- a/target-arm/cpu.c
> +++ b/target-arm/cpu.c
> @@ -544,13 +544,16 @@ 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_strsplit(cpu_model, ",", 1);
> +    typename = g_strdup_printf("%s-" TYPE_ARM_CPU, cpuname[0]);
>      oc = object_class_by_name(typename);
> +    g_strfreev(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..d03f203 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,42 @@ 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);
> +
> +    /* At this time, this property is only allowed if KVM is enabled.  This
> +     * restriction allows us to avoid fixing up functionality that assumes a
> +     * uniform execution state like do_interrupt.
> +     */
> +    if (!kvm_enabled()) {
> +        error_setg(errp,
> +                   "'aarch64' option only supported with '-enable-kvm'");
> +        return;

This check needs to go in the "we're unsetting the feature bit"
code path, and we could make the error message a little clearer:
"'aarch64' feature cannot be disabled unless KVM is enabled"
(setting the feature to on is harmless and will work with TCG :-))

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

"Set on/off to enable/disable support for AArch64 execution
state. Disable this to boot 32-bit guests in AArch32 state."

(Is that space at the end of your description deliberate or
accidental?)

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

-- PMM

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

[Peter Maydell]

On 10 February 2015 at 10:50, 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>
>
> ---
>
> v3 -> v4
> - Rework sync routines to cover various exception levels
> - Move sync routines to helper.c
>
> v2 -> v3
> - Conditionalize interrupt handler update of aarch64.
> ---
>  target-arm/cpu.h        |   2 +
>  target-arm/helper-a64.c |   5 +-
>  target-arm/helper.c     | 181 ++++++++++++++++++++++++++++++++++++++++++++++++
>  target-arm/op_helper.c  |   6 +-
>  4 files changed, 186 insertions(+), 8 deletions(-)
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 1830a12..11845a6 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -495,6 +495,8 @@ typedef struct CPUARMState {
>  ARMCPU *cpu_arm_init(const char *cpu_model);
>  int cpu_arm_exec(CPUARMState *s);
>  uint32_t do_arm_semihosting(CPUARMState *env);
> +void aarch64_sync_32_to_64(CPUARMState *env);
> +void aarch64_sync_64_to_32(CPUARMState *env);
>
>  static inline bool is_a64(CPUARMState *env)
>  {
> diff --git a/target-arm/helper-a64.c b/target-arm/helper-a64.c
> index 8aa40e9..7e0d038 100644
> --- a/target-arm/helper-a64.c
> +++ b/target-arm/helper-a64.c
> @@ -466,7 +466,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) {
> @@ -530,9 +529,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/helper.c b/target-arm/helper.c
> index 1a1a005..c1d6764 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -4096,6 +4096,15 @@ unsigned int arm_excp_target_el(CPUState *cs, unsigned int excp_idx)
>      return 1;
>  }
>
> +void aarch64_sync_64_to_32(CPUARMState *env)
> +{
> +    int i;
> +
> +    for (i = 0; i < 15; i++) {
> +        env->regs[i] = env->xregs[i];
> +    }
> +}

This is inside CONFIG_USER_ONLY, right? Is it called at all?
If so, when?

> +
>  #else
>
>  /* Map CPU modes onto saved register banks.  */
> @@ -4425,6 +4434,178 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
>      env->thumb = addr & 1;
>  }
>
> +/* Function used to synchronize QEMU's AArch64 register set with AArch32
> + * register set.  This is necessary when switching between AArch32 and AArch64
> + * execution state.

This is a bit vague...

> + */
> +void aarch64_sync_32_to_64(CPUARMState *env)
> +{
> +    int i;
> +    uint32_t mode = env->uncached_cpsr & CPSR_M;
> +
> +    /* We can blanket copy R[0:7] to X[0:7] */
> +    for (i = 0; i < 8; i++) {
> +        env->xregs[i] = env->regs[i];
> +    }
> +
> +    /* The latest copy of some registers depend on the current executing mode.

"depends"

> +     * The general purpose copy is used when the current mode corresponds to
> +     * the mapping for the given register.  Otherwise, the banked copy
> +     * corresponding to the register mapping is used.
> +     */
> +    if (mode == 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)];
> +    }
> +
> +    if (mode == ARM_CPU_MODE_HYP) {
> +        env->xregs[15] = env->regs[13];
> +    } else {
> +        env->xregs[15] = env->banked_r13[bank_number(ARM_CPU_MODE_HYP)];
> +    }
> +
> +    if (mode == ARM_CPU_MODE_IRQ) {
> +        env->xregs[16] = env->regs[13];
> +        env->xregs[17] = env->regs[14];
> +    } else {
> +        env->xregs[16] = env->banked_r13[bank_number(ARM_CPU_MODE_IRQ)];
> +        env->xregs[17] = env->banked_r14[bank_number(ARM_CPU_MODE_IRQ)];
> +    }
> +
> +    if (mode == ARM_CPU_MODE_SVC) {
> +        env->xregs[18] = env->regs[13];
> +        env->xregs[19] = env->regs[14];
> +    } else {
> +        env->xregs[18] = env->banked_r13[bank_number(ARM_CPU_MODE_SVC)];
> +        env->xregs[19] = env->banked_r14[bank_number(ARM_CPU_MODE_SVC)];
> +    }
> +
> +    if (mode == ARM_CPU_MODE_ABT) {
> +        env->xregs[20] = env->regs[13];
> +        env->xregs[21] = env->regs[14];
> +    } else {
> +        env->xregs[20] = env->banked_r13[bank_number(ARM_CPU_MODE_ABT)];
> +        env->xregs[21] = env->banked_r14[bank_number(ARM_CPU_MODE_ABT)];
> +    }
> +
> +    if (mode == ARM_CPU_MODE_UND) {
> +        env->xregs[22] = env->regs[13];
> +        env->xregs[23] = env->regs[14];
> +    } else {
> +        env->xregs[22] = env->banked_r13[bank_number(ARM_CPU_MODE_UND)];
> +        env->xregs[23] = env->banked_r14[bank_number(ARM_CPU_MODE_UND)];
> +    }
> +
> +    if (mode == ARM_CPU_MODE_FIQ) {
> +        for (i = 24; i < 31; i++) {
> +            env->xregs[i] = env->regs[i - 16];   /* X[24:30] -> R[8:14] */
> +        }
> +    } else {
> +        for (i = 24; i < 29; i++) {
> +            env->xregs[i] = env->fiq_regs[i - 24];
> +        }
> +        env->xregs[29] = env->banked_r13[bank_number(ARM_CPU_MODE_FIQ)];
> +        env->xregs[30] = env->banked_r14[bank_number(ARM_CPU_MODE_FIQ)];
> +    }
> +
> +    env->pc = env->regs[15];
> +}
> +
> +/* Function used to synchronize QEMU's AArch32 register set with AArch64
> + * register set.  This is necessary when switching between AArch32 and AArch64
> + * execution state.
> + */
> +void aarch64_sync_64_to_32(CPUARMState *env)
> +{
> +    int i;
> +    uint32_t mode = env->uncached_cpsr & CPSR_M;
> +
> +    /* We can blanket copy X[0:7] to R[0:7] */
> +    for (i = 0; i < 8; i++) {
> +        env->regs[i] = env->xregs[i];
> +    }
> +
> +    /* The destination of some registers depend on the current executing mode.

"depends"

> +     * The AArch32 registers 8-12 are only sync'd when we are in USR or FIQ
> +     * mode as they are the only modes where AArch64 registers map to these
> +     * registers.  In all other cases, the respective USR and FIQ banks are
> +     * sync'd.
> +     * The AArch32 registers 13 & 14 are sync'd depending on the execution mode
> +     * we are in.  If we are not in a given mode, the bank corresponding to the
> +     * AArch64 register is sync'd.
> +     */
> +    if (mode == ARM_CPU_MODE_USR) {
> +        for (i = 8; i < 15; i++) {
> +            env->regs[i] = env->xregs[i];
> +        }

Something is wrong here, because we don't seem to be writing
anything to env->regs[8..15] if mode is neither USR nor FIQ.

I think you should rearrange this function. The 32->64 sync
needs to write a value to all of xregs[0..30], and it's
ordered such that we basically write them all in that order,
which makes it clear we don't miss any cases.

This function, on the other hand, needs to write to
regs[0..15] and also to the banked_* registers, so
it should be written to update them in that order,
rather than in xregs order.

You may also find it easier to always update the banked_*
copies regardless of the current mode -- it's safe to
write the value to them always, it just might be ignored
if we're currently executing for that mode.

-- PMM

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

[Peter Maydell]

On 10 February 2015 at 10:50, 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>
>
> ---
>
> v3 -> v4
> - Add check that to make sure KVM64 is only being used on AArch64 family of
>   machines.
> - Relocate register sync to follow register fetches.
> - Refresh env->aarch64 prior to use.
>
> v2 -> v3
> - Conditionalize sync of 32-bit and 64-bit registers
> ---
>  target-arm/kvm64.c | 38 ++++++++++++++++++++++++++++++++++----
>  1 file changed, 34 insertions(+), 4 deletions(-)
>
> diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c
> index 033babf..789933e 100644
> --- a/target-arm/kvm64.c
> +++ b/target-arm/kvm64.c
> @@ -81,8 +81,8 @@ 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 &&
> +        object_dynamic_cast(cpu, TYPE_AARCH64_CPU)) {

You've changed an OR check (fail if this CPU isn't supported
by KVM at all, or if it's not an AArch64-capable CPU) into
an AND check...

>          fprintf(stderr, "KVM is not supported for this guest CPU type\n");
>          return -EINVAL;
>      }
> @@ -96,6 +96,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 +136,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.

"out to". Also, you're not syncing the 64 bit regs with the 32 bit ones,
you're copying the data from the 32-bit register state fields into
the 64 bit fields.

> +     */
> +    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 +172,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);
> @@ -242,7 +256,14 @@ int kvm_arch_get_registers(CPUState *cs)
>      if (ret) {
>          return ret;
>      }
> -    pstate_write(env, val);
> +
> +    env->aarch64 = ((val & PSTATE_nRW) == 0);
> +    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.
> @@ -256,6 +277,15 @@ int kvm_arch_get_registers(CPUState *cs)
>          return ret;
>      }
>
> +    /* If we are in AArch32 mode then we need to sync the AArch32 regs with the
> +     * incoming AArch64 regs received from 64-bit KVM.
> +     * We must perform this after all of the registers have been acquired from
> +     * the kernel.
> +     */
> +    if (!is_a64(env)) {
> +        aarch64_sync_64_to_32(env);
> +    }
> +
>      reg.id = AARCH64_CORE_REG(elr_el1);
>      reg.addr = (uintptr_t) &env->elr_el[1];
>      ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
> --
> 1.8.3.2
>

thanks
-- PMM

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

[Greg Bellows]

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

On Tue, Feb 10, 2015 at 10:03 PM, Peter Maydell <peter.maydell@linaro.org>
wrote:

> On 10 February 2015 at 10:50, 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>
> >
> > ---
> >
> > v3 -> v4
> > - Switch from using strtok to g_strsplit
> > - Add disablement of aarch64 option if KVM is not enabled.
> >
> > 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   |  5 ++++-
> >  target-arm/cpu64.c | 39 +++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 43 insertions(+), 1 deletion(-)
> >
> > diff --git a/target-arm/cpu.c b/target-arm/cpu.c
> > index d38af74..986f04c 100644
> > --- a/target-arm/cpu.c
> > +++ b/target-arm/cpu.c
> > @@ -544,13 +544,16 @@ 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_strsplit(cpu_model, ",", 1);
> > +    typename = g_strdup_printf("%s-" TYPE_ARM_CPU, cpuname[0]);
> >      oc = object_class_by_name(typename);
> > +    g_strfreev(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..d03f203 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,42 @@ 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);
> > +
> > +    /* At this time, this property is only allowed if KVM is enabled.
> This
> > +     * restriction allows us to avoid fixing up functionality that
> assumes a
> > +     * uniform execution state like do_interrupt.
> > +     */
> > +    if (!kvm_enabled()) {
> > +        error_setg(errp,
> > +                   "'aarch64' option only supported with
> '-enable-kvm'");
> > +        return;
>
> This check needs to go in the "we're unsetting the feature bit"
> code path, and we could make the error message a little clearer:
> "'aarch64' feature cannot be disabled unless KVM is enabled"
> (setting the feature to on is harmless and will work with TCG :-))
>
> ​Although it may be benign, the user may be doing something they think may
have an effect which is why I catch any case (not just off).  I see this as
being cleaner.​

As far as the message, the user does not really know about an "aarch64"
feature, that is internal to QEMU.  Given this is a command line option
error, I believe it makes more sense to keep it in that domain.  The
message as is describes the error in terms the command line options.  Maybe
a compromise such as below would work:

"aarch64 execution state can only be disabled when enabling KVM using the
'-enable-kvm' option


> > +    }
> > +
> > +    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);
>
> "Set on/off to enable/disable support for AArch64 execution
> state. Disable this to boot 32-bit guests in AArch32 state."
>

​I'll add suggested wording.​


>
> (Is that space at the end of your description deliberate or
> accidental?)
>

​Space is inadvertant, I'll remove.​


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

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

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

[Greg Bellows]

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

On Tue, Feb 10, 2015 at 10:16 PM, Peter Maydell <peter.maydell@linaro.org>
wrote:

> On 10 February 2015 at 10:50, 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>
> >
> > ---
> >
> > v3 -> v4
> > - Add check that to make sure KVM64 is only being used on AArch64 family
> of
> >   machines.
> > - Relocate register sync to follow register fetches.
> > - Refresh env->aarch64 prior to use.
> >
> > v2 -> v3
> > - Conditionalize sync of 32-bit and 64-bit registers
> > ---
> >  target-arm/kvm64.c | 38 ++++++++++++++++++++++++++++++++++----
> >  1 file changed, 34 insertions(+), 4 deletions(-)
> >
> > diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c
> > index 033babf..789933e 100644
> > --- a/target-arm/kvm64.c
> > +++ b/target-arm/kvm64.c
> > @@ -81,8 +81,8 @@ 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 &&
> > +        object_dynamic_cast(cpu, TYPE_AARCH64_CPU)) {
>
> You've changed an OR check (fail if this CPU isn't supported
> by KVM at all, or if it's not an AArch64-capable CPU) into
> an AND check...
>

​Bah... that was inadvertent, will fix.​


>
> >          fprintf(stderr, "KVM is not supported for this guest CPU
> type\n");
> >          return -EINVAL;
> >      }
> > @@ -96,6 +96,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 +136,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.
>
> "out to". Also, you're not syncing the 64 bit regs with the 32 bit ones,
> you're copying the data from the 32-bit register state fields into
> the 64 bit fields.
>

​Fixed in next version.​


>
> > +     */
> > +    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 +172,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);
> > @@ -242,7 +256,14 @@ int kvm_arch_get_registers(CPUState *cs)
> >      if (ret) {
> >          return ret;
> >      }
> > -    pstate_write(env, val);
> > +
> > +    env->aarch64 = ((val & PSTATE_nRW) == 0);
> > +    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.
> > @@ -256,6 +277,15 @@ int kvm_arch_get_registers(CPUState *cs)
> >          return ret;
> >      }
> >
> > +    /* If we are in AArch32 mode then we need to sync the AArch32 regs
> with the
> > +     * incoming AArch64 regs received from 64-bit KVM.
> > +     * We must perform this after all of the registers have been
> acquired from
> > +     * the kernel.
> > +     */
> > +    if (!is_a64(env)) {
> > +        aarch64_sync_64_to_32(env);
> > +    }
> > +
> >      reg.id = AARCH64_CORE_REG(elr_el1);
> >      reg.addr = (uintptr_t) &env->elr_el[1];
> >      ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
> > --
> > 1.8.3.2
> >
>
> thanks
> -- PMM
>

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

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

[Peter Maydell]

On 11 February 2015 at 04:27, Greg Bellows <greg.bellows@linaro.org> wrote:
>
>
> On Tue, Feb 10, 2015 at 10:03 PM, Peter Maydell <peter.maydell@linaro.org>
> wrote:
>>
>> On 10 February 2015 at 10:50, Greg Bellows <greg.bellows@linaro.org>
>> wrote:
>> > +    if (!kvm_enabled()) {
>> > +        error_setg(errp,
>> > +                   "'aarch64' option only supported with
>> > '-enable-kvm'");
>> > +        return;
>>
>> This check needs to go in the "we're unsetting the feature bit"
>> code path, and we could make the error message a little clearer:
>> "'aarch64' feature cannot be disabled unless KVM is enabled"
>> (setting the feature to on is harmless and will work with TCG :-))
>>
> Although it may be benign, the user may be doing something they think may
> have an effect which is why I catch any case (not just off).  I see this as
> being cleaner.

OK; I don't feel strongly about this.

> As far as the message, the user does not really know about an "aarch64"
> feature, that is internal to QEMU.  Given this is a command line option
> error, I believe it makes more sense to keep it in that domain.

The point is that it's not a command line option. That would be
"qemu-system-aarch64 -aarch64", but what we actually have is
"-cpu whatever,-aarch64=off", which is a feature enable/disable.
You could say "Setting CPU property 'aarch64' to off is not
valid unless KVM is enabled" if you prefer that wording.

> The message
> as is describes the error in terms the command line options.  Maybe a
> compromise such as below would work:
>
> "aarch64 execution state can only be disabled when enabling KVM using the
> '-enable-kvm' option

This seems backwards, because it isn't precise about what
the user has done wrong on their command line (asked us to
disable the 'aarch64' cpu feature) but is precise about
something that's not very important (the option you can use
to enable KVM, and in fact you can enable KVM via other
command line syntax too).

Also, if you're referring to the CPU state, that needs capitals:
AArch64. Only use lowercase if you're talking about the
user-facing feature-switch name (in which case it should
go in quotes).

thanks
-- PMM

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

[Greg Bellows]

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

On Tue, Feb 10, 2015 at 10:13 PM, Peter Maydell <peter.maydell@linaro.org>
wrote:

> On 10 February 2015 at 10:50, 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>
> >
> > ---
> >
> > v3 -> v4
> > - Rework sync routines to cover various exception levels
> > - Move sync routines to helper.c
> >
> > v2 -> v3
> > - Conditionalize interrupt handler update of aarch64.
> > ---
> >  target-arm/cpu.h        |   2 +
> >  target-arm/helper-a64.c |   5 +-
> >  target-arm/helper.c     | 181
> ++++++++++++++++++++++++++++++++++++++++++++++++
> >  target-arm/op_helper.c  |   6 +-
> >  4 files changed, 186 insertions(+), 8 deletions(-)
> >
> > diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> > index 1830a12..11845a6 100644
> > --- a/target-arm/cpu.h
> > +++ b/target-arm/cpu.h
> > @@ -495,6 +495,8 @@ typedef struct CPUARMState {
> >  ARMCPU *cpu_arm_init(const char *cpu_model);
> >  int cpu_arm_exec(CPUARMState *s);
> >  uint32_t do_arm_semihosting(CPUARMState *env);
> > +void aarch64_sync_32_to_64(CPUARMState *env);
> > +void aarch64_sync_64_to_32(CPUARMState *env);
> >
> >  static inline bool is_a64(CPUARMState *env)
> >  {
> > diff --git a/target-arm/helper-a64.c b/target-arm/helper-a64.c
> > index 8aa40e9..7e0d038 100644
> > --- a/target-arm/helper-a64.c
> > +++ b/target-arm/helper-a64.c
> > @@ -466,7 +466,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) {
> > @@ -530,9 +529,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/helper.c b/target-arm/helper.c
> > index 1a1a005..c1d6764 100644
> > --- a/target-arm/helper.c
> > +++ b/target-arm/helper.c
> > @@ -4096,6 +4096,15 @@ unsigned int arm_excp_target_el(CPUState *cs,
> unsigned int excp_idx)
> >      return 1;
> >  }
> >
> > +void aarch64_sync_64_to_32(CPUARMState *env)
> > +{
> > +    int i;
> > +
> > +    for (i = 0; i < 15; i++) {
> > +        env->regs[i] = env->xregs[i];
> > +    }
> > +}
>
> This is inside CONFIG_USER_ONLY, right? Is it called at all?
> If so, when?
>

​The exception_return helper calls the function so I had to either add a
USER_CONFIG version of wrap the call in exception return with
CONFIG_USER_ONLY.  I chose the former, but either would work.  As you would
already know, the exception_return helper is likely not getting called in a
USER_ONLY build.

>
> > +
> >  #else
> >
> >  /* Map CPU modes onto saved register banks.  */
> > @@ -4425,6 +4434,178 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
> >      env->thumb = addr & 1;
> >  }
> >
> > +/* Function used to synchronize QEMU's AArch64 register set with AArch32
> > + * register set.  This is necessary when switching between AArch32 and
> AArch64
> > + * execution state.
>
> This is a bit vague...
>

​I'll elaborate.​


>
> > + */
> > +void aarch64_sync_32_to_64(CPUARMState *env)
> > +{
> > +    int i;
> > +    uint32_t mode = env->uncached_cpsr & CPSR_M;
> > +
> > +    /* We can blanket copy R[0:7] to X[0:7] */
> > +    for (i = 0; i < 8; i++) {
> > +        env->xregs[i] = env->regs[i];
> > +    }
> > +
> > +    /* The latest copy of some registers depend on the current
> executing mode.
>
> "depends"
>

Fixed in next version.

>
> > +     * The general purpose copy is used when the current mode
> corresponds to
> > +     * the mapping for the given register.  Otherwise, the banked copy
> > +     * corresponding to the register mapping is used.
> > +     */
> > +    if (mode == 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)];
> > +    }
> > +
> > +    if (mode == ARM_CPU_MODE_HYP) {
> > +        env->xregs[15] = env->regs[13];
> > +    } else {
> > +        env->xregs[15] = env->banked_r13[bank_number(ARM_CPU_MODE_HYP)];
> > +    }
> > +
> > +    if (mode == ARM_CPU_MODE_IRQ) {
> > +        env->xregs[16] = env->regs[13];
> > +        env->xregs[17] = env->regs[14];
> > +    } else {
> > +        env->xregs[16] = env->banked_r13[bank_number(ARM_CPU_MODE_IRQ)];
> > +        env->xregs[17] = env->banked_r14[bank_number(ARM_CPU_MODE_IRQ)];
> > +    }
> > +
> > +    if (mode == ARM_CPU_MODE_SVC) {
> > +        env->xregs[18] = env->regs[13];
> > +        env->xregs[19] = env->regs[14];
> > +    } else {
> > +        env->xregs[18] = env->banked_r13[bank_number(ARM_CPU_MODE_SVC)];
> > +        env->xregs[19] = env->banked_r14[bank_number(ARM_CPU_MODE_SVC)];
> > +    }
> > +
> > +    if (mode == ARM_CPU_MODE_ABT) {
> > +        env->xregs[20] = env->regs[13];
> > +        env->xregs[21] = env->regs[14];
> > +    } else {
> > +        env->xregs[20] = env->banked_r13[bank_number(ARM_CPU_MODE_ABT)];
> > +        env->xregs[21] = env->banked_r14[bank_number(ARM_CPU_MODE_ABT)];
> > +    }
> > +
> > +    if (mode == ARM_CPU_MODE_UND) {
> > +        env->xregs[22] = env->regs[13];
> > +        env->xregs[23] = env->regs[14];
> > +    } else {
> > +        env->xregs[22] = env->banked_r13[bank_number(ARM_CPU_MODE_UND)];
> > +        env->xregs[23] = env->banked_r14[bank_number(ARM_CPU_MODE_UND)];
> > +    }
> > +
> > +    if (mode == ARM_CPU_MODE_FIQ) {
> > +        for (i = 24; i < 31; i++) {
> > +            env->xregs[i] = env->regs[i - 16];   /* X[24:30] -> R[8:14]
> */
> > +        }
> > +    } else {
> > +        for (i = 24; i < 29; i++) {
> > +            env->xregs[i] = env->fiq_regs[i - 24];
> > +        }
> > +        env->xregs[29] = env->banked_r13[bank_number(ARM_CPU_MODE_FIQ)];
> > +        env->xregs[30] = env->banked_r14[bank_number(ARM_CPU_MODE_FIQ)];
> > +    }
> > +
> > +    env->pc = env->regs[15];
> > +}
> > +
> > +/* Function used to synchronize QEMU's AArch32 register set with AArch64
> > + * register set.  This is necessary when switching between AArch32 and
> AArch64
> > + * execution state.
> > + */
> > +void aarch64_sync_64_to_32(CPUARMState *env)
> > +{
> > +    int i;
> > +    uint32_t mode = env->uncached_cpsr & CPSR_M;
> > +
> > +    /* We can blanket copy X[0:7] to R[0:7] */
> > +    for (i = 0; i < 8; i++) {
> > +        env->regs[i] = env->xregs[i];
> > +    }
> > +
> > +    /* The destination of some registers depend on the current
> executing mode.
>
> "depends"
>

​Fixed in next version.​


>
> > +     * The AArch32 registers 8-12 are only sync'd when we are in USR or
> FIQ
> > +     * mode as they are the only modes where AArch64 registers map to
> these
> > +     * registers.  In all other cases, the respective USR and FIQ banks
> are
> > +     * sync'd.
> > +     * The AArch32 registers 13 & 14 are sync'd depending on the
> execution mode
> > +     * we are in.  If we are not in a given mode, the bank
> corresponding to the
> > +     * AArch64 register is sync'd.
> > +     */
> > +    if (mode == ARM_CPU_MODE_USR) {
> > +        for (i = 8; i < 15; i++) {
> > +            env->regs[i] = env->xregs[i];
> > +        }
>
> Something is wrong here, because we don't seem to be writing
> anything to env->regs[8..15] if mode is neither USR nor FIQ.
>
> ​I wrestled with this myself.  As I understand it, nothing maps to
regs[8..15] unless we are in USR or FIQ, which I covered.  This based on
the ARM ARM xregs[8:15] are defined to specifically map to USR,   Outside
of the these modes, what should be copied to regs[8..15]?
​


> I think you should rearrange this function. The 32->64 sync
> needs to write a value to all of xregs[0..30], and it's
> ordered such that we basically write them all in that order,
> which makes it clear we don't miss any cases.
>
> This function, on the other hand, needs to write to
> regs[0..15] and also to the banked_* registers, so
> it should be written to update them in that order,
> rather than in xregs order.
>
> You may also find it easier to always update the banked_*
> copies regardless of the current mode -- it's safe to
> write the value to them always, it just might be ignored
> if we're currently executing for that mode.
>
> -- PMM
>

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

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

[Peter Maydell]

On 11 February 2015 at 06:08, Greg Bellows <greg.bellows@linaro.org> wrote:
>
>
> On Tue, Feb 10, 2015 at 10:13 PM, Peter Maydell <peter.maydell@linaro.org>
> wrote:
>>
>> On 10 February 2015 at 10:50, Greg Bellows <greg.bellows@linaro.org>
>> wrote:
>> > +void aarch64_sync_64_to_32(CPUARMState *env)
>> > +{
>> > +    int i;
>> > +
>> > +    for (i = 0; i < 15; i++) {
>> > +        env->regs[i] = env->xregs[i];
>> > +    }
>> > +}
>>
>> This is inside CONFIG_USER_ONLY, right? Is it called at all?
>> If so, when?
>
>
> The exception_return helper calls the function so I had to either add a
> USER_CONFIG version of wrap the call in exception return with
> CONFIG_USER_ONLY.  I chose the former, but either would work.  As you would
> already know, the exception_return helper is likely not getting called in a
> USER_ONLY build.

Right, so make it just g_assert_not_reached().

>>
>> > +     * The AArch32 registers 8-12 are only sync'd when we are in USR or
>> > FIQ
>> > +     * mode as they are the only modes where AArch64 registers map to
>> > these
>> > +     * registers.  In all other cases, the respective USR and FIQ banks
>> > are
>> > +     * sync'd.
>> > +     * The AArch32 registers 13 & 14 are sync'd depending on the
>> > execution mode
>> > +     * we are in.  If we are not in a given mode, the bank
>> > corresponding to the
>> > +     * AArch64 register is sync'd.
>> > +     */
>> > +    if (mode == ARM_CPU_MODE_USR) {
>> > +        for (i = 8; i < 15; i++) {
>> > +            env->regs[i] = env->xregs[i];
>> > +        }
>>
>> Something is wrong here, because we don't seem to be writing
>> anything to env->regs[8..15] if mode is neither USR nor FIQ.
>>
> I wrestled with this myself.  As I understand it, nothing maps to
> regs[8..15] unless we are in USR or FIQ, which I covered.  This based on the
> ARM ARM xregs[8:15] are defined to specifically map to USR,   Outside of the
> these modes, what should be copied to regs[8..15]?

There is always *something* that is the architecturally defined
state for all the AArch32 registers. Otherwise a 64-bit hypervisor
would be unable to interrupt and restart a 32-bit guest.
(And all the registers r0..r15 exist in all AArch32 modes;
the question is just whether they're banked registers or
shared with some other mode).

All modes other than FIQ use the USR registers for r0..r12.
(See the v8 ARM ARM fig G1-3, and note the footnote about the
meaning of empty cells in the table.) r13 is the appropriate
sp for the mode (noting that system mode shares with usr),
and r14 ditto (but system and hyp share with usr).

-- PMM

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

[Alexander Spyridakis]

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

On 10 February 2015 at 11:50, Greg Bellows <greg.bellows@linaro.org> 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 ...


Hello,

Tested this series on the Juno board with an ARMv7 bare metal binary and it
worked as expected.

Thanks.

Tested-by: Alexander Spyridakis <a.spyridakis@virtualopensystems.com>

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