[Qemu-devel] [PULL for-2.9 0/4] target-arm queue

[Qemu-devel] [PULL for-2.9 0/4] target-arm queue

[Peter Maydell]

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.

thanks
-- PMM

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)

----------------------------------------------------------------
target-arm queue:
 * 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(-)

[Qemu-devel] [PULL 1/4] arm: HVC and SMC encodings don't exist for M profile

[Peter Maydell]

M profile doesn't have the HVC or SMC encodings, so make them always
UNDEF rather than generating calls to helper functions that assume
A/R profile.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Message-id: 1487616072-9226-2-git-send-email-peter.maydell@linaro.org
---
 target/arm/translate.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/target/arm/translate.c b/target/arm/translate.c
index b859f10..216852b 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -10377,6 +10377,9 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw
                     goto illegal_op;
 
                 if (insn & (1 << 26)) {
+                    if (arm_dc_feature(s, ARM_FEATURE_M)) {
+                        goto illegal_op;
+                    }
                     if (!(insn & (1 << 20))) {
                         /* Hypervisor call (v7) */
                         int imm16 = extract32(insn, 16, 4) << 12
-- 
2.7.4

[Qemu-devel] [PULL 2/4] arm: Don't decode MRS(banked) or MSR(banked) for M profile

[Peter Maydell]

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.)

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Message-id: 1487616072-9226-3-git-send-email-peter.maydell@linaro.org
---
 target/arm/translate.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/target/arm/translate.c b/target/arm/translate.c
index 216852b..a5f5a28 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -10500,7 +10500,8 @@ 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);
@@ -10521,7 +10522,8 @@ 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

[Qemu-devel] [PULL 3/4] arm: Enforce should-be-1 bits in MRS decoding

[Peter Maydell]

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.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Message-id: 1487616072-9226-4-git-send-email-peter.maydell@linaro.org
---
 target/arm/translate.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/target/arm/translate.c b/target/arm/translate.c
index a5f5a28..c4acff5 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -10510,6 +10510,14 @@ 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)) {
@@ -10537,6 +10545,12 @@ 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

[Qemu-devel] [PULL 4/4] arm: Fix APSR writes via M profile MSR

[Peter Maydell]

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
index 8646a7a..8cb7a94 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -8548,8 +8548,18 @@ 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;
@@ -8558,8 +8568,16 @@ 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 c4acff5..e32e38c 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -10403,7 +10403,8 @@ 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

Re: [Qemu-devel] [PULL for-2.9 0/4] target-arm queue

[Peter Maydell]

On 20 March 2017 at 12:54, Peter Maydell <peter.maydell@linaro.org> wrote:
> 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.
>
> thanks
> -- PMM
>
> 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)
>
> ----------------------------------------------------------------
> target-arm queue:
>  * fix MSR/MRS decoding for M profile CPUs
>

Applied, thanks.

-- PMM