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