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

[Mostafa Saleh]

Hi Eric,

On Mon, Mar 20, 2023 at 03:56:26PM +0100, Eric Auger wrote:
> 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
Will do.

> > + * 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?
Will do.

> > +                          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
Will do.

> > +    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?
Yes, this is checked in the STE patch in function
s2_pgtable_config_valid, where it checks that the max input will not
need more than 16 tables which means that the pagetable config is
inconsistent, which means the STE is ILLEGAL.

> > +    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
IAS = OAS for AA64. Described in "3.4 Address sizes".
The first check is actually not correct, as input address should be
compared to IAS(or OAS) while s2cfg.oas is effective PS.
I will rename s2cfg.oas to s2cfg.eff_ps to avoid confusion.
I will change the check here to be against s2t0sz and in this case it
causes stage-2 addr size fault.

I think the check for the IAS shouldn't be done here.

> > +     */
> > +    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
You mean replace SMMU_LEVELS with SMMU_LEVELs?

> >  
> > +#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
Will do.

> > +    (!((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?.
Yes, they are part of VMSA, however they are named this way as they are
part of SMMU headers, should we rename them to something else?

> > +
> > +/*
> > + * 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()
We do sanity check in STE parsing, I added this line as this function
is in a header file and anyone can use it, so to make it clear.
However, it is very trivial, I will remove the comment.

> > + * 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?
This returns the index of the page table descriptor in a concatenated
structure, this is actually close to what kvm calls it
“kvm_pgd_page_idx()”, however, I can call it something more clear as
pgd_concatenated_idx()?

> > +{
> > +    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.
Yes, I will use level_shift() instead.
> s/granule_sz/gsz or granule_sz to match the rest of the code
Will do.

Thanks,
Mostafa