Peter Maydell <peter.mayd...@linaro.org> writes:
> From: Michael Davidsaver <mdavidsa...@gmail.com>
>
> The v7m CONTROL register bit 1 is SPSEL, which indicates
> the stack being used. We were storing this information
> not in v7m.control but in the separate v7m.other_sp
> structure field. Unfortunately, the code handling reads
> of the CONTROL register didn't take account of this, and
> so if SPSEL was updated by an exception entry or exit then
> a subsequent guest read of CONTROL would get the wrong value.
>
> Using a separate structure field doesn't really gain us
> anything in efficiency, so drop this unnecessary complexity
> in favour of simply storing all the bits in v7m.control.
>
> This is a migration compatibility break for M profile
> CPUs only.
>
> Signed-off-by: Michael Davidsaver <mdavidsa...@gmail.com>
> [PMM: rewrote commit message;
> use deposit32(); use FIELD to define constants for
> masking and shifting of CONTROL register fields
> ]
> Signed-off-by: Peter Maydell <peter.mayd...@linaro.org>
> ---
> target/arm/cpu.h | 1 -
> target/arm/internals.h | 7 +++++++
> target/arm/helper.c | 35 +++++++++++++++++++++++------------
> target/arm/machine.c | 6 ++----
> 4 files changed, 32 insertions(+), 17 deletions(-)
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 151a5d7..521c11b 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -405,7 +405,6 @@ typedef struct CPUARMState {
> uint32_t vecbase;
> uint32_t basepri;
> uint32_t control;
> - int current_sp;
> int exception;
> } v7m;
>
> diff --git a/target/arm/internals.h b/target/arm/internals.h
> index 3cae5ff..2e65bc1 100644
> --- a/target/arm/internals.h
> +++ b/target/arm/internals.h
> @@ -25,6 +25,8 @@
> #ifndef TARGET_ARM_INTERNALS_H
> #define TARGET_ARM_INTERNALS_H
>
> +#include "hw/registerfields.h"
> +
> /* register banks for CPU modes */
> #define BANK_USRSYS 0
> #define BANK_SVC 1
> @@ -75,6 +77,11 @@ static const char * const excnames[] = {
> */
> #define GTIMER_SCALE 16
>
> +/* Bit definitions for the v7M CONTROL register */
> +FIELD(V7M_CONTROL, NPRIV, 0, 1)
> +FIELD(V7M_CONTROL, SPSEL, 1, 1)
> +FIELD(V7M_CONTROL, FPCA, 2, 1)
> +
> /*
> * 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()
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 8edb08c..dc383d1 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -5947,14 +5947,19 @@ static uint32_t v7m_pop(CPUARMState *env)
> }
>
> /* Switch to V7M main or process stack pointer. */
> -static void switch_v7m_sp(CPUARMState *env, int process)
> +static void switch_v7m_sp(CPUARMState *env, bool new_spsel)
> {
> uint32_t tmp;
> - if (env->v7m.current_sp != process) {
> + bool old_spsel = env->v7m.control & R_V7M_CONTROL_SPSEL_MASK;
> +
> + if (old_spsel != new_spsel) {
> tmp = env->v7m.other_sp;
> env->v7m.other_sp = env->regs[13];
> env->regs[13] = tmp;
> - env->v7m.current_sp = process;
> +
> + env->v7m.control = deposit32(env->v7m.control,
> + R_V7M_CONTROL_SPSEL_SHIFT,
> + R_V7M_CONTROL_SPSEL_LENGTH,
> new_spsel);
I was thrown by the use of deposit32 here without the extract32. However
I see we are dealing with a bit here - shame there isn't set_bit32()
method.
> }
> }
>
> @@ -6049,8 +6054,9 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
> arm_log_exception(cs->exception_index);
>
> lr = 0xfffffff1;
> - if (env->v7m.current_sp)
> + if (env->v7m.control & R_V7M_CONTROL_SPSEL_MASK) {
> lr |= 4;
> + }
> if (env->v7m.exception == 0)
> lr |= 8;
>
> @@ -8294,9 +8300,11 @@ uint32_t HELPER(v7m_mrs)(CPUARMState *env, uint32_t
> reg)
>
> switch (reg) {
> case 8: /* MSP */
> - return env->v7m.current_sp ? env->v7m.other_sp : env->regs[13];
> + return (env->v7m.control & R_V7M_CONTROL_SPSEL_MASK) ?
> + env->v7m.other_sp : env->regs[13];
> case 9: /* PSP */
> - return env->v7m.current_sp ? env->regs[13] : env->v7m.other_sp;
> + return (env->v7m.control & R_V7M_CONTROL_SPSEL_MASK) ?
> + env->regs[13] : env->v7m.other_sp;
> case 16: /* PRIMASK */
> return (env->daif & PSTATE_I) != 0;
> case 17: /* BASEPRI */
> @@ -8326,16 +8334,18 @@ void HELPER(v7m_msr)(CPUARMState *env, uint32_t reg,
> uint32_t val)
> }
> break;
> case 8: /* MSP */
> - if (env->v7m.current_sp)
> + if (env->v7m.control & R_V7M_CONTROL_SPSEL_MASK) {
> env->v7m.other_sp = val;
> - else
> + } else {
> env->regs[13] = val;
> + }
> break;
> case 9: /* PSP */
> - if (env->v7m.current_sp)
> + if (env->v7m.control & R_V7M_CONTROL_SPSEL_MASK) {
> env->regs[13] = val;
> - else
> + } else {
> env->v7m.other_sp = val;
> + }
> break;
> case 16: /* PRIMASK */
> if (val & 1) {
> @@ -8360,8 +8370,9 @@ void HELPER(v7m_msr)(CPUARMState *env, uint32_t reg,
> uint32_t val)
> }
> break;
> case 20: /* CONTROL */
> - env->v7m.control = val & 3;
> - switch_v7m_sp(env, (val & 2) != 0);
> + switch_v7m_sp(env, (val & R_V7M_CONTROL_SPSEL_MASK) != 0);
> + env->v7m.control = val & (R_V7M_CONTROL_SPSEL_MASK |
> + R_V7M_CONTROL_NPRIV_MASK);
> break;
> default:
> qemu_log_mask(LOG_GUEST_ERROR, "Attempt to write unknown special"
> diff --git a/target/arm/machine.c b/target/arm/machine.c
> index d90943b..8ed24bf 100644
> --- a/target/arm/machine.c
> +++ b/target/arm/machine.c
> @@ -96,15 +96,13 @@ static bool m_needed(void *opaque)
>
> static const VMStateDescription vmstate_m = {
> .name = "cpu/m",
> - .version_id = 1,
> - .minimum_version_id = 1,
> + .version_id = 2,
> + .minimum_version_id = 2,
> .needed = m_needed,
> .fields = (VMStateField[]) {
> - VMSTATE_UINT32(env.v7m.other_sp, ARMCPU),
> VMSTATE_UINT32(env.v7m.vecbase, ARMCPU),
> VMSTATE_UINT32(env.v7m.basepri, ARMCPU),
> VMSTATE_UINT32(env.v7m.control, ARMCPU),
> - VMSTATE_INT32(env.v7m.current_sp, ARMCPU),
> VMSTATE_INT32(env.v7m.exception, ARMCPU),
> VMSTATE_END_OF_LIST()
> }
Not that it matters for this but is there a way to add another level of
indirection to the reading and writing of these fields to keep the same
migration format?
Anyway:
Reviewed-by: Alex Bennée <alex.ben...@linaro.org>
--
Alex Bennée
