Re: [PATCH v2 1/2] iommu: Prevent RESV_DIRECT devices from blocking domains

From: Baolu Lu <baolu.lu@linux.intel.com>
To: "Tian, Kevin" <kevin.tian@intel.com>,
	Jason Gunthorpe <jgg@ziepe.ca>, Joerg Roedel <joro@8bytes.org>,
	Will Deacon <will@kernel.org>,
	Robin Murphy <robin.murphy@arm.com>,
	Alex Williamson <alex.williamson@redhat.com>,
	Nicolin Chen <nicolinc@nvidia.com>
Cc: baolu.lu@linux.intel.com,
	"iommu@lists.linux.dev" <iommu@lists.linux.dev>,
	"kvm@vger.kernel.org" <kvm@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	Jason Gunthorpe <jgg@nvidia.com>
Subject: Re: [PATCH v2 1/2] iommu: Prevent RESV_DIRECT devices from blocking domains
Date: Sat, 22 Jul 2023 21:58:03 +0800	[thread overview]
Message-ID: <46a5bbcd-424a-7d85-bb2a-0d5634166c8c@linux.intel.com> (raw)
In-Reply-To: <BN9PR11MB52768040BD1C88E4EB8001878C3FA@BN9PR11MB5276.namprd11.prod.outlook.com>

On 2023/7/21 11:07, Tian, Kevin wrote:
>> From: Lu Baolu <baolu.lu@linux.intel.com>
>> Sent: Thursday, July 13, 2023 12:33 PM
>>
>> @@ -409,6 +409,7 @@ struct iommu_fault_param {
>>    * @priv:	 IOMMU Driver private data
>>    * @max_pasids:  number of PASIDs this device can consume
>>    * @attach_deferred: the dma domain attachment is deferred
>> + * @requires_direct: The driver requested IOMMU_RESV_DIRECT
> 
> it's not accurate to say "driver requested" as it's a device attribute.
> 
> s/requires_direct/require_direct/
> 
> what about "has_resv_direct"?

How about

  * @require_direct: device requires IOMMU_RESV_DIRECT reserved regions

?

> 
>> @@ -959,14 +959,12 @@ static int
>> iommu_create_device_direct_mappings(struct iommu_domain *domain,
>>   	unsigned long pg_size;
>>   	int ret = 0;
>>
>> -	if (!iommu_is_dma_domain(domain))
>> -		return 0;
>> -
>> -	BUG_ON(!domain->pgsize_bitmap);
>> -
>> -	pg_size = 1UL << __ffs(domain->pgsize_bitmap);
>> +	pg_size = domain->pgsize_bitmap ? 1UL << __ffs(domain-
>>> pgsize_bitmap) : 0;
>>   	INIT_LIST_HEAD(&mappings);
>>
>> +	if (WARN_ON_ONCE(iommu_is_dma_domain(domain) && !pg_size))
>> +		return -EINVAL;
>> +
>>   	iommu_get_resv_regions(dev, &mappings);
>>
>>   	/* We need to consider overlapping regions for different devices */
>> @@ -974,13 +972,17 @@ static int
>> iommu_create_device_direct_mappings(struct iommu_domain *domain,
>>   		dma_addr_t start, end, addr;
>>   		size_t map_size = 0;
>>
>> +		if (entry->type == IOMMU_RESV_DIRECT)
>> +			dev->iommu->requires_direct = 1;
>> +
>> +		if ((entry->type != IOMMU_RESV_DIRECT &&
>> +		     entry->type != IOMMU_RESV_DIRECT_RELAXABLE) ||
>> +		    !iommu_is_dma_domain(domain))
>> +			continue;
>> +
>>   		start = ALIGN(entry->start, pg_size);
>>   		end   = ALIGN(entry->start + entry->length, pg_size);
>>
>> -		if (entry->type != IOMMU_RESV_DIRECT &&
>> -		    entry->type != IOMMU_RESV_DIRECT_RELAXABLE)
>> -			continue;
>> -
>>   		for (addr = start; addr <= end; addr += pg_size) {
>>   			phys_addr_t phys_addr;
>>
> 
> piggybacking a device attribute detection in a function which tries to
> populate domain mappings is a bit confusing.
> 
> Does it work better to introduce a new function to detect this attribute
> and has it directly called in the probe path?

Jason answered this.

> 
>> @@ -2121,6 +2123,21 @@ static int __iommu_device_set_domain(struct
>> iommu_group *group,
>>   {
>>   	int ret;
>>
>> +	/*
>> +	 * If the driver has requested IOMMU_RESV_DIRECT then we cannot
> 
> ditto. It's not requested by the driver.
> 
>> allow
>> +	 * the blocking domain to be attached as it does not contain the
>> +	 * required 1:1 mapping. This test effectively exclusive the device
> 
> s/exclusive/excludes/
> 

Updated. Thanks!

Best regards,
baolu