Re: [PATCH] iommu/dma: Reserve iova ranges for reserved regions of all devices

[Robin Murphy <robin.murphy@arm.com>]

On 2024-11-06 7:13 pm, Jerry Snitselaar wrote:
> On Thu, Oct 24, 2024 at 08:34:12AM -0700, Jerry Snitselaar wrote:
>> Only the first device that is passed when the domain is set up will
>> have its reserved regions reserved in the iova address space.  So if
>> there are other devices in the group with unique reserved regions,
>> those regions will not get reserved in the iova address space.  All of
>> the ranges do get set up in the iopagetables via calls to
>> iommu_create_device_direct_mappings for all devices in a group.
>>
>> In the case of vt-d system this resulted in messages like the following:
>>
>> [ 1632.693264] DMAR: ERROR: DMA PTE for vPFN 0xf1f7e already set (to f1f7e003 not 173025001)
>>
>> To make sure iova ranges are reserved for the reserved regions all of
>> the devices, call iova_reserve_iommu_regions in iommu_dma_init_domain
>> prior to exiting in the case where the domain is already initialized.
>>
>> Cc: Robin Murphy <robin.murphy@arm.com>
>> Cc: Joerg Roedel <joro@8bytes.org>
>> Cc: Will Deacon <will@kernel.org>
>> Cc: linux-kernel@vger.kernel.org
>> Cc: stable@vger.kernel.org
>> Fixes: 7c1b058c8b5a ("iommu/dma: Handle IOMMU API reserved regions")
>> Signed-off-by: Jerry Snitselaar <jsnitsel@redhat.com>
>> ---
>> Robin: I wasn't positive if this is the correct solution, or if it should be
>>         done for the entire group at once.
>>
>>   drivers/iommu/dma-iommu.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
>> index 2a9fa0c8cc00..5fd3cccbb233 100644
>> --- a/drivers/iommu/dma-iommu.c
>> +++ b/drivers/iommu/dma-iommu.c
>> @@ -707,7 +707,7 @@ static int iommu_dma_init_domain(struct iommu_domain *domain, struct device *dev
>>   			goto done_unlock;
>>   		}
>>   
>> -		ret = 0;
>> +		ret = iova_reserve_iommu_regions(dev, domain);
>>   		goto done_unlock;
>>   	}
>>   
>> -- 
>> 2.44.0
>>
> 
> Robin,
> 
> Any thoughts on this patch? In the case where this originally popped
> up it was likely a crap DMAR table in an HPE system with an ilo, as
> the RMRR in question had a device in the list that as far as I could
> tell didn't actually exist. The 2nd function of the sata controller
> was in the list, but not the first, and the first function was the
> device where the group/domain was initialized. With some debugging
> code I could see it set up the ioptes for the 2nd function, but it
> wasn't reserving the range of iovas.

Yeah, this one's tricky - the current behaviour is not entirely 
unintentional since there's not really a right answer. It's also 
possible for the second device to get here after the first device has 
already started using the domain, so at that point it's no longer safe 
to use reserve_iova() due to how it merges overlapping and adjacent 
rbtree nodes, which could really screw things up once there are normal 
allocations in the tree as well. So in truth it was rather a case of 
crossing my fingers and quietly hoping this particular set of 
circumstances was unlikely enough to never come up... :/

Thanks,
Robin.