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

-- 
Regards,
Yi Liu