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.
> + /*
> + * 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?
> --- 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?
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. Maybe we can leave 10-bit tags enabled on hot-removal
and only disable them on hot-add? 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).
Thanks,
Lukas
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