Messages in this thread | | | Subject | Re: [RFC 06/20] iommu: Add iommu_device_init[exit]_user_dma interfaces | From | Lu Baolu <> | Date | Wed, 29 Sep 2021 10:38:36 +0800 |
| |
On 9/29/21 10:29 AM, Tian, Kevin wrote: >> From: Lu Baolu <baolu.lu@linux.intel.com> >> Sent: Wednesday, September 29, 2021 10:22 AM >> >> On 9/28/21 10:07 PM, Jason Gunthorpe wrote: >>> On Tue, Sep 28, 2021 at 09:35:05PM +0800, Lu Baolu wrote: >>>> Another issue is, when putting a device into user-dma mode, all devices >>>> belonging to the same iommu group shouldn't be bound with a kernel- >> dma >>>> driver. Kevin's prototype checks this by READ_ONCE(dev->driver). This is >>>> not lock safe as discussed below, >>>> >>>> https://lore.kernel.org/linux- >> iommu/20210927130935.GZ964074@nvidia.com/ >>>> >>>> Any guidance on this? >>> >>> Something like this? >>> >>> >>> int iommu_set_device_dma_owner(struct device *dev, enum >> device_dma_owner mode, >>> struct file *user_owner) >>> { >>> struct iommu_group *group = group_from_dev(dev); >>> >>> spin_lock(&iommu_group->dma_owner_lock); >>> switch (mode) { >>> case DMA_OWNER_KERNEL: >>> if (iommu_group- >>> dma_users[DMA_OWNER_USERSPACE]) >>> return -EBUSY; >>> break; >>> case DMA_OWNER_SHARED: >>> break; >>> case DMA_OWNER_USERSPACE: >>> if (iommu_group- >>> dma_users[DMA_OWNER_KERNEL]) >>> return -EBUSY; >>> if (iommu_group->dma_owner_file != user_owner) { >>> if (iommu_group- >>> dma_users[DMA_OWNER_USERSPACE]) >>> return -EPERM; >>> get_file(user_owner); >>> iommu_group->dma_owner_file = >> user_owner; >>> } >>> break; >>> default: >>> spin_unlock(&iommu_group->dma_owner_lock); >>> return -EINVAL; >>> } >>> iommu_group->dma_users[mode]++; >>> spin_unlock(&iommu_group->dma_owner_lock); >>> return 0; >>> } >>> >>> int iommu_release_device_dma_owner(struct device *dev, >>> enum device_dma_owner mode) >>> { >>> struct iommu_group *group = group_from_dev(dev); >>> >>> spin_lock(&iommu_group->dma_owner_lock); >>> if (WARN_ON(!iommu_group->dma_users[mode])) >>> goto err_unlock; >>> if (!iommu_group->dma_users[mode]--) { >>> if (mode == DMA_OWNER_USERSPACE) { >>> fput(iommu_group->dma_owner_file); >>> iommu_group->dma_owner_file = NULL; >>> } >>> } >>> err_unlock: >>> spin_unlock(&iommu_group->dma_owner_lock); >>> } >>> >>> >>> Where, the driver core does before probe: >>> >>> iommu_set_device_dma_owner(dev, DMA_OWNER_KERNEL, NULL) >>> >>> pci_stub/etc does in their probe func: >>> >>> iommu_set_device_dma_owner(dev, DMA_OWNER_SHARED, NULL) >>> >>> And vfio/iommfd does when a struct vfio_device FD is attached: >>> >>> iommu_set_device_dma_owner(dev, DMA_OWNER_USERSPACE, >> group_file/iommu_file) >> >> Really good design. It also helps alleviating some pains elsewhere in >> the iommu core. >> >> Just a nit comment, we also need DMA_OWNER_NONE which will be set >> when >> the driver core unbinds the driver from the device. >> > > Not necessarily. NONE is represented by none of dma_user[mode] > is valid. >
Fair enough.
Best regards, baolu
| |