[2/2] iommu/dma: Avoid SAC address trick for PCIe devices
diff mbox series

Message ID d412c292d222eb36469effd338e985f9d9e24cd6.1594207679.git.robin.murphy@arm.com
State Not Applicable
Headers show
Series
  • [1/2] iommu/intel: Avoid SAC address trick for PCIe devices
Related show

Commit Message

Robin Murphy July 8, 2020, 11:32 a.m. UTC
As for the intel-iommu implementation, relegate the opportunistic
attempt to allocate a SAC address to the domain of conventional PCI
devices only, to avoid it increasingly causing far more performance
issues than possible benefits on modern PCI Express systems.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/iommu/dma-iommu.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Joerg Roedel July 13, 2020, 1:14 p.m. UTC | #1
On Wed, Jul 08, 2020 at 12:32:42PM +0100, Robin Murphy wrote:
> As for the intel-iommu implementation, relegate the opportunistic
> attempt to allocate a SAC address to the domain of conventional PCI
> devices only, to avoid it increasingly causing far more performance
> issues than possible benefits on modern PCI Express systems.
> 
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
>  drivers/iommu/dma-iommu.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> index 4959f5df21bd..0ff124f16ad4 100644
> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c
> @@ -426,7 +426,8 @@ static dma_addr_t iommu_dma_alloc_iova(struct iommu_domain *domain,
>  		dma_limit = min(dma_limit, (u64)domain->geometry.aperture_end);
>  
>  	/* Try to get PCI devices a SAC address */
> -	if (dma_limit > DMA_BIT_MASK(32) && dev_is_pci(dev))
> +	if (dma_limit > DMA_BIT_MASK(32) &&
> +	    dev_is_pci(dev) && !pci_is_pcie(to_pci_dev(dev)))
>  		iova = alloc_iova_fast(iovad, iova_len,
>  				       DMA_BIT_MASK(32) >> shift, false);
>  

Unfortunatly this patch causes XHCI initialization failures on my AMD
Ryzen system. I will remove both from the IOMMU tree for now.

I guess the XHCI chip in my system does not support full 64bit dma
addresses and needs a quirk or something like that. But until this is
resolved its better to not change the IOVA allocation behavior.

Regards,

	Joerg
Robin Murphy July 14, 2020, 11:42 a.m. UTC | #2
On 2020-07-13 14:14, Joerg Roedel wrote:
> On Wed, Jul 08, 2020 at 12:32:42PM +0100, Robin Murphy wrote:
>> As for the intel-iommu implementation, relegate the opportunistic
>> attempt to allocate a SAC address to the domain of conventional PCI
>> devices only, to avoid it increasingly causing far more performance
>> issues than possible benefits on modern PCI Express systems.
>>
>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>> ---
>>   drivers/iommu/dma-iommu.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
>> index 4959f5df21bd..0ff124f16ad4 100644
>> --- a/drivers/iommu/dma-iommu.c
>> +++ b/drivers/iommu/dma-iommu.c
>> @@ -426,7 +426,8 @@ static dma_addr_t iommu_dma_alloc_iova(struct iommu_domain *domain,
>>   		dma_limit = min(dma_limit, (u64)domain->geometry.aperture_end);
>>   
>>   	/* Try to get PCI devices a SAC address */
>> -	if (dma_limit > DMA_BIT_MASK(32) && dev_is_pci(dev))
>> +	if (dma_limit > DMA_BIT_MASK(32) &&
>> +	    dev_is_pci(dev) && !pci_is_pcie(to_pci_dev(dev)))
>>   		iova = alloc_iova_fast(iovad, iova_len,
>>   				       DMA_BIT_MASK(32) >> shift, false);
>>   
> 
> Unfortunatly this patch causes XHCI initialization failures on my AMD
> Ryzen system. I will remove both from the IOMMU tree for now.
> 
> I guess the XHCI chip in my system does not support full 64bit dma
> addresses and needs a quirk or something like that. But until this is
> resolved its better to not change the IOVA allocation behavior.

Oh bother - yes, this could have been masking all manner of bugs. That 
system will presumably also break if you managed to exhaust the 32-bit 
IOVA space such that the allocator moved up to the higher range anyway, 
or if you passed the XHCI through to a VM with a sufficiently wacky GPA 
layout, but I guess those are cases that simply nobody's run into yet.

Does the firmware actually report any upper address constraint such that 
Sebastian's IVRS aperture patches might help?

Robin.
Joerg Roedel July 22, 2020, 12:48 p.m. UTC | #3
On Tue, Jul 14, 2020 at 12:42:36PM +0100, Robin Murphy wrote:
> Oh bother - yes, this could have been masking all manner of bugs. That
> system will presumably also break if you managed to exhaust the 32-bit IOVA
> space such that the allocator moved up to the higher range anyway, or if you
> passed the XHCI through to a VM with a sufficiently wacky GPA layout, but I
> guess those are cases that simply nobody's run into yet.
> 
> Does the firmware actually report any upper address constraint such that
> Sebastian's IVRS aperture patches might help?

No, it doesn't. I am not sure what the best way is to get these issues
found and fixed. I doubt they will be getting fixed when the allocation
pattern isn't changed, maybe we can put your changes behind a config
variable and start testing/reporting bugs/etc.

Regards,

	Joerg

Patch
diff mbox series

diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 4959f5df21bd..0ff124f16ad4 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -426,7 +426,8 @@  static dma_addr_t iommu_dma_alloc_iova(struct iommu_domain *domain,
 		dma_limit = min(dma_limit, (u64)domain->geometry.aperture_end);
 
 	/* Try to get PCI devices a SAC address */
-	if (dma_limit > DMA_BIT_MASK(32) && dev_is_pci(dev))
+	if (dma_limit > DMA_BIT_MASK(32) &&
+	    dev_is_pci(dev) && !pci_is_pcie(to_pci_dev(dev)))
 		iova = alloc_iova_fast(iovad, iova_len,
 				       DMA_BIT_MASK(32) >> shift, false);