[PATCH v2 0/2] Fix missing case for concatenation

[PATCH v2 0/2] Fix missing case for concatenation

[Mostafa Saleh]

First patch fixes a missing case for 16K granule and 40 bits SMMUs
where concatenation is mandatory but ignored.

Second patch, imporves coverage of OAS in selftests to make it
possible to test all concatenation cases.

Cc: Daniel Mentz <danielmentz@google.com>

Mostafa Saleh (2):
  iommu/io-pgtable-arm: Fix stage-2 concatenation with 16K
  iommu/io-pgtable-arm: Add coverage for different OAS in selftest

 drivers/iommu/io-pgtable-arm.c | 72 ++++++++++++++++++++++------------
 1 file changed, 48 insertions(+), 24 deletions(-)

-- 
V2:
- Rely on OAS and don't assume OAS = IAS
- Re-write the comment and commit message to be more clear
- Improve selftests
V1: https://lore.kernel.org/linux-iommu/20241115172235.1493328-1-smostafa@google.com/

2.47.0.338.g60cca15819-goog

[PATCH v2 1/2] iommu/io-pgtable-arm: Fix stage-2 concatenation with 16K

[Mostafa Saleh]

At the moment, io-pgtable-arm uses concatenation only if it is
possible at level 0, which misses a case where concatenation is
mandatory at level 1 according to R_SRKBC in Arm spec DDI0487 K.a.

Also, that means concatenation can be used when not mandated,
contradicting the comment on the code. However, these cases can only
happen if the SMMUv3 driver is changed to use ias != oas for stage-2.

This patch re-writes the code to use concatenation only if mandatory,
fixing the missing case for level-1 and granule 16K with PA = 40 bits.

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

diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
index 6b9bb58a414f..0055876b3527 100644
--- a/drivers/iommu/io-pgtable-arm.c
+++ b/drivers/iommu/io-pgtable-arm.c
@@ -223,6 +223,33 @@ static inline int arm_lpae_max_entries(int i, struct arm_lpae_io_pgtable *data)
 	return ptes_per_table - (i & (ptes_per_table - 1));
 }
 
+/*
+ * Check if concatenated PGDs are mandatory according to Arm DDI0487 (K.a)
+ * 1) R_DXBSH: For 16KB, and 48-bit input size, use level 1 instead of 0.
+ * 2) R_SRKBC: After de-ciphering the table for PA size and valid initial lookup
+ *   a) 40 bits PA size with 4K: use level 1 instead of level 0 (2 tables for ias = oas)
+ *   b) 40 bits PA size with 16K: use level 2 instead of level 1 (16 tables for ias = oas)
+ *   c) 42 bits PA size with 4K: use level 1 instead of level 0 (8 tables for ias = oas)
+ *   d) 48 bits PA size with 16K: use level 1 instead of level 0 (2 tables for ias = oas)
+ */
+static inline bool arm_lpae_concat_mandatory(struct arm_lpae_io_pgtable *data)
+{
+	unsigned int ias = data->iop.cfg.ias;
+	unsigned int oas = data->iop.cfg.oas;
+
+	/* Covers 1  and 2.d */
+	if ((ARM_LPAE_GRANULE(data) == SZ_16K) && (data->start_level == 0))
+		return (oas == 48) || (ias = 48);
+
+	/* Covers 2.a and 2.c */
+	if ((ARM_LPAE_GRANULE(data) == SZ_4K) && (data->start_level == 0))
+		return (oas == 40) || (oas == 42);
+
+	/* Case 2.b */
+	return (ARM_LPAE_GRANULE(data) == SZ_16K) &&
+	       (data->start_level == 1) && (oas == 40);
+}
+
 static bool selftest_running = false;
 
 static dma_addr_t __arm_lpae_dma_addr(void *pages)
@@ -1006,18 +1033,12 @@ arm_64_lpae_alloc_pgtable_s2(struct io_pgtable_cfg *cfg, void *cookie)
 	if (!data)
 		return NULL;
 
-	/*
-	 * Concatenate PGDs at level 1 if possible in order to reduce
-	 * the depth of the stage-2 walk.
-	 */
-	if (data->start_level == 0) {
-		unsigned long pgd_pages;
-
-		pgd_pages = ARM_LPAE_PGD_SIZE(data) / sizeof(arm_lpae_iopte);
-		if (pgd_pages <= ARM_LPAE_S2_MAX_CONCAT_PAGES) {
-			data->pgd_bits += data->bits_per_level;
-			data->start_level++;
-		}
+	if (arm_lpae_concat_mandatory(data))  {
+		if (WARN_ON((ARM_LPAE_PGD_SIZE(data) / sizeof(arm_lpae_iopte)) >
+			    ARM_LPAE_S2_MAX_CONCAT_PAGES))
+			return NULL;
+		data->pgd_bits += data->bits_per_level;
+		data->start_level++;
 	}
 
 	/* VTCR */
-- 
2.47.0.338.g60cca15819-goog

Re: [PATCH v2 1/2] iommu/io-pgtable-arm: Fix stage-2 concatenation with 16K

[Will Deacon]

On Mon, Dec 02, 2024 at 02:06:03PM +0000, Mostafa Saleh wrote:
> At the moment, io-pgtable-arm uses concatenation only if it is
> possible at level 0, which misses a case where concatenation is
> mandatory at level 1 according to R_SRKBC in Arm spec DDI0487 K.a.
> 
> Also, that means concatenation can be used when not mandated,
> contradicting the comment on the code. However, these cases can only
> happen if the SMMUv3 driver is changed to use ias != oas for stage-2.
> 
> This patch re-writes the code to use concatenation only if mandatory,
> fixing the missing case for level-1 and granule 16K with PA = 40 bits.
> 
> Signed-off-by: Mostafa Saleh <smostafa@google.com>
> ---
>  drivers/iommu/io-pgtable-arm.c | 45 +++++++++++++++++++++++++---------
>  1 file changed, 33 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
> index 6b9bb58a414f..0055876b3527 100644
> --- a/drivers/iommu/io-pgtable-arm.c
> +++ b/drivers/iommu/io-pgtable-arm.c
> @@ -223,6 +223,33 @@ static inline int arm_lpae_max_entries(int i, struct arm_lpae_io_pgtable *data)
>  	return ptes_per_table - (i & (ptes_per_table - 1));
>  }
>  
> +/*
> + * Check if concatenated PGDs are mandatory according to Arm DDI0487 (K.a)
> + * 1) R_DXBSH: For 16KB, and 48-bit input size, use level 1 instead of 0.
> + * 2) R_SRKBC: After de-ciphering the table for PA size and valid initial lookup
> + *   a) 40 bits PA size with 4K: use level 1 instead of level 0 (2 tables for ias = oas)
> + *   b) 40 bits PA size with 16K: use level 2 instead of level 1 (16 tables for ias = oas)
> + *   c) 42 bits PA size with 4K: use level 1 instead of level 0 (8 tables for ias = oas)
> + *   d) 48 bits PA size with 16K: use level 1 instead of level 0 (2 tables for ias = oas)
> + */
> +static inline bool arm_lpae_concat_mandatory(struct arm_lpae_io_pgtable *data)
> +{
> +	unsigned int ias = data->iop.cfg.ias;
> +	unsigned int oas = data->iop.cfg.oas;
> +
> +	/* Covers 1  and 2.d */
> +	if ((ARM_LPAE_GRANULE(data) == SZ_16K) && (data->start_level == 0))
> +		return (oas == 48) || (ias = 48);

I'm guessing the second disjunct here should be:

	(ias == 48);

I'll make that change when applying...

Will

Re: [PATCH v2 1/2] iommu/io-pgtable-arm: Fix stage-2 concatenation with 16K

[Mostafa Saleh]

On Mon, Dec 9, 2024 at 11:58 PM Will Deacon <will@kernel.org> wrote:
>
> On Mon, Dec 02, 2024 at 02:06:03PM +0000, Mostafa Saleh wrote:
> > At the moment, io-pgtable-arm uses concatenation only if it is
> > possible at level 0, which misses a case where concatenation is
> > mandatory at level 1 according to R_SRKBC in Arm spec DDI0487 K.a.
> >
> > Also, that means concatenation can be used when not mandated,
> > contradicting the comment on the code. However, these cases can only
> > happen if the SMMUv3 driver is changed to use ias != oas for stage-2.
> >
> > This patch re-writes the code to use concatenation only if mandatory,
> > fixing the missing case for level-1 and granule 16K with PA = 40 bits.
> >
> > Signed-off-by: Mostafa Saleh <smostafa@google.com>
> > ---
> >  drivers/iommu/io-pgtable-arm.c | 45 +++++++++++++++++++++++++---------
> >  1 file changed, 33 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
> > index 6b9bb58a414f..0055876b3527 100644
> > --- a/drivers/iommu/io-pgtable-arm.c
> > +++ b/drivers/iommu/io-pgtable-arm.c
> > @@ -223,6 +223,33 @@ static inline int arm_lpae_max_entries(int i, struct arm_lpae_io_pgtable *data)
> >       return ptes_per_table - (i & (ptes_per_table - 1));
> >  }
> >
> > +/*
> > + * Check if concatenated PGDs are mandatory according to Arm DDI0487 (K.a)
> > + * 1) R_DXBSH: For 16KB, and 48-bit input size, use level 1 instead of 0.
> > + * 2) R_SRKBC: After de-ciphering the table for PA size and valid initial lookup
> > + *   a) 40 bits PA size with 4K: use level 1 instead of level 0 (2 tables for ias = oas)
> > + *   b) 40 bits PA size with 16K: use level 2 instead of level 1 (16 tables for ias = oas)
> > + *   c) 42 bits PA size with 4K: use level 1 instead of level 0 (8 tables for ias = oas)
> > + *   d) 48 bits PA size with 16K: use level 1 instead of level 0 (2 tables for ias = oas)
> > + */
> > +static inline bool arm_lpae_concat_mandatory(struct arm_lpae_io_pgtable *data)
> > +{
> > +     unsigned int ias = data->iop.cfg.ias;
> > +     unsigned int oas = data->iop.cfg.oas;
> > +
> > +     /* Covers 1  and 2.d */
> > +     if ((ARM_LPAE_GRANULE(data) == SZ_16K) && (data->start_level == 0))
> > +             return (oas == 48) || (ias = 48);
>
> I'm guessing the second disjunct here should be:
>
>         (ias == 48);
>

Ouch, my bad.

> I'll make that change when applying...
>
> Will

[PATCH v2 2/2] iommu/io-pgtable-arm: Add coverage for different OAS in selftest

[Mostafa Saleh]

Run selftests with different OAS values intead of hardcoding it to 48
bits.

We always keep OAS >= IAS to make the config valid for stage-2.

This can be further improved, if we split IAS/OAS configuration for
stage-1 and stage-2 (to use input sizes compatible with VA_BITS as
SMMUv3 does, or IAS > OAS which is valid for stage-1).

However, that adds more complexity, and the current change improves
coverage and makes it possible to test all concatenation cases.

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

diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
index 0055876b3527..62015fdd6c6a 100644
--- a/drivers/iommu/io-pgtable-arm.c
+++ b/drivers/iommu/io-pgtable-arm.c
@@ -1385,15 +1385,14 @@ static int __init arm_lpae_do_selftests(void)
 		SZ_64K | SZ_512M,
 	};
 
-	static const unsigned int ias[] __initconst = {
+	static const unsigned int address_size[] __initconst = {
 		32, 36, 40, 42, 44, 48,
 	};
 
-	int i, j, pass = 0, fail = 0;
+	int i, j, k, pass = 0, fail = 0;
 	struct device dev;
 	struct io_pgtable_cfg cfg = {
 		.tlb = &dummy_tlb_ops,
-		.oas = 48,
 		.coherent_walk = true,
 		.iommu_dev = &dev,
 	};
@@ -1402,15 +1401,19 @@ static int __init arm_lpae_do_selftests(void)
 	set_dev_node(&dev, NUMA_NO_NODE);
 
 	for (i = 0; i < ARRAY_SIZE(pgsize); ++i) {
-		for (j = 0; j < ARRAY_SIZE(ias); ++j) {
-			cfg.pgsize_bitmap = pgsize[i];
-			cfg.ias = ias[j];
-			pr_info("selftest: pgsize_bitmap 0x%08lx, IAS %u\n",
-				pgsize[i], ias[j]);
-			if (arm_lpae_run_tests(&cfg))
-				fail++;
-			else
-				pass++;
+		for (j = 0; j < ARRAY_SIZE(address_size); ++j) {
+			/* Don't use ias > oas as it is not valid for stage-2. */
+			for (k = 0; k <= j; ++k) {
+				cfg.pgsize_bitmap = pgsize[i];
+				cfg.ias = address_size[k];
+				cfg.oas = address_size[j];
+				pr_info("selftest: pgsize_bitmap 0x%08lx, IAS %u OAS %u\n",
+					pgsize[i], cfg.ias, cfg.oas);
+				if (arm_lpae_run_tests(&cfg))
+					fail++;
+				else
+					pass++;
+			}
 		}
 	}
 
-- 
2.47.0.338.g60cca15819-goog

Re: [PATCH v2 0/2] Fix missing case for concatenation

[Will Deacon]

On Mon, 02 Dec 2024 14:06:02 +0000, Mostafa Saleh wrote:
> First patch fixes a missing case for 16K granule and 40 bits SMMUs
> where concatenation is mandatory but ignored.
> 
> Second patch, imporves coverage of OAS in selftests to make it
> possible to test all concatenation cases.
> 
> Cc: Daniel Mentz <danielmentz@google.com>
> 
> [...]

Applied to will (for-joerg/arm-smmu/updates), thanks!

[1/2] iommu/io-pgtable-arm: Fix stage-2 concatenation with 16K
      https://git.kernel.org/will/c/4dcac8407fe1
[2/2] iommu/io-pgtable-arm: Add coverage for different OAS in selftest
      https://git.kernel.org/will/c/376ce8b35ed1

Cheers,
-- 
Will

https://fixes.arm64.dev
https://next.arm64.dev
https://will.arm64.dev