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

From: Baolu Lu
Date: Thu Mar 02 2023 - 23:39:28 EST


On 3/3/23 11:02 AM, Tian, Kevin wrote:
From: Baolu Lu <baolu.lu@xxxxxxxxxxxxxxx>
Sent: Friday, March 3, 2023 10:49 AM

On 3/3/23 10:36 AM, Tian, Kevin wrote:
From: Baolu Lu <baolu.lu@xxxxxxxxxxxxxxx>
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