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

[Yi Liu <yi.l.liu@intel.com>]

On 2024/11/6 17:40, Tian, Kevin wrote:
>> 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?

oh, yes. But there are multiple PASID paths that need flush. e.g. the
pasid teardown. However, this path cannot use this helper introduced
here as it modifies the Present bit. Per table 28, the teardown path
should check pgtt to decide between p_iotlb_inv and iotlb_inv.

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

hmmm, I added it mainly by referring to the below helper. This helper
is for newly setup pasid entries, so in flight DMA is not relevant.

/*
  * This function flushes cache for a newly setup pasid table entry.
  * Caller of it should not modify the in-use pasid table entries.
  */
static void pasid_flush_caches(struct intel_iommu *iommu,
				struct pasid_entry *pte,
			       u32 pasid, u16 did)
{

-- 
Regards,
Yi Liu