Re: [PATCH v2 02/12] iommu: Introduce a replace API for device pasid

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]


On 2024/4/29 21:55, Jason Gunthorpe wrote:
On Fri, Apr 12, 2024 at 01:15:06AM -0700, Yi Liu wrote:

-		if (device == last_gdev)
+		/*
+		 * Rollback the devices/pasid that have attached to the new
+		 * domain. And it is a driver bug to fail attaching with a
+		 * previously good domain.
+		 */
+		if (device == last_gdev) {
+			WARN_ON(old->ops->set_dev_pasid(old, device->dev,
+							pasid, NULL));
-		ops->remove_dev_pasid(device->dev, pasid, domain);

Suggest writing this as

if (WARN_ON(old->ops->set_dev_pasid(old, device->dev, pasid, curr)))
     ops->remove_dev_pasid(device->dev, pasid, domain);

As we may as well try to bring the system back to some kind of safe
state before we continue on.

Also NULL doesn't seem right, if we here then the new domain was
attached successfully and we are put it back to old.

ok, and given your another remark [1], there is no more need to do rollback
for the last_gdev, just need to break the loop when comes to the last_gdev.


+	mutex_lock(&group->mutex);
+	curr = xa_store(&group->pasid_array, pasid, domain, GFP_KERNEL);
+	if (!curr) {
+		xa_erase(&group->pasid_array, pasid);

A comment here about order is likely a good idea..

At this point the pasid_array and the translation are not matched. If
we get a PRI at this instant it will deliver to the new domain until
the translation is updated.


There is nothing to do about this race, but lets note it and say the
concurrent PRI path will eventually become consistent and there is no
harm in directing PRI to the wrong domain.

If the old and new domain points to the same address space, it is fine.
How about they point to different address spaces? Delivering the PRI to
new domain seems problematic. Or, do we allow such domain replacement
when there is still ongoing DMA?

Let's also check that receiving a PRI on a domain that is not PRI
capable doesn't explode in case someone uses replace to change from a
PRI to non PRI domain.

Just need to refuse the receiving PRI, is it? BTW. Should the PRI cap
be disabled in the devices side and the translation structure (e.g.
PRI enable bit in pasid entry) when the replacement is done?

Yi Liu

[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux