Re: [PATCH v2 3/6] iommu/vt-d: Make intel_iommu_set_dev_pasid() to handle domain replacement

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

On 2024/9/30 15:19, Tian, Kevin wrote:
>> From: Baolu Lu <baolu.lu@linux.intel.com>
>> Sent: Friday, September 13, 2024 10:17 AM
>>
>> On 9/13/24 9:35 AM, Baolu Lu wrote:
>>> On 9/12/24 9:04 PM, Yi Liu wrote:
>>>> +static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t
>>>> pasid,
>>>> +                     struct iommu_domain *domain)
>>>> +{
>>>> +    struct device_domain_info *info = dev_iommu_priv_get(dev);
>>>> +    struct intel_iommu *iommu = info->iommu;
>>>> +
>>>>        intel_pasid_tear_down_entry(iommu, dev, pasid,
>>>>                        INTEL_PASID_TEARDOWN_DRAIN_PRQ);
>>>> +    if (domain->type == IOMMU_DOMAIN_IDENTITY)
>>>> +        return;
>>>
>>> The static identity domain is not capable of handling page requests.
>>> Therefore there is no need to drain PRQ for an identity domain removal.
>>>
>>> So it probably should be something like this:
>>>
>>>       if (domain->type == IOMMU_DOMAIN_IDENTITY) {
>>>           intel_pasid_tear_down_entry(iommu, dev, pasid, 0);
>>>           return;
>>>       }
>>>
>>>       intel_pasid_tear_down_entry(iommu, dev, pasid,
>>>                                       INTEL_PASID_TEARDOWN_DRAIN_PRQ);
>>
>> Just revisited this. It seems that we just need to drain PRQ if the
>> attached domain is iopf-capable. Therefore, how about making it like
>> this?
>>
>> 	unsigned int flags = 0;
>>
>> 	if (domain->iopf_handler)
>> 		flags |= INTEL_PASID_TEARDOWN_DRAIN_PRQ;
>>
>> 	intel_pasid_tear_down_entry(iommu, dev, pasid, flags);
>>
>> 	/* Identity domain has no meta data for pasid. */
>> 	if (domain->type == IOMMU_DOMAIN_IDENTITY)
>> 		return;
>>
> 
> this is the right thing to do, but also suggesting a bug in existing
> code. intel_pasid_tear_down_entry() is not just for PRQ drain.
> It's also about iotlb/devtlb invalidation. From device p.o.v it
> has no idea about the translation mode in the IOMMU side and
> always caches the valid mappings in devtlb when ATS is enabled.

Yes. You are right.

intel_pasid_tear_down_entry() takes care of iotlb/devtlb invalidation.
So it's fine as long as intel_pasid_tear_down_entry() is called for the
IDENTITY domain path, right?

> Existing code skips all those housekeeping for identify domain
> by early return before intel_pasid_tear_down_entry(). We need
> a separate fix for it before this series?

Existing code doesn't skip intel_pasid_tear_down_entry().

         if (domain->type == IOMMU_DOMAIN_IDENTITY) {
                 intel_pasid_tear_down_entry(iommu, dev, pasid, false);
                 return;
         }

Or anything I overlooked?

Thanks,
baolu