[PATCH 0/2] failover: trivial cleanup and fix

[PATCH 0/2] failover: trivial cleanup and fix

[Laurent Vivier]

The first patch removes a duplicate assignment to allow_unplug_during_migrati=
on,
and simplify the code.

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

Laurent Vivier (2):
  pci: cleanup failover sanity check
  virtio-net: add missing object_unref()

 hw/net/virtio-net.c | 2 ++
 hw/pci/pci.c        | 6 ++----
 2 files changed, 4 insertions(+), 4 deletions(-)

--=20
2.29.2

[PATCH 1/2] pci: cleanup failover sanity check

[Laurent Vivier]

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@virtlab415.virt.lab.eng.bos.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

[PATCH 2/2] virtio-net: add missing object_unref()

[Laurent Vivier]

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

Re: [PATCH 1/2] pci: cleanup failover sanity check

[Laurent Vivier]

On 06/02/2021 13:39, 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@virtlab415.virt.lab.eng.bos.redhat.com>

Sorry, git misconfiguration, read:

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

Re: [PATCH 1/2] pci: cleanup failover sanity check

[Jens Freimann]

On Sat, Feb 06, 2021 at 01:39:54PM +0100, 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@virtlab415.virt.lab.eng.bos.redhat.com>
>---
> hw/pci/pci.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>

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

Thank you Laurent!

regards,
Jens 

Re: [PATCH 2/2] virtio-net: add missing object_unref()

[Jens Freimann]

On Sat, Feb 06, 2021 at 01:39:55PM +0100, 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>
>---
> hw/net/virtio-net.c | 2 ++
> 1 file changed, 2 insertions(+)
>

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

Thank you Laurent!

regards,
Jens 

Re: [PATCH 1/2] pci: cleanup failover sanity check

[Laurent Vivier]

CC: Michael

On 06/02/2021 13:39, 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@virtlab415.virt.lab.eng.bos.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));
> 

Re: [PATCH 2/2] virtio-net: add missing object_unref()

[Laurent Vivier]

CC: Michael

On 06/02/2021 13:39, 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>
> ---
>  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");
> 

Re: [PATCH 0/2] failover: trivial cleanup and fix

[Laurent Vivier]

On 06/02/2021 13:39, Laurent Vivier wrote:
> The first patch removes a duplicate assignment to allow_unplug_during_migrati=
> on,
> and simplify the code.
> 
> The second patch fixes a dangling object in failover_add_primary() that preve=
> nts
> to cleanup the internal structure after the object has been unplugged.
> 
> Laurent Vivier (2):
>   pci: cleanup failover sanity check
>   virtio-net: add missing object_unref()

I can collect these two patches via the trivial branch if there will be no PR for virtio
or PCI soon.

Michael?

Thanks,
Laurent