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

[Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>]

Hi,

Mykola Kvach <mykola_kvach@xxxxxxxx> 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@xxxxxxxx>
> ---
>  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