Re: [RFC PATCH 07/12] hw/arm/smmu-common: Support nested translation

From: Julien Grall <julien@xen.org>
To: Mostafa Saleh <smostafa@google.com>,
	qemu-arm@nongnu.org, eric.auger@redhat.com,
	peter.maydell@linaro.org, qemu-devel@nongnu.org
Cc: jean-philippe@linaro.org, alex.bennee@linaro.org, maz@kernel.org,
	nicolinc@nvidia.com
Subject: Re: [RFC PATCH 07/12] hw/arm/smmu-common: Support nested translation
Date: Mon, 25 Mar 2024 14:20:07 +0000	[thread overview]
Message-ID: <bd24dc1a-d474-4cc7-87f9-2d5059a19602@xen.org> (raw)
In-Reply-To: <20240325101442.1306300-8-smostafa@google.com>

Hi Mostafa,

On 25/03/2024 10:14, Mostafa Saleh wrote:
> @@ -524,7 +551,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;
> @@ -537,6 +564,35 @@ error:
>       return -EINVAL;
>   }
>   
> +/* Combine 2 TLB enteries and return in tlbe. */
> +static void combine_tlb(SMMUTLBEntry *tlbe, SMMUTLBEntry *tlbe_s2,
> +                        dma_addr_t iova, SMMUTransCfg *cfg)
> +{
> +        if (cfg->stage == SMMU_NESTED) {
> +
> +            /*
> +             * tg and level are used from stage-1, while the addr mask can be
With the current approach, I can't boot a guest if I create a dummy 
stage-1 using 512GB mapping and a stage-2 using 2MB mapping. It looks 
like this is because the level will be used during the TLB lookup.

I managed to solve the issue by using the max level of the two stages. I 
think we may need to do a minimum for the granule.

> +             * smaller in case stage-2 size(based on granule and level) was
> +             * smaller than stage-1.
> +             * That should have no impact on:
> +             * - lookup: as iova is properly aligned with the stage-1 level and
> +             *   granule.
> +             * - Invalidation: as it uses the entry mask.
> +             */
> +            tlbe->entry.addr_mask = MIN(tlbe->entry.addr_mask,
> +                                        tlbe_s2->entry.addr_mask);
> +            tlbe->entry.translated_addr = CACHED_ENTRY_TO_ADDR(tlbe_s2,
> +                                          tlbe->entry.translated_addr);
> +
> +            /* parent_perm has s2 perm while perm has s1 perm. */
> +            tlbe->parent_perm = tlbe_s2->entry.perm;
> +            return;
> +        }
> +
> +        /* That was not nested, use the s2. */
> +        memcpy(tlbe, tlbe_s2, sizeof(*tlbe));
> +}

Cheers,

-- 
Julien Grall