lkml.org 
[lkml]   [2022]   [Jul]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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

\
 
 \ /
  Last update: 2022-07-23 16:12    [W:0.275 / U:0.904 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site