Re: [PATCH 2/2] xen/arm: smmuv3: Advertise coherent table walk if supported

[Julien Grall <julien@xxxxxxx>]

On 15/05/2023 10:03, Michal Orzel wrote:
> Hi Julien,
Hi Michal,

> On 15/05/2023 10:56, Julien Grall wrote:
> > Hi,
> >
> > On 12/05/2023 15:35, Michal Orzel wrote:
> > > At the moment, even in case of a SMMU being I/O coherent, we clean the
> > > updated PT as a result of not advertising the coherency feature. SMMUv3
> > > coherency feature means that page table walks, accesses to memory
> > > structures and queues are I/O coherent (refer ARM IHI 0070 E.A, 3.15).
> > >
> > > Follow the same steps that were done for SMMU v1,v2 driver by the commit:
> > > 080dcb781e1bc3bb22f55a9dfdecb830ccbabe88
> > >
> > > The same restrictions apply, meaning that in order to advertise coherent
> > > table walk platform feature, all the SMMU devices need to report coherency
> > > feature. This is because the page tables (we are sharing them with CPU)
> > > are populated before any device assignment and in case of a device being
> > > behind non-coherent SMMU, we would have to scan the tables and clean
> > > the cache.
> > >
> > > It is to be noted that the SBSA/BSA (refer ARM DEN0094C 1.0C, section D)
> > > requires that all SMMUv3 devices support I/O coherency.
> > >
> > > Signed-off-by: Michal Orzel <michal.orzel@xxxxxxx>
> > > ---
> > > There are very few platforms out there with SMMUv3 but I have never seen
> > > a SMMUv3 that is not I/O coherent.
> > > ---
> > >    xen/drivers/passthrough/arm/smmu-v3.c | 24 +++++++++++++++++++++++-
> > >    1 file changed, 23 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/xen/drivers/passthrough/arm/smmu-v3.c 
> > > b/xen/drivers/passthrough/arm/smmu-v3.c
> > > index bf053cdb6d5c..2adaad0fa038 100644
> > > --- a/xen/drivers/passthrough/arm/smmu-v3.c
> > > +++ b/xen/drivers/passthrough/arm/smmu-v3.c
> > > @@ -2526,6 +2526,15 @@ static const struct dt_device_match arm_smmu_of_match[] 
> > > = {
> > >    };
> > >
> > >    /* Start of Xen specific code. */
> > > +
> > > +/*
> > > + * Platform features. It indicates the list of features supported by all
> > > + * SMMUs. Actually we only care about coherent table walk, which in case of
> > > + * SMMUv3 is implied by the overall coherency feature (refer ARM IHI 0070 E.A,
> > > + * section 3.15 and SMMU_IDR0.COHACC bit description).
> > > + */
> > > +static uint32_t platform_features = ARM_SMMU_FEAT_COHERENCY;
> > AFAICT, this variable is not meant to change after boot. So please add
> > the attribute __ro_after_init.
> Yes, that makes total sense. After probing this variable is not meant to be 
> modified.
> Is it something that can be done on commit or would you want me to respin this 
> patch?
I can do it on commit. With that:

Reviewed-by: Julien Grall <jgrall@xxxxxxxxxx>


Cheers,

--
Julien Grall