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

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

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, ...
> 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.

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
> +        /*
> +         * 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?
> +         */
> +        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

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

Eric