In add_cpreg_to_hashtable() we have some code that automatically
marks the secure version of an AArch32 banked cpreg as ARM_CP_ALIAS
under two conditions:
1) If the register has both 32-bit and 64-bit instances then we can
count on the 64-bit instance taking care of the non-secure bank.
2) If ARMv8 is enabled then we can count on a 64-bit version taking
care of the secure bank. This requires that separate 32 and
64-bit definitions are provided.
Item 1 is correct: if the ARMCPRegInfo has ARM_CP_STATE_BOTH then we
will always add it as an AA32 entry (which then splits into an S
entry with bank_fieldoffsets[0] and an NS entry with
bank_fieldoffsets[1]) and as an AA64 entry (which uses
bank_fieldoffsets[1]), so the AA64 entry will always be present to
cover the NS banked AA32 entry.
Item 2 is not correct: the idea was that because typically AArch32
Secure banked registers are (non-architecturally) mapped to EL3
AArch64 registers, the AArch64 EL3 register would handle things
(and that we would be adding the AArch64 register to the hashtable
even if the CPU didn't have AArch64).
However, this fails in several cases:
(a) registers like CSSELR, where there is only a CSSELR_EL1, and
no _EL3 version. CSSELR_S's underlying state isn't migrated
(b) registers like TPIDRURW, where the secure banked register
isn't mapped to any AArch64 register, so the state isn't
migrated
(c) IFAR_S and DFAR_S are odd special cases, because they map to
AArch32 HIFAR and HDFAR, which in turn map to AArch64 FAR_EL2.
If the CPU doesn't have EL2, we don't register FAR_EL2 and
the IFAR_S and DFAR_S registers don't get migrated.
More generally, the architecture dropped the concept of documenting
secure AArch32 registers as being "non-architecturally mapped" to EL3
AArch64 registers and relegated it to an implementation detail, which
is why TPIDRURW and friends got mishandled.
It also conflates "has 64-bit registers" with "is ARMv8", which is
wrong for the qemu-system-arm "max" type and for any AArch64 CPU
started with "aarch64=off".
Instead of marking the S-banked register as an alias if
ARM_FEATURE_V8, don't put it into the hashtable at all if
ARM_FEATURE_AARCH64 is present.
There are not all that many banked registers, and they're easy to
find by searching for regdefs that set bank_fieldoffsets:
DACR, IFAR, DFAR, IFSR, DFSR, CSSELR, MAIR0, MAIR1,
TPIDRURW, TPIDRURO, TPIDRPRW, TTBR0, TTBR1, TTBCR, TTBCR2,
PAR, VBAR, SCTLR
None of these do complicated things on reset, so having both an
AArch64 view and an AArch32 view reset the same register state to the
same value and migrate the same data twice is harmless. This is
better than sometimes not migrating or resetting it at all.
This changes the set of cpregs we will try to migrate for CPUs with
ARM_FEATURE_V8 but not ARM_FEATURE_AARCH64, and is a migration break
there. However, the only such CPUs are the qemu-system-aarch32 "max"
CPU and AArch64 CPUs with "aarch64=off".
In an ideal world we would also not bother putting the S cpreg into
the hashtable if the CPU is AArch32 without EL3 (i.e. it doesn't
implement Secure state at all), because nothing will ever look up the
S version of the register (access_secure_reg() always returns false).
But that would be a migration compat break for those CPUs.
Signed-off-by: Peter Maydell <[email protected]>
---
target/arm/helper.c | 35 +++++++++++++++++++++++++----------
1 file changed, 25 insertions(+), 10 deletions(-)
diff --git a/target/arm/helper.c b/target/arm/helper.c
index c3f607e6d6b..1b024adc703 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -7787,18 +7787,33 @@ static void add_cpreg_to_hashtable(ARMCPU *cpu,
ARMCPRegInfo *r,
if (state == ARM_CP_STATE_AA32) {
if (isbanked) {
/*
- * If the register is banked then we don't need to migrate or
- * reset the 32-bit instance in certain cases:
+ * If this is an AArch64 CPU then AArch32 cpregs are never
+ * banked, and if we put an NS version into the hash table
+ * it would never be accessed (compare the condition we test
+ * in access_secure_reg()). So drop it rather than adding it.
*
- * 1) If the register has both 32-bit and 64-bit instances
- * then we can count on the 64-bit instance taking care
- * of the non-secure bank.
- * 2) If ARMv8 is enabled then we can count on a 64-bit
- * version taking care of the secure bank. This requires
- * that separate 32 and 64-bit definitions are provided.
+ * Ideally we would also ignore the S banked register here
+ * for an AArch32 register without EL3; however, that would
+ * be a migration compatibility break for those CPUs, so we
+ * continue with having the cpregs in the hashtable.
*/
- if ((r->state == ARM_CP_STATE_BOTH && ns) ||
- (arm_feature(env, ARM_FEATURE_V8) && !ns)) {
+ if (!ns && arm_feature(env, ARM_FEATURE_AARCH64)) {
+ g_free(r);
+ return;
+ }
+ /*
+ * If the register is banked then we don't need to migrate or
+ * reset the 32-bit instance if this is a STATE_BOTH regdef.
+ * This is because we can know for certain that there's a
+ * 64-bit regdef that's using bank_fieldoffsets[1] as its
+ * fieldoffset, because the code that handles STATE_BOTH
+ * always registers it. In other situations either the NS
+ * or the S banked register might be aliased (architecturally
+ * or non-architecturally) to a 64-bit register, but it might
+ * not be. For those we must manually mark the alias in the
+ * regdef struct if we care.
+ */
+ if (r->state == ARM_CP_STATE_BOTH && ns) {
r->type |= ARM_CP_ALIAS;
}
} else if ((secstate != r->secure) && !ns) {
--
2.43.0
