All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3 0/4] failover: trivial cleanup and fix
@ 2021-02-12 13:52 Laurent Vivier
  2021-02-12 13:52 ` [PATCH v3 1/4] pci: cleanup failover sanity check Laurent Vivier
                   ` (3 more replies)
  0 siblings, 4 replies; 9+ messages in thread
From: Laurent Vivier @ 2021-02-12 13:52 UTC (permalink / raw)
  To: qemu-devel
  Cc: alex.williamson, Michael S. Tsirkin, qemu-trivial, Jason Wang,
	quintela, Michael Tokarev, Laurent Vivier, zhengxiang9,
	jfreimann

The first patch removes a duplicate assignment to
allow_unplug_during_migration, and simplify the code.

The second patch fixes a dangling object in failover_add_primary()
that prevents to cleanup the internal structure after the object has
been unplugged.

v2: add PATCH 3

Report a warning if the failover device is not found

v4: add PATCH 4

Fix a problem in pcie slot link status that makes the kernel
to unplug the card while the virtio-net driver hotplugs it.

Laurent Vivier (4):
  pci: cleanup failover sanity check
  virtio-net: add missing object_unref()
  failover: really display a warning when the primary device is not
    found
  pcie: don't set link state active if the slot is empty

 hw/net/virtio-net.c | 18 +++++++++---------
 hw/pci/pci.c        |  6 ++----
 hw/pci/pcie.c       | 19 +++++++++----------
 3 files changed, 20 insertions(+), 23 deletions(-)

-- 
2.29.2




^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v3 1/4] pci: cleanup failover sanity check
  2021-02-12 13:52 [PATCH v3 0/4] failover: trivial cleanup and fix Laurent Vivier
@ 2021-02-12 13:52 ` Laurent Vivier
  2021-02-12 14:18   ` Laurent Vivier
  2021-02-12 13:52 ` [PATCH v3 2/4] virtio-net: add missing object_unref() Laurent Vivier
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 9+ messages in thread
From: Laurent Vivier @ 2021-02-12 13:52 UTC (permalink / raw)
  To: qemu-devel
  Cc: alex.williamson, Michael S. Tsirkin, qemu-trivial, Jason Wang,
	quintela, Michael Tokarev, Laurent Vivier, zhengxiang9,
	jfreimann

Commit a1190ab628 has added a "allow_unplug_during_migration = true" at
the end of the main "if" block, so it is not needed to set it anymore
in the previous checking.

Remove it, to have only sub-ifs that check for needed conditions and exit
if one fails.

Fixes: 4f5b6a05a4e7 ("pci: add option for net failover")
Fixes: a1190ab628c0 ("migration: allow unplug during migration for failover devices")
Cc: jfreimann@redhat.com
Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
 hw/pci/pci.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 512e9042ffae..ecb7aa31fabd 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -2120,10 +2120,8 @@ static void pci_qdev_realize(DeviceState *qdev, Error **errp)
             pci_qdev_unrealize(DEVICE(pci_dev));
             return;
         }
-        if (!(pci_dev->cap_present & QEMU_PCI_CAP_MULTIFUNCTION)
-            && (PCI_FUNC(pci_dev->devfn) == 0)) {
-            qdev->allow_unplug_during_migration = true;
-        } else {
+        if ((pci_dev->cap_present & QEMU_PCI_CAP_MULTIFUNCTION)
+            || (PCI_FUNC(pci_dev->devfn) != 0)) {
             error_setg(errp, "failover: primary device must be in its own "
                               "PCI slot");
             pci_qdev_unrealize(DEVICE(pci_dev));
-- 
2.29.2



^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v3 2/4] virtio-net: add missing object_unref()
  2021-02-12 13:52 [PATCH v3 0/4] failover: trivial cleanup and fix Laurent Vivier
  2021-02-12 13:52 ` [PATCH v3 1/4] pci: cleanup failover sanity check Laurent Vivier
@ 2021-02-12 13:52 ` Laurent Vivier
  2021-02-12 14:18   ` Laurent Vivier
  2021-02-12 13:52 ` [PATCH v3 3/4] failover: really display a warning when the primary device is not found Laurent Vivier
  2021-02-12 13:52 ` [PATCH v3 4/4] pcie: don't set link state active if the slot is empty Laurent Vivier
  3 siblings, 1 reply; 9+ messages in thread
From: Laurent Vivier @ 2021-02-12 13:52 UTC (permalink / raw)
  To: qemu-devel
  Cc: alex.williamson, Michael S. Tsirkin, qemu-trivial, Jason Wang,
	quintela, Michael Tokarev, Laurent Vivier, zhengxiang9,
	jfreimann

failover_add_primary() calls qdev_device_add() and doesn't unref
the device. Because of that, when the device is unplugged a reference
is remaining and prevents the cleanup of the object.

This prevents to be able to plugin back the failover primary device,
with errors like:

  (qemu) device_add vfio-pci,host=0000:41:00.0,id=hostdev0,bus=root.3,failover_pair_id=net0
  (qemu) device_del hostdev0

We can check with "info qtree" and "info pci" that the device has been removed, and then:

  (qemu) device_add vfio-pci,host=0000:41:00.0,id=hostdev1,bus=root.3,failover_pair_id=net0
  Error: vfio 0000:41:00.0: device is already attached
  (qemu) device_add vfio-pci,host=0000:41:00.0,id=hostdev0,bus=root.3,failover_pair_id=net0
  qemu-kvm: Duplicate ID 'hostdev0' for device

Fixes: 21e8709b29cd ("failover: Remove primary_dev member")
Cc: quintela@redhat.com
Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
 hw/net/virtio-net.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 5150f295e8c5..1c5af08dc556 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -862,6 +862,8 @@ static void failover_add_primary(VirtIONet *n, Error **errp)
         dev = qdev_device_add(opts, &err);
         if (err) {
             qemu_opts_del(opts);
+        } else {
+            object_unref(OBJECT(dev));
         }
     } else {
         error_setg(errp, "Primary device not found");
-- 
2.29.2



^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v3 3/4] failover: really display a warning when the primary device is not found
  2021-02-12 13:52 [PATCH v3 0/4] failover: trivial cleanup and fix Laurent Vivier
  2021-02-12 13:52 ` [PATCH v3 1/4] pci: cleanup failover sanity check Laurent Vivier
  2021-02-12 13:52 ` [PATCH v3 2/4] virtio-net: add missing object_unref() Laurent Vivier
@ 2021-02-12 13:52 ` Laurent Vivier
  2021-02-12 14:40   ` Jens Freimann
  2021-02-12 13:52 ` [PATCH v3 4/4] pcie: don't set link state active if the slot is empty Laurent Vivier
  3 siblings, 1 reply; 9+ messages in thread
From: Laurent Vivier @ 2021-02-12 13:52 UTC (permalink / raw)
  To: qemu-devel
  Cc: alex.williamson, Michael S. Tsirkin, qemu-trivial, Jason Wang,
	quintela, Michael Tokarev, Laurent Vivier, zhengxiang9,
	jfreimann

In failover_add_primary(), we search the id of the failover device by
scanning the list of the devices in the opts list to find a device with
a failover_pair_id equals to the id of the virtio-net device.

If the failover_pair_id is not found, QEMU ignores the primary
device silently (which also means it will not be hidden and
it will be enabled directly at boot).

After that, we search the id in the opts list to do a qdev_device_add()
with it. The device will be always found as otherwise we had exited
before, and thus the warning is never displayed.

Fix that by moving the error report to the first exit condition.
Also add a g_assert() to be sure the compiler will not complain
about a possibly NULL pointer.

Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
 hw/net/virtio-net.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 1c5af08dc556..439f823b190c 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -855,21 +855,19 @@ static void failover_add_primary(VirtIONet *n, Error **errp)
 
     id = failover_find_primary_device_id(n);
     if (!id) {
+        error_setg(errp, "Primary device not found");
+        error_append_hint(errp, "Virtio-net failover will not work. Make "
+                          "sure primary device has parameter"
+                          " failover_pair_id=%s\n", n->netclient_name);
         return;
     }
     opts = qemu_opts_find(qemu_find_opts("device"), id);
-    if (opts) {
-        dev = qdev_device_add(opts, &err);
-        if (err) {
-            qemu_opts_del(opts);
-        } else {
-            object_unref(OBJECT(dev));
-        }
+    g_assert(opts); /* cannot be NULL because id was found using opts list */
+    dev = qdev_device_add(opts, &err);
+    if (err) {
+        qemu_opts_del(opts);
     } else {
-        error_setg(errp, "Primary device not found");
-        error_append_hint(errp, "Virtio-net failover will not work. Make "
-                          "sure primary device has parameter"
-                          " failover_pair_id=<virtio-net-id>\n");
+        object_unref(OBJECT(dev));
     }
     error_propagate(errp, err);
 }
-- 
2.29.2



^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v3 4/4] pcie: don't set link state active if the slot is empty
  2021-02-12 13:52 [PATCH v3 0/4] failover: trivial cleanup and fix Laurent Vivier
                   ` (2 preceding siblings ...)
  2021-02-12 13:52 ` [PATCH v3 3/4] failover: really display a warning when the primary device is not found Laurent Vivier
@ 2021-02-12 13:52 ` Laurent Vivier
  2021-02-19 10:10   ` Laurent Vivier
  3 siblings, 1 reply; 9+ messages in thread
From: Laurent Vivier @ 2021-02-12 13:52 UTC (permalink / raw)
  To: qemu-devel
  Cc: alex.williamson, Michael S. Tsirkin, qemu-trivial, Jason Wang,
	quintela, Michael Tokarev, Laurent Vivier, zhengxiang9,
	jfreimann

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);
         }
-- 
2.29.2



^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH v3 1/4] pci: cleanup failover sanity check
  2021-02-12 13:52 ` [PATCH v3 1/4] pci: cleanup failover sanity check Laurent Vivier
@ 2021-02-12 14:18   ` Laurent Vivier
  0 siblings, 0 replies; 9+ messages in thread
From: Laurent Vivier @ 2021-02-12 14:18 UTC (permalink / raw)
  To: qemu-devel
  Cc: quintela, qemu-trivial, Jason Wang, Michael S. Tsirkin,
	Michael Tokarev, Laurent Vivier, alex.williamson, zhengxiang9,
	jfreimann

On 12/02/2021 14:52, Laurent Vivier wrote:
> Commit a1190ab628 has added a "allow_unplug_during_migration = true" at
> the end of the main "if" block, so it is not needed to set it anymore
> in the previous checking.
> 
> Remove it, to have only sub-ifs that check for needed conditions and exit
> if one fails.
> 
> Fixes: 4f5b6a05a4e7 ("pci: add option for net failover")
> Fixes: a1190ab628c0 ("migration: allow unplug during migration for failover devices")
> Cc: jfreimann@redhat.com
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>

I forgot to copy from the first review:
Reviewed-by: Jens Freimann <jfreimann@redhat.com>

> ---
>  hw/pci/pci.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 512e9042ffae..ecb7aa31fabd 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -2120,10 +2120,8 @@ static void pci_qdev_realize(DeviceState *qdev, Error **errp)
>              pci_qdev_unrealize(DEVICE(pci_dev));
>              return;
>          }
> -        if (!(pci_dev->cap_present & QEMU_PCI_CAP_MULTIFUNCTION)
> -            && (PCI_FUNC(pci_dev->devfn) == 0)) {
> -            qdev->allow_unplug_during_migration = true;
> -        } else {
> +        if ((pci_dev->cap_present & QEMU_PCI_CAP_MULTIFUNCTION)
> +            || (PCI_FUNC(pci_dev->devfn) != 0)) {
>              error_setg(errp, "failover: primary device must be in its own "
>                                "PCI slot");
>              pci_qdev_unrealize(DEVICE(pci_dev));
> 



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v3 2/4] virtio-net: add missing object_unref()
  2021-02-12 13:52 ` [PATCH v3 2/4] virtio-net: add missing object_unref() Laurent Vivier
@ 2021-02-12 14:18   ` Laurent Vivier
  0 siblings, 0 replies; 9+ messages in thread
From: Laurent Vivier @ 2021-02-12 14:18 UTC (permalink / raw)
  To: qemu-devel
  Cc: quintela, qemu-trivial, Jason Wang, Michael S. Tsirkin,
	Michael Tokarev, Laurent Vivier, alex.williamson, zhengxiang9,
	jfreimann

On 12/02/2021 14:52, Laurent Vivier wrote:
> failover_add_primary() calls qdev_device_add() and doesn't unref
> the device. Because of that, when the device is unplugged a reference
> is remaining and prevents the cleanup of the object.
> 
> This prevents to be able to plugin back the failover primary device,
> with errors like:
> 
>   (qemu) device_add vfio-pci,host=0000:41:00.0,id=hostdev0,bus=root.3,failover_pair_id=net0
>   (qemu) device_del hostdev0
> 
> We can check with "info qtree" and "info pci" that the device has been removed, and then:
> 
>   (qemu) device_add vfio-pci,host=0000:41:00.0,id=hostdev1,bus=root.3,failover_pair_id=net0
>   Error: vfio 0000:41:00.0: device is already attached
>   (qemu) device_add vfio-pci,host=0000:41:00.0,id=hostdev0,bus=root.3,failover_pair_id=net0
>   qemu-kvm: Duplicate ID 'hostdev0' for device
> 
> Fixes: 21e8709b29cd ("failover: Remove primary_dev member")
> Cc: quintela@redhat.com
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>

I forgot to copy from the first review:
Reviewed-by: Jens Freimann <jfreimann@redhat.com>

> ---
>  hw/net/virtio-net.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 5150f295e8c5..1c5af08dc556 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -862,6 +862,8 @@ static void failover_add_primary(VirtIONet *n, Error **errp)
>          dev = qdev_device_add(opts, &err);
>          if (err) {
>              qemu_opts_del(opts);
> +        } else {
> +            object_unref(OBJECT(dev));
>          }
>      } else {
>          error_setg(errp, "Primary device not found");
> 



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v3 3/4] failover: really display a warning when the primary device is not found
  2021-02-12 13:52 ` [PATCH v3 3/4] failover: really display a warning when the primary device is not found Laurent Vivier
@ 2021-02-12 14:40   ` Jens Freimann
  0 siblings, 0 replies; 9+ messages in thread
From: Jens Freimann @ 2021-02-12 14:40 UTC (permalink / raw)
  To: Laurent Vivier
  Cc: alex.williamson, Michael S. Tsirkin, qemu-trivial, Jason Wang,
	quintela, Michael Tokarev, Laurent Vivier, qemu-devel,
	zhengxiang9

On Fri, Feb 12, 2021 at 02:52:49PM +0100, Laurent Vivier wrote:
>In failover_add_primary(), we search the id of the failover device by
>scanning the list of the devices in the opts list to find a device with
>a failover_pair_id equals to the id of the virtio-net device.
>
>If the failover_pair_id is not found, QEMU ignores the primary
>device silently (which also means it will not be hidden and
>it will be enabled directly at boot).
>
>After that, we search the id in the opts list to do a qdev_device_add()
>with it. The device will be always found as otherwise we had exited
>before, and thus the warning is never displayed.
>
>Fix that by moving the error report to the first exit condition.
>Also add a g_assert() to be sure the compiler will not complain
>about a possibly NULL pointer.
>
>Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>---
> hw/net/virtio-net.c | 20 +++++++++-----------
> 1 file changed, 9 insertions(+), 11 deletions(-)
>

Thank you Laurent!

Reviewed-by: Jens Freimann <jfreimann@redhat.com>

regards,
Jens



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v3 4/4] pcie: don't set link state active if the slot is empty
  2021-02-12 13:52 ` [PATCH v3 4/4] pcie: don't set link state active if the slot is empty Laurent Vivier
@ 2021-02-19 10:10   ` Laurent Vivier
  0 siblings, 0 replies; 9+ messages in thread
From: Laurent Vivier @ 2021-02-19 10:10 UTC (permalink / raw)
  To: qemu-devel
  Cc: quintela, qemu-trivial, Jason Wang, Michael S. Tsirkin,
	Michael Tokarev, Laurent Vivier, alex.williamson, zhengxiang9,
	jfreimann

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);
>          }
> 



^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2021-02-19 10:12 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-12 13:52 [PATCH v3 0/4] failover: trivial cleanup and fix Laurent Vivier
2021-02-12 13:52 ` [PATCH v3 1/4] pci: cleanup failover sanity check Laurent Vivier
2021-02-12 14:18   ` Laurent Vivier
2021-02-12 13:52 ` [PATCH v3 2/4] virtio-net: add missing object_unref() Laurent Vivier
2021-02-12 14:18   ` Laurent Vivier
2021-02-12 13:52 ` [PATCH v3 3/4] failover: really display a warning when the primary device is not found Laurent Vivier
2021-02-12 14:40   ` Jens Freimann
2021-02-12 13:52 ` [PATCH v3 4/4] pcie: don't set link state active if the slot is empty Laurent Vivier
2021-02-19 10:10   ` Laurent Vivier

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.