Messages in this thread | | | Date | Sat, 23 Jul 2022 11:11:18 -0300 | From | Jason Gunthorpe <> | Subject | Re: [PATCH v10 04/12] iommu: Add attach/detach_dev_pasid iommu interface |
| |
On Tue, Jul 05, 2022 at 01:07:02PM +0800, Lu Baolu wrote: > Attaching an IOMMU domain to a PASID of a device is a generic operation > for modern IOMMU drivers which support PASID-granular DMA address > translation. Currently visible usage scenarios include (but not limited): > > - SVA (Shared Virtual Address) > - kernel DMA with PASID > - hardware-assist mediated device > > This adds a pair of domain ops for this purpose and adds the interfaces > for device drivers to attach/detach a domain to/from a {device, PASID}. > Some buses, like PCI, route packets without considering the PASID > value.
Below the comments touch on ACS, so this is a bit out of date
> +static bool iommu_group_immutable_singleton(struct iommu_group *group, > + struct device *dev) > +{ > + int count; > + > + mutex_lock(&group->mutex); > + count = iommu_group_device_count(group); > + mutex_unlock(&group->mutex); > + > + if (count != 1) > + return false; > + > + /* > + * The PCI device could be considered to be fully isolated if all > + * devices on the path from the device to the host-PCI bridge are > + * protected from peer-to-peer DMA by ACS. > + */ > + if (dev_is_pci(dev)) > + return pci_acs_path_enabled(to_pci_dev(dev), NULL, > + REQ_ACS_FLAGS);
You might want to explain what condition causes ACS isolated devices to share a group in the first place..
> + > + /* > + * Otherwise, the device came from DT/ACPI, assume it is static and > + * then singleton can know from the device count in the group. > + */ > + return true; > +}
I would be happer if probe was changed to refuse to add a device to a group if the group's pasid xarray is not empty, as a protective measure.
> +int iommu_attach_device_pasid(struct iommu_domain *domain, struct device *dev, > + ioasid_t pasid) > +{ > + struct iommu_group *group; > + void *curr; > + int ret; > + > + if (!domain->ops->set_dev_pasid) > + return -EOPNOTSUPP; > + > + group = iommu_group_get(dev); > + if (!group || !iommu_group_immutable_singleton(group, dev)) { > + iommu_group_put(group); > + return -EINVAL;
goto error below
> + } > + > + mutex_lock(&group->mutex);
Just hold the group->mutex a few lines above and don't put locking in iommu_group_immutable_singleton(), it is clearer
> +void iommu_detach_device_pasid(struct iommu_domain *domain, struct device *dev, > + ioasid_t pasid) > +{ > + struct iommu_group *group = iommu_group_get(dev); > + > + mutex_lock(&group->mutex); > + domain->ops->block_dev_pasid(domain, dev, pasid);
I still really this OP, it is nonsense to invoke 'block_dev_pasid' on a domain, it should be on the iommu ops and it should not take in a domain parameter. This is why I prefer we write it as
domain->ops->set_dev_pasid(group->blocking_domain, dev, pasid);
> + xa_erase(&group->pasid_array, pasid);
It is worth checking that the value returned from xa_erase is domain and WARN_ON if not, since we are passing domain in..
Jason
| |