Hi Eric, On Mon, May 20, 2024 at 11:48:18AM +0200, Eric Auger wrote: > Hi Mostafa, > > On 4/29/24 05:23, Mostafa Saleh wrote: > > When nested translation is requested, do the following: > > > > - Translate stage-1 IPA using stage-2 to a physical address. > stage-1 table address IPA into PA through S2 stage > Will do.
> > - Translate stage-1 table walks using stage-2. > output of S1 stage (IPA) through S2. Will do. > > - Combine both to create a single TLB entry using the logic > > introduced before. > this applies to second only. First one is associated with an S2 TLB > entry, right? Yes, I will clarify that. > > > > For stage-1 table translation, the spec (ARM IHI 0070 F.b) says in: > > 7.3.12 F_WALK_EABT: > > Translation of an IPA for Stage 1 descriptor fetch: > > S2 == 1 (stage 2), CLASS == T > > So, F_WALK_EABT is used which propagtes to CLASS == TT. > > > > 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. > > > > Signed-off-by: Mostafa Saleh <smost...@google.com> > > --- > > hw/arm/smmu-common.c | 67 ++++++++++++++++++++++++++++++------ > > include/hw/arm/smmu-common.h | 2 +- > > 2 files changed, 58 insertions(+), 11 deletions(-) > > > > diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c > > index c67af3bc6d..d48ec08947 100644 > > --- a/hw/arm/smmu-common.c > > +++ b/hw/arm/smmu-common.c > > @@ -306,6 +306,32 @@ 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_s1(dma_addr_t *table_addr, SMMUTransCfg > > *cfg, > > + SMMUPTWEventInfo *info, SMMUState *bs) > would suggest translate_table_addr_ipa(). Wil do. > > +{ > > + dma_addr_t addr = *table_addr; > > + SMMUTLBEntry *cached_entry; > > + int asid; > > + > > + asid = cfg->asid; > > + cfg->stage = SMMU_STAGE_2; > > + cfg->asid = -1; > > + cached_entry = smmu_translate(bs, cfg, addr, IOMMU_RO, info); > so this is going to be cached as an S2 entry. Maybe worth adding a comment. This is part of the IPA space and it is translated using a stage-2 page table, so it would be cached as an S2, I will add a comment quoting the spec. Thanks, Mostafa > > + 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->type = SMMU_PTW_ERR_WALK_EABT; > > + info->addr = addr; > > + return -EINVAL; > > +} > > + > > /** > > * smmu_ptw_64_s1 - VMSAv8-64 Walk of the page tables for a given IOVA > > * @cfg: translation config > > @@ -321,7 +347,8 @@ SMMUTransTableInfo *select_tt(SMMUTransCfg *cfg, > > dma_addr_t iova) > > */ > > static int smmu_ptw_64_s1(SMMUTransCfg *cfg, > > dma_addr_t iova, IOMMUAccessFlags perm, > > - SMMUTLBEntry *tlbe, SMMUPTWEventInfo *info) > > + SMMUTLBEntry *tlbe, SMMUPTWEventInfo *info, > > + SMMUState *bs) > > { > > dma_addr_t baseaddr, indexmask; > > SMMUStage stage = cfg->stage; > > @@ -369,6 +396,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_s1(&baseaddr, cfg, info, bs)) { > > + goto error; > > + } > > + } > > level++; > > continue; > > } else if (is_page_pte(pte, level)) { > > @@ -551,10 +583,8 @@ error: > > } > > > > /* combine 2 TLB entries and return in tlbe in nested config. */ > > -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; > > @@ -579,14 +609,19 @@ static void __attribute__((unused)) > > combine_tlb(SMMUTLBEntry *tlbe, > > * @perm: tentative access type > > * @tlbe: returned entry > > * @info: ptw event handle > > + * @bs: smmu state which includes TLB instance > > * > > * return 0 on success > > */ > > int smmu_ptw(SMMUTransCfg *cfg, dma_addr_t iova, IOMMUAccessFlags perm, > > - SMMUTLBEntry *tlbe, SMMUPTWEventInfo *info) > > + SMMUTLBEntry *tlbe, SMMUPTWEventInfo *info, SMMUState *bs) > > { > > + 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(cfg, iova, perm, tlbe, info, bs); > > } else if (cfg->stage == SMMU_STAGE_2) { > > /* > > * If bypassing stage 1(or unimplemented), the input address is > > passed > > @@ -600,11 +635,23 @@ int smmu_ptw(SMMUTransCfg *cfg, dma_addr_t iova, > > IOMMUAccessFlags perm, > > tlbe->entry.perm = IOMMU_NONE; > > return -EINVAL; > > } > > - > spurious line change > > return smmu_ptw_64_s2(cfg, iova, perm, tlbe, info); > > } > > > > - g_assert_not_reached(); > > + /* SMMU_NESTED. */ > > + ret = smmu_ptw_64_s1(cfg, iova, perm, tlbe, info, bs); > > + 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, > > @@ -660,7 +707,7 @@ SMMUTLBEntry *smmu_translate(SMMUState *bs, > > SMMUTransCfg *cfg, dma_addr_t addr, > > } > > > > cached_entry = g_new0(SMMUTLBEntry, 1); > > - status = smmu_ptw(cfg, aligned_addr, flag, cached_entry, info); > > + status = smmu_ptw(cfg, aligned_addr, flag, cached_entry, info, bs); > > if (status) { > > g_free(cached_entry); > > return NULL; > > diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h > > index 1db566d451..cf0fd3ec74 100644 > > --- a/include/hw/arm/smmu-common.h > > +++ b/include/hw/arm/smmu-common.h > > @@ -185,7 +185,7 @@ static inline uint16_t smmu_get_sid(SMMUDevice *sdev) > > * pair, according to @cfg translation config > > */ > > int smmu_ptw(SMMUTransCfg *cfg, dma_addr_t iova, IOMMUAccessFlags perm, > > - SMMUTLBEntry *tlbe, SMMUPTWEventInfo *info); > > + SMMUTLBEntry *tlbe, SMMUPTWEventInfo *info, SMMUState *bs); > > > > > > /* > Thanks > > Eric >