[RFC PATCH 6/7] target/arm/ptw: Add MECID checks

Gustavo Romero posted 7 patches 2 weeks, 4 days ago
Maintainers: Peter Maydell <peter.maydell@linaro.org>
[RFC PATCH 6/7] target/arm/ptw: Add MECID checks
Posted by Gustavo Romero 2 weeks, 4 days ago
Add MECID checks. The MECID registers were introduced before and this
commit uses them to perform the proper MECID checks, which is part of
FEAT_MEC implementation.

On MECID mismatches (wrong MECID) we return a substitute encrypted page
instead of the real encrypted page, which doesn't hurt performance on
memory accesses and behaves like FEAT_MEC design on the physical
hardware.

Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
---
 target/arm/internals.h |   2 +
 target/arm/ptw.c       | 215 ++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 216 insertions(+), 1 deletion(-)

diff --git a/target/arm/internals.h b/target/arm/internals.h
index 8ec2750847..d668948949 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -1473,6 +1473,8 @@ typedef struct GetPhysAddrResult {
      * with the stage1 protection.
      */
     int s2prot;
+    /* FEAT_MEC AMEC found in Block or Page descriptor. */
+    bool amec;
 } GetPhysAddrResult;
 
 /**
diff --git a/target/arm/ptw.c b/target/arm/ptw.c
index 8b8dc09e72..aacc32ba33 100644
--- a/target/arm/ptw.c
+++ b/target/arm/ptw.c
@@ -2090,6 +2090,17 @@ static bool get_phys_addr_lpae(CPUARMState *env, S1Translate *ptw,
         goto do_fault;
     }
 
+    /*
+     * XXX(gromero): Add checks for NS bit and AMEC0/1 bits, generating the due
+     * fault in case AMEC0/1 = 0 and AMEC bit is 1. So far we just get the AMEC
+     * bit directly in case of a Block or Page descriptor.
+     *
+     */
+    if (((level == 1 || level == 2) && (extract64(descriptor, 0, 2) == 1)) ||
+        level == 3) {
+        result->amec = extract64(descriptor, 63, 1);
+    }
+
     if ((descriptor & 2) && (level < 3)) {
         /*
          * Table entry. The top five bits are attributes which may
@@ -3766,6 +3777,208 @@ static bool get_phys_addr_nogpc(CPUARMState *env, S1Translate *ptw,
     }
 }
 
+static uint32_t *get_mecid_ptr(CPUARMState *env, hwaddr pa)
+{
+    MemoryRegion *mr;
+    AddressSpace *mec_as;
+    hwaddr mec_paddr, xlat;
+    MemTxAttrs memattrs = { 0x0 };
+
+    /* Find out the page number to use it as an offset in mec AS. */
+    mec_paddr = pa >> TARGET_PAGE_BITS;
+    mec_paddr *= 4; /* Assume MECID maximum size is 4 bytes. */
+    /* MECIDs are kept in their own Address Space. */
+    mec_as = cpu_get_address_space(env_cpu(env), ARMASIdx_MEC);
+    mr = address_space_translate(mec_as, mec_paddr, &xlat, NULL, true, memattrs);
+
+    /*
+     * Return pointer in the mec AS associated to physical address 'pa', which
+     * is used to store the MECID.
+     */
+    return memory_region_get_ram_ptr(mr) + xlat;
+}
+
+static uint32_t get_mecid(CPUARMState *env, hwaddr pa)
+{
+    return *get_mecid_ptr(env, pa);
+}
+
+static void set_mecid(CPUARMState *env, hwaddr pa, uint32_t mecid)
+{
+    *get_mecid_ptr(env, pa) = mecid;
+}
+
+/*
+ * Returns 'false' on MECID mismatch and 'true' on MECID match (success).
+ */
+static bool mecid_check(CPUARMState *env, S1Translate *ptw, hwaddr va,
+                        MMUAccessType access_type, GetPhysAddrResult *result,
+                        ARMMMUIdx s1_mmu_idx)
+{
+    ARMSecuritySpace ss = ptw->out_space;
+    /* Final physical address after translation. */
+    hwaddr pa = result->f.phys_addr;
+
+    switch (s1_mmu_idx) {
+    case ARMMMUIdx_Phys_S:
+    case ARMMMUIdx_Phys_NS:
+    case ARMMMUIdx_Phys_Root:
+    case ARMMMUIdx_Phys_Realm:
+    case ARMMMUIdx_Stage2:
+    case ARMMMUIdx_Stage2_S:
+    /* In the middle of a translation, so result->f.phys_addr has no PA yet. */
+        return true;
+        break;
+    default:
+        /* Pass */
+        break;
+    }
+
+    /* Find out which EL controls EMEC for Stage 1 translations. */
+    uint32_t el = regime_el(s1_mmu_idx) < 3 ? 2 : 3;
+
+    /* XXX(gromero): Do we need to check for SCR_EL3.SCTLR2En when el == 2? */
+    if (!(cpu_isar_feature(aa64_mec, env_archcpu(env)) &&
+        (env->cp15.sctlr2_el[el] & SCTLR2_EMEC))) {
+        /* FEAT_MEC is disabled. */
+        return true;
+    }
+
+    if (ss != ARMSS_Realm) {
+        /*
+         * GPT checks are already done, so if SS is Root here MECIDs are
+         * irrelevants for EL3 accesses. If SS is Secure or NonSecure that's not
+         * pertinent to FEAT_MEC. Hence, only proceed with MECID checks if SS is
+         * Realm.
+         */
+        return true;
+    }
+
+    bool amec = result->amec;
+    bool varange_lower = extract64(va, 55, 1) ? false : true;
+    /* MECID in register set given a translation regime. */
+    uint32_t mecid;
+
+    /* ARMMMUIdx after full ptw (all stages). */
+    ARMMMUIdx ptw_mmu_idx = ptw->in_mmu_idx;
+
+    switch (ptw_mmu_idx) {
+    case ARMMMUIdx_Stage2:
+    case ARMMMUIdx_Stage2_S:
+    /* In the middle of a two stage translation, so result->f.phys_addr has no PA yet. */
+        return true;
+        break;
+    default:
+        /* Pass */
+        break;
+    }
+
+    /*
+     * NB: No MECID is used for the ptw itself, i.e., used for the level
+     * lookups, so AArch64.S1TTWalkMECID() and AArch64.S2TTWalkMECID() are not
+     * relevant for our FEAT_MEC implementation.
+     */
+    bool is_pa_from_s2 = regime_is_stage2(ptw_mmu_idx);
+    bool is_mmu_disabled = regime_translation_disabled(env, ptw_mmu_idx, ss);
+    if (is_pa_from_s2) { /* PA from Stage 2. */
+        /* As per AArch64.S2OutputMECID(). */
+        mecid = amec ? env->cp15.vmecid_a_el2 : env->cp15.vmecid_p_el2;
+
+    } else { /* PA from Stage 1. */
+        if (is_mmu_disabled) { /* PA from Stage 1 and MMU is disabled. */
+            /* As per AArch64.S1DisabledOutputMECID(). */
+            switch (s1_mmu_idx) {
+            case ARMMMUIdx_E3:
+                /* MECID = 0 (default), so return without any check. */
+                return true;
+                break;
+            case ARMMMUIdx_E2:
+            case ARMMMUIdx_E20_0:
+            case ARMMMUIdx_E20_2:
+            case ARMMMUIdx_E20_2_PAN:
+                mecid = env->cp15.mecid_p0_el2;
+                break;
+            case ARMMMUIdx_E10_0:
+            case ARMMMUIdx_E10_1:
+                mecid = env->cp15.vmecid_p_el2;
+                break;
+            default:
+                g_assert_not_reached();
+            }
+
+        } else { /* PA from Stage 1 and MMU is enabled. */
+            /* As per AArch64.S1OutputMECID(). */
+            switch (s1_mmu_idx) {
+            case ARMMMUIdx_E3:
+                mecid = env->cp15.mecid_rl_a_el3;
+                break;
+            case ARMMMUIdx_E2:
+                mecid = amec ? env->cp15.mecid_a0_el2 : env->cp15.mecid_p0_el2;
+                break;
+            case ARMMMUIdx_E20_0:
+            case ARMMMUIdx_E20_2:
+            case ARMMMUIdx_E20_2_PAN:
+                if (varange_lower) {
+                    mecid = amec ? env->cp15.mecid_a0_el2 : env->cp15.mecid_p0_el2;
+                } else {
+                    mecid = amec ? env->cp15.mecid_a1_el2 : env->cp15.mecid_p1_el2;
+                }
+                break;
+            case ARMMMUIdx_E10_0:
+            case ARMMMUIdx_E10_1:
+                mecid = env->cp15.vmecid_p_el2;
+                break;
+            default:
+                g_assert_not_reached();
+            }
+        }
+    }
+
+    /*
+     * MECID is updated on data store and checked on other access types.
+     */
+    if (access_type == MMU_DATA_STORE) {
+       /* Store MECID for physical address 'pa'. */
+       set_mecid(env, pa, mecid);
+       return true;
+    } else {
+        uint32_t stored_mecid;
+        /* Load the MECID stored in memory for physical address 'pa'. */
+        stored_mecid = get_mecid(env, pa);
+        if (stored_mecid == mecid) {
+            /* MECID is correct. */
+            return true;
+        } else {
+            /* MECID is incorrect, so return the substitute encrypted page. */
+            result->f.phys_addr = 0x0; /* Start of the page. */
+            result->f.attrs.encrypted = true; /* Substitute encrypted page. */
+            return false;
+        }
+    }
+}
+
+static bool get_phys_addr_mec(CPUARMState *env, S1Translate *ptw,
+                              vaddr address,
+                              MMUAccessType access_type, MemOp memop,
+                              GetPhysAddrResult *result,
+                              ARMMMUFaultInfo *fi)
+
+{
+    /*
+     * After 'address' is resolved by get_phys_addr_nogpc(), ptw->in_mmu_idx can
+     * change depending on the translation stages, hence stash it.
+     */
+    ARMMMUIdx s1_mmu_idx = ptw->in_mmu_idx;
+
+    if (get_phys_addr_gpc(env, ptw, address, access_type, memop, result, fi)) {
+        return true; /* Translation fault. */
+    }
+    if (!mecid_check(env, ptw, address, access_type, result, s1_mmu_idx)) {
+        return true; /* MECID mismatch. */
+    }
+    return false;
+}
+
 static bool get_phys_addr_gpc(CPUARMState *env, S1Translate *ptw,
                               vaddr address,
                               MMUAccessType access_type, MemOp memop,
@@ -3907,7 +4120,7 @@ bool get_phys_addr(CPUARMState *env, vaddr address,
         .in_prot_check = 1 << access_type,
     };
 
-    return get_phys_addr_gpc(env, &ptw, address, access_type,
+    return get_phys_addr_mec(env, &ptw, address, access_type,
                              memop, result, fi);
 }
 
-- 
2.34.1
Re: [RFC PATCH 6/7] target/arm/ptw: Add MECID checks
Posted by Richard Henderson 1 week, 3 days ago
On 3/19/26 12:23, Gustavo Romero wrote:
> Add MECID checks. The MECID registers were introduced before and this
> commit uses them to perform the proper MECID checks, which is part of
> FEAT_MEC implementation.
> 
> On MECID mismatches (wrong MECID) we return a substitute encrypted page
> instead of the real encrypted page, which doesn't hurt performance on
> memory accesses and behaves like FEAT_MEC design on the physical
> hardware.
> 
> Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
> ---
>   target/arm/internals.h |   2 +
>   target/arm/ptw.c       | 215 ++++++++++++++++++++++++++++++++++++++++-
>   2 files changed, 216 insertions(+), 1 deletion(-)
> 
> diff --git a/target/arm/internals.h b/target/arm/internals.h
> index 8ec2750847..d668948949 100644
> --- a/target/arm/internals.h
> +++ b/target/arm/internals.h
> @@ -1473,6 +1473,8 @@ typedef struct GetPhysAddrResult {
>        * with the stage1 protection.
>        */
>       int s2prot;
> +    /* FEAT_MEC AMEC found in Block or Page descriptor. */
> +    bool amec;
>   } GetPhysAddrResult;
>   
>   /**
> diff --git a/target/arm/ptw.c b/target/arm/ptw.c
> index 8b8dc09e72..aacc32ba33 100644
> --- a/target/arm/ptw.c
> +++ b/target/arm/ptw.c
> @@ -2090,6 +2090,17 @@ static bool get_phys_addr_lpae(CPUARMState *env, S1Translate *ptw,
>           goto do_fault;
>       }
>   
> +    /*
> +     * XXX(gromero): Add checks for NS bit and AMEC0/1 bits, generating the due
> +     * fault in case AMEC0/1 = 0 and AMEC bit is 1. So far we just get the AMEC
> +     * bit directly in case of a Block or Page descriptor.
> +     *
> +     */
> +    if (((level == 1 || level == 2) && (extract64(descriptor, 0, 2) == 1)) ||
> +        level == 3) {

This test is exactly ...

> +        result->amec = extract64(descriptor, 63, 1);
> +    }
> +
>       if ((descriptor & 2) && (level < 3)) {

... the inverse of this one.  Notice the comment that directly follows:

     /*
      * Block entry at level 1 or 2, or page entry at level 3.
      * These are basically the same thing, although the number

So move the assignment of amec later, with no additional test.

>           /*
>            * Table entry. The top five bits are attributes which may
> @@ -3766,6 +3777,208 @@ static bool get_phys_addr_nogpc(CPUARMState *env, S1Translate *ptw,
>       }
>   }
>   
> +static uint32_t *get_mecid_ptr(CPUARMState *env, hwaddr pa)
> +{
> +    MemoryRegion *mr;
> +    AddressSpace *mec_as;
> +    hwaddr mec_paddr, xlat;
> +    MemTxAttrs memattrs = { 0x0 };
> +
> +    /* Find out the page number to use it as an offset in mec AS. */
> +    mec_paddr = pa >> TARGET_PAGE_BITS;
> +    mec_paddr *= 4; /* Assume MECID maximum size is 4 bytes. */
> +    /* MECIDs are kept in their own Address Space. */
> +    mec_as = cpu_get_address_space(env_cpu(env), ARMASIdx_MEC);
> +    mr = address_space_translate(mec_as, mec_paddr, &xlat, NULL, true, memattrs);
> +
> +    /*
> +     * Return pointer in the mec AS associated to physical address 'pa', which
> +     * is used to store the MECID.
> +     */
> +    return memory_region_get_ram_ptr(mr) + xlat;
> +}

In allocation_tag_mem_probe, where we're performing a similar function, I test 
memory_region_is_ram and log an error.  In addition, we have to manually mark these ram 
pages dirty for migration.

Alternately...

> +
> +static uint32_t get_mecid(CPUARMState *env, hwaddr pa)
> +{
> +    return *get_mecid_ptr(env, pa);
> +}
> +
> +static void set_mecid(CPUARMState *env, hwaddr pa, uint32_t mecid)
> +{
> +    *get_mecid_ptr(env, pa) = mecid;
> +}

... drop the direct use of pointers and use address_space_{ldl,stl} instead.

For MTE, there was the additional complication of user-only, which doesn't have these 
separate address spaces, as well as dealing with larger blocks of tag memory.

For MEC, you're only ever touching one entry at a time.

> +
> +/*
> + * Returns 'false' on MECID mismatch and 'true' on MECID match (success).
> + */
> +static bool mecid_check(CPUARMState *env, S1Translate *ptw, hwaddr va,
> +                        MMUAccessType access_type, GetPhysAddrResult *result,
> +                        ARMMMUIdx s1_mmu_idx)
> +{
> +    ARMSecuritySpace ss = ptw->out_space;
> +    /* Final physical address after translation. */
> +    hwaddr pa = result->f.phys_addr;
> +
> +    switch (s1_mmu_idx) {
> +    case ARMMMUIdx_Phys_S:
> +    case ARMMMUIdx_Phys_NS:
> +    case ARMMMUIdx_Phys_Root:
> +    case ARMMMUIdx_Phys_Realm:
> +    case ARMMMUIdx_Stage2:
> +    case ARMMMUIdx_Stage2_S:
> +    /* In the middle of a translation, so result->f.phys_addr has no PA yet. */
> +        return true;
> +        break;
> +    default:
> +        /* Pass */
> +        break;
> +    }

Why do you need to test s1_mmu_idx?
Surely the ptw->in_mmu_idx test below is better?

> +
> +    /* Find out which EL controls EMEC for Stage 1 translations. */
> +    uint32_t el = regime_el(s1_mmu_idx) < 3 ? 2 : 3;
> +
> +    /* XXX(gromero): Do we need to check for SCR_EL3.SCTLR2En when el == 2? */
> +    if (!(cpu_isar_feature(aa64_mec, env_archcpu(env)) &&
> +        (env->cp15.sctlr2_el[el] & SCTLR2_EMEC))) {
> +        /* FEAT_MEC is disabled. */
> +        return true;
> +    }
> +
> +    if (ss != ARMSS_Realm) {
> +        /*
> +         * GPT checks are already done, so if SS is Root here MECIDs are
> +         * irrelevants for EL3 accesses. If SS is Secure or NonSecure that's not
> +         * pertinent to FEAT_MEC. Hence, only proceed with MECID checks if SS is
> +         * Realm.
> +         */
> +        return true;
> +    }

Sort these tests in order of likelyhood and simplicity.

For instance, ss == Realm is least likely and is a trivial test.

In addition, ss == Realm implies ARM_FEATURE_AARCH64, which you have forgotten to test 
before testing isar feature aa64_mec.

> +
> +    bool amec = result->amec;
> +    bool varange_lower = extract64(va, 55, 1) ? false : true;

Use "!" not "?:".

But extracting varange_upper instead would simplify merging E2 with E2&0 regimes, like so:

     varange_upper = regime_has_2_ranges(idx) && extract64(va, 55, 1)

> +    /* MECID in register set given a translation regime. */
> +    uint32_t mecid;
> +
> +    /* ARMMMUIdx after full ptw (all stages). */
> +    ARMMMUIdx ptw_mmu_idx = ptw->in_mmu_idx;
> +
> +    switch (ptw_mmu_idx) {
> +    case ARMMMUIdx_Stage2:
> +    case ARMMMUIdx_Stage2_S:
> +    /* In the middle of a two stage translation, so result->f.phys_addr has no PA yet. */
> +        return true;
> +        break;
> +    default:
> +        /* Pass */
> +        break;
> +    }
> +
> +    /*
> +     * NB: No MECID is used for the ptw itself, i.e., used for the level
> +     * lookups, so AArch64.S1TTWalkMECID() and AArch64.S2TTWalkMECID() are not
> +     * relevant for our FEAT_MEC implementation.
> +     */
> +    bool is_pa_from_s2 = regime_is_stage2(ptw_mmu_idx);

You just rejected the stage2 mmu indexes.

> +    bool is_mmu_disabled = regime_translation_disabled(env, ptw_mmu_idx, ss);
> +    if (is_pa_from_s2) { /* PA from Stage 2. */
> +        /* As per AArch64.S2OutputMECID(). */
> +        mecid = amec ? env->cp15.vmecid_a_el2 : env->cp15.vmecid_p_el2;

So this isn't reachable.
This suggests the switch above is wrong?

> +
> +    } else { /* PA from Stage 1. */
> +        if (is_mmu_disabled) { /* PA from Stage 1 and MMU is disabled. */
> +            /* As per AArch64.S1DisabledOutputMECID(). */
> +            switch (s1_mmu_idx) {
> +            case ARMMMUIdx_E3:
> +                /* MECID = 0 (default), so return without any check. */
> +                return true;
> +                break;

Unreachable break.

> +            case ARMMMUIdx_E2:
> +            case ARMMMUIdx_E20_0:
> +            case ARMMMUIdx_E20_2:
> +            case ARMMMUIdx_E20_2_PAN:
> +                mecid = env->cp15.mecid_p0_el2;
> +                break;
> +            case ARMMMUIdx_E10_0:
> +            case ARMMMUIdx_E10_1:
> +                mecid = env->cp15.vmecid_p_el2;
> +                break;

There are lots of these you've missed.
Better to test regime_el instead.

> +            default:
> +                g_assert_not_reached();
> +            }
> +
> +        } else { /* PA from Stage 1 and MMU is enabled. */
> +            /* As per AArch64.S1OutputMECID(). */
> +            switch (s1_mmu_idx) {
> +            case ARMMMUIdx_E3:
> +                mecid = env->cp15.mecid_rl_a_el3;
> +                break;
> +            case ARMMMUIdx_E2:
> +                mecid = amec ? env->cp15.mecid_a0_el2 : env->cp15.mecid_p0_el2;
> +                break;
> +            case ARMMMUIdx_E20_0:
> +            case ARMMMUIdx_E20_2:
> +            case ARMMMUIdx_E20_2_PAN:
> +                if (varange_lower) {
> +                    mecid = amec ? env->cp15.mecid_a0_el2 : env->cp15.mecid_p0_el2;
> +                } else {
> +                    mecid = amec ? env->cp15.mecid_a1_el2 : env->cp15.mecid_p1_el2;
> +                }
> +                break;

E2 and E20_* can be merged with varange_upper, per above.

> +static bool get_phys_addr_mec(CPUARMState *env, S1Translate *ptw,
> +                              vaddr address,
> +                              MMUAccessType access_type, MemOp memop,
> +                              GetPhysAddrResult *result,
> +                              ARMMMUFaultInfo *fi)
> +
> +{
> +    /*
> +     * After 'address' is resolved by get_phys_addr_nogpc(), ptw->in_mmu_idx can
> +     * change depending on the translation stages, hence stash it.
> +     */
> +    ARMMMUIdx s1_mmu_idx = ptw->in_mmu_idx;
> +
> +    if (get_phys_addr_gpc(env, ptw, address, access_type, memop, result, fi)) {
> +        return true; /* Translation fault. */
> +    }
> +    if (!mecid_check(env, ptw, address, access_type, result, s1_mmu_idx)) {
> +        return true; /* MECID mismatch. */
> +    }

MECID mismatch does *not* raise an exception, therefore returning true is incorrect.

Thus the return value from mecid_check is really unused.


r~