From 2769c6ada028ccd6f52c6ae55bfa8f048a46aad2 Mon Sep 17 00:00:00 2001 From: Michael Davidsaver Date: Fri, 2 Mar 2018 13:25:58 -0500 Subject: [PATCH] armv7m: Fix reads of CONTROL register bit 1 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. Backports commit abc24d86cc0364f402e438fae3acb14289b40734 from qemu --- qemu/include/hw/registerfields.h | 73 ++++++++++++++++++++++++++++++++ qemu/target/arm/cpu.h | 1 - qemu/target/arm/helper.c | 34 +++++++++------ qemu/target/arm/internals.h | 7 +++ 4 files changed, 102 insertions(+), 13 deletions(-) create mode 100644 qemu/include/hw/registerfields.h diff --git a/qemu/include/hw/registerfields.h b/qemu/include/hw/registerfields.h new file mode 100644 index 00000000..e759e2d9 --- /dev/null +++ b/qemu/include/hw/registerfields.h @@ -0,0 +1,73 @@ +/* + * Register Definition API: field macros + * + * Copyright (c) 2016 Xilinx Inc. + * Copyright (c) 2013 Peter Crosthwaite + * + * This work is licensed under the terms of the GNU GPL, version 2. See + * the COPYING file in the top-level directory. + */ + +#ifndef REGISTERFIELDS_H +#define REGISTERFIELDS_H + +#include "qemu/bitops.h" + +/* Define constants for a 32 bit register */ + +/* This macro will define A_FOO, for the byte address of a register + * as well as R_FOO for the uint32_t[] register number (A_FOO / 4). + */ +#define REG32(reg, addr) \ + enum { A_ ## reg = (addr) }; \ + enum { R_ ## reg = (addr) / 4 }; + +/* Define SHIFT, LENGTH and MASK constants for a field within a register */ + +/* This macro will define R_FOO_BAR_MASK, R_FOO_BAR_SHIFT and R_FOO_BAR_LENGTH + * constants for field BAR in register FOO. + */ +#define FIELD(reg, field, shift, length) \ + enum { R_ ## reg ## _ ## field ## _SHIFT = (shift)}; \ + enum { R_ ## reg ## _ ## field ## _LENGTH = (length)}; \ + enum { R_ ## reg ## _ ## field ## _MASK = \ + MAKE_64BIT_MASK(shift, length)}; + +/* Extract a field from a register */ +#define FIELD_EX32(storage, reg, field) \ + extract32((storage), R_ ## reg ## _ ## field ## _SHIFT, \ + R_ ## reg ## _ ## field ## _LENGTH) +#define FIELD_EX64(storage, reg, field) \ + extract64((storage), R_ ## reg ## _ ## field ## _SHIFT, \ + R_ ## reg ## _ ## field ## _LENGTH) + +/* Extract a field from an array of registers */ +#define ARRAY_FIELD_EX32(regs, reg, field) \ + FIELD_EX32((regs)[R_ ## reg], reg, field) + +/* Deposit a register field. + * Assigning values larger then the target field will result in + * compilation warnings. + */ +#define FIELD_DP32(storage, reg, field, val) ({ \ + struct { \ + unsigned int v:R_ ## reg ## _ ## field ## _LENGTH; \ + } v = { val }; \ + uint32_t d; \ + d = deposit32((storage), R_ ## reg ## _ ## field ## _SHIFT, \ + R_ ## reg ## _ ## field ## _LENGTH, v.v); \ + d; }) +#define FIELD_DP64(storage, reg, field, val) ({ \ + struct { \ + unsigned int v:R_ ## reg ## _ ## field ## _LENGTH; \ + } v = { val }; \ + uint64_t d; \ + d = deposit64((storage), R_ ## reg ## _ ## field ## _SHIFT, \ + R_ ## reg ## _ ## field ## _LENGTH, v.v); \ + d; }) + +/* Deposit a field to array of registers. */ +#define ARRAY_FIELD_DP32(regs, reg, field, val) \ + (regs)[R_ ## reg] = FIELD_DP32((regs)[R_ ## reg], reg, field, val); + +#endif diff --git a/qemu/target/arm/cpu.h b/qemu/target/arm/cpu.h index 8cfc0083..90c7b3a2 100644 --- a/qemu/target/arm/cpu.h +++ b/qemu/target/arm/cpu.h @@ -414,7 +414,6 @@ typedef struct CPUARMState { uint32_t vecbase; uint32_t basepri; uint32_t control; - int current_sp; int exception; uint32_t secure; /* Is CPU in Secure state? (not guest visible) */ } v7m; diff --git a/qemu/target/arm/helper.c b/qemu/target/arm/helper.c index 5199e145..9d7fa459 100644 --- a/qemu/target/arm/helper.c +++ b/qemu/target/arm/helper.c @@ -5318,14 +5318,18 @@ 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); } } @@ -5395,8 +5399,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; @@ -7640,9 +7645,11 @@ uint32_t HELPER(v7m_mrs)(CPUARMState *env, uint32_t reg) case 7: /* IEPSR */ return xpsr_read(env) & 0x0700edff; 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 */ @@ -7686,16 +7693,18 @@ void HELPER(v7m_msr)(CPUARMState *env, uint32_t reg, uint32_t val) xpsr_write(env, val, 0x0600fc00); 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) { @@ -7720,8 +7729,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: /* ??? For debugging only. */ diff --git a/qemu/target/arm/internals.h b/qemu/target/arm/internals.h index c6b65e11..3e459b99 100644 --- a/qemu/target/arm/internals.h +++ b/qemu/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 @@ -77,6 +79,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()