[Qemu-devel] [PATCH 0/4] arm: Fix M profile MSR/MRS

[Qemu-devel] [PATCH 0/4] arm: Fix M profile MSR/MRS

[Peter Maydell]

This patchseries fixes up some deficiencies and one nasty
bug in the M profile MSR/MRS handling.

The first three patches are just cleaning up the decode
so that we UNDEF where we should in the MRS/MSR space
for M profile -- this won't have caused any problems in
practice since real world code doesn't generally execute
UNDEFfing instructions on purpose.

The fourth patch fixes a nasty bug that I introduced in
commit 58117c9bb429cd which broke APSR writes via MSR,
and brings them into line with the pseudocode by allowing
writes to the APSR GE[3:0] bits when the CPU implements
the DSP extensions.

Alex -- I should have paid closer attention to your review
comments on the patch that became commit 58117c9bb429cd;
sorry about that. I knew we didn't get the GE[3:0] stuff
right yet but I didn't spot that we'd managed to invert
the sense of the SYSm bit 2 test in that patch :-(

thanks
-- PMM

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

-- 
2.7.4

[Qemu-devel] [PATCH 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>
---
 target/arm/translate.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/target/arm/translate.c b/target/arm/translate.c
index 9fded03..895b399 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -10365,6 +10365,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] [PATCH 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>
---
 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 895b399..0f8548f 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -10488,7 +10488,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);
@@ -10509,7 +10510,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] [PATCH 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>
---
 target/arm/translate.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/target/arm/translate.c b/target/arm/translate.c
index 0f8548f..9090356 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -10498,6 +10498,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)) {
@@ -10525,6 +10533,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] [PATCH 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>
---
 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 948aba2..8349e1f 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -8478,8 +8478,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;
@@ -8488,8 +8498,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 9090356..ce7f19f 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -10391,7 +10391,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] [Qemu-arm] [PATCH 0/4] arm: Fix M profile MSR/MRS

[Peter Maydell]

Ping for review -- since this is a bugfix it should go
into 2.9.

thanks
-- PMM

On 20 February 2017 at 19:41, Peter Maydell <peter.maydell@linaro.org> wrote:
> This patchseries fixes up some deficiencies and one nasty
> bug in the M profile MSR/MRS handling.
>
> The first three patches are just cleaning up the decode
> so that we UNDEF where we should in the MRS/MSR space
> for M profile -- this won't have caused any problems in
> practice since real world code doesn't generally execute
> UNDEFfing instructions on purpose.
>
> The fourth patch fixes a nasty bug that I introduced in
> commit 58117c9bb429cd which broke APSR writes via MSR,
> and brings them into line with the pseudocode by allowing
> writes to the APSR GE[3:0] bits when the CPU implements
> the DSP extensions.
>
> Alex -- I should have paid closer attention to your review
> comments on the patch that became commit 58117c9bb429cd;
> sorry about that. I knew we didn't get the GE[3:0] stuff
> right yet but I didn't spot that we'd managed to invert
> the sense of the SYSm bit 2 test in that patch :-(
>
> thanks
> -- PMM
>
> 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(-)

Re: [Qemu-devel] [Qemu-arm] [PATCH 0/4] arm: Fix M profile MSR/MRS

[Peter Maydell]

Last ping for review; otherwise I'll put them into rc1 anyway
since it's fixing a regression.

thanks
-- PMM

On 14 March 2017 at 11:52, Peter Maydell <peter.maydell@linaro.org> wrote:
> Ping for review -- since this is a bugfix it should go
> into 2.9.
>
> thanks
> -- PMM
>
> On 20 February 2017 at 19:41, Peter Maydell <peter.maydell@linaro.org> wrote:
>> This patchseries fixes up some deficiencies and one nasty
>> bug in the M profile MSR/MRS handling.
>>
>> The first three patches are just cleaning up the decode
>> so that we UNDEF where we should in the MRS/MSR space
>> for M profile -- this won't have caused any problems in
>> practice since real world code doesn't generally execute
>> UNDEFfing instructions on purpose.
>>
>> The fourth patch fixes a nasty bug that I introduced in
>> commit 58117c9bb429cd which broke APSR writes via MSR,
>> and brings them into line with the pseudocode by allowing
>> writes to the APSR GE[3:0] bits when the CPU implements
>> the DSP extensions.
>>
>> Alex -- I should have paid closer attention to your review
>> comments on the patch that became commit 58117c9bb429cd;
>> sorry about that. I knew we didn't get the GE[3:0] stuff
>> right yet but I didn't spot that we'd managed to invert
>> the sense of the SYSm bit 2 test in that patch :-(
>>
>> thanks
>> -- PMM
>>
>> 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(-)

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

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

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

> ---
>  target/arm/translate.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index 9fded03..895b399 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -10365,6 +10365,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


--
Alex Bennée

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

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> 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 ARMv7-A documentation talks about banked registers being a feature
of application processors with Virtualisation Extensions which make the
sense of the test a bit weird. But I guess they are functionally
equivalent. Are there in practice any -A cores without virt?

>
> (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>

Anyway digressions aside:

Reviewed-by: Alex Bennée <alex.bennee@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 895b399..0f8548f 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -10488,7 +10488,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);
> @@ -10509,7 +10510,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);


--
Alex Bennée

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

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

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


> ---
>  target/arm/translate.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index 0f8548f..9090356 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -10498,6 +10498,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)) {
> @@ -10525,6 +10533,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;


--
Alex Bennée

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

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

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

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 948aba2..8349e1f 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -8478,8 +8478,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;
> @@ -8488,8 +8498,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 9090356..ce7f19f 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -10391,7 +10391,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);


--
Alex Bennée

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

[Peter Maydell]

On 20 March 2017 at 10:57, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Peter Maydell <peter.maydell@linaro.org> writes:
>
>> 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 ARMv7-A documentation talks about banked registers being a feature
> of application processors with Virtualisation Extensions which make the
> sense of the test a bit weird. But I guess they are functionally
> equivalent. Are there in practice any -A cores without virt?

You can have an A profile core without EL2 implemented, yes.
In particular in ARMv8 this encoding is legal (and has defined
effects) even if EL2 is not implemented. It's introduced in
ARMv7VE, and there's an argument for allowing it even if we
don't implement EL2 for those CPUs which in real hardware do
implement it, because system software might assume it can use
these encodings.

We should probably UNDEF it for pre-v7VE cores though. For
this patchset what I really wanted was to avoid the different
decode used by M profile getting mixed up with the A profile
decode path.

thanks
-- PMM