Hi Mostafa,
On 7/9/24 09:18, Mostafa Saleh wrote:
> Hi Eric,
>
> On Mon, Jul 08, 2024 at 05:19:59PM +0200, Eric Auger wrote:
>> Hi Mostafa,
>>
>> On 7/1/24 13:02, 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.
>>>
>>> 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 | 72 +++++++++++++++++++++++++++++++-----
>>> include/hw/arm/smmu-common.h | 2 +-
>>> 2 files changed, 64 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
>>> index 24b7d09e2b..71afd486ba 100644
>>> --- a/hw/arm/smmu-common.c
>>> +++ b/hw/arm/smmu-common.c
>>> @@ -318,6 +318,38 @@ 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(dma_addr_t *table_addr,
>>> + SMMUTransCfg *cfg,
>>> + SMMUPTWEventInfo *info,
>>> + SMMUState *bs)
>> Nit: in general the SMMUState if the 1st arg, as the most global state.
>>> +{
>>> + 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->type = SMMU_PTW_ERR_WALK_EABT;
>>> + info->addr = addr;
>> so I guess also here the recorded address should be the IOVA (Jean's
>> previous comment)?
> This address maps to FetchAddr and not InputAddr, which is set from the
> calling function, so that should be correct. (besides event type as Jean
> mentioned it needs be fixed).
Ah OK I mixed them up. Sorry for the noise then
Eric
>
> Thanks,
> Mostafa
>
>> Eric
>>> + return -EINVAL;
>>> +}
>>> +
>>> /**
>>> * smmu_ptw_64_s1 - VMSAv8-64 Walk of the page tables for a given IOVA
>>> * @cfg: translation config
>>> @@ -333,7 +365,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;
>>> @@ -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(&baseaddr, cfg, info, bs)) {
>>> + 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;
>>> @@ -596,14 +632,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
>>> @@ -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(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,
>>> @@ -677,7 +731,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);
>>>
>>>
>>> /*
