[Qemu-devel] [PATCH 0/2] Fix the last Hyp mode bug and turn it on for A7, A15

[Qemu-devel] [PATCH 0/2] Fix the last Hyp mode bug and turn it on for A7, A15

[Peter Maydell]

This patchset fixes the last serious bug in our implementation
of Hyp mode (aka EL2 for AArch32), and turns the feature bit
on for the Cortex-A7 and Cortex-A15 CPUs.

The bug is that Hyp mode is an exception to the previous
general rule that every AArch32 mode (except SYS, which
always shares with USR) has its own banked r13, r14 and
SPSR. Instead Hyp has a banked r13 and SPSR, but r14 is
shared with USR and SYS. We were accidentally implementing
it as banked, which results in remarkably nonobvious
failure modes.

With this fix, I can boot an AArch32 guest that uses KVM to
boot an AArch32 nested guest, and I can also boot an L4Re/
Fiasco guest successfully.

Not entirely sure what to do about this for 3.1 -- maybe
put in the bugfix patch but hold off on actually setting
the feature bit til 4.0?

thanks
-- PMM

Peter Maydell (2):
  target/arm: Hyp mode R14 is shared with User and System
  target/arm/cpu: Give Cortex-A15 and -A7 the EL2 feature

 target/arm/internals.h | 16 ++++++++++++++++
 target/arm/cpu.c       |  2 ++
 target/arm/helper.c    | 29 +++++++++++++++--------------
 target/arm/kvm32.c     |  4 ++--
 target/arm/op_helper.c |  2 +-
 5 files changed, 36 insertions(+), 17 deletions(-)

-- 
2.19.1

[Qemu-devel] [PATCH 1/2] target/arm: Hyp mode R14 is shared with User and System

[Peter Maydell]

Hyp mode is an exception to the general rule that each AArch32
mode has its own r13, r14 and SPSR -- it has a banked r13 and
SPSR but shares its r14 with User and System mode. We were
incorrectly implementing it as banked, which meant that on
entry to Hyp mode r14 was 0 rather than the USR/SYS r14.

We provide a new function r14_bank_number() which is like
the existing bank_number() but provides the index into
env->banked_r14[]; bank_number() provides the index to use
for env->banked_r13[] and env->banked_cpsr[].

All the points in the code that were using bank_number()
to index into env->banked_r14[] are updated for consintency:
 * switch_mode() -- this is the only place where we fix
   an actual bug
 * aarch64_sync_32_to_64() and aarch64_sync_64_to_32():
   no behavioural change as we already special-cased Hyp R14
 * kvm32.c: no behavioural change since the guest can't ever
   be in Hyp mode, but conceptually the right thing to do
 * msr_banked()/mrs_banked(): we can never get to the case
   that accesses banked_r14[] with tgtmode == ARM_CPU_MODE_HYP,
   so no behavioural change

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/internals.h | 16 ++++++++++++++++
 target/arm/helper.c    | 29 +++++++++++++++--------------
 target/arm/kvm32.c     |  4 ++--
 target/arm/op_helper.c |  2 +-
 4 files changed, 34 insertions(+), 17 deletions(-)

diff --git a/target/arm/internals.h b/target/arm/internals.h
index 6c2bb2deebd..e5341f21f6f 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -145,6 +145,22 @@ static inline int bank_number(int mode)
     g_assert_not_reached();
 }
 
+/**
+ * r14_bank_number: Map CPU mode onto register bank for r14
+ *
+ * Given an AArch32 CPU mode, return the index into the saved register
+ * banks to use for the R14 (LR) in that mode. This is the same as
+ * bank_number(), except for the special case of Hyp mode, where
+ * R14 is shared with USR and SYS, unlike its R13 and SPSR.
+ * This should be used as the index into env->banked_r14[], and
+ * bank_number() used for the index into env->banked_r13[] and
+ * env->banked_spsr[].
+ */
+static inline int r14_bank_number(int mode)
+{
+    return (mode == ARM_CPU_MODE_HYP) ? BANK_USRSYS : bank_number(mode);
+}
+
 void arm_cpu_register_gdb_regs_for_features(ARMCPU *cpu);
 void arm_translate_init(void);
 
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 96301930cc8..6fb1ddc5506 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -6455,13 +6455,14 @@ static void switch_mode(CPUARMState *env, int mode)
 
     i = bank_number(old_mode);
     env->banked_r13[i] = env->regs[13];
-    env->banked_r14[i] = env->regs[14];
     env->banked_spsr[i] = env->spsr;
 
     i = bank_number(mode);
     env->regs[13] = env->banked_r13[i];
-    env->regs[14] = env->banked_r14[i];
     env->spsr = env->banked_spsr[i];
+
+    env->banked_r14[r14_bank_number(old_mode)] = env->regs[14];
+    env->regs[14] = env->banked_r14[r14_bank_number(mode)];
 }
 
 /* Physical Interrupt Target EL Lookup Table
@@ -8040,7 +8041,7 @@ void aarch64_sync_32_to_64(CPUARMState *env)
         if (mode == ARM_CPU_MODE_HYP) {
             env->xregs[14] = env->regs[14];
         } else {
-            env->xregs[14] = env->banked_r14[bank_number(ARM_CPU_MODE_USR)];
+            env->xregs[14] = env->banked_r14[r14_bank_number(ARM_CPU_MODE_USR)];
         }
     }
 
@@ -8054,7 +8055,7 @@ void aarch64_sync_32_to_64(CPUARMState *env)
         env->xregs[16] = env->regs[14];
         env->xregs[17] = env->regs[13];
     } else {
-        env->xregs[16] = env->banked_r14[bank_number(ARM_CPU_MODE_IRQ)];
+        env->xregs[16] = env->banked_r14[r14_bank_number(ARM_CPU_MODE_IRQ)];
         env->xregs[17] = env->banked_r13[bank_number(ARM_CPU_MODE_IRQ)];
     }
 
@@ -8062,7 +8063,7 @@ void aarch64_sync_32_to_64(CPUARMState *env)
         env->xregs[18] = env->regs[14];
         env->xregs[19] = env->regs[13];
     } else {
-        env->xregs[18] = env->banked_r14[bank_number(ARM_CPU_MODE_SVC)];
+        env->xregs[18] = env->banked_r14[r14_bank_number(ARM_CPU_MODE_SVC)];
         env->xregs[19] = env->banked_r13[bank_number(ARM_CPU_MODE_SVC)];
     }
 
@@ -8070,7 +8071,7 @@ void aarch64_sync_32_to_64(CPUARMState *env)
         env->xregs[20] = env->regs[14];
         env->xregs[21] = env->regs[13];
     } else {
-        env->xregs[20] = env->banked_r14[bank_number(ARM_CPU_MODE_ABT)];
+        env->xregs[20] = env->banked_r14[r14_bank_number(ARM_CPU_MODE_ABT)];
         env->xregs[21] = env->banked_r13[bank_number(ARM_CPU_MODE_ABT)];
     }
 
@@ -8078,7 +8079,7 @@ void aarch64_sync_32_to_64(CPUARMState *env)
         env->xregs[22] = env->regs[14];
         env->xregs[23] = env->regs[13];
     } else {
-        env->xregs[22] = env->banked_r14[bank_number(ARM_CPU_MODE_UND)];
+        env->xregs[22] = env->banked_r14[r14_bank_number(ARM_CPU_MODE_UND)];
         env->xregs[23] = env->banked_r13[bank_number(ARM_CPU_MODE_UND)];
     }
 
@@ -8095,7 +8096,7 @@ void aarch64_sync_32_to_64(CPUARMState *env)
             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->xregs[30] = env->banked_r14[r14_bank_number(ARM_CPU_MODE_FIQ)];
     }
 
     env->pc = env->regs[15];
@@ -8145,7 +8146,7 @@ void aarch64_sync_64_to_32(CPUARMState *env)
         if (mode == ARM_CPU_MODE_HYP) {
             env->regs[14] = env->xregs[14];
         } else {
-            env->banked_r14[bank_number(ARM_CPU_MODE_USR)] = env->xregs[14];
+            env->banked_r14[r14_bank_number(ARM_CPU_MODE_USR)] = env->xregs[14];
         }
     }
 
@@ -8159,7 +8160,7 @@ void aarch64_sync_64_to_32(CPUARMState *env)
         env->regs[14] = env->xregs[16];
         env->regs[13] = env->xregs[17];
     } else {
-        env->banked_r14[bank_number(ARM_CPU_MODE_IRQ)] = env->xregs[16];
+        env->banked_r14[r14_bank_number(ARM_CPU_MODE_IRQ)] = env->xregs[16];
         env->banked_r13[bank_number(ARM_CPU_MODE_IRQ)] = env->xregs[17];
     }
 
@@ -8167,7 +8168,7 @@ void aarch64_sync_64_to_32(CPUARMState *env)
         env->regs[14] = env->xregs[18];
         env->regs[13] = env->xregs[19];
     } else {
-        env->banked_r14[bank_number(ARM_CPU_MODE_SVC)] = env->xregs[18];
+        env->banked_r14[r14_bank_number(ARM_CPU_MODE_SVC)] = env->xregs[18];
         env->banked_r13[bank_number(ARM_CPU_MODE_SVC)] = env->xregs[19];
     }
 
@@ -8175,7 +8176,7 @@ void aarch64_sync_64_to_32(CPUARMState *env)
         env->regs[14] = env->xregs[20];
         env->regs[13] = env->xregs[21];
     } else {
-        env->banked_r14[bank_number(ARM_CPU_MODE_ABT)] = env->xregs[20];
+        env->banked_r14[r14_bank_number(ARM_CPU_MODE_ABT)] = env->xregs[20];
         env->banked_r13[bank_number(ARM_CPU_MODE_ABT)] = env->xregs[21];
     }
 
@@ -8183,7 +8184,7 @@ void aarch64_sync_64_to_32(CPUARMState *env)
         env->regs[14] = env->xregs[22];
         env->regs[13] = env->xregs[23];
     } else {
-        env->banked_r14[bank_number(ARM_CPU_MODE_UND)] = env->xregs[22];
+        env->banked_r14[r14_bank_number(ARM_CPU_MODE_UND)] = env->xregs[22];
         env->banked_r13[bank_number(ARM_CPU_MODE_UND)] = env->xregs[23];
     }
 
@@ -8200,7 +8201,7 @@ void aarch64_sync_64_to_32(CPUARMState *env)
             env->fiq_regs[i - 24] = env->xregs[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];
+        env->banked_r14[r14_bank_number(ARM_CPU_MODE_FIQ)] = env->xregs[30];
     }
 
     env->regs[15] = env->pc;
diff --git a/target/arm/kvm32.c b/target/arm/kvm32.c
index 0f1e94c7b5e..cb3fb73a961 100644
--- a/target/arm/kvm32.c
+++ b/target/arm/kvm32.c
@@ -318,8 +318,8 @@ int kvm_arch_put_registers(CPUState *cs, int level)
         memcpy(env->usr_regs, env->regs + 8, 5 * sizeof(uint32_t));
     }
     env->banked_r13[bn] = env->regs[13];
-    env->banked_r14[bn] = env->regs[14];
     env->banked_spsr[bn] = env->spsr;
+    env->banked_r14[r14_bank_number(mode)] = env->regs[14];
 
     /* Now we can safely copy stuff down to the kernel */
     for (i = 0; i < ARRAY_SIZE(regs); i++) {
@@ -430,8 +430,8 @@ int kvm_arch_get_registers(CPUState *cs)
         memcpy(env->regs + 8, env->usr_regs, 5 * sizeof(uint32_t));
     }
     env->regs[13] = env->banked_r13[bn];
-    env->regs[14] = env->banked_r14[bn];
     env->spsr = env->banked_spsr[bn];
+    env->regs[14] = env->banked_r14[r14_bank_number(mode)];
 
     /* VFP registers */
     r.id = KVM_REG_ARM | KVM_REG_SIZE_U64 | KVM_REG_ARM_VFP;
diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c
index 90741f6331d..2b62c53f5b5 100644
--- a/target/arm/op_helper.c
+++ b/target/arm/op_helper.c
@@ -694,7 +694,7 @@ void HELPER(msr_banked)(CPUARMState *env, uint32_t value, uint32_t tgtmode,
         env->banked_r13[bank_number(tgtmode)] = value;
         break;
     case 14:
-        env->banked_r14[bank_number(tgtmode)] = value;
+        env->banked_r14[r14_bank_number(tgtmode)] = value;
         break;
     case 8 ... 12:
         switch (tgtmode) {
-- 
2.19.1

[Qemu-devel] [PATCH 2/2] target/arm/cpu: Give Cortex-A15 and -A7 the EL2 feature

[Peter Maydell]

The Cortex-A15 and Cortex-A7 both have EL2; now we've implemented
it properly we can enable the feature bit.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/cpu.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 784a4c2dfcc..b7185234d85 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1587,6 +1587,7 @@ static void cortex_a7_initfn(Object *obj)
     set_feature(&cpu->env, ARM_FEATURE_GENERIC_TIMER);
     set_feature(&cpu->env, ARM_FEATURE_DUMMY_C15_REGS);
     set_feature(&cpu->env, ARM_FEATURE_CBAR_RO);
+    set_feature(&cpu->env, ARM_FEATURE_EL2);
     set_feature(&cpu->env, ARM_FEATURE_EL3);
     cpu->kvm_target = QEMU_KVM_ARM_TARGET_CORTEX_A7;
     cpu->midr = 0x410fc075;
@@ -1633,6 +1634,7 @@ static void cortex_a15_initfn(Object *obj)
     set_feature(&cpu->env, ARM_FEATURE_GENERIC_TIMER);
     set_feature(&cpu->env, ARM_FEATURE_DUMMY_C15_REGS);
     set_feature(&cpu->env, ARM_FEATURE_CBAR_RO);
+    set_feature(&cpu->env, ARM_FEATURE_EL2);
     set_feature(&cpu->env, ARM_FEATURE_EL3);
     cpu->kvm_target = QEMU_KVM_ARM_TARGET_CORTEX_A15;
     cpu->midr = 0x412fc0f1;
-- 
2.19.1

Re: [Qemu-devel] [Qemu-arm] [PATCH 1/2] target/arm: Hyp mode R14 is shared with User and System

[Peter Maydell]

On 9 November 2018 at 17:35, Peter Maydell <peter.maydell@linaro.org> wrote:
> Hyp mode is an exception to the general rule that each AArch32
> mode has its own r13, r14 and SPSR -- it has a banked r13 and
> SPSR but shares its r14 with User and System mode. We were
> incorrectly implementing it as banked, which meant that on
> entry to Hyp mode r14 was 0 rather than the USR/SYS r14.
>
> We provide a new function r14_bank_number() which is like
> the existing bank_number() but provides the index into
> env->banked_r14[]; bank_number() provides the index to use
> for env->banked_r13[] and env->banked_cpsr[].
>
> All the points in the code that were using bank_number()
> to index into env->banked_r14[] are updated for consintency:
>  * switch_mode() -- this is the only place where we fix
>    an actual bug
>  * aarch64_sync_32_to_64() and aarch64_sync_64_to_32():
>    no behavioural change as we already special-cased Hyp R14
>  * kvm32.c: no behavioural change since the guest can't ever
>    be in Hyp mode, but conceptually the right thing to do
>  * msr_banked()/mrs_banked(): we can never get to the case
>    that accesses banked_r14[] with tgtmode == ARM_CPU_MODE_HYP,
>    so no behavioural change
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/internals.h | 16 ++++++++++++++++
>  target/arm/helper.c    | 29 +++++++++++++++--------------
>  target/arm/kvm32.c     |  4 ++--
>  target/arm/op_helper.c |  2 +-
>  4 files changed, 34 insertions(+), 17 deletions(-)

Rats, this bit accidentally didn't make it into this patch:

diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c
index 2b62c53f5b5..eb6fb82fb81 100644
--- a/target/arm/op_helper.c
+++ b/target/arm/op_helper.c
@@ -725,7 +725,7 @@ uint32_t HELPER(mrs_banked)(CPUARMState *env,
uint32_t tgtmode, uint32_t regno)
     case 13:
         return env->banked_r13[bank_number(tgtmode)];
     case 14:
-        return env->banked_r14[bank_number(tgtmode)];
+        return env->banked_r14[r14_bank_number(tgtmode)];
     case 8 ... 12:
         switch (tgtmode) {
         case ARM_CPU_MODE_USR:


(it's one of the "no behavioural change" bits).

thanks
-- PMM

Re: [Qemu-devel] [Qemu-arm] [PATCH 1/2] target/arm: Hyp mode R14 is shared with User and System

[Philippe Mathieu-Daudé]

On 11/9/18 7:15 PM, Peter Maydell wrote:
> On 9 November 2018 at 17:35, Peter Maydell <peter.maydell@linaro.org> wrote:
>> Hyp mode is an exception to the general rule that each AArch32
>> mode has its own r13, r14 and SPSR -- it has a banked r13 and
>> SPSR but shares its r14 with User and System mode. We were
>> incorrectly implementing it as banked, which meant that on
>> entry to Hyp mode r14 was 0 rather than the USR/SYS r14.
>>
>> We provide a new function r14_bank_number() which is like
>> the existing bank_number() but provides the index into
>> env->banked_r14[]; bank_number() provides the index to use
>> for env->banked_r13[] and env->banked_cpsr[].
>>
>> All the points in the code that were using bank_number()
>> to index into env->banked_r14[] are updated for consintency:
>>  * switch_mode() -- this is the only place where we fix
>>    an actual bug
>>  * aarch64_sync_32_to_64() and aarch64_sync_64_to_32():
>>    no behavioural change as we already special-cased Hyp R14
>>  * kvm32.c: no behavioural change since the guest can't ever
>>    be in Hyp mode, but conceptually the right thing to do
>>  * msr_banked()/mrs_banked(): we can never get to the case
>>    that accesses banked_r14[] with tgtmode == ARM_CPU_MODE_HYP,
>>    so no behavioural change
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>>  target/arm/internals.h | 16 ++++++++++++++++
>>  target/arm/helper.c    | 29 +++++++++++++++--------------
>>  target/arm/kvm32.c     |  4 ++--
>>  target/arm/op_helper.c |  2 +-
>>  4 files changed, 34 insertions(+), 17 deletions(-)
> 
> Rats, this bit accidentally didn't make it into this patch:
> 
> diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c
> index 2b62c53f5b5..eb6fb82fb81 100644
> --- a/target/arm/op_helper.c
> +++ b/target/arm/op_helper.c
> @@ -725,7 +725,7 @@ uint32_t HELPER(mrs_banked)(CPUARMState *env,
> uint32_t tgtmode, uint32_t regno)
>      case 13:
>          return env->banked_r13[bank_number(tgtmode)];
>      case 14:
> -        return env->banked_r14[bank_number(tgtmode)];
> +        return env->banked_r14[r14_bank_number(tgtmode)];
>      case 8 ... 12:
>          switch (tgtmode) {
>          case ARM_CPU_MODE_USR:
> 
> 
> (it's one of the "no behavioural change" bits).

With this hunk:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Thanks for the detailed patch description,

Phil.

> thanks
> -- PMM
> 

Re: [Qemu-devel] [Qemu-arm] [PATCH 2/2] target/arm/cpu: Give Cortex-A15 and -A7 the EL2 feature

[Philippe Mathieu-Daudé]

On Fri, Nov 9, 2018 at 6:42 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> The Cortex-A15 and Cortex-A7 both have EL2; now we've implemented

still PL2 there :)

> it properly we can enable the feature bit.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  target/arm/cpu.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 784a4c2dfcc..b7185234d85 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -1587,6 +1587,7 @@ static void cortex_a7_initfn(Object *obj)
>      set_feature(&cpu->env, ARM_FEATURE_GENERIC_TIMER);
>      set_feature(&cpu->env, ARM_FEATURE_DUMMY_C15_REGS);
>      set_feature(&cpu->env, ARM_FEATURE_CBAR_RO);
> +    set_feature(&cpu->env, ARM_FEATURE_EL2);
>      set_feature(&cpu->env, ARM_FEATURE_EL3);
>      cpu->kvm_target = QEMU_KVM_ARM_TARGET_CORTEX_A7;
>      cpu->midr = 0x410fc075;
> @@ -1633,6 +1634,7 @@ static void cortex_a15_initfn(Object *obj)
>      set_feature(&cpu->env, ARM_FEATURE_GENERIC_TIMER);
>      set_feature(&cpu->env, ARM_FEATURE_DUMMY_C15_REGS);
>      set_feature(&cpu->env, ARM_FEATURE_CBAR_RO);
> +    set_feature(&cpu->env, ARM_FEATURE_EL2);
>      set_feature(&cpu->env, ARM_FEATURE_EL3);
>      cpu->kvm_target = QEMU_KVM_ARM_TARGET_CORTEX_A15;
>      cpu->midr = 0x412fc0f1;
> --
> 2.19.1
>
>

Re: [Qemu-devel] [Qemu-arm] [PATCH 0/2] Fix the last Hyp mode bug and turn it on for A7, A15

[Philippe Mathieu-Daudé]

Hi Peter,

On Fri, Nov 9, 2018 at 6:36 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> This patchset fixes the last serious bug in our implementation
> of Hyp mode (aka EL2 for AArch32), and turns the feature bit
> on for the Cortex-A7 and Cortex-A15 CPUs.
>
> The bug is that Hyp mode is an exception to the previous
> general rule that every AArch32 mode (except SYS, which
> always shares with USR) has its own banked r13, r14 and
> SPSR. Instead Hyp has a banked r13 and SPSR, but r14 is
> shared with USR and SYS. We were accidentally implementing
> it as banked, which results in remarkably nonobvious
> failure modes.
>
> With this fix, I can boot an AArch32 guest that uses KVM to
> boot an AArch32 nested guest, and I can also boot an L4Re/
> Fiasco guest successfully.

Nice!
More acceptance tests to add :)

>
> Not entirely sure what to do about this for 3.1 -- maybe
> put in the bugfix patch but hold off on actually setting
> the feature bit til 4.0?

The bugfix surely fits.

Do you think enabling the feature isn't well tested and might trigger
unexpected side effects?
It is certainly not tested... except by you. But if you include it, it
might be more tested :)

>
> thanks
> -- PMM
>
> Peter Maydell (2):
>   target/arm: Hyp mode R14 is shared with User and System
>   target/arm/cpu: Give Cortex-A15 and -A7 the EL2 feature
>
>  target/arm/internals.h | 16 ++++++++++++++++
>  target/arm/cpu.c       |  2 ++
>  target/arm/helper.c    | 29 +++++++++++++++--------------
>  target/arm/kvm32.c     |  4 ++--
>  target/arm/op_helper.c |  2 +-
>  5 files changed, 36 insertions(+), 17 deletions(-)
>
> --
> 2.19.1
>
>

Re: [Qemu-devel] [PATCH 0/2] Fix the last Hyp mode bug and turn it on for A7, A15

[Richard Henderson]

On 11/9/18 6:35 PM, Peter Maydell wrote:
> Not entirely sure what to do about this for 3.1 -- maybe
> put in the bugfix patch but hold off on actually setting
> the feature bit til 4.0?

I would put them both in for 3.1.


r~

Re: [Qemu-devel] [PATCH 1/2] target/arm: Hyp mode R14 is shared with User and System

[Edgar E. Iglesias]

On Fri, Nov 09, 2018 at 05:35:52PM +0000, Peter Maydell wrote:
> Hyp mode is an exception to the general rule that each AArch32
> mode has its own r13, r14 and SPSR -- it has a banked r13 and
> SPSR but shares its r14 with User and System mode. We were
> incorrectly implementing it as banked, which meant that on
> entry to Hyp mode r14 was 0 rather than the USR/SYS r14.
> 
> We provide a new function r14_bank_number() which is like
> the existing bank_number() but provides the index into
> env->banked_r14[]; bank_number() provides the index to use
> for env->banked_r13[] and env->banked_cpsr[].
> 
> All the points in the code that were using bank_number()
> to index into env->banked_r14[] are updated for consintency:
>  * switch_mode() -- this is the only place where we fix
>    an actual bug
>  * aarch64_sync_32_to_64() and aarch64_sync_64_to_32():
>    no behavioural change as we already special-cased Hyp R14
>  * kvm32.c: no behavioural change since the guest can't ever
>    be in Hyp mode, but conceptually the right thing to do
>  * msr_banked()/mrs_banked(): we can never get to the case
>    that accesses banked_r14[] with tgtmode == ARM_CPU_MODE_HYP,
>    so no behavioural change
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>



> ---
>  target/arm/internals.h | 16 ++++++++++++++++
>  target/arm/helper.c    | 29 +++++++++++++++--------------
>  target/arm/kvm32.c     |  4 ++--
>  target/arm/op_helper.c |  2 +-
>  4 files changed, 34 insertions(+), 17 deletions(-)
> 
> diff --git a/target/arm/internals.h b/target/arm/internals.h
> index 6c2bb2deebd..e5341f21f6f 100644
> --- a/target/arm/internals.h
> +++ b/target/arm/internals.h
> @@ -145,6 +145,22 @@ static inline int bank_number(int mode)
>      g_assert_not_reached();
>  }
>  
> +/**
> + * r14_bank_number: Map CPU mode onto register bank for r14
> + *
> + * Given an AArch32 CPU mode, return the index into the saved register
> + * banks to use for the R14 (LR) in that mode. This is the same as
> + * bank_number(), except for the special case of Hyp mode, where
> + * R14 is shared with USR and SYS, unlike its R13 and SPSR.
> + * This should be used as the index into env->banked_r14[], and
> + * bank_number() used for the index into env->banked_r13[] and
> + * env->banked_spsr[].
> + */
> +static inline int r14_bank_number(int mode)
> +{
> +    return (mode == ARM_CPU_MODE_HYP) ? BANK_USRSYS : bank_number(mode);
> +}
> +
>  void arm_cpu_register_gdb_regs_for_features(ARMCPU *cpu);
>  void arm_translate_init(void);
>  
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 96301930cc8..6fb1ddc5506 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -6455,13 +6455,14 @@ static void switch_mode(CPUARMState *env, int mode)
>  
>      i = bank_number(old_mode);
>      env->banked_r13[i] = env->regs[13];
> -    env->banked_r14[i] = env->regs[14];
>      env->banked_spsr[i] = env->spsr;
>  
>      i = bank_number(mode);
>      env->regs[13] = env->banked_r13[i];
> -    env->regs[14] = env->banked_r14[i];
>      env->spsr = env->banked_spsr[i];
> +
> +    env->banked_r14[r14_bank_number(old_mode)] = env->regs[14];
> +    env->regs[14] = env->banked_r14[r14_bank_number(mode)];
>  }
>  
>  /* Physical Interrupt Target EL Lookup Table
> @@ -8040,7 +8041,7 @@ void aarch64_sync_32_to_64(CPUARMState *env)
>          if (mode == ARM_CPU_MODE_HYP) {
>              env->xregs[14] = env->regs[14];
>          } else {
> -            env->xregs[14] = env->banked_r14[bank_number(ARM_CPU_MODE_USR)];
> +            env->xregs[14] = env->banked_r14[r14_bank_number(ARM_CPU_MODE_USR)];
>          }
>      }
>  
> @@ -8054,7 +8055,7 @@ void aarch64_sync_32_to_64(CPUARMState *env)
>          env->xregs[16] = env->regs[14];
>          env->xregs[17] = env->regs[13];
>      } else {
> -        env->xregs[16] = env->banked_r14[bank_number(ARM_CPU_MODE_IRQ)];
> +        env->xregs[16] = env->banked_r14[r14_bank_number(ARM_CPU_MODE_IRQ)];
>          env->xregs[17] = env->banked_r13[bank_number(ARM_CPU_MODE_IRQ)];
>      }
>  
> @@ -8062,7 +8063,7 @@ void aarch64_sync_32_to_64(CPUARMState *env)
>          env->xregs[18] = env->regs[14];
>          env->xregs[19] = env->regs[13];
>      } else {
> -        env->xregs[18] = env->banked_r14[bank_number(ARM_CPU_MODE_SVC)];
> +        env->xregs[18] = env->banked_r14[r14_bank_number(ARM_CPU_MODE_SVC)];
>          env->xregs[19] = env->banked_r13[bank_number(ARM_CPU_MODE_SVC)];
>      }
>  
> @@ -8070,7 +8071,7 @@ void aarch64_sync_32_to_64(CPUARMState *env)
>          env->xregs[20] = env->regs[14];
>          env->xregs[21] = env->regs[13];
>      } else {
> -        env->xregs[20] = env->banked_r14[bank_number(ARM_CPU_MODE_ABT)];
> +        env->xregs[20] = env->banked_r14[r14_bank_number(ARM_CPU_MODE_ABT)];
>          env->xregs[21] = env->banked_r13[bank_number(ARM_CPU_MODE_ABT)];
>      }
>  
> @@ -8078,7 +8079,7 @@ void aarch64_sync_32_to_64(CPUARMState *env)
>          env->xregs[22] = env->regs[14];
>          env->xregs[23] = env->regs[13];
>      } else {
> -        env->xregs[22] = env->banked_r14[bank_number(ARM_CPU_MODE_UND)];
> +        env->xregs[22] = env->banked_r14[r14_bank_number(ARM_CPU_MODE_UND)];
>          env->xregs[23] = env->banked_r13[bank_number(ARM_CPU_MODE_UND)];
>      }
>  
> @@ -8095,7 +8096,7 @@ void aarch64_sync_32_to_64(CPUARMState *env)
>              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->xregs[30] = env->banked_r14[r14_bank_number(ARM_CPU_MODE_FIQ)];
>      }
>  
>      env->pc = env->regs[15];
> @@ -8145,7 +8146,7 @@ void aarch64_sync_64_to_32(CPUARMState *env)
>          if (mode == ARM_CPU_MODE_HYP) {
>              env->regs[14] = env->xregs[14];
>          } else {
> -            env->banked_r14[bank_number(ARM_CPU_MODE_USR)] = env->xregs[14];
> +            env->banked_r14[r14_bank_number(ARM_CPU_MODE_USR)] = env->xregs[14];
>          }
>      }
>  
> @@ -8159,7 +8160,7 @@ void aarch64_sync_64_to_32(CPUARMState *env)
>          env->regs[14] = env->xregs[16];
>          env->regs[13] = env->xregs[17];
>      } else {
> -        env->banked_r14[bank_number(ARM_CPU_MODE_IRQ)] = env->xregs[16];
> +        env->banked_r14[r14_bank_number(ARM_CPU_MODE_IRQ)] = env->xregs[16];
>          env->banked_r13[bank_number(ARM_CPU_MODE_IRQ)] = env->xregs[17];
>      }
>  
> @@ -8167,7 +8168,7 @@ void aarch64_sync_64_to_32(CPUARMState *env)
>          env->regs[14] = env->xregs[18];
>          env->regs[13] = env->xregs[19];
>      } else {
> -        env->banked_r14[bank_number(ARM_CPU_MODE_SVC)] = env->xregs[18];
> +        env->banked_r14[r14_bank_number(ARM_CPU_MODE_SVC)] = env->xregs[18];
>          env->banked_r13[bank_number(ARM_CPU_MODE_SVC)] = env->xregs[19];
>      }
>  
> @@ -8175,7 +8176,7 @@ void aarch64_sync_64_to_32(CPUARMState *env)
>          env->regs[14] = env->xregs[20];
>          env->regs[13] = env->xregs[21];
>      } else {
> -        env->banked_r14[bank_number(ARM_CPU_MODE_ABT)] = env->xregs[20];
> +        env->banked_r14[r14_bank_number(ARM_CPU_MODE_ABT)] = env->xregs[20];
>          env->banked_r13[bank_number(ARM_CPU_MODE_ABT)] = env->xregs[21];
>      }
>  
> @@ -8183,7 +8184,7 @@ void aarch64_sync_64_to_32(CPUARMState *env)
>          env->regs[14] = env->xregs[22];
>          env->regs[13] = env->xregs[23];
>      } else {
> -        env->banked_r14[bank_number(ARM_CPU_MODE_UND)] = env->xregs[22];
> +        env->banked_r14[r14_bank_number(ARM_CPU_MODE_UND)] = env->xregs[22];
>          env->banked_r13[bank_number(ARM_CPU_MODE_UND)] = env->xregs[23];
>      }
>  
> @@ -8200,7 +8201,7 @@ void aarch64_sync_64_to_32(CPUARMState *env)
>              env->fiq_regs[i - 24] = env->xregs[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];
> +        env->banked_r14[r14_bank_number(ARM_CPU_MODE_FIQ)] = env->xregs[30];
>      }
>  
>      env->regs[15] = env->pc;
> diff --git a/target/arm/kvm32.c b/target/arm/kvm32.c
> index 0f1e94c7b5e..cb3fb73a961 100644
> --- a/target/arm/kvm32.c
> +++ b/target/arm/kvm32.c
> @@ -318,8 +318,8 @@ int kvm_arch_put_registers(CPUState *cs, int level)
>          memcpy(env->usr_regs, env->regs + 8, 5 * sizeof(uint32_t));
>      }
>      env->banked_r13[bn] = env->regs[13];
> -    env->banked_r14[bn] = env->regs[14];
>      env->banked_spsr[bn] = env->spsr;
> +    env->banked_r14[r14_bank_number(mode)] = env->regs[14];
>  
>      /* Now we can safely copy stuff down to the kernel */
>      for (i = 0; i < ARRAY_SIZE(regs); i++) {
> @@ -430,8 +430,8 @@ int kvm_arch_get_registers(CPUState *cs)
>          memcpy(env->regs + 8, env->usr_regs, 5 * sizeof(uint32_t));
>      }
>      env->regs[13] = env->banked_r13[bn];
> -    env->regs[14] = env->banked_r14[bn];
>      env->spsr = env->banked_spsr[bn];
> +    env->regs[14] = env->banked_r14[r14_bank_number(mode)];
>  
>      /* VFP registers */
>      r.id = KVM_REG_ARM | KVM_REG_SIZE_U64 | KVM_REG_ARM_VFP;
> diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c
> index 90741f6331d..2b62c53f5b5 100644
> --- a/target/arm/op_helper.c
> +++ b/target/arm/op_helper.c
> @@ -694,7 +694,7 @@ void HELPER(msr_banked)(CPUARMState *env, uint32_t value, uint32_t tgtmode,
>          env->banked_r13[bank_number(tgtmode)] = value;
>          break;
>      case 14:
> -        env->banked_r14[bank_number(tgtmode)] = value;
> +        env->banked_r14[r14_bank_number(tgtmode)] = value;
>          break;
>      case 8 ... 12:
>          switch (tgtmode) {
> -- 
> 2.19.1
> 

Re: [Qemu-devel] [PATCH 2/2] target/arm/cpu: Give Cortex-A15 and -A7 the EL2 feature

[Edgar E. Iglesias]

On Fri, Nov 09, 2018 at 05:35:53PM +0000, Peter Maydell wrote:
> The Cortex-A15 and Cortex-A7 both have EL2; now we've implemented
> it properly we can enable the feature bit.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>


> ---
>  target/arm/cpu.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 784a4c2dfcc..b7185234d85 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -1587,6 +1587,7 @@ static void cortex_a7_initfn(Object *obj)
>      set_feature(&cpu->env, ARM_FEATURE_GENERIC_TIMER);
>      set_feature(&cpu->env, ARM_FEATURE_DUMMY_C15_REGS);
>      set_feature(&cpu->env, ARM_FEATURE_CBAR_RO);
> +    set_feature(&cpu->env, ARM_FEATURE_EL2);
>      set_feature(&cpu->env, ARM_FEATURE_EL3);
>      cpu->kvm_target = QEMU_KVM_ARM_TARGET_CORTEX_A7;
>      cpu->midr = 0x410fc075;
> @@ -1633,6 +1634,7 @@ static void cortex_a15_initfn(Object *obj)
>      set_feature(&cpu->env, ARM_FEATURE_GENERIC_TIMER);
>      set_feature(&cpu->env, ARM_FEATURE_DUMMY_C15_REGS);
>      set_feature(&cpu->env, ARM_FEATURE_CBAR_RO);
> +    set_feature(&cpu->env, ARM_FEATURE_EL2);
>      set_feature(&cpu->env, ARM_FEATURE_EL3);
>      cpu->kvm_target = QEMU_KVM_ARM_TARGET_CORTEX_A15;
>      cpu->midr = 0x412fc0f1;
> -- 
> 2.19.1
> 

Re: [Qemu-devel] [Qemu-arm] [PATCH 1/2] target/arm: Hyp mode R14 is shared with User and System

[Edgar E. Iglesias]

On Fri, Nov 09, 2018 at 06:15:20PM +0000, Peter Maydell wrote:
> On 9 November 2018 at 17:35, Peter Maydell <peter.maydell@linaro.org> wrote:
> > Hyp mode is an exception to the general rule that each AArch32
> > mode has its own r13, r14 and SPSR -- it has a banked r13 and
> > SPSR but shares its r14 with User and System mode. We were
> > incorrectly implementing it as banked, which meant that on
> > entry to Hyp mode r14 was 0 rather than the USR/SYS r14.
> >
> > We provide a new function r14_bank_number() which is like
> > the existing bank_number() but provides the index into
> > env->banked_r14[]; bank_number() provides the index to use
> > for env->banked_r13[] and env->banked_cpsr[].
> >
> > All the points in the code that were using bank_number()
> > to index into env->banked_r14[] are updated for consintency:
> >  * switch_mode() -- this is the only place where we fix
> >    an actual bug
> >  * aarch64_sync_32_to_64() and aarch64_sync_64_to_32():
> >    no behavioural change as we already special-cased Hyp R14
> >  * kvm32.c: no behavioural change since the guest can't ever
> >    be in Hyp mode, but conceptually the right thing to do
> >  * msr_banked()/mrs_banked(): we can never get to the case
> >    that accesses banked_r14[] with tgtmode == ARM_CPU_MODE_HYP,
> >    so no behavioural change
> >
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> >  target/arm/internals.h | 16 ++++++++++++++++
> >  target/arm/helper.c    | 29 +++++++++++++++--------------
> >  target/arm/kvm32.c     |  4 ++--
> >  target/arm/op_helper.c |  2 +-
> >  4 files changed, 34 insertions(+), 17 deletions(-)
> 
> Rats, this bit accidentally didn't make it into this patch:
> 
> diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c
> index 2b62c53f5b5..eb6fb82fb81 100644
> --- a/target/arm/op_helper.c
> +++ b/target/arm/op_helper.c
> @@ -725,7 +725,7 @@ uint32_t HELPER(mrs_banked)(CPUARMState *env,
> uint32_t tgtmode, uint32_t regno)
>      case 13:
>          return env->banked_r13[bank_number(tgtmode)];
>      case 14:
> -        return env->banked_r14[bank_number(tgtmode)];
> +        return env->banked_r14[r14_bank_number(tgtmode)];
>      case 8 ... 12:
>          switch (tgtmode) {
>          case ARM_CPU_MODE_USR:
> 
> 
> (it's one of the "no behavioural change" bits).
> 


Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>

Re: [Qemu-devel] [Qemu-arm] [PATCH 1/2] target/arm: Hyp mode R14 is shared with User and System

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 9 November 2018 at 17:35, Peter Maydell <peter.maydell@linaro.org> wrote:
>> Hyp mode is an exception to the general rule that each AArch32
>> mode has its own r13, r14 and SPSR -- it has a banked r13 and
>> SPSR but shares its r14 with User and System mode. We were
>> incorrectly implementing it as banked, which meant that on
>> entry to Hyp mode r14 was 0 rather than the USR/SYS r14.
>>
>> We provide a new function r14_bank_number() which is like
>> the existing bank_number() but provides the index into
>> env->banked_r14[]; bank_number() provides the index to use
>> for env->banked_r13[] and env->banked_cpsr[].
>>
>> All the points in the code that were using bank_number()
>> to index into env->banked_r14[] are updated for consintency:
>>  * switch_mode() -- this is the only place where we fix
>>    an actual bug
>>  * aarch64_sync_32_to_64() and aarch64_sync_64_to_32():
>>    no behavioural change as we already special-cased Hyp R14
>>  * kvm32.c: no behavioural change since the guest can't ever
>>    be in Hyp mode, but conceptually the right thing to do
>>  * msr_banked()/mrs_banked(): we can never get to the case
>>    that accesses banked_r14[] with tgtmode == ARM_CPU_MODE_HYP,
>>    so no behavioural change
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>>  target/arm/internals.h | 16 ++++++++++++++++
>>  target/arm/helper.c    | 29 +++++++++++++++--------------
>>  target/arm/kvm32.c     |  4 ++--
>>  target/arm/op_helper.c |  2 +-
>>  4 files changed, 34 insertions(+), 17 deletions(-)
>
> Rats, this bit accidentally didn't make it into this patch:
>
> diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c
> index 2b62c53f5b5..eb6fb82fb81 100644
> --- a/target/arm/op_helper.c
> +++ b/target/arm/op_helper.c
> @@ -725,7 +725,7 @@ uint32_t HELPER(mrs_banked)(CPUARMState *env,
> uint32_t tgtmode, uint32_t regno)
>      case 13:
>          return env->banked_r13[bank_number(tgtmode)];
>      case 14:
> -        return env->banked_r14[bank_number(tgtmode)];
> +        return env->banked_r14[r14_bank_number(tgtmode)];
>      case 8 ... 12:
>          switch (tgtmode) {
>          case ARM_CPU_MODE_USR:
>
>
> (it's one of the "no behavioural change" bits).
<snip>

With the fix:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

--
Alex Bennée