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

From: Robin Murphy <robin.murphy@arm.com>
To: "Tian, Kevin" <kevin.tian@intel.com>, Jason Gunthorpe <jgg@nvidia.com>
Cc: Will Deacon <will@kernel.org>,
	"iommu@lists.linux-foundation.org"
	<iommu@lists.linux-foundation.org>,
	"Rodel, Jorg" <jroedel@suse.de>,
	Qian Cai <quic_qiancai@quicinc.com>
Subject: Re: [PATCH v2] iommu: iommu_group_claim_dma_owner() must always assign a domain
Date: Fri, 6 May 2022 10:32:50 +0100	[thread overview]
Message-ID: <9f6680ed-77b6-8440-078c-623406c823aa@arm.com> (raw)
In-Reply-To: <BN9PR11MB5276476BB203D8D61CA8849C8CC29@BN9PR11MB5276.namprd11.prod.outlook.com>

On 2022-05-06 00:28, Tian, Kevin wrote:
>> From: Jason Gunthorpe
>> Sent: Thursday, May 5, 2022 11:33 PM
>>>>   	/*
>>>> -	 * If the group has been claimed already, do not re-attach the default
>>>> -	 * domain.
>>>> +	 * New drivers should support default domains and so the
>>>> detach_dev() op
>>>> +	 * will never be called. Otherwise the NULL domain indicates the
>>>> +	 * translation for the group should be set so it will work with the
>>>
>>> translation should be 'blocked'?
>>
>> No, not blocked.
>>
>>>> +	 * platform DMA ops.
>>>
>>> I didn't get the meaning of the last sentence.
>>
>> It is as discussed with Robin, NULL means to return the group back to
>> some platform defined translation, and in some cases this means
>> returning back to work with the platform's DMA ops - presumably if it
>> isn't using the dma api.
> 
> This is clearer than the original text.

Perhaps we could just leave that sentence as "Otherwise the NULL domain 
represents platform-specific behaviour."

>>
>>>> +	/*
>>>> +	 * Changing the domain is done by calling attach on the new domain.
>>>> +	 * Drivers should implement this so that DMA is always translated by
>>>
>>> what does 'this' mean?
>>
>> The code below - attach_dev called in a loop for each dev in the group.
> 
> Yes.
> 
>>
>>>> +	 * either the new, old, or a blocking domain. DMA should never
>>>
>>> isn't the blocking domain passed in as the new domain?
>>
>> Soemtimes. This is a statement about the required
>> atomicity. New/old/block are all valid translations during the
>> operation. Identity is not.
> 
> but new/old/block are not the same type of classifications. A group
> has an old domain and a new domain at this transition point, and
> both old/new domains have a domain type (dma, unmanged, block,
> identity, etc.). Mixing them together only increases confusion here.

Good point - in particular I think the "DMA is always translated" part 
would be more accurately said as "DMA is always managed". When we're 
reattaching back to the default domain here, and it happens to be an 
identity domain, then DMA *may* be untranslated, but in a manner that 
we've knowingly chosen. The key point is that if the driver supports 
core domains, then it should never have any in-between state that allows 
access to anything *other* than the current domain or the new domain.

Thanks,
Robin.

> 
>>
>> So, I'm going to leave this patch as-is since it has been tested now
>> and we need to get the series back into iommu-next ASAP.
>>
> 
> Agree. Just hope some improvements on the code comment.
> 
> But either way given no more code change:
> 
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> _______________________________________________
> iommu mailing list
> iommu@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu