Small target-arm queue for 2.9: just the patches

which fix bugs in our MRS/MSR decoding for M profile,

including a fix for a regression introduced in commit

58117c9bb429cd.

The following changes since commit 00e7c07b06d004cf54b19724f82afde8a7a37f37:

Merge remote-tracking branch 'remotes/cohuck/tags/s390x-20170320' into staging (2017-03-20 10:51:30 +0000)

are available in the git repository at:

git://git.linaro.org/people/pmaydell/qemu-arm.git tags/pull-target-arm-20170320

for you to fetch changes up to b28b3377d7e9ba35611d454d5a63ef50cab1f8c5:

arm: Fix APSR writes via M profile MSR (2017-03-20 12:41:44 +0000)

* fix MSR/MRS decoding for M profile CPUs

Peter Maydell (4):

arm: HVC and SMC encodings don't exist for M profile

arm: Don't decode MRS(banked) or MSR(banked) for M profile

arm: Enforce should-be-1 bits in MRS decoding

arm: Fix APSR writes via M profile MSR

target/arm/helper.c | 26 ++++++++++++++++++++++----

target/arm/translate.c | 26 +++++++++++++++++++++++---

2 files changed, 45 insertions(+), 7 deletions(-)

Our implementation of writes to the APSR for M-profile via the MSR

instruction was badly broken.

First and worst, we had the sense wrong on the test of bit 2 of the

SYSm field -- this is supposed to request an APSR write if bit 2 is 0

but we were doing it if bit 2 was 1. This bug was introduced in

commit 58117c9bb429cd, so hasn't been in a QEMU release.

Secondly, the choice of exactly which parts of APSR should be written

is defined by bits in the 'mask' field. We were not passing these

through from instruction decode, making it impossible to check them

in the helper.

Pass the mask bits through from the instruction decode to the helper

function and process them appropriately; fix the wrong sense of the

SYSm bit 2 check.

Invalid mask values and invalid combinations of mask and register

number are UNPREDICTABLE; we choose to treat them as if the mask

values were valid.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Message-id: 1487616072-9226-5-git-send-email-peter.maydell@linaro.org

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

---

target/arm/helper.c | 26 ++++++++++++++++++++++----

target/arm/translate.c | 3 ++-

2 files changed, 24 insertions(+), 5 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c

--- a/target/arm/helper.c

+++ b/target/arm/helper.c

@@ -XXX,XX +XXX,XX @@ uint32_t HELPER(v7m_mrs)(CPUARMState *env, uint32_t reg)

}

-void HELPER(v7m_msr)(CPUARMState *env, uint32_t reg, uint32_t val)

-{

+void HELPER(v7m_msr)(CPUARMState *env, uint32_t maskreg, uint32_t val)

+{

+ /* We're passed bits [11..0] of the instruction; extract

+ * SYSm and the mask bits.

+ * Invalid combinations of SYSm and mask are UNPREDICTABLE;

+ * we choose to treat them as if the mask bits were valid.

+ * NB that the pseudocode 'mask' variable is bits [11..10],

+ * whereas ours is [11..8].

+ */

+ uint32_t mask = extract32(maskreg, 8, 4);

+ uint32_t reg = extract32(maskreg, 0, 8);

+

if (arm_current_el(env) == 0 && reg > 7) {

/* only xPSR sub-fields may be written by unprivileged */

return;

@@ -XXX,XX +XXX,XX @@ void HELPER(v7m_msr)(CPUARMState *env, uint32_t reg, uint32_t val)

switch (reg) {

case 0 ... 7: /* xPSR sub-fields */

/* only APSR is actually writable */

- if (reg & 4) {

- xpsr_write(env, val, 0xf8000000); /* APSR */

+ if (!(reg & 4)) {

+ uint32_t apsrmask = 0;

+

+ if (mask & 8) {

+ apsrmask |= 0xf8000000; /* APSR NZCVQ */

+ }

+ if ((mask & 4) && arm_feature(env, ARM_FEATURE_THUMB_DSP)) {

+ apsrmask |= 0x000f0000; /* APSR GE[3:0] */

+ }

+ xpsr_write(env, val, apsrmask);

}

break;

case 8: /* MSP */

diff --git a/target/arm/translate.c b/target/arm/translate.c

index XXXXXXX..XXXXXXX 100644

--- a/target/arm/translate.c

+++ b/target/arm/translate.c

@@ -XXX,XX +XXX,XX @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw

case 0: /* msr cpsr. */

if (arm_dc_feature(s, ARM_FEATURE_M)) {

tmp = load_reg(s, rn);

- addr = tcg_const_i32(insn & 0xff);

+ /* the constant is the mask and SYSm fields */

+ addr = tcg_const_i32(insn & 0xfff);

gen_helper_v7m_msr(cpu_env, addr, tmp);

tcg_temp_free_i32(addr);

tcg_temp_free_i32(tmp);

2.7.4

The MRS instruction requires that bits [19..16] are all 1s, and for

A/R profile also that bits [7..0] are all 0s. At this point in the

decode tree we have checked all of the rest of the instruction but

were allowing these to be any value. If these bits are not set then

the result is architecturally UNPREDICTABLE, but choosing to UNDEF is

more helpful to the user and avoids unexpected odd behaviour if the

encodings are used for some purpose in future architecture versions.

target/arm/translate.c | 14 ++++++++++++++

1 file changed, 14 insertions(+)

diff --git a/target/arm/translate.c b/target/arm/translate.c

--- a/target/arm/translate.c

+++ b/target/arm/translate.c

@@ -XXX,XX +XXX,XX @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw

break;

}

+ if (extract32(insn, 16, 4) != 0xf) {

+ goto illegal_op;

+ }

+ if (!arm_dc_feature(s, ARM_FEATURE_M) &&

+ extract32(insn, 0, 8) != 0) {

+ goto illegal_op;

+ }

/* mrs cpsr */

tmp = tcg_temp_new_i32();

if (arm_dc_feature(s, ARM_FEATURE_M)) {

@@ -XXX,XX +XXX,XX @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw

if (IS_USER(s) || arm_dc_feature(s, ARM_FEATURE_M)) {

goto illegal_op;

}

+

+ if (extract32(insn, 16, 4) != 0xf ||

+ extract32(insn, 0, 8) != 0) {

+ goto illegal_op;

+ }

+

tmp = load_cpu_field(spsr);

store_reg(s, rd, tmp);

break;

2.7.4

M profile doesn't have the MSR(banked) and MRS(banked) instructions

and uses the encodings for different kinds of M-profile MRS/MSR.

Guard the relevant bits of the decode logic to make sure we don't

accidentally fall into them by accident on M-profile.

(The bit being checked for this (bit 5) is part of the SYSm field on

M-profile, but since no currently allocated system registers have

encodings with bit 5 of SYSm set, this hasn't been a problem in

practice.)

target/arm/translate.c | 6 ++++--

1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/target/arm/translate.c b/target/arm/translate.c

--- a/target/arm/translate.c

+++ b/target/arm/translate.c

@@ -XXX,XX +XXX,XX @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw

gen_exception_return(s, tmp);

break;

case 6: /* MRS */

- if (extract32(insn, 5, 1)) {

+ if (extract32(insn, 5, 1) &&

+ !arm_dc_feature(s, ARM_FEATURE_M)) {

/* MRS (banked) */

int sysm = extract32(insn, 16, 4) |

(extract32(insn, 4, 1) << 4);

@@ -XXX,XX +XXX,XX @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw

store_reg(s, rd, tmp);

break;

case 7: /* MRS */

- if (extract32(insn, 5, 1)) {

+ if (extract32(insn, 5, 1) &&

+ !arm_dc_feature(s, ARM_FEATURE_M)) {

/* MRS (banked) */

int sysm = extract32(insn, 16, 4) |

(extract32(insn, 4, 1) << 4);

2.7.4