RE: [PATCH v4 02/13] iommu/vt-d: Add a helper to flush cache for updating present pasid entry

Tian, Kevin
2024-11-06T09:41:26Z

> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Wednesday, November 6, 2024 4:46 PM
>
> On 2024/11/6 15:11, Tian, Kevin wrote:
> >> From: Liu, Yi L <yi.l.liu@intel.com>
> >> Sent: Monday, November 4, 2024 9:19 PM
> >>
> >> There are more paths that need to flush cache for present pasid entry
> >> after adding pasid replacement. Hence, add a helper for it. Per the
> >> VT-d spec, the changes to the fields other than SSADE and P bit can
> >> share the same code. So intel_pasid_setup_page_snoop_control() is the
> >> first user of this helper.
> >
> > No hw spec would ever talk about coding sharing in sw implementation.
>
> yes, it's just sw choice.
>
> > and according to the following context the fact is just that two flows
> > between RID and PASID are similar so you decide to create a common
> > helper for both.
>
> I'm not quite getting why RID is related. This is only about the cache
> flush per pasid entry updating.

the comment says:

+ * - If (pasid is RID_PASID)
+ * - Global Device-TLB invalidation to affected functions
+ * Else
+ * - PASID-based Device-TLB invalidation (with S=1 and
+ * Addr[63:12]=0x7FFFFFFF_FFFFF) to affected functions

so that is the only difference between two invalidation flows?

>
> >>
> >> No functional change is intended.
> >>
> >> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> >> ---
> >> drivers/iommu/intel/pasid.c | 54 ++++++++++++++++++++++++-------------
> >> 1 file changed, 36 insertions(+), 18 deletions(-)
> >>
> >> diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
> >> index 977c4ac00c4c..81d038222414 100644
> >> --- a/drivers/iommu/intel/pasid.c
> >> +++ b/drivers/iommu/intel/pasid.c
> >> @@ -286,6 +286,41 @@ static void pasid_flush_caches(struct
> intel_iommu
> >> *iommu,
> >> }
> >> }
> >>
> >> +/*
> >> + * This function is supposed to be used after caller updates the fields
> >> + * except for the SSADE and P bit of a pasid table entry. It does the
> >> + * below:
> >> + * - Flush cacheline if needed
> >> + * - Flush the caches per the guidance of VT-d spec 5.0 Table 28.
> >
> > while at it please add the name for the table.
>
> sure.
>
> >
> >> + * ”Guidance to Software for Invalidations“
> >> + *
> >> + * Caller of it should not modify the in-use pasid table entries.
> >
> > I'm not sure about this statement. As long as the change doesn't
> > impact in-fly DMAs it's always the caller's right for whether to do it.
> >
> > actually based on the main intention of supporting replace it's
> > quite possible.
>
> This comment is mainly due to the clflush_cache_range() within this
> helper. If caller modifies the pte content, it will need to call
> this again.
>

Isn't it obvious about the latter part?