Re: [PATCH v2] iommu/arm-smmu-v3: Align memory attributes for SMMU-originated accesses

[Robin Murphy <robin.murphy@arm.com>]

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) */