Re: [PATCH] PCI/P2PDMA: Root complex whitelist should not apply when an IOMMU is present

From: "Koenig, Christian" <Christian.Koenig@amd.com>
To: Logan Gunthorpe <logang@deltatee.com>, Christoph Hellwig <hch@lst.de>
Cc: "linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
	Bjorn Helgaas <bhelgaas@google.com>
Subject: Re: [PATCH] PCI/P2PDMA: Root complex whitelist should not apply when an IOMMU is present
Date: Thu, 23 May 2019 08:12:18 +0000	[thread overview]
Message-ID: <b9e94126-8686-4306-77c3-bd0b96680775@amd.com> (raw)
In-Reply-To: <97aa52fc-f062-acf1-0e0c-5a4d1d505777@deltatee.com>

Am 22.05.19 um 22:41 schrieb Logan Gunthorpe:
> [CAUTION: External Email]
>
> On 2019-05-22 2:30 p.m., Koenig, Christian wrote:
>> Hi Logan,
>>
>> Am 22.05.2019 22:12 schrieb Logan Gunthorpe <logang@deltatee.com>:
>>
>>     [CAUTION: External Email]
>>
>>     Presently, there is no path to DMA map P2PDMA memory, so if a TLP
>>     targeting this memory hits the root complex and an IOMMU is present,
>>     the IOMMU will reject the transaction, even if the RC would support
>>     P2PDMA.
>>
>>     So until the kernel knows to map these DMA addresses in the IOMMU,
>>     we should not enable the whitelist when an IOMMU is present.
>>
>>
>> Well NAK, cause that is exactly what we are doing.
>
> Are you DMA-mapping the addresses outside the P2PDMA code? If so there's
> a huge mismatch with the existing users of P2PDMA (nvme-fabrics). If
> you're not dma-mapping then I can't see how it could work because the
> IOMMU should reject any requests to access those addresses.

Well, we are using the DMA API (dma_map_resource) for this. If the P2P 
code is not using this then I would rather say that the P2P code is 
actually broken.

Adding Christoph as well, cause he is usually the one discussion stuff 
like that with me.

> By adding the whitelist in this way you will break any user that
> attempts to use P2P in nvme-fabrics on whitelisted RCs with an IOMMU
> enabled.
>
> Currently, the users of P2PDMA use pci_p2pdma_map_sg() which only
> returns the PCI bus address. If P2PDMA transactions can now go through
> an IOMMU, then this is wrong and broken.
>
> We need to ensure that all users of P2PDMA map this memory in the same
> way. Which means, if the TLPs will go through an IOMMU they get
> dma-map'd and, if they don't, they use the PCI Bus address (as the
> current code does).

Well that is exactly what dma_map_resource() already does, so we should 
probably just make using the DMA API mandatory.

> Without the change proposed in this patch, I have to retract my review
> and NAK your patch until we can sort out the mapping issues.

Yeah, completely agree we can't do that right now.

Regards,
Christian.

>
>
> Logan