[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3 4/4] pcie: don't set link state active if the slot is empt
From: |
Laurent Vivier |
Subject: |
Re: [PATCH v3 4/4] pcie: don't set link state active if the slot is empty |
Date: |
Fri, 19 Feb 2021 11:10:55 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.7.0 |
Hi,
Any comment?
This fix is really needed to fix a bug that prevents to use virtio-net failover
on recent
kernel.
Thanks,
Laurent
On 12/02/2021 14:52, Laurent Vivier wrote:
> When the pcie slot is initialized, by default PCI_EXP_LNKSTA_DLLLA
> (Data Link Layer Link Active) is set in PCI_EXP_LNKSTA
> (Link Status) without checking if the slot is empty or not.
>
> This is confusing for the kernel because as it sees the link is up
> it tries to read the vendor ID and fails:
>
> (From https://bugzilla.kernel.org/show_bug.cgi?id=211691)
>
> [ 1.661105] pcieport 0000:00:02.2: pciehp: Slot Capabilities :
> 0x0002007b
> [ 1.661115] pcieport 0000:00:02.2: pciehp: Slot Status : 0x0010
> [ 1.661123] pcieport 0000:00:02.2: pciehp: Slot Control : 0x07c0
> [ 1.661138] pcieport 0000:00:02.2: pciehp: Slot #0 AttnBtn+ PwrCtrl+ MRL-
> AttnInd+ PwrInd+ HotPlug+ Surprise+ Interlock+ NoCompl- IbPresDis- LLActRep+
> [ 1.662581] pcieport 0000:00:02.2: pciehp: pciehp_get_power_status:
> SLOTCTRL 6c value read 7c0
> [ 1.662597] pcieport 0000:00:02.2: pciehp: pciehp_check_link_active:
> lnk_status = 2204
> [ 1.662703] pcieport 0000:00:02.2: pciehp: pending interrupts 0x0010 from
> Slot Status
> [ 1.662706] pcieport 0000:00:02.2: pciehp: pcie_enable_notification:
> SLOTCTRL 6c write cmd 1031
> [ 1.662730] pcieport 0000:00:02.2: pciehp: pciehp_check_link_active:
> lnk_status = 2204
> [ 1.662748] pcieport 0000:00:02.2: pciehp: pciehp_check_link_active:
> lnk_status = 2204
> [ 1.662750] pcieport 0000:00:02.2: pciehp: Slot(0-2): Link Up
> [ 2.896132] pcieport 0000:00:02.2: pciehp: pciehp_check_link_status:
> lnk_status = 2204
> [ 2.896135] pcieport 0000:00:02.2: pciehp: Slot(0-2): No device found
> [ 2.896900] pcieport 0000:00:02.2: pciehp: pending interrupts 0x0010 from
> Slot Status
> [ 2.896903] pcieport 0000:00:02.2: pciehp: pciehp_power_off_slot: SLOTCTRL
> 6c write cmd 400
> [ 3.656901] pcieport 0000:00:02.2: pciehp: pending interrupts 0x0009 from
> Slot Status
>
> This is really a problem with virtio-net failover that hotplugs a VFIO
> card during the boot process. The kernel can shutdown the slot while
> QEMU is hotplugging it, and this likely ends by an automatic unplug of
> the card. At the end of the boot sequence the card has disappeared.
>
> To fix that, don't set the "Link Active" state in the init function, but
> rely on the plug function to do it, as the mechanism has already been
> introduced by 2f2b18f60bf1.
>
> Fixes: 2f2b18f60bf1 ("pcie: set link state inactive/active after hot
> unplug/plug")
> Cc: zhengxiang9@huawei.com
> Fixes: 3d67447fe7c2 ("pcie: Fill PCIESlot link fields to support higher
> speeds and widths")
> Cc: alex.williamson@redhat.com
> Fixes: b2101eae63ea ("pcie: Set the "link active" in the link status
> register")
> Cc: benh@kernel.crashing.org
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
> hw/pci/pcie.c | 19 +++++++++----------
> 1 file changed, 9 insertions(+), 10 deletions(-)
>
> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> index d4010cf8f361..a733e2fb879a 100644
> --- a/hw/pci/pcie.c
> +++ b/hw/pci/pcie.c
> @@ -75,11 +75,6 @@ pcie_cap_v1_fill(PCIDevice *dev, uint8_t port, uint8_t
> type, uint8_t version)
> QEMU_PCI_EXP_LNKSTA_NLW(QEMU_PCI_EXP_LNK_X1) |
> QEMU_PCI_EXP_LNKSTA_CLS(QEMU_PCI_EXP_LNK_2_5GT));
>
> - if (dev->cap_present & QEMU_PCIE_LNKSTA_DLLLA) {
> - pci_word_test_and_set_mask(exp_cap + PCI_EXP_LNKSTA,
> - PCI_EXP_LNKSTA_DLLLA);
> - }
> -
> /* We changed link status bits over time, and changing them across
> * migrations is generally fine as hardware changes them too.
> * Let's not bother checking.
> @@ -125,8 +120,7 @@ static void pcie_cap_fill_slot_lnk(PCIDevice *dev)
> */
> pci_long_test_and_set_mask(exp_cap + PCI_EXP_LNKCAP,
> PCI_EXP_LNKCAP_DLLLARC);
> - pci_word_test_and_set_mask(exp_cap + PCI_EXP_LNKSTA,
> - PCI_EXP_LNKSTA_DLLLA);
> + /* the PCI_EXP_LNKSTA_DLLLA will be set in the hotplug function */
>
> /*
> * Target Link Speed defaults to the highest link speed supported by
> @@ -427,6 +421,7 @@ void pcie_cap_slot_plug_cb(HotplugHandler *hotplug_dev,
> DeviceState *dev,
> PCIDevice *hotplug_pdev = PCI_DEVICE(hotplug_dev);
> uint8_t *exp_cap = hotplug_pdev->config + hotplug_pdev->exp.exp_cap;
> PCIDevice *pci_dev = PCI_DEVICE(dev);
> + uint32_t lnkcap = pci_get_long(exp_cap + PCI_EXP_LNKCAP);
>
> /* Don't send event when device is enabled during qemu machine creation:
> * it is present on boot, no hotplug event is necessary. We do send an
> @@ -434,7 +429,8 @@ void pcie_cap_slot_plug_cb(HotplugHandler *hotplug_dev,
> DeviceState *dev,
> if (!dev->hotplugged) {
> pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTSTA,
> PCI_EXP_SLTSTA_PDS);
> - if (pci_dev->cap_present & QEMU_PCIE_LNKSTA_DLLLA) {
> + if (pci_dev->cap_present & QEMU_PCIE_LNKSTA_DLLLA ||
> + (lnkcap & PCI_EXP_LNKCAP_DLLLARC)) {
> pci_word_test_and_set_mask(exp_cap + PCI_EXP_LNKSTA,
> PCI_EXP_LNKSTA_DLLLA);
> }
> @@ -448,7 +444,8 @@ void pcie_cap_slot_plug_cb(HotplugHandler *hotplug_dev,
> DeviceState *dev,
> if (pci_get_function_0(pci_dev)) {
> pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTSTA,
> PCI_EXP_SLTSTA_PDS);
> - if (pci_dev->cap_present & QEMU_PCIE_LNKSTA_DLLLA) {
> + if (pci_dev->cap_present & QEMU_PCIE_LNKSTA_DLLLA ||
> + (lnkcap & PCI_EXP_LNKCAP_DLLLARC)) {
> pci_word_test_and_set_mask(exp_cap + PCI_EXP_LNKSTA,
> PCI_EXP_LNKSTA_DLLLA);
> }
> @@ -640,6 +637,7 @@ void pcie_cap_slot_write_config(PCIDevice *dev,
> uint32_t pos = dev->exp.exp_cap;
> uint8_t *exp_cap = dev->config + pos;
> uint16_t sltsta = pci_get_word(exp_cap + PCI_EXP_SLTSTA);
> + uint32_t lnkcap = pci_get_long(exp_cap + PCI_EXP_LNKCAP);
>
> if (ranges_overlap(addr, len, pos + PCI_EXP_SLTSTA, 2)) {
> /*
> @@ -695,7 +693,8 @@ void pcie_cap_slot_write_config(PCIDevice *dev,
>
> pci_word_test_and_clear_mask(exp_cap + PCI_EXP_SLTSTA,
> PCI_EXP_SLTSTA_PDS);
> - if (dev->cap_present & QEMU_PCIE_LNKSTA_DLLLA) {
> + if (dev->cap_present & QEMU_PCIE_LNKSTA_DLLLA ||
> + (lnkcap & PCI_EXP_LNKCAP_DLLLARC)) {
> pci_word_test_and_clear_mask(exp_cap + PCI_EXP_LNKSTA,
> PCI_EXP_LNKSTA_DLLLA);
> }
>