[PATCH 09/35] target/arm: Record correct opcode fields in cpreg for E2H aliases

Peter Maydell posted 35 patches 11 months, 2 weeks ago
Maintainers: Peter Maydell <peter.maydell@linaro.org>
[PATCH 09/35] target/arm: Record correct opcode fields in cpreg for E2H aliases
Posted by Peter Maydell 11 months, 2 weeks ago
For FEAT_VHE, we define a set of register aliases, so that for instance:
 * the SCTLR_EL1 either accesses the real SCTLR_EL1, or (if E2H is 1)
   SCTLR_EL2
 * a new SCTLR_EL12 register accesses SCTLR_EL1 if E2H is 1

However when we create the 'new_reg' cpreg struct for the SCTLR_EL12
register, we duplicate the information in the SCTLR_EL1 cpreg, which
means the opcode fields are those of SCTLR_EL1, not SCTLR_EL12.  This
is a problem for code which looks at the cpreg opcode fields to
determine behaviour (e.g.  in access_check_cp_reg()). In practice
the current checks we do there don't intersect with the *_EL12
registers, but for FEAT_NV this will become a problem.

Write the correct values from the encoding into the new_reg struct.
This restores the invariant that the cpreg that you get back
from the hashtable has opcode fields that match the key you used
to retrieve it.

When we call the readfn or writefn for the target register, we
pass it the cpreg struct for that target register, not the one
for the alias, in case the readfn/writefn want to look at the
opcode fields to determine behaviour. This means we need to
interpose custom read/writefns for the e12 aliases.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/helper.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index f1c7fbf319c..c6f069b74cd 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -6506,6 +6506,19 @@ static void el2_e2h_write(CPUARMState *env, const ARMCPRegInfo *ri,
     writefn(env, ri, value);
 }
 
+static uint64_t el2_e2h_e12_read(CPUARMState *env, const ARMCPRegInfo *ri)
+{
+    /* Pass the EL1 register accessor its ri, not the EL12 alias ri */
+    return ri->orig_readfn(env, ri->opaque);
+}
+
+static void el2_e2h_e12_write(CPUARMState *env, const ARMCPRegInfo *ri,
+                              uint64_t value)
+{
+    /* Pass the EL1 register accessor its ri, not the EL12 alias ri */
+    return ri->orig_writefn(env, ri->opaque, value);
+}
+
 static void define_arm_vh_e2h_redirects_aliases(ARMCPU *cpu)
 {
     struct E2HAlias {
@@ -6605,6 +6618,28 @@ static void define_arm_vh_e2h_redirects_aliases(ARMCPU *cpu)
         new_reg->type |= ARM_CP_ALIAS;
         /* Remove PL1/PL0 access, leaving PL2/PL3 R/W in place.  */
         new_reg->access &= PL2_RW | PL3_RW;
+        /* The new_reg op fields are as per new_key, not the target reg */
+        new_reg->crn = (a->new_key & CP_REG_ARM64_SYSREG_CRN_MASK)
+            >> CP_REG_ARM64_SYSREG_CRN_SHIFT;
+        new_reg->crm = (a->new_key & CP_REG_ARM64_SYSREG_CRM_MASK)
+            >> CP_REG_ARM64_SYSREG_CRM_SHIFT;
+        new_reg->opc0 = (a->new_key & CP_REG_ARM64_SYSREG_OP0_MASK)
+            >> CP_REG_ARM64_SYSREG_OP0_SHIFT;
+        new_reg->opc1 = (a->new_key & CP_REG_ARM64_SYSREG_OP1_MASK)
+            >> CP_REG_ARM64_SYSREG_OP1_SHIFT;
+        new_reg->opc2 = (a->new_key & CP_REG_ARM64_SYSREG_OP2_MASK)
+            >> CP_REG_ARM64_SYSREG_OP2_SHIFT;
+        new_reg->opaque = src_reg;
+        new_reg->orig_readfn = src_reg->readfn ?: raw_read;
+        new_reg->orig_writefn = src_reg->writefn ?: raw_write;
+        if (!new_reg->raw_readfn) {
+            new_reg->raw_readfn = raw_read;
+        }
+        if (!new_reg->raw_writefn) {
+            new_reg->raw_writefn = raw_write;
+        }
+        new_reg->readfn = el2_e2h_e12_read;
+        new_reg->writefn = el2_e2h_e12_write;
 
         ok = g_hash_table_insert(cpu->cp_regs,
                                  (gpointer)(uintptr_t)a->new_key, new_reg);
-- 
2.34.1
Re: [PATCH 09/35] target/arm: Record correct opcode fields in cpreg for E2H aliases
Posted by Richard Henderson 11 months ago
On 12/18/23 22:32, Peter Maydell wrote:
> For FEAT_VHE, we define a set of register aliases, so that for instance:
>   * the SCTLR_EL1 either accesses the real SCTLR_EL1, or (if E2H is 1)
>     SCTLR_EL2
>   * a new SCTLR_EL12 register accesses SCTLR_EL1 if E2H is 1
> 
> However when we create the 'new_reg' cpreg struct for the SCTLR_EL12
> register, we duplicate the information in the SCTLR_EL1 cpreg, which
> means the opcode fields are those of SCTLR_EL1, not SCTLR_EL12.  This
> is a problem for code which looks at the cpreg opcode fields to
> determine behaviour (e.g.  in access_check_cp_reg()). In practice
> the current checks we do there don't intersect with the *_EL12
> registers, but for FEAT_NV this will become a problem.
> 
> Write the correct values from the encoding into the new_reg struct.
> This restores the invariant that the cpreg that you get back
> from the hashtable has opcode fields that match the key you used
> to retrieve it.
> 
> When we call the readfn or writefn for the target register, we
> pass it the cpreg struct for that target register, not the one
> for the alias, in case the readfn/writefn want to look at the
> opcode fields to determine behaviour. This means we need to
> interpose custom read/writefns for the e12 aliases.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   target/arm/helper.c | 35 +++++++++++++++++++++++++++++++++++
>   1 file changed, 35 insertions(+)

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

r~