[PATCH v3] failover: fix unplug pending detection

[PATCH v3] failover: fix unplug pending detection

[Laurent Vivier]

Failover needs to detect the end of the PCI unplug to start migration
after the VFIO card has been unplugged.

To do that, a flag is set in pcie_cap_slot_unplug_request_cb() and reset in
pcie_unplug_device().

But since
    17858a169508 ("hw/acpi/ich9: Set ACPI PCI hot-plug as default on Q35")
we have switched to ACPI unplug and these functions are not called anymore
and the flag not set. So failover migration is not able to detect if card
is really unplugged and acts as it's done as soon as it's started. So it
doesn't wait the end of the unplug to start the migration. We don't see any
problem when we test that because ACPI unplug is faster than PCIe native
hotplug and when the migration really starts the unplug operation is
already done.

See c000a9bd06ea ("pci: mark device having guest unplug request pending")
    a99c4da9fc2a ("pci: mark devices partially unplugged")

Signed-off-by: Laurent Vivier <lvivier@redhat.com>
Reviewed-by: Ani Sinha <ani@anisinha.ca>
---

Notes:
    v3: add some inlined comments to explain partially_hotplugged and
        pending_deleted_event
    v2: move partially_hotplugged to acpi_pcihp_eject_slot()

 hw/acpi/pcihp.c | 30 +++++++++++++++++++++++++++---
 1 file changed, 27 insertions(+), 3 deletions(-)

diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
index f610a25d2ef9..30405b5113d7 100644
--- a/hw/acpi/pcihp.c
+++ b/hw/acpi/pcihp.c
@@ -222,9 +222,27 @@ static void acpi_pcihp_eject_slot(AcpiPciHpState *s, unsigned bsel, unsigned slo
         PCIDevice *dev = PCI_DEVICE(qdev);
         if (PCI_SLOT(dev->devfn) == slot) {
             if (!acpi_pcihp_pc_no_hotplug(s, dev)) {
-                hotplug_ctrl = qdev_get_hotplug_handler(qdev);
-                hotplug_handler_unplug(hotplug_ctrl, qdev, &error_abort);
-                object_unparent(OBJECT(qdev));
+                /*
+                 * partially_hotplugged is used by virtio-net failover:
+                 * failover has asked the guest OS to unplug the device
+                 * but we need to keep some references to the device
+                 * to be able to plug it back in case of failure so
+                 * we don't execute hotplug_handler_unplug().
+                 */
+                if (dev->partially_hotplugged) {
+                    /*
+                     * pending_deleted_event is set to true when
+                     * virtio-net failover asks to unplug the device,
+                     * and set to false here when the operation is done
+                     * This is used by the migration loop to detect the
+                     * end of the operation and really start the migration.
+                     */
+                    qdev->pending_deleted_event = false;
+                } else {
+                    hotplug_ctrl = qdev_get_hotplug_handler(qdev);
+                    hotplug_handler_unplug(hotplug_ctrl, qdev, &error_abort);
+                    object_unparent(OBJECT(qdev));
+                }
             }
         }
     }
@@ -396,6 +414,12 @@ void acpi_pcihp_device_unplug_request_cb(HotplugHandler *hotplug_dev,
         return;
     }
 
+    /*
+     * pending_deleted_event is used by virtio-net failover to detect the
+     * end of the unplug operation, the flag is set to false in
+     * acpi_pcihp_eject_slot() when the operation is completed.
+     */
+    pdev->qdev.pending_deleted_event = true;
     s->acpi_pcihp_pci_status[bsel].down |= (1U << slot);
     acpi_send_event(DEVICE(hotplug_dev), ACPI_PCI_HOTPLUG_STATUS);
 }
-- 
2.31.1

Re: [PATCH v3] failover: fix unplug pending detection

[Gerd Hoffmann]

On Fri, Oct 01, 2021 at 10:25:02AM +0200, Laurent Vivier wrote:
> Failover needs to detect the end of the PCI unplug to start migration
> after the VFIO card has been unplugged.
> 
> To do that, a flag is set in pcie_cap_slot_unplug_request_cb() and reset in
> pcie_unplug_device().

> +    /*
> +     * pending_deleted_event is used by virtio-net failover to detect the
> +     * end of the unplug operation, the flag is set to false in
> +     * acpi_pcihp_eject_slot() when the operation is completed.
> +     */
> +    pdev->qdev.pending_deleted_event = true;

This has the side effect of blocking a second 'device_del' command.

So, in case the first time didn't work (for example due to the guest not
listening because grub just doesn't do that), you can try a second time
once the linux kernel is up'n'running.

I suspect this patch will break that (didn't actually test though).


While being at it I'll throw in a link to a bunch of slightly related
pcie hotplug patches:
   https://gitlab.com/kraxel/qemu/-/commits/sirius/pcie-hotplug

I'll be offline next week, so not posting that series for discussion
yet, will probably do that when I'm back, but you might want have a
look nevertheless.

take care,
  Gerd

Re: [PATCH v3] failover: fix unplug pending detection

[Daniel P. Berrangé]

On Fri, Oct 01, 2021 at 12:45:14PM +0200, Gerd Hoffmann wrote:
> On Fri, Oct 01, 2021 at 10:25:02AM +0200, Laurent Vivier wrote:
> > Failover needs to detect the end of the PCI unplug to start migration
> > after the VFIO card has been unplugged.
> > 
> > To do that, a flag is set in pcie_cap_slot_unplug_request_cb() and reset in
> > pcie_unplug_device().
> 
> > +    /*
> > +     * pending_deleted_event is used by virtio-net failover to detect the
> > +     * end of the unplug operation, the flag is set to false in
> > +     * acpi_pcihp_eject_slot() when the operation is completed.
> > +     */
> > +    pdev->qdev.pending_deleted_event = true;
> 
> This has the side effect of blocking a second 'device_del' command.
> 
> So, in case the first time didn't work (for example due to the guest not
> listening because grub just doesn't do that), you can try a second time
> once the linux kernel is up'n'running.
> 
> I suspect this patch will break that (didn't actually test though).

That's important because OpenStack will definitely trigger
multiple device_del commands in a row if they don't get a
success from an initial one.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v3] failover: fix unplug pending detection

[Laurent Vivier]

On 01/10/2021 12:45, Gerd Hoffmann wrote:
> On Fri, Oct 01, 2021 at 10:25:02AM +0200, Laurent Vivier wrote:
>> Failover needs to detect the end of the PCI unplug to start migration
>> after the VFIO card has been unplugged.
>>
>> To do that, a flag is set in pcie_cap_slot_unplug_request_cb() and reset in
>> pcie_unplug_device().
> 
>> +    /*
>> +     * pending_deleted_event is used by virtio-net failover to detect the
>> +     * end of the unplug operation, the flag is set to false in
>> +     * acpi_pcihp_eject_slot() when the operation is completed.
>> +     */
>> +    pdev->qdev.pending_deleted_event = true;
> 
> This has the side effect of blocking a second 'device_del' command.

This problem is not introduced by this patch as it only adds the code we already have in 
PCIe Native hotplug to ACPI hotplug (but see below).

> So, in case the first time didn't work (for example due to the guest not
> listening because grub just doesn't do that), you can try a second time
> once the linux kernel is up'n'running.
> 
> I suspect this patch will break that (didn't actually test though).

I think the solution to this problem is to not check for pending_deleted_event value in 
qmp_device_del().

But this has been explicitly added by:

commit cce8944cc9efab47d4bf29cfffb3470371c3541b
Author: Julia Suvorova <jusual@redhat.com>
Date:   Thu Feb 20 17:55:56 2020 +0100

     qdev-monitor: Forbid repeated device_del

     Device unplug can be done asynchronously. Thus, sending the second
     device_del before the previous unplug is complete may lead to
     unexpected results. On PCIe devices, this cancels the hot-unplug
     process.

     Signed-off-by: Julia Suvorova <jusual@redhat.com>
     Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
     Message-Id: <20200220165556.39388-1-jusual@redhat.com>
     Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

So do you mean ACPI differs from PCIe Native hotplug in this case?

Thanks,
Laurent

Re: [PATCH v3] failover: fix unplug pending detection

[Gerd Hoffmann]

  Hi,

> > So, in case the first time didn't work (for example due to the guest not
> > listening because grub just doesn't do that), you can try a second time
> > once the linux kernel is up'n'running.
> > 
> > I suspect this patch will break that (didn't actually test though).
> 
> I think the solution to this problem is to not check for
> pending_deleted_event value in qmp_device_del().
> 
> But this has been explicitly added by:
> 
> commit cce8944cc9efab47d4bf29cfffb3470371c3541b
> Author: Julia Suvorova <jusual@redhat.com>
> Date:   Thu Feb 20 17:55:56 2020 +0100
> 
>     qdev-monitor: Forbid repeated device_del
> 
>     [ ... ]
> 
> So do you mean ACPI differs from PCIe Native hotplug in this case?

Yes.

It's one of the issues I'm trying to address with the

  https://gitlab.com/kraxel/qemu/-/commits/sirius/pcie-hotplug

series.  See this commit:

  https://gitlab.com/kraxel/qemu/-/commit/675d9257d794c9d59ea7c80f48fe176a2aa3f8ba

So, yes, I think acpi and pcie hotplug should show consistent behavior
here.  And I think we need some way to recover in case the guest didn't
respond to an unplug event.  Just allowing to send device_del multiple
times looks like a sensible approach to me, and given OpenStack already
does that it looks like the most sensible way forward.

take care,
  Gerd

Re: [PATCH v3] failover: fix unplug pending detection

[Ani Sinha]

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

On Fri, Oct 1, 2021 at 18:19 Gerd Hoffmann <kraxel@redhat.com> wrote:

>   Hi,
>
> > > So, in case the first time didn't work (for example due to the guest
> not
> > > listening because grub just doesn't do that), you can try a second time
> > > once the linux kernel is up'n'running.
> > >
> > > I suspect this patch will break that (didn't actually test though).
> >
> > I think the solution to this problem is to not check for
> > pending_deleted_event value in qmp_device_del().
> >
> > But this has been explicitly added by:
> >
> > commit cce8944cc9efab47d4bf29cfffb3470371c3541b
> > Author: Julia Suvorova <jusual@redhat.com>
> > Date:   Thu Feb 20 17:55:56 2020 +0100
> >
> >     qdev-monitor: Forbid repeated device_del
> >
> >     [ ... ]
> >
> > So do you mean ACPI differs from PCIe Native hotplug in this case?
>
> Yes.
>
> It's one of the issues I'm trying to address with the
>
>   https://gitlab.com/kraxel/qemu/-/commits/sirius/pcie-hotplug
>
> series.  See this commit:
>
>
> https://gitlab.com/kraxel/qemu/-/commit/675d9257d794c9d59ea7c80f48fe176a2aa3f8ba


I think the scope of this patch is limited to making the acpi hotplug path
identical to PCIE native path wrt failover. If there are issues with the
existing approach, it should be looked into separately using subsequent
patches.

<https://gitlab.com/kraxel/qemu/-/commit/675d9257d794c9d59ea7c80f48fe176a2aa3f8ba>
>
> So, yes, I think acpi and pcie hotplug should show consistent behavior
> here.  And I think we need some way to recover in case the guest didn't
> respond to an unplug event.  Just allowing to send device_del multiple
> times looks like a sensible approach to me, and given OpenStack already
> does that it looks like the most sensible way forward.
>
> take care,
>   Gerd
>
>

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

Re: [PATCH v3] failover: fix unplug pending detection

[Ani Sinha]

On Fri, 1 Oct 2021, Gerd Hoffmann wrote:

>   Hi,
>
> > > So, in case the first time didn't work (for example due to the guest not
> > > listening because grub just doesn't do that), you can try a second time
> > > once the linux kernel is up'n'running.
> > >
> > > I suspect this patch will break that (didn't actually test though).
> >
> > I think the solution to this problem is to not check for
> > pending_deleted_event value in qmp_device_del().
> >
> > But this has been explicitly added by:
> >
> > commit cce8944cc9efab47d4bf29cfffb3470371c3541b
> > Author: Julia Suvorova <jusual@redhat.com>
> > Date:   Thu Feb 20 17:55:56 2020 +0100
> >
> >     qdev-monitor: Forbid repeated device_del
> >
> >     [ ... ]
> >
> > So do you mean ACPI differs from PCIe Native hotplug in this case?
>
> Yes.
>
> It's one of the issues I'm trying to address with the
>
>   https://gitlab.com/kraxel/qemu/-/commits/sirius/pcie-hotplug
>
> series.  See this commit:
>
>   https://gitlab.com/kraxel/qemu/-/commit/675d9257d794c9d59ea7c80f48fe176a2aa3f8ba
>

I think the scope of this patch is limited to making the acpi hotplug path
identical to PCIE native path wrt failover. If there are issues with the
existing approach, it should be looked into separately using subsequent
patches.


> So, yes, I think acpi and pcie hotplug should show consistent behavior
> here.  And I think we need some way to recover in case the guest didn't
> respond to an unplug event.  Just allowing to send device_del multiple
> times looks like a sensible approach to me, and given OpenStack already
> does that it looks like the most sensible way forward.
>
> take care,
>   Gerd
>
>

Re: [PATCH v3] failover: fix unplug pending detection

[Michael S. Tsirkin]

On Fri, Oct 01, 2021 at 10:25:02AM +0200, Laurent Vivier wrote:
> Failover needs to detect the end of the PCI unplug to start migration
> after the VFIO card has been unplugged.
> 
> To do that, a flag is set in pcie_cap_slot_unplug_request_cb() and reset in
> pcie_unplug_device().
> 
> But since
>     17858a169508 ("hw/acpi/ich9: Set ACPI PCI hot-plug as default on Q35")
> we have switched to ACPI unplug and these functions are not called anymore
> and the flag not set. So failover migration is not able to detect if card
> is really unplugged and acts as it's done as soon as it's started. So it
> doesn't wait the end of the unplug to start the migration. We don't see any
> problem when we test that because ACPI unplug is faster than PCIe native
> hotplug and when the migration really starts the unplug operation is
> already done.
> 
> See c000a9bd06ea ("pci: mark device having guest unplug request pending")
>     a99c4da9fc2a ("pci: mark devices partially unplugged")
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> Reviewed-by: Ani Sinha <ani@anisinha.ca>

Laurent, are you thinking of addressing Gerd's comment?

> ---
> 
> Notes:
>     v3: add some inlined comments to explain partially_hotplugged and
>         pending_deleted_event
>     v2: move partially_hotplugged to acpi_pcihp_eject_slot()
> 
>  hw/acpi/pcihp.c | 30 +++++++++++++++++++++++++++---
>  1 file changed, 27 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> index f610a25d2ef9..30405b5113d7 100644
> --- a/hw/acpi/pcihp.c
> +++ b/hw/acpi/pcihp.c
> @@ -222,9 +222,27 @@ static void acpi_pcihp_eject_slot(AcpiPciHpState *s, unsigned bsel, unsigned slo
>          PCIDevice *dev = PCI_DEVICE(qdev);
>          if (PCI_SLOT(dev->devfn) == slot) {
>              if (!acpi_pcihp_pc_no_hotplug(s, dev)) {
> -                hotplug_ctrl = qdev_get_hotplug_handler(qdev);
> -                hotplug_handler_unplug(hotplug_ctrl, qdev, &error_abort);
> -                object_unparent(OBJECT(qdev));
> +                /*
> +                 * partially_hotplugged is used by virtio-net failover:
> +                 * failover has asked the guest OS to unplug the device
> +                 * but we need to keep some references to the device
> +                 * to be able to plug it back in case of failure so
> +                 * we don't execute hotplug_handler_unplug().
> +                 */
> +                if (dev->partially_hotplugged) {
> +                    /*
> +                     * pending_deleted_event is set to true when
> +                     * virtio-net failover asks to unplug the device,
> +                     * and set to false here when the operation is done
> +                     * This is used by the migration loop to detect the
> +                     * end of the operation and really start the migration.
> +                     */
> +                    qdev->pending_deleted_event = false;
> +                } else {
> +                    hotplug_ctrl = qdev_get_hotplug_handler(qdev);
> +                    hotplug_handler_unplug(hotplug_ctrl, qdev, &error_abort);
> +                    object_unparent(OBJECT(qdev));
> +                }
>              }
>          }
>      }
> @@ -396,6 +414,12 @@ void acpi_pcihp_device_unplug_request_cb(HotplugHandler *hotplug_dev,
>          return;
>      }
>  
> +    /*
> +     * pending_deleted_event is used by virtio-net failover to detect the
> +     * end of the unplug operation, the flag is set to false in
> +     * acpi_pcihp_eject_slot() when the operation is completed.
> +     */
> +    pdev->qdev.pending_deleted_event = true;
>      s->acpi_pcihp_pci_status[bsel].down |= (1U << slot);
>      acpi_send_event(DEVICE(hotplug_dev), ACPI_PCI_HOTPLUG_STATUS);
>  }
> -- 
> 2.31.1

Re: [PATCH v3] failover: fix unplug pending detection

[Laurent Vivier]

On 05/10/2021 17:14, Michael S. Tsirkin wrote:
> On Fri, Oct 01, 2021 at 10:25:02AM +0200, Laurent Vivier wrote:
>> Failover needs to detect the end of the PCI unplug to start migration
>> after the VFIO card has been unplugged.
>>
>> To do that, a flag is set in pcie_cap_slot_unplug_request_cb() and reset in
>> pcie_unplug_device().
>>
>> But since
>>      17858a169508 ("hw/acpi/ich9: Set ACPI PCI hot-plug as default on Q35")
>> we have switched to ACPI unplug and these functions are not called anymore
>> and the flag not set. So failover migration is not able to detect if card
>> is really unplugged and acts as it's done as soon as it's started. So it
>> doesn't wait the end of the unplug to start the migration. We don't see any
>> problem when we test that because ACPI unplug is faster than PCIe native
>> hotplug and when the migration really starts the unplug operation is
>> already done.
>>
>> See c000a9bd06ea ("pci: mark device having guest unplug request pending")
>>      a99c4da9fc2a ("pci: mark devices partially unplugged")
>>
>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>> Reviewed-by: Ani Sinha <ani@anisinha.ca>
> 
> Laurent, are you thinking of addressing Gerd's comment?

No, because as said by Ani, it's not the scope of this patch. The patch only aligns ACPI 
to PCIe Native to be able to manage failover.

The problem reported by Gerd and Daniel has been introduced by another patch, globally.

Except if you think it's really required I will not send a new version.

Thanks,
Laurent

Re: [PATCH v3] failover: fix unplug pending detection

[Ani Sinha]

On Tue, 5 Oct 2021, Laurent Vivier wrote:

> On 05/10/2021 17:14, Michael S. Tsirkin wrote:
> > On Fri, Oct 01, 2021 at 10:25:02AM +0200, Laurent Vivier wrote:
> > > Failover needs to detect the end of the PCI unplug to start migration
> > > after the VFIO card has been unplugged.
> > >
> > > To do that, a flag is set in pcie_cap_slot_unplug_request_cb() and reset
> > > in
> > > pcie_unplug_device().
> > >
> > > But since
> > >      17858a169508 ("hw/acpi/ich9: Set ACPI PCI hot-plug as default on
> > > Q35")
> > > we have switched to ACPI unplug and these functions are not called anymore
> > > and the flag not set. So failover migration is not able to detect if card
> > > is really unplugged and acts as it's done as soon as it's started. So it
> > > doesn't wait the end of the unplug to start the migration. We don't see
> > > any
> > > problem when we test that because ACPI unplug is faster than PCIe native
> > > hotplug and when the migration really starts the unplug operation is
> > > already done.
> > >
> > > See c000a9bd06ea ("pci: mark device having guest unplug request pending")
> > >      a99c4da9fc2a ("pci: mark devices partially unplugged")
> > >
> > > Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> > > Reviewed-by: Ani Sinha <ani@anisinha.ca>
> >
> > Laurent, are you thinking of addressing Gerd's comment?
>
> No, because as said by Ani, it's not the scope of this patch. The patch only
> aligns ACPI to PCIe Native to be able to manage failover.
>
> The problem reported by Gerd and Daniel has been introduced by another patch,
> globally.
>

but I thought Julia's fix commit cce8944cc9efab47d4bf29cfffb3470371c3541b
addressed this for native pcie and since it is at a high enough level, it
should catch the acpi hotplug path equally as well.

Re: [PATCH v3] failover: fix unplug pending detection

[Laurent Vivier]

Hi,

I don't understand if there are some issues to address to have this patch merged?

Thanks,
Laurent

On 01/10/2021 10:25, Laurent Vivier wrote:
> Failover needs to detect the end of the PCI unplug to start migration
> after the VFIO card has been unplugged.
> 
> To do that, a flag is set in pcie_cap_slot_unplug_request_cb() and reset in
> pcie_unplug_device().
> 
> But since
>      17858a169508 ("hw/acpi/ich9: Set ACPI PCI hot-plug as default on Q35")
> we have switched to ACPI unplug and these functions are not called anymore
> and the flag not set. So failover migration is not able to detect if card
> is really unplugged and acts as it's done as soon as it's started. So it
> doesn't wait the end of the unplug to start the migration. We don't see any
> problem when we test that because ACPI unplug is faster than PCIe native
> hotplug and when the migration really starts the unplug operation is
> already done.
> 
> See c000a9bd06ea ("pci: mark device having guest unplug request pending")
>      a99c4da9fc2a ("pci: mark devices partially unplugged")
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> Reviewed-by: Ani Sinha <ani@anisinha.ca>
> ---
> 
> Notes:
>      v3: add some inlined comments to explain partially_hotplugged and
>          pending_deleted_event
>      v2: move partially_hotplugged to acpi_pcihp_eject_slot()
> 
>   hw/acpi/pcihp.c | 30 +++++++++++++++++++++++++++---
>   1 file changed, 27 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> index f610a25d2ef9..30405b5113d7 100644
> --- a/hw/acpi/pcihp.c
> +++ b/hw/acpi/pcihp.c
> @@ -222,9 +222,27 @@ static void acpi_pcihp_eject_slot(AcpiPciHpState *s, unsigned bsel, unsigned slo
>           PCIDevice *dev = PCI_DEVICE(qdev);
>           if (PCI_SLOT(dev->devfn) == slot) {
>               if (!acpi_pcihp_pc_no_hotplug(s, dev)) {
> -                hotplug_ctrl = qdev_get_hotplug_handler(qdev);
> -                hotplug_handler_unplug(hotplug_ctrl, qdev, &error_abort);
> -                object_unparent(OBJECT(qdev));
> +                /*
> +                 * partially_hotplugged is used by virtio-net failover:
> +                 * failover has asked the guest OS to unplug the device
> +                 * but we need to keep some references to the device
> +                 * to be able to plug it back in case of failure so
> +                 * we don't execute hotplug_handler_unplug().
> +                 */
> +                if (dev->partially_hotplugged) {
> +                    /*
> +                     * pending_deleted_event is set to true when
> +                     * virtio-net failover asks to unplug the device,
> +                     * and set to false here when the operation is done
> +                     * This is used by the migration loop to detect the
> +                     * end of the operation and really start the migration.
> +                     */
> +                    qdev->pending_deleted_event = false;
> +                } else {
> +                    hotplug_ctrl = qdev_get_hotplug_handler(qdev);
> +                    hotplug_handler_unplug(hotplug_ctrl, qdev, &error_abort);
> +                    object_unparent(OBJECT(qdev));
> +                }
>               }
>           }
>       }
> @@ -396,6 +414,12 @@ void acpi_pcihp_device_unplug_request_cb(HotplugHandler *hotplug_dev,
>           return;
>       }
>   
> +    /*
> +     * pending_deleted_event is used by virtio-net failover to detect the
> +     * end of the unplug operation, the flag is set to false in
> +     * acpi_pcihp_eject_slot() when the operation is completed.
> +     */
> +    pdev->qdev.pending_deleted_event = true;
>       s->acpi_pcihp_pci_status[bsel].down |= (1U << slot);
>       acpi_send_event(DEVICE(hotplug_dev), ACPI_PCI_HOTPLUG_STATUS);
>   }
> 

Re: [PATCH v3] failover: fix unplug pending detection

[Michael S. Tsirkin]

On Mon, Oct 18, 2021 at 09:19:16AM +0200, Laurent Vivier wrote:
> Hi,
> 
> I don't understand if there are some issues

Gerd did identify some issues, you felt they aren't related to the patch
and need to be addressed separately.

Gerd posted patches that are supposed to address them since.
"try improve native hotplug for pcie root ports"
Could you please either
- test and report that your series depend on
  Gerd's one to now work without the issues.
  preferably by reposting a patch that applies on top.
- test and report that the functionality is still partially
  broken but explain in the commit log that this is not due
  to the patch itself, and not made worse.

in both cases please CC reviewers: Daniel, Gerd.

Thanks!

> to address to have this patch merged?
> 
> Thanks,
> Laurent


> On 01/10/2021 10:25, Laurent Vivier wrote:
> > Failover needs to detect the end of the PCI unplug to start migration
> > after the VFIO card has been unplugged.
> > 
> > To do that, a flag is set in pcie_cap_slot_unplug_request_cb() and reset in
> > pcie_unplug_device().
> > 
> > But since
> >      17858a169508 ("hw/acpi/ich9: Set ACPI PCI hot-plug as default on Q35")
> > we have switched to ACPI unplug and these functions are not called anymore
> > and the flag not set. So failover migration is not able to detect if card
> > is really unplugged and acts as it's done as soon as it's started. So it
> > doesn't wait the end of the unplug to start the migration. We don't see any
> > problem when we test that because ACPI unplug is faster than PCIe native
> > hotplug and when the migration really starts the unplug operation is
> > already done.
> > 
> > See c000a9bd06ea ("pci: mark device having guest unplug request pending")
> >      a99c4da9fc2a ("pci: mark devices partially unplugged")
> > 
> > Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> > Reviewed-by: Ani Sinha <ani@anisinha.ca>
> > ---
> > 
> > Notes:
> >      v3: add some inlined comments to explain partially_hotplugged and
> >          pending_deleted_event
> >      v2: move partially_hotplugged to acpi_pcihp_eject_slot()
> > 
> >   hw/acpi/pcihp.c | 30 +++++++++++++++++++++++++++---
> >   1 file changed, 27 insertions(+), 3 deletions(-)
> > 
> > diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> > index f610a25d2ef9..30405b5113d7 100644
> > --- a/hw/acpi/pcihp.c
> > +++ b/hw/acpi/pcihp.c
> > @@ -222,9 +222,27 @@ static void acpi_pcihp_eject_slot(AcpiPciHpState *s, unsigned bsel, unsigned slo
> >           PCIDevice *dev = PCI_DEVICE(qdev);
> >           if (PCI_SLOT(dev->devfn) == slot) {
> >               if (!acpi_pcihp_pc_no_hotplug(s, dev)) {
> > -                hotplug_ctrl = qdev_get_hotplug_handler(qdev);
> > -                hotplug_handler_unplug(hotplug_ctrl, qdev, &error_abort);
> > -                object_unparent(OBJECT(qdev));
> > +                /*
> > +                 * partially_hotplugged is used by virtio-net failover:
> > +                 * failover has asked the guest OS to unplug the device
> > +                 * but we need to keep some references to the device
> > +                 * to be able to plug it back in case of failure so
> > +                 * we don't execute hotplug_handler_unplug().
> > +                 */
> > +                if (dev->partially_hotplugged) {
> > +                    /*
> > +                     * pending_deleted_event is set to true when
> > +                     * virtio-net failover asks to unplug the device,
> > +                     * and set to false here when the operation is done
> > +                     * This is used by the migration loop to detect the
> > +                     * end of the operation and really start the migration.
> > +                     */
> > +                    qdev->pending_deleted_event = false;
> > +                } else {
> > +                    hotplug_ctrl = qdev_get_hotplug_handler(qdev);
> > +                    hotplug_handler_unplug(hotplug_ctrl, qdev, &error_abort);
> > +                    object_unparent(OBJECT(qdev));
> > +                }
> >               }
> >           }
> >       }
> > @@ -396,6 +414,12 @@ void acpi_pcihp_device_unplug_request_cb(HotplugHandler *hotplug_dev,
> >           return;
> >       }
> > +    /*
> > +     * pending_deleted_event is used by virtio-net failover to detect the
> > +     * end of the unplug operation, the flag is set to false in
> > +     * acpi_pcihp_eject_slot() when the operation is completed.
> > +     */
> > +    pdev->qdev.pending_deleted_event = true;
> >       s->acpi_pcihp_pci_status[bsel].down |= (1U << slot);
> >       acpi_send_event(DEVICE(hotplug_dev), ACPI_PCI_HOTPLUG_STATUS);
> >   }
> > 

Re: [PATCH v3] failover: fix unplug pending detection

[Laurent Vivier]

On 18/10/2021 10:27, Michael S. Tsirkin wrote:
> On Mon, Oct 18, 2021 at 09:19:16AM +0200, Laurent Vivier wrote:
>> Hi,
>>
>> I don't understand if there are some issues
> 
> Gerd did identify some issues, you felt they aren't related to the patch
> and need to be addressed separately.
> 
> Gerd posted patches that are supposed to address them since.
> "try improve native hotplug for pcie root ports"
> Could you please either
> - test and report that your series depend on
>    Gerd's one to now work without the issues.
>    preferably by reposting a patch that applies on top.
> - test and report that the functionality is still partially
>    broken but explain in the commit log that this is not due
>    to the patch itself, and not made worse.
> 
> in both cases please CC reviewers: Daniel, Gerd.
>

I'm writing a test in tests/qtest that tests virtio-net failover, and I've added a test 
that checks the migration doesn't start while the card is not unplugged.

I've run the test on top of current qemu master (where Gerd's series is merged) and the 
problem still exists.

I will re-send this fix and the test in the same series.

Thanks,
Laurent

Re: [PATCH v3] failover: fix unplug pending detection

[Ani Sinha]

On Thu, Nov 18, 2021 at 2:45 PM Laurent Vivier <lvivier@redhat.com> wrote:
>
> On 18/10/2021 10:27, Michael S. Tsirkin wrote:
> > On Mon, Oct 18, 2021 at 09:19:16AM +0200, Laurent Vivier wrote:
> >> Hi,
> >>
> >> I don't understand if there are some issues
> >
> > Gerd did identify some issues, you felt they aren't related to the patch
> > and need to be addressed separately.
> >
> > Gerd posted patches that are supposed to address them since.
> > "try improve native hotplug for pcie root ports"
> > Could you please either
> > - test and report that your series depend on
> >    Gerd's one to now work without the issues.
> >    preferably by reposting a patch that applies on top.
> > - test and report that the functionality is still partially
> >    broken but explain in the commit log that this is not due
> >    to the patch itself, and not made worse.
> >
> > in both cases please CC reviewers: Daniel, Gerd.
> >
>
> I'm writing a test in tests/qtest that tests virtio-net failover, and I've added a test
> that checks the migration doesn't start while the card is not unplugged.
>
> I've run the test on top of current qemu master (where Gerd's series is merged) and the
> problem still exists.

btw, for the records, we have decided to continue to use acpi hotplug
as default and not revert to native for 6.2. So regardless of what
Gerd's patches does, we need to address issues around acpi hotplug if
it exists, for failover.

>
> I will re-send this fix and the test in the same series.
>
> Thanks,
> Laurent
>