[PATCH] pci: ensure valid link status bits for downstream ports

[PATCH] pci: ensure valid link status bits for downstream ports

[Sebastian Ott]

PCI hotplug for downstream endpoints on arm fails because Linux'
PCIe hotplug driver doesn't like the QEMU provided LNKSTA:

  pcieport 0000:08:01.0: pciehp: Slot(2): Card present
  pcieport 0000:08:01.0: pciehp: Slot(2): Link Up
  pcieport 0000:08:01.0: pciehp: Slot(2): Cannot train link: status 0x2000

There's 2 cases where LNKSTA isn't setup properly:
* the downstream device has no express capability
* max link width of the bridge is 0

Fix these by making the LNKSTA modifications independent of each other.

Signed-off-by: Sebastian Ott <sebott@redhat.com>
---
 hw/pci/pcie.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index 0b455c8654..f714f4fb7c 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -1109,20 +1109,20 @@ void pcie_sync_bridge_lnk(PCIDevice *bridge_dev)
         lnksta = target->config_read(target,
                                      target->exp.exp_cap + PCI_EXP_LNKSTA,
                                      sizeof(lnksta));
-
-        if ((lnksta & PCI_EXP_LNKSTA_NLW) > (lnkcap & PCI_EXP_LNKCAP_MLW)) {
-            lnksta &= ~PCI_EXP_LNKSTA_NLW;
-            lnksta |= lnkcap & PCI_EXP_LNKCAP_MLW;
-        } else if (!(lnksta & PCI_EXP_LNKSTA_NLW)) {
-            lnksta |= QEMU_PCI_EXP_LNKSTA_NLW(QEMU_PCI_EXP_LNK_X1);
-        }
-
-        if ((lnksta & PCI_EXP_LNKSTA_CLS) > (lnkcap & PCI_EXP_LNKCAP_SLS)) {
-            lnksta &= ~PCI_EXP_LNKSTA_CLS;
-            lnksta |= lnkcap & PCI_EXP_LNKCAP_SLS;
-        } else if (!(lnksta & PCI_EXP_LNKSTA_CLS)) {
-            lnksta |= QEMU_PCI_EXP_LNKSTA_CLS(QEMU_PCI_EXP_LNK_2_5GT);
-        }
+    }
+    if ((lnksta & PCI_EXP_LNKSTA_NLW) > (lnkcap & PCI_EXP_LNKCAP_MLW)) {
+        lnksta &= ~PCI_EXP_LNKSTA_NLW;
+        lnksta |= lnkcap & PCI_EXP_LNKCAP_MLW;
+    }
+    if (!(lnksta & PCI_EXP_LNKSTA_NLW)) {
+        lnksta |= QEMU_PCI_EXP_LNKSTA_NLW(QEMU_PCI_EXP_LNK_X1);
+    }
+    if ((lnksta & PCI_EXP_LNKSTA_CLS) > (lnkcap & PCI_EXP_LNKCAP_SLS)) {
+        lnksta &= ~PCI_EXP_LNKSTA_CLS;
+        lnksta |= lnkcap & PCI_EXP_LNKCAP_SLS;
+    }
+    if (!(lnksta & PCI_EXP_LNKSTA_CLS)) {
+        lnksta |= QEMU_PCI_EXP_LNKSTA_CLS(QEMU_PCI_EXP_LNK_2_5GT);
     }
 
     pci_word_test_and_clear_mask(exp_cap + PCI_EXP_LNKSTA,
-- 
2.42.0

Re: [PATCH] pci: ensure valid link status bits for downstream ports

[Sebastian Ott]

On Mon, 11 Nov 2024, Sebastian Ott wrote:
> PCI hotplug for downstream endpoints on arm fails because Linux'
> PCIe hotplug driver doesn't like the QEMU provided LNKSTA:
>
>  pcieport 0000:08:01.0: pciehp: Slot(2): Card present
>  pcieport 0000:08:01.0: pciehp: Slot(2): Link Up
>  pcieport 0000:08:01.0: pciehp: Slot(2): Cannot train link: status 0x2000
>
> There's 2 cases where LNKSTA isn't setup properly:
> * the downstream device has no express capability

I stumbled over this while debugging - is a pci device attached
to a pcie downstream even a valid usecase?

> * max link width of the bridge is 0

MLW for the downstream is initialized with defaults but gets overwritten
later because speed and width properties are 0. Dunno if that's the issue
we should address?

Sebastian

Re: [PATCH] pci: ensure valid link status bits for downstream ports

[Zhenyu Zhang]

[-- Attachment #1: Type: text/plain, Size: 2717 bytes --]


On 2024/11/11 20:37, Sebastian Ott wrote:
> PCI hotplug for downstream endpoints on arm fails because Linux'
> PCIe hotplug driver doesn't like the QEMU provided LNKSTA:
>
>    pcieport 0000:08:01.0: pciehp: Slot(2): Card present
>    pcieport 0000:08:01.0: pciehp: Slot(2): Link Up
>    pcieport 0000:08:01.0: pciehp: Slot(2): Cannot train link: status 0x2000
>
> There's 2 cases where LNKSTA isn't setup properly:
> * the downstream device has no express capability
> * max link width of the bridge is 0
>
> Fix these by making the LNKSTA modifications independent of each other.
>
> Signed-off-by: Sebastian Ott<sebott@redhat.com>
> ---
>   hw/pci/pcie.c | 28 ++++++++++++++--------------
>   1 file changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> index 0b455c8654..f714f4fb7c 100644
> --- a/hw/pci/pcie.c
> +++ b/hw/pci/pcie.c
> @@ -1109,20 +1109,20 @@ void pcie_sync_bridge_lnk(PCIDevice *bridge_dev)
>           lnksta = target->config_read(target,
>                                        target->exp.exp_cap + PCI_EXP_LNKSTA,
>                                        sizeof(lnksta));
> -
> -        if ((lnksta & PCI_EXP_LNKSTA_NLW) > (lnkcap & PCI_EXP_LNKCAP_MLW)) {
> -            lnksta &= ~PCI_EXP_LNKSTA_NLW;
> -            lnksta |= lnkcap & PCI_EXP_LNKCAP_MLW;
> -        } else if (!(lnksta & PCI_EXP_LNKSTA_NLW)) {
> -            lnksta |= QEMU_PCI_EXP_LNKSTA_NLW(QEMU_PCI_EXP_LNK_X1);
> -        }
> -
> -        if ((lnksta & PCI_EXP_LNKSTA_CLS) > (lnkcap & PCI_EXP_LNKCAP_SLS)) {
> -            lnksta &= ~PCI_EXP_LNKSTA_CLS;
> -            lnksta |= lnkcap & PCI_EXP_LNKCAP_SLS;
> -        } else if (!(lnksta & PCI_EXP_LNKSTA_CLS)) {
> -            lnksta |= QEMU_PCI_EXP_LNKSTA_CLS(QEMU_PCI_EXP_LNK_2_5GT);
> -        }
> +    }
> +    if ((lnksta & PCI_EXP_LNKSTA_NLW) > (lnkcap & PCI_EXP_LNKCAP_MLW)) {
> +        lnksta &= ~PCI_EXP_LNKSTA_NLW;
> +        lnksta |= lnkcap & PCI_EXP_LNKCAP_MLW;
> +    }
> +    if (!(lnksta & PCI_EXP_LNKSTA_NLW)) {
> +        lnksta |= QEMU_PCI_EXP_LNKSTA_NLW(QEMU_PCI_EXP_LNK_X1);
> +    }
> +    if ((lnksta & PCI_EXP_LNKSTA_CLS) > (lnkcap & PCI_EXP_LNKCAP_SLS)) {
> +        lnksta &= ~PCI_EXP_LNKSTA_CLS;
> +        lnksta |= lnkcap & PCI_EXP_LNKCAP_SLS;
> +    }
> +    if (!(lnksta & PCI_EXP_LNKSTA_CLS)) {
> +        lnksta |= QEMU_PCI_EXP_LNKSTA_CLS(QEMU_PCI_EXP_LNK_2_5GT);
>       }
>   
>       pci_word_test_and_clear_mask(exp_cap + PCI_EXP_LNKSTA,

|[PATCH] pci: ensure valid link status bits for downstream ports

Test on aarch64 host(v9.2.0-rc0-1-gbde0d70333)
PCI hotplug for downstream endpoints on arm succeed
The test results are as expected.
|

|Tested-by: Zhenyu Zhang <zhenyzha@redhat.com> |

[-- Attachment #2: Type: text/html, Size: 3329 bytes --]

Re: [PATCH] pci: ensure valid link status bits for downstream ports

[Sebastian Ott]

On Mon, 11 Nov 2024, Sebastian Ott wrote:
> PCI hotplug for downstream endpoints on arm fails because Linux'
> PCIe hotplug driver doesn't like the QEMU provided LNKSTA:
>
>  pcieport 0000:08:01.0: pciehp: Slot(2): Card present
>  pcieport 0000:08:01.0: pciehp: Slot(2): Link Up
>  pcieport 0000:08:01.0: pciehp: Slot(2): Cannot train link: status 0x2000
>
> There's 2 cases where LNKSTA isn't setup properly:
> * the downstream device has no express capability
> * max link width of the bridge is 0
>
> Fix these by making the LNKSTA modifications independent of each other.
>
> Signed-off-by: Sebastian Ott <sebott@redhat.com>

Friendly ping. This fixes PCI hotplug to a downstream port on ARM.

Thanks,
Sebastian

Re: [PATCH] pci: ensure valid link status bits for downstream ports

[Alex Williamson]

On Mon, 11 Nov 2024 13:37:56 +0100
Sebastian Ott <sebott@redhat.com> wrote:

> PCI hotplug for downstream endpoints on arm fails because Linux'
> PCIe hotplug driver doesn't like the QEMU provided LNKSTA:
> 
>   pcieport 0000:08:01.0: pciehp: Slot(2): Card present
>   pcieport 0000:08:01.0: pciehp: Slot(2): Link Up
>   pcieport 0000:08:01.0: pciehp: Slot(2): Cannot train link: status 0x2000
> 
> There's 2 cases where LNKSTA isn't setup properly:
> * the downstream device has no express capability
> * max link width of the bridge is 0
> 
> Fix these by making the LNKSTA modifications independent of each other.
> 
> Signed-off-by: Sebastian Ott <sebott@redhat.com>
> ---
>  hw/pci/pcie.c | 28 ++++++++++++++--------------
>  1 file changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> index 0b455c8654..f714f4fb7c 100644
> --- a/hw/pci/pcie.c
> +++ b/hw/pci/pcie.c
> @@ -1109,20 +1109,20 @@ void pcie_sync_bridge_lnk(PCIDevice *bridge_dev)
>          lnksta = target->config_read(target,
>                                       target->exp.exp_cap + PCI_EXP_LNKSTA,
>                                       sizeof(lnksta));
> -
> -        if ((lnksta & PCI_EXP_LNKSTA_NLW) > (lnkcap & PCI_EXP_LNKCAP_MLW)) {
> -            lnksta &= ~PCI_EXP_LNKSTA_NLW;
> -            lnksta |= lnkcap & PCI_EXP_LNKCAP_MLW;
> -        } else if (!(lnksta & PCI_EXP_LNKSTA_NLW)) {
> -            lnksta |= QEMU_PCI_EXP_LNKSTA_NLW(QEMU_PCI_EXP_LNK_X1);
> -        }
> -
> -        if ((lnksta & PCI_EXP_LNKSTA_CLS) > (lnkcap & PCI_EXP_LNKCAP_SLS)) {
> -            lnksta &= ~PCI_EXP_LNKSTA_CLS;
> -            lnksta |= lnkcap & PCI_EXP_LNKCAP_SLS;
> -        } else if (!(lnksta & PCI_EXP_LNKSTA_CLS)) {
> -            lnksta |= QEMU_PCI_EXP_LNKSTA_CLS(QEMU_PCI_EXP_LNK_2_5GT);
> -        }
> +    }
> +    if ((lnksta & PCI_EXP_LNKSTA_NLW) > (lnkcap & PCI_EXP_LNKCAP_MLW)) {
> +        lnksta &= ~PCI_EXP_LNKSTA_NLW;
> +        lnksta |= lnkcap & PCI_EXP_LNKCAP_MLW;
> +    }
> +    if (!(lnksta & PCI_EXP_LNKSTA_NLW)) {
> +        lnksta |= QEMU_PCI_EXP_LNKSTA_NLW(QEMU_PCI_EXP_LNK_X1);
> +    }
> +    if ((lnksta & PCI_EXP_LNKSTA_CLS) > (lnkcap & PCI_EXP_LNKCAP_SLS)) {
> +        lnksta &= ~PCI_EXP_LNKSTA_CLS;
> +        lnksta |= lnkcap & PCI_EXP_LNKCAP_SLS;
> +    }
> +    if (!(lnksta & PCI_EXP_LNKSTA_CLS)) {
> +        lnksta |= QEMU_PCI_EXP_LNKSTA_CLS(QEMU_PCI_EXP_LNK_2_5GT);
>      }
>  
>      pci_word_test_and_clear_mask(exp_cap + PCI_EXP_LNKSTA,

Only half of these seem valid to me.  How can we ever hit the
conditions where the status fields exceed the capability fields in the
case where we've set the status field from the capability field?  It
seems like we'd only want to move the sanity checks added in
88c869198aa63 outside of the branch.  Thanks,

Alex