Re: [RFC PATCH 08/16] hw/arm/smmuv3: Support S2AFFD

[Eric Auger]

Hi Mostafa,

On 2/5/23 10:44, Mostafa Saleh wrote:
> Parse S2AFFD from STE and use it in stage-2 translation.
>
> This is described in the SMMUv3 manual "5.2. Stream Table Entry" in
> "[181] S2AFFD".

from a patch structure pov, to me it would make more sense to add the
STE field decoding in
[RFC PATCH 06/16] hw/arm/smmuv3: Parse STE config for stage-2 and use it
in hw/arm/smmuv3: Add page table walk for stage-2
>
> HTTU is not supported, SW is expected to maintain the Access flag.
>
> This flag determines the behavior on access of a stage-2 page whose
> descriptor has AF == 0:
> - 0b0: An Access flag fault occurs (stall not supported).
> - 0b1: An Access flag fault never occurs.
>
> An Access fault takes priority over a Permission fault.
>
> Signed-off-by: Mostafa Saleh <smostafa@google.com>
> ---
>  hw/arm/smmu-common.c     | 10 ++++++++++
>  hw/arm/smmu-internal.h   |  2 ++
>  hw/arm/smmuv3-internal.h |  1 +
>  hw/arm/smmuv3.c          |  2 ++
>  4 files changed, 15 insertions(+)
>
> diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
> index df0d1dc024..541c427684 100644
> --- a/hw/arm/smmu-common.c
> +++ b/hw/arm/smmu-common.c
> @@ -434,6 +434,16 @@ static int smmu_ptw_64_s2(SMMUTransCfg *cfg,
>                                       pte_addr, pte, iova, gpa,
>                                       block_size >> 20);
>          }
> +
> +        /*
> +         * If S2AFFD and PTE.AF are 0 => fault. (5.2. Stream Table Entry)
> +         * An Access fault takes priority over a Permission fault.
> +         */
> +        if (!PTE_AF(pte) && !cfg->s2cfg.affd) {
> +            info->type = SMMU_PTW_ERR_ACCESS;
Wondering how you are going to differentiate page faults at S1 versus
page faults at S2.
Event number #10 differentiates both and recorded fields are different.

Do you handle that somewhere?

Thanks

Eric

> +            goto error;
> +        }
> +
>          ap = PTE_AP(pte);
>          if (is_permission_fault_s2(ap, perm)) {
>              info->type = SMMU_PTW_ERR_PERMISSION;
> diff --git a/hw/arm/smmu-internal.h b/hw/arm/smmu-internal.h
> index b02c05319f..7d3f76ce14 100644
> --- a/hw/arm/smmu-internal.h
> +++ b/hw/arm/smmu-internal.h
> @@ -66,6 +66,8 @@
>  #define PTE_APTABLE(pte) \
>      (extract64(pte, 61, 2))
>  
> +#define PTE_AF(pte) \
> +    (extract64(pte, 10, 1))
>  /*
>   * TODO: At the moment all transactions are considered as privileged (EL1)
>   * as IOMMU translation callback does not pass user/priv attributes.
> diff --git a/hw/arm/smmuv3-internal.h b/hw/arm/smmuv3-internal.h
> index ec64fb43a0..3ccb9d118e 100644
> --- a/hw/arm/smmuv3-internal.h
> +++ b/hw/arm/smmuv3-internal.h
> @@ -524,6 +524,7 @@ typedef struct CD {
>  #define STE_S2TG(x)        extract32((x)->word[5], 14, 2)
>  #define STE_S2PS(x)        extract32((x)->word[5], 16, 3)
>  #define STE_S2AA64(x)      extract32((x)->word[5], 19, 1)
> +#define STE_S2AFFD(x)      extract32((x)->word[5], 21, 1)
>  #define STE_S2HD(x)        extract32((x)->word[5], 24, 1)
>  #define STE_S2HA(x)        extract32((x)->word[5], 25, 1)
>  #define STE_S2S(x)         extract32((x)->word[5], 26, 1)
> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> index c49b341287..7884401475 100644
> --- a/hw/arm/smmuv3.c
> +++ b/hw/arm/smmuv3.c
> @@ -436,6 +436,8 @@ static int decode_ste(SMMUv3State *s, SMMUTransCfg *cfg,
>              goto bad_ste;
>          }
>  
> +        cfg->s2cfg.affd = STE_S2AFFD(ste);
> +
>          /* This is still here as stage 2 has not been fully enabled yet. */
>          qemu_log_mask(LOG_UNIMP, "SMMUv3 does not support stage 2 yet\n");
>          goto bad_ste;