[WIP-for-10.1 v2 2/5] target/arm: Add FEAT_MEC registers

Gustavo Romero posted 5 patches 5 months, 2 weeks ago
There is a newer version of this series
[WIP-for-10.1 v2 2/5] target/arm: Add FEAT_MEC registers
Posted by Gustavo Romero 5 months, 2 weeks ago
Add all FEAT_MEC registers.

To work properly, FEAT_MEC also depends on FEAT_SCTLR2 and FEAT_TCR2,
which are not implemented in this commit. The bits in SCTLR2 and TCR2
control which translation regimes use MECIDs, and determine which MECID
is selected.

FEAT_MEC also requires two new cache management instructions, not
included in this commit, that will be implemented in subsequent commits.

Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
---
 target/arm/cpu.h    | 14 +++++++
 target/arm/helper.c | 98 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 112 insertions(+)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 8ce30ca857..9509217486 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -565,6 +565,16 @@ typedef struct CPUArchState {
 
         /* NV2 register */
         uint64_t vncr_el2;
+
+        /* MEC registers */
+        uint64_t mecidr_el2;
+        uint64_t mecid_p0_el2;
+        uint64_t mecid_a0_el2;
+        uint64_t mecid_p1_el2;
+        uint64_t mecid_a1_el2;
+        uint64_t mecid_rl_a_el3;
+        uint64_t vmecid_p_el2;
+        uint64_t vmecid_a_el2;
     } cp15;
 
     struct {
@@ -2389,6 +2399,10 @@ FIELD(MFAR, FPA, 12, 40)
 FIELD(MFAR, NSE, 62, 1)
 FIELD(MFAR, NS, 63, 1)
 
+FIELD(MECIDR, MECIDW, 0, 4)
+FIELD(MECID, MECID, 0, 16)
+FIELD(VMECID, MECID, 0, 16)
+
 QEMU_BUILD_BUG_ON(ARRAY_SIZE(((ARMCPU *)0)->ccsidr) <= R_V7M_CSSELR_INDEX_MASK);
 
 /* If adding a feature bit which corresponds to a Linux ELF
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 889d308807..9f8a284261 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -6823,6 +6823,100 @@ static const ARMCPRegInfo nmi_reginfo[] = {
       .resetfn = arm_cp_reset_ignore },
 };
 
+static void mecidr_reset(CPUARMState *env, const ARMCPRegInfo *ri)
+{
+    /* MECIDWidthm1 = 15, i.e. 16 bits is the width of a MECID. */
+    env->cp15.mecidr_el2 = FIELD_DP64(0, MECIDR, MECIDW, 15);
+}
+
+static uint64_t mecidr_read(CPUARMState *env, const ARMCPRegInfo *ri)
+{
+    uint64_t valid_mask;
+
+    if (!arm_is_el2_enabled(env)) {
+        /* All bits are RES0. */
+        return 0ULL;
+    }
+
+    valid_mask = R_MECIDR_MECIDW_MASK;
+    return env->cp15.mecidr_el2 & valid_mask;
+}
+
+static CPAccessResult mecid_access(CPUARMState *env,
+                                   const ARMCPRegInfo *ri, bool isread)
+{
+    int el;
+
+    el = arm_current_el(env);
+    if (el == 2 && !(env->cp15.scr_el3 & SCR_MECEN)) {
+        return CP_ACCESS_TRAP_EL3;
+    }
+
+    return CP_ACCESS_OK;
+}
+
+static void mecid_write(CPUARMState *env, const ARMCPRegInfo *ri,
+                        uint64_t value)
+{
+    uint64_t valid_mask;
+
+    valid_mask = R_MECID_MECID_MASK;
+    value &= valid_mask;
+    raw_write(env, ri, value);
+}
+
+static uint64_t mecid_read(CPUARMState *env, const ARMCPRegInfo *ri)
+{
+    uint64_t valid_mask;
+
+    valid_mask = R_MECID_MECID_MASK;
+    return raw_read(env, ri) & valid_mask;
+}
+
+static const ARMCPRegInfo mec_reginfo[] = {
+    { .name = "MECIDR_EL2", .state = ARM_CP_STATE_AA64,
+      .opc0 = 3, .opc1 = 4, .opc2 = 7, .crn = 10, .crm = 8,
+      .resetfn = mecidr_reset,
+      .access = PL2_RW, .accessfn = mecid_access, .readfn = mecidr_read,
+      .writefn = arm_cp_write_ignore,
+      .fieldoffset = offsetof(CPUARMState, cp15.mecidr_el2) },
+    { .name = "MECID_P0_EL2", .state = ARM_CP_STATE_AA64,
+      .opc0 = 3, .opc1 = 4, .opc2 = 0, .crn = 10, .crm = 8,
+      .access = PL2_RW, .accessfn = mecid_access,
+      .readfn = mecid_read, .writefn = mecid_write,
+      .fieldoffset = offsetof(CPUARMState, cp15.mecid_p0_el2) },
+    { .name = "MECID_A0_EL2", .state = ARM_CP_STATE_AA64,
+      .opc0 = 3, .opc1 = 4, .opc2 = 1, .crn = 10, .crm = 8,
+      .access = PL2_RW, .accessfn = mecid_access,
+      .readfn = mecid_read, .writefn = mecid_write,
+      .fieldoffset = offsetof(CPUARMState, cp15.mecid_a0_el2) },
+    { .name = "MECID_P1_EL2", .state = ARM_CP_STATE_AA64,
+      .opc0 = 3, .opc1 = 4, .opc2 = 2, .crn = 10, .crm = 8,
+      .access = PL2_RW, .accessfn = mecid_access,
+      .readfn = mecid_read, .writefn = mecid_write,
+      .fieldoffset = offsetof(CPUARMState, cp15.mecid_p1_el2) },
+    { .name = "MECID_A1_EL2", .state = ARM_CP_STATE_AA64,
+      .opc0 = 3, .opc1 = 4, .opc2 = 3, .crn = 10, .crm = 8,
+      .access = PL2_RW, .accessfn = mecid_access,
+      .readfn = mecid_read, .writefn = mecid_write,
+      .fieldoffset = offsetof(CPUARMState, cp15.mecid_a1_el2) },
+    { .name = "MECID_RL_A_EL3", .state = ARM_CP_STATE_AA64,
+      .opc0 = 3, .opc1 = 6, .opc2 = 1, .crn = 10, .crm = 10,
+      .access = PL2_RW, .accessfn = mecid_access,
+      .readfn = mecid_read, .writefn = mecid_write,
+      .fieldoffset = offsetof(CPUARMState, cp15.mecid_rl_a_el3) },
+    { .name = "VMECID_P_EL2", .state = ARM_CP_STATE_AA64,
+      .opc0 = 3, .opc1 = 4, .opc2 = 0, .crn = 10, .crm = 9,
+      .access = PL2_RW, .accessfn = mecid_access,
+      .readfn = mecid_read, .writefn = mecid_write,
+      .fieldoffset = offsetof(CPUARMState, cp15.vmecid_p_el2) },
+    { .name = "VMECID_A_EL2", .state = ARM_CP_STATE_AA64,
+      .opc0 = 3, .opc1 = 4, .opc2 = 1, .crn = 10, .crm = 9,
+      .access = PL2_RW, .accessfn = mecid_access,
+      .readfn = mecid_read, .writefn = mecid_write,
+      .fieldoffset = offsetof(CPUARMState, cp15.vmecid_a_el2) },
+};
+
 static void define_pmu_regs(ARMCPU *cpu)
 {
     /*
@@ -9008,6 +9102,10 @@ void register_cp_regs_for_features(ARMCPU *cpu)
         define_arm_cp_regs(cpu, nmi_reginfo);
     }
 
+    if (cpu_isar_feature(aa64_mec, cpu)) {
+        define_arm_cp_regs(cpu, mec_reginfo);
+    }
+
     if (cpu_isar_feature(any_predinv, cpu)) {
         define_arm_cp_regs(cpu, predinv_reginfo);
     }
-- 
2.34.1
Re: [WIP-for-10.1 v2 2/5] target/arm: Add FEAT_MEC registers
Posted by Richard Henderson 5 months, 2 weeks ago
On 7/4/25 09:14, Gustavo Romero wrote:
> +        /* MEC registers */
> +        uint64_t mecidr_el2;

Don't need this one.

> +        uint64_t mecid_p0_el2;
> +        uint64_t mecid_a0_el2;
> +        uint64_t mecid_p1_el2;
> +        uint64_t mecid_a1_el2;
> +        uint64_t mecid_rl_a_el3;
> +        uint64_t vmecid_p_el2;
> +        uint64_t vmecid_a_el2;
>       } cp15;
>   
>       struct {
> @@ -2389,6 +2399,10 @@ FIELD(MFAR, FPA, 12, 40)
>   FIELD(MFAR, NSE, 62, 1)
>   FIELD(MFAR, NS, 63, 1)
>   
> +FIELD(MECIDR, MECIDW, 0, 4)
> +FIELD(MECID, MECID, 0, 16)
> +FIELD(VMECID, MECID, 0, 16)

You don't really need these.

> +
>   QEMU_BUILD_BUG_ON(ARRAY_SIZE(((ARMCPU *)0)->ccsidr) <= R_V7M_CSSELR_INDEX_MASK);
>   
>   /* If adding a feature bit which corresponds to a Linux ELF
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 889d308807..9f8a284261 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -6823,6 +6823,100 @@ static const ARMCPRegInfo nmi_reginfo[] = {
>         .resetfn = arm_cp_reset_ignore },
>   };
>   
> +static void mecidr_reset(CPUARMState *env, const ARMCPRegInfo *ri)
> +{
> +    /* MECIDWidthm1 = 15, i.e. 16 bits is the width of a MECID. */
> +    env->cp15.mecidr_el2 = FIELD_DP64(0, MECIDR, MECIDW, 15);
> +}
> +
> +static uint64_t mecidr_read(CPUARMState *env, const ARMCPRegInfo *ri)
> +{
> +    uint64_t valid_mask;
> +
> +    if (!arm_is_el2_enabled(env)) {
> +        /* All bits are RES0. */
> +        return 0ULL;
> +    }
> +
> +    valid_mask = R_MECIDR_MECIDW_MASK;
> +    return env->cp15.mecidr_el2 & valid_mask;
> +}

How do you get the arm_is_el2_enabled check?

MECIDR_EL2 is a read-only register with one field: MECIDWidthm1, containing a constant 
value.  I would expect to see

     { .name = "MECIDR_EL2", .state = ARM_CP_STATE_AA64,
       .opc0 = 3, .opc1 = 4, .opc2 = 7, .crn = 10, .crm = 8,
       .access = PL2_R, .type = ARM_CP_CONST, .reset = MECID_WIDTH - 1 },



> +
> +static CPAccessResult mecid_access(CPUARMState *env,
> +                                   const ARMCPRegInfo *ri, bool isread)
> +{
> +    int el;
> +
> +    el = arm_current_el(env);
> +    if (el == 2 && !(env->cp15.scr_el3 & SCR_MECEN)) {
> +        return CP_ACCESS_TRAP_EL3;
> +    }
> +
> +    return CP_ACCESS_OK;
> +}

It's better practice to initialize local variables when at all possible, especially now 
that we build with -ftrivial-auto-var-init:

     int el = arm_current_el(env);

Missing security state check:

     if (el == 2) {
         if (arm_security_space(env) != ARMSS_Realm) {
             return CP_ACCESS_UNDEFINED;
         }
         if (!(env->cp15.scr_el3 & SCR_MECEN)) {
             return CP_ACCESS_TRAP_EL3;
         }
     }
     return CP_ACCESS_OK;


> +
> +static void mecid_write(CPUARMState *env, const ARMCPRegInfo *ri,
> +                        uint64_t value)
> +{
> +    uint64_t valid_mask;
> +
> +    valid_mask = R_MECID_MECID_MASK;
> +    value &= valid_mask;
> +    raw_write(env, ri, value);
> +}

The valid mask does not come from R_MECID_MECID_MASK,
but from the contents of that field.  This should be

     value = extract64(value, 0, MECID_WIDTH);


> +
> +static uint64_t mecid_read(CPUARMState *env, const ARMCPRegInfo *ri)
> +{
> +    uint64_t valid_mask;
> +
> +    valid_mask = R_MECID_MECID_MASK;
> +    return raw_read(env, ri) & valid_mask;
> +}

If you've properly masked writes, you don't need to mask reads too.


> +    { .name = "MECID_RL_A_EL3", .state = ARM_CP_STATE_AA64,
> +      .opc0 = 3, .opc1 = 6, .opc2 = 1, .crn = 10, .crm = 10,
> +      .access = PL2_RW, .accessfn = mecid_access,

.access = PL3_RW, no accessfn.


r~