Re: [PATCH v4 23/24] target/arm: Implement FEAT_HAFDBS, dirty bit portion

[Peter Maydell]

On Tue, 11 Oct 2022 at 04:41, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Perform the atomic update for hardware management of the dirty bit.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/cpu64.c |  2 +-
>  target/arm/ptw.c   | 20 ++++++++++++++++++++
>  2 files changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
> index fe1369fe96..0732796559 100644
> --- a/target/arm/cpu64.c
> +++ b/target/arm/cpu64.c
> @@ -1165,7 +1165,7 @@ static void aarch64_max_initfn(Object *obj)
>      cpu->isar.id_aa64mmfr0 = t;
>
>      t = cpu->isar.id_aa64mmfr1;
> -    t = FIELD_DP64(t, ID_AA64MMFR1, HAFDBS, 1);   /* FEAT_HAFDBS, AF only */
> +    t = FIELD_DP64(t, ID_AA64MMFR1, HAFDBS, 2);   /* FEAT_HAFDBS */
>      t = FIELD_DP64(t, ID_AA64MMFR1, VMIDBITS, 2); /* FEAT_VMID16 */
>      t = FIELD_DP64(t, ID_AA64MMFR1, VH, 1);       /* FEAT_VHE */
>      t = FIELD_DP64(t, ID_AA64MMFR1, HPDS, 1);     /* FEAT_HPDS */
> diff --git a/target/arm/ptw.c b/target/arm/ptw.c
> index 82b6ab029e..0dbbb7d4d4 100644
> --- a/target/arm/ptw.c
> +++ b/target/arm/ptw.c
> @@ -1484,10 +1484,30 @@ static bool get_phys_addr_lpae(CPUARMState *env, 
> S1Translate *ptw,
>      ap = extract32(attrs, 6, 2);
>
>      if (mmu_idx == ARMMMUIdx_Stage2 || mmu_idx == ARMMMUIdx_Stage2_S) {
> +        if (param.hd
> +            && extract64(attrs, 51, 1)  /* DBM */
> +            && access_type == MMU_DATA_STORE) {
> +            /*
> +             * Pre-emptively set S2AP[1], so that we compute EXEC properly.
> +             * C.f. AArch64.S2ApplyOutputPerms, which does the same thing.
> +             */
> +            ap |= 2;
> +            new_descriptor |= 1ull << 7;
> +        }
>          ns = mmu_idx == ARMMMUIdx_Stage2;
>          xn = extract64(attrs, 54, 2);
>          result->f.prot = get_S2prot(env, ap, xn, s1_is_el0);
>      } else {
> +        if (param.hd
> +            && extract64(attrs, 51, 1)  /* DBM */
> +            && access_type == MMU_DATA_STORE) {
> +            /*
> +             * Pre-emptively clear AP[2], so that we compute EXEC properly.
> +             * C.f. AArch64.S1ApplyOutputPerms, which does the same thing.
> +             */
> +            ap &= ~2;
> +            new_descriptor &= ~(1ull << 7);
> +        }

At this point in the code we've already merged the table
attributes into attrs. If the APTable[] bits forbid write permission
then HAFDBS isn't allowed to grant write permission. I think we
need to do this a bit further up, before we have merged the table
attributes in (so that the table attributes merge can force our
attribute bit back to 1 if the table attrs forbid writes).

(In the pseudocode the Permissions struct keeps track of the ap_table
and ap permissions settings separately and doesn't combine them until
AArch64.S1HasPermissionsFault().)

thanks
-- PMM