Hi Bjorn,
> Subject: Re: [PATCH v2 1/5] PCI/P2PDMA: Don't enforce ACS check for
> functions of same device
>
> On Sun, Oct 20, 2024 at 10:21:29PM -0700, 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
> > pci_devs_are_p2pdma_compatible() to determine if the provider and
> > client belong to the same device and facilitate P2P DMA between
> > them by not enforcing the ACS check.
> >
> > v2:
> > - Relax the enforcment of ACS check only for Intel GPU functions
> > as they are P2PDMA compatible given the way the PF provisions
> > the resources among multiple VFs.
>
> I don't want version history in the commit log. If the content is
> useful, just incorporate it here directly (without the version info),
> and put the version-to-version changelog below the "---".
Ok, noted; will follow your suggestion for the next versions.
>
> > Cc: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
> > Cc: Logan Gunthorpe <logang@xxxxxxxxxxxx>
> > Cc: <linux-pci@xxxxxxxxxxxxxxx>
> > Signed-off-by: Vivek Kasireddy <vivek.kasireddy@xxxxxxxxx>
> > ---
> > drivers/pci/p2pdma.c | 17 +++++++++++++++--
> > 1 file changed, 15 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c
> > index 4f47a13cb500..a230e661f939 100644
> > --- a/drivers/pci/p2pdma.c
> > +++ b/drivers/pci/p2pdma.c
> > @@ -535,6 +535,17 @@ static unsigned long map_types_idx(struct pci_dev
> *client)
> > return (pci_domain_nr(client->bus) << 16) | pci_dev_id(client);
> > }
> >
> > +static bool pci_devs_are_p2pdma_compatible(struct pci_dev *provider,
> > + struct pci_dev *client)
> > +{
> > + if (provider->vendor == PCI_VENDOR_ID_INTEL) {
> > + if (pci_is_vga(provider) && pci_is_vga(client))
> > + return pci_physfn(provider) == pci_physfn(client);
> > + }
>
> This doesn't explain why this should be specific to Intel or VGA. As
> far as I can tell, everything mentioned in the commit log is generic.
>
> I see the previous comments
> (https://lore.kernel.org/all/eddb423c-945f-40c9-b904-
> 43ea8371f1c4@xxxxxxxxxxxx/),
> but none of that context was captured here.
Ok, I'll augment the commit message to include this context.
>
> I'm not sure what you refer to by "PF provisions resources for the
> VF". Isn't it *always* the case that the architected PCI resources
> (BARs) are configured by the PF? It sounds like you're referring to
> something Intel GPU-specific beyond that?
What I meant to say is that since PF provisions the resources for the VF
in a typical scenario, they should be automatically P2PDMA compatible
particularly when the provider is the VF and PF is the client. However,
since this cannot be guaranteed on all the PCI devices out there for various
reasons, my objective is to start including the ones that can be tested and
are known to be compatible (Intel GPUs).
I'll capture these additional details in the next version.
Thanks,
Vivek
>
> > + return false;
> > +}
> > +
> > /*
> > * Calculate the P2PDMA mapping type and distance between two PCI
> devices.
> > *
> > @@ -634,7 +645,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 || pci_devs_are_p2pdma_compatible(provider, client)) {
> > map_type = PCI_P2PDMA_MAP_BUS_ADDR;
> > goto done;
> > }
> > @@ -696,7 +707,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]) ?
> > + pci_dev_get(to_pci_dev(clients[i])) :
> > + find_parent_pci_dev(clients[i]);
> > if (!pci_client) {
> > if (verbose)
> > dev_warn(clients[i],
> > --
> > 2.45.1
> >
