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

[Logan Gunthorpe <logang@deltatee.com>]

On 2024-10-22 09:16, Bjorn Helgaas wrote:
> 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 "---".
> 
>> 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 | 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);
>> +	}

I'd echo many of Bjorn's concerns. In addition, I think the name of the
pci_devs_are_p2pdma_compatible() isn't quite right. Specifically this is
dealing with PCI functions within a single device that are known to
allow P2P traffic. So I think the name should probably reflect that.

Thanks,

Logan