[PATCH v1 1/5] PCI/P2PDMA: Don't enforce ACS check for functions of same device

[PATCH v1 1/5] PCI/P2PDMA: Don't enforce ACS check for functions of same device

[Vivek Kasireddy]

Functions of the same PCI device (such as a PF and a VF) share the
same bus and have a common root port and typically, the PF provisions
resources for the VF. Therefore, they can be considered compatible
as far as P2P access is considered.

Currently, although the distance (2) is correctly calculated for
functions of the same device, an ACS check failure prevents P2P DMA
access between them. Therefore, introduce a small function named
same_pci_device_functions() to determine if the provider and
client belong to the same device and facilitate P2P DMA between
them by not enforcing the ACS check.

Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Logan Gunthorpe <logang@deltatee.com>
Cc: <linux-pci@vger.kernel.org>
Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
---
 drivers/pci/p2pdma.c | 12 ++++++++++--
 include/linux/pci.h  |  1 +
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c
index 4f47a13cb500..34bee1ac94f2 100644
--- a/drivers/pci/p2pdma.c
+++ b/drivers/pci/p2pdma.c
@@ -535,6 +535,12 @@ static unsigned long map_types_idx(struct pci_dev *client)
 	return (pci_domain_nr(client->bus) << 16) | pci_dev_id(client);
 }
 
+static bool same_pci_device_functions(struct pci_dev *provider,
+				      struct pci_dev *client)
+{
+	return pci_physfn(provider) == pci_physfn(client);
+}
+
 /*
  * Calculate the P2PDMA mapping type and distance between two PCI devices.
  *
@@ -634,7 +640,7 @@ calc_map_type_and_dist(struct pci_dev *provider, struct pci_dev *client,
 
 	*dist = dist_a + dist_b;
 
-	if (!acs_cnt) {
+	if (!acs_cnt || same_pci_device_functions(provider, client)) {
 		map_type = PCI_P2PDMA_MAP_BUS_ADDR;
 		goto done;
 	}
@@ -696,7 +702,9 @@ int pci_p2pdma_distance_many(struct pci_dev *provider, struct device **clients,
 		return -1;
 
 	for (i = 0; i < num_clients; i++) {
-		pci_client = find_parent_pci_dev(clients[i]);
+		pci_client = dev_is_pf(clients[i]) || dev_is_vf(clients[i]) ?
+				pci_dev_get(to_pci_dev(clients[i])) :
+				find_parent_pci_dev(clients[i]);
 		if (!pci_client) {
 			if (verbose)
 				dev_warn(clients[i],
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 573b4c4c2be6..a9b07a15f5aa 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1183,6 +1183,7 @@ void pcibios_setup_bridge(struct pci_bus *bus, unsigned long type);
 void pci_sort_breadthfirst(void);
 #define dev_is_pci(d) ((d)->bus == &pci_bus_type)
 #define dev_is_pf(d) ((dev_is_pci(d) ? to_pci_dev(d)->is_physfn : false))
+#define dev_is_vf(d) ((dev_is_pci(d) ? to_pci_dev(d)->is_virtfn : false))
 
 /* Generic PCI functions exported to card drivers */
 
-- 
2.45.1

Re: [PATCH v1 1/5] PCI/P2PDMA: Don't enforce ACS check for functions of same device

[Logan Gunthorpe]

On 2024-10-11 20:40, Vivek Kasireddy wrote:
> Functions of the same PCI device (such as a PF and a VF) share the
> same bus and have a common root port and typically, the PF provisions
> resources for the VF. Therefore, they can be considered compatible
> as far as P2P access is considered.
> 
> Currently, although the distance (2) is correctly calculated for
> functions of the same device, an ACS check failure prevents P2P DMA
> access between them. Therefore, introduce a small function named
> same_pci_device_functions() to determine if the provider and
> client belong to the same device and facilitate P2P DMA between
> them by not enforcing the ACS check.

I'm not totally opposed to this. But the current code was done this way
for a reason: we can't be sure that functions on any given device can
talk to each other. So this change may break if used with other devices
with multiple functions that can't talk to each other.

That being said, the only alternative I can think of is another list of
allowed devices. However, given the pain it's been maintaining allowed
root ports, I'm not very enthusiastic about creating another list of
allowed devices in the kernel.

Logan

RE: [PATCH v1 1/5] PCI/P2PDMA: Don't enforce ACS check for functions of same device

[Kasireddy, Vivek]

Hi Logan,

> 
> On 2024-10-11 20:40, Vivek Kasireddy wrote:
> > Functions of the same PCI device (such as a PF and a VF) share the
> > same bus and have a common root port and typically, the PF provisions
> > resources for the VF. Therefore, they can be considered compatible
> > as far as P2P access is considered.
> >
> > Currently, although the distance (2) is correctly calculated for
> > functions of the same device, an ACS check failure prevents P2P DMA
> > access between them. Therefore, introduce a small function named
> > same_pci_device_functions() to determine if the provider and
> > client belong to the same device and facilitate P2P DMA between
> > them by not enforcing the ACS check.
> 
> I'm not totally opposed to this. But the current code was done this way
> for a reason: we can't be sure that functions on any given device can
> talk to each other. So this change may break if used with other devices
> with multiple functions that can't talk to each other.
> 
> That being said, the only alternative I can think of is another list of
> allowed devices. However, given the pain it's been maintaining allowed
> root ports, I'm not very enthusiastic about creating another list of
> allowed devices in the kernel.
I think it would make sense to limit the passing criteria for device functions'
compatibility to Intel GPUs for now. These are the devices I am currently
testing that we know are P2P compatible. Would this be OK?

Thanks,
Vivek

> 
> Logan

Re: [PATCH v1 1/5] PCI/P2PDMA: Don't enforce ACS check for functions of same device

[Logan Gunthorpe]

On 2024-10-15 23:29, Kasireddy, Vivek wrote:
> I think it would make sense to limit the passing criteria for device functions'
> compatibility to Intel GPUs for now. These are the devices I am currently
> testing that we know are P2P compatible. Would this be OK?

Yes, this sounds good to me. We can reconsider if we get more rules like
it in the future.

Thanks,

Logan