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

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

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.

-- 
Regards,
Yi Liu