Re: [RFC PATCH v2 04/11] hw/arm/smmuv3: Add page table walk for stage-2

[Eric Auger <eric.auger@redhat.com>]

Hi Mostafa,

On 2/26/23 23:06, Mostafa Saleh wrote:
> In preparation for adding stage-2 support, add Stage-2 PTW code.
> Only Aarch64 format is supported as stage-1.
>
> Nesting stage-1 and stage-2 is not supported right now.
>
> HTTU is not supported, SW is expected to maintain the Access flag.
> This is described in the SMMUv3 manual "5.2. Stream Table Entry" in
> "[181] S2AFFD".
> 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.
>
> Checks for IPA and output PA are done according to the user manual
> "3.4 Address sizes".
replace user manual by the exact ref of the doc. I guess this is IHI0070E
>
> Signed-off-by: Mostafa Saleh <smostafa@google.com>
> ---
> Changes in v2:
> - Squash S2AFF PTW code.
> - Use common functions between stage-1 and stage-2.
> - Add checks for IPA and out PA.
> ---
>  hw/arm/smmu-common.c   | 132 ++++++++++++++++++++++++++++++++++++++++-
>  hw/arm/smmu-internal.h |  39 ++++++++++++
>  2 files changed, 170 insertions(+), 1 deletion(-)
>
> diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
> index b49c1affdb..3f448bc82e 100644
> --- a/hw/arm/smmu-common.c
> +++ b/hw/arm/smmu-common.c
> @@ -363,6 +363,130 @@ error:
>      return -EINVAL;
>  }
>  
> +/**
> + * smmu_ptw_64_s2 - VMSAv8-64 Walk of the page tables for a given IOVA
> + * for stage-2.
> + * @cfg: translation config
> + * @iova: iova to translate
> + * @perm: access type
> + * @tlbe: SMMUTLBEntry (out)
> + * @info: handle to an error info
> + *
> + * Return 0 on success, < 0 on error. In case of error, @info is filled
> + * and tlbe->perm is set to IOMMU_NONE.
> + * Upon success, @tlbe is filled with translated_addr and entry
> + * permission rights.
> + */
> +static int smmu_ptw_64_s2(SMMUTransCfg *cfg,
> +                          dma_addr_t iova, IOMMUAccessFlags perm,
Rename iova into ipa?
> +                          SMMUTLBEntry *tlbe, SMMUPTWEventInfo *info)
> +{
> +    const int stage = 2;
> +    int granule_sz = cfg->s2cfg.granule_sz;
> +    /* ARM ARM: Table D8-7. */
You may refer the full reference
in  DDI0487I.a as the chapter/table may vary. For instance in
ARM DDI 0487F.c D8 corresponds to the activity monitor extensions
> +    int inputsize = 64 - cfg->s2cfg.tsz;
> +    int level = get_start_level(cfg->s2cfg.sl0, granule_sz);
> +    int stride = SMMU_STRIDE(granule_sz);
> +    int idx = pgd_idx(level, granule_sz, iova);
> +    /*
> +     * Get the ttb from concatenated structure.
> +     * The offset is the idx * size of each ttb(number of ptes * (sizeof(pte))
> +     */
what I don't get is the spec that concatenated tables are used if the
initial lookup level would require less or equal than 16 entries. I
don't see such kind of check or is done implicitly somewhere else?
> +    uint64_t baseaddr = extract64(cfg->s2cfg.vttb, 0, 48) + (1 << stride) *
> +                                  idx * sizeof(uint64_t);
> +    dma_addr_t indexmask = SMMU_IDXMSK(inputsize, stride, level);
> +
> +    baseaddr &= ~indexmask;
> +
> +    /*
> +     * If the input address of a transaction exceeds the size of the IAS, a
> +     * stage 1 Address Size fault occurs.
> +     * For AA64, IAS = OAS
then you may use a local variable max_as = cfg->s2cfg.oas used below and
in 

if (gpa >= (1ULL << cfg->s2cfg.oas)) {
this is not obvious though that the ias = oas. Where does it come from?

In implementations of SMMUv3.1 and later, this value is Reserved and S2PS behaves as 0b110 (52 bits).
Effective_S2PS = MIN(STE.S2PS, SMMU_IDR5.OAS);
OAS is a maximum of 52 bits in SMMUv3.1 and later

> +     */
> +    if (iova >= (1ULL << cfg->s2cfg.oas)) {
> +        info->type = SMMU_PTW_ERR_ADDR_SIZE;
> +        info->stage = 1;
> +        goto error_no_stage;
> +    }
> +
> +    while (level < SMMU_LEVELS) {
I would rename SMMU_LEVELs
> +        uint64_t subpage_size = 1ULL << level_shift(level, granule_sz);
> +        uint64_t mask = subpage_size - 1;
> +        uint32_t offset = iova_level_offset(iova, inputsize, level, granule_sz);
> +        uint64_t pte, gpa;
> +        dma_addr_t pte_addr = baseaddr + offset * sizeof(pte);
> +        uint8_t ap;
> +
> +        if (get_pte(baseaddr, offset, &pte, info)) {
> +                goto error;
> +        }
> +        trace_smmu_ptw_level(stage, level, iova, subpage_size,
> +                             baseaddr, offset, pte);
> +        if (is_invalid_pte(pte) || is_reserved_pte(pte, level)) {
> +            trace_smmu_ptw_invalid_pte(stage, level, baseaddr,
> +                                       pte_addr, offset, pte);
> +            break;
> +        }
> +
> +        if (is_table_pte(pte, level)) {
> +            baseaddr = get_table_pte_address(pte, granule_sz);
> +            level++;
> +            continue;
> +        } else if (is_page_pte(pte, level)) {
> +            gpa = get_page_pte_address(pte, granule_sz);
> +            trace_smmu_ptw_page_pte(stage, level, iova,
> +                                    baseaddr, pte_addr, pte, gpa);
> +        } else {
> +            uint64_t block_size;
> +
> +            gpa = get_block_pte_address(pte, level, granule_sz,
> +                                        &block_size);
> +            trace_smmu_ptw_block_pte(stage, level, baseaddr,
> +                                     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;
> +            goto error;
> +        }
> +
> +        ap = PTE_AP(pte);
> +        if (is_permission_fault_s2(ap, perm)) {
> +            info->type = SMMU_PTW_ERR_PERMISSION;
> +            goto error;
> +        }
> +
> +        /*
> +         * The address output from the translation causes a stage 2 Address
> +         * Size fault if it exceeds the effective PA output range.
> +         */
> +        if (gpa >= (1ULL << cfg->s2cfg.oas)) {
> +            info->type = SMMU_PTW_ERR_ADDR_SIZE;
> +            goto error;
> +        }
> +
> +        tlbe->entry.translated_addr = gpa;
> +        tlbe->entry.iova = iova & ~mask;
> +        tlbe->entry.addr_mask = mask;
> +        tlbe->entry.perm = ap;
> +        tlbe->level = level;
> +        tlbe->granule = granule_sz;
> +        return 0;
> +    }
> +    info->type = SMMU_PTW_ERR_TRANSLATION;
> +
> +error:
> +    info->stage = 2;
> +error_no_stage:
> +    tlbe->entry.perm = IOMMU_NONE;
> +    return -EINVAL;
> +}
> +
>  /**
>   * smmu_ptw - Walk the page tables for an IOVA, according to @cfg
>   *
> @@ -377,7 +501,13 @@ error:
>  int smmu_ptw(SMMUTransCfg *cfg, dma_addr_t iova, IOMMUAccessFlags perm,
>               SMMUTLBEntry *tlbe, SMMUPTWEventInfo *info)
>  {
> -    return smmu_ptw_64_s1(cfg, iova, perm, tlbe, info);
> +    if (cfg->stage == 1) {
> +        return smmu_ptw_64_s1(cfg, iova, perm, tlbe, info);
> +    } else if (cfg->stage == 2) {
> +        return smmu_ptw_64_s2(cfg, iova, perm, tlbe, info);
> +    }
> +
> +    g_assert_not_reached();
>  }
>  
>  /**
> diff --git a/hw/arm/smmu-internal.h b/hw/arm/smmu-internal.h
> index 2d75b31953..f79c389cd3 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.
> @@ -73,6 +75,9 @@
>  #define is_permission_fault(ap, perm) \
>      (((perm) & IOMMU_WO) && ((ap) & 0x2))
>  
> +#define is_permission_fault_s2(ap, perm) \
I would rename ap param into s2ap. This is the name of the field in the spec
> +    (!((ap & perm) == perm))
> +
>  #define PTE_AP_TO_PERM(ap) \
>      (IOMMU_ACCESS_FLAG(true, !((ap) & 0x2)))
>  
> @@ -96,6 +101,40 @@ uint64_t iova_level_offset(uint64_t iova, int inputsize,
>              MAKE_64BIT_MASK(0, gsz - 3);
>  }
>  
> +#define SMMU_MAX_S2_CONCAT    16
it is not really an SMMU property (same as SMMU_LEVELS by the way). This
is rather something related to VMSA spec, no?.
> +
> +/*
> + * Relies on correctness of gran and sl0 from caller.
I would remove the above line as we generally expect the caller to
behave properly or do you mean we do not handle any sanity check?
I refer to some of them in target/arm/ptw.c:check_s2_mmu_setup()
> + * FEAT_LPA2 and FEAT_TTST are not implemented.
> + */
> +static inline int get_start_level(int sl0 , int gran)
> +{
> +    /* ARM ARM: Table D8-12. */
> +    if (gran == 12) {
> +        return 2 - sl0;
> +    }
> +    /* ARM ARM: Table D8-22 and Table D8-31. */
> +    return 3 - sl0;
> +}
> +
> +/*
> + * Index in a concatenated first level stage-2 page table.
> + * ARM ARM: D8.2.2 Concatenated translation tables.
> + */
> +static inline int pgd_idx(int start_level, int granule, dma_addr_t iova)
then the name of the function may better reflect what is does?
> +{
> +    uint64_t ret;
> +    /*
> +     * Get the number of bits handled by next levels, then any extra bits in
> +     * the address should index the concatenated tables. This relation can
> +     * deduced from tables in ARM ARM: D8.2.7-9
> +     */
> +    int shift = (SMMU_LEVELS - start_level) * (granule - 3) + granule;
this looks like level_shift() with level = start_level - 1, no.
s/granule_sz/gsz or granule_sz to match the rest of the code
> +
> +    ret = iova >> shift;
> +    return ret;
> +}
> +
>  #define SMMU_IOTLB_ASID(key) ((key).asid)
>  
>  typedef struct SMMUIOTLBPageInvInfo {
Thanks

Eric