Re: [PATCH 01/11] iommu: Add device dma ownership set/release interfaces

On Wed, Nov 17, 2021 at 01:22:19PM +0800, Lu Baolu wrote:

Hi Jason,

On 11/16/21 9:46 PM, Jason Gunthorpe wrote:

quoted

On Tue, Nov 16, 2021 at 09:57:30AM +0800, Lu Baolu wrote:

quoted

Hi Christoph,

On 11/15/21 9:14 PM, Christoph Hellwig wrote:

quoted

On Mon, Nov 15, 2021 at 10:05:42AM +0800, Lu Baolu wrote:

quoted

+enum iommu_dma_owner {
+	DMA_OWNER_NONE,
+	DMA_OWNER_KERNEL,
+	DMA_OWNER_USER,
+};
+

quoted

+	enum iommu_dma_owner dma_owner;
+	refcount_t owner_cnt;
+	struct file *owner_user_file;

I'd just overload the ownership into owner_user_file,

   NULL			-> no owner
   (struct file *)1UL)	-> kernel
   real pointer		-> user

Which could simplify a lot of the code dealing with the owner.

Yeah! Sounds reasonable. I will make this in the next version.

It would be good to figure out how to make iommu_attach_device()
enforce no other driver binding as a kernel user without a file *, as
Robin pointed to, before optimizing this.

This fixes an existing bug where iommu_attach_device() only checks the
group size and is vunerable to a hot plug increasing the group size
after it returns. That check should be replaced by this series's logic
instead.

As my my understanding, the essence of this problem is that only the
user owner of the iommu_group could attach an UNMANAGED domain to it.
If I understand it right, how about introducing a new interface to
allocate a user managed domain and storing the user file pointer in it.

For iommu_attach_device() the semantic is simple non-sharing, so there
is no need for the file * at all, it can just be NULL.

Does above help here?

No, iommu_attach_device() is kernel only and should not interact with
userspace.

I'm also going to see if I can learn what Tegra is doing with
iommu_attach_group()

Jason