[v5,1/6] target-arm: Store SPSR_EL1 state in banked_spsr[1] (SPSR_svc)

Message ID 1427130344-27986-2-git-send-email-alex.bennee@linaro.org
State Superseded
Headers show

Commit Message

Alex Bennée March 23, 2015, 5:05 p.m.
From: Peter Maydell <peter.maydell@linaro.org>

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>

Comments

Greg Bellows March 24, 2015, 2:32 p.m. | #1
On Mon, Mar 23, 2015 at 12:05 PM, Alex Bennée <alex.bennee@linaro.org> wrote:
> From: Peter Maydell <peter.maydell@linaro.org>
>
> 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>
>
> 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);

Are the other banks (2-5) only used for KVM?  It seems we go out of
our way to manage this larger SPSR array then not use all of the slots
in QEMU itself.

>          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.  */
>      };
> --
> 2.3.2
>
>
Peter Maydell March 24, 2015, 2:37 p.m. | #2
On 24 March 2015 at 14:32, Greg Bellows <greg.bellows@linaro.org> wrote:
> On Mon, Mar 23, 2015 at 12:05 PM, Alex Bennée <alex.bennee@linaro.org> wrote:
>> From: Peter Maydell <peter.maydell@linaro.org>

>> @@ -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);
>
> Are the other banks (2-5) only used for KVM?  It seems we go out of
> our way to manage this larger SPSR array then not use all of the slots
> in QEMU itself.

They're used in AArch32 (where they are the SPSR for various
32 bit modes). In AArch64 you can access those registers via
MSR/MRS (we probably haven't implemented those yet because they
are only accessible at EL2 and above) so hypervisors can do
worldswitches. But for exception entry and return (which is
what this code is) we only use SPSR_EL0/SPSR_EL1/SPSR_EL2/SPSR_EL3
which is a subset of the AArch32 SPSRs.

-- PMM

Patch

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.  */
     };