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

Yi Liu
2024-11-06T08:42:12Z

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.

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

> otherwise this looks good,
>
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>

--
Regards,
Yi Liu