Copy Offload in NVMe Fabrics with P2P PCI Memory

Some PCI devices may have memory mapped in a BAR space that's intended for use in peer-to-peer transactions. In order to enable such transactions the memory must be registered with ZONE_DEVICE pages so it can be used by DMA interfaces in existing drivers.

Add an interface for other subsystems to find and allocate chunks of P2P memory as necessary to facilitate transfers between two PCI peers:

int pci_p2pdma_add_client();
struct pci_dev *pci_p2pmem_find();
void *pci_alloc_p2pmem();

The new interface requires a driver to collect a list of client devices involved in the transaction with the pci_p2pmem_add_client*() functions then call pci_p2pmem_find() to obtain any suitable P2P memory. Once this is done the list is bound to the memory and the calling driver is free to add and remove clients as necessary (adding incompatible clients will fail). With a suitable p2pmem device, memory can then be allocated with pci_alloc_p2pmem() for use in DMA transactions.

Depending on hardware, using peer-to-peer memory may reduce the bandwidth of the transfer but can significantly reduce pressure on system memory. This may be desirable in many cases: for example a system could be designed with a small CPU connected to a PCI switch by a small number of lanes

which would maximize the number of lanes available to connect to NVMe devices.

The code is designed to only utilize the p2pmem device if all the devices involved in a transfer are behind the same root port (typically through

a network of PCIe switches). This is because we have no way of knowing whether peer-to-peer routing between PCIe Root Ports is supported (PCIe r4.0, sec 1.3.1). Additionally, the benefits of P2P transfers that go through the RC is limited to only reducing DRAM usage and, in some cases, coding convenience. The PCI-SIG may be exploring adding a new capability bit to advertise whether this is possible for future hardware.

This commit includes significant rework and feedback from Christoph Hellwig.

Signed-off-by Christoph Hellwig <[email protected]>

Signed-off-by Logan Gunthorpe <[email protected]>

---
 drivers/pci/Kconfig        |  17 ++
 drivers/pci/Makefile       |   1 +
 drivers/pci/p2pdma.c       | 694 +++++++++++++++++++++++++++++++++++++++++++++
 include/linux/memremap.h   |  18 ++
 include/linux/pci-p2pdma.h | 100 +++++++
 include/linux/pci.h        |   4 +
 6 files changed, 834 insertions(+)
 create mode 100644 drivers/pci/p2pdma.c
 create mode 100644 include/linux/pci-p2pdma.h

drivers/pci/Kconfig +3 -0

index 34b56a8f8480..b2396c22b53e 100644
@@ -124,6 +124,23 @@ config PCI_PASID
 
 	  If unsure, say N.
 
+config PCI_P2PDMA
+	bool "PCI peer-to-peer transfer support"
+	depends on PCI && ZONE_DEVICE && EXPERT

drivers/pci/Kconfig +14 -0

+{
+	struct pci_dev *up1, *up2;
+
+	if (!pdev)
+		return NULL;
+
+	up1 = pci_dev_get(pci_upstream_bridge(pdev));
+	if (!up1)
+		return NULL;
+
+	up2 = pci_dev_get(pci_upstream_bridge(up1));
+	pci_dev_put(up1);
+
+	return up2;

drivers/pci/Kconfig +8 -0

+ * one step back to the same device).
+ *
+ * In the case where two devices are connected to the same PCIe switch, the
+ * value 4 will be returned. This corresponds to the following PCI tree:
+ *
+ *     -+  Root Port
+ *      \+ Switch Upstream Port
+ *       +-+ Switch Downstream Port

drivers/pci/Kconfig +2 -0

+ */
+static int upstream_bridge_distance(struct pci_dev *a, > +				    struct pci_dev *b)

drivers/pci/p2pdma.c line 193

Quoted 8 lines

drivers/pci/p2pdma.c new file +8 -0

+/*
+ * If a device is behind a switch, we try to find the upstream bridge
+ * port of the switch. This requires two calls to pci_upstream_bridge():
+ * one for the upstream port on the switch, one on the upstream port
+ * for the next level in the hierarchy. Because of this, devices connected
+ * to the root port will be rejected.
+ */
+static struct pci_dev *get_upstream_bridge_port(struct pci_dev *pdev)

This function doesn't seem to be used anymore. Thanks for all your hard work to get rid of it!

The DMA address used when mapping PCI P2P memory must be the PCI bus address. Thus, introduce pci_p2pmem_[un]map_sg() to map the correct addresses when using P2P memory.

For this, we assume that an SGL passed to these functions contain all P2P memory or no P2P memory.

Signed-off-by Logan Gunthorpe <[email protected]>

drivers/pci/Kconfig +6 -0

+	  individual devices behind any switch will not be able to be
+	  assigned to separate VMs because there is no isolation between
+	  them. Additionally, any malicious PCIe devices will be able to
+	  DMA to memory exposed by other EPs in the same domain as TLPs
+	  will not be checked by the IOMMU.
+
 	  If unsure, say N.
 
 config PCI_LABEL

For peer-to-peer transactions to work the downstream ports in each switch must not have the ACS flags set. At this time there is no way to dynamically change the flags and update the corresponding IOMMU groups so this is done at enumeration time before the groups are assigned.

This effectively means that if CONFIG_PCI_P2PDMA is selected then all devices behind any PCIe switch heirarchy will be in the same IOMMU group. Which implies that individual devices behind any switch heirarchy will not be able to be assigned to separate VMs because there is no isolation between them. Additionally, any malicious PCIe devices will be able to DMA to memory exposed by other EPs in the same domain as TLPs will not be checked by the IOMMU.

Given that the intended use case of P2P Memory is for users with custom hardware designed for purpose, we do not expect distributors to ever need to enable this option. Users that want to use P2P must have compiled a custom kernel with this configuration option and understand the implications regarding ACS. They will either not require ACS or will have design the system in such a way that devices that require isolation will be separate from those using P2P transactions.

Signed-off-by Logan Gunthorpe <[email protected]>

---
 drivers/pci/Kconfig        |  9 +++++++++
 drivers/pci/p2pdma.c       | 45 ++++++++++++++++++++++++++++++---------------
 drivers/pci/pci.c          |  6 ++++++
 include/linux/pci-p2pdma.h |  5 +++++
 4 files changed, 50 insertions(+), 15 deletions(-)

drivers/pci/Kconfig +0 -0

index b2396c22b53e..b6db41d4b708 100644

drivers/pci/Kconfig +6 -0

+	  individual devices behind any switch will not be able to be
+	  assigned to separate VMs because there is no isolation between
+	  them. Additionally, any malicious PCIe devices will be able to
+	  DMA to memory exposed by other EPs in the same domain as TLPs
+	  will not be checked by the IOMMU.
+
 	  If unsure, say N.
 
 config PCI_LABEL

drivers/pci/p2pdma.c +0 -1

index ed9dce8552a2..e9f43b43acac 100644
@@ -240,27 +240,42 @@ static struct pci_dev *find_parent_pci_dev(struct device *dev)
 }
 
 /*
- * If a device is behind a switch, we try to find the upstream bridge

drivers/pci/p2pdma.c +24 -10

+ *
+ * This function is called when the devices are first enumerated and
+ * will result in all devices behind any bridge to be in the same IOMMU
+ * group. At this time, there is no way to "hotplug" IOMMU groups so we rely
+ * on this largish hammer. If you need the devices to be in separate groups
+ * don't enable CONFIG_PCI_P2PDMA.
+ *
+ * Returns 1 if the ACS bits for this device was cleared, otherwise 0.
  */
-static struct pci_dev *get_upstream_bridge_port(struct pci_dev *pdev)
+int pci_p2pdma_disable_acs(struct pci_dev *pdev)
 {
-	struct pci_dev *up1, *up2;
+	int pos;
+	u16 ctrl;
 
-	if (!pdev)
-		return NULL;
+	if (!pci_is_bridge(pdev))
+		return 0;
 
-	up1 = pci_dev_get(pci_upstream_bridge(pdev));
-	if (!up1)
-		return NULL;
+	pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_ACS);
+	if (!pos)
+		return 0;
+
+	pci_info(pdev, "disabling ACS flags for peer-to-peer DMA\n");
+
+	pci_read_config_word(pdev, pos + PCI_ACS_CTRL, &ctrl);
+
+	ctrl &= ~(PCI_ACS_RR | PCI_ACS_CR);
 
-	up2 = pci_dev_get(pci_upstream_bridge(up1));
-	pci_dev_put(up1);
+	pci_write_config_word(pdev, pos + PCI_ACS_CTRL, ctrl);
 
-	return up2;
+	return 1;
 }
 
 /*

drivers/pci/pci.c +6 -0

index e597655a5643..7e2f5724ba22 100644
@@ -16,6 +16,7 @@
 #include <linux/of.h>
 #include <linux/of_pci.h>
 #include <linux/pci.h>
+#include <linux/pci-p2pdma.h>
 #include <linux/pm.h>
 #include <linux/slab.h>
 #include <linux/module.h>

@@ -2835,6 +2836,11 @@ static void pci_std_enable_acs(struct pci_dev *dev)
  */
 void pci_enable_acs(struct pci_dev *dev)
 {
+#ifdef CONFIG_PCI_P2PDMA
+	if (pci_p2pdma_disable_acs(dev))
+		return;
+#endif
+
 	if (!pci_acs_enable)
 		return;
 

include/linux/pci-p2pdma.h +5 -0

index 0cde88341eeb..fcb3437a2f3c 100644
@@ -18,6 +18,7 @@ struct block_device;
 struct scatterlist;
 
 #ifdef CONFIG_PCI_P2PDMA
+int pci_p2pdma_disable_acs(struct pci_dev *pdev);
 int pci_p2pdma_add_resource(struct pci_dev *pdev, int bar, size_t size,
 		u64 offset);
 int pci_p2pdma_add_client(struct list_head *head, struct device *dev);

@@ -40,6 +41,10 @@ int pci_p2pdma_map_sg(struct device *dev, struct scatterlist *sg, int nents,
 void pci_p2pdma_unmap_sg(struct device *dev, struct scatterlist *sg, int nents,
 			 enum dma_data_direction dir);
 #else /* CONFIG_PCI_P2PDMA */
+static inline int pci_p2pdma_disable_acs(struct pci_dev *pdev)
+{
+	return 0;
+}
 static inline int pci_p2pdma_add_resource(struct pci_dev *pdev, int bar,
 		size_t size, u64 offset)
 {

Signature · 1 line

2.11.0

[+to Alex]

Alex,

Are you happy with this strategy of turning off ACS based on CONFIG_PCI_P2PDMA? We only check this at enumeration-time and I don't know if there are other places we would care?

Quoted 36 lines

For peer-to-peer transactions to work the downstream ports in each switch must not have the ACS flags set. At this time there is no way to dynamically change the flags and update the corresponding IOMMU groups so this is done at enumeration time before the groups are assigned.

This effectively means that if CONFIG_PCI_P2PDMA is selected then all devices behind any PCIe switch heirarchy will be in the same IOMMU group. Which implies that individual devices behind any switch heirarchy will not be able to be assigned to separate VMs because there is no isolation between them. Additionally, any malicious PCIe devices will be able to DMA to memory exposed by other EPs in the same domain as TLPs will not be checked by the IOMMU.

Given that the intended use case of P2P Memory is for users with custom hardware designed for purpose, we do not expect distributors to ever need to enable this option. Users that want to use P2P must have compiled a custom kernel with this configuration option and understand the implications regarding ACS. They will either not require ACS or will have design the system in such a way that devices that require isolation will be separate from those using P2P transactions.

Signed-off-by Logan Gunthorpe <[email protected]>

---
  drivers/pci/Kconfig        |  9 +++++++++
  drivers/pci/p2pdma.c       | 45 ++++++++++++++++++++++++++++++---------------
  drivers/pci/pci.c          |  6 ++++++
  include/linux/pci-p2pdma.h |  5 +++++
  4 files changed, 50 insertions(+), 15 deletions(-)

drivers/pci/Kconfig +0 -0

index b2396c22b53e..b6db41d4b708 100644

drivers/pci/Kconfig config PCI_P2PDMA · line 144

Quoted 6 lines

drivers/pci/Kconfig +3 -0

@@ -139,6 +139,15 @@ config PCI_P2PDMA
  	  transations must be between devices behind the same root port.
  	  (Typically behind a network of PCIe switches).
 
+	  Enabling this option will also disable ACS on all ports behind
+	  any PCIe switch. This effectively puts all devices behind any
+	  switch heirarchy into the same IOMMU group. Which implies that

s/heirarchy/hierarchy/ (also above in changelog)

Quoted 18 lines

drivers/pci/Kconfig +6 -0

+	  individual devices behind any switch will not be able to be
+	  assigned to separate VMs because there is no isolation between
+	  them. Additionally, any malicious PCIe devices will be able to
+	  DMA to memory exposed by other EPs in the same domain as TLPs
+	  will not be checked by the IOMMU.
+
  	  If unsure, say N.
 
  config PCI_LABEL

drivers/pci/p2pdma.c +0 -1

index ed9dce8552a2..e9f43b43acac 100644
@@ -240,27 +240,42 @@ static struct pci_dev *find_parent_pci_dev(struct device *dev)
  }
 
  /*
- * If a device is behind a switch, we try to find the upstream bridge

drivers/pci/p2pdma.c static struct pci_dev *find_parent_pci_dev(struct device *dev) · line 248

Quoted 10 lines

drivers/pci/p2pdma.c +6 -4

- * port of the switch. This requires two calls to pci_upstream_bridge():
- * one for the upstream port on the switch, one on the upstream port
- * for the next level in the hierarchy. Because of this, devices connected
- * to the root port will be rejected.
+ * pci_p2pdma_disable_acs - disable ACS flags for all PCI bridges
+ * @pdev: device to disable ACS flags for
+ *
+ * The ACS flags for P2P Request Redirect and P2P Completion Redirect need
+ * to be disabled on any PCI bridge in order for the TLPS to not be forwarded
+ * up to the RC which is not what we want for P2P.

s/PCI bridge/PCIe switch/ (ACS doesn't apply to conventional PCI)

Quoted 93 lines

drivers/pci/p2pdma.c +24 -10

+ *
+ * This function is called when the devices are first enumerated and
+ * will result in all devices behind any bridge to be in the same IOMMU
+ * group. At this time, there is no way to "hotplug" IOMMU groups so we rely
+ * on this largish hammer. If you need the devices to be in separate groups
+ * don't enable CONFIG_PCI_P2PDMA.
+ *
+ * Returns 1 if the ACS bits for this device was cleared, otherwise 0.
   */
-static struct pci_dev *get_upstream_bridge_port(struct pci_dev *pdev)
+int pci_p2pdma_disable_acs(struct pci_dev *pdev)
  {
-	struct pci_dev *up1, *up2;
+	int pos;
+	u16 ctrl;
 
-	if (!pdev)
-		return NULL;
+	if (!pci_is_bridge(pdev))
+		return 0;
 
-	up1 = pci_dev_get(pci_upstream_bridge(pdev));
-	if (!up1)
-		return NULL;
+	pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_ACS);
+	if (!pos)
+		return 0;
+
+	pci_info(pdev, "disabling ACS flags for peer-to-peer DMA\n");
+
+	pci_read_config_word(pdev, pos + PCI_ACS_CTRL, &ctrl);
+
+	ctrl &= ~(PCI_ACS_RR | PCI_ACS_CR);
 
-	up2 = pci_dev_get(pci_upstream_bridge(up1));
-	pci_dev_put(up1);
+	pci_write_config_word(pdev, pos + PCI_ACS_CTRL, ctrl);
 
-	return up2;
+	return 1;
  }
 
  /*

drivers/pci/pci.c +6 -0

index e597655a5643..7e2f5724ba22 100644
@@ -16,6 +16,7 @@
  #include <linux/of.h>
  #include <linux/of_pci.h>
  #include <linux/pci.h>
+#include <linux/pci-p2pdma.h>
  #include <linux/pm.h>
  #include <linux/slab.h>
  #include <linux/module.h>

@@ -2835,6 +2836,11 @@ static void pci_std_enable_acs(struct pci_dev *dev)
   */
  void pci_enable_acs(struct pci_dev *dev)
  {
+#ifdef CONFIG_PCI_P2PDMA
+	if (pci_p2pdma_disable_acs(dev))
+		return;
+#endif
+
  	if (!pci_acs_enable)
  		return;
 

include/linux/pci-p2pdma.h +5 -0

index 0cde88341eeb..fcb3437a2f3c 100644
@@ -18,6 +18,7 @@ struct block_device;
  struct scatterlist;
 
  #ifdef CONFIG_PCI_P2PDMA
+int pci_p2pdma_disable_acs(struct pci_dev *pdev);
  int pci_p2pdma_add_resource(struct pci_dev *pdev, int bar, size_t size,
  		u64 offset);
  int pci_p2pdma_add_client(struct list_head *head, struct device *dev);

@@ -40,6 +41,10 @@ int pci_p2pdma_map_sg(struct device *dev, struct scatterlist *sg, int nents,
  void pci_p2pdma_unmap_sg(struct device *dev, struct scatterlist *sg, int nents,
  			 enum dma_data_direction dir);
  #else /* CONFIG_PCI_P2PDMA */
+static inline int pci_p2pdma_disable_acs(struct pci_dev *pdev)
+{
+	return 0;
+}
  static inline int pci_p2pdma_add_resource(struct pci_dev *pdev, int bar,
  		size_t size, u64 offset)
  {

Signature · 1 line

2.11.0

Hi Christian

Quoted 2 lines

AMD APUs mandatory need the ACS flag set for the GPU integrated in the CPU when IOMMU is enabled or otherwise you will break SVM.

OK but in this case aren't you losing (many of) the benefits of P2P since all DMAs will now get routed up to the IOMMU before being passed down to the destination PCIe EP?

Quoted 3 lines

Similar problems arise when you do this for dedicated GPU, but we haven't upstreamed the support for this yet.

Hmm, as above. With ACS enabled on all downstream ports any P2P enabled DMA will be routed to the IOMMU which removes a lot of the benefit.

Quoted 1 line

So that is a clear NAK from my side for the approach.

Do you have an alternative? This is the approach we arrived it after a reasonably lengthy discussion on the mailing lists. Alex, are you still comfortable with this approach?

Quoted 1 line

And what exactly is the problem here?

We had a pretty lengthy discussion on this topic on one of the previous revisions. The issue is that currently there is no mechanism in the IOMMU code to inform VMs if IOMMU groupings change. Since p2pdma can dynamically change its topology (due to PCI hotplug) we had to be cognizant of the fact that ACS settings could change. Since there is no way to currently handle changing ACS settings and hence IOMMU groupings the consensus was to simply disable ACS on all ports in a p2pdma domain. This effectively makes all the devices in the p2pdma domain part of the same IOMMU grouping. The plan will be to address this in time and add a mechanism for IOMMU grouping changes and notification to VMs but that's not part of this series. Note you are still allowed to have ACS functioning on other PCI domains so if you do not a plurality of IOMMU groupings you can still achieve it (but you can't do p2pdma across IOMMU groupings, which is safe).

Quoted 1 line

I'm currently testing P2P with GPUs in different IOMMU domains and at least with AMD IOMMUs that works perfectly fine.

Yup that should work though again I have to ask are you disabling ACS on the ports between the two peer devices to get the p2p benefit? If not you are not getting all the performance benefit (due to IOMMU routing), if you are then there are obviously security implications between those IOMMU domains if they are assigned to different VMs. And now the issue is if new devices are added and the p2p topology needed to change there would be no way to inform the VMs of any IOMMU group change.

Cheers

Stephen

AMD APUs mandatory need the ACS flag set for the GPU integrated in the CPU when IOMMU is enabled or otherwise you will break SVM.

And what exactly is the problem here? I'm currently testing P2P with GPUs in different IOMMU domains and at least with AMD IOMMUs that works perfectly fine.

Quoted 2 lines

AMD APUs mandatory need the ACS flag set for the GPU integrated in the CPU when IOMMU is enabled or otherwise you will break SVM.

Well, given that the current set only disables ACS bits on bridges (previous versions were only on switches) this shouldn't be an issue for integrated devices. We do not disable ACS flags globally.

Quoted 4 lines

And what exactly is the problem here? I'm currently testing P2P with GPUs in different IOMMU domains and at least with AMD IOMMUs that works perfectly fine.

In addition to Stephen's comments, seeing we've established a general need to avoid the root complex (until we have a whitelist at least) we must have ACS disabled along the path between the devices. Otherwise, all TLPs will go through the root complex and if there is no support it will fail.

If the consensus is we want a command line option, then so be it. But we'll have to deny pretty much all P2P transactions unless the user correctly disables ACS along the path using the command line option and this is really annoying for users of this functionality to understand how to do that correctly.

Logan

E.g. transactions are initially send to the root complex for translation, that's for sure. But at least for AMD GPUs the root complex answers with the translated address which is then cached in the device.

So further transactions for the same address range then go directly to the destination.

Quoted 2 lines · On 08/05/18 10:50 AM, Christian K=C3=B6nig wrote:

E.g. transactions are initially send to the root complex for=20 translation, that's for sure. But at least for AMD GPUs the root comple=

Quoted 3 lines

answers with the translated address which is then cached in the device. =20
So further transactions for the same address range then go directly to=

Quoted 1 line

the destination. =20

=20
Sounds like you are referring to Address Translation Services (ATS). This is quite separate from ACS and, to my knowledge, isn't widely supported by switch hardware.

They are not so unrelated, see the ACS Direct Translated P2P capability, which in fact must be implemented by switch downstream ports implementing ACS and works specifically with ATS. This appears to be the way the PCI SIG would intend for P2P to occur within an IOMMU managed topology, routing pre-translated DMA directly between peer devices while requiring non-translated requests to bounce through the IOMMU. Really, what's the value of having an I/O virtual address space provided by an IOMMU if we're going to allow physical DMA between downstream devices, couldn't we just turn off the IOMMU altogether? Of course ATS is not without holes itself, basically that we trust the endpoint's implementation of ATS implicitly. Thanks,

Quoted 11 lines · Alex Williamson

They are not so unrelated, see the ACS Direct Translated P2P capability, which in fact must be implemented by switch downstream ports implementing ACS and works specifically with ATS. This appears to be the way the PCI SIG would intend for P2P to occur within an IOMMU managed topology, routing pre-translated DMA directly between peer devices while requiring non-translated requests to bounce through the IOMMU. Really, what's the value of having an I/O virtual address space provided by an IOMMU if we're going to allow physical DMA between downstream devices, couldn't we just turn off the IOMMU altogether? Of course ATS is not without holes itself, basically that we trust the endpoint's implementation of ATS implicitly. Thanks,

I agree that this is what the SIG intends, but I don't think hardware fully supports this methodology yet. The Direct Translated capability just requires switches to forward packets that have the AT request type set. It does not require them to do the translation or to support ATS such that P2P requests can be translated by the IOMMU. I expect this is so that an downstream device can implement ATS and not get messed up by an upstream switch that doesn't support it.

Well, I'm a bit confused, this patch series is specifically disabling ACS on switches, but per the spec downstream switch ports implementing ACS MUST implement direct translated P2P. So it seems the only potential gap here is the endpoint, which must support ATS or else there's nothing for direct translated P2P to do. The switch port plays

no part in the actual translation of the request, ATS on the endpoint has already cached the translation and is now attempting to use it. For the switch port, this only becomes a routing decision, the request is already translated, therefore ACS RR and EC can be ignored to perform "normal" (direct) routing, as if ACS were not present. It would be a shame to go to all the trouble of creating this no-ACS mode to find out the target hardware supports ATS and should have simply used it, or we should have disabled the IOMMU altogether, which leaves ACS disabled.

Quoted 5 lines · Alex Williamson

Well, I'm a bit confused, this patch series is specifically disabling ACS on switches, but per the spec downstream switch ports implementing ACS MUST implement direct translated P2P. So it seems the only potential gap here is the endpoint, which must support ATS or else there's nothing for direct translated P2P to do. The switch port plays

Quoted 8 lines

no part in the actual translation of the request, ATS on the endpoint has already cached the translation and is now attempting to use it. For the switch port, this only becomes a routing decision, the request is already translated, therefore ACS RR and EC can be ignored to perform "normal" (direct) routing, as if ACS were not present. It would be a shame to go to all the trouble of creating this no-ACS mode to find out the target hardware supports ATS and should have simply used it, or we should have disabled the IOMMU altogether, which leaves ACS disabled.

Ah, ok, I didn't think it was the endpoint that had to implement ATS. But in that case, for our application, we need NVMe cards and RDMA NICs to all have ATS support and I expect that is just as unlikely. At least none of the endpoints on my system support it. Maybe only certain GPUs have this support.

Yes, GPUs seem to be leading the pack in implementing ATS. So now the dumb question, why not simply turn off the IOMMU and thus ACS? The argument of using the IOMMU for security is rather diminished if we're specifically enabling devices to poke one another directly and clearly this isn't favorable for device assignment either. Are there target systems where this is not a simple kernel commandline option? Thanks,

Quoted 6 lines · Alex Williamson

Yes, GPUs seem to be leading the pack in implementing ATS. So now the dumb question, why not simply turn off the IOMMU and thus ACS? The argument of using the IOMMU for security is rather diminished if we're specifically enabling devices to poke one another directly and clearly this isn't favorable for device assignment either. Are there target systems where this is not a simple kernel commandline option? Thanks,

Well, turning off the IOMMU doesn't necessarily turn off ACS. We've run into some bios's that set the bits on boot (which is annoying).

I also don't expect people will respond well to making the IOMMU and P2P exclusive. The IOMMU is often used for more than just security and on many platforms it's enabled by default. I'd much rather allow IOMMU use but have fewer isolation groups in much the same way as if you had PCI bridges that didn't support ACS.

Hi Alex

Quoted 2 lines

But it would be a much easier proposal to disable ACS when the
IOMMU is not enabled, ACS has no real purpose in that case.

I guess one issue I have with this is that it disables IOMMU groups for all Root Ports and not just the one(s) we wish to do p2pdma on.

Quoted 6 lines

The IOMMU and P2P are already not exclusive, we can bounce off
the IOMMU or make use of ATS as we've previously discussed. We were
previously talking about a build time config option that you
didn't expect distros to use, so I don't think intervention for the user to disable the IOMMU if it's enabled by default is a serious concern either.

ATS definitely makes things more interesting for the cases where the EPs support it. However I don't really have a handle on how common ATS support is going to be in the kinds of devices we have been focused on (NVMe SSDs and RDMA NICs mostly).

Quoted 3 lines

What you're trying to do is enabled direct peer-to-peer for endpoints which do not support ATS when the IOMMU is enabled, which is not something that necessarily makes sense to me.

As above the advantage of leaving the IOMMU on is that it allows for both p2pdma PCI domains and IOMMU groupings PCI domains in the same system. It is just that these domains will be separate to each other.

Quoted 2 lines

So that leaves avoiding bounce buffers as the remaining IOMMU
feature

I agree with you here that the devices we will want to use for p2p will probably not require a bounce buffer and will support 64 bit DMA addressing.

Quoted 3 lines

I'm still not seeing why it's terribly undesirable to require devices to support ATS if they want to do direct P2P with an IOMMU enabled.

I think the one reason is for the use-case above. Allowing IOMMU groupings on one domain and p2pdma on another domain....

If IOMMU grouping implies device assignment (because nobody else uses it to the same extent as device assignment) then the build-time option falls to pieces, we need a single kernel that can do both. I think we need to get more clever about allowing the user to specify exactly at which points in the topology they want to disable isolation. Thanks,

Quoted 4 lines

Yeah, so based on the discussion I'm leaning toward just having a command line option that takes a list of BDFs and disables ACS
for them. (Essentially as Dan has suggested.) This avoids the shotgun.

I concur that this seems to be where the conversation is taking us.

@Alex - Before we go do this can you provide input on the approach? I don't want to re-spin only to find we are still not converging on the ACS issue....

On to the implementation details... I already mentioned the BDF issue in my other reply. If we had a way to persistently identify a device, would we specify the downstream points at which we want to disable ACS or the endpoints that we want to connect? The latter has a problem that the grouping upstream of an endpoint is already set by the time we discover the endpoint, so we might need to unwind to get the grouping correct. The former might be more difficult for users to find the necessary nodes, but easier for the kernel to deal with during discovery.

A runtime, sysfs approach has some benefits here,
especially in identifying the device assuming we're ok with leaving the persistence problem to userspace tools. I'm still a little fond of the idea of exposing an acs_flags attribute for devices in sysfs where a write would do a soft unplug and re-add of all affected devices to automatically recreate the proper grouping. Any dynamic change in routing and grouping would require all DMA be re-established anyway and a soft hotplug seems like an elegant way of handling it. Thanks,

Quoted 8 lines · Alex Williamson

A runtime, sysfs approach has some benefits here,
especially in identifying the device assuming we're ok with leaving the persistence problem to userspace tools. I'm still a little fond of the idea of exposing an acs_flags attribute for devices in sysfs where a write would do a soft unplug and re-add of all affected devices to automatically recreate the proper grouping. Any dynamic change in routing and grouping would require all DMA be re-established anyway and a soft hotplug seems like an elegant way of handling it. Thanks,

This approach sounds like it has a lot more issues to contend with:

For starters, a soft unplug/re-add of all the devices behind a switch is going to be difficult if a lot of those devices have had drivers installed and their respective resources are now mounted or otherwise in use.

Then, do we have to redo a the soft-replace every time we change the ACS bit for every downstream port? That could mean you have to do dozens soft-replaces before you have all the ACS bits set which means you have a storm of drivers being added and removed.

This would require some kind of fancy custom setup software that runs at just the right time in the boot sequence or a lot of work on the users part to unbind all the resources, setup the ACS bits and then rebind everything (assuming the soft re-add doesn't rebind it every time you adjust one ACS bit). Ugly.

IMO, if we need to do the sysfs approach then we need to be able to adjust the groups dynamically in a sensible way and not through the large hammer that is soft-replaces. I think this would be great but I don't think we will be tackling that with this patch set.

Quoted 5 lines · Alex Williamson

If IOMMU grouping implies device assignment (because nobody else uses it to the same extent as device assignment) then the build-time option falls to pieces, we need a single kernel that can do both. I think we need to get more clever about allowing the user to specify exactly at which points in the topology they want to disable isolation. Thanks,

Yeah, so based on the discussion I'm leaning toward just having a command line option that takes a list of BDFs and disables ACS for them. (Essentially as Dan has suggested.) This avoids the shotgun.

Then, the pci_p2pdma_distance command needs to check that ACS is disabled for all bridges between the two devices. If this is not the case, it returns -1. Future work can check if the EP has ATS support, in which case it has to check for the ACS direct translated bit.

A user then needs to either disable the IOMMU and/or add the command line option to disable ACS for the specific downstream ports in the PCI hierarchy. This means the IOMMU groups will be less granular but presumably the person adding the command line argument understands this.

We may also want to do some work so that there's informative dmesgs on which BDFs need to be specified on the command line so it's not so difficult for the user to figure out.

Quoted 17 lines · On Tue, 8 May 2018 16:10:19 -0600 Logan Gunthorpe <[email protected]> wrote:

Quoted 5 lines · Alex Williamson

If IOMMU grouping implies device assignment (because nobody else uses it to the same extent as device assignment) then the build-time option falls to pieces, we need a single kernel that can do both. I think we need to get more clever about allowing the user to specify exactly at which points in the topology they want to disable isolation. Thanks,

Yeah, so based on the discussion I'm leaning toward just having a command line option that takes a list of BDFs and disables ACS for them. (Essentially as Dan has suggested.) This avoids the shotgun.

Then, the pci_p2pdma_distance command needs to check that ACS is disabled for all bridges between the two devices. If this is not the case, it returns -1. Future work can check if the EP has ATS support, in which case it has to check for the ACS direct translated bit.

Quoted 8 lines

A user then needs to either disable the IOMMU and/or add the command line option to disable ACS for the specific downstream ports in the PCI hierarchy. This means the IOMMU groups will be less granular but presumably the person adding the command line argument understands this.

We may also want to do some work so that there's informative dmesgs on which BDFs need to be specified on the command line so it's not so difficult for the user to figure out.

I'd advise caution with a user supplied BDF approach, we have no guaranteed persistence for a device's PCI address. Adding a device might renumber the buses, replacing a device with one that consumes more/less bus numbers can renumber the buses, motherboard firmware updates could renumber the buses, pci=assign-buses can renumber the buses, etc. This is why the VT-d spec makes use of device paths when describing PCI hierarchies, firmware can't know what bus number will be assigned to a device, but it does know the base bus number and the path of devfns needed to get to it. I don't know how we come up with an option that's easy enough for a user to understand, but reasonably robust against hardware changes. Thanks,

Quoted 11 lines

I'd advise caution with a user supplied BDF approach, we have no guaranteed persistence for a device's PCI address. Adding a device might renumber the buses, replacing a device with one that consumes more/less bus numbers can renumber the buses, motherboard firmware updates could renumber the buses, pci=assign-buses can renumber the buses, etc. This is why the VT-d spec makes use of device paths when describing PCI hierarchies, firmware can't know what bus number will be assigned to a device, but it does know the base bus number and the path of devfns needed to get to it. I don't know how we come up with an option that's easy enough for a user to understand, but reasonably robust against hardware changes. Thanks,

True, but at the same time this feature is for "users with custom hardware designed for purpose", I assume they would be willing to take on the bus renumbering risk. It's already the case that /sys/bus/pci/drivers/<x>/bind takes BDF, which is why it seemed to make a similar interface for the command line. Ideally we could later get something into ACPI or other platform firmware to arrange for bridges to disable ACS by default if we see p2p becoming a common-off-the-shelf feature. I.e. a BIOS switch to enable p2p in a given PCI-E sub-domain.

Quoted 7 lines · On Tue, 8 May 2018 21:42:27 +0000 "Stephen Bates" <[email protected]> wrote:

Hi Alex

Quoted 2 lines

But it would be a much easier proposal to disable ACS when the
IOMMU is not enabled, ACS has no real purpose in that case.

I guess one issue I have with this is that it disables IOMMU groups for all Root Ports and not just the one(s) we wish to do p2pdma on.

But as I understand this series, we're not really targeting specific sets of devices either. It's more of a shotgun approach that we disable ACS on downstream switch ports and hope that we get the right set of devices, but with the indecisiveness that we might later white-list select root ports to further increase the blast radius.

Quoted 12 lines

Quoted 6 lines

The IOMMU and P2P are already not exclusive, we can bounce off
the IOMMU or make use of ATS as we've previously discussed. We were
previously talking about a build time config option that you
didn't expect distros to use, so I don't think intervention for the user to disable the IOMMU if it's enabled by default is a serious concern either.

ATS definitely makes things more interesting for the cases where the EPs support it. However I don't really have a handle on how common ATS support is going to be in the kinds of devices we have been focused on (NVMe SSDs and RDMA NICs mostly).

Quoted 7 lines

Quoted 3 lines

What you're trying to do is enabled direct peer-to-peer for endpoints which do not support ATS when the IOMMU is enabled, which is not something that necessarily makes sense to me.

As above the advantage of leaving the IOMMU on is that it allows for both p2pdma PCI domains and IOMMU groupings PCI domains in the same system. It is just that these domains will be separate to each other.

That argument makes sense if we had the ability to select specific sets of devices, but that's not the case here, right? With the shotgun approach, we're clearly favoring one at the expense of the other and it's not clear why we don't simple force the needle all the way in that direction such that the results are at least predictable.

Quoted 7 lines

Quoted 2 lines

So that leaves avoiding bounce buffers as the remaining IOMMU
feature

I agree with you here that the devices we will want to use for p2p will probably not require a bounce buffer and will support 64 bit DMA addressing.

Quoted 6 lines

Quoted 3 lines

I'm still not seeing why it's terribly undesirable to require devices to support ATS if they want to do direct P2P with an IOMMU enabled.

I think the one reason is for the use-case above. Allowing IOMMU groupings on one domain and p2pdma on another domain....

If IOMMU grouping implies device assignment (because nobody else uses it to the same extent as device assignment) then the build-time option falls to pieces, we need a single kernel that can do both. I think we need to get more clever about allowing the user to specify exactly at which points in the topology they want to disable isolation. Thanks,

Alex

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Hi Don

Quoted 3 lines

RDMA VFs lend themselves to NVMEoF w/device-assignment.... need a way to put NVME 'resources' into an assignable/manageable object for 'IOMMU-grouping', which is really a 'DMA security domain' and less an 'IOMMU grouping domain'.

Ha, I like your term "DMA Security Domain" which sounds about right for what we are discussing with p2pdma and ACS disablement ;-). The problem is that ACS is, in some ways, too big of hammer for what we want here in the sense that it is either on or off for the bridge or MF EP we enable/disable it for. ACS can't filter the TLPs by address or ID though PCI-SIG are having some discussions on extending ACS. That's a long term solution and won't be applicable to us for some time.

NVMe SSDs that support SR-IOV are coming to market but we can't assume all NVMe SSDs with support SR-IOV. That will probably be a pretty high end-feature...

Stephen

Quoted 5 lines · Alex Williamson

Well, I'm a bit confused, this patch series is specifically disabling ACS on switches, but per the spec downstream switch ports implementing ACS MUST implement direct translated P2P. So it seems the only potential gap here is the endpoint, which must support ATS or else there's nothing for direct translated P2P to do. The switch port plays

Quoted 8 lines

no part in the actual translation of the request, ATS on the endpoint has already cached the translation and is now attempting to use it. For the switch port, this only becomes a routing decision, the request is already translated, therefore ACS RR and EC can be ignored to perform "normal" (direct) routing, as if ACS were not present. It would be a shame to go to all the trouble of creating this no-ACS mode to find out the target hardware supports ATS and should have simply used it, or we should have disabled the IOMMU altogether, which leaves ACS disabled.

Ah, ok, I didn't think it was the endpoint that had to implement ATS. But in that case, for our application, we need NVMe cards and RDMA NICs to all have ATS support and I expect that is just as unlikely. At least none of the endpoints on my system support it. Maybe only certain GPUs have this support.

Jerome and Christian

Quoted 4 lines

I think there is confusion here, Alex properly explained the scheme PCIE-device do a ATS request to the IOMMU which returns a valid translation for a virtual address. Device can then use that address directly without going through IOMMU for translation.

So I went through ATS in version 4.0r1 of the PCI spec. It looks like even a ATS translated TLP is still impacted by ACS though it has a separate control knob for translated address TLPs (see 7.7.7.2 of 4.0r1 of the spec). So even if your device supports ATS a P2P DMA will still be routed to the associated RP of the domain and down again unless we disable ACS DT P2P on all bridges between the two devices involved in the P2P DMA.

So we still don't get fine grained control with ATS and I guess we still have security issues because a rogue or malfunctioning EP could just as easily issue TLPs with TA set vs not set.

Quoted 1 line

Also ATS is meaningless without something like PASID as far as i know.

ATS is still somewhat valuable without PSAID in the sense you can cache IOMMU address translations at the EP. This saves hammering on the IOMMU as much in certain workloads.

Interestingly Section 7.7.7.2 almost mentions that Root Ports that support ATS AND can implement P2P between root ports should advertise "ACS Direct Translated P2P (T)" capability. This ties into the discussion around P2P between route ports we had a few weeks ago...

Stephen

Christian

Quoted 2 lines

Interesting point, give me a moment to check that. That finally makes all the hardware I have standing around here valuable :)

Yes. At the very least it provides an initial standards based path for P2P DMAs across RPs which is something we have discussed on this list in the past as being desirable.

BTW I am trying to understand how an ATS capable EP function determines when to perform an ATS Translation Request (ATS TR). Is there an upstream example of the driver for your APU that uses ATS? If so, can you provide a pointer to it. Do you provide some type of entry in the submission queues for commands going to the APU to indicate if the address associated with a specific command should be translated using ATS or not? Or do you simply enable ATS and then all addresses passed to your APU that miss the local cache result in a ATS TR?

Hi Jerome

Quoted 3 lines

Now inside that page table you can point GPU virtual address to use GPU memory or use system memory. Those system memory entry can also be mark as ATS against a given PASID.

Thanks. This all makes sense.

But do you have examples of this in a kernel driver (if so can you point me too it) or is this all done via user-space? Based on my grepping of the kernel code I see zero EP drivers using in-kernel ATS functionality right now...

Hi Jerome

Quoted 3 lines

As it is tie to PASID this is done using IOMMU so looks for caller of amd_iommu_bind_pasid() or intel_svm_bind_mm() in GPU the existing
user is the AMD GPU driver see:

Ah thanks. This cleared things up for me. A quick search shows there are still no users of intel_svm_bind_mm() but I see the AMD version used in that GPU driver.

One thing I could not grok from the code how the GPU driver indicates which DMA events require ATS translations and which do not. I am assuming the driver implements someway of indicating that and its not just a global ON or OFF for all DMAs? The reason I ask is that I looking at if NVMe was to support ATS what would need to be added in the NVMe spec above and beyond what we have in PCI ATS to support efficient use of ATS (for example would we need a flag in the submission queue entries to indicate a particular IO's SGL/PRP should undergo ATS).

Cheers

Stephen

Am 10.05.2018 um 16:20 schrieb Stephen Bates:

Quoted 6 lines

Hi Jerome

Quoted 3 lines

As it is tie to PASID this is done using IOMMU so looks for caller of amd_iommu_bind_pasid() or intel_svm_bind_mm() in GPU the existing
user is the AMD GPU driver see:

Ah thanks. This cleared things up for me. A quick search shows there are still no users of intel_svm_bind_mm() but I see the AMD version used in that GPU driver.

Just FYI: There is also another effort ongoing to give both the AMD, Intel as well as ARM IOMMUs a common interface so that drivers can use whatever the platform offers fro SVM support.

Quoted 1 line

One thing I could not grok from the code how the GPU driver indicates which DMA events require ATS translations and which do not. I am assuming the driver implements someway of indicating that and its not just a global ON or OFF for all DMAs? The reason I ask is that I looking at if NVMe was to support ATS what would need to be added in the NVMe spec above and beyond what we have in PCI ATS to support efficient use of ATS (for example would we need a flag in the submission queue entries to indicate a particular IO's SGL/PRP should undergo ATS).

Oh, well that is complicated at best.

On very old hardware it wasn't a window, but instead you had to use special commands in your shader which indicated that you want to use an ATS transaction instead of a normal PCIe transaction for your read/write/atomic.

As Jerome explained on most hardware we have a window inside the internal GPU address space which when accessed issues a ATS transaction with a configurable PASID.

But on very newer hardware that window became a bit in the GPUVM page tables, so in theory we now can control it on a 4K granularity basis for the internal 48bit GPU address space.

The key takeaway is that when any device has ATS enabled you can't disable ACS without breaking it (even if you unplug and replug it).

Quoted 2 lines

The key takeaway is that when any device has ATS enabled you can't disable ACS without breaking it (even if you unplug and replug it).

I don't follow how you came to this conclusion...
The ACS bits we'd be turning off are the ones that force TLPs addressed
at a peer to go to the RC. However, ATS translation packets will be addressed to an untranslated address which a switch will not identify as a peer address so it should send upstream regardless the state of the ACS Req/Comp redirect bits.

Once the translation comes back, the ATS endpoint should send the TLP to the peer address with the AT packet type and it will be directed to the peer provided the Direct Translated bit is set (or the redirect bits are unset).

I can't see how turning off the Req/Comp redirect bits could break anything except for the isolation they provide.

Logan

Hi Christian

Quoted 3 lines

Why would a switch not identify that as a peer address? We use the PASID
together with ATS to identify the address space which a transaction should use.

I think you are conflating two types of TLPs here. If the device supports ATS then it will issue a TR TLP to obtain a translated address from the IOMMU. This TR TLP will be addressed to the RP and so regardless of ACS it is going up to the Root Port. When it gets the response it gets the physical address and can use that with the TA bit set for the p2pdma. In the case of ATS support we also have more control over ACS as we can disable it just for TA addresses (as per 7.7.7.7.2 of the spec).

Quoted 3 lines

If I'm not completely mistaken when you disable ACS it is perfectly possible that a bridge identifies a transaction as belonging to a peer address, which isn't what we want here.

You are right here and I think this illustrates a problem for using the IOMMU at all when P2PDMA devices do not support ATS. Let me explain:

If we want to do a P2PDMA and the DMA device does not support ATS then I think we have to disable the IOMMU (something Mike suggested earlier). The reason is that since ATS is not an option the EP must initiate the DMA using the addresses passed down to it. If the IOMMU is on then this is an IOVA that could (with some non-zero probability) point to an IO Memory address in the same PCI domain. So if we disable ACS we are in trouble as we might MemWr to the wrong place but if we enable ACS we lose much of the benefit of P2PDMA. Disabling the IOMMU removes the IOVA risk and ironically also resolves the IOMMU grouping issues.

So I think if we want to support performant P2PDMA for devices that don't have ATS (and no NVMe SSDs today support ATS) then we have to disable the IOMMU. I know this is problematic for AMDs use case so perhaps we also need to consider a mode for P2PDMA for devices that DO support ATS where we can enable the IOMMU (but in this case EPs without ATS cannot participate as P2PDMA DMA iniators).

Make sense?

Hi Jerome

Quoted 3 lines

Note on GPU we do would not rely on ATS for peer to peer. Some part of the GPU (DMA engines) do not necessarily support ATS. Yet those are the part likely to be use in peer to peer.

OK this is good to know. I agree the DMA engine is probably one of the GPU components most applicable to p2pdma.

Quoted 4 lines

We (ake GPU people aka the good guys ;)) do no want to do peer to peer for performance reasons ie we do not care having our transaction going to the root complex and back down the destination. At least in use case i am working on this is fine.

If the GPU people are the good guys does that make the NVMe people the bad guys ;-). If so, what are the RDMA people??? Again good to know.

Quoted 1 line

Reasons is that GPU are giving up on PCIe (see all specialize link l=

Quoted 1 line

NVlink that are popping up in GPU space). So for fast GPU inter-conn=

Quoted 1 line

we have this new links. =20

=20
I look forward to Nvidia open-licensing NVLink to anyone who wants to use=

Quoted 1 line

Also the IOMMU isolation do matter a lot to us. Think someone using =

Quoted 1 line

peer to peer to gain control of a server in the cloud. =20

Quoted 4 lines · On Thu, 10 May 2018 18:41:09 +0000 "Stephen Bates" <[email protected]> wrote:

Quoted 3 lines

Reasons is that GPU are giving up on PCIe (see all specialize link like NVlink that are popping up in GPU space). So for fast GPU inter-connect we have this new links.

Quoted 1 line

I look forward to Nvidia open-licensing NVLink to anyone who wants to use it ;-).

No doubt, the marketing for it is quick to point out the mesh topology of NVLink, but I haven't seen any technical documents that describe the isolation capabilities or IOMMU interaction. Whether this is included or an afterthought, I have no idea.

Quoted 3 lines

Quoted 3 lines

Also the IOMMU isolation do matter a lot to us. Think someone using this peer to peer to gain control of a server in the cloud.

From that perspective, do we have any idea what NVLink means for topology and IOMMU provided isolation and translation? I've seen a device assignment user report that seems to suggest it might pretend to be PCIe compatible, but the assigned GPU ultimately doesn't work correctly in a VM, so perhaps the software compatibility is only so deep. Thanks,

Hi Christian

Quoted 3 lines

Why would a switch not identify that as a peer address? We use the PASID
together with ATS to identify the address space which a transaction should use.

I think you are conflating two types of TLPs here. If the device supports ATS then it will issue a TR TLP to obtain a translated address from the IOMMU. This TR TLP will be addressed to the RP and so regardless of ACS it is going up to the Root Port. When it gets the response it gets the physical address and can use that with the TA bit set for the p2pdma. In the case of ATS support we also have more control over ACS as we can disable it just for TA addresses (as per 7.7.7.7.2 of the spec).

Quoted 3 lines

If I'm not completely mistaken when you disable ACS it is perfectly possible that a bridge identifies a transaction as belonging to a peer address, which isn't what we want here.

You are right here and I think this illustrates a problem for using the IOMMU at all when P2PDMA devices do not support ATS. Let me explain:

If we want to do a P2PDMA and the DMA device does not support ATS then I think we have to disable the IOMMU (something Mike suggested earlier). The reason is that since ATS is not an option the EP must initiate the DMA using the addresses passed down to it. If the IOMMU is on then this is an IOVA that could (with some non-zero probability) point to an IO Memory address in the same PCI domain. So if we disable ACS we are in trouble as we might MemWr to the wrong place but if we enable ACS we lose much of the benefit of P2PDMA. Disabling the IOMMU removes the IOVA risk and ironically also resolves the IOMMU grouping issues. So I think if we want to support performant P2PDMA for devices that don't have ATS (and no NVMe SSDs today support ATS) then we have to disable the IOMMU. I know this is problematic for AMDs use case so perhaps we also need to consider a mode for P2PDMA for devices that DO support ATS where we can enable the IOMMU (but in this case EPs without ATS cannot participate as P2PDMA DMA iniators).

Make sense?

Quoted 2 lines

Not to me. In the p2pdma code we specifically program DMA engines with the PCI bus address.

Ah yes of course. Brain fart on my part. We are not programming the P2PDMA initiator with an IOVA but with the PCI bus address...

Quoted 5 lines

So regardless of whether we are using the IOMMU or
not, the packets will be forwarded directly to the peer. If the ACS
Redir bits are on they will be forced back to the RC by the switch and the transaction will fail. If we clear the ACS bits, the TLPs will go where we want and everything will work (but we lose the isolation of ACS).

Agreed.

Quoted 7 lines

For EPs that support ATS, we should (but don't necessarily have to) program them with the IOVA address so they can go through the translation process which will allow P2P without disabling the ACS Redir bits -- provided the ACS direct translation bit is set. (And btw, if it is, then we lose the benefit of ACS protecting against malicious EPs). But, per above, the ATS transaction should involve only the IOVA address so the ACS bits not being set should not break ATS.

Well we would still have to clear some ACS bits but now we can clear only for translated addresses.

Quoted 3 lines

Quoted 2 lines

Not to me. In the p2pdma code we specifically program DMA engines with the PCI bus address.

Ah yes of course. Brain fart on my part. We are not programming the P2PDMA initiator with an IOVA but with the PCI bus address...

Quoted 7 lines

Quoted 6 lines

So regardless of whether we are using the IOMMU or
not, the packets will be forwarded directly to the peer. If the ACS
Redir bits are on they will be forced back to the RC by the switch and the transaction will fail. If we clear the ACS bits, the TLPs will go where we want and everything will work (but we lose the isolation of ACS).

Agreed.

Quoted 9 lines

Quoted 7 lines

For EPs that support ATS, we should (but don't necessarily have to) program them with the IOVA address so they can go through the translation process which will allow P2P without disabling the ACS Redir bits -- provided the ACS direct translation bit is set. (And btw, if it is, then we lose the benefit of ACS protecting against malicious EPs). But, per above, the ATS transaction should involve only the IOVA address so the ACS bits not being set should not break ATS.

Well we would still have to clear some ACS bits but now we can clear only for translated addresses.

We don't have to clear the ACS Redir bits as we did in the first case. We just have to make sure the ACS Direct Translated bit is set.

Logan

This only works when the IOVA and the PCI bus addresses never overlap. I'm not sure how the IOVA allocation works but I don't think we guarantee that on Linux.

If we really want to enable P2P without ATS and IOMMU enabled I think we should probably approach it like this:

a) Make double sure that IOVA in an IOMMU group never overlap with PCI BARs in that group.

b) Add configuration options to put a whole PCI branch of devices (e.g. a bridge) into a single IOMMU group.

c) Add a configuration option to disable the ACS bit on bridges in the same IOMMU group.

=A0Alex (or anyone else) can you point to where IOVA addresses are generat=

All

Quoted 1 line

?Alex (or anyone else) can you point to where IOVA addresses are generated?

A case of RTFM perhaps (though a pointer to the code would still be appreciated).

https://www.kernel.org/doc/Documentation/Intel-IOMMU.txt

Some exceptions to IOVA
-----------------------
Interrupt ranges are not address translated, (0xfee00000 - 0xfeefffff). The same is true for peer to peer transactions. Hence we reserve the address from PCI MMIO ranges so they are not allocated for IOVA addresses.

For peer-to-peer transactions to work the downstream ports in each switch must not have the ACS flags set. At this time there is no way to dynamically change the flags and update the corresponding IOMMU groups so this is done at enumeration time before the groups are assigned.

This effectively means that if CONFIG_PCI_P2PDMA is selected then all devices behind any PCIe switch heirarchy will be in the same IOMMU group. Which implies that individual devices behind any switch heirarchy will not be able to be assigned to separate VMs because there is no isolation between them. Additionally, any malicious PCIe devices will be able to DMA to memory exposed by other EPs in the same domain as TLPs will not be checked by the IOMMU.

Given that the intended use case of P2P Memory is for users with custom hardware designed for purpose, we do not expect distributors to ever need to enable this option. Users that want to use P2P must have compiled a custom kernel with this configuration option and understand the implications regarding ACS. They will either not require ACS or will have design the system in such a way that devices that require isolation will be separate from those using P2P transactions.

Signed-off-by Logan Gunthorpe <[email protected]>

---
 drivers/pci/Kconfig        |  9 +++++++++
 drivers/pci/p2pdma.c       | 45 ++++++++++++++++++++++++++++++---------------
 drivers/pci/pci.c          |  6 ++++++
 include/linux/pci-p2pdma.h |  5 +++++
 4 files changed, 50 insertions(+), 15 deletions(-)

drivers/pci/Kconfig +0 -0

index b2396c22b53e..b6db41d4b708 100644
@@ -139,6 +139,15 @@ config PCI_P2PDMA
          transations must be between devices behind the same root port.
          (Typically behind a network of PCIe switches).
 

Hi Dan

Quoted 3 lines

It seems unwieldy that this is a compile time option and not a runtime option. Can't we have a kernel command line option to opt-in to this behavior rather than require a wholly separate kernel image?

I think because of the security implications associated with p2pdma and ACS we wanted to make it very clear people were choosing one (p2pdma) or the other (IOMMU groupings and isolation). However personally I would prefer including the option of a run-time kernel parameter too. In fact a few months ago I proposed a small patch that did just that [1]. It never really went anywhere but if people were open to the idea we could look at adding it to the series.

Quoted 2 lines

Why is this text added in a follow on patch and not the patch that
introduced the config option?

Because the ACS section was added later in the series and this information is associated with that additional functionality.

Quoted 3 lines

I'm also wondering if that command line option can take a 'bus device function' address of a switch to limit the scope of where ACS is disabled.

By this you mean the address for either a RP, DSP, USP or MF EP below which we disable ACS? We could do that but I don't think it avoids the issue of changes in IOMMU groupings as devices are added/removed. It simply changes the problem from affecting and entire PCI domain to a sub-set of the domain. We can already handle this by doing p2pdma on one RP and normal IOMMU isolation on the other RPs in the system.

Stephen

[1] https://marc.info/?l=linux-doc&m=150907188310838&w=2

Hi Don

Quoted 3 lines

Well, p2p DMA is a function of a cooperating 'agent' somewhere above the two devices.
That agent should 'request' to the kernel that ACS be removed/circumvented (p2p enabled) btwn two endpoints. I recommend doing so via a sysfs method.

Yes we looked at something like this in the past but it does hit the IOMMU grouping issue I discussed earlier today which is not acceptable right now. In the long term, once we get IOMMU grouping callbacks to VMs we can look at extending p2pdma in this way. But I don't think this is viable for the initial series.

Quoted 1 line

So I don't understand the comments why VMs should need to know.

As I understand it VMs need to know because VFIO passes IOMMU grouping up into the VMs. So if a IOMMU grouping changes the VM's view of its PCIe topology changes. I think we even have to be cognizant of the fact the OS running on the VM may not even support hot-plug of PCI devices.

Quoted 2 lines

Is there a thread I need to read up to explain /clear-up the thoughts above?

If you search for p2pdma you should find the previous discussions. Thanks for the input!

Stephen

Quoted 5 lines · Stephen Bates

As I understand it VMs need to know because VFIO passes IOMMU grouping up into the VMs. So if a IOMMU grouping changes the VM's view of its PCIe topology changes. I think we even have to be cognizant of the fact the OS running on the VM may not even support hot-plug of PCI devices.

Alex:
Really? IOMMU groups are created by the kernel, so don't know how they would be passed into the VMs, unless indirectly via PCI(e) layout. At best, twiddling w/ACS enablement (emulation) would cause VMs to see different IOMMU groups, but again, VMs are not the security point/level, the host/HV's are.

Hi Alex and Don

Quoted 2 lines

Correct, the VM has no concept of the host's IOMMU groups, only
the hypervisor knows about the groups,

But as I understand it these groups are usually passed through to VMs on a pre-group basis by the hypervisor? So IOMMU group 1 might be passed to VM A and IOMMU group 2 passed to VM B. So I agree the VM is not aware of IOMMU groupings but it is impacted by them in the sense that if the groupings change the PCI topology presented to the VM needs to change too.

Quoted 11 lines · On Wed, 9 May 2018 12:35:56 +0000 "Stephen Bates" <[email protected]> wrote:

Hi Alex and Don

Quoted 2 lines

Correct, the VM has no concept of the host's IOMMU groups, only
the hypervisor knows about the groups,

But as I understand it these groups are usually passed through to VMs on a pre-group basis by the hypervisor? So IOMMU group 1 might be passed to VM A and IOMMU group 2 passed to VM B. So I agree the VM is not aware of IOMMU groupings but it is impacted by them in the sense that if the groupings change the PCI topology presented to the VM needs to change too.

Hypervisors don't currently expose any topology based on the grouping, the only case where such a concept even makes sense is when a vIOMMU is present as devices within the same group cannot have separate address spaces. Our options for exposing such information is also limited, our only real option would seem to be placing devices within the same group together on a conventional PCI bus to denote the address space granularity. Currently we strongly recommend singleton groups for this case and leave any additional configuration constraints to the admin.

The case you note of a group passed to VM A and another passed to VM B is exactly an example of why any sort of dynamic routing change needs to have the groups fully released, such as via hot-unplug. For instance, a routing change at a shared node above groups 1 & 2 could result in the merging of these groups and there is absolutely no way to handle that with portions of the group being owned by two separate VMs after the merge. Thanks,

Alex

Quoted 6 lines · On Tue, 8 May 2018 19:06:17 -0400 Don Dutile <[email protected]> wrote:

Quoted 5 lines · Stephen Bates

As I understand it VMs need to know because VFIO passes IOMMU grouping up into the VMs. So if a IOMMU grouping changes the VM's view of its PCIe topology changes. I think we even have to be cognizant of the fact the OS running on the VM may not even support hot-plug of PCI devices.

Quoted 6 lines

Alex:
Really? IOMMU groups are created by the kernel, so don't know how they would be passed into the VMs, unless indirectly via PCI(e) layout. At best, twiddling w/ACS enablement (emulation) would cause VMs to see different IOMMU groups, but again, VMs are not the security point/level, the host/HV's are.

Correct, the VM has no concept of the host's IOMMU groups, only the hypervisor knows about the groups, but really only to the extent of which device belongs to which group and whether the group is viable. Any runtime change to grouping though would require DMA mapping updates, which I don't see how we can reasonably do with drivers, vfio-pci or native host drivers, bound to the affected devices. Thanks,

Alex

Hi Don

Quoted 3 lines

Well, p2p DMA is a function of a cooperating 'agent' somewhere above the two devices.
That agent should 'request' to the kernel that ACS be removed/circumvented (p2p enabled) btwn two endpoints. I recommend doing so via a sysfs method.

Yes we looked at something like this in the past but it does hit the IOMMU grouping issue I discussed earlier today which is not acceptable right now. In the long term, once we get IOMMU grouping callbacks to VMs we can look at extending p2pdma in this way. But I don't think this is viable for the initial series.

Quoted 1 line

So I don't understand the comments why VMs should need to know.

As I understand it VMs need to know because VFIO passes IOMMU grouping up into the VMs. So if a IOMMU grouping changes the VM's view of its PCIe topology changes. I think we even have to be cognizant of the fact the OS running on the VM may not even support hot-plug of PCI devices.

Quoted 1 line

Is there a thread I need to read up to explain /clear-up the thoughts above?

If you search for p2pdma you should find the previous discussions. Thanks for the input!

Stephen

Add a restructured text file describing how to write drivers with support for P2P DMA transactions. The document describes how to use the APIs that were added in the previous few commits.

Also adds an index for the PCI documentation tree even though this is the only PCI document that has been converted to restructured text at this time.

Signed-off-by Logan Gunthorpe <[email protected]>

Cc Jonathan Corbet <[email protected]>

---
 Documentation/PCI/index.rst             |  14 +++
 Documentation/driver-api/pci/index.rst  |   1 +
 Documentation/driver-api/pci/p2pdma.rst | 166 ++++++++++++++++++++++++++++++++
 Documentation/index.rst                 |   3 +-
 4 files changed, 183 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/PCI/index.rst
 create mode 100644 Documentation/driver-api/pci/p2pdma.rst

Documentation/PCI/index.rst new file +14 -0

new file mode 100644
index 000000000000..2fdc4b3c291d
@@ -0,0 +1,14 @@
+==================================
+Linux PCI Driver Developer's Guide
+==================================
+
+.. toctree::
+
+   p2pdma
+
+.. only::  subproject and html
+
+   Indices
+   =======
+
+   * :ref:`genindex`

Documentation/driver-api/pci/index.rst +1 -0

index 03b57cbf8cc2..d12eeafbfc90 100644
@@ -10,6 +10,7 @@ The Linux PCI driver implementer's API guide
    :maxdepth: 2
 
    pci
+   p2pdma
 
 .. only::  subproject and html
 

Documentation/driver-api/pci/p2pdma.rst new file +0 -0

new file mode 100644
index 000000000000..49a512c405b2

Documentation/driver-api/pci/p2pdma.rst new file +6 -0

+
+The second issue is that to make use of existing interfaces in Linux,
+memory that is used for P2P transactions needs to be backed by struct
+pages. However, PCI BARs are not typically cache coherent so there are
+a few corner case gotchas with these pages so developers need to
+be careful about what they do with them.

Add a restructured text file describing how to write drivers with support for P2P DMA transactions. The document describes how to use the APIs that were added in the previous few commits.

Also adds an index for the PCI documentation tree even though this is the only PCI document that has been converted to restructured text at this time.

Signed-off-by Logan Gunthorpe <[email protected]>

Cc Jonathan Corbet <[email protected]>

---
 Documentation/PCI/index.rst             |  14 +++
 Documentation/driver-api/pci/index.rst  |   1 +
 Documentation/driver-api/pci/p2pdma.rst | 166 ++++++++++++++++++++++++++++++++
 Documentation/index.rst                 |   3 +-
 4 files changed, 183 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/PCI/index.rst
 create mode 100644 Documentation/driver-api/pci/p2pdma.rst

Documentation/driver-api/pci/p2pdma.rst new file +22 -0

new file mode 100644
index 000000000000..49a512c405b2
@@ -0,0 +1,166 @@
+============================
+PCI Peer-to-Peer DMA Support
+============================
+
+The PCI bus has pretty decent support for performing DMA transfers
+between two endpoints on the bus. This type of transaction is
+henceforth called Peer-to-Peer (or P2P). However, there are a number of
+issues that make P2P transactions tricky to do in a perfectly safe way.
+
+One of the biggest issues is that PCI Root Complexes are not required
+to support forwarding packets between Root Ports. To make things worse,
+there is no simple way to determine if a given Root Complex supports
+this or not. (See PCIe r4.0, sec 1.3.1). Therefore, as of this writing,
+the kernel only supports doing P2P when the endpoints involved are all
+behind the same PCIe root port as the spec guarantees that all
+packets will always be routable but does not require routing between
+root ports.
+
+The second issue is that to make use of existing interfaces in Linux,
+memory that is used for P2P transactions needs to be backed by struct
+pages. However, PCI BARs are not typically cache coherent so there are
+a few corner case gotchas with these pages so developers need to

Documentation/driver-api/pci/p2pdma.rst new file +53 -0

+it may be typical for a driver to be both a provider and a client).
+
+For example, in the NVMe Target Copy Offload implementation:
+
+* The NVMe PCI driver is both a client, provider and orchestrator
+  in that it exposes any CMB (Controller Memory Buffer) as a P2P memory
+  resource (provider), it accepts P2P memory pages as buffers in requests
+  to be used directly (client) and it can also make use the CMB as
+  submission queue entries.
+* The RDMA driver is a client in this arrangement so that an RNIC
+  can DMA directly to the memory exposed by the NVMe device.
+* The NVMe Target driver (nvmet) can orchestrate the data from the RNIC
+  to the P2P memory (CMB) and then to the NVMe device (and vice versa).
+
+This is currently the only arrangement supported by the kernel but
+one could imagine slight tweaks to this that would allow for the same
+functionality. For example, if a specific RNIC added a BAR with some
+memory behind it, its driver could add support as a P2P provider and
+then the NVMe Target could use the RNIC's memory instead of the CMB
+in cases where the NVMe cards in use do not have CMB support.
+
+
+Provider Drivers
+----------------
+
+A provider simply needs to register a BAR (or a portion of a BAR)
+as a P2P DMA resource using :c:func:`pci_p2pdma_add_resource()`.
+This will register struct pages for all the specified memory.
+
+After that it may optionally publish all of its resources as
+P2P memory using :c:func:`pci_p2pmem_publish()`. This will allow
+any orchestrator drivers to find and use the memory. When marked in
+this way, the resource must be regular memory with no side effects.
+
+For the time being this is fairly rudimentary in that all resources
+are typically going to be P2P memory. Future work will likely expand
+this to include other types of resources like doorbells.
+
+
+Client Drivers
+--------------
+
+A client driver typically only has to conditionally change its DMA map
+routine to use the mapping functions :c:func:`pci_p2pdma_map_sg()` and
+:c:func:`pci_p2pdma_unmap_sg()` instead of the usual :c:func:`dma_map_sg()`
+functions.
+
+The client may also, optionally, make use of
+:c:func:`is_pci_p2pdma_page()` to determine when to use the P2P mapping
+functions and when to use the regular mapping functions. In some
+situations, it may be more appropriate to use a flag to indicate a
+given request is P2P memory and map appropriately (for example the
+block layer uses a flag to keep P2P memory out of queues that do not

Documentation/driver-api/pci/p2pdma.rst new file +4 -0

+away, the one returned will be chosen at random. This function returns the PCI
+device to use for the provider with a reference taken and therefore
+when it's no longer needed it should be returned with pci_dev_put().
+

Documentation/driver-api/pci/p2pdma.rst new file +38 -0

+
+Once a provider is assigned to a client list via either
+:c:func:`pci_p2pmem_find()` or :c:func:`pci_p2pdma_assign_provider()`,
+the list is permanently bound to the provider such that any new clients
+added to the list must be supported by the already selected provider.
+If they are not supported, :c:func:`pci_p2pdma_add_client()` will return
+an error. In this way, orchestrators are free to add and remove devices
+without having to recheck support or tear down existing transfers to
+change P2P providers.
+
+Once a provider is selected, the orchestrator can then use
+:c:func:`pci_alloc_p2pmem()` and :c:func:`pci_free_p2pmem()` to
+allocate P2P memory from the provider. :c:func:`pci_p2pmem_alloc_sgl()`
+and :c:func:`pci_p2pmem_free_sgl()` are convenience functions for
+allocating scatter-gather lists with P2P memory.
+
+Struct Page Caveats
+-------------------
+
+Driver writers should be very careful about not passing these special
+struct pages to code that isn't prepared for it. At this time, the kernel
+interfaces do not have any checks for ensuring this. This obviously
+precludes passing these pages to userspace.
+
+P2P memory is also technically IO memory but should never have any side
+effects behind it. Thus, the order of loads and stores should not be important
+and ioreadX(), iowriteX() and friends should not be necessary.
+However, as the memory is not cache coherent, if access ever needs to
+be protected by a spinlock then :c:func:`mmiowb()` must be used before
+unlocking the lock. (See ACQUIRES VS I/O ACCESSES in
+Documentation/memory-barriers.txt)
+
+
+P2P DMA Support Library
+=====================
+
+.. kernel-doc:: drivers/pci/p2pdma.c
+   :export:

Documentation/driver-api/pci/p2pdma.rst line 102

Quoted 3 lines

Documentation/driver-api/pci/p2pdma.rst new file +3 -0

+The first task an orchestrator driver must do is compile a list of
+all client drivers that will be involved in a given transaction. For
+example, the NVMe Target driver creates a list including all NVMe drives

^^^^^^
or drivers ?
Could be either, I guess, but the previous sentence says "compile a list of drivers."

Hi Everyone,

Here's v4 of our series to introduce P2P based copy offload to NVMe fabrics. This version has been rebased onto v4.17-rc2. A git repo is here:

https://github.com/sbates130272/linux-p2pmem pci-p2p-v4

Thanks,

Logan

Changes in v4:

* Change the original upstream_bridges_match() function to upstream_bridge_distance() which calculates the distance between two devices as long as they are behind the same root port. This should address Bjorn's concerns that the code was to focused on being behind a single switch.

* The disable ACS function now disables ACS for all bridge ports instead of switch ports (ie. those that had two upstream_bridge ports).

* Change the pci_p2pmem_alloc_sgl() and pci_p2pmem_free_sgl() API to be more like sgl_alloc() in that the alloc function returns the allocated scatterlist and nents is not required bythe free function.

* Moved the new documentation into the driver-api tree as requested by Jonathan

* Add SGL alloc and free helpers in the nvmet code so that the individual drivers can share the code that allocates P2P memory. As requested by Christoph.

* Cleanup the nvmet_p2pmem_store() function as Christoph thought my first attempt was ugly.

* Numerous commit message and comment fix-ups

Changes in v3:

* Many more fixes and minor cleanups that were spotted by Bjorn

* Additional explanation of the ACS change in both the commit message and Kconfig doc. Also, the code that disables the ACS bits is surrounded explicitly by an #ifdef

* Removed the flag we added to rdma_rw_ctx() in favour of using is_pci_p2pdma_page(), as suggested by Sagi.

* Adjust pci_p2pmem_find() so that it prefers P2P providers that are closest to (or the same as) the clients using them. In cases of ties, the provider is randomly chosen.

* Modify the NVMe Target code so that the PCI device name of the provider may be explicitly specified, bypassing the logic in pci_p2pmem_find(). (Note: it's still enforced that the provider must be behind the same switch as the clients).

* As requested by Bjorn, added documentation for driver writers.

Changes in v2:

* Renamed everything to 'p2pdma' per the suggestion from Bjorn as well as a bunch of cleanup and spelling fixes he pointed out in the last series.

* To address Alex's ACS concerns, we change to a simpler method of just disabling ACS behind switches for any kernel that has CONFIG_PCI_P2PDMA.

* We also reject using devices that employ 'dma_virt_ops' which should fairly simply handle Jason's concerns that this work might break with the HFI, QIB and rxe drivers that use the virtual ops to implement their own special DMA operations.

--

This is a continuation of our work to enable using Peer-to-Peer PCI memory in the kernel with initial support for the NVMe fabrics target subsystem. Many thanks go to Christoph Hellwig who provided valuable feedback to get these patches to where they are today.

The concept here is to use memory that's exposed on a PCI BAR as data buffers in the NVMe target code such that data can be transferred from an RDMA NIC to the special memory and then directly to an NVMe device avoiding system memory entirely. The upside of this is better QoS for applications running on the CPU utilizing memory and lower PCI bandwidth required to the CPU (such that systems could be designed with fewer lanes connected to the CPU).

Due to these trade-offs we've designed the system to only enable using the PCI memory in cases where the NIC, NVMe devices and memory are all behind the same PCI switch hierarchy. This will mean many setups that could likely work well will not be supported so that we can be more confident it will work and not place any responsibility on the user to understand their topology. (We chose to go this route based on feedback we received at the last LSF). Future work may enable these transfers using a white list of known good root complexes. However, at this time, there is no reliable way to ensure that Peer-to-Peer transactions are permitted between PCI Root Ports.

In order to enable this functionality, we introduce a few new PCI functions such that a driver can register P2P memory with the system. Struct pages are created for this memory using devm_memremap_pages() and the PCI bus offset is stored in the corresponding pagemap structure.

When the PCI P2PDMA config option is selected the ACS bits in every bridge port in the system are turned off to allow traffic to pass freely behind the root port. At this time, the bit must be disabled at boot so the IOMMU subsystem can correctly create the groups, though this could be addressed in the future. There is no way to dynamically disable the bit and alter the groups.

Another set of functions allow a client driver to create a list of client devices that will be used in a given P2P transactions and then use that list to find any P2P memory that is supported by all the client devices.

In the block layer, we also introduce a P2P request flag to indicate a given request targets P2P memory as well as a flag for a request queue to indicate a given queue supports targeting P2P memory. P2P requests will only be accepted by queues that support it. Also, P2P requests are marked to not be merged seeing a non-homogenous request would complicate the DMA mapping requirements.

In the PCI NVMe driver, we modify the existing CMB support to utilize the new PCI P2P memory infrastructure and also add support for P2P memory in its request queue. When a P2P request is received it uses the pci_p2pmem_map_sg() function which applies the necessary transformation to get the corrent pci_bus_addr_t for the DMA transactions.

In the RDMA core, we also adjust rdma_rw_ctx_init() and rdma_rw_ctx_destroy() to take a flags argument which indicates whether to use the PCI P2P mapping functions or not. To avoid odd RDMA devices that don't use the proper DMA infrastructure this code rejects using any device that employs the virt_dma_ops implementation.

Finally, in the NVMe fabrics target port we introduce a new configuration boolean: 'allow_p2pmem'. When set, the port will attempt to find P2P memory supported by the RDMA NIC and all namespaces. If supported memory is found, it will be used in all IO transfers. And if a port is using P2P memory, adding new namespaces that are not supported by that memory will fail.

These patches have been tested on a number of Intel based systems and for a variety of RDMA NICs (Mellanox, Broadcomm, Chelsio) and NVMe SSDs (Intel, Seagate, Samsung) and p2pdma devices (Eideticom, Microsemi, Chelsio and Everspin) using switches from both Microsemi and Broadcomm.

Logan Gunthorpe (14):
PCI/P2PDMA: Support peer-to-peer memory
PCI/P2PDMA: Add sysfs group to display p2pmem stats PCI/P2PDMA: Add PCI p2pmem dma mappings to adjust the bus offset PCI/P2PDMA: Clear ACS P2P flags for all devices behind switches
docs-rst: Add a new directory for PCI documentation PCI/P2PDMA: Add P2P DMA driver writer's documentation
block: Introduce PCI P2P flags for request and request queue IB/core: Ensure we map P2P memory correctly in

     rdma_rw_ctx_[init|destroy]()

nvme-pci: Use PCI p2pmem subsystem to manage the CMB
nvme-pci: Add support for P2P memory in requests
nvme-pci: Add a quirk for a pseudo CMB
nvmet: Introduce helper functions to allocate and free request SGLs
nvmet-rdma: Use new SGL alloc/free helper for requests
nvmet: Optionally use PCI P2P memory

  Documentation/ABI/testing/sysfs-bus-pci    |  25 +
  Documentation/PCI/index.rst                |  14 +
  Documentation/driver-api/index.rst         |   2 +-
  Documentation/driver-api/pci/index.rst     |  20 +
  Documentation/driver-api/pci/p2pdma.rst    | 166 ++++++
  Documentation/driver-api/{ => pci}/pci.rst |   0
  Documentation/index.rst                    |   3 +-
  block/blk-core.c                           |   3 +
  drivers/infiniband/core/rw.c               |  13 +-
  drivers/nvme/host/core.c                   |   4 +
  drivers/nvme/host/nvme.h                   |   8 +
  drivers/nvme/host/pci.c                    | 118 +++--
  drivers/nvme/target/configfs.c             |  67 +++
  drivers/nvme/target/core.c                 | 143 ++++-
  drivers/nvme/target/io-cmd.c               |   3 +
  drivers/nvme/target/nvmet.h                |  15 +
  drivers/nvme/target/rdma.c                 |  22 +-
  drivers/pci/Kconfig                        |  26 +
  drivers/pci/Makefile                       |   1 +
  drivers/pci/p2pdma.c                       | 814 +++++++++++++++++++++++++++++
  drivers/pci/pci.c                          |   6 +
  include/linux/blk_types.h                  |  18 +-
  include/linux/blkdev.h                     |   3 +
  include/linux/memremap.h                   |  19 +
  include/linux/pci-p2pdma.h                 | 118 +++++
  include/linux/pci.h                        |   4 +
  26 files changed, 1579 insertions(+), 56 deletions(-)

create mode 100644 Documentation/PCI/index.rst create mode 100644 Documentation/driver-api/pci/index.rst create mode 100644 Documentation/driver-api/pci/p2pdma.rst rename Documentation/driver-api/{ => pci}/pci.rst (100%) create mode 100644 drivers/pci/p2pdma.c
create mode 100644 include/linux/pci-p2pdma.h

--
2.11.0

it would be rather nice to have if you could separate out the functions to detect if peer2peer is possible between two devices.

Hi Christian,

Quoted 1 line · Christian König

it would be rather nice to have if you could separate out the functions to detect if peer2peer is possible between two devices.

This would essentially be pci_p2pdma_distance() in the existing patchset. It returns the sum of the distance between a list of clients and a P2PDMA provider. It returns -1 if peer2peer is not possible between the devices (presently this means they are not behind the same root port).

Logan

Ok, I'm still missing the big picture here. First question is what is the P2PDMA provider?

Second question is how to you want to handle things when device are not behind the same root port (which is perfectly possible in the cases I deal with)?

Third question why multiple clients? That feels a bit like you are pushing something special to your use case into the common PCI subsystem. Something which usually isn't a good idea.

As far as I can see we need a function which return the distance between a initiator and target device. This function then returns -1 if the transaction can't be made and a positive value otherwise.

We also need to give the direction of the transaction and have a whitelist root complex PCI-IDs which can handle P2P transactions from different ports for a certain DMA direction.

Quoted 3 lines

Second question is how to you want to handle things when device are not behind the same root port (which is perfectly possible in the cases I deal with)?

I think we need to implement a whitelist. If both root ports are in the white list and are on the same bus then we return a larger distance instead of -1.

Quoted 3 lines

Third question why multiple clients? That feels a bit like you are pushing something special to your use case into the common PCI subsystem. Something which usually isn't a good idea.

No, I think this will be pretty standard. In the simple general case you are going to have one provider and at least two clients (one which writes the memory and one which reads it). However, one client is likely, but not necessarily, the same as the provider.

In the NVMeof case, we might have N clients: 1 RDMA device and N-1 block devices. The code doesn't care which device provides the memory as it could be the RDMA device or one/all of the block devices (or, in theory, a completely separate device with P2P-able memory). However, it does require that all devices involved are accessible per pci_p2pdma_distance() or it won't use P2P transactions.

I could also imagine other use cases: ie. an RDMA NIC sends data to a GPU for processing and then sends the data to an NVMe device for storage (or vice-versa). In this case we have 3 clients and one provider.

Quoted 3 lines

As far as I can see we need a function which return the distance between a initiator and target device. This function then returns -1 if the transaction can't be made and a positive value otherwise.

If you need to make a simpler convenience function for your use case I'm not against it.

Quoted 4 lines

We also need to give the direction of the transaction and have a whitelist root complex PCI-IDs which can handle P2P transactions from different ports for a certain DMA direction.

Yes. In the NVMeof case we need all devices to be able to DMA in both directions so we did not need the DMA direction. But I can see this being useful once we add the whitelist.

Logan

Ok, that is the point where I'm stuck. Why do we need that in one function call in the PCIe subsystem?

The problem at least with GPUs is that we seriously don't have that information here, cause the PCI subsystem might not be aware of all the interconnections.

For example it isn't uncommon to put multiple GPUs on one board. To the PCI subsystem that looks like separate devices, but in reality all GPUs are interconnected and can access each others memory directly without going over the PCIe bus.

I seriously don't want to model that in the PCI subsystem, but rather the driver. That's why it feels like a mistake to me to push all that into the PCI function.

Why can't we model that as two separate transactions?

Yeah, same for me. If Bjorn is ok with that specialized NVM functions that I'm fine with that as well.

I think it would just be more convenient when we can come up with functions which can handle all use cases, cause there still seems to be a lot of similarities.

Quoted 7 lines

Ok, that is the point where I'm stuck. Why do we need that in one function call in the PCIe subsystem?

The problem at least with GPUs is that we seriously don't have that information here, cause the PCI subsystem might not be aware of all the interconnections.

Quoted 8 lines

For example it isn't uncommon to put multiple GPUs on one board. To the PCI subsystem that looks like separate devices, but in reality all GPUs are interconnected and can access each others memory directly without going over the PCIe bus.

I seriously don't want to model that in the PCI subsystem, but rather the driver. That's why it feels like a mistake to me to push all that into the PCI function.

Huh? I'm lost. If you have a bunch of PCI devices you can send them as a list to this API, if you want. If the driver is _sure_ they are all the same, you only have to send one. In your terminology, you'd just have to call the interface with:

pci_p2pdma_distance(target, [initiator, target])

Quoted 1 line

Why can't we model that as two separate transactions?

You could, but this is more convenient for users of the API that need to deal with multiple devices (and manage devices that may be added or removed at any time).

Quoted 6 lines

Yeah, same for me. If Bjorn is ok with that specialized NVM functions that I'm fine with that as well.

I think it would just be more convenient when we can come up with functions which can handle all use cases, cause there still seems to be a lot of similarities.

The way it's implemented is more general and can handle all use cases. You are arguing for a function that can handle your case (albeit with a bit more fuss) but can't handle mine and is therefore less general. Calling my interface specialized is wrong.

Logan

Are you sure that this is more convenient? At least on first glance it feels overly complicated.

I mean what's the difference between the two approaches?

    sum = pci_p2pdma_distance(target, [A, B, C, target]);

and

    sum = pci_p2pdma_distance(target, A);
    sum += pci_p2pdma_distance(target, B);
    sum += pci_p2pdma_distance(target, C);

Hi Everyone,

Here's v4 of our series to introduce P2P based copy offload to NVMe fabrics. This version has been rebased onto v4.17-rc2. A git repo is here:

https://github.com/sbates130272/linux-p2pmem pci-p2p-v4
...

Logan Gunthorpe (14):
PCI/P2PDMA: Support peer-to-peer memory
PCI/P2PDMA: Add sysfs group to display p2pmem stats PCI/P2PDMA: Add PCI p2pmem dma mappings to adjust the bus offset PCI/P2PDMA: Clear ACS P2P flags for all devices behind switches
docs-rst: Add a new directory for PCI documentation PCI/P2PDMA: Add P2P DMA driver writer's documentation
block: Introduce PCI P2P flags for request and request queue IB/core: Ensure we map P2P memory correctly in

    rdma_rw_ctx_[init|destroy]()

nvme-pci: Use PCI p2pmem subsystem to manage the CMB
nvme-pci: Add support for P2P memory in requests
nvme-pci: Add a quirk for a pseudo CMB
nvmet: Introduce helper functions to allocate and free request SGLs
nvmet-rdma: Use new SGL alloc/free helper for requests
nvmet: Optionally use PCI P2P memory

 Documentation/ABI/testing/sysfs-bus-pci    |  25 +
 Documentation/PCI/index.rst                |  14 +
 Documentation/driver-api/index.rst         |   2 +-
 Documentation/driver-api/pci/index.rst     |  20 +
 Documentation/driver-api/pci/p2pdma.rst    | 166 ++++++
 Documentation/driver-api/{ => pci}/pci.rst |   0
 Documentation/index.rst                    |   3 +-
 block/blk-core.c                           |   3 +
 drivers/infiniband/core/rw.c               |  13 +-
 drivers/nvme/host/core.c                   |   4 +
 drivers/nvme/host/nvme.h                   |   8 +
 drivers/nvme/host/pci.c                    | 118 +++--
 drivers/nvme/target/configfs.c             |  67 +++
 drivers/nvme/target/core.c                 | 143 ++++-
 drivers/nvme/target/io-cmd.c               |   3 +
 drivers/nvme/target/nvmet.h                |  15 +
 drivers/nvme/target/rdma.c                 |  22 +-
 drivers/pci/Kconfig                        |  26 +
 drivers/pci/Makefile                       |   1 +
 drivers/pci/p2pdma.c                       | 814 +++++++++++++++++++++++++++++
 drivers/pci/pci.c                          |   6 +
 include/linux/blk_types.h                  |  18 +-
 include/linux/blkdev.h                     |   3 +
 include/linux/memremap.h                   |  19 +
 include/linux/pci-p2pdma.h                 | 118 +++++

 include/linux/pci.h                        |   4 +
 26 files changed, 1579 insertions(+), 56 deletions(-)
 create mode 100644 Documentation/PCI/index.rst
 create mode 100644 Documentation/driver-api/pci/index.rst
 create mode 100644 Documentation/driver-api/pci/p2pdma.rst
 rename Documentation/driver-api/{ => pci}/pci.rst (100%)
 create mode 100644 drivers/pci/p2pdma.c
 create mode 100644 include/linux/pci-p2pdma.h

How do you envison merging this? There's a big chunk in drivers/pci, but really no opportunity for conflicts there, and there's significant stuff in block and nvme that I don't really want to merge.

If Alex is OK with the ACS situation, I can ack the PCI parts and you could merge it elsewhere?

Quoted 51 lines · Logan Gunthorpe

Hi Everyone,

Here's v4 of our series to introduce P2P based copy offload to NVMe fabrics. This version has been rebased onto v4.17-rc2. A git repo is here:

https://github.com/sbates130272/linux-p2pmem pci-p2p-v4
...

Logan Gunthorpe (14):
PCI/P2PDMA: Support peer-to-peer memory
PCI/P2PDMA: Add sysfs group to display p2pmem stats PCI/P2PDMA: Add PCI p2pmem dma mappings to adjust the bus offset PCI/P2PDMA: Clear ACS P2P flags for all devices behind switches
docs-rst: Add a new directory for PCI documentation PCI/P2PDMA: Add P2P DMA driver writer's documentation
block: Introduce PCI P2P flags for request and request queue IB/core: Ensure we map P2P memory correctly in

    rdma_rw_ctx_[init|destroy]()

nvme-pci: Use PCI p2pmem subsystem to manage the CMB
nvme-pci: Add support for P2P memory in requests
nvme-pci: Add a quirk for a pseudo CMB
nvmet: Introduce helper functions to allocate and free request SGLs
nvmet-rdma: Use new SGL alloc/free helper for requests
nvmet: Optionally use PCI P2P memory

 Documentation/ABI/testing/sysfs-bus-pci    |  25 +
 Documentation/PCI/index.rst                |  14 +
 Documentation/driver-api/index.rst         |   2 +-
 Documentation/driver-api/pci/index.rst     |  20 +
 Documentation/driver-api/pci/p2pdma.rst    | 166 ++++++
 Documentation/driver-api/{ => pci}/pci.rst |   0
 Documentation/index.rst                    |   3 +-
 block/blk-core.c                           |   3 +
 drivers/infiniband/core/rw.c               |  13 +-
 drivers/nvme/host/core.c                   |   4 +
 drivers/nvme/host/nvme.h                   |   8 +
 drivers/nvme/host/pci.c                    | 118 +++--
 drivers/nvme/target/configfs.c             |  67 +++
 drivers/nvme/target/core.c                 | 143 ++++-
 drivers/nvme/target/io-cmd.c               |   3 +
 drivers/nvme/target/nvmet.h                |  15 +
 drivers/nvme/target/rdma.c                 |  22 +-
 drivers/pci/Kconfig                        |  26 +
 drivers/pci/Makefile                       |   1 +
 drivers/pci/p2pdma.c                       | 814 +++++++++++++++++++++++++++++
 drivers/pci/pci.c                          |   6 +
 include/linux/blk_types.h                  |  18 +-
 include/linux/blkdev.h                     |   3 +
 include/linux/memremap.h                   |  19 +
 include/linux/pci-p2pdma.h                 | 118 +++++

Quoted 8 lines

 include/linux/pci.h                        |   4 +
 26 files changed, 1579 insertions(+), 56 deletions(-)
 create mode 100644 Documentation/PCI/index.rst
 create mode 100644 Documentation/driver-api/pci/index.rst
 create mode 100644 Documentation/driver-api/pci/p2pdma.rst
 rename Documentation/driver-api/{ => pci}/pci.rst (100%)
 create mode 100644 drivers/pci/p2pdma.c
 create mode 100644 include/linux/pci-p2pdma.h  

How do you envison merging this? There's a big chunk in drivers/pci, but really no opportunity for conflicts there, and there's significant stuff in block and nvme that I don't really want to merge.

If Alex is OK with the ACS situation, I can ack the PCI parts and you could merge it elsewhere?

AIUI from previously questioning this, the change is hidden behind a build-time config option and only custom kernels or distros optimized for this sort of support would enable that build option. I'm more than a little dubious though that we're not going to have a wave of distros enabling this only to get user complaints that they can no longer make effective use of their devices for assignment due to the resulting span of the IOMMU groups, nor is there any sort of compromise, configure the kernel for p2p or device assignment, not both. Is this really such a unique feature that distro users aren't going to be asking for both features? Thanks,

Quoted 61 lines · On Mon, 7 May 2018 18:23:46 -0500 Bjorn Helgaas <[email protected]> wrote:

Quoted 53 lines · Logan Gunthorpe

Hi Everyone,

Here's v4 of our series to introduce P2P based copy offload to NVMe fabrics. This version has been rebased onto v4.17-rc2. A git repo is here:

https://github.com/sbates130272/linux-p2pmem pci-p2p-v4
...

Logan Gunthorpe (14):
PCI/P2PDMA: Support peer-to-peer memory
PCI/P2PDMA: Add sysfs group to display p2pmem stats PCI/P2PDMA: Add PCI p2pmem dma mappings to adjust the bus offset PCI/P2PDMA: Clear ACS P2P flags for all devices behind switches
docs-rst: Add a new directory for PCI documentation PCI/P2PDMA: Add P2P DMA driver writer's documentation
block: Introduce PCI P2P flags for request and request queue IB/core: Ensure we map P2P memory correctly in

     rdma_rw_ctx_[init|destroy]()

nvme-pci: Use PCI p2pmem subsystem to manage the CMB
nvme-pci: Add support for P2P memory in requests
nvme-pci: Add a quirk for a pseudo CMB
nvmet: Introduce helper functions to allocate and free request SGLs
nvmet-rdma: Use new SGL alloc/free helper for requests
nvmet: Optionally use PCI P2P memory

  Documentation/ABI/testing/sysfs-bus-pci    |  25 +
  Documentation/PCI/index.rst                |  14 +
  Documentation/driver-api/index.rst         |   2 +-
  Documentation/driver-api/pci/index.rst     |  20 +
  Documentation/driver-api/pci/p2pdma.rst    | 166 ++++++
  Documentation/driver-api/{ => pci}/pci.rst |   0
  Documentation/index.rst                    |   3 +-
  block/blk-core.c                           |   3 +
  drivers/infiniband/core/rw.c               |  13 +-
  drivers/nvme/host/core.c                   |   4 +
  drivers/nvme/host/nvme.h                   |   8 +
  drivers/nvme/host/pci.c                    | 118 +++--
  drivers/nvme/target/configfs.c             |  67 +++
  drivers/nvme/target/core.c                 | 143 ++++-
  drivers/nvme/target/io-cmd.c               |   3 +
  drivers/nvme/target/nvmet.h                |  15 +
  drivers/nvme/target/rdma.c                 |  22 +-
  drivers/pci/Kconfig                        |  26 +
  drivers/pci/Makefile                       |   1 +
  drivers/pci/p2pdma.c                       | 814 +++++++++++++++++++++++++++++
  drivers/pci/pci.c                          |   6 +
  include/linux/blk_types.h                  |  18 +-
  include/linux/blkdev.h                     |   3 +
  include/linux/memremap.h                   |  19 +
  include/linux/pci-p2pdma.h                 | 118 +++++
  include/linux/pci.h                        |   4 +
  26 files changed, 1579 insertions(+), 56 deletions(-)

Quoted 6 lines

create mode 100644 Documentation/PCI/index.rst create mode 100644 Documentation/driver-api/pci/index.rst create mode 100644 Documentation/driver-api/pci/p2pdma.rst rename Documentation/driver-api/{ => pci}/pci.rst (100%) create mode 100644 drivers/pci/p2pdma.c
create mode 100644 include/linux/pci-p2pdma.h

Quoted 6 lines

How do you envison merging this? There's a big chunk in drivers/pci, but really no opportunity for conflicts there, and there's significant stuff in block and nvme that I don't really want to merge.

If Alex is OK with the ACS situation, I can ack the PCI parts and you could merge it elsewhere?

AIUI from previously questioning this, the change is hidden behind a build-time config option and only custom kernels or distros optimized for this sort of support would enable that build option. I'm more than a little dubious though that we're not going to have a wave of distros enabling this only to get user complaints that they can no longer make effective use of their devices for assignment due to the resulting span of the IOMMU groups, nor is there any sort of compromise, configure the kernel for p2p or device assignment, not both. Is this really such a unique feature that distro users aren't going to be asking for both features? Thanks,

Alex

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 82 lines · Alex Williamson

Quoted 61 lines · On Mon, 7 May 2018 18:23:46 -0500 Bjorn Helgaas <[email protected]> wrote:

Quoted 53 lines · Logan Gunthorpe

Hi Everyone,

Here's v4 of our series to introduce P2P based copy offload to NVMe fabrics. This version has been rebased onto v4.17-rc2. A git repo is here:

https://github.com/sbates130272/linux-p2pmem pci-p2p-v4
...

Logan Gunthorpe (14):
PCI/P2PDMA: Support peer-to-peer memory
PCI/P2PDMA: Add sysfs group to display p2pmem stats PCI/P2PDMA: Add PCI p2pmem dma mappings to adjust the bus offset PCI/P2PDMA: Clear ACS P2P flags for all devices behind switches
docs-rst: Add a new directory for PCI documentation PCI/P2PDMA: Add P2P DMA driver writer's documentation
block: Introduce PCI P2P flags for request and request queue IB/core: Ensure we map P2P memory correctly in

     rdma_rw_ctx_[init|destroy]()

nvme-pci: Use PCI p2pmem subsystem to manage the CMB
nvme-pci: Add support for P2P memory in requests
nvme-pci: Add a quirk for a pseudo CMB
nvmet: Introduce helper functions to allocate and free request SGLs
nvmet-rdma: Use new SGL alloc/free helper for requests
nvmet: Optionally use PCI P2P memory

  Documentation/ABI/testing/sysfs-bus-pci    |  25 +
  Documentation/PCI/index.rst                |  14 +
  Documentation/driver-api/index.rst         |   2 +-
  Documentation/driver-api/pci/index.rst     |  20 +
  Documentation/driver-api/pci/p2pdma.rst    | 166 ++++++
  Documentation/driver-api/{ => pci}/pci.rst |   0
  Documentation/index.rst                    |   3 +-
  block/blk-core.c                           |   3 +
  drivers/infiniband/core/rw.c               |  13 +-
  drivers/nvme/host/core.c                   |   4 +
  drivers/nvme/host/nvme.h                   |   8 +
  drivers/nvme/host/pci.c                    | 118 +++--
  drivers/nvme/target/configfs.c             |  67 +++
  drivers/nvme/target/core.c                 | 143 ++++-
  drivers/nvme/target/io-cmd.c               |   3 +
  drivers/nvme/target/nvmet.h                |  15 +
  drivers/nvme/target/rdma.c                 |  22 +-
  drivers/pci/Kconfig                        |  26 +
  drivers/pci/Makefile                       |   1 +
  drivers/pci/p2pdma.c                       | 814 +++++++++++++++++++++++++++++
  drivers/pci/pci.c                          |   6 +
  include/linux/blk_types.h                  |  18 +-
  include/linux/blkdev.h                     |   3 +
  include/linux/memremap.h                   |  19 +
  include/linux/pci-p2pdma.h                 | 118 +++++
  include/linux/pci.h                        |   4 +
  26 files changed, 1579 insertions(+), 56 deletions(-)

Quoted 6 lines

create mode 100644 Documentation/PCI/index.rst create mode 100644 Documentation/driver-api/pci/index.rst create mode 100644 Documentation/driver-api/pci/p2pdma.rst rename Documentation/driver-api/{ => pci}/pci.rst (100%) create mode 100644 drivers/pci/p2pdma.c
create mode 100644 include/linux/pci-p2pdma.h

Quoted 6 lines

How do you envison merging this? There's a big chunk in drivers/pci, but really no opportunity for conflicts there, and there's significant stuff in block and nvme that I don't really want to merge.

If Alex is OK with the ACS situation, I can ack the PCI parts and you could merge it elsewhere?

AIUI from previously questioning this, the change is hidden behind a build-time config option and only custom kernels or distros optimized for this sort of support would enable that build option. I'm more than a little dubious though that we're not going to have a wave of distros enabling this only to get user complaints that they can no longer make effective use of their devices for assignment due to the resulting span of the IOMMU groups, nor is there any sort of compromise, configure the kernel for p2p or device assignment, not both. Is this really such a unique feature that distro users aren't going to be asking for both features? Thanks,

Quoted 1 line

Alex

At least 1/2 the cases presented to me by existing customers want it in a tunable kernel,
and tunable btwn two points, if the hw allows it to be 'contained' in that manner, which
a (layer of) switch(ing) provides.
To me, that means a kernel cmdline parameter to _enable_, and another sysfs (configfs? ... i'm not a configfs afficionato to say which is best), method to make two points p2p dma capable.