Re: [PATCH v5 11/18] hw/arm/smmu-common: Support nested translation

[Eric Auger]

On 7/15/24 10:45, Mostafa Saleh wrote:
> When nested translation is requested, do the following:
> - Translate stage-1 table address IPA into PA through stage-2.
> - Translate stage-1 table walk output (IPA) through stage-2.
> - Create a single TLB entry from stage-1 and stage-2 translations
>   using logic introduced before.
>
> smmu_ptw() has a new argument SMMUState which include the TLB as
> stage-1 table address can be cached in there.
>
> Also in smmu_ptw(), a separate path used for nesting to simplify the
> code, although some logic can be combined.
>
> With nested translation class of translation fault can be different,
> from the class of the translation, as faults from translating stage-1
> tables are considered as CLASS_TT and not CLASS_IN, a new member
> "is_ipa_descriptor" added to "SMMUPTWEventInfo" to differ faults
> from walking stage 1 translation table and faults from translating
> an IPA for a transaction.
>
> Signed-off-by: Mostafa Saleh <smostafa@google.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Eric
> ---
>  hw/arm/smmu-common.c         | 74 +++++++++++++++++++++++++++++++-----
>  hw/arm/smmuv3.c              | 14 +++++++
>  include/hw/arm/smmu-common.h |  7 ++--
>  3 files changed, 82 insertions(+), 13 deletions(-)
>
> diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
> index c894c4c621..8ed53f5b1d 100644
> --- a/hw/arm/smmu-common.c
> +++ b/hw/arm/smmu-common.c
> @@ -318,8 +318,41 @@ SMMUTransTableInfo *select_tt(SMMUTransCfg *cfg, 
> dma_addr_t iova)
>      return NULL;
>  }
>  
> +/* Translate stage-1 table address using stage-2 page table. */
> +static inline int translate_table_addr_ipa(SMMUState *bs,
> +                                           dma_addr_t *table_addr,
> +                                           SMMUTransCfg *cfg,
> +                                           SMMUPTWEventInfo *info)
> +{
> +    dma_addr_t addr = *table_addr;
> +    SMMUTLBEntry *cached_entry;
> +    int asid;
> +
> +    /*
> +     * The translation table walks performed from TTB0 or TTB1 are always
> +     * performed in IPA space if stage 2 translations are enabled.
> +     */
> +    asid = cfg->asid;
> +    cfg->stage = SMMU_STAGE_2;
> +    cfg->asid = -1;
> +    cached_entry = smmu_translate(bs, cfg, addr, IOMMU_RO, info);
> +    cfg->asid = asid;
> +    cfg->stage = SMMU_NESTED;
> +
> +    if (cached_entry) {
> +        *table_addr = CACHED_ENTRY_TO_ADDR(cached_entry, addr);
> +        return 0;
> +    }
> +
> +    info->stage = SMMU_STAGE_2;
> +    info->addr = addr;
> +    info->is_ipa_descriptor = true;
> +    return -EINVAL;
> +}
> +
>  /**
>   * smmu_ptw_64_s1 - VMSAv8-64 Walk of the page tables for a given IOVA
> + * @bs: smmu state which includes TLB instance
>   * @cfg: translation config
>   * @iova: iova to translate
>   * @perm: access type
> @@ -331,7 +364,7 @@ SMMUTransTableInfo *select_tt(SMMUTransCfg *cfg, 
> dma_addr_t iova)
>   * Upon success, @tlbe is filled with translated_addr and entry
>   * permission rights.
>   */
> -static int smmu_ptw_64_s1(SMMUTransCfg *cfg,
> +static int smmu_ptw_64_s1(SMMUState *bs, SMMUTransCfg *cfg,
>                            dma_addr_t iova, IOMMUAccessFlags perm,
>                            SMMUTLBEntry *tlbe, SMMUPTWEventInfo *info)
>  {
> @@ -381,6 +414,11 @@ static int smmu_ptw_64_s1(SMMUTransCfg *cfg,
>                  goto error;
>              }
>              baseaddr = get_table_pte_address(pte, granule_sz);
> +            if (cfg->stage == SMMU_NESTED) {
> +                if (translate_table_addr_ipa(bs, &baseaddr, cfg, info)) {
> +                    goto error;
> +                }
> +            }
>              level++;
>              continue;
>          } else if (is_page_pte(pte, level)) {
> @@ -568,10 +606,8 @@ error:
>   * combine S1 and S2 TLB entries into a single entry.
>   * As a result the S1 entry is overriden with combined data.
>   */
> -static void __attribute__((unused)) combine_tlb(SMMUTLBEntry *tlbe,
> -                                                SMMUTLBEntry *tlbe_s2,
> -                                                dma_addr_t iova,
> -                                                SMMUTransCfg *cfg)
> +static void combine_tlb(SMMUTLBEntry *tlbe, SMMUTLBEntry *tlbe_s2,
> +                        dma_addr_t iova, SMMUTransCfg *cfg)
>  {
>      if (tlbe_s2->entry.addr_mask < tlbe->entry.addr_mask) {
>          tlbe->entry.addr_mask = tlbe_s2->entry.addr_mask;
> @@ -591,6 +627,7 @@ static void __attribute__((unused)) 
> combine_tlb(SMMUTLBEntry *tlbe,
>  /**
>   * smmu_ptw - Walk the page tables for an IOVA, according to @cfg
>   *
> + * @bs: smmu state which includes TLB instance
>   * @cfg: translation configuration
>   * @iova: iova to translate
>   * @perm: tentative access type
> @@ -599,11 +636,15 @@ static void __attribute__((unused)) 
> combine_tlb(SMMUTLBEntry *tlbe,
>   *
>   * return 0 on success
>   */
> -int smmu_ptw(SMMUTransCfg *cfg, dma_addr_t iova, IOMMUAccessFlags perm,
> -             SMMUTLBEntry *tlbe, SMMUPTWEventInfo *info)
> +int smmu_ptw(SMMUState *bs, SMMUTransCfg *cfg, dma_addr_t iova,
> +             IOMMUAccessFlags perm, SMMUTLBEntry *tlbe, SMMUPTWEventInfo 
> *info)
>  {
> +    int ret;
> +    SMMUTLBEntry tlbe_s2;
> +    dma_addr_t ipa;
> +
>      if (cfg->stage == SMMU_STAGE_1) {
> -        return smmu_ptw_64_s1(cfg, iova, perm, tlbe, info);
> +        return smmu_ptw_64_s1(bs, cfg, iova, perm, tlbe, info);
>      } else if (cfg->stage == SMMU_STAGE_2) {
>          /*
>           * If bypassing stage 1(or unimplemented), the input address is 
> passed
> @@ -621,7 +662,20 @@ int smmu_ptw(SMMUTransCfg *cfg, dma_addr_t iova, 
> IOMMUAccessFlags perm,
>          return smmu_ptw_64_s2(cfg, iova, perm, tlbe, info);
>      }
>  
> -    g_assert_not_reached();
> +    /* SMMU_NESTED. */
> +    ret = smmu_ptw_64_s1(bs, cfg, iova, perm, tlbe, info);
> +    if (ret) {
> +        return ret;
> +    }
> +
> +    ipa = CACHED_ENTRY_TO_ADDR(tlbe, iova);
> +    ret = smmu_ptw_64_s2(cfg, ipa, perm, &tlbe_s2, info);
> +    if (ret) {
> +        return ret;
> +    }
> +
> +    combine_tlb(tlbe, &tlbe_s2, iova, cfg);
> +    return 0;
>  }
>  
>  SMMUTLBEntry *smmu_translate(SMMUState *bs, SMMUTransCfg *cfg, dma_addr_t 
> addr,
> @@ -667,7 +721,7 @@ SMMUTLBEntry *smmu_translate(SMMUState *bs, SMMUTransCfg 
> *cfg, dma_addr_t addr,
>      }
>  
>      cached_entry = g_new0(SMMUTLBEntry, 1);
> -    status = smmu_ptw(cfg, addr, flag, cached_entry, info);
> +    status = smmu_ptw(bs, cfg, addr, flag, cached_entry, info);
>      if (status) {
>              g_free(cached_entry);
>              return NULL;
> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> index 73d5a25705..06a96c65eb 100644
> --- a/hw/arm/smmuv3.c
> +++ b/hw/arm/smmuv3.c
> @@ -910,6 +910,20 @@ static SMMUTranslationStatus 
> smmuv3_do_translate(SMMUv3State *s, hwaddr addr,
>      if (!cached_entry) {
>          /* All faults from PTW has S2 field. */
>          event->u.f_walk_eabt.s2 = (ptw_info.stage == SMMU_STAGE_2);
> +        /*
> +         * Fault class is set as follows based on "class" input to
> +         * the function and to "ptw_info" from "smmu_translate()"
> +         * For stage-1:
> +         *   - EABT => CLASS_TT (hardcoded)
> +         *   - other events => CLASS_IN (input to function)
> +         * For stage-2 => CLASS_IN (input to function)
> +         * For nested, for all events:
> +         *  - CD fetch => CLASS_CD (input to function)
> +         *  - walking stage 1 translation table  => CLASS_TT (from
> +         *    is_ipa_descriptor or input in case of TTBx)
> +         *  - s2 translation => CLASS_IN (input to function)
> +         */
> +        class = ptw_info.is_ipa_descriptor ? SMMU_CLASS_TT : class;
>          switch (ptw_info.type) {
>          case SMMU_PTW_ERR_WALK_EABT:
>              event->type = SMMU_EVT_F_WALK_EABT;
> diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
> index d84de64122..a3e6ab1b36 100644
> --- a/include/hw/arm/smmu-common.h
> +++ b/include/hw/arm/smmu-common.h
> @@ -63,6 +63,7 @@ typedef struct SMMUPTWEventInfo {
>      SMMUStage stage;
>      SMMUPTWEventType type;
>      dma_addr_t addr; /* fetched address that induced an abort, if any */
> +    bool is_ipa_descriptor; /* src for fault in nested translation. */
>  } SMMUPTWEventInfo;
>  
>  typedef struct SMMUTransTableInfo {
> @@ -184,9 +185,9 @@ static inline uint16_t smmu_get_sid(SMMUDevice *sdev)
>   * smmu_ptw - Perform the page table walk for a given iova / access flags
>   * pair, according to @cfg translation config
>   */
> -int smmu_ptw(SMMUTransCfg *cfg, dma_addr_t iova, IOMMUAccessFlags perm,
> -             SMMUTLBEntry *tlbe, SMMUPTWEventInfo *info);
> -
> +int smmu_ptw(SMMUState *bs, SMMUTransCfg *cfg, dma_addr_t iova,
> +             IOMMUAccessFlags perm, SMMUTLBEntry *tlbe,
> +             SMMUPTWEventInfo *info);
>  
>  /*
>   * smmu_translate - Look for a translation in TLB, if not, do a PTW.