mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* [PATCH v2] iommu/arm-smmu-v3: Align memory attributes for SMMU-originated accesses
@ 2026-10-04 19:50 Daniel Mentz
  2026-10-05 16:42 ` Robin Murphy
  0 siblings, 1 reply; 3+ messages in thread
From: Daniel Mentz @ 2026-10-04 19:50 UTC (permalink / raw)
  To: iommu
  Cc: will, robin.murphy, joro, nicolinc, smostafa, linux-arm-kernel,
	linux-kernel, dawei.li, jgg, praan, Daniel Mentz

The SMMU specification defines several types of SMMU-originated memory
accesses, including Stage 1 and Stage 2 translation table walks, stream
table accesses (L1STD and STE fetches), CD table accesses (L1CD and CD
fetches), and queue accesses (CMDQ fetch, EVENTQ write, PRIQ write).

While the memory attributes used for Stage 1 and Stage 2 translation
table walks are configured in io-pgtable-arm based on whether the SMMU
is coherent (ARM_SMMU_FEAT_COHERENCY), the attributes for stream tables,
CD tables, and queues are currently hardcoded to Inner Shareable,
Write-Back.

On non-coherent systems, however, memory for stream tables, CD tables,
and queues is allocated via dma_alloc_coherent() /
dmam_alloc_coherent(), which provides CPU mappings with Normal
Non-Cacheable attributes. Having a non-coherent SMMU access these
buffers with Inner Shareable, Write-Back attributes results in
mismatched memory attributes between the CPU and the SMMU.

Configure the memory attributes for tables and queues in
arm_smmu_device_reset() and arm_smmu_make_cdtable_ste() based on
ARM_SMMU_FEAT_COHERENCY, matching the attributes used for translation
table walks:

- In SMMU_CR1, use Outer Shareable, Non-Cacheable for non-coherent
  SMMUs, while retaining Inner Shareable, Write-Back for coherent SMMUs.
  This applies to stream table accesses as well as queue accesses (CMDQ
  fetch, EVENTQ write, PRIQ write).
- In STE.{S1CIR, S1COR, S1CSH}, use Outer Shareable, Non-Cacheable for
  non-coherent SMMUs, while retaining Inner Shareable, Write-Back
  Read-Allocate for coherent SMMUs. This applies to CD table accesses.

Assisted-by: LLM
Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>
Signed-off-by: Daniel Mentz <danielmentz@google.com>
---
Changes in v2:
- Rename local variables 'cache' and 'sh' to 'cr1_cache' and 'cr1_sh' in
  arm_smmu_device_reset() (Nicolin, Will)
- Collect Reviewed-by from Nicolin
- Link to v1: https://lore.kernel.org/linux-iommu/20260929032229.3532247-1-danielmentz@google.com/

 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 37 +++++++++++++++------
 1 file changed, 27 insertions(+), 10 deletions(-)

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index 34e916ea339f..02cc6fb83461 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -1905,6 +1905,15 @@ void arm_smmu_make_cdtable_ste(struct arm_smmu_ste *target,
 {
 	struct arm_smmu_ctx_desc_cfg *cd_table = &master->cd_table;
 	struct arm_smmu_device *smmu = master->smmu;
+	u64 s1c, s1csh;
+
+	if (smmu->features & ARM_SMMU_FEAT_COHERENCY) {
+		s1c = STRTAB_STE_1_S1C_CACHE_WBRA;
+		s1csh = ARM_SMMU_SH_ISH;
+	} else {
+		s1c = STRTAB_STE_1_S1C_CACHE_NC;
+		s1csh = ARM_SMMU_SH_OSH;
+	}
 
 	memset(target, 0, sizeof(*target));
 	target->data[0] = cpu_to_le64(
@@ -1916,9 +1925,9 @@ void arm_smmu_make_cdtable_ste(struct arm_smmu_ste *target,
 
 	target->data[1] = cpu_to_le64(
 		FIELD_PREP(STRTAB_STE_1_S1DSS, s1dss) |
-		FIELD_PREP(STRTAB_STE_1_S1CIR, STRTAB_STE_1_S1C_CACHE_WBRA) |
-		FIELD_PREP(STRTAB_STE_1_S1COR, STRTAB_STE_1_S1C_CACHE_WBRA) |
-		FIELD_PREP(STRTAB_STE_1_S1CSH, ARM_SMMU_SH_ISH) |
+		FIELD_PREP(STRTAB_STE_1_S1CIR, s1c) |
+		FIELD_PREP(STRTAB_STE_1_S1COR, s1c) |
+		FIELD_PREP(STRTAB_STE_1_S1CSH, s1csh) |
 		((smmu->features & ARM_SMMU_FEAT_STALLS &&
 		  !master->stall_enabled) ?
 			 STRTAB_STE_1_S1STALLD :
@@ -5102,7 +5111,7 @@ static void arm_smmu_write_strtab(struct arm_smmu_device *smmu)
 static int arm_smmu_device_reset(struct arm_smmu_device *smmu)
 {
 	int ret;
-	u32 reg, enables;
+	u32 reg, enables, cr1_cache, cr1_sh;
 
 	/* Clear CR0 and sync (disables SMMU and queue processing) */
 	reg = readl_relaxed(smmu->base + ARM_SMMU_CR0);
@@ -5116,12 +5125,20 @@ static int arm_smmu_device_reset(struct arm_smmu_device *smmu)
 		return ret;
 
 	/* CR1 (table and queue memory attributes) */
-	reg = FIELD_PREP(CR1_TABLE_SH, ARM_SMMU_SH_ISH) |
-	      FIELD_PREP(CR1_TABLE_OC, CR1_CACHE_WB) |
-	      FIELD_PREP(CR1_TABLE_IC, CR1_CACHE_WB) |
-	      FIELD_PREP(CR1_QUEUE_SH, ARM_SMMU_SH_ISH) |
-	      FIELD_PREP(CR1_QUEUE_OC, CR1_CACHE_WB) |
-	      FIELD_PREP(CR1_QUEUE_IC, CR1_CACHE_WB);
+	if (smmu->features & ARM_SMMU_FEAT_COHERENCY) {
+		cr1_cache = CR1_CACHE_WB;
+		cr1_sh = ARM_SMMU_SH_ISH;
+	} else {
+		cr1_cache = CR1_CACHE_NC;
+		cr1_sh = ARM_SMMU_SH_OSH;
+	}
+
+	reg = FIELD_PREP(CR1_TABLE_SH, cr1_sh) |
+	      FIELD_PREP(CR1_TABLE_OC, cr1_cache) |
+	      FIELD_PREP(CR1_TABLE_IC, cr1_cache) |
+	      FIELD_PREP(CR1_QUEUE_SH, cr1_sh) |
+	      FIELD_PREP(CR1_QUEUE_OC, cr1_cache) |
+	      FIELD_PREP(CR1_QUEUE_IC, cr1_cache);
 	writel_relaxed(reg, smmu->base + ARM_SMMU_CR1);
 
 	/* CR2 (random crap) */
-- 
2.56.0.rc1.315.gc6ed9934b7-goog


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH v2] iommu/arm-smmu-v3: Align memory attributes for SMMU-originated accesses
  2026-10-04 19:50 [PATCH v2] iommu/arm-smmu-v3: Align memory attributes for SMMU-originated accesses Daniel Mentz
@ 2026-10-05 16:42 ` Robin Murphy
  2026-10-05 21:26   ` Daniel Mentz
  0 siblings, 1 reply; 3+ messages in thread
From: Robin Murphy @ 2026-10-05 16:42 UTC (permalink / raw)
  To: Daniel Mentz, iommu
  Cc: will, joro, nicolinc, smostafa, linux-arm-kernel, linux-kernel,
	dawei.li, jgg, praan

On 04/10/2026 8:50 pm, Daniel Mentz wrote:
> The SMMU specification defines several types of SMMU-originated memory
> accesses, including Stage 1 and Stage 2 translation table walks, stream
> table accesses (L1STD and STE fetches), CD table accesses (L1CD and CD
> fetches), and queue accesses (CMDQ fetch, EVENTQ write, PRIQ write).
> 
> While the memory attributes used for Stage 1 and Stage 2 translation
> table walks are configured in io-pgtable-arm based on whether the SMMU
> is coherent (ARM_SMMU_FEAT_COHERENCY), the attributes for stream tables,
> CD tables, and queues are currently hardcoded to Inner Shareable,
> Write-Back.
> 
> On non-coherent systems, however, memory for stream tables, CD tables,
> and queues is allocated via dma_alloc_coherent() /
> dmam_alloc_coherent(), which provides CPU mappings with Normal
> Non-Cacheable attributes. Having a non-coherent SMMU access these
> buffers with Inner Shareable, Write-Back attributes results in
> mismatched memory attributes between the CPU and the SMMU.
> 
> Configure the memory attributes for tables and queues in
> arm_smmu_device_reset() and arm_smmu_make_cdtable_ste() based on
> ARM_SMMU_FEAT_COHERENCY, matching the attributes used for translation
> table walks:

This still isn't answering the question of "why?" though. Yes the 
architecture says some things, but if we were strict about avoiding 
mismatched attributes then Linux couldn't ever support non-coherent DMA 
at all! Similarly while the architecture does permit SMMU 
implementations to be picky about their output attributes, does any such 
implementation actually exist at all, let alone in a system capable of 
running mainline Linux?

And yes, io-pgtable-arm does happen to be a little more fastidious with 
attributes, but that is more to do with Qualcomm SMMUv2 platforms that 
gave a magic meaning to the outer-cacheable attribute on a 
still-otherwise-non-coherent downstream interconnect - which we support 
via IO_PGTABLE_QUIRK_ARM_OUTER_WBWA - but is not directly relevant to 
arm-smmu-v3 itself.

So, short of a justification why it is unavoidably _necessary_ to change 
an assumption that in practice has held since day one, I'm still going 
to hold the opinion that at worst this smells like a sneaky hack around 
DT/ACPI describing coherency incorrectly, which really should be fixed 
in the firmware; or at best is just churn for no practical benefit that 
will only help hide that bug in future. And if someone does have a 
justifiable use-case for wanting to use a coherent SMMU non-coherently, 
then it's all the more reason to add proper DMA API support for that 
rather than bodging firmware to try to trick Linux, as there's already a 
wishlist of similar use-cases (PCIe No Snoop, Panfrost's tiler heap, etc.)

Thanks,
Robin.

> - In SMMU_CR1, use Outer Shareable, Non-Cacheable for non-coherent
>    SMMUs, while retaining Inner Shareable, Write-Back for coherent SMMUs.
>    This applies to stream table accesses as well as queue accesses (CMDQ
>    fetch, EVENTQ write, PRIQ write).
> - In STE.{S1CIR, S1COR, S1CSH}, use Outer Shareable, Non-Cacheable for
>    non-coherent SMMUs, while retaining Inner Shareable, Write-Back
>    Read-Allocate for coherent SMMUs. This applies to CD table accesses.
> 
> Assisted-by: LLM
> Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>
> Signed-off-by: Daniel Mentz <danielmentz@google.com>
> ---
> Changes in v2:
> - Rename local variables 'cache' and 'sh' to 'cr1_cache' and 'cr1_sh' in
>    arm_smmu_device_reset() (Nicolin, Will)
> - Collect Reviewed-by from Nicolin
> - Link to v1: https://lore.kernel.org/linux-iommu/20260929032229.3532247-1-danielmentz@google.com/
> 
>   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 37 +++++++++++++++------
>   1 file changed, 27 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> index 34e916ea339f..02cc6fb83461 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -1905,6 +1905,15 @@ void arm_smmu_make_cdtable_ste(struct arm_smmu_ste *target,
>   {
>   	struct arm_smmu_ctx_desc_cfg *cd_table = &master->cd_table;
>   	struct arm_smmu_device *smmu = master->smmu;
> +	u64 s1c, s1csh;
> +
> +	if (smmu->features & ARM_SMMU_FEAT_COHERENCY) {
> +		s1c = STRTAB_STE_1_S1C_CACHE_WBRA;
> +		s1csh = ARM_SMMU_SH_ISH;
> +	} else {
> +		s1c = STRTAB_STE_1_S1C_CACHE_NC;
> +		s1csh = ARM_SMMU_SH_OSH;
> +	}
>   
>   	memset(target, 0, sizeof(*target));
>   	target->data[0] = cpu_to_le64(
> @@ -1916,9 +1925,9 @@ void arm_smmu_make_cdtable_ste(struct arm_smmu_ste *target,
>   
>   	target->data[1] = cpu_to_le64(
>   		FIELD_PREP(STRTAB_STE_1_S1DSS, s1dss) |
> -		FIELD_PREP(STRTAB_STE_1_S1CIR, STRTAB_STE_1_S1C_CACHE_WBRA) |
> -		FIELD_PREP(STRTAB_STE_1_S1COR, STRTAB_STE_1_S1C_CACHE_WBRA) |
> -		FIELD_PREP(STRTAB_STE_1_S1CSH, ARM_SMMU_SH_ISH) |
> +		FIELD_PREP(STRTAB_STE_1_S1CIR, s1c) |
> +		FIELD_PREP(STRTAB_STE_1_S1COR, s1c) |
> +		FIELD_PREP(STRTAB_STE_1_S1CSH, s1csh) |
>   		((smmu->features & ARM_SMMU_FEAT_STALLS &&
>   		  !master->stall_enabled) ?
>   			 STRTAB_STE_1_S1STALLD :
> @@ -5102,7 +5111,7 @@ static void arm_smmu_write_strtab(struct arm_smmu_device *smmu)
>   static int arm_smmu_device_reset(struct arm_smmu_device *smmu)
>   {
>   	int ret;
> -	u32 reg, enables;
> +	u32 reg, enables, cr1_cache, cr1_sh;
>   
>   	/* Clear CR0 and sync (disables SMMU and queue processing) */
>   	reg = readl_relaxed(smmu->base + ARM_SMMU_CR0);
> @@ -5116,12 +5125,20 @@ static int arm_smmu_device_reset(struct arm_smmu_device *smmu)
>   		return ret;
>   
>   	/* CR1 (table and queue memory attributes) */
> -	reg = FIELD_PREP(CR1_TABLE_SH, ARM_SMMU_SH_ISH) |
> -	      FIELD_PREP(CR1_TABLE_OC, CR1_CACHE_WB) |
> -	      FIELD_PREP(CR1_TABLE_IC, CR1_CACHE_WB) |
> -	      FIELD_PREP(CR1_QUEUE_SH, ARM_SMMU_SH_ISH) |
> -	      FIELD_PREP(CR1_QUEUE_OC, CR1_CACHE_WB) |
> -	      FIELD_PREP(CR1_QUEUE_IC, CR1_CACHE_WB);
> +	if (smmu->features & ARM_SMMU_FEAT_COHERENCY) {
> +		cr1_cache = CR1_CACHE_WB;
> +		cr1_sh = ARM_SMMU_SH_ISH;
> +	} else {
> +		cr1_cache = CR1_CACHE_NC;
> +		cr1_sh = ARM_SMMU_SH_OSH;
> +	}
> +
> +	reg = FIELD_PREP(CR1_TABLE_SH, cr1_sh) |
> +	      FIELD_PREP(CR1_TABLE_OC, cr1_cache) |
> +	      FIELD_PREP(CR1_TABLE_IC, cr1_cache) |
> +	      FIELD_PREP(CR1_QUEUE_SH, cr1_sh) |
> +	      FIELD_PREP(CR1_QUEUE_OC, cr1_cache) |
> +	      FIELD_PREP(CR1_QUEUE_IC, cr1_cache);
>   	writel_relaxed(reg, smmu->base + ARM_SMMU_CR1);
>   
>   	/* CR2 (random crap) */


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH v2] iommu/arm-smmu-v3: Align memory attributes for SMMU-originated accesses
  2026-10-05 16:42 ` Robin Murphy
@ 2026-10-05 21:26   ` Daniel Mentz
  0 siblings, 0 replies; 3+ messages in thread
From: Daniel Mentz @ 2026-10-05 21:26 UTC (permalink / raw)
  To: Robin Murphy
  Cc: iommu, will, joro, nicolinc, smostafa, linux-arm-kernel,
	linux-kernel, dawei.li, jgg, praan

On Mon, Oct 5, 2026 at 9:42 AM Robin Murphy <robin.murphy@arm.com> wrote:
> This still isn't answering the question of "why?" though. Yes the
> architecture says some things, but if we were strict about avoiding
> mismatched attributes then Linux couldn't ever support non-coherent DMA
> at all! Similarly while the architecture does permit SMMU
> implementations to be picky about their output attributes, does any such
> implementation actually exist at all, let alone in a system capable of
> running mainline Linux?

Fair point that existing implementations have been forgiving in
practice. My thinking here is simply that a small, self-contained
cleanup that brings the driver into line with the architecture spec is
worthwhile on its own merits, even without a known implementation where
this currently causes problems.

This is really in the same spirit as your commit 7618e4790982
("iommu/io-pgtable-arm: Improve attribute handling"), which aligned the
attribute handling with the architecture specification on the basis
that:

  "Although the SMMU architectures seem to give some slightly stronger
   guarantees of Non-Cacheable output types becoming implicitly Outer
   Shareable in most cases, we may as well be explicit and not take any
   chances."

As I noted in my reply on the v1 thread [1], under ARM IHI 0070
(sections 3.15, 6.3.11, and 13.1.2), a non-coherent SMMUv3
implementation (SMMU_IDR0.COHACC == 0) in which every SMMU-originated
access configured with Normal Write-Back, Inner Shareable attributes
fails and records an External Abort (F_STE_FETCH, F_CD_FETCH,
CERROR_ABT, etc.) is completely architecturally compliant, yet wouldn't
work with the current arm-smmu-v3 driver.

It seems reasonable to be explicit here too and program attributes that
are valid per the spec, rather than relying on the interconnect to
silently degrade Write-Back attributes to Non-Cacheable.

[1] https://lore.kernel.org/linux-iommu/CAE2F3rACz6Z7X3NNfLWEfjTD9K9YJyYHHyGzcBafEemTFGhgqQ@mail.gmail.com/

Thanks,
Daniel

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2026-10-05 21:26 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-10-04 19:50 [PATCH v2] iommu/arm-smmu-v3: Align memory attributes for SMMU-originated accesses Daniel Mentz
2026-10-05 16:42 ` Robin Murphy
2026-10-05 21:26   ` Daniel Mentz

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®