2019-07-23 10:31:34

by Logan Gunthorpe

[permalink] [raw]
Subject: [PATCH 00/14] PCI/P2PDMA: Support transactions that hit the host bridge

As discussed on the list previously, in order to fully support the
whitelist Christian added with the IOMMU, we must ensure that we
map any buffer going through the IOMMU with an aprropriate dma_map
call. This patchset accomplishes this by cleaning up the output of
upstream_bridge_distance() to better indicate the mapping requirements,
caching these requirements in an xarray, then looking them up at map
time and applying the appropriate mapping method.

After this patchset, it's possible to use the NVMe-of P2P support to
transfer between devices without a switch on the whitelisted root
complexes. A couple Intel device I have tested this on have also
been added to the white list.

Most of the changes are contained within the p2pdma.c, but there are
a few minor touches to other subsystems, mostly to add support
to call an unmap function.

The final patch in this series demonstrates a possible
pci_p2pdma_map_resource() function that I expect Christian will need
but does not have any users at this time so I don't intend for it to be
considered for merging.

This patchset is based on 5.3-rc1 and a git branch is available here:

https://github.com/sbates130272/linux-p2pmem/ p2pdma_rc_map_v1

--

Logan Gunthorpe (14):
PCI/P2PDMA: Add constants for not-supported result
upstream_bridge_distance()
PCI/P2PDMA: Factor out __upstream_bridge_distance()
PCI/P2PDMA: Apply host bridge white list for ACS
PCI/P2PDMA: Cache the result of upstream_bridge_distance()
PCI/P2PDMA: Factor out host_bridge_whitelist()
PCI/P2PDMA: Add whitelist support for Intel Host Bridges
PCI/P2PDMA: Add the provider's pci_dev to the dev_pgmap struct
PCI/P2PDMA: Add attrs argument to pci_p2pdma_map_sg()
PCI/P2PDMA: Introduce pci_p2pdma_unmap_sg()
PCI/P2PDMA: Factor out __pci_p2pdma_map_sg()
PCI/P2PDMA: dma_map P2PDMA map requests that traverse the host bridge
PCI/P2PDMA: No longer require no-mmu for host bridge whitelist
PCI/P2PDMA: Update documentation for pci_p2pdma_distance_many()
PCI/P2PDMA: Introduce pci_p2pdma_[un]map_resource()

drivers/infiniband/core/rw.c | 6 +-
drivers/nvme/host/pci.c | 10 +-
drivers/pci/p2pdma.c | 400 +++++++++++++++++++++++++++--------
include/linux/memremap.h | 1 +
include/linux/pci-p2pdma.h | 28 ++-
5 files changed, 341 insertions(+), 104 deletions(-)

--
2.20.1


2019-07-23 10:32:11

by Logan Gunthorpe

[permalink] [raw]
Subject: [PATCH 06/14] PCI/P2PDMA: Add whitelist support for Intel Host Bridges

Intel devices do not have good support for P2P requests that span
different host bridges as the transactions will cross the QPI/UPI bus
and this does not perform well.

Therefore, enable support for these devices only if the host bridges
match.

Adds the Intel device's that have been tested to work. There are
likely many others out there that will need to be tested and added.

Signed-off-by: Logan Gunthorpe <[email protected]>
---
drivers/pci/p2pdma.c | 36 ++++++++++++++++++++++++++++++++----
1 file changed, 32 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c
index dfb802afc8ca..143e11d2a5c3 100644
--- a/drivers/pci/p2pdma.c
+++ b/drivers/pci/p2pdma.c
@@ -250,9 +250,28 @@ static void seq_buf_print_bus_devfn(struct seq_buf *buf, struct pci_dev *pdev)
seq_buf_printf(buf, "%s;", pci_name(pdev));
}

-static bool __host_bridge_whitelist(struct pci_host_bridge *host)
+static const struct pci_p2pdma_whitelist_entry {
+ unsigned short vendor;
+ unsigned short device;
+ bool req_same_host_bridge;
+} pci_p2pdma_whitelist[] = {
+ /* AMD ZEN */
+ {PCI_VENDOR_ID_AMD, 0x1450, false},
+
+ /* Intel Xeon E5/Core i7 */
+ {PCI_VENDOR_ID_INTEL, 0x3c00, true},
+ {PCI_VENDOR_ID_INTEL, 0x3c01, true},
+ /* Intel Xeon E7 v3/Xeon E5 v3/Core i7 */
+ {PCI_VENDOR_ID_INTEL, 0x2f00, true},
+ {PCI_VENDOR_ID_INTEL, 0x2f01, true},
+ {}
+};
+
+static bool __host_bridge_whitelist(struct pci_host_bridge *host,
+ bool same_host_bridge)
{
struct pci_dev *root = pci_get_slot(host->bus, PCI_DEVFN(0, 0));
+ const struct pci_p2pdma_whitelist_entry *entry;
unsigned short vendor, device;

if (!root)
@@ -262,9 +281,14 @@ static bool __host_bridge_whitelist(struct pci_host_bridge *host)
device = root->device;
pci_dev_put(root);

- /* AMD ZEN host bridges can do peer to peer */
- if (vendor == PCI_VENDOR_ID_AMD && device == 0x1450)
+ for (entry = pci_p2pdma_whitelist; entry->vendor; entry++) {
+ if (vendor != entry->vendor || device != entry->device)
+ continue;
+ if (entry->req_same_host_bridge && !same_host_bridge)
+ return false;
+
return true;
+ }

return false;
}
@@ -281,7 +305,11 @@ static bool host_bridge_whitelist(struct pci_dev *a, struct pci_dev *b)
if (iommu_present(a->dev.bus) || iommu_present(b->dev.bus))
return false;

- if (__host_bridge_whitelist(host_a) && __host_bridge_whitelist(host_b))
+ if (host_a == host_b)
+ return __host_bridge_whitelist(host_a, true);
+
+ if (__host_bridge_whitelist(host_a, false) &&
+ __host_bridge_whitelist(host_b, false))
return true;

return false;
--
2.20.1

2019-07-24 01:32:22

by Christian König

[permalink] [raw]
Subject: Re: [PATCH 00/14] PCI/P2PDMA: Support transactions that hit the host bridge

Am 23.07.19 um 01:08 schrieb Logan Gunthorpe:
> As discussed on the list previously, in order to fully support the
> whitelist Christian added with the IOMMU, we must ensure that we
> map any buffer going through the IOMMU with an aprropriate dma_map
> call. This patchset accomplishes this by cleaning up the output of
> upstream_bridge_distance() to better indicate the mapping requirements,
> caching these requirements in an xarray, then looking them up at map
> time and applying the appropriate mapping method.
>
> After this patchset, it's possible to use the NVMe-of P2P support to
> transfer between devices without a switch on the whitelisted root
> complexes. A couple Intel device I have tested this on have also
> been added to the white list.
>
> Most of the changes are contained within the p2pdma.c, but there are
> a few minor touches to other subsystems, mostly to add support
> to call an unmap function.
>
> The final patch in this series demonstrates a possible
> pci_p2pdma_map_resource() function that I expect Christian will need
> but does not have any users at this time so I don't intend for it to be
> considered for merging.
>
> This patchset is based on 5.3-rc1 and a git branch is available here:
>
> https://github.com/sbates130272/linux-p2pmem/ p2pdma_rc_map_v1

I reviewed patches #1-#3 and #14.

Feel free to stick an Acked-by: Christian König
<[email protected]> to the rest, but I'm not really deep into the
NVMe P2P handling here.

Regards,
Christian.


>
> --
>
> Logan Gunthorpe (14):
> PCI/P2PDMA: Add constants for not-supported result
> upstream_bridge_distance()
> PCI/P2PDMA: Factor out __upstream_bridge_distance()
> PCI/P2PDMA: Apply host bridge white list for ACS
> PCI/P2PDMA: Cache the result of upstream_bridge_distance()
> PCI/P2PDMA: Factor out host_bridge_whitelist()
> PCI/P2PDMA: Add whitelist support for Intel Host Bridges
> PCI/P2PDMA: Add the provider's pci_dev to the dev_pgmap struct
> PCI/P2PDMA: Add attrs argument to pci_p2pdma_map_sg()
> PCI/P2PDMA: Introduce pci_p2pdma_unmap_sg()
> PCI/P2PDMA: Factor out __pci_p2pdma_map_sg()
> PCI/P2PDMA: dma_map P2PDMA map requests that traverse the host bridge
> PCI/P2PDMA: No longer require no-mmu for host bridge whitelist
> PCI/P2PDMA: Update documentation for pci_p2pdma_distance_many()
> PCI/P2PDMA: Introduce pci_p2pdma_[un]map_resource()
>
> drivers/infiniband/core/rw.c | 6 +-
> drivers/nvme/host/pci.c | 10 +-
> drivers/pci/p2pdma.c | 400 +++++++++++++++++++++++++++--------
> include/linux/memremap.h | 1 +
> include/linux/pci-p2pdma.h | 28 ++-
> 5 files changed, 341 insertions(+), 104 deletions(-)
>
> --
> 2.20.1

2019-07-24 02:24:50

by Logan Gunthorpe

[permalink] [raw]
Subject: Re: [PATCH 00/14] PCI/P2PDMA: Support transactions that hit the host bridge



On 2019-07-23 10:30 a.m., Koenig, Christian wrote:
> Am 23.07.19 um 01:08 schrieb Logan Gunthorpe:
>> As discussed on the list previously, in order to fully support the
>> whitelist Christian added with the IOMMU, we must ensure that we
>> map any buffer going through the IOMMU with an aprropriate dma_map
>> call. This patchset accomplishes this by cleaning up the output of
>> upstream_bridge_distance() to better indicate the mapping requirements,
>> caching these requirements in an xarray, then looking them up at map
>> time and applying the appropriate mapping method.
>>
>> After this patchset, it's possible to use the NVMe-of P2P support to
>> transfer between devices without a switch on the whitelisted root
>> complexes. A couple Intel device I have tested this on have also
>> been added to the white list.
>>
>> Most of the changes are contained within the p2pdma.c, but there are
>> a few minor touches to other subsystems, mostly to add support
>> to call an unmap function.
>>
>> The final patch in this series demonstrates a possible
>> pci_p2pdma_map_resource() function that I expect Christian will need
>> but does not have any users at this time so I don't intend for it to be
>> considered for merging.
>>
>> This patchset is based on 5.3-rc1 and a git branch is available here:
>>
>> https://github.com/sbates130272/linux-p2pmem/ p2pdma_rc_map_v1
>
> I reviewed patches #1-#3 and #14.
>
> Feel free to stick an Acked-by: Christian König
> <[email protected]> to the rest, but I'm not really deep into the
> NVMe P2P handling here.

Thanks!

Logan

2019-07-25 18:53:39

by Jason Gunthorpe

[permalink] [raw]
Subject: Re: [PATCH 06/14] PCI/P2PDMA: Add whitelist support for Intel Host Bridges

On Mon, Jul 22, 2019 at 05:08:51PM -0600, Logan Gunthorpe wrote:
> Intel devices do not have good support for P2P requests that span
> different host bridges as the transactions will cross the QPI/UPI bus
> and this does not perform well.
>
> Therefore, enable support for these devices only if the host bridges
> match.
>
> Adds the Intel device's that have been tested to work. There are
> likely many others out there that will need to be tested and added.
>
> Signed-off-by: Logan Gunthorpe <[email protected]>
> drivers/pci/p2pdma.c | 36 ++++++++++++++++++++++++++++++++----
> 1 file changed, 32 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c
> index dfb802afc8ca..143e11d2a5c3 100644
> +++ b/drivers/pci/p2pdma.c
> @@ -250,9 +250,28 @@ static void seq_buf_print_bus_devfn(struct seq_buf *buf, struct pci_dev *pdev)
> seq_buf_printf(buf, "%s;", pci_name(pdev));
> }
>
> -static bool __host_bridge_whitelist(struct pci_host_bridge *host)
> +static const struct pci_p2pdma_whitelist_entry {
> + unsigned short vendor;
> + unsigned short device;
> + bool req_same_host_bridge;

This would be more readable in the initializer as a flags not a bool

Jason

2019-07-25 19:16:10

by Logan Gunthorpe

[permalink] [raw]
Subject: Re: [PATCH 06/14] PCI/P2PDMA: Add whitelist support for Intel Host Bridges



On 2019-07-25 12:52 p.m., Jason Gunthorpe wrote:
> On Mon, Jul 22, 2019 at 05:08:51PM -0600, Logan Gunthorpe wrote:
>> Intel devices do not have good support for P2P requests that span
>> different host bridges as the transactions will cross the QPI/UPI bus
>> and this does not perform well.
>>
>> Therefore, enable support for these devices only if the host bridges
>> match.
>>
>> Adds the Intel device's that have been tested to work. There are
>> likely many others out there that will need to be tested and added.
>>
>> Signed-off-by: Logan Gunthorpe <[email protected]>
>> drivers/pci/p2pdma.c | 36 ++++++++++++++++++++++++++++++++----
>> 1 file changed, 32 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c
>> index dfb802afc8ca..143e11d2a5c3 100644
>> +++ b/drivers/pci/p2pdma.c
>> @@ -250,9 +250,28 @@ static void seq_buf_print_bus_devfn(struct seq_buf *buf, struct pci_dev *pdev)
>> seq_buf_printf(buf, "%s;", pci_name(pdev));
>> }
>>
>> -static bool __host_bridge_whitelist(struct pci_host_bridge *host)
>> +static const struct pci_p2pdma_whitelist_entry {
>> + unsigned short vendor;
>> + unsigned short device;
>> + bool req_same_host_bridge;
>
> This would be more readable in the initializer as a flags not a bool

Ok, will change for v2.

Logan