[Qemu-devel] [PATCH v2] target-arm: Store SPSR_EL1 state in banked_spsr[1] (SPSR_svc)

[Qemu-devel] [PATCH v2] target-arm: Store SPSR_EL1 state in banked_spsr[1] (SPSR_svc)

[Peter Maydell]

The AArch64 SPSR_EL1 register is architecturally mandated to
be mapped to the AArch32 SPSR_svc register. This means its
state should live in QEMU's env->banked_spsr[1] field.
Correct the various places in the code that incorrectly
put it in banked_spsr[0].

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
v1->v2: fix all the places which were using [0], not just one...

 target-arm/helper-a64.c | 2 +-
 target-arm/helper.c     | 2 +-
 target-arm/internals.h  | 5 ++++-
 3 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/target-arm/helper-a64.c b/target-arm/helper-a64.c
index 7e0d038..861f6fa 100644
--- a/target-arm/helper-a64.c
+++ b/target-arm/helper-a64.c
@@ -523,7 +523,7 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
         aarch64_save_sp(env, arm_current_el(env));
         env->elr_el[new_el] = env->pc;
     } else {
-        env->banked_spsr[0] = cpsr_read(env);
+        env->banked_spsr[aarch64_banked_spsr_index(new_el)] = cpsr_read(env);
         if (!env->thumb) {
             env->cp15.esr_el[new_el] |= 1 << 25;
         }
diff --git a/target-arm/helper.c b/target-arm/helper.c
index 10886c5..d77c6de 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -2438,7 +2438,7 @@ static const ARMCPRegInfo v8_cp_reginfo[] = {
     { .name = "SPSR_EL1", .state = ARM_CP_STATE_AA64,
       .type = ARM_CP_ALIAS,
       .opc0 = 3, .opc1 = 0, .crn = 4, .crm = 0, .opc2 = 0,
-      .access = PL1_RW, .fieldoffset = offsetof(CPUARMState, banked_spsr[0]) },
+      .access = PL1_RW, .fieldoffset = offsetof(CPUARMState, banked_spsr[1]) },
     /* We rely on the access checks not allowing the guest to write to the
      * state field when SPSel indicates that it's being used as the stack
      * pointer.
diff --git a/target-arm/internals.h b/target-arm/internals.h
index bb171a7..2cc3017 100644
--- a/target-arm/internals.h
+++ b/target-arm/internals.h
@@ -82,11 +82,14 @@ static inline void arm_log_exception(int idx)
 
 /*
  * For AArch64, map a given EL to an index in the banked_spsr array.
+ * Note that this mapping and the AArch32 mapping defined in bank_number()
+ * must agree such that the AArch64<->AArch32 SPSRs have the architecturally
+ * mandated mapping between each other.
  */
 static inline unsigned int aarch64_banked_spsr_index(unsigned int el)
 {
     static const unsigned int map[4] = {
-        [1] = 0, /* EL1.  */
+        [1] = 1, /* EL1.  */
         [2] = 6, /* EL2.  */
         [3] = 7, /* EL3.  */
     };
-- 
1.9.1

Re: [Qemu-devel] [PATCH v2] target-arm: Store SPSR_EL1 state in banked_spsr[1] (SPSR_svc)

[Christoffer Dall]

On Tue, Mar 17, 2015 at 07:32:15PM +0000, Peter Maydell wrote:
> The AArch64 SPSR_EL1 register is architecturally mandated to
> be mapped to the AArch32 SPSR_svc register. This means its
> state should live in QEMU's env->banked_spsr[1] field.
> Correct the various places in the code that incorrectly
> put it in banked_spsr[0].
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>


Alex, will you respin the SPSR patch based on this one then?  You can
change the authorship on the SPSR patch to yourself if you prefer.

Thanks,
-Christoffer

Re: [Qemu-devel] [PATCH v2] target-arm: Store SPSR_EL1 state in banked_spsr[1] (SPSR_svc)

[Peter Maydell]

On 17 March 2015 at 19:38, Christoffer Dall <christoffer.dall@linaro.org> wrote:
> On Tue, Mar 17, 2015 at 07:32:15PM +0000, Peter Maydell wrote:
>> The AArch64 SPSR_EL1 register is architecturally mandated to
>> be mapped to the AArch32 SPSR_svc register. This means its
>> state should live in QEMU's env->banked_spsr[1] field.
>> Correct the various places in the code that incorrectly
>> put it in banked_spsr[0].
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>
> Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>

Thanks. It also occurs to me that we could avoid some of these
magic 1/6/7 numbers by defining some constants BANK_USR,
BANK_SVC, BANK_ABT, etc for the return values from bank_number
(ie the indexes into env->banked_* arrays),
and then using them where we need to refer to particular
banked_* entries elsewhere. Then for instance this array
in aarch64_banked_spsr_index() looks like

         [1] = BANK_SVC, /* EL1.  */
         [2] = BANK_HYP, /* EL2.  */
         [3] = BANK_MON, /* EL3.  */

and the mapping between the AArch32 and AArch64 SPSRs is
suddenly much more obvious and easy to check.

-- PMM

Re: [Qemu-devel] [PATCH v2] target-arm: Store SPSR_EL1 state in banked_spsr[1] (SPSR_svc)

[Alex Bennée]

Christoffer Dall <christoffer.dall@linaro.org> writes:

> On Tue, Mar 17, 2015 at 07:32:15PM +0000, Peter Maydell wrote:
>> The AArch64 SPSR_EL1 register is architecturally mandated to
>> be mapped to the AArch32 SPSR_svc register. This means its
>> state should live in QEMU's env->banked_spsr[1] field.
>> Correct the various places in the code that incorrectly
>> put it in banked_spsr[0].
>> 
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>
> Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>
>
>
> Alex, will you respin the SPSR patch based on this one then?  You can
> change the authorship on the SPSR patch to yourself if you prefer.

Yeah I'll re-spin the series hopefully by the end of this week or early next.

>
> Thanks,
> -Christoffer

-- 
Alex Bennée