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

Tian, Kevin
2024-11-06T07:11:40Z

> 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.

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.

>
> 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.

> + * ”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.

otherwise this looks good,

Reviewed-by: Kevin Tian <kevin.tian@intel.com>