Re: [PATCH 20/22] target/arm: Implement the granule protection check

[Peter Maydell]

On Tue, 24 Jan 2023 at 00:01, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Place the check at the end of get_phys_addr_with_struct,
> so that we check all physical results.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/ptw.c | 253 +++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 234 insertions(+), 19 deletions(-)
>
> diff --git a/target/arm/ptw.c b/target/arm/ptw.c
> index 3205339957..8249d93326 100644
> --- a/target/arm/ptw.c
> +++ b/target/arm/ptw.c
> @@ -32,11 +32,18 @@ typedef struct S1Translate {
>      void *out_host;
>  } S1Translate;
>
> -static bool get_phys_addr_with_struct(CPUARMState *env, S1Translate *ptw,
> -                                      target_ulong address,
> -                                      MMUAccessType access_type,
> -                                      GetPhysAddrResult *result,
> -                                      ARMMMUFaultInfo *fi)
> +static bool get_phys_addr_inner(CPUARMState *env, S1Translate *ptw,
> +                                target_ulong address,
> +                                MMUAccessType access_type,
> +                                GetPhysAddrResult *result,
> +                                ARMMMUFaultInfo *fi)
> +    __attribute__((nonnull));
> +
> +static bool get_phys_addr_outer(CPUARMState *env, S1Translate *ptw,
> +                                target_ulong address,
> +                                MMUAccessType access_type,
> +                                GetPhysAddrResult *result,
> +                                ARMMMUFaultInfo *fi)
>      __attribute__((nonnull));

I find these function names confusing, and the spec doesn't seem
to use the inner/outer terminology. Maybe there are clearer names?
Failing that, we should have a comment explaining the difference.

>  /* This mapping is common between ID_AA64MMFR0.PARANGE and TCR_ELx.{I}PS. */
> @@ -193,6 +200,197 @@ static bool regime_translation_disabled(CPUARMState 
> *env, ARMMMUIdx mmu_idx,
>      return (regime_sctlr(env, mmu_idx) & SCTLR_M) == 0;
>  }
>
> +static bool granule_protection_check(CPUARMState *env, uint64_t paddress,
> +                                     ARMSecuritySpace pspace,
> +                                     ARMMMUFaultInfo *fi)
> +{
> +    MemTxAttrs attrs = {
> +        .secure = true,
> +        .space = ARMSS_Root,
> +    };
> +    ARMCPU *cpu = env_archcpu(env);
> +    uint64_t gpccr = env->cp15.gpccr_el3;
> +    unsigned pps, pgs, l0gptsz, level = 0;
> +    uint64_t tableaddr, pps_mask, align, entry, index;
> +    AddressSpace *as;
> +    MemTxResult result;
> +    int gpi;
> +
> +    if (!FIELD_EX64(gpccr, GPCCR, GPC)) {
> +        return true;
> +    }
> +
> +    /*
> +     * GPC Priority 1 (R_GMGRR):
> +     * R_JWCSM: If the configuration of GPCCR_EL3 is invalid,
> +     * the access fails as GPT walk fault at level 0.
> +     */
> +
> +    /*
> +     * Configuration of PPS to a value exceeding the implemented
> +     * physical address size is invalid.
> +     */
> +    pps = FIELD_EX64(gpccr, GPCCR, PPS);
> +    if (pps > FIELD_EX64(cpu->isar.id_aa64mmfr0, ID_AA64MMFR0, PARANGE)) {
> +        goto fault_walk;
> +    }
> +    pps = pamax_map[pps];
> +    pps_mask = MAKE_64BIT_MASK(0, pps);
> +
> +    switch (FIELD_EX64(gpccr, GPCCR, SH)) {
> +    case 0b10: /* outer sharable */
> +        break;
> +    case 0b00: /* non-sharable */
> +    case 0b11: /* inner sharable */
> +        /* Inner and Outer non-cacheable requires Outer sharable. */

"Shareable" with an 'e' in all these comments.

> +        if (FIELD_EX64(gpccr, GPCCR, ORGN) == 0 &&
> +            FIELD_EX64(gpccr, GPCCR, IRGN) == 0) {
> +            goto fault_walk;
> +        }
> +        break;

Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM