Hi Mostafa,
On 4/29/24 05:23, Mostafa Saleh wrote:
> This patch adds support for nested(combined) TLB entries.
space between nested and (.
> 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 entry added in the TLB, parent_perm, for nested TLB, holds the
s/entry/field, s/TLB/SMMUTLBEntry struct
> 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 (which maps to S2
> in the event) used to be set based on the current stage for TLB
I don't understand "(which maps to S2 in the event)". What do you mean?
This could be S1 or S2 depending on the active stage, no?
> 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.
tbh I am scared bout swapping s1/s2 tg and level. In smmu_iotlb_lookup()
I see tt->granule_sz being used which is s1 data. I mean it is not
obvious to me this is correct. Could you maybe give more explanations
detailing why/how this is guaranted to work.
Can you give additional details about what s1+s2 combinations were tested?
> - Based on that the iova that would be cached is recalculated.
> - Translated_addr is chosen from stage-2.
>
> Signed-off-by: Mostafa Saleh <smost...@google.com>
> ---
> hw/arm/smmu-common.c | 32 ++++++++++++++++++++++++++++----
> include/hw/arm/smmu-common.h | 1 +
> 2 files changed, 29 insertions(+), 4 deletions(-)
>
> diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
> index 21982621c0..0d6945fa54 100644
> --- a/hw/arm/smmu-common.c
> +++ b/hw/arm/smmu-common.c
> @@ -394,7 +394,7 @@ 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 = tlbe->entry.perm = PTE_AP_TO_PERM(ap);
nit: I would prefer on separate lines.
> tlbe->level = level;
> tlbe->granule = granule_sz;
> return 0;
> @@ -515,7 +515,7 @@ 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 = tlbe->entry.perm = s2ap;
> tlbe->level = level;
> tlbe->granule = granule_sz;
> return 0;
> @@ -530,6 +530,27 @@ error:
> return -EINVAL;
> }
>
> +/* combine 2 TLB entries and return in tlbe in nested config. */
suggestion: 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 has s1 perm. */
suggestion: 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
> *
> @@ -607,9 +628,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. */
Thanks
Eric
