Re: [PATCH v3 3/9] iommu/vt-d: Let intel_pasid_tear_down_entry() return pasid entry

[Baolu Lu <baolu.lu@linux.intel.com>]

On 2024/10/22 17:38, Yi Liu wrote:
> On 2024/10/22 17:23, Baolu Lu wrote:
>> On 2024/10/21 15:24, Yi Liu wrote:
>>> On 2024/10/21 14:59, Baolu Lu wrote:
>>>> On 2024/10/21 14:35, Yi Liu wrote:
>>>>> On 2024/10/21 14:13, Baolu Lu wrote:
>>>>>> On 2024/10/18 13:53, Yi Liu wrote:
>>>>>>> intel_pasid_tear_down_entry() finds the pasid entry and tears it 
>>>>>>> down.
>>>>>>> There are paths that need to get the pasid entry, tear it down and
>>>>>>> re-configure it. Letting intel_pasid_tear_down_entry() return the 
>>>>>>> pasid
>>>>>>> entry can avoid duplicate codes to get the pasid entry. No 
>>>>>>> functional
>>>>>>> change is intended.
>>>>>>>
>>>>>>> Signed-off-by: Yi Liu<yi.l.liu@intel.com>
>>>>>>> ---
>>>>>>>   drivers/iommu/intel/pasid.c | 11 ++++++++---
>>>>>>>   drivers/iommu/intel/pasid.h |  5 +++--
>>>>>>>   2 files changed, 11 insertions(+), 5 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/ 
>>>>>>> pasid.c
>>>>>>> index 2898e7af2cf4..336f9425214c 100644
>>>>>>> --- a/drivers/iommu/intel/pasid.c
>>>>>>> +++ b/drivers/iommu/intel/pasid.c
>>>>>>> @@ -239,9 +239,12 @@ devtlb_invalidation_with_pasid(struct 
>>>>>>> intel_iommu *iommu,
>>>>>>>   /*
>>>>>>>    * Caller can request to drain PRQ in this helper if it hasn't 
>>>>>>> done so,
>>>>>>>    * e.g. in a path which doesn't follow remove_dev_pasid().
>>>>>>> + * Return the pasid entry pointer if the entry is found or NULL 
>>>>>>> if no
>>>>>>> + * entry found.
>>>>>>>    */
>>>>>>> -void intel_pasid_tear_down_entry(struct intel_iommu *iommu, 
>>>>>>> struct device *dev,
>>>>>>> -                 u32 pasid, u32 flags)
>>>>>>> +struct pasid_entry *
>>>>>>> +intel_pasid_tear_down_entry(struct intel_iommu *iommu, struct 
>>>>>>> device *dev,
>>>>>>> +                u32 pasid, u32 flags)
>>>>>>>   {
>>>>>>>       struct pasid_entry *pte;
>>>>>>>       u16 did, pgtt;
>>>>>>> @@ -250,7 +253,7 @@ void intel_pasid_tear_down_entry(struct 
>>>>>>> intel_iommu *iommu, struct device *dev,
>>>>>>>       pte = intel_pasid_get_entry(dev, pasid);
>>>>>>>       if (WARN_ON(!pte) || !pasid_pte_is_present(pte)) {
>>>>>>>           spin_unlock(&iommu->lock);
>>>>>>> -        return;
>>>>>>> +        goto out;
>>>>>>
>>>>>> The pasid table entry is protected by iommu->lock. It's  not 
>>>>>> reasonable
>>>>>> to return the pte pointer which is beyond the lock protected range.
>>>>>
>>>>> Per my understanding, the iommu->lock protects the content of the 
>>>>> entry,
>>>>> so the modifications to the entry need to hold it. While, it looks not
>>>>> necessary to protect the pasid entry pointer itself. The pasid 
>>>>> table should
>>>>> exist during device probe and release. is it?
>>>>
>>>> The pattern of the code that modifies a pasid table entry is,
>>>>
>>>>      spin_lock(&iommu->lock);
>>>>      pte = intel_pasid_get_entry(dev, pasid);
>>>>      ... modify the pasid table entry ...
>>>>      spin_unlock(&iommu->lock);
>>>>
>>>> Returning the pte pointer to the caller introduces a potential race
>>>> condition. If the caller subsequently modifies the pte without re-
>>>> acquiring the spin lock, there's a risk of data corruption or
>>>> inconsistencies.
>>>
>>> it appears that we are on the same page about if pte pointer needs to be
>>> protected or not. And I agree the modifications to the pte should be
>>> protected by iommu->lock. If so, will documenting that the caller 
>>> must hold
>>> iommu->lock if is tries to modify the content of pte work? Also, it 
>>> might
>>> be helpful to add lockdep to make sure all the modifications of pte 
>>> entry
>>> are under protection.
>>
>> People will soon forget about this lock and may modify the returned pte
>> pointer without locking, introducing a race condition silently.
>>
>>> Or any suggestion from you given a path that needs to get pte first, 
>>> check
>>> if it exists and then call intel_pasid_tear_down_entry(). For example 
>>> the
>>> intel_pasid_setup_first_level() [1], in my series, I need to call the
>>> unlock iommu->lock and call intel_pasid_tear_down_entry() and then lock
>>> iommu->lock and do more modifications on the pasid entry. It would 
>>> invoke
>>> the intel_pasid_get_entry() twice if no change to
>>> intel_pasid_tear_down_entry().
>>
>> There is no need to check the present of a pte entry before calling into
>> intel_pasid_tear_down_entry(). The helper will return directly if the
>> pte is not present:
>>
>>          spin_lock(&iommu->lock);
>>          pte = intel_pasid_get_entry(dev, pasid);
>>          if (WARN_ON(!pte) || !pasid_pte_is_present(pte)) {
>>                  spin_unlock(&iommu->lock);
>>                  return;
>>          }
>>
>> Does it work for you?
> 
> This is not I'm talking about. My intention is to avoid duplicated
> intel_pasid_get_entry() call when calling intel_pasid_tear_down_entry() in
> intel_pasid_setup_first_level(). Both the two functions call the
> intel_pasid_get_entry() to get pte pointer. So I think it might be good to
> save one of them.

Then, perhaps you can add a pasid_entry_tear_down() helper which asserts
iommu->lock and call it in both intel_pasid_tear_down_entry() and
intel_pasid_setup_first_level()?

Thanks,
baolu