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

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

[Logan Gunthorpe]

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

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

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

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

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

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

--

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

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

--
2.20.1

[PATCH 01/14] PCI/P2PDMA: Add constants for not-supported result upstream_bridge_distance()

[Logan Gunthorpe]

Add constant flags to indicate two devices are not supported or whether
the data path goes through the host bridge instead of using the negative
values -1 and -2.

This helps annotate the code better, but the main reason is so we
can cache the result in an xarray which does not allow us to store
negative values.

Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
---
 drivers/pci/p2pdma.c | 52 ++++++++++++++++++++++++++------------------
 1 file changed, 31 insertions(+), 21 deletions(-)

diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c
index 234476226529..e8ec86e1dd00 100644
--- a/drivers/pci/p2pdma.c
+++ b/drivers/pci/p2pdma.c
@@ -273,6 +273,20 @@ static bool root_complex_whitelist(struct pci_dev *dev)
 	return false;
 }
 
+enum {
+	/*
+	 * Thes arbitrary offset are or'd onto the upstream distance
+	 * calculation for the following conditions:
+	 */
+
+	/* The data path includes the host-bridge */
+	P2PDMA_THRU_HOST_BRIDGE		= 0x02000000,
+	/* The data path is forced through the host-bridge due to ACS */
+	P2PDMA_ACS_FORCES_UPSTREAM	= 0x04000000,
+	/* The data path is not supported by P2PDMA */
+	P2PDMA_NOT_SUPPORTED		= 0x08000000,
+};
+
 /*
  * Find the distance through the nearest common upstream bridge between
  * two PCI devices.
@@ -297,22 +311,17 @@ static bool root_complex_whitelist(struct pci_dev *dev)
  * port of the switch, to the common upstream port, back up to the second
  * downstream port and then to Device B.
  *
- * Any two devices that don't have a common upstream bridge will return -1.
- * In this way devices on separate PCIe root ports will be rejected, which
- * is what we want for peer-to-peer seeing each PCIe root port defines a
- * separate hierarchy domain and there's no way to determine whether the root
- * complex supports forwarding between them.
+ * Any two devices that cannot communicate using p2pdma will return the distance
+ * with the flag P2PDMA_NOT_SUPPORTED.
  *
- * In the case where two devices are connected to different PCIe switches,
- * this function will still return a positive distance as long as both
- * switches eventually have a common upstream bridge. Note this covers
- * the case of using multiple PCIe switches to achieve a desired level of
- * fan-out from a root port. The exact distance will be a function of the
- * number of switches between Device A and Device B.
+ * Any two devices that have a data path that goes through the host bridge
+ * will consult a whitelist. If the host bridges are on the whitelist,
+ * then the distance will be returned with the flag P2PDMA_THRU_HOST_BRIDGE set.
+ * If either bridge is not on the whitelist, the flag P2PDMA_NOT_SUPPORTED will
+ * be set.
  *
  * If a bridge which has any ACS redirection bits set is in the path
- * then this functions will return -2. This is so we reject any
- * cases where the TLPs are forwarded up into the root complex.
+ * then this functions will flag the result with P2PDMA_ACS_FORCES_UPSTREAM.
  * In this case, a list of all infringing bridge addresses will be
  * populated in acs_list (assuming it's non-null) for printk purposes.
  */
@@ -359,9 +368,9 @@ static int upstream_bridge_distance(struct pci_dev *provider,
 	 */
 	if (root_complex_whitelist(provider) &&
 	    root_complex_whitelist(client))
-		return 0x1000 + dist_a + dist_b;
+		return (dist_a + dist_b) | P2PDMA_THRU_HOST_BRIDGE;
 
-	return -1;
+	return (dist_a + dist_b) | P2PDMA_NOT_SUPPORTED;
 
 check_b_path_acs:
 	bb = b;
@@ -379,7 +388,7 @@ static int upstream_bridge_distance(struct pci_dev *provider,
 	}
 
 	if (acs_cnt)
-		return -2;
+		return P2PDMA_NOT_SUPPORTED | P2PDMA_ACS_FORCES_UPSTREAM;
 
 	return dist_a + dist_b;
 }
@@ -395,16 +404,17 @@ static int upstream_bridge_distance_warn(struct pci_dev *provider,
 		return -ENOMEM;
 
 	ret = upstream_bridge_distance(provider, client, &acs_list);
-	if (ret == -2) {
-		pci_warn(client, "cannot be used for peer-to-peer DMA as ACS redirect is set between the client and provider (%s)\n",
+	if (ret & P2PDMA_ACS_FORCES_UPSTREAM) {
+		pci_warn(client, "ACS redirect is set between the client and provider (%s)\n",
 			 pci_name(provider));
 		/* Drop final semicolon */
 		acs_list.buffer[acs_list.len-1] = 0;
 		pci_warn(client, "to disable ACS redirect for this path, add the kernel parameter: pci=disable_acs_redir=%s\n",
 			 acs_list.buffer);
+	}
 
-	} else if (ret < 0) {
-		pci_warn(client, "cannot be used for peer-to-peer DMA as the client and provider (%s) do not share an upstream bridge\n",
+	if (ret & P2PDMA_NOT_SUPPORTED) {
+		pci_warn(client, "cannot be used for peer-to-peer DMA as the client and provider (%s) do not share an upstream bridge or whitelisted host bridge\n",
 			 pci_name(provider));
 	}
 
@@ -468,7 +478,7 @@ int pci_p2pdma_distance_many(struct pci_dev *provider, struct device **clients,
 
 		pci_dev_put(pci_client);
 
-		if (ret < 0)
+		if (ret & P2PDMA_NOT_SUPPORTED)
 			not_supported = true;
 
 		if (not_supported && !verbose)
-- 
2.20.1

Re: [PATCH 01/14] PCI/P2PDMA: Add constants for not-supported result upstream_bridge_distance()

[Koenig, Christian]

Am 23.07.19 um 01:08 schrieb Logan Gunthorpe:
> Add constant flags to indicate two devices are not supported or whether
> the data path goes through the host bridge instead of using the negative
> values -1 and -2.
>
> This helps annotate the code better, but the main reason is so we
> can cache the result in an xarray which does not allow us to store
> negative values.
>
> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>

Reviewed-by: Christian König <christian.koenig@amd.com>

> ---
>   drivers/pci/p2pdma.c | 52 ++++++++++++++++++++++++++------------------
>   1 file changed, 31 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c
> index 234476226529..e8ec86e1dd00 100644
> --- a/drivers/pci/p2pdma.c
> +++ b/drivers/pci/p2pdma.c
> @@ -273,6 +273,20 @@ static bool root_complex_whitelist(struct pci_dev *dev)
>   	return false;
>   }
>   
> +enum {
> +	/*
> +	 * Thes arbitrary offset are or'd onto the upstream distance
> +	 * calculation for the following conditions:
> +	 */
> +
> +	/* The data path includes the host-bridge */
> +	P2PDMA_THRU_HOST_BRIDGE		= 0x02000000,
> +	/* The data path is forced through the host-bridge due to ACS */
> +	P2PDMA_ACS_FORCES_UPSTREAM	= 0x04000000,
> +	/* The data path is not supported by P2PDMA */
> +	P2PDMA_NOT_SUPPORTED		= 0x08000000,
> +};
> +
>   /*
>    * Find the distance through the nearest common upstream bridge between
>    * two PCI devices.
> @@ -297,22 +311,17 @@ static bool root_complex_whitelist(struct pci_dev *dev)
>    * port of the switch, to the common upstream port, back up to the second
>    * downstream port and then to Device B.
>    *
> - * Any two devices that don't have a common upstream bridge will return -1.
> - * In this way devices on separate PCIe root ports will be rejected, which
> - * is what we want for peer-to-peer seeing each PCIe root port defines a
> - * separate hierarchy domain and there's no way to determine whether the root
> - * complex supports forwarding between them.
> + * Any two devices that cannot communicate using p2pdma will return the distance
> + * with the flag P2PDMA_NOT_SUPPORTED.
>    *
> - * In the case where two devices are connected to different PCIe switches,
> - * this function will still return a positive distance as long as both
> - * switches eventually have a common upstream bridge. Note this covers
> - * the case of using multiple PCIe switches to achieve a desired level of
> - * fan-out from a root port. The exact distance will be a function of the
> - * number of switches between Device A and Device B.
> + * Any two devices that have a data path that goes through the host bridge
> + * will consult a whitelist. If the host bridges are on the whitelist,
> + * then the distance will be returned with the flag P2PDMA_THRU_HOST_BRIDGE set.
> + * If either bridge is not on the whitelist, the flag P2PDMA_NOT_SUPPORTED will
> + * be set.
>    *
>    * If a bridge which has any ACS redirection bits set is in the path
> - * then this functions will return -2. This is so we reject any
> - * cases where the TLPs are forwarded up into the root complex.
> + * then this functions will flag the result with P2PDMA_ACS_FORCES_UPSTREAM.
>    * In this case, a list of all infringing bridge addresses will be
>    * populated in acs_list (assuming it's non-null) for printk purposes.
>    */
> @@ -359,9 +368,9 @@ static int upstream_bridge_distance(struct pci_dev *provider,
>   	 */
>   	if (root_complex_whitelist(provider) &&
>   	    root_complex_whitelist(client))
> -		return 0x1000 + dist_a + dist_b;
> +		return (dist_a + dist_b) | P2PDMA_THRU_HOST_BRIDGE;
>   
> -	return -1;
> +	return (dist_a + dist_b) | P2PDMA_NOT_SUPPORTED;
>   
>   check_b_path_acs:
>   	bb = b;
> @@ -379,7 +388,7 @@ static int upstream_bridge_distance(struct pci_dev *provider,
>   	}
>   
>   	if (acs_cnt)
> -		return -2;
> +		return P2PDMA_NOT_SUPPORTED | P2PDMA_ACS_FORCES_UPSTREAM;
>   
>   	return dist_a + dist_b;
>   }
> @@ -395,16 +404,17 @@ static int upstream_bridge_distance_warn(struct pci_dev *provider,
>   		return -ENOMEM;
>   
>   	ret = upstream_bridge_distance(provider, client, &acs_list);
> -	if (ret == -2) {
> -		pci_warn(client, "cannot be used for peer-to-peer DMA as ACS redirect is set between the client and provider (%s)\n",
> +	if (ret & P2PDMA_ACS_FORCES_UPSTREAM) {
> +		pci_warn(client, "ACS redirect is set between the client and provider (%s)\n",
>   			 pci_name(provider));
>   		/* Drop final semicolon */
>   		acs_list.buffer[acs_list.len-1] = 0;
>   		pci_warn(client, "to disable ACS redirect for this path, add the kernel parameter: pci=disable_acs_redir=%s\n",
>   			 acs_list.buffer);
> +	}
>   
> -	} else if (ret < 0) {
> -		pci_warn(client, "cannot be used for peer-to-peer DMA as the client and provider (%s) do not share an upstream bridge\n",
> +	if (ret & P2PDMA_NOT_SUPPORTED) {
> +		pci_warn(client, "cannot be used for peer-to-peer DMA as the client and provider (%s) do not share an upstream bridge or whitelisted host bridge\n",
>   			 pci_name(provider));
>   	}
>   
> @@ -468,7 +478,7 @@ int pci_p2pdma_distance_many(struct pci_dev *provider, struct device **clients,
>   
>   		pci_dev_put(pci_client);
>   
> -		if (ret < 0)
> +		if (ret & P2PDMA_NOT_SUPPORTED)
>   			not_supported = true;
>   
>   		if (not_supported && !verbose)

[PATCH 02/14] PCI/P2PDMA: Factor out __upstream_bridge_distance()

[Logan Gunthorpe]

This is a prep patch to create a second level helper. There are no
functional changes.

The root complex whitelist code will be moved into this function in
a subsequent patch.

Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
---
 drivers/pci/p2pdma.c | 89 ++++++++++++++++++++++++--------------------
 1 file changed, 48 insertions(+), 41 deletions(-)

diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c
index e8ec86e1dd00..289d03a31e7d 100644
--- a/drivers/pci/p2pdma.c
+++ b/drivers/pci/p2pdma.c
@@ -287,47 +287,9 @@ enum {
 	P2PDMA_NOT_SUPPORTED		= 0x08000000,
 };
 
-/*
- * Find the distance through the nearest common upstream bridge between
- * two PCI devices.
- *
- * If the two devices are the same device then 0 will be returned.
- *
- * If there are two virtual functions of the same device behind the same
- * bridge port then 2 will be returned (one step down to the PCIe switch,
- * then 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
- *       + \- Device A
- *       \-+ Switch Downstream Port
- *         \- Device B
- *
- * The distance is 4 because we traverse from Device A through the downstream
- * port of the switch, to the common upstream port, back up to the second
- * downstream port and then to Device B.
- *
- * Any two devices that cannot communicate using p2pdma will return the distance
- * with the flag P2PDMA_NOT_SUPPORTED.
- *
- * Any two devices that have a data path that goes through the host bridge
- * will consult a whitelist. If the host bridges are on the whitelist,
- * then the distance will be returned with the flag P2PDMA_THRU_HOST_BRIDGE set.
- * If either bridge is not on the whitelist, the flag P2PDMA_NOT_SUPPORTED will
- * be set.
- *
- * If a bridge which has any ACS redirection bits set is in the path
- * then this functions will flag the result with P2PDMA_ACS_FORCES_UPSTREAM.
- * In this case, a list of all infringing bridge addresses will be
- * populated in acs_list (assuming it's non-null) for printk purposes.
- */
-static int upstream_bridge_distance(struct pci_dev *provider,
-				    struct pci_dev *client,
-				    struct seq_buf *acs_list)
+static int __upstream_bridge_distance(struct pci_dev *provider,
+				      struct pci_dev *client,
+				      struct seq_buf *acs_list)
 {
 	struct pci_dev *a = provider, *b = client, *bb;
 	int dist_a = 0;
@@ -393,6 +355,51 @@ static int upstream_bridge_distance(struct pci_dev *provider,
 	return dist_a + dist_b;
 }
 
+/*
+ * Find the distance through the nearest common upstream bridge between
+ * two PCI devices.
+ *
+ * If the two devices are the same device then 0 will be returned.
+ *
+ * If there are two virtual functions of the same device behind the same
+ * bridge port then 2 will be returned (one step down to the PCIe switch,
+ * then 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
+ *       + \- Device A
+ *       \-+ Switch Downstream Port
+ *         \- Device B
+ *
+ * The distance is 4 because we traverse from Device A through the downstream
+ * port of the switch, to the common upstream port, back up to the second
+ * downstream port and then to Device B.
+ *
+ * Any two devices that cannot communicate using p2pdma will return the distance
+ * with the flag P2PDMA_NOT_SUPPORTED.
+ *
+ * Any two devices that have a data path that goes through the host bridge
+ * will consult a whitelist. If the host bridges are on the whitelist,
+ * then the distance will be returned with the flag P2PDMA_THRU_HOST_BRIDGE set.
+ * If either bridge is not on the whitelist, the flag P2PDMA_NOT_SUPPORTED will
+ * be set.
+ *
+ * If a bridge which has any ACS redirection bits set is in the path
+ * then this functions will flag the result with P2PDMA_ACS_FORCES_UPSTREAM.
+ * In this case, a list of all infringing bridge addresses will be
+ * populated in acs_list (assuming it's non-null) for printk purposes.
+ */
+static int upstream_bridge_distance(struct pci_dev *provider,
+				    struct pci_dev *client,
+				    struct seq_buf *acs_list)
+{
+	return __upstream_bridge_distance(provider, client, acs_list);
+}
+
 static int upstream_bridge_distance_warn(struct pci_dev *provider,
 					 struct pci_dev *client)
 {
-- 
2.20.1

[PATCH 03/14] PCI/P2PDMA: Apply host bridge white list for ACS

[Logan Gunthorpe]

When a P2PDMA transfer is rejected due to ACS being set, we
can also check the white list and allow the transactions.

Do this by pushing the whitelist check into the
upstream_bridge_distance() function.

Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
---
 drivers/pci/p2pdma.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c
index 289d03a31e7d..d5034e28d1e1 100644
--- a/drivers/pci/p2pdma.c
+++ b/drivers/pci/p2pdma.c
@@ -324,15 +324,7 @@ static int __upstream_bridge_distance(struct pci_dev *provider,
 		dist_a++;
 	}
 
-	/*
-	 * Allow the connection if both devices are on a whitelisted root
-	 * complex, but add an arbitrary large value to the distance.
-	 */
-	if (root_complex_whitelist(provider) &&
-	    root_complex_whitelist(client))
-		return (dist_a + dist_b) | P2PDMA_THRU_HOST_BRIDGE;
-
-	return (dist_a + dist_b) | P2PDMA_NOT_SUPPORTED;
+	return (dist_a + dist_b) | P2PDMA_THRU_HOST_BRIDGE;
 
 check_b_path_acs:
 	bb = b;
@@ -350,7 +342,8 @@ static int __upstream_bridge_distance(struct pci_dev *provider,
 	}
 
 	if (acs_cnt)
-		return P2PDMA_NOT_SUPPORTED | P2PDMA_ACS_FORCES_UPSTREAM;
+		return (dist_a + dist_b) | P2PDMA_ACS_FORCES_UPSTREAM |
+			P2PDMA_THRU_HOST_BRIDGE;
 
 	return dist_a + dist_b;
 }
@@ -397,7 +390,17 @@ static int upstream_bridge_distance(struct pci_dev *provider,
 				    struct pci_dev *client,
 				    struct seq_buf *acs_list)
 {
-	return __upstream_bridge_distance(provider, client, acs_list);
+	int dist;
+
+	dist = __upstream_bridge_distance(provider, client, acs_list);
+
+	if (!(dist & P2PDMA_THRU_HOST_BRIDGE))
+		return dist;
+
+	if (root_complex_whitelist(provider) && root_complex_whitelist(client))
+		return dist;
+
+	return dist | P2PDMA_NOT_SUPPORTED;
 }
 
 static int upstream_bridge_distance_warn(struct pci_dev *provider,
-- 
2.20.1

Re: [PATCH 03/14] PCI/P2PDMA: Apply host bridge white list for ACS

[Koenig, Christian]

Am 23.07.19 um 01:08 schrieb Logan Gunthorpe:
> When a P2PDMA transfer is rejected due to ACS being set, we
> can also check the white list and allow the transactions.
>
> Do this by pushing the whitelist check into the
> upstream_bridge_distance() function.
>
> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>

This one and patch #2 are Reviewed-by: Christian König 
<christian.koenig@amd.com>.

But I actually think the two patches could be merged.

Christian.

> ---
>   drivers/pci/p2pdma.c | 25 ++++++++++++++-----------
>   1 file changed, 14 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c
> index 289d03a31e7d..d5034e28d1e1 100644
> --- a/drivers/pci/p2pdma.c
> +++ b/drivers/pci/p2pdma.c
> @@ -324,15 +324,7 @@ static int __upstream_bridge_distance(struct pci_dev *provider,
>   		dist_a++;
>   	}
>   
> -	/*
> -	 * Allow the connection if both devices are on a whitelisted root
> -	 * complex, but add an arbitrary large value to the distance.
> -	 */
> -	if (root_complex_whitelist(provider) &&
> -	    root_complex_whitelist(client))
> -		return (dist_a + dist_b) | P2PDMA_THRU_HOST_BRIDGE;
> -
> -	return (dist_a + dist_b) | P2PDMA_NOT_SUPPORTED;
> +	return (dist_a + dist_b) | P2PDMA_THRU_HOST_BRIDGE;
>   
>   check_b_path_acs:
>   	bb = b;
> @@ -350,7 +342,8 @@ static int __upstream_bridge_distance(struct pci_dev *provider,
>   	}
>   
>   	if (acs_cnt)
> -		return P2PDMA_NOT_SUPPORTED | P2PDMA_ACS_FORCES_UPSTREAM;
> +		return (dist_a + dist_b) | P2PDMA_ACS_FORCES_UPSTREAM |
> +			P2PDMA_THRU_HOST_BRIDGE;
>   
>   	return dist_a + dist_b;
>   }
> @@ -397,7 +390,17 @@ static int upstream_bridge_distance(struct pci_dev *provider,
>   				    struct pci_dev *client,
>   				    struct seq_buf *acs_list)
>   {
> -	return __upstream_bridge_distance(provider, client, acs_list);
> +	int dist;
> +
> +	dist = __upstream_bridge_distance(provider, client, acs_list);
> +
> +	if (!(dist & P2PDMA_THRU_HOST_BRIDGE))
> +		return dist;
> +
> +	if (root_complex_whitelist(provider) && root_complex_whitelist(client))
> +		return dist;
> +
> +	return dist | P2PDMA_NOT_SUPPORTED;
>   }
>   
>   static int upstream_bridge_distance_warn(struct pci_dev *provider,

[PATCH 04/14] PCI/P2PDMA: Cache the result of upstream_bridge_distance()

[Logan Gunthorpe]

The result of upstream_bridge_distance() will be needed every time
we map the dma address so cache it in an xarray stored in the
provider's p2pdma struct.

Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
---
 drivers/pci/p2pdma.c | 29 +++++++++++++++++++++++++----
 1 file changed, 25 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c
index d5034e28d1e1..25663c1d8bc9 100644
--- a/drivers/pci/p2pdma.c
+++ b/drivers/pci/p2pdma.c
@@ -19,10 +19,12 @@
 #include <linux/random.h>
 #include <linux/seq_buf.h>
 #include <linux/iommu.h>
+#include <linux/xarray.h>
 
 struct pci_p2pdma {
 	struct gen_pool *pool;
 	bool p2pmem_published;
+	struct xarray dist_cache;
 };
 
 static ssize_t size_show(struct device *dev, struct device_attribute *attr,
@@ -98,6 +100,8 @@ static int pci_p2pdma_setup(struct pci_dev *pdev)
 	if (!p2p)
 		return -ENOMEM;
 
+	xa_init(&p2p->dist_cache);
+
 	p2p->pool = gen_pool_create(PAGE_SHIFT, dev_to_node(&pdev->dev));
 	if (!p2p->pool)
 		goto out;
@@ -390,17 +394,34 @@ static int upstream_bridge_distance(struct pci_dev *provider,
 				    struct pci_dev *client,
 				    struct seq_buf *acs_list)
 {
+	void *entry;
 	int dist;
+	int idx;
+
+	idx = (pci_domain_nr(client->bus) << 16) |
+		(client->bus->number << 8) | client->devfn;
+
+	if (provider->p2pdma) {
+		entry = xa_load(&provider->p2pdma->dist_cache, idx);
+		if (entry)
+			return xa_to_value(entry);
+	}
 
 	dist = __upstream_bridge_distance(provider, client, acs_list);
 
 	if (!(dist & P2PDMA_THRU_HOST_BRIDGE))
-		return dist;
+		goto store_and_return;
+
+	if (!root_complex_whitelist(provider) ||
+	    !root_complex_whitelist(client))
+		dist |= P2PDMA_NOT_SUPPORTED;
 
-	if (root_complex_whitelist(provider) && root_complex_whitelist(client))
-		return dist;
+store_and_return:
+	if (provider->p2pdma)
+		xa_store(&provider->p2pdma->dist_cache, idx,
+			 xa_mk_value(dist), GFP_KERNEL);
 
-	return dist | P2PDMA_NOT_SUPPORTED;
+	return dist;
 }
 
 static int upstream_bridge_distance_warn(struct pci_dev *provider,
-- 
2.20.1

[PATCH 05/14] PCI/P2PDMA: Factor out host_bridge_whitelist()

[Logan Gunthorpe]

Push both PCI devices into the whitelist checking function seeing
some hardware will require us ensuring they are on the same host
bridge.

At the same time we rename root_complex_whitelist() to
host_bridge_whitelist() to match the terminology used in the code.

Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
---
 drivers/pci/p2pdma.c | 31 ++++++++++++++++++++-----------
 1 file changed, 20 insertions(+), 11 deletions(-)

diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c
index 25663c1d8bc9..dfb802afc8ca 100644
--- a/drivers/pci/p2pdma.c
+++ b/drivers/pci/p2pdma.c
@@ -250,19 +250,11 @@ static void seq_buf_print_bus_devfn(struct seq_buf *buf, struct pci_dev *pdev)
 	seq_buf_printf(buf, "%s;", pci_name(pdev));
 }
 
-/*
- * If we can't find a common upstream bridge take a look at the root
- * complex and compare it to a whitelist of known good hardware.
- */
-static bool root_complex_whitelist(struct pci_dev *dev)
+static bool __host_bridge_whitelist(struct pci_host_bridge *host)
 {
-	struct pci_host_bridge *host = pci_find_host_bridge(dev->bus);
 	struct pci_dev *root = pci_get_slot(host->bus, PCI_DEVFN(0, 0));
 	unsigned short vendor, device;
 
-	if (iommu_present(dev->dev.bus))
-		return false;
-
 	if (!root)
 		return false;
 
@@ -277,6 +269,24 @@ static bool root_complex_whitelist(struct pci_dev *dev)
 	return false;
 }
 
+/*
+ * If we can't find a common upstream bridge take a look at the root
+ * complex and compare it to a whitelist of known good hardware.
+ */
+static bool host_bridge_whitelist(struct pci_dev *a, struct pci_dev *b)
+{
+	struct pci_host_bridge *host_a = pci_find_host_bridge(a->bus);
+	struct pci_host_bridge *host_b = pci_find_host_bridge(b->bus);
+
+	if (iommu_present(a->dev.bus) || iommu_present(b->dev.bus))
+		return false;
+
+	if (__host_bridge_whitelist(host_a) && __host_bridge_whitelist(host_b))
+		return true;
+
+	return false;
+}
+
 enum {
 	/*
 	 * Thes arbitrary offset are or'd onto the upstream distance
@@ -412,8 +422,7 @@ static int upstream_bridge_distance(struct pci_dev *provider,
 	if (!(dist & P2PDMA_THRU_HOST_BRIDGE))
 		goto store_and_return;
 
-	if (!root_complex_whitelist(provider) ||
-	    !root_complex_whitelist(client))
+	if (!host_bridge_whitelist(provider, client))
 		dist |= P2PDMA_NOT_SUPPORTED;
 
 store_and_return:
-- 
2.20.1

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

[Logan Gunthorpe]

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

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

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

Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
---
 drivers/pci/p2pdma.c | 36 ++++++++++++++++++++++++++++++++----
 1 file changed, 32 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c
index dfb802afc8ca..143e11d2a5c3 100644
--- a/drivers/pci/p2pdma.c
+++ b/drivers/pci/p2pdma.c
@@ -250,9 +250,28 @@ static void seq_buf_print_bus_devfn(struct seq_buf *buf, struct pci_dev *pdev)
 	seq_buf_printf(buf, "%s;", pci_name(pdev));
 }
 
-static bool __host_bridge_whitelist(struct pci_host_bridge *host)
+static const struct pci_p2pdma_whitelist_entry {
+	unsigned short vendor;
+	unsigned short device;
+	bool req_same_host_bridge;
+} pci_p2pdma_whitelist[] = {
+	/* AMD ZEN */
+	{PCI_VENDOR_ID_AMD,	0x1450,	false},
+
+	/* Intel Xeon E5/Core i7 */
+	{PCI_VENDOR_ID_INTEL,	0x3c00, true},
+	{PCI_VENDOR_ID_INTEL,	0x3c01, true},
+	/* Intel Xeon E7 v3/Xeon E5 v3/Core i7 */
+	{PCI_VENDOR_ID_INTEL,	0x2f00, true},
+	{PCI_VENDOR_ID_INTEL,	0x2f01, true},
+	{}
+};
+
+static bool __host_bridge_whitelist(struct pci_host_bridge *host,
+				    bool same_host_bridge)
 {
 	struct pci_dev *root = pci_get_slot(host->bus, PCI_DEVFN(0, 0));
+	const struct pci_p2pdma_whitelist_entry *entry;
 	unsigned short vendor, device;
 
 	if (!root)
@@ -262,9 +281,14 @@ static bool __host_bridge_whitelist(struct pci_host_bridge *host)
 	device = root->device;
 	pci_dev_put(root);
 
-	/* AMD ZEN host bridges can do peer to peer */
-	if (vendor == PCI_VENDOR_ID_AMD && device == 0x1450)
+	for (entry = pci_p2pdma_whitelist; entry->vendor; entry++) {
+		if (vendor != entry->vendor || device != entry->device)
+			continue;
+		if (entry->req_same_host_bridge && !same_host_bridge)
+			return false;
+
 		return true;
+	}
 
 	return false;
 }
@@ -281,7 +305,11 @@ static bool host_bridge_whitelist(struct pci_dev *a, struct pci_dev *b)
 	if (iommu_present(a->dev.bus) || iommu_present(b->dev.bus))
 		return false;
 
-	if (__host_bridge_whitelist(host_a) && __host_bridge_whitelist(host_b))
+	if (host_a == host_b)
+		return __host_bridge_whitelist(host_a, true);
+
+	if (__host_bridge_whitelist(host_a, false) &&
+	    __host_bridge_whitelist(host_b, false))
 		return true;
 
 	return false;
-- 
2.20.1

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

[Jason Gunthorpe]

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

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

Jason

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

[Logan Gunthorpe]

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

Ok, will change for v2.

Logan

[PATCH 07/14] PCI/P2PDMA: Add the provider's pci_dev to the dev_pgmap struct

[Logan Gunthorpe]

The provider will be needed to figure out how to properly map a device.

Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
---
 drivers/pci/p2pdma.c     | 1 +
 include/linux/memremap.h | 1 +
 2 files changed, 2 insertions(+)

diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c
index 143e11d2a5c3..70c262b7c731 100644
--- a/drivers/pci/p2pdma.c
+++ b/drivers/pci/p2pdma.c
@@ -168,6 +168,7 @@ int pci_p2pdma_add_resource(struct pci_dev *pdev, int bar, size_t size,
 	pgmap->res.end = pgmap->res.start + size - 1;
 	pgmap->res.flags = pci_resource_flags(pdev, bar);
 	pgmap->type = MEMORY_DEVICE_PCI_P2PDMA;
+	pgmap->pci_p2pdma_provider = pdev;
 	pgmap->pci_p2pdma_bus_offset = pci_bus_address(pdev, bar) -
 		pci_resource_start(pdev, bar);
 
diff --git a/include/linux/memremap.h b/include/linux/memremap.h
index f8a5b2a19945..91aee61796bc 100644
--- a/include/linux/memremap.h
+++ b/include/linux/memremap.h
@@ -112,6 +112,7 @@ struct dev_pagemap {
 	struct device *dev;
 	enum memory_type type;
 	unsigned int flags;
+	struct pci_dev *pci_p2pdma_provider;
 	u64 pci_p2pdma_bus_offset;
 	const struct dev_pagemap_ops *ops;
 };
-- 
2.20.1

Re: [PATCH 07/14] PCI/P2PDMA: Add the provider's pci_dev to the dev_pgmap struct

[Christoph Hellwig]

On Mon, Jul 22, 2019 at 05:08:52PM -0600, Logan Gunthorpe wrote:
> diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c
> index 143e11d2a5c3..70c262b7c731 100644
> --- a/drivers/pci/p2pdma.c
> +++ b/drivers/pci/p2pdma.c
> @@ -168,6 +168,7 @@ int pci_p2pdma_add_resource(struct pci_dev *pdev, int bar, size_t size,
>  	pgmap->res.end = pgmap->res.start + size - 1;
>  	pgmap->res.flags = pci_resource_flags(pdev, bar);
>  	pgmap->type = MEMORY_DEVICE_PCI_P2PDMA;
> +	pgmap->pci_p2pdma_provider = pdev;
>  	pgmap->pci_p2pdma_bus_offset = pci_bus_address(pdev, bar) -
>  		pci_resource_start(pdev, bar);

I think we need to bite the bullet and move the PCIe P2P specific
information out of struct dev_pagemap and into a pci-specific structure
that embedds struct dev_pagemap.

Re: [PATCH 07/14] PCI/P2PDMA: Add the provider's pci_dev to the dev_pgmap struct

[Logan Gunthorpe]

On 2019-07-24 12:32 a.m., Christoph Hellwig wrote:
> On Mon, Jul 22, 2019 at 05:08:52PM -0600, Logan Gunthorpe wrote:
>> diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c
>> index 143e11d2a5c3..70c262b7c731 100644
>> --- a/drivers/pci/p2pdma.c
>> +++ b/drivers/pci/p2pdma.c
>> @@ -168,6 +168,7 @@ int pci_p2pdma_add_resource(struct pci_dev *pdev, int bar, size_t size,
>>  	pgmap->res.end = pgmap->res.start + size - 1;
>>  	pgmap->res.flags = pci_resource_flags(pdev, bar);
>>  	pgmap->type = MEMORY_DEVICE_PCI_P2PDMA;
>> +	pgmap->pci_p2pdma_provider = pdev;
>>  	pgmap->pci_p2pdma_bus_offset = pci_bus_address(pdev, bar) -
>>  		pci_resource_start(pdev, bar);
> 
> I think we need to bite the bullet and move the PCIe P2P specific
> information out of struct dev_pagemap and into a pci-specific structure
> that embedds struct dev_pagemap.

OK, I was going to do that, but you just removed the p2p specific page
map. ;)

I'll change this for a v2.

Logan

Re: [PATCH 07/14] PCI/P2PDMA: Add the provider's pci_dev to the dev_pgmap struct

[Christoph Hellwig]

On Wed, Jul 24, 2019 at 09:50:03AM -0600, Logan Gunthorpe wrote:
> OK, I was going to do that, but you just removed the p2p specific page
> map. ;)

Only because it was empty at that time..

[PATCH 08/14] PCI/P2PDMA: Add attrs argument to pci_p2pdma_map_sg()

[Logan Gunthorpe]

This is to match the dma_map_sg() API which this function will have
to call in an future patch.

Adds a pci_p2pdma_map_sg_attrs() function and helper to call it with
no attributes just like the dma_map_sg() function.

Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
---
 drivers/nvme/host/pci.c    |  4 ++--
 drivers/pci/p2pdma.c       |  7 ++++---
 include/linux/pci-p2pdma.h | 15 +++++++++++----
 3 files changed, 17 insertions(+), 9 deletions(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index bb970ca82517..7747712054cd 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -832,8 +832,8 @@ static blk_status_t nvme_map_data(struct nvme_dev *dev, struct request *req,
 		goto out;
 
 	if (is_pci_p2pdma_page(sg_page(iod->sg)))
-		nr_mapped = pci_p2pdma_map_sg(dev->dev, iod->sg, iod->nents,
-					      rq_dma_dir(req));
+		nr_mapped = pci_p2pdma_map_sg_attrs(dev->dev, iod->sg,
+				iod->nents, rq_dma_dir(req), DMA_ATTR_NO_WARN);
 	else
 		nr_mapped = dma_map_sg_attrs(dev->dev, iod->sg, iod->nents,
 					     rq_dma_dir(req), DMA_ATTR_NO_WARN);
diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c
index 70c262b7c731..ce361e287543 100644
--- a/drivers/pci/p2pdma.c
+++ b/drivers/pci/p2pdma.c
@@ -791,13 +791,14 @@ EXPORT_SYMBOL_GPL(pci_p2pmem_publish);
  * @sg: scatter list to map
  * @nents: elements in the scatterlist
  * @dir: DMA direction
+ * @attrs: dma attributes passed to dma_map_sg() (if called)
  *
  * Scatterlists mapped with this function should not be unmapped in any way.
  *
  * Returns the number of SG entries mapped or 0 on error.
  */
-int pci_p2pdma_map_sg(struct device *dev, struct scatterlist *sg, int nents,
-		      enum dma_data_direction dir)
+int pci_p2pdma_map_sg_attrs(struct device *dev, struct scatterlist *sg,
+		int nents, enum dma_data_direction dir, unsigned long attrs)
 {
 	struct dev_pagemap *pgmap;
 	struct scatterlist *s;
@@ -824,7 +825,7 @@ int pci_p2pdma_map_sg(struct device *dev, struct scatterlist *sg, int nents,
 
 	return nents;
 }
-EXPORT_SYMBOL_GPL(pci_p2pdma_map_sg);
+EXPORT_SYMBOL_GPL(pci_p2pdma_map_sg_attrs);
 
 /**
  * pci_p2pdma_enable_store - parse a configfs/sysfs attribute store
diff --git a/include/linux/pci-p2pdma.h b/include/linux/pci-p2pdma.h
index bca9bc3e5be7..7fd51954f93a 100644
--- a/include/linux/pci-p2pdma.h
+++ b/include/linux/pci-p2pdma.h
@@ -30,8 +30,8 @@ struct scatterlist *pci_p2pmem_alloc_sgl(struct pci_dev *pdev,
 					 unsigned int *nents, u32 length);
 void pci_p2pmem_free_sgl(struct pci_dev *pdev, struct scatterlist *sgl);
 void pci_p2pmem_publish(struct pci_dev *pdev, bool publish);
-int pci_p2pdma_map_sg(struct device *dev, struct scatterlist *sg, int nents,
-		      enum dma_data_direction dir);
+int pci_p2pdma_map_sg_attrs(struct device *dev, struct scatterlist *sg,
+		int nents, enum dma_data_direction dir, unsigned long attrs);
 int pci_p2pdma_enable_store(const char *page, struct pci_dev **p2p_dev,
 			    bool *use_p2pdma);
 ssize_t pci_p2pdma_enable_show(char *page, struct pci_dev *p2p_dev,
@@ -81,8 +81,9 @@ static inline void pci_p2pmem_free_sgl(struct pci_dev *pdev,
 static inline void pci_p2pmem_publish(struct pci_dev *pdev, bool publish)
 {
 }
-static inline int pci_p2pdma_map_sg(struct device *dev,
-		struct scatterlist *sg, int nents, enum dma_data_direction dir)
+static inline int pci_p2pdma_map_sg_attrs(struct device *dev,
+		struct scatterlist *sg, int nents, enum dma_data_direction dir,
+		unsigned long attrs)
 {
 	return 0;
 }
@@ -111,4 +112,10 @@ static inline struct pci_dev *pci_p2pmem_find(struct device *client)
 	return pci_p2pmem_find_many(&client, 1);
 }
 
+static inline int pci_p2pdma_map_sg(struct device *dev, struct scatterlist *sg,
+				    int nents, enum dma_data_direction dir)
+{
+	return pci_p2pdma_map_sg_attrs(dev, sg, nents, dir, 0);
+}
+
 #endif /* _LINUX_PCI_P2P_H */
-- 
2.20.1

[PATCH 09/14] PCI/P2PDMA: Introduce pci_p2pdma_unmap_sg()

[Logan Gunthorpe]

Add pci_p2pdma_unmap_sg() to the two places that call
pci_p2pdma_map_sg().

This is a prep patch to introduce correct mappings for p2pdma
transactions that go through the root complex.

Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
---
 drivers/infiniband/core/rw.c |  6 ++++--
 drivers/nvme/host/pci.c      |  6 ++++--
 drivers/pci/p2pdma.c         | 18 +++++++++++++++++-
 include/linux/pci-p2pdma.h   | 13 +++++++++++++
 4 files changed, 38 insertions(+), 5 deletions(-)

diff --git a/drivers/infiniband/core/rw.c b/drivers/infiniband/core/rw.c
index dce06108c8c3..5337393d4dfe 100644
--- a/drivers/infiniband/core/rw.c
+++ b/drivers/infiniband/core/rw.c
@@ -583,8 +583,10 @@ void rdma_rw_ctx_destroy(struct rdma_rw_ctx *ctx, struct ib_qp *qp, u8 port_num,
 		break;
 	}
 
-	/* P2PDMA contexts do not need to be unmapped */
-	if (!is_pci_p2pdma_page(sg_page(sg)))
+	if (is_pci_p2pdma_page(sg_page(sg)))
+		pci_p2pdma_unmap_sg(qp->pd->device->dma_device, sg,
+				    sg_cnt, dir);
+	else
 		ib_dma_unmap_sg(qp->pd->device, sg, sg_cnt, dir);
 }
 EXPORT_SYMBOL(rdma_rw_ctx_destroy);
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 7747712054cd..2348b15f6bd0 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -547,8 +547,10 @@ static void nvme_unmap_data(struct nvme_dev *dev, struct request *req)
 
 	WARN_ON_ONCE(!iod->nents);
 
-	/* P2PDMA requests do not need to be unmapped */
-	if (!is_pci_p2pdma_page(sg_page(iod->sg)))
+	if (is_pci_p2pdma_page(sg_page(iod->sg)))
+		pci_p2pdma_unmap_sg(dev->dev, iod->sg, iod->nents,
+				    rq_dma_dir(req));
+	else
 		dma_unmap_sg(dev->dev, iod->sg, iod->nents, rq_dma_dir(req));
 
 
diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c
index ce361e287543..d3ced8eee30a 100644
--- a/drivers/pci/p2pdma.c
+++ b/drivers/pci/p2pdma.c
@@ -793,7 +793,8 @@ EXPORT_SYMBOL_GPL(pci_p2pmem_publish);
  * @dir: DMA direction
  * @attrs: dma attributes passed to dma_map_sg() (if called)
  *
- * Scatterlists mapped with this function should not be unmapped in any way.
+ * Scatterlists mapped with this function should be unmapped using
+ * pci_p2pdma_unmap_sg_attrs().
  *
  * Returns the number of SG entries mapped or 0 on error.
  */
@@ -827,6 +828,21 @@ int pci_p2pdma_map_sg_attrs(struct device *dev, struct scatterlist *sg,
 }
 EXPORT_SYMBOL_GPL(pci_p2pdma_map_sg_attrs);
 
+/**
+ * pci_p2pdma_unmap_sg - unmap a PCI peer-to-peer scatterlist that was
+ *	mapped with pci_p2pdma_map_sg()
+ * @dev: device doing the DMA request
+ * @sg: scatter list to map
+ * @nents: number of elements returned by pci_p2pdma_map_sg()
+ * @dir: DMA direction
+ * @attrs: dma attributes passed to dma_unmap_sg() (if called)
+ */
+void pci_p2pdma_unmap_sg_attrs(struct device *dev, struct scatterlist *sg,
+		int nents, enum dma_data_direction dir, unsigned long attrs)
+{
+}
+EXPORT_SYMBOL_GPL(pci_p2pdma_unmap_sg_attrs);
+
 /**
  * pci_p2pdma_enable_store - parse a configfs/sysfs attribute store
  *		to enable p2pdma
diff --git a/include/linux/pci-p2pdma.h b/include/linux/pci-p2pdma.h
index 7fd51954f93a..8318a97c9c61 100644
--- a/include/linux/pci-p2pdma.h
+++ b/include/linux/pci-p2pdma.h
@@ -32,6 +32,8 @@ void pci_p2pmem_free_sgl(struct pci_dev *pdev, struct scatterlist *sgl);
 void pci_p2pmem_publish(struct pci_dev *pdev, bool publish);
 int pci_p2pdma_map_sg_attrs(struct device *dev, struct scatterlist *sg,
 		int nents, enum dma_data_direction dir, unsigned long attrs);
+void pci_p2pdma_unmap_sg_attrs(struct device *dev, struct scatterlist *sg,
+		int nents, enum dma_data_direction dir, unsigned long attrs);
 int pci_p2pdma_enable_store(const char *page, struct pci_dev **p2p_dev,
 			    bool *use_p2pdma);
 ssize_t pci_p2pdma_enable_show(char *page, struct pci_dev *p2p_dev,
@@ -87,6 +89,11 @@ static inline int pci_p2pdma_map_sg_attrs(struct device *dev,
 {
 	return 0;
 }
+static inline void pci_p2pdma_unmap_sg_attrs(struct device *dev,
+		struct scatterlist *sg, int nents, enum dma_data_direction dir,
+		unsigned long attrs)
+{
+}
 static inline int pci_p2pdma_enable_store(const char *page,
 		struct pci_dev **p2p_dev, bool *use_p2pdma)
 {
@@ -118,4 +125,10 @@ static inline int pci_p2pdma_map_sg(struct device *dev, struct scatterlist *sg,
 	return pci_p2pdma_map_sg_attrs(dev, sg, nents, dir, 0);
 }
 
+static inline void pci_p2pdma_unmap_sg(struct device *dev,
+		struct scatterlist *sg, int nents, enum dma_data_direction dir)
+{
+	pci_p2pdma_unmap_sg_attrs(dev, sg, nents, dir, 0);
+}
+
 #endif /* _LINUX_PCI_P2P_H */
-- 
2.20.1

[PATCH 10/14] PCI/P2PDMA: Factor out __pci_p2pdma_map_sg()

[Logan Gunthorpe]

Factor out the bus only mapping into it's own static function.
No functional changes. The original pci_p2pdma_map_sg_attrs() will
be used to decide whether this is an appropriate way to map.

Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
---
 drivers/pci/p2pdma.c | 39 +++++++++++++++++++++++----------------
 1 file changed, 23 insertions(+), 16 deletions(-)

diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c
index d3ced8eee30a..5f43f92f9336 100644
--- a/drivers/pci/p2pdma.c
+++ b/drivers/pci/p2pdma.c
@@ -785,23 +785,9 @@ void pci_p2pmem_publish(struct pci_dev *pdev, bool publish)
 }
 EXPORT_SYMBOL_GPL(pci_p2pmem_publish);
 
-/**
- * pci_p2pdma_map_sg - map a PCI peer-to-peer scatterlist for DMA
- * @dev: device doing the DMA request
- * @sg: scatter list to map
- * @nents: elements in the scatterlist
- * @dir: DMA direction
- * @attrs: dma attributes passed to dma_map_sg() (if called)
- *
- * Scatterlists mapped with this function should be unmapped using
- * pci_p2pdma_unmap_sg_attrs().
- *
- * Returns the number of SG entries mapped or 0 on error.
- */
-int pci_p2pdma_map_sg_attrs(struct device *dev, struct scatterlist *sg,
-		int nents, enum dma_data_direction dir, unsigned long attrs)
+static int __pci_p2pdma_map_sg(struct dev_pagemap *pgmap,
+		struct device *dev, struct scatterlist *sg, int nents)
 {
-	struct dev_pagemap *pgmap;
 	struct scatterlist *s;
 	phys_addr_t paddr;
 	int i;
@@ -826,6 +812,27 @@ int pci_p2pdma_map_sg_attrs(struct device *dev, struct scatterlist *sg,
 
 	return nents;
 }
+
+/**
+ * pci_p2pdma_map_sg - map a PCI peer-to-peer scatterlist for DMA
+ * @dev: device doing the DMA request
+ * @sg: scatter list to map
+ * @nents: elements in the scatterlist
+ * @dir: DMA direction
+ * @attrs: dma attributes passed to dma_map_sg() (if called)
+ *
+ * Scatterlists mapped with this function should be unmapped using
+ * pci_p2pdma_unmap_sg_attrs().
+ *
+ * Returns the number of SG entries mapped or 0 on error.
+ */
+int pci_p2pdma_map_sg_attrs(struct device *dev, struct scatterlist *sg,
+		int nents, enum dma_data_direction dir, unsigned long attrs)
+{
+	struct dev_pagemap *pgmap = sg_page(sg)->pgmap;
+
+	return __pci_p2pdma_map_sg(pgmap, dev, sg, nents);
+}
 EXPORT_SYMBOL_GPL(pci_p2pdma_map_sg_attrs);
 
 /**
-- 
2.20.1

[PATCH 11/14] PCI/P2PDMA: dma_map P2PDMA map requests that traverse the host bridge

[Logan Gunthorpe]

Any requests that traverse the host bridge will need to be mapped into
the IOMMU, so call dma_map_sg() inside pci_p2pdma_map_sg() when
appropriate.

Similarly, call dma_unmap_sg() inside pci_p2pdma_unmap_sg().

Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
---
 drivers/pci/p2pdma.c | 31 ++++++++++++++++++++++++++++++-
 1 file changed, 30 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c
index 5f43f92f9336..76f51678342c 100644
--- a/drivers/pci/p2pdma.c
+++ b/drivers/pci/p2pdma.c
@@ -830,8 +830,22 @@ int pci_p2pdma_map_sg_attrs(struct device *dev, struct scatterlist *sg,
 		int nents, enum dma_data_direction dir, unsigned long attrs)
 {
 	struct dev_pagemap *pgmap = sg_page(sg)->pgmap;
+	struct pci_dev *client;
+	int dist;
+
+	client = find_parent_pci_dev(dev);
+	if (WARN_ON_ONCE(!client))
+		return 0;
 
-	return __pci_p2pdma_map_sg(pgmap, dev, sg, nents);
+	dist = upstream_bridge_distance(pgmap->pci_p2pdma_provider,
+					client, NULL);
+	if (WARN_ON_ONCE(dist & P2PDMA_NOT_SUPPORTED))
+		return 0;
+
+	if (dist & P2PDMA_THRU_HOST_BRIDGE)
+		return dma_map_sg_attrs(dev, sg, nents, dir, attrs);
+	else
+		return __pci_p2pdma_map_sg(pgmap, dev, sg, nents);
 }
 EXPORT_SYMBOL_GPL(pci_p2pdma_map_sg_attrs);
 
@@ -847,6 +861,21 @@ EXPORT_SYMBOL_GPL(pci_p2pdma_map_sg_attrs);
 void pci_p2pdma_unmap_sg_attrs(struct device *dev, struct scatterlist *sg,
 		int nents, enum dma_data_direction dir, unsigned long attrs)
 {
+	struct dev_pagemap *pgmap = sg_page(sg)->pgmap;
+	struct pci_dev *client;
+	int dist;
+
+	client = find_parent_pci_dev(dev);
+	if (!client)
+		return;
+
+	dist = upstream_bridge_distance(pgmap->pci_p2pdma_provider,
+					client, NULL);
+	if (dist & P2PDMA_NOT_SUPPORTED)
+		return;
+
+	if (dist & P2PDMA_THRU_HOST_BRIDGE)
+		dma_unmap_sg_attrs(dev, sg, nents, dir, attrs);
 }
 EXPORT_SYMBOL_GPL(pci_p2pdma_unmap_sg_attrs);
 
-- 
2.20.1

Re: [PATCH 11/14] PCI/P2PDMA: dma_map P2PDMA map requests that traverse the host bridge

[Christoph Hellwig]

>  	struct dev_pagemap *pgmap = sg_page(sg)->pgmap;
> +	struct pci_dev *client;
> +	int dist;
> +
> +	client = find_parent_pci_dev(dev);
> +	if (WARN_ON_ONCE(!client))
> +		return 0;
>  
> +	dist = upstream_bridge_distance(pgmap->pci_p2pdma_provider,
> +					client, NULL);

Doing this on every mapping call sounds expensive..

> +	if (WARN_ON_ONCE(dist & P2PDMA_NOT_SUPPORTED))
> +		return 0;
> +
> +	if (dist & P2PDMA_THRU_HOST_BRIDGE)
> +		return dma_map_sg_attrs(dev, sg, nents, dir, attrs);
> +	else
> +		return __pci_p2pdma_map_sg(pgmap, dev, sg, nents);

Can't we organize the values so that we can switch on the return
value instead of doing flag checks?

>  }
>  EXPORT_SYMBOL_GPL(pci_p2pdma_map_sg_attrs);
>  
> @@ -847,6 +861,21 @@ EXPORT_SYMBOL_GPL(pci_p2pdma_map_sg_attrs);
>  void pci_p2pdma_unmap_sg_attrs(struct device *dev, struct scatterlist *sg,
>  		int nents, enum dma_data_direction dir, unsigned long attrs)
>  {
> +	struct dev_pagemap *pgmap = sg_page(sg)->pgmap;
> +	struct pci_dev *client;
> +	int dist;
> +
> +	client = find_parent_pci_dev(dev);
> +	if (!client)
> +		return;
> +
> +	dist = upstream_bridge_distance(pgmap->pci_p2pdma_provider,
> +					client, NULL);

And then we do it for every unmap again..

Re: [PATCH 11/14] PCI/P2PDMA: dma_map P2PDMA map requests that traverse the host bridge

[Logan Gunthorpe]

On 2019-07-24 12:32 a.m., Christoph Hellwig wrote:
>>  	struct dev_pagemap *pgmap = sg_page(sg)->pgmap;
>> +	struct pci_dev *client;
>> +	int dist;
>> +
>> +	client = find_parent_pci_dev(dev);
>> +	if (WARN_ON_ONCE(!client))
>> +		return 0;
>>  
>> +	dist = upstream_bridge_distance(pgmap->pci_p2pdma_provider,
>> +					client, NULL);
> 
> Doing this on every mapping call sounds expensive..

The result of this function is cached in an xarray (per patch 4) so, on
the hot path, it should just be a single xa_load() which should be a
relatively fast lookup which is similarly used for other hot path
operations.

> 
>> +	if (WARN_ON_ONCE(dist & P2PDMA_NOT_SUPPORTED))
>> +		return 0;
>> +
>> +	if (dist & P2PDMA_THRU_HOST_BRIDGE)
>> +		return dma_map_sg_attrs(dev, sg, nents, dir, attrs);
>> +	else
>> +		return __pci_p2pdma_map_sg(pgmap, dev, sg, nents);
> 
> Can't we organize the values so that we can switch on the return
> value instead of doing flag checks?

Sorry, I don't follow what you are saying here. If you mean for
upstream_bridge_distance() to just return how to map and not the
distance that would interfere with other uses of that function.

>>  }
>>  EXPORT_SYMBOL_GPL(pci_p2pdma_map_sg_attrs);
>>  
>> @@ -847,6 +861,21 @@ EXPORT_SYMBOL_GPL(pci_p2pdma_map_sg_attrs);
>>  void pci_p2pdma_unmap_sg_attrs(struct device *dev, struct scatterlist *sg,
>>  		int nents, enum dma_data_direction dir, unsigned long attrs)
>>  {
>> +	struct dev_pagemap *pgmap = sg_page(sg)->pgmap;
>> +	struct pci_dev *client;
>> +	int dist;
>> +
>> +	client = find_parent_pci_dev(dev);
>> +	if (!client)
>> +		return;
>> +
>> +	dist = upstream_bridge_distance(pgmap->pci_p2pdma_provider,
>> +					client, NULL);
> 
> And then we do it for every unmap again..

Yup, I don't think there's much else we can do here.

This is why I was fighting against doing lookups against the phys_addr_t
because that means you have to do these additional lookups.

My hope is if we can move to the phys_addr_t and flags as I described
here[1] we can get rid of these hot path lookups, but with the way
things are structured now this is necessary.

Logan

[1]
https://lore.kernel.org/linux-block/e63d0259-e17f-effe-b76d-43dbfda8ae3a@deltatee.com/

Re: [PATCH 11/14] PCI/P2PDMA: dma_map P2PDMA map requests that traverse the host bridge

[Christoph Hellwig]

On Wed, Jul 24, 2019 at 09:58:59AM -0600, Logan Gunthorpe wrote:
> 
> 
> On 2019-07-24 12:32 a.m., Christoph Hellwig wrote:
> >>  	struct dev_pagemap *pgmap = sg_page(sg)->pgmap;
> >> +	struct pci_dev *client;
> >> +	int dist;
> >> +
> >> +	client = find_parent_pci_dev(dev);
> >> +	if (WARN_ON_ONCE(!client))
> >> +		return 0;
> >>  
> >> +	dist = upstream_bridge_distance(pgmap->pci_p2pdma_provider,
> >> +					client, NULL);
> > 
> > Doing this on every mapping call sounds expensive..
> 
> The result of this function is cached in an xarray (per patch 4) so, on
> the hot path, it should just be a single xa_load() which should be a
> relatively fast lookup which is similarly used for other hot path
> operations.

We don't cache find_parent_pci_dev, though.  So we should probably
export find_parent_pci_dev with a proper namespaces name and cache
that in the caler.

> > 
> >> +	if (WARN_ON_ONCE(dist & P2PDMA_NOT_SUPPORTED))
> >> +		return 0;
> >> +
> >> +	if (dist & P2PDMA_THRU_HOST_BRIDGE)
> >> +		return dma_map_sg_attrs(dev, sg, nents, dir, attrs);
> >> +	else
> >> +		return __pci_p2pdma_map_sg(pgmap, dev, sg, nents);
> > 
> > Can't we organize the values so that we can switch on the return
> > value instead of doing flag checks?
> 
> Sorry, I don't follow what you are saying here. If you mean for
> upstream_bridge_distance() to just return how to map and not the
> distance that would interfere with other uses of that function.

The point is that in the map path we don't even care about the
distance.  I think we should just have a function that returns the
P2PDMA_ values from the xarray (maybe also store it there as two
values, but that isn't quite as important), and get rid of even
the concept of distance in the map path. e.g.:

	switch (pci_p2pdma_supported(pgmap->pci_p2pdma_provider, client))) {
	case P2PDMA_HOST_BRIDGE:
		return dma_map_sg_attrs(dev, sg, nents, dir, attrs);
	case P2PDMA_SWITCH:
		return __pci_p2pdma_map_sg(pgmap, dev, sg, nents);
	default:
		WARN_ON_ONCE(1);
		return 0;
	}

Re: [PATCH 11/14] PCI/P2PDMA: dma_map P2PDMA map requests that traverse the host bridge

[Logan Gunthorpe]

On 2019-07-25 12:10 a.m., Christoph Hellwig wrote:
> On Wed, Jul 24, 2019 at 09:58:59AM -0600, Logan Gunthorpe wrote:
>>
>>
>> On 2019-07-24 12:32 a.m., Christoph Hellwig wrote:
>>>>  	struct dev_pagemap *pgmap = sg_page(sg)->pgmap;
>>>> +	struct pci_dev *client;
>>>> +	int dist;
>>>> +
>>>> +	client = find_parent_pci_dev(dev);
>>>> +	if (WARN_ON_ONCE(!client))
>>>> +		return 0;
>>>>  
>>>> +	dist = upstream_bridge_distance(pgmap->pci_p2pdma_provider,
>>>> +					client, NULL);
>>>
>>> Doing this on every mapping call sounds expensive..
>>
>> The result of this function is cached in an xarray (per patch 4) so, on
>> the hot path, it should just be a single xa_load() which should be a
>> relatively fast lookup which is similarly used for other hot path
>> operations.
> 
> We don't cache find_parent_pci_dev, though.  So we should probably
> export find_parent_pci_dev with a proper namespaces name and cache
> that in the caler.

Oh, yes, I'll take a look at this. Of the two callers: NVMe should be
easy we could just pass the PCI device instead of the struct device.
RDMA is significantly more unclear: would you add a pci_dev to struct
ib_device? Or maybe we should be able to simply rely on the fact that
the DMA device *must* be a PCI device and just use to_pci_dev() directly?

>>>
>>>> +	if (WARN_ON_ONCE(dist & P2PDMA_NOT_SUPPORTED))
>>>> +		return 0;
>>>> +
>>>> +	if (dist & P2PDMA_THRU_HOST_BRIDGE)
>>>> +		return dma_map_sg_attrs(dev, sg, nents, dir, attrs);
>>>> +	else
>>>> +		return __pci_p2pdma_map_sg(pgmap, dev, sg, nents);
>>>
>>> Can't we organize the values so that we can switch on the return
>>> value instead of doing flag checks?
>>
>> Sorry, I don't follow what you are saying here. If you mean for
>> upstream_bridge_distance() to just return how to map and not the
>> distance that would interfere with other uses of that function.
> 
> The point is that in the map path we don't even care about the
> distance.  I think we should just have a function that returns the
> P2PDMA_ values from the xarray (maybe also store it there as two
> values, but that isn't quite as important), and get rid of even
> the concept of distance in the map path. e.g.:
> 
> 	switch (pci_p2pdma_supported(pgmap->pci_p2pdma_provider, client))) {
> 	case P2PDMA_HOST_BRIDGE:
> 		return dma_map_sg_attrs(dev, sg, nents, dir, attrs);
> 	case P2PDMA_SWITCH:
> 		return __pci_p2pdma_map_sg(pgmap, dev, sg, nents);
> 	default:
> 		WARN_ON_ONCE(1);
> 		return 0;
> 	}

Ok, will change for v2.

Thanks,

Logan

Re: [PATCH 11/14] PCI/P2PDMA: dma_map P2PDMA map requests that traverse the host bridge

[Jason Gunthorpe]

On Thu, Jul 25, 2019 at 10:00:25AM -0600, Logan Gunthorpe wrote:
> 
> 
> On 2019-07-25 12:10 a.m., Christoph Hellwig wrote:
> > On Wed, Jul 24, 2019 at 09:58:59AM -0600, Logan Gunthorpe wrote:
> >>
> >>
> >> On 2019-07-24 12:32 a.m., Christoph Hellwig wrote:
> >>>>  	struct dev_pagemap *pgmap = sg_page(sg)->pgmap;
> >>>> +	struct pci_dev *client;
> >>>> +	int dist;
> >>>> +
> >>>> +	client = find_parent_pci_dev(dev);
> >>>> +	if (WARN_ON_ONCE(!client))
> >>>> +		return 0;
> >>>>  
> >>>> +	dist = upstream_bridge_distance(pgmap->pci_p2pdma_provider,
> >>>> +					client, NULL);
> >>>
> >>> Doing this on every mapping call sounds expensive..
> >>
> >> The result of this function is cached in an xarray (per patch 4) so, on
> >> the hot path, it should just be a single xa_load() which should be a
> >> relatively fast lookup which is similarly used for other hot path
> >> operations.
> > 
> > We don't cache find_parent_pci_dev, though.  So we should probably
> > export find_parent_pci_dev with a proper namespaces name and cache
> > that in the caler.
> 
> Oh, yes, I'll take a look at this. Of the two callers: NVMe should be
> easy we could just pass the PCI device instead of the struct device.
> RDMA is significantly more unclear: would you add a pci_dev to struct
> ib_device? Or maybe we should be able to simply rely on the fact that
> the DMA device *must* be a PCI device and just use to_pci_dev() directly?

AFAIK you need to use the ib_device->dma_device and add some kind of
is_pci_dev to make it safe

Jason

Re: [PATCH 11/14] PCI/P2PDMA: dma_map P2PDMA map requests that traverse the host bridge

[Logan Gunthorpe]

On 2019-07-25 10:34 a.m., Jason Gunthorpe wrote:
> On Thu, Jul 25, 2019 at 10:00:25AM -0600, Logan Gunthorpe wrote:
>>
>>
>> On 2019-07-25 12:10 a.m., Christoph Hellwig wrote:
>>> On Wed, Jul 24, 2019 at 09:58:59AM -0600, Logan Gunthorpe wrote:
>>>>
>>>>
>>>> On 2019-07-24 12:32 a.m., Christoph Hellwig wrote:
>>>>>>  	struct dev_pagemap *pgmap = sg_page(sg)->pgmap;
>>>>>> +	struct pci_dev *client;
>>>>>> +	int dist;
>>>>>> +
>>>>>> +	client = find_parent_pci_dev(dev);
>>>>>> +	if (WARN_ON_ONCE(!client))
>>>>>> +		return 0;
>>>>>>  
>>>>>> +	dist = upstream_bridge_distance(pgmap->pci_p2pdma_provider,
>>>>>> +					client, NULL);
>>>>>
>>>>> Doing this on every mapping call sounds expensive..
>>>>
>>>> The result of this function is cached in an xarray (per patch 4) so, on
>>>> the hot path, it should just be a single xa_load() which should be a
>>>> relatively fast lookup which is similarly used for other hot path
>>>> operations.
>>>
>>> We don't cache find_parent_pci_dev, though.  So we should probably
>>> export find_parent_pci_dev with a proper namespaces name and cache
>>> that in the caler.
>>
>> Oh, yes, I'll take a look at this. Of the two callers: NVMe should be
>> easy we could just pass the PCI device instead of the struct device.
>> RDMA is significantly more unclear: would you add a pci_dev to struct
>> ib_device? Or maybe we should be able to simply rely on the fact that
>> the DMA device *must* be a PCI device and just use to_pci_dev() directly?
> 
> AFAIK you need to use the ib_device->dma_device and add some kind of
> is_pci_dev to make it safe

Yes, that's my thinking. The dma_device *should* be a PCI device. We can
just be sure by doing is_pci_dev() and failing the mapping if it is not.

So I *think* we should be able to simply replace the
find_parent_pci_dev() with:

if (!dev_is_pci(dev))
     return 0;

client = to_pci_dev(dev);

Which should be fast and reliable.

The alternative is to push this out into the caller which may have a bit
more information (like the nvme driver does).

Logan

Re: [PATCH 11/14] PCI/P2PDMA: dma_map P2PDMA map requests that traverse the host bridge

[Jason Gunthorpe]

On Mon, Jul 22, 2019 at 05:08:56PM -0600, Logan Gunthorpe wrote:
> Any requests that traverse the host bridge will need to be mapped into
> the IOMMU, so call dma_map_sg() inside pci_p2pdma_map_sg() when
> appropriate.
> 
> Similarly, call dma_unmap_sg() inside pci_p2pdma_unmap_sg().
> 
> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
>  drivers/pci/p2pdma.c | 31 ++++++++++++++++++++++++++++++-
>  1 file changed, 30 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c
> index 5f43f92f9336..76f51678342c 100644
> +++ b/drivers/pci/p2pdma.c
> @@ -830,8 +830,22 @@ int pci_p2pdma_map_sg_attrs(struct device *dev, struct scatterlist *sg,
>  		int nents, enum dma_data_direction dir, unsigned long attrs)
>  {
>  	struct dev_pagemap *pgmap = sg_page(sg)->pgmap;
> +	struct pci_dev *client;
> +	int dist;
> +
> +	client = find_parent_pci_dev(dev);
> +	if (WARN_ON_ONCE(!client))
> +		return 0;
>  
> -	return __pci_p2pdma_map_sg(pgmap, dev, sg, nents);
> +	dist = upstream_bridge_distance(pgmap->pci_p2pdma_provider,
> +					client, NULL);
> +	if (WARN_ON_ONCE(dist & P2PDMA_NOT_SUPPORTED))
> +		return 0;
> +
> +	if (dist & P2PDMA_THRU_HOST_BRIDGE)
> +		return dma_map_sg_attrs(dev, sg, nents, dir, attrs);

IIRC at this point the SG will have struct page references to the BAR
memory - so (all?) the IOMMU drivers are able to handle P2P setup in
this case?

Didn't you also have a patch to remove the struct page's for the BAR
memory?  

Confused what the plan is here

Jason

Re: [PATCH 11/14] PCI/P2PDMA: dma_map P2PDMA map requests that traverse the host bridge

[Logan Gunthorpe]

On 2019-07-25 12:58 p.m., Jason Gunthorpe wrote:
> On Mon, Jul 22, 2019 at 05:08:56PM -0600, Logan Gunthorpe wrote:
>> Any requests that traverse the host bridge will need to be mapped into
>> the IOMMU, so call dma_map_sg() inside pci_p2pdma_map_sg() when
>> appropriate.
>>
>> Similarly, call dma_unmap_sg() inside pci_p2pdma_unmap_sg().
>>
>> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
>>  drivers/pci/p2pdma.c | 31 ++++++++++++++++++++++++++++++-
>>  1 file changed, 30 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c
>> index 5f43f92f9336..76f51678342c 100644
>> +++ b/drivers/pci/p2pdma.c
>> @@ -830,8 +830,22 @@ int pci_p2pdma_map_sg_attrs(struct device *dev, struct scatterlist *sg,
>>  		int nents, enum dma_data_direction dir, unsigned long attrs)
>>  {
>>  	struct dev_pagemap *pgmap = sg_page(sg)->pgmap;
>> +	struct pci_dev *client;
>> +	int dist;
>> +
>> +	client = find_parent_pci_dev(dev);
>> +	if (WARN_ON_ONCE(!client))
>> +		return 0;
>>  
>> -	return __pci_p2pdma_map_sg(pgmap, dev, sg, nents);
>> +	dist = upstream_bridge_distance(pgmap->pci_p2pdma_provider,
>> +					client, NULL);
>> +	if (WARN_ON_ONCE(dist & P2PDMA_NOT_SUPPORTED))
>> +		return 0;
>> +
>> +	if (dist & P2PDMA_THRU_HOST_BRIDGE)
>> +		return dma_map_sg_attrs(dev, sg, nents, dir, attrs);
> 
> IIRC at this point the SG will have struct page references to the BAR
> memory - so (all?) the IOMMU drivers are able to handle P2P setup in
> this case?

Yes. The IOMMU drivers refer to the physical address for BAR which they
can get from the struct page. And this works fine today.

> Didn't you also have a patch to remove the struct page's for the BAR
> memory?  

Well that was an RFC that's not going anywhere in it's current form. I'd
still like to remove struct pages but that's going to take a while and
when that happens, this will have to be reworked. Probably to use
dma_map_resource() instead but will depend on the form the removal takes.

Logan

Re: [PATCH 11/14] PCI/P2PDMA: dma_map P2PDMA map requests that traverse the host bridge

[Jason Gunthorpe]

On Thu, Jul 25, 2019 at 01:17:02PM -0600, Logan Gunthorpe wrote:
> 
> 
> On 2019-07-25 12:58 p.m., Jason Gunthorpe wrote:
> > On Mon, Jul 22, 2019 at 05:08:56PM -0600, Logan Gunthorpe wrote:
> >> Any requests that traverse the host bridge will need to be mapped into
> >> the IOMMU, so call dma_map_sg() inside pci_p2pdma_map_sg() when
> >> appropriate.
> >>
> >> Similarly, call dma_unmap_sg() inside pci_p2pdma_unmap_sg().
> >>
> >> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
> >>  drivers/pci/p2pdma.c | 31 ++++++++++++++++++++++++++++++-
> >>  1 file changed, 30 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c
> >> index 5f43f92f9336..76f51678342c 100644
> >> +++ b/drivers/pci/p2pdma.c
> >> @@ -830,8 +830,22 @@ int pci_p2pdma_map_sg_attrs(struct device *dev, struct scatterlist *sg,
> >>  		int nents, enum dma_data_direction dir, unsigned long attrs)
> >>  {
> >>  	struct dev_pagemap *pgmap = sg_page(sg)->pgmap;
> >> +	struct pci_dev *client;
> >> +	int dist;
> >> +
> >> +	client = find_parent_pci_dev(dev);
> >> +	if (WARN_ON_ONCE(!client))
> >> +		return 0;
> >>  
> >> -	return __pci_p2pdma_map_sg(pgmap, dev, sg, nents);
> >> +	dist = upstream_bridge_distance(pgmap->pci_p2pdma_provider,
> >> +					client, NULL);

Isn't is a bit of a leap to assume that the pgmap is uniform across
all the sgs?

> >> +	if (WARN_ON_ONCE(dist & P2PDMA_NOT_SUPPORTED))
> >> +		return 0;
> >> +
> >> +	if (dist & P2PDMA_THRU_HOST_BRIDGE)
> >> +		return dma_map_sg_attrs(dev, sg, nents, dir, attrs);
> > 
> > IIRC at this point the SG will have struct page references to the BAR
> > memory - so (all?) the IOMMU drivers are able to handle P2P setup in
> > this case?
> 
> Yes. The IOMMU drivers refer to the physical address for BAR which they
> can get from the struct page. And this works fine today.

Interesting.

So, for the places where we already map BAR memory to userspace, if I
were to make struct pages for those BARs and use vm_insert_page()
instead of io_remap_pfn_range(), then the main thing missing in RDMA
to actually do P2P DMA is a way to get those struct pages out of
get_user_pages and know to call the pci_p2pdma_map_sg version (ie in
ib_umem_get())?

Jason

Re: [PATCH 11/14] PCI/P2PDMA: dma_map P2PDMA map requests that traverse the host bridge

[Logan Gunthorpe]

On 2019-07-25 1:29 p.m., Jason Gunthorpe wrote:
> On Thu, Jul 25, 2019 at 01:17:02PM -0600, Logan Gunthorpe wrote:
>>
>>
>> On 2019-07-25 12:58 p.m., Jason Gunthorpe wrote:
>>> On Mon, Jul 22, 2019 at 05:08:56PM -0600, Logan Gunthorpe wrote:
>>>> Any requests that traverse the host bridge will need to be mapped into
>>>> the IOMMU, so call dma_map_sg() inside pci_p2pdma_map_sg() when
>>>> appropriate.
>>>>
>>>> Similarly, call dma_unmap_sg() inside pci_p2pdma_unmap_sg().
>>>>
>>>> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
>>>>  drivers/pci/p2pdma.c | 31 ++++++++++++++++++++++++++++++-
>>>>  1 file changed, 30 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c
>>>> index 5f43f92f9336..76f51678342c 100644
>>>> +++ b/drivers/pci/p2pdma.c
>>>> @@ -830,8 +830,22 @@ int pci_p2pdma_map_sg_attrs(struct device *dev, struct scatterlist *sg,
>>>>  		int nents, enum dma_data_direction dir, unsigned long attrs)
>>>>  {
>>>>  	struct dev_pagemap *pgmap = sg_page(sg)->pgmap;
>>>> +	struct pci_dev *client;
>>>> +	int dist;
>>>> +
>>>> +	client = find_parent_pci_dev(dev);
>>>> +	if (WARN_ON_ONCE(!client))
>>>> +		return 0;
>>>>  
>>>> -	return __pci_p2pdma_map_sg(pgmap, dev, sg, nents);
>>>> +	dist = upstream_bridge_distance(pgmap->pci_p2pdma_provider,
>>>> +					client, NULL);
> 
> Isn't is a bit of a leap to assume that the pgmap is uniform across
> all the sgs?

This is definitely a wart but there's not much we can do about it.
Currently we can't support mixing p2p pages with regular pages. In the
same way we can't support mixing p2p pages from different sources. No
current users do that and it would be weird for them to want to at this
point.

>>>> +	if (WARN_ON_ONCE(dist & P2PDMA_NOT_SUPPORTED))
>>>> +		return 0;
>>>> +
>>>> +	if (dist & P2PDMA_THRU_HOST_BRIDGE)
>>>> +		return dma_map_sg_attrs(dev, sg, nents, dir, attrs);
>>>
>>> IIRC at this point the SG will have struct page references to the BAR
>>> memory - so (all?) the IOMMU drivers are able to handle P2P setup in
>>> this case?
>>
>> Yes. The IOMMU drivers refer to the physical address for BAR which they
>> can get from the struct page. And this works fine today.
> 
> Interesting.
> 
> So, for the places where we already map BAR memory to userspace, if I
> were to make struct pages for those BARs and use vm_insert_page()
> instead of io_remap_pfn_range(), then the main thing missing in RDMA
> to actually do P2P DMA is a way to get those struct pages out of
> get_user_pages and know to call the pci_p2pdma_map_sg version (ie in
> ib_umem_get())?

Yes, we've been doing that for a long time with hacky code that would
never get upstream.

Essentially, if you expose those pages to userspace we also need to
ensure that all other users of GUP either reject those pages or map them
correctly.

Logan

[PATCH 12/14] PCI/P2PDMA: No longer require no-mmu for host bridge whitelist

[Logan Gunthorpe]

Now that we map the requests correctly we can remove the iommu_present()
restriction.

Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
---
 drivers/pci/p2pdma.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c
index 76f51678342c..053f9ad59d79 100644
--- a/drivers/pci/p2pdma.c
+++ b/drivers/pci/p2pdma.c
@@ -18,7 +18,6 @@
 #include <linux/percpu-refcount.h>
 #include <linux/random.h>
 #include <linux/seq_buf.h>
-#include <linux/iommu.h>
 #include <linux/xarray.h>
 
 struct pci_p2pdma {
@@ -303,9 +302,6 @@ static bool host_bridge_whitelist(struct pci_dev *a, struct pci_dev *b)
 	struct pci_host_bridge *host_a = pci_find_host_bridge(a->bus);
 	struct pci_host_bridge *host_b = pci_find_host_bridge(b->bus);
 
-	if (iommu_present(a->dev.bus) || iommu_present(b->dev.bus))
-		return false;
-
 	if (host_a == host_b)
 		return __host_bridge_whitelist(host_a, true);
 
-- 
2.20.1

[PATCH 13/14] PCI/P2PDMA: Update documentation for pci_p2pdma_distance_many()

[Logan Gunthorpe]

The comment describing pci_p2pdma_distance_many() still referred to
the devices being behind the same root port. This no longer applies
so reword the documentation.

Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
---
 drivers/pci/p2pdma.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c
index 053f9ad59d79..baf476039396 100644
--- a/drivers/pci/p2pdma.c
+++ b/drivers/pci/p2pdma.c
@@ -496,15 +496,14 @@ static int upstream_bridge_distance_warn(struct pci_dev *provider,
  * @num_clients: number of clients in the array
  * @verbose: if true, print warnings for devices when we return -1
  *
- * Returns -1 if any of the clients are not compatible (behind the same
- * root port as the provider), otherwise returns a positive number where
- * a lower number is the preferable choice. (If there's one client
- * that's the same as the provider it will return 0, which is best choice).
+ * Returns -1 if any of the clients are not compatible, otherwise returns a
+ * positive number where a lower number is the preferable choice. (If there's
+ * one client that's the same as the provider it will return 0, which is best
+ * choice).
  *
- * For now, "compatible" means the provider and the clients are all behind
- * the same PCI root port. This cuts out cases that may work but is safest
- * for the user. Future work can expand this to white-list root complexes that
- * can safely forward between each ports.
+ * "compatible" means the provider and the clients are either all behind
+ * the same PCI root port or the host bridge connected to each of the devices
+ * are is listed in the 'pci_p2pdma_whitelist'.
  */
 int pci_p2pdma_distance_many(struct pci_dev *provider, struct device **clients,
 			     int num_clients, bool verbose)
-- 
2.20.1

[PATCH 14/14] PCI/P2PDMA: Introduce pci_p2pdma_[un]map_resource()

[Logan Gunthorpe]

pci_p2pdma_[un]map_resource() can be used to map a resource given
it's physical address and the backing pci_dev. The functions will call
dma_[un]map_resource() when appropriate.

This is for demonstration purposes only as there are no users of this
function at this time. Thus, this patch should not be merged at
this time.

Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
---
 drivers/pci/p2pdma.c | 85 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 85 insertions(+)

diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c
index baf476039396..20c834cfd2d3 100644
--- a/drivers/pci/p2pdma.c
+++ b/drivers/pci/p2pdma.c
@@ -874,6 +874,91 @@ void pci_p2pdma_unmap_sg_attrs(struct device *dev, struct scatterlist *sg,
 }
 EXPORT_SYMBOL_GPL(pci_p2pdma_unmap_sg_attrs);
 
+static pci_bus_addr_t pci_p2pdma_phys_to_bus(struct pci_dev *dev,
+		phys_addr_t start, size_t size)
+{
+	struct pci_host_bridge *bridge = pci_find_host_bridge(dev->bus);
+	phys_addr_t end = start + size;
+	struct resource_entry *window;
+
+	resource_list_for_each_entry(window, &bridge->windows) {
+		if (window->res->start <= start && window->res->end >= end)
+			return start - window->offset;
+	}
+
+	return DMA_MAPPING_ERROR;
+}
+EXPORT_SYMBOL_GPL(pci_p2pdma_phys_to_bus);
+
+/**
+ * pci_p2pdma_map_resource - map a PCI peer-to-peer physical address for DMA
+ * @provider: pci device that provides the memory backed by phys_addr
+ * @dma_dev: device doing the DMA request
+ * @phys_addr: physical address of the memory to map
+ * @size: size of the memory to map
+ * @dir: DMA direction
+ * @attrs: dma attributes passed to dma_map_resource() (if called)
+ *
+ * Maps a BAR physical address for programming a DMA engine.
+ *
+ * Returns the dma_addr_t to map or DMA_MAPPING_ERROR on failure
+ */
+dma_addr_t pci_p2pdma_map_resource(struct pci_dev *provider,
+		struct device *dma_dev, phys_addr_t phys_addr, size_t size,
+		enum dma_data_direction dir, unsigned long attrs)
+{
+	struct pci_dev *client;
+	int dist;
+
+	client = find_parent_pci_dev(dma_dev);
+	if (!client)
+		return DMA_MAPPING_ERROR;
+
+	dist = upstream_bridge_distance(provider, client, NULL);
+	if (dist & P2PDMA_NOT_SUPPORTED)
+		return DMA_MAPPING_ERROR;
+
+	if (dist & P2PDMA_THRU_HOST_BRIDGE)
+		return dma_map_resource(dma_dev, phys_addr, size, dir, attrs);
+	else
+		return pci_p2pdma_phys_to_bus(provider, phys_addr, size);
+}
+EXPORT_SYMBOL_GPL(pci_p2pdma_map_resource);
+
+/**
+ * pci_p2pdma_unmap_resource - unmap a resource mapped with
+ *		pci_p2pdma_map_resource()
+ * @provider: pci device that provides the memory backed by phys_addr
+ * @dma_dev: device doing the DMA request
+ * @addr: dma address returned by pci_p2pdma_unmap_resource()
+ * @size: size of the memory to map
+ * @dir: DMA direction
+ * @attrs: dma attributes passed to dma_unmap_resource() (if called)
+ *
+ * Maps a BAR physical address for programming a DMA engine.
+ *
+ * Returns the dma_addr_t to map or DMA_MAPPING_ERROR on failure
+ */
+void pci_p2pdma_unmap_resource(struct pci_dev *provider,
+		struct device *dma_dev, dma_addr_t addr, size_t size,
+		enum dma_data_direction dir, unsigned long attrs)
+{
+	struct pci_dev *client;
+	int dist;
+
+	client = find_parent_pci_dev(dma_dev);
+	if (!client)
+		return;
+
+	dist = upstream_bridge_distance(provider, client, NULL);
+	if (dist & P2PDMA_NOT_SUPPORTED)
+		return;
+
+	if (dist & P2PDMA_THRU_HOST_BRIDGE)
+		dma_unmap_resource(dma_dev, addr, size, dir, attrs);
+}
+EXPORT_SYMBOL_GPL(pci_p2pdma_unmap_resource);
+
 /**
  * pci_p2pdma_enable_store - parse a configfs/sysfs attribute store
  *		to enable p2pdma
-- 
2.20.1

Re: [PATCH 14/14] PCI/P2PDMA: Introduce pci_p2pdma_[un]map_resource()

[Koenig, Christian]

Am 23.07.19 um 01:08 schrieb Logan Gunthorpe:
> pci_p2pdma_[un]map_resource() can be used to map a resource given
> it's physical address and the backing pci_dev. The functions will call
> dma_[un]map_resource() when appropriate.
>
> This is for demonstration purposes only as there are no users of this
> function at this time. Thus, this patch should not be merged at
> this time.
>
> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>

Not sure if pci_p2pdma_phys_to_bus actually needs to be exported. But 
apart from that looks fine to me.

Reviewed-by: Christian König <christian.koenig@amd.com>

Christian.

> ---
>   drivers/pci/p2pdma.c | 85 ++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 85 insertions(+)
>
> diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c
> index baf476039396..20c834cfd2d3 100644
> --- a/drivers/pci/p2pdma.c
> +++ b/drivers/pci/p2pdma.c
> @@ -874,6 +874,91 @@ void pci_p2pdma_unmap_sg_attrs(struct device *dev, struct scatterlist *sg,
>   }
>   EXPORT_SYMBOL_GPL(pci_p2pdma_unmap_sg_attrs);
>   
> +static pci_bus_addr_t pci_p2pdma_phys_to_bus(struct pci_dev *dev,
> +		phys_addr_t start, size_t size)
> +{
> +	struct pci_host_bridge *bridge = pci_find_host_bridge(dev->bus);
> +	phys_addr_t end = start + size;
> +	struct resource_entry *window;
> +
> +	resource_list_for_each_entry(window, &bridge->windows) {
> +		if (window->res->start <= start && window->res->end >= end)
> +			return start - window->offset;
> +	}
> +
> +	return DMA_MAPPING_ERROR;
> +}
> +EXPORT_SYMBOL_GPL(pci_p2pdma_phys_to_bus);
> +
> +/**
> + * pci_p2pdma_map_resource - map a PCI peer-to-peer physical address for DMA
> + * @provider: pci device that provides the memory backed by phys_addr
> + * @dma_dev: device doing the DMA request
> + * @phys_addr: physical address of the memory to map
> + * @size: size of the memory to map
> + * @dir: DMA direction
> + * @attrs: dma attributes passed to dma_map_resource() (if called)
> + *
> + * Maps a BAR physical address for programming a DMA engine.
> + *
> + * Returns the dma_addr_t to map or DMA_MAPPING_ERROR on failure
> + */
> +dma_addr_t pci_p2pdma_map_resource(struct pci_dev *provider,
> +		struct device *dma_dev, phys_addr_t phys_addr, size_t size,
> +		enum dma_data_direction dir, unsigned long attrs)
> +{
> +	struct pci_dev *client;
> +	int dist;
> +
> +	client = find_parent_pci_dev(dma_dev);
> +	if (!client)
> +		return DMA_MAPPING_ERROR;
> +
> +	dist = upstream_bridge_distance(provider, client, NULL);
> +	if (dist & P2PDMA_NOT_SUPPORTED)
> +		return DMA_MAPPING_ERROR;
> +
> +	if (dist & P2PDMA_THRU_HOST_BRIDGE)
> +		return dma_map_resource(dma_dev, phys_addr, size, dir, attrs);
> +	else
> +		return pci_p2pdma_phys_to_bus(provider, phys_addr, size);
> +}
> +EXPORT_SYMBOL_GPL(pci_p2pdma_map_resource);
> +
> +/**
> + * pci_p2pdma_unmap_resource - unmap a resource mapped with
> + *		pci_p2pdma_map_resource()
> + * @provider: pci device that provides the memory backed by phys_addr
> + * @dma_dev: device doing the DMA request
> + * @addr: dma address returned by pci_p2pdma_unmap_resource()
> + * @size: size of the memory to map
> + * @dir: DMA direction
> + * @attrs: dma attributes passed to dma_unmap_resource() (if called)
> + *
> + * Maps a BAR physical address for programming a DMA engine.
> + *
> + * Returns the dma_addr_t to map or DMA_MAPPING_ERROR on failure
> + */
> +void pci_p2pdma_unmap_resource(struct pci_dev *provider,
> +		struct device *dma_dev, dma_addr_t addr, size_t size,
> +		enum dma_data_direction dir, unsigned long attrs)
> +{
> +	struct pci_dev *client;
> +	int dist;
> +
> +	client = find_parent_pci_dev(dma_dev);
> +	if (!client)
> +		return;
> +
> +	dist = upstream_bridge_distance(provider, client, NULL);
> +	if (dist & P2PDMA_NOT_SUPPORTED)
> +		return;
> +
> +	if (dist & P2PDMA_THRU_HOST_BRIDGE)
> +		dma_unmap_resource(dma_dev, addr, size, dir, attrs);
> +}
> +EXPORT_SYMBOL_GPL(pci_p2pdma_unmap_resource);
> +
>   /**
>    * pci_p2pdma_enable_store - parse a configfs/sysfs attribute store
>    *		to enable p2pdma

Re: [PATCH 14/14] PCI/P2PDMA: Introduce pci_p2pdma_[un]map_resource()

[Logan Gunthorpe]

On 2019-07-23 10:28 a.m., Koenig, Christian wrote:
> Am 23.07.19 um 01:08 schrieb Logan Gunthorpe:
>> pci_p2pdma_[un]map_resource() can be used to map a resource given
>> it's physical address and the backing pci_dev. The functions will call
>> dma_[un]map_resource() when appropriate.
>>
>> This is for demonstration purposes only as there are no users of this
>> function at this time. Thus, this patch should not be merged at
>> this time.
>>
>> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
> 
> Not sure if pci_p2pdma_phys_to_bus actually needs to be exported. But 
> apart from that looks fine to me.

Yes, oops, it certainly shouldn't be exported if it's static. I'll fix that.

> Reviewed-by: Christian König <christian.koenig@amd.com>
> 
> Christian.
> 
>> ---
>>   drivers/pci/p2pdma.c | 85 ++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 85 insertions(+)
>>
>> diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c
>> index baf476039396..20c834cfd2d3 100644
>> --- a/drivers/pci/p2pdma.c
>> +++ b/drivers/pci/p2pdma.c
>> @@ -874,6 +874,91 @@ void pci_p2pdma_unmap_sg_attrs(struct device *dev, struct scatterlist *sg,
>>   }
>>   EXPORT_SYMBOL_GPL(pci_p2pdma_unmap_sg_attrs);
>>   
>> +static pci_bus_addr_t pci_p2pdma_phys_to_bus(struct pci_dev *dev,
>> +		phys_addr_t start, size_t size)
>> +{
>> +	struct pci_host_bridge *bridge = pci_find_host_bridge(dev->bus);
>> +	phys_addr_t end = start + size;
>> +	struct resource_entry *window;
>> +
>> +	resource_list_for_each_entry(window, &bridge->windows) {
>> +		if (window->res->start <= start && window->res->end >= end)
>> +			return start - window->offset;
>> +	}
>> +
>> +	return DMA_MAPPING_ERROR;
>> +}
>> +EXPORT_SYMBOL_GPL(pci_p2pdma_phys_to_bus);
>> +
>> +/**
>> + * pci_p2pdma_map_resource - map a PCI peer-to-peer physical address for DMA
>> + * @provider: pci device that provides the memory backed by phys_addr
>> + * @dma_dev: device doing the DMA request
>> + * @phys_addr: physical address of the memory to map
>> + * @size: size of the memory to map
>> + * @dir: DMA direction
>> + * @attrs: dma attributes passed to dma_map_resource() (if called)
>> + *
>> + * Maps a BAR physical address for programming a DMA engine.
>> + *
>> + * Returns the dma_addr_t to map or DMA_MAPPING_ERROR on failure
>> + */
>> +dma_addr_t pci_p2pdma_map_resource(struct pci_dev *provider,
>> +		struct device *dma_dev, phys_addr_t phys_addr, size_t size,
>> +		enum dma_data_direction dir, unsigned long attrs)
>> +{
>> +	struct pci_dev *client;
>> +	int dist;
>> +
>> +	client = find_parent_pci_dev(dma_dev);
>> +	if (!client)
>> +		return DMA_MAPPING_ERROR;
>> +
>> +	dist = upstream_bridge_distance(provider, client, NULL);
>> +	if (dist & P2PDMA_NOT_SUPPORTED)
>> +		return DMA_MAPPING_ERROR;
>> +
>> +	if (dist & P2PDMA_THRU_HOST_BRIDGE)
>> +		return dma_map_resource(dma_dev, phys_addr, size, dir, attrs);
>> +	else
>> +		return pci_p2pdma_phys_to_bus(provider, phys_addr, size);
>> +}
>> +EXPORT_SYMBOL_GPL(pci_p2pdma_map_resource);
>> +
>> +/**
>> + * pci_p2pdma_unmap_resource - unmap a resource mapped with
>> + *		pci_p2pdma_map_resource()
>> + * @provider: pci device that provides the memory backed by phys_addr
>> + * @dma_dev: device doing the DMA request
>> + * @addr: dma address returned by pci_p2pdma_unmap_resource()
>> + * @size: size of the memory to map
>> + * @dir: DMA direction
>> + * @attrs: dma attributes passed to dma_unmap_resource() (if called)
>> + *
>> + * Maps a BAR physical address for programming a DMA engine.
>> + *
>> + * Returns the dma_addr_t to map or DMA_MAPPING_ERROR on failure
>> + */
>> +void pci_p2pdma_unmap_resource(struct pci_dev *provider,
>> +		struct device *dma_dev, dma_addr_t addr, size_t size,
>> +		enum dma_data_direction dir, unsigned long attrs)
>> +{
>> +	struct pci_dev *client;
>> +	int dist;
>> +
>> +	client = find_parent_pci_dev(dma_dev);
>> +	if (!client)
>> +		return;
>> +
>> +	dist = upstream_bridge_distance(provider, client, NULL);
>> +	if (dist & P2PDMA_NOT_SUPPORTED)
>> +		return;
>> +
>> +	if (dist & P2PDMA_THRU_HOST_BRIDGE)
>> +		dma_unmap_resource(dma_dev, addr, size, dir, attrs);
>> +}
>> +EXPORT_SYMBOL_GPL(pci_p2pdma_unmap_resource);
>> +
>>   /**
>>    * pci_p2pdma_enable_store - parse a configfs/sysfs attribute store
>>    *		to enable p2pdma
> 

Re: [PATCH 14/14] PCI/P2PDMA: Introduce pci_p2pdma_[un]map_resource()

[Christoph Hellwig]

> diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c
> index baf476039396..20c834cfd2d3 100644
> --- a/drivers/pci/p2pdma.c
> +++ b/drivers/pci/p2pdma.c
> @@ -874,6 +874,91 @@ void pci_p2pdma_unmap_sg_attrs(struct device *dev, struct scatterlist *sg,
>  }
>  EXPORT_SYMBOL_GPL(pci_p2pdma_unmap_sg_attrs);
>  
> +static pci_bus_addr_t pci_p2pdma_phys_to_bus(struct pci_dev *dev,
> +		phys_addr_t start, size_t size)
> +{
> +	struct pci_host_bridge *bridge = pci_find_host_bridge(dev->bus);
> +	phys_addr_t end = start + size;
> +	struct resource_entry *window;
> +
> +	resource_list_for_each_entry(window, &bridge->windows) {
> +		if (window->res->start <= start && window->res->end >= end)
> +			return start - window->offset;
> +	}
> +
> +	return DMA_MAPPING_ERROR;

This does once again look very expensive for something called in the
hot path.

Re: [PATCH 14/14] PCI/P2PDMA: Introduce pci_p2pdma_[un]map_resource()

[Logan Gunthorpe]

On 2019-07-24 12:32 a.m., Christoph Hellwig wrote:
>> diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c
>> index baf476039396..20c834cfd2d3 100644
>> --- a/drivers/pci/p2pdma.c
>> +++ b/drivers/pci/p2pdma.c
>> @@ -874,6 +874,91 @@ void pci_p2pdma_unmap_sg_attrs(struct device *dev, struct scatterlist *sg,
>>  }
>>  EXPORT_SYMBOL_GPL(pci_p2pdma_unmap_sg_attrs);
>>  
>> +static pci_bus_addr_t pci_p2pdma_phys_to_bus(struct pci_dev *dev,
>> +		phys_addr_t start, size_t size)
>> +{
>> +	struct pci_host_bridge *bridge = pci_find_host_bridge(dev->bus);
>> +	phys_addr_t end = start + size;
>> +	struct resource_entry *window;
>> +
>> +	resource_list_for_each_entry(window, &bridge->windows) {
>> +		if (window->res->start <= start && window->res->end >= end)
>> +			return start - window->offset;
>> +	}
>> +
>> +	return DMA_MAPPING_ERROR;
> 
> This does once again look very expensive for something called in the
> hot path.

Yes. This is the downside of dealing only with a phys_addr_t: we have to
look up against it. Unfortunately, I believe it's possible for different
BARs on a device to be in different windows, so something like this is
necessary unless we already know the BAR the phys_addr_t belongs to. It
might probably be sped up a bit by storing the offsets of each bar
instead of looping through all the bridge windows, but I don't think it
will get you *that* much.

As this is an example with no users, the answer here will really depend
on what the use-case is doing. If they can lookup, ahead of time, the
mapping type and offset then they don't have to do this work on the hot
path and it means that pci_p2pdma_map_resource() is simply not a
suitable API.

Logan

Re: [PATCH 14/14] PCI/P2PDMA: Introduce pci_p2pdma_[un]map_resource()

[Christoph Hellwig]

On Wed, Jul 24, 2019 at 10:06:22AM -0600, Logan Gunthorpe wrote:
> Yes. This is the downside of dealing only with a phys_addr_t: we have to
> look up against it. Unfortunately, I believe it's possible for different
> BARs on a device to be in different windows, so something like this is
> necessary unless we already know the BAR the phys_addr_t belongs to. It
> might probably be sped up a bit by storing the offsets of each bar
> instead of looping through all the bridge windows, but I don't think it
> will get you *that* much.
> 
> As this is an example with no users, the answer here will really depend
> on what the use-case is doing. If they can lookup, ahead of time, the
> mapping type and offset then they don't have to do this work on the hot
> path and it means that pci_p2pdma_map_resource() is simply not a
> suitable API.

Ok.  So lets just keep this out as an RFC and don't merge it until an
actual concrete user shows up.

Re: [PATCH 14/14] PCI/P2PDMA: Introduce pci_p2pdma_[un]map_resource()

[Logan Gunthorpe]

On 2019-07-25 5:50 a.m., Christoph Hellwig wrote:
> On Wed, Jul 24, 2019 at 10:06:22AM -0600, Logan Gunthorpe wrote:
>> Yes. This is the downside of dealing only with a phys_addr_t: we have to
>> look up against it. Unfortunately, I believe it's possible for different
>> BARs on a device to be in different windows, so something like this is
>> necessary unless we already know the BAR the phys_addr_t belongs to. It
>> might probably be sped up a bit by storing the offsets of each bar
>> instead of looping through all the bridge windows, but I don't think it
>> will get you *that* much.
>>
>> As this is an example with no users, the answer here will really depend
>> on what the use-case is doing. If they can lookup, ahead of time, the
>> mapping type and offset then they don't have to do this work on the hot
>> path and it means that pci_p2pdma_map_resource() is simply not a
>> suitable API.
> 
> Ok.  So lets just keep this out as an RFC and don't merge it until an
> actual concrete user shows up.


Yup, that was my intention and I mentioned that in the commit message.

Logan

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

[Koenig, Christian]

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

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

Feel free to stick an Acked-by: Christian König 
<christian.koenig@amd.com> to the rest, but I'm not really deep into the 
NVMe P2P handling here.

Regards,
Christian.


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

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

[Logan Gunthorpe]

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

Thanks!

Logan