Re: [PATCH] iommu: iommu_group_claim_dma_owner() must always assign a domain

From: Robin Murphy <robin.murphy@arm.com>
To: Jason Gunthorpe <jgg@nvidia.com>
Cc: Kevin Tian <kevin.tian@intel.com>,
	Qian Cai <quic_qiancai@quicinc.com>,
	iommu@lists.linux-foundation.org, Will Deacon <will@kernel.org>
Subject: Re: [PATCH] iommu: iommu_group_claim_dma_owner() must always assign a domain
Date: Wed, 4 May 2022 16:29:24 +0100	[thread overview]
Message-ID: <b3e79abf-646b-fa98-5a4d-26fdf5e550a9@arm.com> (raw)
In-Reply-To: <20220504145454.GI49344@nvidia.com>

On 2022-05-04 15:54, Jason Gunthorpe wrote:
> On Wed, May 04, 2022 at 03:42:09PM +0100, Robin Murphy wrote:
> 
>>> This fixes an oops with VFIO and SMMUv3 because VFIO will call
>>> iommu_detach_group() and then immediately iommu_domain_free(), but
>>> SMMUv3 has no way to know that the domain it is holding a pointer to
>>> has been freed. Now the iommu_detach_group() will assign the blocking
>>> domain and SMMUv3 will no longer hold a stale domain reference.
>>
>> Thanks for taking this on! I do like the overall structure and naming much
>> more than my initial sketch :)
> 
> Thanks, no problem!
>   
>>>    	/*
>>> -	 * If the group has been claimed already, do not re-attach the default
>>> -	 * domain.
>>> +	 * A NULL domain means to call the detach_dev() op. New drivers should
>>> +	 * use a IOMMU_DOMAIN_IDENTITY domain instead of a NULL default_domain
>>
>> Nit: IOMMU_DOMAIN_DMA is the baseline of default domain support, passthrough
>> is more of an optional extra.
> 
> Can you elaborate on this a bit more for the comment, I'm not sure I
> understand all the historical stuff here.

Well, the comment could effectively just be "New drivers should support 
default domains."

What supporting default domains means in practice is two things: that 
.attach_dev handles moving directly between domains without .detach_dev 
being called, and that .domain_alloc supports at least IOMMU_DOMAIN_DMA 
- other unsupported default domain types can fall back to that, but not 
vice versa, see iommu_group_alloc_default_domain().

> Here we are looking at a case where group->domain becomes NULL - what
> does this mean in the historical world? ie what should the iommu
> driver do when detach_dev is called?
> 
> I had guessed it was remove all translation - ie IOMMU_DOMAIN_IDENTITY?

Historically, whatever a NULL domain means is mostly between the IOMMU 
driver and the platform DMA ops - I honestly have no idea what the likes 
of s390 and fsl-pamu do, for example. For SMMUv3 it was always configurable.

Cheers,
Robin.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu