[PATCH] arm/smmuv3: clean stream tables for non-coherent hardware

[PATCH] arm/smmuv3: clean stream tables for non-coherent hardware

[Mykola Kvach]

Xen's dmam_alloc_coherent() returns cacheable memory. A non-coherent
SMMU cannot see writes that remain in the CPU cache.

On Orange Pi 5 (RK3588S), NVMe initialization failed with C_BAD_STREAMID
for SID 0x4100 while the CPU saw a populated L1 descriptor.

Clean new stream tables before use and updated STEs before the existing
configuration invalidations. Clean the L2 table before writing its L1
pointer, then clean the pointer. Skip these cache operations on coherent
SMMUs.

See Arm IHI 0070G.b, sections 3.15 and 3.21.3.1.

Fixes: 452ddbe3592b ("xen/arm: smmuv3: Add support for SMMUv3 driver")
Assisted-by: Codex:GPT-6.1
Signed-off-by: Mykola Kvach <mykola_kvach@epam.com>
---
 xen/drivers/passthrough/arm/smmu-v3.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/xen/drivers/passthrough/arm/smmu-v3.c b/xen/drivers/passthrough/arm/smmu-v3.c
index bf153227db..a396e2b9e0 100644
--- a/xen/drivers/passthrough/arm/smmu-v3.c
+++ b/xen/drivers/passthrough/arm/smmu-v3.c
@@ -637,6 +637,14 @@ static int arm_smmu_cmdq_issue_sync(struct arm_smmu_device *smmu)
 }
 
 /* Stream table manipulation functions */
+static void arm_smmu_clean_strtab(struct arm_smmu_device *smmu,
+				const void *ptr, size_t size)
+{
+	/* Xen's dmam_alloc_coherent() returns cacheable memory. */
+	if (!(smmu->features & ARM_SMMU_FEAT_COHERENCY))
+		(void)clean_dcache_va_range(ptr, size);
+}
+
 static void
 arm_smmu_write_strtab_l1_desc(__le64 *dst, struct arm_smmu_strtab_l1_desc *desc)
 {
@@ -735,8 +743,11 @@ static void arm_smmu_write_strtab_ent(struct arm_smmu_master *master, u32 sid,
 		 * The SMMU can perform negative caching, so we must sync
 		 * the STE regardless of whether the old value was live.
 		 */
-		if (smmu)
+		if (smmu) {
+			arm_smmu_clean_strtab(smmu, dst,
+					      STRTAB_STE_DWORDS * sizeof(*dst));
 			arm_smmu_sync_ste_for_sid(smmu, sid);
+		}
 		return;
 	}
 
@@ -762,8 +773,10 @@ static void arm_smmu_write_strtab_ent(struct arm_smmu_master *master, u32 sid,
 		dst[1] |= cpu_to_le64(FIELD_PREP(STRTAB_STE_1_EATS,
 						 STRTAB_STE_1_EATS_TRANS));
 
+	arm_smmu_clean_strtab(smmu, dst, STRTAB_STE_DWORDS * sizeof(*dst));
 	arm_smmu_sync_ste_for_sid(smmu, sid);
 	write_atomic(&dst[0], cpu_to_le64(val));
+	arm_smmu_clean_strtab(smmu, dst, sizeof(*dst));
 	arm_smmu_sync_ste_for_sid(smmu, sid);
 
 	/* It's likely that we'll want to use the new STE soon */
@@ -805,7 +818,11 @@ static int arm_smmu_init_l2_strtab(struct arm_smmu_device *smmu, u32 sid)
 	}
 
 	arm_smmu_init_bypass_stes(desc->l2ptr, 1 << STRTAB_SPLIT);
+	/* Clean the L2 table before writing the L1 pointer. */
+	arm_smmu_clean_strtab(smmu, desc->l2ptr, size);
 	arm_smmu_write_strtab_l1_desc(strtab, desc);
+	arm_smmu_clean_strtab(smmu, strtab,
+			      STRTAB_L1_DESC_DWORDS * sizeof(__le64));
 	return 0;
 }
 
@@ -1700,6 +1717,9 @@ static int arm_smmu_init_l1_strtab(struct arm_smmu_device *smmu)
 		strtab += STRTAB_L1_DESC_DWORDS << 3;
 	}
 
+	arm_smmu_clean_strtab(smmu, cfg->strtab,
+		cfg->num_l1_ents * STRTAB_L1_DESC_DWORDS * sizeof(__le64));
+
 	return 0;
 }
 
@@ -1766,6 +1786,7 @@ static int arm_smmu_init_strtab_linear(struct arm_smmu_device *smmu)
 	cfg->strtab_base_cfg = reg;
 
 	arm_smmu_init_bypass_stes(strtab, cfg->num_l1_ents);
+	arm_smmu_clean_strtab(smmu, strtab, size);
 	return 0;
 }
 
-- 
2.53.0

Re: [PATCH] arm/smmuv3: clean stream tables for non-coherent hardware

[Volodymyr Babchuk]

Hi,

Mykola Kvach <mykola_kvach@epam.com> writes:

> Xen's dmam_alloc_coherent() returns cacheable memory. A non-coherent
> SMMU cannot see writes that remain in the CPU cache.
>
> On Orange Pi 5 (RK3588S), NVMe initialization failed with C_BAD_STREAMID
> for SID 0x4100 while the CPU saw a populated L1 descriptor.
>
> Clean new stream tables before use and updated STEs before the existing
> configuration invalidations. Clean the L2 table before writing its L1
> pointer, then clean the pointer. Skip these cache operations on coherent
> SMMUs.
>
> See Arm IHI 0070G.b, sections 3.15 and 3.21.3.1.
>
> Fixes: 452ddbe3592b ("xen/arm: smmuv3: Add support for SMMUv3 driver")
> Assisted-by: Codex:GPT-6.1
> Signed-off-by: Mykola Kvach <mykola_kvach@epam.com>
> ---
>  xen/drivers/passthrough/arm/smmu-v3.c | 23 ++++++++++++++++++++++-
>  1 file changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/xen/drivers/passthrough/arm/smmu-v3.c b/xen/drivers/passthrough/arm/smmu-v3.c
> index bf153227db..a396e2b9e0 100644
> --- a/xen/drivers/passthrough/arm/smmu-v3.c
> +++ b/xen/drivers/passthrough/arm/smmu-v3.c
> @@ -637,6 +637,14 @@ static int arm_smmu_cmdq_issue_sync(struct arm_smmu_device *smmu)
>  }
>  
>  /* Stream table manipulation functions */
> +static void arm_smmu_clean_strtab(struct arm_smmu_device *smmu,

I think the name is a bit misleading. You are not cleaning the stream
table. You are cleaning cache entries. Also, behaviour of this function
is more generic, as it know nothing about stream tables and clears any
cache line you are giving it.

Maybe call it something like arm_smmu_flush_strtab, or
arm_smmu_clean_dcache, or something along these variants?

> +				const void *ptr, size_t size)
> +{
> +	/* Xen's dmam_alloc_coherent() returns cacheable memory. */
> +	if (!(smmu->features & ARM_SMMU_FEAT_COHERENCY))
> +		(void)clean_dcache_va_range(ptr, size);
> +}
> +
>  static void
>  arm_smmu_write_strtab_l1_desc(__le64 *dst, struct arm_smmu_strtab_l1_desc *desc)
>  {
> @@ -735,8 +743,11 @@ static void arm_smmu_write_strtab_ent(struct arm_smmu_master *master, u32 sid,
>  		 * The SMMU can perform negative caching, so we must sync
>  		 * the STE regardless of whether the old value was live.
>  		 */
> -		if (smmu)
> +		if (smmu) {
> +			arm_smmu_clean_strtab(smmu, dst,
> +					      STRTAB_STE_DWORDS * sizeof(*dst));
>  			arm_smmu_sync_ste_for_sid(smmu, sid);

I see that there a lot of places where you are calling
arm_smmu_clean_strtab() before arm_smmu_sync_ste_for_sid(). I think it
is better to move arm_smmu_clean_strtab() to the beginning of
arm_smmu_sync_ste_for_sid().

> +		}
>  		return;
>  	}
>  
> @@ -762,8 +773,10 @@ static void arm_smmu_write_strtab_ent(struct arm_smmu_master *master, u32 sid,
>  		dst[1] |= cpu_to_le64(FIELD_PREP(STRTAB_STE_1_EATS,
>  						 STRTAB_STE_1_EATS_TRANS));
>  
> +	arm_smmu_clean_strtab(smmu, dst, STRTAB_STE_DWORDS * sizeof(*dst));
>  	arm_smmu_sync_ste_for_sid(smmu, sid);
>  	write_atomic(&dst[0], cpu_to_le64(val));
> +	arm_smmu_clean_strtab(smmu, dst, sizeof(*dst));
>  	arm_smmu_sync_ste_for_sid(smmu, sid);
>  
>  	/* It's likely that we'll want to use the new STE soon */
> @@ -805,7 +818,11 @@ static int arm_smmu_init_l2_strtab(struct arm_smmu_device *smmu, u32 sid)
>  	}
>  
>  	arm_smmu_init_bypass_stes(desc->l2ptr, 1 << STRTAB_SPLIT);
> +	/* Clean the L2 table before writing the L1 pointer. */
> +	arm_smmu_clean_strtab(smmu, desc->l2ptr, size);
>  	arm_smmu_write_strtab_l1_desc(strtab, desc);
> +	arm_smmu_clean_strtab(smmu, strtab,
> +			      STRTAB_L1_DESC_DWORDS * sizeof(__le64));
>  	return 0;
>  }
>  
> @@ -1700,6 +1717,9 @@ static int arm_smmu_init_l1_strtab(struct arm_smmu_device *smmu)
>  		strtab += STRTAB_L1_DESC_DWORDS << 3;
>  	}
>  
> +	arm_smmu_clean_strtab(smmu, cfg->strtab,
> +		cfg->num_l1_ents * STRTAB_L1_DESC_DWORDS * sizeof(__le64));
> +
>  	return 0;
>  }
>  
> @@ -1766,6 +1786,7 @@ static int arm_smmu_init_strtab_linear(struct arm_smmu_device *smmu)
>  	cfg->strtab_base_cfg = reg;
>  
>  	arm_smmu_init_bypass_stes(strtab, cfg->num_l1_ents);
> +	arm_smmu_clean_strtab(smmu, strtab, size);
>  	return 0;
>  }

-- 
WBR, Volodymyr