> From: Liu, Yi L <yi.l.liu@xxxxxxxxx> > Sent: Wednesday, November 6, 2024 4:46 PM > > On 2024/11/6 15:11, Tian, Kevin wrote: > >> From: Liu, Yi L <yi.l.liu@xxxxxxxxx> > >> 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@xxxxxxxxx> > >> --- > >> 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?
