> From: Jason Gunthorpe <jgg@xxxxxxxxxx> > Sent: Tuesday, September 28, 2021 10:07 PM > > 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? > > yes, with this group level atomics we don't need loop every dev->driver respectively. > 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) > Just a nit. Per your comment in previous mail: /* If set the driver must call iommu_XX as the first action in probe() */ bool suppress_dma_owner:1; Following above logic userspace drivers won't call iommu_XX in probe(). Just want to double confirm whether you see any issue here with this relaxed behavior. If no problem: /* If set the driver must call iommu_XX as the first action in probe() or * before it attempts to do DMA */ bool suppress_dma_owner:1; Thanks Kevin
