drivers/io-pgtable-arm: Fix stage concatenation with 16K

[PATCH] drivers/io-pgtable-arm: Fix stage concatenation with 16K

Posted by Mostafa Saleh 6 days, 17 hours ago

According to the Arm spec DDI0487 K.a, in:
"Table D8-9 Implications of the effective minimum T0SZ value on the
initial stage 2 lookup level"

Some combinations of granule and input size with stage-2 would
require to use initial lookup levels that can only be achieved
with concatenated PGDs.

There was one missing case in the current implementation for 16K,
which is 40-bits.

Cc: Daniel Mentz <danielmentz@google.com>

Signed-off-by: Mostafa Saleh <smostafa@google.com>
---
 drivers/iommu/io-pgtable-arm.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
index 0e67f1721a3d..9a57874a5cb8 100644
--- a/drivers/iommu/io-pgtable-arm.c
+++ b/drivers/iommu/io-pgtable-arm.c
@@ -1044,10 +1044,18 @@ arm_64_lpae_alloc_pgtable_s2(struct io_pgtable_cfg *cfg, void *cookie)
 		return NULL;
 
 	/*
-	 * Concatenate PGDs at level 1 if possible in order to reduce
-	 * the depth of the stage-2 walk.
+	 * Some cases where concatenation is mandatory after de-ciphering RSRKBC
+	 * in the Arm DDI0487 (K.a):
+	 * - 40 bits with 4K: use 2 table at level 1 instead of level 0
+	 * - 40 bits with 16K: use 16 tables at level 2 instead of level 1
+	 * - 42 bits with 4K: use 8 tabels at level 1 instead of level 0
+	 * - 48 bits with 16K: use 2 tabels at level 1 instead of level 0
+	 * Looking at the possible valid input size, that concludes to always
+	 * use level 1 with concatentation if possible or at level 2 only
+	 * with 16K.
 	 */
-	if (data->start_level == 0) {
+	if ((data->start_level == 0) ||
+	    ((data->start_level == 1) && (ARM_LPAE_GRANULE(data) == SZ_16K))) {
 		unsigned long pgd_pages;
 
 		pgd_pages = ARM_LPAE_PGD_SIZE(data) / sizeof(arm_lpae_iopte);
-- 
2.47.0.338.g60cca15819-goog

Re: [PATCH] drivers/io-pgtable-arm: Fix stage concatenation with 16K

Posted by Robin Murphy 6 days, 16 hours ago

On 2024-11-15 5:22 pm, Mostafa Saleh wrote:
> According to the Arm spec DDI0487 K.a, in:
> "Table D8-9 Implications of the effective minimum T0SZ value on the
> initial stage 2 lookup level"
> 
> Some combinations of granule and input size with stage-2 would
> require to use initial lookup levels that can only be achieved
> with concatenated PGDs.
> 
> There was one missing case in the current implementation for 16K,
> which is 40-bits.
> 
> Cc: Daniel Mentz <danielmentz@google.com>
> 
> Signed-off-by: Mostafa Saleh <smostafa@google.com>
> ---
>   drivers/iommu/io-pgtable-arm.c | 14 +++++++++++---
>   1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
> index 0e67f1721a3d..9a57874a5cb8 100644
> --- a/drivers/iommu/io-pgtable-arm.c
> +++ b/drivers/iommu/io-pgtable-arm.c
> @@ -1044,10 +1044,18 @@ arm_64_lpae_alloc_pgtable_s2(struct io_pgtable_cfg *cfg, void *cookie)
>   		return NULL;
>   
>   	/*
> -	 * Concatenate PGDs at level 1 if possible in order to reduce
> -	 * the depth of the stage-2 walk.
> +	 * Some cases where concatenation is mandatory after de-ciphering RSRKBC
> +	 * in the Arm DDI0487 (K.a):
> +	 * - 40 bits with 4K: use 2 table at level 1 instead of level 0
> +	 * - 40 bits with 16K: use 16 tables at level 2 instead of level 1
> +	 * - 42 bits with 4K: use 8 tabels at level 1 instead of level 0
> +	 * - 48 bits with 16K: use 2 tabels at level 1 instead of level 0

This confused me, since per R_DXBSH, that last one is the only one which 
is actually mandatory in general; the others may be valid per R_PZFHQ 
and R_FBHPY. The additional R_SRKBC constraints come from the PA size, 
not the choice of T0SZ (and thus ultimately start_level) itself, so 
although I guess this probably works out true in practice based on how 
the SMMU drivers happen to behave today, it's none too obvious why.

Thanks,
Robin.

> +	 * Looking at the possible valid input size, that concludes to always
> +	 * use level 1 with concatentation if possible or at level 2 only
> +	 * with 16K.
>   	 */
> -	if (data->start_level == 0) {
> +	if ((data->start_level == 0) ||
> +	    ((data->start_level == 1) && (ARM_LPAE_GRANULE(data) == SZ_16K))) {
>   		unsigned long pgd_pages;
>   
>   		pgd_pages = ARM_LPAE_PGD_SIZE(data) / sizeof(arm_lpae_iopte);

Re: [PATCH] drivers/io-pgtable-arm: Fix stage concatenation with 16K

Posted by Mostafa Saleh 6 days, 15 hours ago

Hi Robin,

On Fri, Nov 15, 2024 at 06:29:20PM +0000, Robin Murphy wrote:
> On 2024-11-15 5:22 pm, Mostafa Saleh wrote:
> > According to the Arm spec DDI0487 K.a, in:
> > "Table D8-9 Implications of the effective minimum T0SZ value on the
> > initial stage 2 lookup level"
> > 
> > Some combinations of granule and input size with stage-2 would
> > require to use initial lookup levels that can only be achieved
> > with concatenated PGDs.
> > 
> > There was one missing case in the current implementation for 16K,
> > which is 40-bits.
> > 
> > Cc: Daniel Mentz <danielmentz@google.com>
> > 
> > Signed-off-by: Mostafa Saleh <smostafa@google.com>
> > ---
> >   drivers/iommu/io-pgtable-arm.c | 14 +++++++++++---
> >   1 file changed, 11 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
> > index 0e67f1721a3d..9a57874a5cb8 100644
> > --- a/drivers/iommu/io-pgtable-arm.c
> > +++ b/drivers/iommu/io-pgtable-arm.c
> > @@ -1044,10 +1044,18 @@ arm_64_lpae_alloc_pgtable_s2(struct io_pgtable_cfg *cfg, void *cookie)
> >   		return NULL;
> >   	/*
> > -	 * Concatenate PGDs at level 1 if possible in order to reduce
> > -	 * the depth of the stage-2 walk.
> > +	 * Some cases where concatenation is mandatory after de-ciphering RSRKBC
> > +	 * in the Arm DDI0487 (K.a):
> > +	 * - 40 bits with 4K: use 2 table at level 1 instead of level 0
> > +	 * - 40 bits with 16K: use 16 tables at level 2 instead of level 1
> > +	 * - 42 bits with 4K: use 8 tabels at level 1 instead of level 0
> > +	 * - 48 bits with 16K: use 2 tabels at level 1 instead of level 0
> 
> This confused me, since per R_DXBSH, that last one is the only one which is
> actually mandatory in general; the others may be valid per R_PZFHQ and
> R_FBHPY. The additional R_SRKBC constraints come from the PA size, not the
> choice of T0SZ (and thus ultimately start_level) itself, so although I guess
> this probably works out true in practice based on how the SMMU drivers
> happen to behave today, it's none too obvious why.

Ah, you are right, I got a bit confused, it's not actually about the
input size, but the fact that the SMMUv3 driver uses IAS = OAS for
stage-2.
And constraint R_SRKBC is about the OAS size.

So in case we change that IAS = OAS for stage-2, we could end up using
concatenation when it's not mandatory.
I don't think that's a bad thing (I was actually thinking of changing
the code to always use concatenation if possible)
But, I can re-write the code in terms of OAS + granule instead so it's
more robust incase we change the stage-2 config in the future.

Thanks,
Mostafa
> 
> Thanks,
> Robin.
> 
> > +	 * Looking at the possible valid input size, that concludes to always
> > +	 * use level 1 with concatentation if possible or at level 2 only
> > +	 * with 16K.
> >   	 */
> > -	if (data->start_level == 0) {
> > +	if ((data->start_level == 0) ||
> > +	    ((data->start_level == 1) && (ARM_LPAE_GRANULE(data) == SZ_16K))) {
> >   		unsigned long pgd_pages;
> >   		pgd_pages = ARM_LPAE_PGD_SIZE(data) / sizeof(arm_lpae_iopte);
>