> From: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx> > 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@xxxxxxxxxx/ > >> > >> 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.
