[PATCH 0/2] target/arm: SCR_EL3 RES0, RAO/WI tweaks

[PATCH 0/2] target/arm: SCR_EL3 RES0, RAO/WI tweaks

[Richard Henderson]

Adjust RW, fixing #1062, and adjusting bits [4:2].

r~

Richard Henderson (2):
  target/arm: SCR_EL3 bits 4,5 are always res0
  target/arm: SCR_EL3.RW is RAO/WI without AArch32 EL[12]

 target/arm/cpu.h    |  5 +++++
 target/arm/helper.c | 11 ++++++-----
 2 files changed, 11 insertions(+), 5 deletions(-)

-- 
2.34.1

[PATCH 1/2] target/arm: SCR_EL3 bits 4,5 are always res0

[Richard Henderson]

These bits do not depend on whether or not el1 supports aa32.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/helper.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 40da63913c..c262b00c3c 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -1752,11 +1752,8 @@ static void scr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
     ARMCPU *cpu = env_archcpu(env);
 
     if (ri->state == ARM_CP_STATE_AA64) {
-        if (arm_feature(env, ARM_FEATURE_AARCH64) &&
-            !cpu_isar_feature(aa64_aa32_el1, cpu)) {
-                value |= SCR_FW | SCR_AW;   /* these two bits are RES1.  */
-        }
-        valid_mask &= ~SCR_NET;
+        value |= SCR_FW | SCR_AW;      /* RES1 */
+        valid_mask &= ~SCR_NET;        /* RES0 */
 
         if (cpu_isar_feature(aa64_ras, cpu)) {
             valid_mask |= SCR_TERR;
-- 
2.34.1

[PATCH 2/2] target/arm: SCR_EL3.RW is RAO/WI without AArch32 EL[12]

[Richard Henderson]

Since DDI0487F.a, the RW bit is RAO/WI.  When specifically
targeting such a cpu, e.g. cortex-a76, it is legitimate to
ignore the bit within the secure monitor.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1062
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/cpu.h    | 5 +++++
 target/arm/helper.c | 4 ++++
 2 files changed, 9 insertions(+)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index c1865ad5da..a7c45d0d66 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -3947,6 +3947,11 @@ static inline bool isar_feature_aa64_aa32_el1(const ARMISARegisters *id)
     return FIELD_EX64(id->id_aa64pfr0, ID_AA64PFR0, EL1) >= 2;
 }
 
+static inline bool isar_feature_aa64_aa32_el2(const ARMISARegisters *id)
+{
+    return FIELD_EX64(id->id_aa64pfr0, ID_AA64PFR0, EL2) >= 2;
+}
+
 static inline bool isar_feature_aa64_ras(const ARMISARegisters *id)
 {
     return FIELD_EX64(id->id_aa64pfr0, ID_AA64PFR0, RAS) != 0;
diff --git a/target/arm/helper.c b/target/arm/helper.c
index c262b00c3c..84232a6437 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -1755,6 +1755,10 @@ static void scr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
         value |= SCR_FW | SCR_AW;      /* RES1 */
         valid_mask &= ~SCR_NET;        /* RES0 */
 
+        if (!cpu_isar_feature(aa64_aa32_el1, cpu) &&
+            !cpu_isar_feature(aa64_aa32_el2, cpu)) {
+            value |= SCR_RW;           /* RAO/WI*/
+        }
         if (cpu_isar_feature(aa64_ras, cpu)) {
             valid_mask |= SCR_TERR;
         }
-- 
2.34.1

Re: [PATCH 1/2] target/arm: SCR_EL3 bits 4,5 are always res0

[Peter Maydell]

On Sun, 5 Jun 2022 at 17:14, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> These bits do not depend on whether or not el1 supports aa32.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Isn't this effectively reverting commit 10d0ef3e6cfe2, which
got applied as a bug fix last year ?

In particular, the reason we need to check something is that even
if the CPU is entirely AArch32-only, reset of the register is
handled by scr_reset() on the AArch64 reginfo struct, so on reset
we need to give the correct answer for the CPU and not assume
"regdef is AA64" implies "EL3 is AA64".

We should probably be checking "is EL3 AArch64 or AArch32" rather
than "does EL1 support AArch32", though...

There's a testcase in the original patch cover letter for the
bug it's trying to fix:
https://lore.kernel.org/qemu-devel/20210203165552.16306-1-michael.nawrocki@gtri.gatech.edu/

thanks
-- PMM

Re: [PATCH 2/2] target/arm: SCR_EL3.RW is RAO/WI without AArch32 EL[12]

[Peter Maydell]

On Sun, 5 Jun 2022 at 17:16, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Since DDI0487F.a, the RW bit is RAO/WI.  When specifically
> targeting such a cpu, e.g. cortex-a76, it is legitimate to
> ignore the bit within the secure monitor.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1062
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/cpu.h    | 5 +++++
>  target/arm/helper.c | 4 ++++
>  2 files changed, 9 insertions(+)
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index c1865ad5da..a7c45d0d66 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -3947,6 +3947,11 @@ static inline bool isar_feature_aa64_aa32_el1(const ARMISARegisters *id)
>      return FIELD_EX64(id->id_aa64pfr0, ID_AA64PFR0, EL1) >= 2;
>  }
>
> +static inline bool isar_feature_aa64_aa32_el2(const ARMISARegisters *id)
> +{
> +    return FIELD_EX64(id->id_aa64pfr0, ID_AA64PFR0, EL2) >= 2;
> +}
> +
>  static inline bool isar_feature_aa64_ras(const ARMISARegisters *id)
>  {
>      return FIELD_EX64(id->id_aa64pfr0, ID_AA64PFR0, RAS) != 0;
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index c262b00c3c..84232a6437 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -1755,6 +1755,10 @@ static void scr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
>          value |= SCR_FW | SCR_AW;      /* RES1 */
>          valid_mask &= ~SCR_NET;        /* RES0 */
>
> +        if (!cpu_isar_feature(aa64_aa32_el1, cpu) &&
> +            !cpu_isar_feature(aa64_aa32_el2, cpu)) {
> +            value |= SCR_RW;           /* RAO/WI*/
> +        }

True in principle, but we probably need to do something to handle
the reset case for AArch32 CPUs, where cpu_isar_feature() will
return false becaese id_aa64pfr0 is zero but the bit should
nonetheless be RES0.

thanks
-- PMM