Re: [PATCH v12 3/5] iommu/vt-d: simplify parameters of qi_submit_sync() ATS invalidation callers

[Ethan Zhao <haifeng.zhao@linux.intel.com>]

On 1/29/2024 5:37 PM, Yi Liu wrote:
> On 2024/1/29 11:49, Ethan Zhao wrote:
>> fold parameters back to struct device_domain_info *info instead of 
>> extract
>> and pass them, thus decrease the number of the parameter passed for
>> following functions
>>
>> qi_flush_dev_iotlb()
>> qi_flush_dev_iotlb_pasid()
>> quirk_extra_dev_tlb_flush()
>>
>> no function change.
>>
>> Signed-off-by: Ethan Zhao <haifeng.zhao@linux.intel.com>
>> ---
>>   drivers/iommu/intel/dmar.c   | 26 ++++++++++++++++++++++----
>>   drivers/iommu/intel/iommu.c  | 29 +++++++----------------------
>>   drivers/iommu/intel/iommu.h  | 17 ++++++++---------
>>   drivers/iommu/intel/nested.c |  9 ++-------
>>   drivers/iommu/intel/pasid.c  |  9 ++-------
>>   drivers/iommu/intel/svm.c    | 17 ++++++++---------
>>   6 files changed, 49 insertions(+), 58 deletions(-)
>>
>> diff --git a/drivers/iommu/intel/dmar.c b/drivers/iommu/intel/dmar.c
>> index 23cb80d62a9a..ab5e1760bd59 100644
>> --- a/drivers/iommu/intel/dmar.c
>> +++ b/drivers/iommu/intel/dmar.c
>> @@ -1517,11 +1517,20 @@ void qi_flush_iotlb(struct intel_iommu 
>> *iommu, u16 did, u64 addr,
>>       qi_submit_sync(iommu, &desc, 1, 0);
>>   }
>>   -void qi_flush_dev_iotlb(struct intel_iommu *iommu, u16 sid, u16 
>> pfsid,
>> -            u16 qdep, u64 addr, unsigned mask)
>> +void qi_flush_dev_iotlb(struct intel_iommu *iommu,
>> +            struct device_domain_info *info, u64 addr,
>
> If you want to fold the parameters, why iommu is left? info also includes
> iommu pointer.

Good catch.

No reason to leave it there.


Thanks,

Ethan

>
>> +            unsigned int mask)
>>   {
>> +    u16 sid, qdep, pfsid;
>>       struct qi_desc desc;
>>   +    if (!info || !info->ats_enabled)
>> +        return;
>> +
>> +    sid = info->bus << 8 | info->devfn;
>> +    qdep = info->ats_qdep;
>> +    pfsid = info->pfsid;
>> +
>>       /*
>>        * VT-d spec, section 4.3:
>>        *
>> @@ -1590,11 +1599,20 @@ void qi_flush_piotlb(struct intel_iommu 
>> *iommu, u16 did, u32 pasid, u64 addr,
>>   }
>>     /* PASID-based device IOTLB Invalidate */
>> -void qi_flush_dev_iotlb_pasid(struct intel_iommu *iommu, u16 sid, 
>> u16 pfsid,
>> -                  u32 pasid,  u16 qdep, u64 addr, unsigned int 
>> size_order)
>> +void qi_flush_dev_iotlb_pasid(struct intel_iommu *iommu,
>> +                  struct device_domain_info *info, u64 addr, u32 pasid,
>> +                  unsigned int size_order)
>>   {
>>       unsigned long mask = 1UL << (VTD_PAGE_SHIFT + size_order - 1);
>>       struct qi_desc desc = {.qw1 = 0, .qw2 = 0, .qw3 = 0};
>> +    u16 sid, qdep, pfsid;
>> +
>> +    if (!info || !dev_is_pci(info->dev))
>> +        return;
>> +
>> +    sid = info->bus << 8 | info->devfn;
>> +    qdep = info->ats_qdep;
>> +    pfsid = info->pfsid;
>>         /*
>>        * VT-d spec, section 4.3:
>> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
>> index 6fb5f6fceea1..e5902944b3db 100644
>> --- a/drivers/iommu/intel/iommu.c
>> +++ b/drivers/iommu/intel/iommu.c
>> @@ -1310,16 +1310,11 @@ static void iommu_disable_pci_caps(struct 
>> device_domain_info *info)
>>   static void __iommu_flush_dev_iotlb(struct device_domain_info *info,
>>                       u64 addr, unsigned int mask)
>>   {
>> -    u16 sid, qdep;
>> -
>>       if (!info || !info->ats_enabled)
>>           return;
>>   -    sid = info->bus << 8 | info->devfn;
>> -    qdep = info->ats_qdep;
>> -    qi_flush_dev_iotlb(info->iommu, sid, info->pfsid,
>> -               qdep, addr, mask);
>> -    quirk_extra_dev_tlb_flush(info, addr, mask, IOMMU_NO_PASID, qdep);
>> +    qi_flush_dev_iotlb(info->iommu, info, addr, mask);
>> +    quirk_extra_dev_tlb_flush(info, addr, IOMMU_NO_PASID, mask);
>>   }
>>     static void iommu_flush_dev_iotlb(struct dmar_domain *domain,
>> @@ -1342,11 +1337,7 @@ static void iommu_flush_dev_iotlb(struct 
>> dmar_domain *domain,
>>           if (!info->ats_enabled)
>>               continue;
>>   -        qi_flush_dev_iotlb_pasid(info->iommu,
>> -                     PCI_DEVID(info->bus, info->devfn),
>> -                     info->pfsid, dev_pasid->pasid,
>> -                     info->ats_qdep, addr,
>> -                     mask);
>> +        qi_flush_dev_iotlb_pasid(info->iommu, info, addr, 
>> dev_pasid->pasid, mask);
>>       }
>>       spin_unlock_irqrestore(&domain->lock, flags);
>>   }
>> @@ -4990,22 +4981,16 @@ static void __init check_tylersburg_isoch(void)
>>    *
>>    * As a reminder, #6 will *NEED* this quirk as we enable nested 
>> translation.
>>    */
>> -void quirk_extra_dev_tlb_flush(struct device_domain_info *info,
>> -                   unsigned long address, unsigned long mask,
>> -                   u32 pasid, u16 qdep)
>> +void quirk_extra_dev_tlb_flush(struct device_domain_info *info, u32 
>> pasid,
>> +                   unsigned long address, unsigned long mask)
>>   {
>> -    u16 sid;
>> -
>>       if (likely(!info->dtlb_extra_inval))
>>           return;
>>   -    sid = PCI_DEVID(info->bus, info->devfn);
>>       if (pasid == IOMMU_NO_PASID) {
>> -        qi_flush_dev_iotlb(info->iommu, sid, info->pfsid,
>> -                   qdep, address, mask);
>> +        qi_flush_dev_iotlb(info->iommu, info, address, mask);
>>       } else {
>> -        qi_flush_dev_iotlb_pasid(info->iommu, sid, info->pfsid,
>> -                     pasid, qdep, address, mask);
>> +        qi_flush_dev_iotlb_pasid(info->iommu, info, address, pasid, 
>> mask);
>>       }
>>   }
>>   diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h
>> index d02f916d8e59..f68f17476d85 100644
>> --- a/drivers/iommu/intel/iommu.h
>> +++ b/drivers/iommu/intel/iommu.h
>> @@ -1037,18 +1037,17 @@ void qi_flush_context(struct intel_iommu 
>> *iommu, u16 did,
>>                 u16 sid, u8 fm, u64 type);
>>   void qi_flush_iotlb(struct intel_iommu *iommu, u16 did, u64 addr,
>>               unsigned int size_order, u64 type);
>> -void qi_flush_dev_iotlb(struct intel_iommu *iommu, u16 sid, u16 pfsid,
>> -            u16 qdep, u64 addr, unsigned mask);
>> -
>> +void qi_flush_dev_iotlb(struct intel_iommu *iommu,
>> +            struct device_domain_info *info, u64 addr,
>> +            unsigned int mask);
>>   void qi_flush_piotlb(struct intel_iommu *iommu, u16 did, u32 pasid, 
>> u64 addr,
>>                unsigned long npages, bool ih);
>>   -void qi_flush_dev_iotlb_pasid(struct intel_iommu *iommu, u16 sid, 
>> u16 pfsid,
>> -                  u32 pasid, u16 qdep, u64 addr,
>> -                  unsigned int size_order);
>> -void quirk_extra_dev_tlb_flush(struct device_domain_info *info,
>> -                   unsigned long address, unsigned long pages,
>> -                   u32 pasid, u16 qdep);
>> +void qi_flush_dev_iotlb_pasid(struct intel_iommu *iommu,
>> +                  struct device_domain_info *info, u64 addr,
>> +                  u32 pasid, unsigned int size_order);
>> +void quirk_extra_dev_tlb_flush(struct device_domain_info *info, u32 
>> pasid,
>> +                   unsigned long address, unsigned long mask);
>>   void qi_flush_pasid_cache(struct intel_iommu *iommu, u16 did, u64 
>> granu,
>>                 u32 pasid);
>>   diff --git a/drivers/iommu/intel/nested.c 
>> b/drivers/iommu/intel/nested.c
>> index f26c7f1c46cc..d15f72b55940 100644
>> --- a/drivers/iommu/intel/nested.c
>> +++ b/drivers/iommu/intel/nested.c
>> @@ -78,18 +78,13 @@ static void nested_flush_dev_iotlb(struct 
>> dmar_domain *domain, u64 addr,
>>   {
>>       struct device_domain_info *info;
>>       unsigned long flags;
>> -    u16 sid, qdep;
>>         spin_lock_irqsave(&domain->lock, flags);
>>       list_for_each_entry(info, &domain->devices, link) {
>>           if (!info->ats_enabled)
>>               continue;
>> -        sid = info->bus << 8 | info->devfn;
>> -        qdep = info->ats_qdep;
>> -        qi_flush_dev_iotlb(info->iommu, sid, info->pfsid,
>> -                   qdep, addr, mask);
>> -        quirk_extra_dev_tlb_flush(info, addr, mask,
>> -                      IOMMU_NO_PASID, qdep);
>> +        qi_flush_dev_iotlb(info->iommu, info, addr, mask);
>> +        quirk_extra_dev_tlb_flush(info, IOMMU_NO_PASID, addr, mask);
>>       }
>>       spin_unlock_irqrestore(&domain->lock, flags);
>>   }
>> diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
>> index 953592125e4a..5dacdea3cab7 100644
>> --- a/drivers/iommu/intel/pasid.c
>> +++ b/drivers/iommu/intel/pasid.c
>> @@ -208,7 +208,6 @@ devtlb_invalidation_with_pasid(struct intel_iommu 
>> *iommu,
>>                      struct device *dev, u32 pasid)
>>   {
>>       struct device_domain_info *info;
>> -    u16 sid, qdep, pfsid;
>>         info = dev_iommu_priv_get(dev);
>>       if (!info || !info->ats_enabled)
>> @@ -217,10 +216,6 @@ devtlb_invalidation_with_pasid(struct 
>> intel_iommu *iommu,
>>       if (pci_dev_is_disconnected(to_pci_dev(dev)))
>>           return;
>>   -    sid = info->bus << 8 | info->devfn;
>> -    qdep = info->ats_qdep;
>> -    pfsid = info->pfsid;
>> -
>>       /*
>>        * When PASID 0 is used, it indicates RID2PASID(DMA request w/o 
>> PASID),
>>        * devTLB flush w/o PASID should be used. For non-zero PASID under
>> @@ -228,9 +223,9 @@ devtlb_invalidation_with_pasid(struct intel_iommu 
>> *iommu,
>>        * efficient to flush devTLB specific to the PASID.
>>        */
>>       if (pasid == IOMMU_NO_PASID)
>> -        qi_flush_dev_iotlb(iommu, sid, pfsid, qdep, 0, 64 - 
>> VTD_PAGE_SHIFT);
>> +        qi_flush_dev_iotlb(iommu, info, 0, 64 - VTD_PAGE_SHIFT);
>>       else
>> -        qi_flush_dev_iotlb_pasid(iommu, sid, pfsid, pasid, qdep, 0, 
>> 64 - VTD_PAGE_SHIFT);
>> +        qi_flush_dev_iotlb_pasid(iommu, info, 0, pasid, 64 - 
>> VTD_PAGE_SHIFT);
>>   }
>>     void intel_pasid_tear_down_entry(struct intel_iommu *iommu, 
>> struct device *dev,
>> diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
>> index 40edd282903f..89168b31bf31 100644
>> --- a/drivers/iommu/intel/svm.c
>> +++ b/drivers/iommu/intel/svm.c
>> @@ -181,11 +181,10 @@ static void __flush_svm_range_dev(struct 
>> intel_svm *svm,
>>         qi_flush_piotlb(sdev->iommu, sdev->did, svm->pasid, address, 
>> pages, ih);
>>       if (info->ats_enabled) {
>> -        qi_flush_dev_iotlb_pasid(sdev->iommu, sdev->sid, info->pfsid,
>> -                     svm->pasid, sdev->qdep, address,
>> +        qi_flush_dev_iotlb_pasid(sdev->iommu, info, address, 
>> svm->pasid,
>>                        order_base_2(pages));
>> -        quirk_extra_dev_tlb_flush(info, address, order_base_2(pages),
>> -                      svm->pasid, sdev->qdep);
>> +        quirk_extra_dev_tlb_flush(info, svm->pasid, address,
>> +                      order_base_2(pages));
>>       }
>>   }
>>   @@ -227,11 +226,11 @@ static void intel_flush_svm_all(struct 
>> intel_svm *svm)
>>             qi_flush_piotlb(sdev->iommu, sdev->did, svm->pasid, 0, 
>> -1UL, 0);
>>           if (info->ats_enabled) {
>> -            qi_flush_dev_iotlb_pasid(sdev->iommu, sdev->sid, 
>> info->pfsid,
>> -                         svm->pasid, sdev->qdep,
>> -                         0, 64 - VTD_PAGE_SHIFT);
>> -            quirk_extra_dev_tlb_flush(info, 0, 64 - VTD_PAGE_SHIFT,
>> -                          svm->pasid, sdev->qdep);
>> +            qi_flush_dev_iotlb_pasid(sdev->iommu, info, 0,
>> +                         svm->pasid,
>> +                         64 - VTD_PAGE_SHIFT);
>> +            quirk_extra_dev_tlb_flush(info, svm->pasid, 0,
>> +                          64 - VTD_PAGE_SHIFT);
>>           }
>>       }
>>       rcu_read_unlock();
>