Re: [RFC 14/20] iommu/iommufd: Add iommufd_device_[de]attach_ioasid()

Jason Gunthorpe via iommu
2021-09-21T18:02:22Z

On Sun, Sep 19, 2021 at 02:38:42PM +0800, Liu Yi L wrote:
> An I/O address space takes effect in the iommu only after it's attached
> by a device. This patch provides iommufd_device_[de/at]tach_ioasid()
> helpers for this purpose. One device can be only attached to one ioasid
> at this point, but one ioasid can be attached by multiple devices.
>
> The caller specifies the iommufd_device (returned at binding time) and
> the target ioasid when calling the helper function. Upon request, iommufd
> installs the specified I/O page table to the correct place in the IOMMU,
> according to the routing information (struct device* which represents
> RID) recorded in iommufd_device. Future variants could allow the caller
> to specify additional routing information (e.g. pasid/ssid) when multiple
> I/O address spaces are supported per device.
>
> Open:
> Per Jason's comment in below link, bus-specific wrappers are recommended.
> This RFC implements one wrapper for pci device. But it looks that struct
> pci_device is not used at all since iommufd_ device already carries all
> necessary info. So want to have another discussion on its necessity, e.g.
> whether making more sense to have bus-specific wrappers for binding, while
> leaving a common attaching helper per iommufd_device.
> https://lore.kernel.org/linux-iommu/20210528233649.GB3816344@nvidia.com/
>
> TODO:
> When multiple devices are attached to a same ioasid, the permitted iova
> ranges and supported pgsize bitmap on this ioasid should be a common
> subset of all attached devices. iommufd needs to track such info per
> ioasid and update it every time when a new device is attached to the
> ioasid. This has not been done in this version yet, due to the temporary
> hack adopted in patch 16-18. The hack reuses vfio type1 driver which
> already includes the necessary logic for iova ranges and pgsize bitmap.
> Once we get a clear direction for those patches, that logic will be moved
> to this patch.
>
> Signed-off-by: Liu Yi L <yi.l.liu@intel.com>
> drivers/iommu/iommufd/iommufd.c | 226 ++++++++++++++++++++++++++++++++
> include/linux/iommufd.h | 29 ++++
> 2 files changed, 255 insertions(+)
>
> diff --git a/drivers/iommu/iommufd/iommufd.c b/drivers/iommu/iommufd/iommufd.c
> index e45d76359e34..25373a0e037a 100644
> +++ b/drivers/iommu/iommufd/iommufd.c
> @@ -51,6 +51,19 @@ struct iommufd_ioas {
> bool enforce_snoop;
> struct iommufd_ctx *ictx;
> refcount_t refs;
> + struct mutex lock;
> + struct list_head device_list;
> + struct iommu_domain *domain;

This should just be another xarray indexed by the device id

> +/* Caller should hold ioas->lock */
> +static struct ioas_device_info *ioas_find_device(struct iommufd_ioas *ioas,
> + struct iommufd_device *idev)
> +{
> + struct ioas_device_info *ioas_dev;
> +
> + list_for_each_entry(ioas_dev, &ioas->device_list, next) {
> + if (ioas_dev->idev == idev)
> + return ioas_dev;
> + }

Which eliminates this search. xarray with tightly packed indexes is
generally more efficient than linked lists..

> +static int ioas_check_device_compatibility(struct iommufd_ioas *ioas,
> + struct device *dev)
> +{
> + bool snoop = false;
> + u32 addr_width;
> + int ret;
> +
> + /*
> + * currently we only support I/O page table with iommu enforce-snoop
> + * format. Attaching a device which doesn't support this format in its
> + * upstreaming iommu is rejected.
> + */
> + ret = iommu_device_get_info(dev, IOMMU_DEV_INFO_FORCE_SNOOP, &snoop);
> + if (ret || !snoop)
> + return -EINVAL;
> +
> + ret = iommu_device_get_info(dev, IOMMU_DEV_INFO_ADDR_WIDTH, &addr_width);
> + if (ret || addr_width < ioas->addr_width)
> + return -EINVAL;
> +
> + /* TODO: also need to check permitted iova ranges and pgsize bitmap */
> +
> + return 0;
> +}

This seems kind of weird..

I expect the iommufd to hold a SW copy of the IO page table and each
time a new domain is to be created it should push the SW copy into the
domain. If the domain cannot support it then the domain driver should
naturally fail a request.

When the user changes the IO page table the SW copy is updated then
all of the domains are updated too - again if any domain cannot
support the change then it fails and the change is rolled back.

It seems like this is a side effect of roughly hacking in the vfio
code?

> +
> +/**
> + * iommufd_device_attach_ioasid - attach device to an ioasid
> + * @idev: [in] Pointer to struct iommufd_device.
> + * @ioasid: [in] ioasid points to an I/O address space.
> + *
> + * Returns 0 for successful attach, otherwise returns error.
> + *
> + */
> +int iommufd_device_attach_ioasid(struct iommufd_device *idev, int ioasid)

Types for the ioas_id again..

> +{
> + struct iommufd_ioas *ioas;
> + struct ioas_device_info *ioas_dev;
> + struct iommu_domain *domain;
> + int ret;
> +
> + ioas = ioasid_get_ioas(idev->ictx, ioasid);
> + if (!ioas) {
> + pr_err_ratelimited("Trying to attach illegal or unkonwn IOASID %u\n", ioasid);
> + return -EINVAL;

No prints triggered by bad userspace

> + }
> +
> + mutex_lock(&ioas->lock);
> +
> + /* Check for duplicates */
> + if (ioas_find_device(ioas, idev)) {
> + ret = -EINVAL;
> + goto out_unlock;
> + }

just xa_cmpxchg NULL, XA_ZERO_ENTRY

> + /*
> + * Each ioas is backed by an iommu domain, which is allocated
> + * when the ioas is attached for the first time and then shared
> + * by following devices.
> + */
> + if (list_empty(&ioas->device_list)) {

Seems strange, what if the devices are forced to have different
domains? We don't want to model that in the SW layer..

> + /* Install the I/O page table to the iommu for this device */
> + ret = iommu_attach_device(domain, idev->dev);
> + if (ret)
> + goto out_domain;

This is where things start to get confusing when you talk about PASID
as the above call needs to be some PASID centric API.

> @@ -27,6 +28,16 @@ struct iommufd_device *
> iommufd_bind_device(int fd, struct device *dev, u64 dev_cookie);
> void iommufd_unbind_device(struct iommufd_device *idev);
>
> +int iommufd_device_attach_ioasid(struct iommufd_device *idev, int ioasid);
> +void iommufd_device_detach_ioasid(struct iommufd_device *idev, int ioasid);
> +
> +static inline int
> +__pci_iommufd_device_attach_ioasid(struct pci_dev *pdev,
> + struct iommufd_device *idev, int ioasid)
> +{
> + return iommufd_device_attach_ioasid(idev, ioasid);
> +}

If think sis taking in the iommfd_device then there isn't a logical
place to signal the PCIness

But, I think the API should at least have a kdoc that this is
capturing the entire device and specify that for PCI this means all
TLPs with the RID.

Jason