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

[Logan Gunthorpe <logang@deltatee.com>]

On 2019-05-23 2:12 a.m., Koenig, Christian wrote:
>> 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.

No, the P2P code is doing what it was designed to do: peer-to-peer
without going through the root complex. If you DMA map in the presence
of an IOMMU, then you force the TLPs to go through the root complex.
This is not what we want and will not be supported by some RCs so you
effectively drop support for everything but the one entry you've added
in the white list. If we're going to start allowing transactions to go
through an IOMMU then the existing users of P2PDMA needs to be updated
to support this without dropping support for existing use cases.

>> 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.

No, it's absolutely not what dma_map_resource() does. DMA map resource
only adds IOVA addresses in the IOMMU for a physical address (if an
IOMMU is present). It does not determine whether that's appropriate to
do for a given transaction. Such logic would belong in pci_p2pdma_map_*
helpers.

>> 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.

Why not? It's not that complicated. We just need a way for
pci_p2pdma_map_* to determine if the transfer path involves the IOMMU.
If it does, call an existing dma_map_* function, if it does not then do
what it currently does and use the PCI bus address. This probably means
creating some kind of cache with flags for the distance between devices.

Logan