Re: [RFC PATCH v2 04/13] hw/arm/smmuv3: Translate CD and TT using stage-2 table

[Mostafa Saleh <smostafa@google.com>]

Hi Eric,

On Thu, Apr 18, 2024 at 02:51:59PM +0200, Eric Auger wrote:
> Hi Mostafa,
> 
> On 4/8/24 16:08, Mostafa Saleh wrote:
> > According to the user manual (ARM IHI 0070 F.b),
> s/user manual/ARM SMMU architecture specification
> > In "5.2 Stream Table Entry":
> >  [51:6] S1ContextPtr
> >  If Config[1] == 1 (stage 2 enabled), this pointer is an IPA translated by
> >  stage 2 and the programmed value must be within the range of the IAS.
> >
> > In "5.4.1 CD notes":
> >  The translation table walks performed from TTB0 or TTB1 are always performed
> >  in IPA space if stage 2 translations are enabled.
> >
> > So translate both the CD and the TTBx in this patch if nested
> translate the S1 context descriptor pointer and TTBx base addresses
> through the S2 stage (IPA -> PA)
> 
> You may describe what you put in place to do the translation in the
> commit msg, new functions, macro, ...

Will do.

> > translation is requested.
> >
> > Signed-off-by: Mostafa Saleh <smostafa@google.com>
> > ---
> >  hw/arm/smmuv3.c              | 49 ++++++++++++++++++++++++++++++------
> >  include/hw/arm/smmu-common.h | 17 +++++++++++++
> >  2 files changed, 59 insertions(+), 7 deletions(-)
> >
> > diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> > index 897f8fe085..a7cf543acc 100644
> > --- a/hw/arm/smmuv3.c
> > +++ b/hw/arm/smmuv3.c
> > @@ -337,14 +337,36 @@ static int smmu_get_ste(SMMUv3State *s, dma_addr_t addr, STE *buf,
> >  
> >  }
> >  
> > +static SMMUTranslationStatus smmuv3_do_translate(SMMUv3State *s, hwaddr addr,
> > +                                                 SMMUTransCfg *cfg,
> > +                                                 SMMUEventInfo *event,
> > +                                                 IOMMUAccessFlags flag,
> > +                                                 SMMUTLBEntry **out_entry);
> >  /* @ssid > 0 not supported yet */
> > -static int smmu_get_cd(SMMUv3State *s, STE *ste, uint32_t ssid,
> > -                       CD *buf, SMMUEventInfo *event)
> > +static int smmu_get_cd(SMMUv3State *s, STE *ste, SMMUTransCfg *cfg,
> > +                       uint32_t ssid, CD *buf, SMMUEventInfo *event)
> >  {
> >      dma_addr_t addr = STE_CTXPTR(ste);
> >      int ret, i;
> > +    SMMUTranslationStatus status;
> > +    SMMUTLBEntry *entry;
> >  
> >      trace_smmuv3_get_cd(addr);
> > +
> > +    if (cfg->stage == SMMU_NESTED) {
> > +        CALL_FUNC_CFG_S2(cfg, status, smmuv3_do_translate, s, addr,
> > +                         cfg, event, IOMMU_RO, &entry);
> the fact we pass 2 times cfg looks pretty weird from a caller pov. See
> my comment below.

Yes, I don’t like it also as I mentioned in the cover letter,
(see me comment below also)

> 
> do we somewhere check addr is within the proper addr range, IAS if S2,
> OAS if S1. This was missing for S1 but I think it is worth improving now.
> see 3.4.3
Yes, this was added in the next patch.

> > +        /*
> > +         * It is not clear what should happen if this fails, so we return here
> > +         * which gets propagated as a translation error.
> but the error event might be different, no?

But the event is passed to the translate function so the right translation
error will be in the event (addr size, permission…).
It isn't clear to me from the specs if this should be a translation error
or some F_CD_FETCH/C_BAD_CD, and hence the comment, but I though the
translation error info would be more useful for SW that's why I used it.

> > +         */
> > +        if (status != SMMU_TRANS_SUCCESS) {
> > +            return -EINVAL;
> > +        }
> > +
> > +        addr = CACHED_ENTRY_TO_ADDR(entry, addr);
> > +    }
> > +
> >      /* TODO: guarantee 64-bit single-copy atomicity */
> >      ret = dma_memory_read(&address_space_memory, addr, buf, sizeof(*buf),
> >                            MEMTXATTRS_UNSPECIFIED);
> > @@ -659,10 +681,13 @@ static int smmu_find_ste(SMMUv3State *s, uint32_t sid, STE *ste,
> >      return 0;
> >  }
> >  
> > -static int decode_cd(SMMUTransCfg *cfg, CD *cd, SMMUEventInfo *event)
> > +static int decode_cd(SMMUv3State *s, SMMUTransCfg *cfg,
> > +                     CD *cd, SMMUEventInfo *event)
> >  {
> >      int ret = -EINVAL;
> >      int i;
> > +    SMMUTranslationStatus status;
> > +    SMMUTLBEntry *entry;
> >  
> >      if (!CD_VALID(cd) || !CD_AARCH64(cd)) {
> >          goto bad_cd;
> > @@ -713,6 +738,17 @@ static int decode_cd(SMMUTransCfg *cfg, CD *cd, SMMUEventInfo *event)
> >  
> >          tt->tsz = tsz;
> >          tt->ttb = CD_TTB(cd, i);
> > +
> > +        /* Translate the TTBx, from IPA to PA if nesting is enabled. */
> > +        if (cfg->stage == SMMU_NESTED) {
> > +            CALL_FUNC_CFG_S2(cfg, status, smmuv3_do_translate, s,
> > +                             tt->ttb, cfg, event, IOMMU_RO, &entry);
> > +            /* See smmu_get_cd(). */
> ditto
> > +            if (status != SMMU_TRANS_SUCCESS) {
> > +                return -EINVAL;
> > +            }
> > +            tt->ttb = CACHED_ENTRY_TO_ADDR(entry, tt->ttb);
> > +        }
> >          if (tt->ttb & ~(MAKE_64BIT_MASK(0, cfg->oas))) {
> >              goto bad_cd;
> >          }
> > @@ -767,12 +803,12 @@ static int smmuv3_decode_config(IOMMUMemoryRegion *mr, SMMUTransCfg *cfg,
> >          return 0;
> >      }
> >  
> > -    ret = smmu_get_cd(s, &ste, 0 /* ssid */, &cd, event);
> > +    ret = smmu_get_cd(s, &ste, cfg, 0 /* ssid */, &cd, event);
> >      if (ret) {
> >          return ret;
> >      }
> >  
> > -    return decode_cd(cfg, &cd, event);
> > +    return decode_cd(s, cfg, &cd, event);
> >  }
> >  
> >  /**
> > @@ -942,8 +978,7 @@ epilogue:
> >      switch (status) {
> >      case SMMU_TRANS_SUCCESS:
> >          entry.perm = cached_entry->entry.perm;
> > -        entry.translated_addr = cached_entry->entry.translated_addr +
> > -                                    (addr & cached_entry->entry.addr_mask);
> > +        entry.translated_addr = CACHED_ENTRY_TO_ADDR(cached_entry, addr);
> >          entry.addr_mask = cached_entry->entry.addr_mask;
> >          trace_smmuv3_translate_success(mr->parent_obj.name, sid, addr,
> >                                         entry.translated_addr, entry.perm,
> > diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
> > index 96eb017e50..2772175115 100644
> > --- a/include/hw/arm/smmu-common.h
> > +++ b/include/hw/arm/smmu-common.h
> > @@ -37,6 +37,23 @@
> >  #define VMSA_IDXMSK(isz, strd, lvl)         ((1ULL << \
> >                                               VMSA_BIT_LVL(isz, strd, lvl)) - 1)
> >  
> > +#define CACHED_ENTRY_TO_ADDR(ent, addr)      (ent)->entry.translated_addr + \
> > +                                             ((addr) & (ent)->entry.addr_mask);
> > +
> > +/*
> > + * From nested context, some functions might need to translate IPA addresses.
> > + * As cfg has SMMU_NESTED, this won't work, this macro calls a function with
> > + * making it a stage-2 cfg and then restore it after.
> > + */
> > +#define CALL_FUNC_CFG_S2(cfg, ret, fn, ...)  ({ \
> > +                                                   int asid = cfg->asid; \
> > +                                                   cfg->stage = SMMU_STAGE_2; \
> > +                                                   cfg->asid = -1; \
> > +                                                   ret = fn(__VA_ARGS__); \
> At this stage of the reading this is not obvious why you need fn()
> parameter, can't you simply call
> 
> smmuv3_do_translate(). If this is useful at some point in the series, you shall document that in the commit msg. 
> Also I think I would prefer a proper function instead of this macro.
> 
> Besides, can't we add an extra parameter to the translate function forcing the S2_only translation although the cfg is a nested one. I think this would make things clearer
> 
This macro is reused with “smmu_translate” in the next patches, I had a
look with adding a a separate arg and I didn’t like it either, I can try
again or may be have a clear separate wrapper for this.


Thanks,
Mostafa
> > +                                                   cfg->asid = asid; \
> > +                                                   cfg->stage = SMMU_NESTED; \
> > +                                              })
> > +
> >  /*
> >   * Page table walk error types
> >   */
> Thanks
> 
> Eric
>