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