Re: [PATCH v4 10/19] hw/arm/smmu-common: Add support for nested TLB

[Jean-Philippe Brucker]

On Mon, Jul 01, 2024 at 11:02:32AM +0000, Mostafa Saleh wrote:
> This patch adds support for nested (combined) TLB entries.
> The main function combine_tlb() is not used here but in the next
> patches, but to simplify the patches it is introduced first.
> 
> Main changes:
> 1) New field added in the SMMUTLBEntry struct: parent_perm, for
>    nested TLB, holds the stage-2 permission, this can be used to know
>    the origin of a permission fault from a cached entry as caching
>    the “and” of the permissions loses this information.
> 
>    SMMUPTWEventInfo is used to hold information about PTW faults so
>    the event can be populated, the value of stage used to be set
>    based on the current stage for TLB permission faults, however
>    with the parent_perm, it is now set based on which perm has
>    the missing permission
> 
>    When nesting is not enabled it has the same value as perm which
>    doesn't change the logic.
> 
> 2) As combined TLB implementation is used, the combination logic
>    chooses:
>    - tg and level from the entry which has the smallest addr_mask.
>    - Based on that the iova that would be cached is recalculated.
>    - Translated_addr is chosen from stage-2.
> 
> Signed-off-by: Mostafa Saleh <smostafa@google.com>

Reviewed-by: Jean-Philippe Brucker <jean-philippe@linaro.org>

> ---
>  hw/arm/smmu-common.c         | 37 ++++++++++++++++++++++++++++++++----
>  include/hw/arm/smmu-common.h |  1 +
>  2 files changed, 34 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
> index 0840b5cffd..24b7d09e2b 100644
> --- a/hw/arm/smmu-common.c
> +++ b/hw/arm/smmu-common.c
> @@ -426,7 +426,8 @@ static int smmu_ptw_64_s1(SMMUTransCfg *cfg,
>          tlbe->entry.translated_addr = gpa;
>          tlbe->entry.iova = iova & ~mask;
>          tlbe->entry.addr_mask = mask;
> -        tlbe->entry.perm = PTE_AP_TO_PERM(ap);
> +        tlbe->parent_perm = PTE_AP_TO_PERM(ap);
> +        tlbe->entry.perm = tlbe->parent_perm;
>          tlbe->level = level;
>          tlbe->granule = granule_sz;
>          return 0;
> @@ -547,7 +548,8 @@ static int smmu_ptw_64_s2(SMMUTransCfg *cfg,
>          tlbe->entry.translated_addr = gpa;
>          tlbe->entry.iova = ipa & ~mask;
>          tlbe->entry.addr_mask = mask;
> -        tlbe->entry.perm = s2ap;
> +        tlbe->parent_perm = s2ap;
> +        tlbe->entry.perm = tlbe->parent_perm;
>          tlbe->level = level;
>          tlbe->granule = granule_sz;
>          return 0;
> @@ -562,6 +564,30 @@ error:
>      return -EINVAL;
>  }
>  
> +/*
> + * 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)
> +{
> +    if (tlbe_s2->entry.addr_mask < tlbe->entry.addr_mask) {
> +        tlbe->entry.addr_mask = tlbe_s2->entry.addr_mask;
> +        tlbe->granule = tlbe_s2->granule;
> +        tlbe->level = tlbe_s2->level;
> +    }
> +
> +    tlbe->entry.translated_addr = CACHED_ENTRY_TO_ADDR(tlbe_s2,
> +                                    tlbe->entry.translated_addr);
> +
> +    tlbe->entry.iova = iova & ~tlbe->entry.addr_mask;
> +    /* parent_perm has s2 perm while perm keeps s1 perm. */
> +    tlbe->parent_perm = tlbe_s2->entry.perm;
> +    return;
> +}
> +
>  /**
>   * smmu_ptw - Walk the page tables for an IOVA, according to @cfg
>   *
> @@ -639,9 +665,12 @@ SMMUTLBEntry *smmu_translate(SMMUState *bs, SMMUTransCfg 
> *cfg, dma_addr_t addr,
>  
>      cached_entry = smmu_iotlb_lookup(bs, cfg, &tt_combined, aligned_addr);
>      if (cached_entry) {
> -        if ((flag & IOMMU_WO) && !(cached_entry->entry.perm & IOMMU_WO)) {
> +        if ((flag & IOMMU_WO) && !(cached_entry->entry.perm &
> +            cached_entry->parent_perm & IOMMU_WO)) {
>              info->type = SMMU_PTW_ERR_PERMISSION;
> -            info->stage = cfg->stage;
> +            info->stage = !(cached_entry->entry.perm & IOMMU_WO) ?
> +                          SMMU_STAGE_1 :
> +                          SMMU_STAGE_2;
>              return NULL;
>          }
>          return cached_entry;
> diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
> index 09d3b9e734..1db566d451 100644
> --- a/include/hw/arm/smmu-common.h
> +++ b/include/hw/arm/smmu-common.h
> @@ -77,6 +77,7 @@ typedef struct SMMUTLBEntry {
>      IOMMUTLBEntry entry;
>      uint8_t level;
>      uint8_t granule;
> +    IOMMUAccessFlags parent_perm;
>  } SMMUTLBEntry;
>  
>  /* Stage-2 configuration. */
> -- 
> 2.45.2.803.g4e1b14247a-goog
>