Re: [RFC PATCH 05/16] hw/arm/smmuv3: Add page table walk for stage-2

[Mostafa Saleh <smostafa@google.com>]

Hi Eric,

On Wed, Feb 15, 2023 at 05:52:39PM +0100, Eric Auger wrote:
> > In preparation for adding stage-2 support. Add Stage-2 PTW code.
> > Only Aarch64 fromat is supported as stage-1.
> format
I will update it.

> > +        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(level, iova, subpage_size,
> > +                             baseaddr, offset, pte);
> I can the trace point names should be updated as well (and
> differentiated between S1/S2)
I was thinking we could leave those with stage argument, and only
update trace_smmu_ptw_level to have stage argument as the others.

> > +        if (is_permission_fault_s2(ap, perm)) {
> > +            info->type = SMMU_PTW_ERR_PERMISSION;
> don't we have to different S1 versus S2 faults?
Yes, I missed that, I see setting info->u.f_walk_eabt.s2 should be
enough, this will set the S2 field in the fault event.

> >  int smmu_ptw(SMMUTransCfg *cfg, dma_addr_t iova, IOMMUAccessFlags perm,
> >               SMMUTLBEntry *tlbe, SMMUPTWEventInfo *info)
> >  {
> > -    if (!cfg->aa64) {
> > -        /*
> > -         * This code path is not entered as we check this while decoding
> > -         * the configuration data in the derived SMMU model.
> > -         */
> > -        g_assert_not_reached();
> if that's still true for S2, maybe keep that check here upfront?
Stage-2 is checked in STE parsing and throws BAD_STE if not aa64,
which I believe is not correct, however I think we can just call
g_assert_not_reached() during STE parsing, I don’t see added value for
saving this field in SMMUTransCfg if we don’t use it.
I am not sure why this check exists for stage-1 as it is hardcoded in
decode_cd anyway.

> > +{
> > +    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_MAX_LEVELS - start_level) * (granule - 3) + granule;
> can't we factorize anything with the S1 PTW?
> indexmask = (1ULL << (inputsize - (stride * (4 - level)))) - 1;
Yes, I think we can refactor some of these in common functions/macros, I
will do that in v2.


> > @@ -28,6 +28,7 @@
> >  #define SMMU_PCI_DEVFN(sid)   (sid & 0xFF)
> >  
> >  #define SMMU_MAX_VA_BITS      48
> > +#define SMMU_MAX_LEVELS       4
> can't this be reused as well with S1 PTW?
I believe yes, I will update it.

Thanks,
Mostafa