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

[Ayan Kumar Halder <ayankuma@xxxxxxx>]

On 15/05/2023 07:46, Michal Orzel wrote:
> Hi Ayan,
Hi Michal,
> On 12/05/2023 18:59, Ayan Kumar Halder wrote:
> > Hi Michal,
> >
> > 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>
Reviewed-by: Ayan Kumar Halder <ayan.kumar.halder@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;
> > > +
> > >    static int __must_check arm_smmu_iotlb_flush_all(struct domain *d)
> > >    {
> > >         struct arm_smmu_xen_domain *xen_domain = dom_iommu(d)->arch.priv;
> > > @@ -2708,8 +2717,12 @@ static int arm_smmu_iommu_xen_domain_init(struct domain 
> > > *d)
> > >         INIT_LIST_HEAD(&xen_domain->contexts);
> > >    
> > >    	dom_iommu(d)->arch.priv = xen_domain;
> > > -       return 0;
> > >    
> > > +	/* Coherent walk can be enabled only when all SMMUs support it. */
> > > +       if (platform_features & ARM_SMMU_FEAT_COHERENCY)
> > > +               iommu_set_feature(d, IOMMU_FEAT_COHERENT_WALK);
> > > +
> > > +       return 0;
> > >    }
> > >    
> > All good till here.
> > >    static void arm_smmu_iommu_xen_domain_teardown(struct domain *d)
> > > @@ -2738,6 +2751,7 @@ static __init int arm_smmu_dt_init(struct dt_device_node 
> > > *dev,
> > >                                 const void *data)
> > >    {
> > >         int rc;
> > > +       const struct arm_smmu_device *smmu;
> > >    
> > >    	/*
> > >          * Even if the device can't be initialized, we don't want to
> > > @@ -2751,6 +2765,14 @@ static __init int arm_smmu_dt_init(struct dt_device_node 
> > > *dev,
> > >    
> > >    	iommu_set_ops(&arm_smmu_iommu_ops);
> > >    
> > > +	/* Find the just added SMMU and retrieve its features. */
> > > +       smmu = arm_smmu_get_by_dev(dt_to_dev(dev));
> > > +
> > > +       /* It would be a bug not to find the SMMU we just added. */
> > > +       BUG_ON(!smmu);
> > > +
> > > +       platform_features &= smmu->features;
> > > +
> > Can you explain this change in the commit message ?
> I think it is already explained by saying that in order to advertise the 
> *platform* feature, all
> SMMUs need to report it. If at least one doesn't, the feature is disabled. This 
> is exactly
> what this line is doing. It is comparing platform features with a just probed 
> SMMU features (arm_smmu_dt_init()
> is called for each SMMU).
All good.
> ~Michal