[PATCH 0/2] target/arm: Load correct half of 64-bit fields

[PATCH 0/2] target/arm: Load correct half of 64-bit fields

[Peter Maydell]

This patchset fixes a bug where on a big-endian 64-bit host the
guest would crash immediately when it did an ERET. This happens
because when we load the new PC value from CPUARMState::esr_el[2]
we do a 32-bit load even though the struct field is 64 bits.
So on 64-bit BE we use the wrong half of the register.

Patch 1 defines a new macro to load the low 32 bits from a
64-bit field, and uses it in the two places where we got this
wrong. Patch 2 adds some compile-time assertions to the
existing load_cpu_field() and store_cpu_field() macros that
catch inadvertent uses on struct fields of the wrong size.

This fix is necessary to be able to run the avocado tests for
the orangepi on a 64-bit BE system.

thanks
-- PMM

Peter Maydell (2):
  target/arm: Define and use new load_cpu_field_low32()
  target/arm: Add compile time asserts to load/store_cpu_field macros

 target/arm/translate-a32.h | 24 ++++++++++++++++++++----
 target/arm/tcg/translate.c |  4 ++--
 2 files changed, 22 insertions(+), 6 deletions(-)

-- 
2.34.1

[PATCH 1/2] target/arm: Define and use new load_cpu_field_low32()

[Peter Maydell]

In several places in the 32-bit Arm translate.c, we try to use
load_cpu_field() to load from a CPUARMState field into a TCGv_i32
where the field is actually 64-bit. This works on little-endian
hosts, but gives the wrong half of the register on big-endian.

Add a new load_cpu_field_low32() which loads the low 32 bits
of a 64-bit field into a TCGv_i32. The new macro includes a
compile-time check against accidentally using it on a field
of the wrong size. Use it to fix the two places in the code
where we were using load_cpu_field() on a 64-bit field.

This fixes a bug where on big-endian hosts the guest would
crash after executing an ERET instruction, and a more corner
case one where some UNDEFs for attempted accesses to MSR
banked registers from Secure EL1 might go to the wrong EL.

Cc: qemu-stable@nongnu.org
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/translate-a32.h | 7 +++++++
 target/arm/tcg/translate.c | 4 ++--
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/target/arm/translate-a32.h b/target/arm/translate-a32.h
index 5339c22f1e0..067044292a4 100644
--- a/target/arm/translate-a32.h
+++ b/target/arm/translate-a32.h
@@ -61,6 +61,13 @@ static inline TCGv_i32 load_cpu_offset(int offset)
 
 #define load_cpu_field(name) load_cpu_offset(offsetof(CPUARMState, name))
 
+/* Load from the low half of a 64-bit field to a TCGv_i32 */
+#define load_cpu_field_low32(name)                                       \
+    ({                                                                   \
+        QEMU_BUILD_BUG_ON(sizeof(typeof_field(CPUARMState, name)) != 8); \
+        load_cpu_offset(offsetoflow32(CPUARMState, name));               \
+    })
+
 void store_cpu_offset(TCGv_i32 var, int offset, int size);
 
 #define store_cpu_field(var, name)                              \
diff --git a/target/arm/tcg/translate.c b/target/arm/tcg/translate.c
index 3c8401e9086..74684767249 100644
--- a/target/arm/tcg/translate.c
+++ b/target/arm/tcg/translate.c
@@ -2816,7 +2816,7 @@ static bool msr_banked_access_decode(DisasContext *s, int r, int sysm, int rn,
             if (arm_dc_feature(s, ARM_FEATURE_AARCH64) &&
                 dc_isar_feature(aa64_sel2, s)) {
                 /* Target EL is EL<3 minus SCR_EL3.EEL2> */
-                tcg_el = load_cpu_field(cp15.scr_el3);
+                tcg_el = load_cpu_field_low32(cp15.scr_el3);
                 tcg_gen_sextract_i32(tcg_el, tcg_el, ctz32(SCR_EEL2), 1);
                 tcg_gen_addi_i32(tcg_el, tcg_el, 3);
             } else {
@@ -6396,7 +6396,7 @@ static bool trans_ERET(DisasContext *s, arg_ERET *a)
     }
     if (s->current_el == 2) {
         /* ERET from Hyp uses ELR_Hyp, not LR */
-        tmp = load_cpu_field(elr_el[2]);
+        tmp = load_cpu_field_low32(elr_el[2]);
     } else {
         tmp = load_reg(s, 14);
     }
-- 
2.34.1

Re: [PATCH 1/2] target/arm: Define and use new load_cpu_field_low32()

[Richard Henderson]

On 4/24/23 16:39, Peter Maydell wrote:
> +        QEMU_BUILD_BUG_ON(sizeof(typeof_field(CPUARMState, name)) != 8); \

Surely just sizeof_field().

Otherwise,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

[PATCH 2/2] target/arm: Add compile time asserts to load/store_cpu_field macros

[Peter Maydell]

Add some compile-time asserts to the load_cpu_field() and store_cpu_field()
macros that the struct field being accessed is the expected size. This
lets us catch cases where we incorrectly tried to do a 32-bit load
from a 64-bit struct field.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/translate-a32.h | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/target/arm/translate-a32.h b/target/arm/translate-a32.h
index 067044292a4..c2bc3ab8567 100644
--- a/target/arm/translate-a32.h
+++ b/target/arm/translate-a32.h
@@ -59,7 +59,12 @@ static inline TCGv_i32 load_cpu_offset(int offset)
     return tmp;
 }
 
-#define load_cpu_field(name) load_cpu_offset(offsetof(CPUARMState, name))
+/* Load from a 32-bit field to a TCGv_i32 */
+#define load_cpu_field(name)                                             \
+    ({                                                                   \
+        QEMU_BUILD_BUG_ON(sizeof(typeof_field(CPUARMState, name)) != 4); \
+        load_cpu_offset(offsetof(CPUARMState, name));                    \
+    })
 
 /* Load from the low half of a 64-bit field to a TCGv_i32 */
 #define load_cpu_field_low32(name)                                       \
@@ -70,9 +75,13 @@ static inline TCGv_i32 load_cpu_offset(int offset)
 
 void store_cpu_offset(TCGv_i32 var, int offset, int size);
 
-#define store_cpu_field(var, name)                              \
-    store_cpu_offset(var, offsetof(CPUARMState, name),          \
-                     sizeof_field(CPUARMState, name))
+#define store_cpu_field(val, name)                                      \
+    ({                                                                  \
+        QEMU_BUILD_BUG_ON(sizeof(typeof_field(CPUARMState, name)) != 4  \
+                          && sizeof(typeof_field(CPUARMState, name)) != 1); \
+        store_cpu_offset(val, offsetof(CPUARMState, name),              \
+                         sizeof_field(CPUARMState, name));              \
+    })
 
 #define store_cpu_field_constant(val, name) \
     store_cpu_field(tcg_constant_i32(val), name)
-- 
2.34.1

Re: [PATCH 2/2] target/arm: Add compile time asserts to load/store_cpu_field macros

[Richard Henderson]

On 4/24/23 16:39, Peter Maydell wrote:
> +        QEMU_BUILD_BUG_ON(sizeof(typeof_field(CPUARMState, name)) != 4); \

Similarly.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 0/2] target/arm: Load correct half of 64-bit fields

[Peter Maydell]

On Mon, 24 Apr 2023 at 16:39, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> This patchset fixes a bug where on a big-endian 64-bit host the
> guest would crash immediately when it did an ERET. This happens
> because when we load the new PC value from CPUARMState::esr_el[2]
> we do a 32-bit load even though the struct field is 64 bits.
> So on 64-bit BE we use the wrong half of the register.
>
> Patch 1 defines a new macro to load the low 32 bits from a
> 64-bit field, and uses it in the two places where we got this
> wrong. Patch 2 adds some compile-time assertions to the
> existing load_cpu_field() and store_cpu_field() macros that
> catch inadvertent uses on struct fields of the wrong size.
>
> This fix is necessary to be able to run the avocado tests for
> the orangepi on a 64-bit BE system.

Applied to target-arm.next, adjusted to use sizeof_field().

thanks
-- PMM