Re: [PATCH v10 04/12] iommu: Add attach/detach_dev_pasid iommu interface

[Jason Gunthorpe <jgg@nvidia.com>]

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