Re: [PATCH v4 2/3] PCI: Enable support for 10-bit Tag during device enumeration

From: Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com>
To: Lukas Wunner <lukas@wunner.de>
Cc: linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org,
	Bjorn Helgaas <bhelgaas@google.com>,
	Mahesh J Salgaonkar <mahesh@linux.ibm.com>,
	Kuppuswamy Sathyanarayanan 
	<sathyanarayanan.kuppuswamy@linux.intel.com>,
	Yazen Ghannam <yazen.ghannam@amd.com>
Subject: Re: [PATCH v4 2/3] PCI: Enable support for 10-bit Tag during device enumeration
Date: Tue, 12 Sep 2023 15:39:33 -0700	[thread overview]
Message-ID: <57b2b450-2fc9-ac68-d016-fb1d061ba992@amd.com> (raw)
In-Reply-To: <20230828095429.GA17864@wunner.de>

On 8/28/2023 2:54 AM, Lukas Wunner wrote:
> On Tue, Aug 15, 2023 at 09:20:42PM +0000, Smita Koralahalli wrote:
>> +void pci_configure_ten_bit_tag(struct pci_dev *dev)
>> +{
>> +	struct pci_dev *bridge;
>> +	u32 cap;
>> +
>> +	if (!pci_is_pcie(dev))
>> +		return;
>> +
>> +	bridge = dev->bus->self;
>> +	if (!bridge)
>> +		return;
> 
> I think you need to use bridge = pcie_find_root_port(dev) because
> "dev" may be further down in the hierarchy with several switches
> in-between it and the Root Port.
> 
> Note that pcie_find_root_port(dev) returns NULL if !pci_is_pcie(dev),
> so the check above may become unnecessary.
> 
> If pcie_find_root_port(dev) == dev, then dev itself is a Root Port,
> in which case you need to bail out.

Will fix thanks!

> 
> 
>> +	/*
>> +	 * According to PCIe r6.0 sec 7.5.3.15, Requester Supported can only be
>> +	 * set if 10-Bit Tag Completer Supported bit is set.
>> +	 */
>> +	pcie_capability_read_dword(bridge, PCI_EXP_DEVCAP2, &cap);
>> +	if (!(cap & PCI_EXP_DEVCAP2_10BIT_TAG_COMP))
>> +		goto out;
>> +
>> +	if (cap & PCI_EXP_DEVCAP2_10BIT_TAG_REQ) {
> 
> Hm, if Requester Supported cannot be set unless Completer Supported is
> also set, why check for Completer Supported at all?

Makes sense to me. Will change.
> 
> 
>> --- a/drivers/pci/probe.c
>> +++ b/drivers/pci/probe.c
>> @@ -2476,6 +2476,7 @@ static void pci_init_capabilities(struct pci_dev *dev)
>>   	pci_pm_init(dev);		/* Power Management */
>>   	pci_vpd_init(dev);		/* Vital Product Data */
>>   	pci_configure_ari(dev);		/* Alternative Routing-ID Forwarding */
>> +	pci_configure_ten_bit_tag(dev); /* 10-bit Tag Requester */
>>   	pci_iov_init(dev);		/* Single Root I/O Virtualization */
>>   	pci_ats_init(dev);		/* Address Translation Services */
>>   	pci_pri_init(dev);		/* Page Request Interface */
> 
> Hm, isn't this too late to disable 10-bit tags if a hot-plugged device
> doesn't support it?  There are plenty of config space reads/writes
> happening before pci_configure_ten_bit_tag() and if the Root Port
> has 10-bit tags enabled by BIOS because a previously unplugged
> device supported it, I assume the Root Port may use 10-bit tags for
> those config space accesses, despite the newly hotplugged device not
> supporting them?
> 
> If so, you may indeed have to unconditionally disable 10-bit tags
> upon device removal and re-enable them once a 10-bit capable device
> is hotplugged.
> 
> I'm wondering what happens if there are switches between the hotplugged
> device and the Root Port.  In that case, there may be further devices
> in the hierarchy below the Root Port.  I assume 10-bit tags can only be
> enabled if *all* devices below the Root Port support them, is that correct?

You are right! I understand I missed the consideration of involving 
switches and hierarchical PCIe structures.

> 
> The corollary would be that if there's an unoccupied hotplug port somewhere
> in the hierarchy below a Root Port, 10-bit tags cannot be enabled at all
> on the Root Port.

Yes, but the BIOS might have already enabled 10-bit tags on root port 
before this hotplug port becomes unoccupied on hot-remove.

   Maybe we can leave 10-bit tags enabled on hot-removal
> and only disable them on hot-add?

Considering all the challenges, would you think we should 
unconditionally clear 10-bit tags on remove and enable them on add? 
Because the TLPs issue will exist even if we leave the tags enabled on 
removal. Disabling unconditionally would atleast resolve config space 
read/writes and p2pdma issues. What do you think?

  That wouldn't work however if TLPs
> are sent to the hot-added device without operating system involvement
> prior to enumeration by the operating system.  Don't CXL devices
> autonomously send PM messages upstream on hot-add?
> 
> There's another quagmire:  Endpoint devices may talk to each other via
> p2pdma (see drivers/pci/p2pdma.c) and if either of them doesn't support
> 10-bit tags, we need to disable 10-bit tags on them upon commencing
> p2pdma.  We may re-enable 10-bit tags once either of the devices is
> hot-removed or p2pdma between them is stopped.
> 
> Finally, PCIe r6.0 added 14-bit tag support.  It may be worth adding
> 10-bit tag support in a way that 14-bit tag support can easily be added
> later on (or is added together with 10-bit tag support).

I agree.

Thanks,
Smita

> 
> Thanks,
> 
> Lukas