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

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

Hi Volodymyr,

Thanks for the review.

On Wed, Oct 28, 2026 at 3:48 AM Volodymyr Babchuk
<Volodymyr_Babchuk@xxxxxxxx> wrote:
>
> 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?

Agreed, arm_smmu_clean_dcache() describes the operation more precisely.
I'll use that name in v2.

>
> > +                             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().

Agreed. I'll pass the STE pointer to arm_smmu_sync_ste_for_sid() and
clean the whole entry there before issuing CFGI_STE and CMD_SYNC.
All current callers already have the pointer available.

This will preserve both sync points around the dst[0] update.

I'll keep the explicit cache cleaning in the table initialisation
paths.

Thanks,
Mykola


>
> > +             }
> >               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