Re: [PATCH 11/11] iommu/vt-d: Remove superfluous IOMMU IOTLB invalidations

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

On 2024/2/16 13:38, Zhang, Tina wrote:
>>> diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
>>> index 79d1f3107847..bf4962eb229c 100644
>>> --- a/drivers/iommu/intel/svm.c
>>> +++ b/drivers/iommu/intel/svm.c
>>> @@ -134,31 +134,39 @@ void intel_svm_check(struct intel_iommu *iommu)
>>>    	iommu->flags |= VTD_FLAG_SVM_CAPABLE;
>>>    }
>>>
>>> -static void __flush_svm_range_dev(struct dmar_domain *domain,
>>> -				  struct dev_pasid_info *dev_pasid,
>>> +static void __flush_svm_range(struct iommu_domain *domain,
>>>    				  unsigned long address,
>>>    				  unsigned long pages, int ih)
>>>    {
>>> -	struct device_domain_info *info = dev_iommu_priv_get(dev_pasid-
>>> dev);
>>> -	u32 pasid = mm_get_enqcmd_pasid(domain->domain.mm);
>>> +	u32 pasid = mm_get_enqcmd_pasid(domain->mm);
>>> +	struct device_domain_info *dev_info;
>>> +	struct iommu_domain_info *iommu_info;
>>> +	struct dev_pasid_info *dev_pasid;
>>> +	unsigned long idx;
>>>
>>>    	if (WARN_ON(!pages))
>>>    		return;
>>>
>>> -	qi_flush_piotlb(info->iommu, dev_pasid->did, pasid, address, pages, ih);
>>> -	if (info->ats_enabled) {
>>> -		qi_flush_dev_iotlb_pasid(info->iommu, dev_pasid->sid, info-
>>> pfsid,
>>> -					 pasid, dev_pasid->qdep, address,
>>> -					 order_base_2(pages));
>>> -		quirk_extra_dev_tlb_flush(info, address, order_base_2(pages),
>>> -					  pasid, dev_pasid->qdep);
>>> +	rcu_read_lock();
>>> +	xa_for_each(&to_dmar_domain(domain)->iommu_array, idx,
>> iommu_info)
>>> +		qi_flush_piotlb(iommu_info->iommu, dev_pasid->did,
>>> +				pasid, address, pages, ih);
>> The xa_array for iommu is already broken. We should fix it before further use.
>>
>> https://lore.kernel.org/linux-iommu/20240103124403.GM50406@nvidia.com/
> Agree that domain->iommu_array needs to protect the element (struct iommu_domain_info) from UAF as well. This could be another topic.
> 
> I realize that we don't need to use domain->iommu_array here. What we want here is a pointer pointing to a struct intel_iommu. We can get the info from device using dev_iommu_priv_get(), instead of getting the info from iommu_array.
> 
> Besides, I'm thinking about sorting the domain->dev_pasids in intel_iommu_set_dev_pasid() to make struct dev_pasid_info of the devices behind the same IOMMU stay together. It can be helpful to reduce the redundant IOMMU IOTLB invalidations.

I think what you want here are the {domain_id, pasid} pairs to flush the
IOTLB cache in the IOMMU, right? Perhaps we could refine this code
further by introducing a structure of cache tag. The cache tags can be
stored in the domain's IOMMU array. I'm working on a solution and I can
post it for discussion later.

Best regards,
baolu