Re: [PATCH 1/4] iommu/vt-d: Implement set device pasid op for default domain

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

On 3/3/23 11:02 AM, Tian, Kevin wrote:
>> From: Baolu Lu <baolu.lu@linux.intel.com>
>> Sent: Friday, March 3, 2023 10:49 AM
>>
>> On 3/3/23 10:36 AM, Tian, Kevin wrote:
>>>> From: Baolu Lu <baolu.lu@linux.intel.com>
>>>> Sent: Thursday, March 2, 2023 10:07 PM
>>>>> +
>>>>> +	if (!sm_supported(iommu) || !info)
>>>>
>>>> @info has been referenced. !info check makes no sense.
>>>>
>>>> Add pasid_supported(iommu).
>>>>
>>>> Do you need to check whether the domain is compatible for this rid
>>>> pasid?
>>>
>>> what kind of compatibility is concerned here? In concept a pasid
>>> can be attached to any domain if it has been successfully attached
>>> to rid. Probably we can add a check here that RID2PASID must
>>> point to the domain already.
>>
>> "...if it has been successfully attached to rid..."
>>
>> We should not have this assumption in iommu driver's callback. The iommu
>> driver has no (and should not have) knowledge about the history of any
>> domain.
> 
> but this is an op for default domain which must have been attached
> to RID2PASID and any compatibility check between this domain and device
> should be passed.

This is an op for DMA, DMA-FQ and UNMANAGED domain. The IOMMU driver
doesn't need to interpret the default domain concept. :-)

> 
> We can have another set_pasid for unmanaged which then need similar
> check as prepare_domain_attach_device() does.

 From the perspective of the iommu driver, there's no essential
difference between DMA and UNMANAGED domains. So almost all IOMMU
drivers maintain a single set of domain ops for them.

>>
>>>
>>>>
>>>>> +		return -ENODEV;
>>>>> +
>>>>> +	if (WARN_ON(pasid == PASID_RID2PASID))
>>>>> +		return -EINVAL;
>>>>
>>>> Add a call to domain_attach_iommu() here to get a refcount of the
>> domain
>>>> ID. And call domain_detach_iommu() in
>> intel_iommu_remove_dev_pasid().
>>>>
>>>
>>> Is it necessary? iommu core doesn't allow taking ownership
>>> if !xa_empty(&group->pasid_array) so if this pasid attach succeeds
>>> this device cannot be attached to another domain before pasid
>>> detach is done on the current domain.
>>
>> It's not about the pasid, but the domain id.
>>
>> This domain's id will be set to a field of the device's pasid entry. It
>> must get a refcount of that domain id to avoid use after free.
>>
> 
> If the domain still has attached device (due to this pasid usage) how could
> domain id be freed?

The Intel IOMMU driver uses a user counter to determine when the domain
id could be freed.

Best regards,
baolu