[PATCH 0/6] RfC: try improve native hotplug for pcie root ports

[PATCH 0/6] RfC: try improve native hotplug for pcie root ports

[Gerd Hoffmann]

Gerd Hoffmann (6):
  pci: implement power state
  pcie: implement slow power control for pcie root ports
  pcie: add power indicator blink check
  pcie: factor out pcie_cap_slot_unplug()
  pcie: fast unplug when slot power is off
  pcie: expire pending delete

 include/hw/pci/pci.h   |  2 ++
 include/hw/qdev-core.h |  1 +
 hw/pci/pci.c           | 25 +++++++++++--
 hw/pci/pci_host.c      |  6 ++--
 hw/pci/pcie.c          | 82 ++++++++++++++++++++++++++++++++++--------
 softmmu/qdev-monitor.c |  4 ++-
 6 files changed, 101 insertions(+), 19 deletions(-)

-- 
2.31.1

[PATCH 1/6] pci: implement power state

[Gerd Hoffmann]

This allows to power off pci devices.  In "off" state the devices will
not be visible.  No pci config space access, no pci bar access, no dma.

Default state is "on", so this patch (alone) should not change behavior.

Use case:  Allows hotplug controllers implement slot power.  Hotplug
controllers doing so should set the inital power state for devices in
the ->plug callback.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 include/hw/pci/pci.h |  2 ++
 hw/pci/pci.c         | 25 +++++++++++++++++++++++--
 hw/pci/pci_host.c    |  6 ++++--
 3 files changed, 29 insertions(+), 4 deletions(-)

diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index 7fc90132cf1d..7cb6d92e17f7 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -268,6 +268,7 @@ typedef struct PCIReqIDCache PCIReqIDCache;
 struct PCIDevice {
     DeviceState qdev;
     bool partially_hotplugged;
+    bool has_power;
 
     /* PCI config space */
     uint8_t *config;
@@ -904,5 +905,6 @@ extern const VMStateDescription vmstate_pci_device;
 }
 
 MSIMessage pci_get_msi_message(PCIDevice *dev, int vector);
+void pci_set_power(PCIDevice *pci_dev, bool state);
 
 #endif
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 186758ee11fe..94f53944c7cc 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -1380,6 +1380,9 @@ static void pci_update_mappings(PCIDevice *d)
             continue;
 
         new_addr = pci_bar_address(d, i, r->type, r->size);
+        if (!d->has_power) {
+            new_addr = PCI_BAR_UNMAPPED;
+        }
 
         /* This bar isn't changed */
         if (new_addr == r->addr)
@@ -1464,8 +1467,8 @@ void pci_default_write_config(PCIDevice *d, uint32_t addr, uint32_t val_in, int
     if (range_covers_byte(addr, l, PCI_COMMAND)) {
         pci_update_irq_disabled(d, was_irq_disabled);
         memory_region_set_enabled(&d->bus_master_enable_region,
-                                  pci_get_word(d->config + PCI_COMMAND)
-                                    & PCI_COMMAND_MASTER);
+                                  (pci_get_word(d->config + PCI_COMMAND)
+                                   & PCI_COMMAND_MASTER) && d->has_power);
     }
 
     msi_write_config(d, addr, val_in, l);
@@ -2190,6 +2193,8 @@ static void pci_qdev_realize(DeviceState *qdev, Error **errp)
         pci_qdev_unrealize(DEVICE(pci_dev));
         return;
     }
+
+    pci_set_power(pci_dev, true);
 }
 
 PCIDevice *pci_new_multifunction(int devfn, bool multifunction,
@@ -2861,6 +2866,22 @@ MSIMessage pci_get_msi_message(PCIDevice *dev, int vector)
     return msg;
 }
 
+void pci_set_power(PCIDevice *d, bool state)
+{
+    if (d->has_power == state) {
+        return;
+    }
+
+    d->has_power = state;
+    pci_update_mappings(d);
+    memory_region_set_enabled(&d->bus_master_enable_region,
+                              (pci_get_word(d->config + PCI_COMMAND)
+                               & PCI_COMMAND_MASTER) && d->has_power);
+    if (!d->has_power) {
+        pci_device_reset(d);
+    }
+}
+
 static const TypeInfo pci_device_type_info = {
     .name = TYPE_PCI_DEVICE,
     .parent = TYPE_DEVICE,
diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c
index cf02f0d6a501..7beafd40a8e9 100644
--- a/hw/pci/pci_host.c
+++ b/hw/pci/pci_host.c
@@ -74,7 +74,8 @@ void pci_host_config_write_common(PCIDevice *pci_dev, uint32_t addr,
     /* non-zero functions are only exposed when function 0 is present,
      * allowing direct removal of unexposed functions.
      */
-    if (pci_dev->qdev.hotplugged && !pci_get_function_0(pci_dev)) {
+    if ((pci_dev->qdev.hotplugged && !pci_get_function_0(pci_dev)) ||
+        !pci_dev->has_power) {
         return;
     }
 
@@ -97,7 +98,8 @@ uint32_t pci_host_config_read_common(PCIDevice *pci_dev, uint32_t addr,
     /* non-zero functions are only exposed when function 0 is present,
      * allowing direct removal of unexposed functions.
      */
-    if (pci_dev->qdev.hotplugged && !pci_get_function_0(pci_dev)) {
+    if ((pci_dev->qdev.hotplugged && !pci_get_function_0(pci_dev)) ||
+        !pci_dev->has_power) {
         return ~0x0;
     }
 
-- 
2.31.1

[PATCH 2/6] pcie: implement slow power control for pcie root ports

[Gerd Hoffmann]

With this patch hot-plugged pci devices will only be visible to the
guest if the guests hotplug driver has enabled slot power.

This should fix the hot-plug race which one can hit when hot-plugging
a pci device at boot, while the guest is in the middle of the pci bus
scan.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/pci/pcie.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index 6e95d829037a..4a52c250615e 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -366,6 +366,29 @@ static void hotplug_event_clear(PCIDevice *dev)
     }
 }
 
+static void pcie_set_power_device(PCIBus *bus, PCIDevice *dev, void *opaque)
+{
+    bool *power = opaque;
+
+    pci_set_power(dev, *power);
+}
+
+static void pcie_cap_update_power(PCIDevice *hotplug_dev)
+{
+    uint8_t *exp_cap = hotplug_dev->config + hotplug_dev->exp.exp_cap;
+    PCIBus *sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE(hotplug_dev));
+    uint32_t sltcap = pci_get_long(exp_cap + PCI_EXP_SLTCAP);
+    uint16_t sltctl = pci_get_word(exp_cap + PCI_EXP_SLTCTL);
+    bool power = true;
+
+    if (sltcap & PCI_EXP_SLTCAP_PCP) {
+        power = (sltctl & PCI_EXP_SLTCTL_PCC) == PCI_EXP_SLTCTL_PWR_ON;
+    }
+
+    pci_for_each_device(sec_bus, pci_bus_num(sec_bus),
+                        pcie_set_power_device, &power);
+}
+
 /*
  * A PCI Express Hot-Plug Event has occurred, so update slot status register
  * and notify OS of the event if necessary.
@@ -434,6 +457,7 @@ void pcie_cap_slot_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
             pci_word_test_and_set_mask(exp_cap + PCI_EXP_LNKSTA,
                                        PCI_EXP_LNKSTA_DLLLA);
         }
+        pcie_cap_update_power(hotplug_pdev);
         return;
     }
 
@@ -451,6 +475,7 @@ void pcie_cap_slot_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
         }
         pcie_cap_slot_event(hotplug_pdev,
                             PCI_EXP_HP_EV_PDC | PCI_EXP_HP_EV_ABP);
+        pcie_cap_update_power(hotplug_pdev);
     }
 }
 
@@ -625,6 +650,7 @@ void pcie_cap_slot_reset(PCIDevice *dev)
                                  PCI_EXP_SLTSTA_PDC |
                                  PCI_EXP_SLTSTA_ABP);
 
+    pcie_cap_update_power(dev);
     hotplug_event_update_event_status(dev);
 }
 
@@ -707,6 +733,7 @@ void pcie_cap_slot_write_config(PCIDevice *dev,
         pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTSTA,
                                        PCI_EXP_SLTSTA_PDC);
     }
+    pcie_cap_update_power(dev);
 
     hotplug_event_notify(dev);
 
@@ -733,6 +760,7 @@ int pcie_cap_slot_post_load(void *opaque, int version_id)
 {
     PCIDevice *dev = opaque;
     hotplug_event_update_event_status(dev);
+    pcie_cap_update_power(dev);
     return 0;
 }
 
-- 
2.31.1

[PATCH 3/6] pcie: add power indicator blink check

[Gerd Hoffmann]

Refuse to push the attention button in case the guest is busy with some
hotplug operation (as indicated by the power indicator blinking).

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/pci/pcie.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index 4a52c250615e..c455f92e16bf 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -506,6 +506,7 @@ void pcie_cap_slot_unplug_request_cb(HotplugHandler *hotplug_dev,
     PCIDevice *hotplug_pdev = PCI_DEVICE(hotplug_dev);
     uint8_t *exp_cap = hotplug_pdev->config + hotplug_pdev->exp.exp_cap;
     uint32_t sltcap = pci_get_word(exp_cap + PCI_EXP_SLTCAP);
+    uint16_t sltctl = pci_get_word(exp_cap + PCI_EXP_SLTCTL);
 
     /* Check if hot-unplug is disabled on the slot */
     if ((sltcap & PCI_EXP_SLTCAP_HPC) == 0) {
@@ -521,6 +522,12 @@ void pcie_cap_slot_unplug_request_cb(HotplugHandler *hotplug_dev,
         return;
     }
 
+    if ((sltctl & PCI_EXP_SLTCTL_PIC) == PCI_EXP_SLTCTL_PWR_IND_BLINK) {
+        error_setg(errp, "Hot-unplug failed: "
+                   "guest is busy (power indicator blinking)");
+        return;
+    }
+
     dev->pending_deleted_event = true;
 
     /* In case user cancel the operation of multi-function hot-add,
-- 
2.31.1

[PATCH 4/6] pcie: factor out pcie_cap_slot_unplug()

[Gerd Hoffmann]

No functional change.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/pci/pcie.c | 35 +++++++++++++++++++++--------------
 1 file changed, 21 insertions(+), 14 deletions(-)

diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index c455f92e16bf..70fc11ba4c7d 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -497,6 +497,26 @@ static void pcie_unplug_device(PCIBus *bus, PCIDevice *dev, void *opaque)
     object_unparent(OBJECT(dev));
 }
 
+static void pcie_cap_slot_do_unplug(PCIDevice *dev)
+{
+    PCIBus *sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE(dev));
+    uint8_t *exp_cap = dev->config + dev->exp.exp_cap;
+    uint32_t lnkcap = pci_get_long(exp_cap + PCI_EXP_LNKCAP);
+
+    pci_for_each_device(sec_bus, pci_bus_num(sec_bus),
+                        pcie_unplug_device, NULL);
+
+    pci_word_test_and_clear_mask(exp_cap + PCI_EXP_SLTSTA,
+                                 PCI_EXP_SLTSTA_PDS);
+    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);
+    }
+    pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTSTA,
+                               PCI_EXP_SLTSTA_PDC);
+}
+
 void pcie_cap_slot_unplug_request_cb(HotplugHandler *hotplug_dev,
                                      DeviceState *dev, Error **errp)
 {
@@ -676,7 +696,6 @@ 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)) {
         /*
@@ -726,19 +745,7 @@ void pcie_cap_slot_write_config(PCIDevice *dev,
         (val & PCI_EXP_SLTCTL_PIC_OFF) == PCI_EXP_SLTCTL_PIC_OFF &&
         (!(old_slt_ctl & PCI_EXP_SLTCTL_PCC) ||
         (old_slt_ctl & PCI_EXP_SLTCTL_PIC_OFF) != PCI_EXP_SLTCTL_PIC_OFF)) {
-        PCIBus *sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE(dev));
-        pci_for_each_device(sec_bus, pci_bus_num(sec_bus),
-                            pcie_unplug_device, NULL);
-
-        pci_word_test_and_clear_mask(exp_cap + PCI_EXP_SLTSTA,
-                                     PCI_EXP_SLTSTA_PDS);
-        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);
-        }
-        pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTSTA,
-                                       PCI_EXP_SLTSTA_PDC);
+        pcie_cap_slot_do_unplug(dev);
     }
     pcie_cap_update_power(dev);
 
-- 
2.31.1

[PATCH 5/6] pcie: fast unplug when slot power is off

[Gerd Hoffmann]

In case the slot is powered off (and the power indicator turned off too)
we can unplug right away, without round-trip to the guest.

Also clear pending attention button press, there is nothing to care
about any more.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/pci/pcie.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index 70fc11ba4c7d..f3ac04399969 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -561,6 +561,16 @@ void pcie_cap_slot_unplug_request_cb(HotplugHandler *hotplug_dev,
         return;
     }
 
+    if (((sltctl & PCI_EXP_SLTCTL_PIC) == PCI_EXP_SLTCTL_PWR_IND_OFF) &&
+        ((sltctl & PCI_EXP_SLTCTL_PCC) == PCI_EXP_SLTCTL_PWR_OFF)) {
+        /* slot is powered off -> unplug without round-trip to the guest */
+        pcie_cap_slot_do_unplug(hotplug_pdev);
+        hotplug_event_notify(hotplug_pdev);
+        pci_word_test_and_clear_mask(exp_cap + PCI_EXP_SLTSTA,
+                                     PCI_EXP_SLTSTA_ABP);
+        return;
+    }
+
     pcie_cap_slot_push_attention_button(hotplug_pdev);
 }
 
-- 
2.31.1

[PATCH 6/6] pcie: expire pending delete

[Gerd Hoffmann]

Add an expire time for pending delete, once the time is over allow
pressing the attention button again.

This makes pcie hotplug behave more like acpi hotplug, where one can
try sending an 'device_del' monitor command again in case the guest
didn't respond to the first attempt.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 include/hw/qdev-core.h | 1 +
 hw/pci/pcie.c          | 2 ++
 softmmu/qdev-monitor.c | 4 +++-
 3 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 4ff19c714bd8..d082a9a00aca 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -180,6 +180,7 @@ struct DeviceState {
     char *canonical_path;
     bool realized;
     bool pending_deleted_event;
+    int64_t pending_deleted_expires_ms;
     QemuOpts *opts;
     int hotplugged;
     bool allow_unplug_during_migration;
diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index f3ac04399969..477c8776aa27 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -549,6 +549,8 @@ void pcie_cap_slot_unplug_request_cb(HotplugHandler *hotplug_dev,
     }
 
     dev->pending_deleted_event = true;
+    dev->pending_deleted_expires_ms =
+        qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + 5000; /* 5 secs */
 
     /* In case user cancel the operation of multi-function hot-add,
      * remove the function that is unexposed to guest individually,
diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index 0705f008466d..5e7960c52a0a 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -910,7 +910,9 @@ void qmp_device_del(const char *id, Error **errp)
 {
     DeviceState *dev = find_device_state(id, errp);
     if (dev != NULL) {
-        if (dev->pending_deleted_event) {
+        if (dev->pending_deleted_event &&
+            (dev->pending_deleted_expires_ms == 0 ||
+             dev->pending_deleted_expires_ms > qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL))) {
             error_setg(errp, "Device %s is already in the "
                              "process of unplug", id);
             return;
-- 
2.31.1

Re: [PATCH 6/6] pcie: expire pending delete

[Michael S. Tsirkin]

On Mon, Oct 11, 2021 at 02:05:04PM +0200, Gerd Hoffmann wrote:
> Add an expire time for pending delete, once the time is over allow
> pressing the attention button again.
> 
> This makes pcie hotplug behave more like acpi hotplug, where one can
> try sending an 'device_del' monitor command again in case the guest
> didn't respond to the first attempt.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  include/hw/qdev-core.h | 1 +
>  hw/pci/pcie.c          | 2 ++
>  softmmu/qdev-monitor.c | 4 +++-
>  3 files changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 4ff19c714bd8..d082a9a00aca 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -180,6 +180,7 @@ struct DeviceState {
>      char *canonical_path;
>      bool realized;
>      bool pending_deleted_event;
> +    int64_t pending_deleted_expires_ms;
>      QemuOpts *opts;
>      int hotplugged;
>      bool allow_unplug_during_migration;
> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> index f3ac04399969..477c8776aa27 100644
> --- a/hw/pci/pcie.c
> +++ b/hw/pci/pcie.c
> @@ -549,6 +549,8 @@ void pcie_cap_slot_unplug_request_cb(HotplugHandler *hotplug_dev,
>      }
>  
>      dev->pending_deleted_event = true;
> +    dev->pending_deleted_expires_ms =
> +        qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + 5000; /* 5 secs */
>  
>      /* In case user cancel the operation of multi-function hot-add,
>       * remove the function that is unexposed to guest individually,


Well this will be barely enough, right?

	Once the Power
	Indicator begins blinking, a 5-second abort interval exists during which a second depression of the
	Attention Button cancels the operation.

So I guess it needs to be more. Problem is of course if guest is
busy because of interrupts and whatnot, it might not get to
handling that in time ...


> diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
> index 0705f008466d..5e7960c52a0a 100644
> --- a/softmmu/qdev-monitor.c
> +++ b/softmmu/qdev-monitor.c
> @@ -910,7 +910,9 @@ void qmp_device_del(const char *id, Error **errp)
>  {
>      DeviceState *dev = find_device_state(id, errp);
>      if (dev != NULL) {
> -        if (dev->pending_deleted_event) {
> +        if (dev->pending_deleted_event &&
> +            (dev->pending_deleted_expires_ms == 0 ||
> +             dev->pending_deleted_expires_ms > qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL))) {
>              error_setg(errp, "Device %s is already in the "
>                               "process of unplug", id);
>              return;
> -- 
> 2.31.1

Re: [PATCH 6/6] pcie: expire pending delete

[Gerd Hoffmann]

> > index f3ac04399969..477c8776aa27 100644
> > --- a/hw/pci/pcie.c
> > +++ b/hw/pci/pcie.c
> > @@ -549,6 +549,8 @@ void pcie_cap_slot_unplug_request_cb(HotplugHandler *hotplug_dev,
> >      }
> >  
> >      dev->pending_deleted_event = true;
> > +    dev->pending_deleted_expires_ms =
> > +        qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + 5000; /* 5 secs */
> >  
> >      /* In case user cancel the operation of multi-function hot-add,
> >       * remove the function that is unexposed to guest individually,
> 
> 
> Well this will be barely enough, right?
> 
> 	Once the Power
> 	Indicator begins blinking, a 5-second abort interval exists during which a second depression of the
> 	Attention Button cancels the operation.

Well, canceling the hot-plug is not supported in qemu right now (there
is no qmp command for that).  I'm also not sure it makes sense in the
first place for virtual machines.

> So I guess it needs to be more. Problem is of course if guest is
> busy because of interrupts and whatnot, it might not get to
> handling that in time ...

See patch #3, that one should take care of a busy guest ...

take care,
  Gerd

Re: [PATCH 6/6] pcie: expire pending delete

[Michael S. Tsirkin]

On Tue, Oct 12, 2021 at 07:30:34AM +0200, Gerd Hoffmann wrote:
> > > index f3ac04399969..477c8776aa27 100644
> > > --- a/hw/pci/pcie.c
> > > +++ b/hw/pci/pcie.c
> > > @@ -549,6 +549,8 @@ void pcie_cap_slot_unplug_request_cb(HotplugHandler *hotplug_dev,
> > >      }
> > >  
> > >      dev->pending_deleted_event = true;
> > > +    dev->pending_deleted_expires_ms =
> > > +        qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + 5000; /* 5 secs */
> > >  
> > >      /* In case user cancel the operation of multi-function hot-add,
> > >       * remove the function that is unexposed to guest individually,
> > 
> > 
> > Well this will be barely enough, right?
> > 
> > 	Once the Power
> > 	Indicator begins blinking, a 5-second abort interval exists during which a second depression of the
> > 	Attention Button cancels the operation.
> 
> Well, canceling the hot-plug is not supported in qemu right now (there
> is no qmp command for that).  I'm also not sure it makes sense in the
> first place for virtual machines.

Yes. However if you resend an attention button press within the
5 second window, guest will think you cancelled hot-plug
and act accordingly.
It's a fundamentally racy algorithm :(


> > So I guess it needs to be more. Problem is of course if guest is
> > busy because of interrupts and whatnot, it might not get to
> > handling that in time ...
> 
> See patch #3, that one should take care of a busy guest ...
> 
> take care,
>   Gerd

Re: [PATCH 5/6] pcie: fast unplug when slot power is off

[Michael S. Tsirkin]

On Mon, Oct 11, 2021 at 02:05:03PM +0200, Gerd Hoffmann wrote:
> In case the slot is powered off (and the power indicator turned off too)
> we can unplug right away, without round-trip to the guest.
> 
> Also clear pending attention button press, there is nothing to care
> about any more.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/pci/pcie.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> index 70fc11ba4c7d..f3ac04399969 100644
> --- a/hw/pci/pcie.c
> +++ b/hw/pci/pcie.c
> @@ -561,6 +561,16 @@ void pcie_cap_slot_unplug_request_cb(HotplugHandler *hotplug_dev,
>          return;
>      }
>  
> +    if (((sltctl & PCI_EXP_SLTCTL_PIC) == PCI_EXP_SLTCTL_PWR_IND_OFF) &&
> +        ((sltctl & PCI_EXP_SLTCTL_PCC) == PCI_EXP_SLTCTL_PWR_OFF)) {
> +        /* slot is powered off -> unplug without round-trip to the guest */
> +        pcie_cap_slot_do_unplug(hotplug_pdev);
> +        hotplug_event_notify(hotplug_pdev);
> +        pci_word_test_and_clear_mask(exp_cap + PCI_EXP_SLTSTA,
> +                                     PCI_EXP_SLTSTA_ABP);

Does this handle all the things including link status etc btw?
I don't remember off-hand.

> +        return;
> +    }
> +
>      pcie_cap_slot_push_attention_button(hotplug_pdev);
>  }
>  
> -- 
> 2.31.1

Re: [PATCH 6/6] pcie: expire pending delete

[Gerd Hoffmann]

On Tue, Oct 12, 2021 at 01:46:35AM -0400, Michael S. Tsirkin wrote:
> On Tue, Oct 12, 2021 at 07:30:34AM +0200, Gerd Hoffmann wrote:
> > > > index f3ac04399969..477c8776aa27 100644
> > > > --- a/hw/pci/pcie.c
> > > > +++ b/hw/pci/pcie.c
> > > > @@ -549,6 +549,8 @@ void pcie_cap_slot_unplug_request_cb(HotplugHandler *hotplug_dev,
> > > >      }
> > > >  
> > > >      dev->pending_deleted_event = true;
> > > > +    dev->pending_deleted_expires_ms =
> > > > +        qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + 5000; /* 5 secs */
> > > >  
> > > >      /* In case user cancel the operation of multi-function hot-add,
> > > >       * remove the function that is unexposed to guest individually,
> > > 
> > > 
> > > Well this will be barely enough, right?
> > > 
> > > 	Once the Power
> > > 	Indicator begins blinking, a 5-second abort interval exists during which a second depression of the
> > > 	Attention Button cancels the operation.
> > 
> > Well, canceling the hot-plug is not supported in qemu right now (there
> > is no qmp command for that).  I'm also not sure it makes sense in the
> > first place for virtual machines.
> 
> Yes. However if you resend an attention button press within the
> 5 second window, guest will think you cancelled hot-plug
> and act accordingly.
> It's a fundamentally racy algorithm :(

That's why re-sending an attention button press is blocked
for 5 seconds.

It's also blocked in case the guest blinks the power
indicator (see patch #3).

Both together work well in my testing, I can flood a (linux) guest
with device_del commands without bad side effects:

First device_del command sends attention button press.
Then device_del is rejected because the 5 secs are not over yet.
Then device_del is rejected because the indicator blinks.
Then unplug completes (and qemu sends event).
Then device_del fails because the device doesn't exist any more.

take care,
  Gerd

Re: [PATCH 5/6] pcie: fast unplug when slot power is off

[Gerd Hoffmann]

On Tue, Oct 12, 2021 at 01:56:31AM -0400, Michael S. Tsirkin wrote:
> On Mon, Oct 11, 2021 at 02:05:03PM +0200, Gerd Hoffmann wrote:
> > In case the slot is powered off (and the power indicator turned off too)
> > we can unplug right away, without round-trip to the guest.
> > 
> > Also clear pending attention button press, there is nothing to care
> > about any more.
> > 
> > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> > ---
> >  hw/pci/pcie.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> > index 70fc11ba4c7d..f3ac04399969 100644
> > --- a/hw/pci/pcie.c
> > +++ b/hw/pci/pcie.c
> > @@ -561,6 +561,16 @@ void pcie_cap_slot_unplug_request_cb(HotplugHandler *hotplug_dev,
> >          return;
> >      }
> >  
> > +    if (((sltctl & PCI_EXP_SLTCTL_PIC) == PCI_EXP_SLTCTL_PWR_IND_OFF) &&
> > +        ((sltctl & PCI_EXP_SLTCTL_PCC) == PCI_EXP_SLTCTL_PWR_OFF)) {
> > +        /* slot is powered off -> unplug without round-trip to the guest */
> > +        pcie_cap_slot_do_unplug(hotplug_pdev);
> > +        hotplug_event_notify(hotplug_pdev);
> > +        pci_word_test_and_clear_mask(exp_cap + PCI_EXP_SLTSTA,
> > +                                     PCI_EXP_SLTSTA_ABP);
> 
> Does this handle all the things including link status etc btw?
> I don't remember off-hand.

Yes.  See patch #4 which moves the relevant code to the new
pcie_cap_slot_do_unplug() helper.

take care,
  Gerd

Re: [PATCH 6/6] pcie: expire pending delete

[Michael S. Tsirkin]

On Tue, Oct 12, 2021 at 08:44:45AM +0200, Gerd Hoffmann wrote:
> On Tue, Oct 12, 2021 at 01:46:35AM -0400, Michael S. Tsirkin wrote:
> > On Tue, Oct 12, 2021 at 07:30:34AM +0200, Gerd Hoffmann wrote:
> > > > > index f3ac04399969..477c8776aa27 100644
> > > > > --- a/hw/pci/pcie.c
> > > > > +++ b/hw/pci/pcie.c
> > > > > @@ -549,6 +549,8 @@ void pcie_cap_slot_unplug_request_cb(HotplugHandler *hotplug_dev,
> > > > >      }
> > > > >  
> > > > >      dev->pending_deleted_event = true;
> > > > > +    dev->pending_deleted_expires_ms =
> > > > > +        qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + 5000; /* 5 secs */
> > > > >  
> > > > >      /* In case user cancel the operation of multi-function hot-add,
> > > > >       * remove the function that is unexposed to guest individually,
> > > > 
> > > > 
> > > > Well this will be barely enough, right?
> > > > 
> > > > 	Once the Power
> > > > 	Indicator begins blinking, a 5-second abort interval exists during which a second depression of the
> > > > 	Attention Button cancels the operation.
> > > 
> > > Well, canceling the hot-plug is not supported in qemu right now (there
> > > is no qmp command for that).  I'm also not sure it makes sense in the
> > > first place for virtual machines.
> > 
> > Yes. However if you resend an attention button press within the
> > 5 second window, guest will think you cancelled hot-plug
> > and act accordingly.
> > It's a fundamentally racy algorithm :(
> 
> That's why re-sending an attention button press is blocked
> for 5 seconds.
> It's also blocked in case the guest blinks the power
> indicator (see patch #3).
> 
> Both together work well in my testing, I can flood a (linux) guest
> with device_del commands without bad side effects:
> 
> First device_del command sends attention button press.
> Then device_del is rejected because the 5 secs are not over yet.
> Then device_del is rejected because the indicator blinks.

Ah, I see. 5 secs is a lot so yea, most likely it's gonnu
be ok, worst case we'll wait another 5 seconds and
send again, right?

Worth checking with windows guests BTW, we saw lots of
races with these too.


> Then unplug completes (and qemu sends event).
> Then device_del fails because the device doesn't exist any more.
> 
> take care,
>   Gerd

Re: [PATCH 0/6] RfC: try improve native hotplug for pcie root ports

[Michael S. Tsirkin]

On Mon, Oct 11, 2021 at 02:04:58PM +0200, Gerd Hoffmann wrote:
> 
> 
> Gerd Hoffmann (6):
>   pci: implement power state
>   pcie: implement slow power control for pcie root ports
>   pcie: add power indicator blink check
>   pcie: factor out pcie_cap_slot_unplug()
>   pcie: fast unplug when slot power is off
>   pcie: expire pending delete

So what's left to do here?
I'm guessing more testing?
I would also like to see a shorter timeout, maybe 100ms, so
that we are more responsive to guest changes in resending request.


>  include/hw/pci/pci.h   |  2 ++
>  include/hw/qdev-core.h |  1 +
>  hw/pci/pci.c           | 25 +++++++++++--
>  hw/pci/pci_host.c      |  6 ++--
>  hw/pci/pcie.c          | 82 ++++++++++++++++++++++++++++++++++--------
>  softmmu/qdev-monitor.c |  4 ++-
>  6 files changed, 101 insertions(+), 19 deletions(-)
> 
> -- 
> 2.31.1
> 

Re: [PATCH 0/6] RfC: try improve native hotplug for pcie root ports

[Gerd Hoffmann]

On Mon, Oct 18, 2021 at 11:36:45AM -0400, Michael S. Tsirkin wrote:
> On Mon, Oct 11, 2021 at 02:04:58PM +0200, Gerd Hoffmann wrote:
> > 
> > 
> > Gerd Hoffmann (6):
> >   pci: implement power state
> >   pcie: implement slow power control for pcie root ports
> >   pcie: add power indicator blink check
> >   pcie: factor out pcie_cap_slot_unplug()
> >   pcie: fast unplug when slot power is off
> >   pcie: expire pending delete
> 
> So what's left to do here?
> I'm guessing more testing?

Yes.  Maybe ask rh qe to run the patch set through their hotplug test
suite (to avoid a apci-hotplug style disaster where qe found various
issues after release)?

> I would also like to see a shorter timeout, maybe 100ms, so
> that we are more responsive to guest changes in resending request.

I don't think it is a good idea to go for a shorter timeout given that
the 5 seconds are in the specs and we want avoid a resent request being
interpreted as cancel.

It also wouldn't change anything at least for linux guests because linux
is waiting those 5 seconds (with power indicator in blinking state).
Only the reason for refusing 'device_del' changes from "5 secs not over
yet" to "guest is busy processing the hotplug request".

We could consider to tackle the 5sec timeout on the guest side, i.e.
have linux skip the 5sec wait in case the root port is virtual (should
be easy to figure by checking the pci id).

take care,
  Gerd

Re: [PATCH 0/6] RfC: try improve native hotplug for pcie root ports

[Michael S. Tsirkin]

On Tue, Oct 19, 2021 at 07:21:44AM +0200, Gerd Hoffmann wrote:
> On Mon, Oct 18, 2021 at 11:36:45AM -0400, Michael S. Tsirkin wrote:
> > On Mon, Oct 11, 2021 at 02:04:58PM +0200, Gerd Hoffmann wrote:
> > > 
> > > 
> > > Gerd Hoffmann (6):
> > >   pci: implement power state
> > >   pcie: implement slow power control for pcie root ports
> > >   pcie: add power indicator blink check
> > >   pcie: factor out pcie_cap_slot_unplug()
> > >   pcie: fast unplug when slot power is off
> > >   pcie: expire pending delete
> > 
> > So what's left to do here?
> > I'm guessing more testing?
> 
> Yes.  Maybe ask rh qe to run the patch set through their hotplug test
> suite (to avoid a apci-hotplug style disaster where qe found various
> issues after release)?

I'll poke around to see if they can help us... we'll need
a backport for that though.

> > I would also like to see a shorter timeout, maybe 100ms, so
> > that we are more responsive to guest changes in resending request.
> 
> I don't think it is a good idea to go for a shorter timeout given that
> the 5 seconds are in the specs and we want avoid a resent request being
> interpreted as cancel.
> It also wouldn't change anything at least for linux guests because linux
> is waiting those 5 seconds (with power indicator in blinking state).
> Only the reason for refusing 'device_del' changes from "5 secs not over
> yet" to "guest is busy processing the hotplug request".

First 5 seconds yes. But the retries afterwards?

> 
> We could consider to tackle the 5sec timeout on the guest side, i.e.
> have linux skip the 5sec wait in case the root port is virtual (should
> be easy to figure by checking the pci id).
> 
> take care,
>   Gerd

Yes ... do we want to control how long it blinks from hypervisor side?
And if we do then what? Shorten retry period?

-- 
MST

Re: [PATCH 0/6] RfC: try improve native hotplug for pcie root ports

[Gerd Hoffmann]

  Hi,

> > Yes.  Maybe ask rh qe to run the patch set through their hotplug test
> > suite (to avoid a apci-hotplug style disaster where qe found various
> > issues after release)?
> 
> I'll poke around to see if they can help us... we'll need
> a backport for that though.

Easy, it's a clean cherry-pick for 6.1, scratch build is on the way.

> > > I would also like to see a shorter timeout, maybe 100ms, so
> > > that we are more responsive to guest changes in resending request.
> > 
> > I don't think it is a good idea to go for a shorter timeout given that
> > the 5 seconds are in the specs and we want avoid a resent request being
> > interpreted as cancel.
> > It also wouldn't change anything at least for linux guests because linux
> > is waiting those 5 seconds (with power indicator in blinking state).
> > Only the reason for refusing 'device_del' changes from "5 secs not over
> > yet" to "guest is busy processing the hotplug request".
> 
> First 5 seconds yes. But the retries afterwards?

Hmm, maybe, but I'd tend to keep it simple and go for 5 secs no matter
what.  If the guest isn't responding (maybe because it is in the middle
of a reboot) it's unlikely that fast re-requests are fundamentally
changing things.

> > We could consider to tackle the 5sec timeout on the guest side, i.e.
> > have linux skip the 5sec wait in case the root port is virtual (should
> > be easy to figure by checking the pci id).
> > 
> > take care,
> >   Gerd
> 
> Yes ... do we want to control how long it blinks from hypervisor side?

Is there a good reason for that?
If not, then no.  Keep it simple.

When the guest powers off the slot pcie_cap_slot_write_config() will
happily unplug the device without additional checks (no check whenever
the 5 seconds are over, also no check whenever there is a pending unplug
request in the first place).

So in theory the guest turning off slot power quickly should work just
fine and speed up the unplug process in the common case (guest is
up'n'running and responsitive).  Down to 1-2 secs instead of 5-7.
Didn't actually test that though.

take care,
  Gerd

Re: [PATCH 0/6] RfC: try improve native hotplug for pcie root ports

[Michael S. Tsirkin]

On Tue, Oct 19, 2021 at 08:29:19AM +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> > > Yes.  Maybe ask rh qe to run the patch set through their hotplug test
> > > suite (to avoid a apci-hotplug style disaster where qe found various
> > > issues after release)?
> > 
> > I'll poke around to see if they can help us... we'll need
> > a backport for that though.
> 
> Easy, it's a clean cherry-pick for 6.1, scratch build is on the way.
> 
> > > > I would also like to see a shorter timeout, maybe 100ms, so
> > > > that we are more responsive to guest changes in resending request.
> > > 
> > > I don't think it is a good idea to go for a shorter timeout given that
> > > the 5 seconds are in the specs and we want avoid a resent request being
> > > interpreted as cancel.
> > > It also wouldn't change anything at least for linux guests because linux
> > > is waiting those 5 seconds (with power indicator in blinking state).
> > > Only the reason for refusing 'device_del' changes from "5 secs not over
> > > yet" to "guest is busy processing the hotplug request".
> > 
> > First 5 seconds yes. But the retries afterwards?
> 
> Hmm, maybe, but I'd tend to keep it simple and go for 5 secs no matter
> what.  If the guest isn't responding (maybe because it is in the middle
> of a reboot) it's unlikely that fast re-requests are fundamentally
> changing things.
> 
> > > We could consider to tackle the 5sec timeout on the guest side, i.e.
> > > have linux skip the 5sec wait in case the root port is virtual (should
> > > be easy to figure by checking the pci id).
> > > 
> > > take care,
> > >   Gerd
> > 
> > Yes ... do we want to control how long it blinks from hypervisor side?
> 
> Is there a good reason for that?
> If not, then no.  Keep it simple.
> 
> When the guest powers off the slot pcie_cap_slot_write_config() will
> happily unplug the device without additional checks (no check whenever
> the 5 seconds are over, also no check whenever there is a pending unplug
> request in the first place).
> 
> So in theory the guest turning off slot power quickly should work just
> fine and speed up the unplug process in the common case (guest is
> up'n'running and responsitive).  Down to 1-2 secs instead of 5-7.
> Didn't actually test that though.
> 
> take care,
>   Gerd

Even if this speeds up unplug, hotplug remains slow, right?
-- 
MST

Re: [PATCH 0/6] RfC: try improve native hotplug for pcie root ports

[Gerd Hoffmann]

  Hi,

> > So in theory the guest turning off slot power quickly should work just
> > fine and speed up the unplug process in the common case (guest is
> > up'n'running and responsitive).  Down to 1-2 secs instead of 5-7.
> > Didn't actually test that though.

Tried mean while, test patch (not polished yet) below.

> Even if this speeds up unplug, hotplug remains slow, right?

Notplug never was slow for me ...

take care,
  Gerd

From 074627a24a54203f2b4baf787fd4110c78222e23 Mon Sep 17 00:00:00 2001
From: Gerd Hoffmann <kraxel@redhat.com>
Date: Tue, 19 Oct 2021 09:12:22 +0200
Subject: [PATCH] pciehp: fast virtual unplug for VMs

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/pci/hotplug/pciehp.h      |  3 +++
 drivers/pci/hotplug/pciehp_core.c |  5 +++++
 drivers/pci/hotplug/pciehp_ctrl.c | 10 +++++++++-
 3 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h
index 69fd401691be..131ffec2e947 100644
--- a/drivers/pci/hotplug/pciehp.h
+++ b/drivers/pci/hotplug/pciehp.h
@@ -79,6 +79,7 @@ extern int pciehp_poll_time;
  * @request_result: result of last user request submitted to the IRQ thread
  * @requester: wait queue to wake up on completion of user request,
  *	used for synchronous slot enable/disable request via sysfs
+ * @is_virtual: virtual machine pcie port.
  *
  * PCIe hotplug has a 1:1 relationship between controller and slot, hence
  * unlike other drivers, the two aren't represented by separate structures.
@@ -109,6 +110,8 @@ struct controller {
 	unsigned int ist_running;
 	int request_result;
 	wait_queue_head_t requester;
+
+	bool is_virtual;
 };
 
 /**
diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c
index ad3393930ecb..28867ec33f5b 100644
--- a/drivers/pci/hotplug/pciehp_core.c
+++ b/drivers/pci/hotplug/pciehp_core.c
@@ -227,6 +227,11 @@ static int pciehp_probe(struct pcie_device *dev)
 		goto err_out_shutdown_notification;
 	}
 
+	if (dev->port->vendor == PCI_VENDOR_ID_REDHAT &&
+	    dev->port->device == 0x000c)
+		/* qemu pcie root port */
+		ctrl->is_virtual = true;
+
 	pciehp_check_presence(ctrl);
 
 	return 0;
diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
index 529c34808440..c0a05bbdb948 100644
--- a/drivers/pci/hotplug/pciehp_ctrl.c
+++ b/drivers/pci/hotplug/pciehp_ctrl.c
@@ -21,6 +21,10 @@
 #include <linux/pci.h>
 #include "pciehp.h"
 
+static bool fast_virtual_unplug = true;
+module_param(fast_virtual_unplug, bool, 0644);
+MODULE_PARM_DESC(fast_virtual_unplug, "Fast unplug (don't wait 5 seconds) for virtual machines.");
+
 /* The following routines constitute the bulk of the
    hotplug controller logic
  */
@@ -176,7 +180,11 @@ void pciehp_handle_button_press(struct controller *ctrl)
 		/* blink power indicator and turn off attention */
 		pciehp_set_indicators(ctrl, PCI_EXP_SLTCTL_PWR_IND_BLINK,
 				      PCI_EXP_SLTCTL_ATTN_IND_OFF);
-		schedule_delayed_work(&ctrl->button_work, 5 * HZ);
+		if (ctrl->is_virtual && fast_virtual_unplug) {
+			schedule_delayed_work(&ctrl->button_work, 1);
+		} else {
+			schedule_delayed_work(&ctrl->button_work, 5 * HZ);
+		}
 		break;
 	case BLINKINGOFF_STATE:
 	case BLINKINGON_STATE:
-- 
2.31.1

Re: [PATCH 0/6] RfC: try improve native hotplug for pcie root ports

[Michael S. Tsirkin]

Given it's a bugfix, and given that I hear through internal channels
that QE results so far have been encouraging, I am inclined to bite the
bullet and merge this for -rc1.

I don't think this conflicts with Julia's patches as users can still
disable ACPI hotplug into bridges.  Gerd, agree? Worth the risk?


On Mon, Oct 11, 2021 at 02:04:58PM +0200, Gerd Hoffmann wrote:
> 
> 
> Gerd Hoffmann (6):
>   pci: implement power state
>   pcie: implement slow power control for pcie root ports
>   pcie: add power indicator blink check
>   pcie: factor out pcie_cap_slot_unplug()
>   pcie: fast unplug when slot power is off
>   pcie: expire pending delete
> 
>  include/hw/pci/pci.h   |  2 ++
>  include/hw/qdev-core.h |  1 +
>  hw/pci/pci.c           | 25 +++++++++++--
>  hw/pci/pci_host.c      |  6 ++--
>  hw/pci/pcie.c          | 82 ++++++++++++++++++++++++++++++++++--------
>  softmmu/qdev-monitor.c |  4 ++-
>  6 files changed, 101 insertions(+), 19 deletions(-)
> 
> -- 
> 2.31.1
> 

Re: [PATCH 0/6] RfC: try improve native hotplug for pcie root ports

[Gerd Hoffmann]

  Hi,

> Given it's a bugfix, and given that I hear through internal channels
> that QE results so far have been encouraging, I am inclined to bite the
> bullet and merge this for -rc1.

Fine with me.

> I don't think this conflicts with Julia's patches as users can still
> disable ACPI hotplug into bridges.  Gerd, agree?  Worth the risk?

Combining this with Julia's patches looks a bit risky to me and surely
needs testing.  I expect the problematic case is both native and acpi
hotplug being enabled.  When the guest uses acpi hotplug nobody will
turn on slot power on the pcie root port ...

I'd suggest to just revert to pcie native hotplug for 6.2.  Give acpi
hotplug another try for 7.0, and post patches early in the devel cycle
then instead of waiting until -rc0 is released.

take care,
  Gerd

Re: [PATCH 0/6] RfC: try improve native hotplug for pcie root ports

[Michael S. Tsirkin]

On Thu, Nov 11, 2021 at 08:53:06AM +0100, Gerd Hoffmann wrote:
>   Hi,
> 
> > Given it's a bugfix, and given that I hear through internal channels
> > that QE results so far have been encouraging, I am inclined to bite the
> > bullet and merge this for -rc1.
> 
> Fine with me.
> 
> > I don't think this conflicts with Julia's patches as users can still
> > disable ACPI hotplug into bridges.  Gerd, agree?  Worth the risk?
> 
> Combining this with Julia's patches looks a bit risky to me and surely
> needs testing.  I expect the problematic case is both native and acpi
> hotplug being enabled.
>  When the guest uses acpi hotplug nobody will
> turn on slot power on the pcie root port ...

I'm not sure I understand what the situation is, and how to trigger it.
Could you clarify pls?

> I'd suggest to just revert to pcie native hotplug for 6.2.

Hmm that kind of change seems even riskier to me. I think I'll try with
Igor's patches.

> Give acpi
> hotplug another try for 7.0, and post patches early in the devel cycle
> then instead of waiting until -rc0 is released.
> 
> take care,
>   Gerd

Re: [PATCH 0/6] RfC: try improve native hotplug for pcie root ports

[Gerd Hoffmann]

On Thu, Nov 11, 2021 at 03:20:07AM -0500, Michael S. Tsirkin wrote:
> On Thu, Nov 11, 2021 at 08:53:06AM +0100, Gerd Hoffmann wrote:
> >   Hi,
> > 
> > > Given it's a bugfix, and given that I hear through internal channels
> > > that QE results so far have been encouraging, I am inclined to bite the
> > > bullet and merge this for -rc1.
> > 
> > Fine with me.
> > 
> > > I don't think this conflicts with Julia's patches as users can still
> > > disable ACPI hotplug into bridges.  Gerd, agree?  Worth the risk?
> > 
> > Combining this with Julia's patches looks a bit risky to me and surely
> > needs testing.  I expect the problematic case is both native and acpi
> > hotplug being enabled.
> >  When the guest uses acpi hotplug nobody will
> > turn on slot power on the pcie root port ...
> 
> I'm not sure I understand what the situation is, and how to trigger it.
> Could you clarify pls?

My patch series implements proper slot power emulation for pcie root
ports, i.e. the pci device plugged into the slot is only visible to
the guest when slot power is enabled.

When the pciehp driver is used the linux kernel will enable slot power
of course.

When the acpihp driver is used the linux kernel will just call the aml
methods and I suspect the pci device will stay invisible then because
nobody flips the slot power control bit (with native-hotplug=on, for
native-hotplug=off this isn't a problem of course).

> > I'd suggest to just revert to pcie native hotplug for 6.2.
> 
> Hmm that kind of change seems even riskier to me.

It's not clear to me why you consider that riskier.  It should fix the
qemu 6.1 regressions.  Given QE has found no problems so far it should
not be worse than qemu 6.0 was.  Most likely it will work better than
qemu 6.0.

Going with a new approach (enable both native and acpi hotplug) after
freeze looks risky to me, even without considering the conflict outlined
above.  Last-minute changes with the chance to repeat the 6.1 disaster,
only with a different set of bugs ...

take care,
  Gerd

Re: [PATCH 0/6] RfC: try improve native hotplug for pcie root ports

[Daniel P. Berrangé]

On Thu, Nov 11, 2021 at 03:20:07AM -0500, Michael S. Tsirkin wrote:
> On Thu, Nov 11, 2021 at 08:53:06AM +0100, Gerd Hoffmann wrote:
> >   Hi,
> > 
> > > Given it's a bugfix, and given that I hear through internal channels
> > > that QE results so far have been encouraging, I am inclined to bite the
> > > bullet and merge this for -rc1.
> > 
> > Fine with me.
> > 
> > > I don't think this conflicts with Julia's patches as users can still
> > > disable ACPI hotplug into bridges.  Gerd, agree?  Worth the risk?
> > 
> > Combining this with Julia's patches looks a bit risky to me and surely
> > needs testing.  I expect the problematic case is both native and acpi
> > hotplug being enabled.
> >  When the guest uses acpi hotplug nobody will
> > turn on slot power on the pcie root port ...
> 
> I'm not sure I understand what the situation is, and how to trigger it.
> Could you clarify pls?
> 
> > I'd suggest to just revert to pcie native hotplug for 6.2.
> 
> Hmm that kind of change seems even riskier to me. I think I'll try with
> Igor's patches.

Why would it be risky ? PCIE native hotplug is what we've used in
QEMU for years & years, until 6.1 enabled the buggy ACPI hotplug.
The behaviour of the current PCIE native hotplug impl is a known
quantity.

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 0/6] RfC: try improve native hotplug for pcie root ports

[Gerd Hoffmann]

  Hi,

> When the acpihp driver is used the linux kernel will just call the aml
> methods and I suspect the pci device will stay invisible then because
> nobody flips the slot power control bit (with native-hotplug=on, for
> native-hotplug=off this isn't a problem of course).

Hmm, on a quick smoke test with both patch series (mine + igors) applied
everything seems to work fine on a quick glance.  Dunno why.  Maybe the
pcieport driver turns on slot power even in case pciehp is not active.

I still feel a bit nervous about trying the new "both pcie + acpi
hotplug enabled" approach that close to the release date ...

take care,
  Gerd

Re: [PATCH 0/6] RfC: try improve native hotplug for pcie root ports

[Michael S. Tsirkin]

On Thu, Nov 11, 2021 at 01:09:05PM +0100, Gerd Hoffmann wrote:
>   Hi,
> 
> > When the acpihp driver is used the linux kernel will just call the aml
> > methods and I suspect the pci device will stay invisible then because
> > nobody flips the slot power control bit (with native-hotplug=on, for
> > native-hotplug=off this isn't a problem of course).
> 
> Hmm, on a quick smoke test with both patch series (mine + igors) applied
> everything seems to work fine on a quick glance.  Dunno why.  Maybe the
> pcieport driver turns on slot power even in case pciehp is not active.

Well power and hotplug capabilities are mostly unrelated, right?

> I still feel a bit nervous about trying the new "both pcie + acpi
> hotplug enabled" approach that close to the release date ...
> 
> take care,
>   Gerd

I feel switching to native so late would be inappropriate, looks more
like a feature than a bugfix. Given that - we need Igor's patches.
Given that - would you say I should apply yours? I'm inclined to
do so but if you prefer waiting until after the release I'll
defer to your judgement.

-- 
MST

Re: [PATCH 0/6] RfC: try improve native hotplug for pcie root ports

[Michael S. Tsirkin]

On Thu, Nov 11, 2021 at 09:35:37AM +0000, Daniel P. Berrangé wrote:
> On Thu, Nov 11, 2021 at 03:20:07AM -0500, Michael S. Tsirkin wrote:
> > On Thu, Nov 11, 2021 at 08:53:06AM +0100, Gerd Hoffmann wrote:
> > >   Hi,
> > > 
> > > > Given it's a bugfix, and given that I hear through internal channels
> > > > that QE results so far have been encouraging, I am inclined to bite the
> > > > bullet and merge this for -rc1.
> > > 
> > > Fine with me.
> > > 
> > > > I don't think this conflicts with Julia's patches as users can still
> > > > disable ACPI hotplug into bridges.  Gerd, agree?  Worth the risk?
> > > 
> > > Combining this with Julia's patches looks a bit risky to me and surely
> > > needs testing.  I expect the problematic case is both native and acpi
> > > hotplug being enabled.
> > >  When the guest uses acpi hotplug nobody will
> > > turn on slot power on the pcie root port ...
> > 
> > I'm not sure I understand what the situation is, and how to trigger it.
> > Could you clarify pls?
> > 
> > > I'd suggest to just revert to pcie native hotplug for 6.2.
> > 
> > Hmm that kind of change seems even riskier to me. I think I'll try with
> > Igor's patches.
> 
> Why would it be risky ? PCIE native hotplug is what we've used in
> QEMU for years & years, until 6.1 enabled the buggy ACPI hotplug.
> The behaviour of the current PCIE native hotplug impl is a known
> quantity.
> 
> Regards,
> Daniel

For example we might regress some of the bugs that the switch to ACPI fixed back to
6.0 state. There were a bunch of these. For example it should be
possible for guests to disable native and use ACPI then, but isn't.
I'm very willing to consider the switch back to native by default
but given the timing doing big changes like that at the last
minute seems unusual.

> -- 
> |: 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 0/6] RfC: try improve native hotplug for pcie root ports

[Daniel P. Berrangé]

On Thu, Nov 11, 2021 at 12:11:19PM -0500, Michael S. Tsirkin wrote:
> On Thu, Nov 11, 2021 at 09:35:37AM +0000, Daniel P. Berrangé wrote:
> > On Thu, Nov 11, 2021 at 03:20:07AM -0500, Michael S. Tsirkin wrote:
> > > On Thu, Nov 11, 2021 at 08:53:06AM +0100, Gerd Hoffmann wrote:
> > > >   Hi,
> > > > 
> > > > > Given it's a bugfix, and given that I hear through internal channels
> > > > > that QE results so far have been encouraging, I am inclined to bite the
> > > > > bullet and merge this for -rc1.
> > > > 
> > > > Fine with me.
> > > > 
> > > > > I don't think this conflicts with Julia's patches as users can still
> > > > > disable ACPI hotplug into bridges.  Gerd, agree?  Worth the risk?
> > > > 
> > > > Combining this with Julia's patches looks a bit risky to me and surely
> > > > needs testing.  I expect the problematic case is both native and acpi
> > > > hotplug being enabled.
> > > >  When the guest uses acpi hotplug nobody will
> > > > turn on slot power on the pcie root port ...
> > > 
> > > I'm not sure I understand what the situation is, and how to trigger it.
> > > Could you clarify pls?
> > > 
> > > > I'd suggest to just revert to pcie native hotplug for 6.2.
> > > 
> > > Hmm that kind of change seems even riskier to me. I think I'll try with
> > > Igor's patches.
> > 
> > Why would it be risky ? PCIE native hotplug is what we've used in
> > QEMU for years & years, until 6.1 enabled the buggy ACPI hotplug.
> > The behaviour of the current PCIE native hotplug impl is a known
> > quantity.
> > 
> > Regards,
> > Daniel
> 
> For example we might regress some of the bugs that the switch to ACPI fixed back to
> 6.0 state. There were a bunch of these. For example it should be
> possible for guests to disable native and use ACPI then, but isn't.

Of course there were bugs fixed by switching to ACPI, but we'd
lived with native hotplug in production and the majority of
the time it worked as users needed. The bugs were edge cases
essentially only affecting a small subset of users.

The switch to ACPI broke the out of the box configuration for
used by OpenStack. That's not an edge case, that's a serious
impact.

> I'm very willing to consider the switch back to native by default
> but given the timing doing big changes like that at the last
> minute seems unusual.

I consider it to be fixing a serious regression by going back
to a known working safe impl, that has been used in production
successfully for a long time. We know there are bugs with
native hotplug, but they're *known* problems.

The unsual thing about timing is having a major functional
regression identified in the previous release and then not
even having patches propposed to fix it until after soft
freeze for the next release arrives :-(

It doesn't give a feeling of confidence, but makes me
wonder what other *unknown* problems we're liable to hit
still.

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 0/6] RfC: try improve native hotplug for pcie root ports

[Michael S. Tsirkin]

On Thu, Nov 11, 2021 at 06:08:11PM +0000, Daniel P. Berrangé wrote:
> On Thu, Nov 11, 2021 at 12:11:19PM -0500, Michael S. Tsirkin wrote:
> > On Thu, Nov 11, 2021 at 09:35:37AM +0000, Daniel P. Berrangé wrote:
> > > On Thu, Nov 11, 2021 at 03:20:07AM -0500, Michael S. Tsirkin wrote:
> > > > On Thu, Nov 11, 2021 at 08:53:06AM +0100, Gerd Hoffmann wrote:
> > > > >   Hi,
> > > > > 
> > > > > > Given it's a bugfix, and given that I hear through internal channels
> > > > > > that QE results so far have been encouraging, I am inclined to bite the
> > > > > > bullet and merge this for -rc1.
> > > > > 
> > > > > Fine with me.
> > > > > 
> > > > > > I don't think this conflicts with Julia's patches as users can still
> > > > > > disable ACPI hotplug into bridges.  Gerd, agree?  Worth the risk?
> > > > > 
> > > > > Combining this with Julia's patches looks a bit risky to me and surely
> > > > > needs testing.  I expect the problematic case is both native and acpi
> > > > > hotplug being enabled.
> > > > >  When the guest uses acpi hotplug nobody will
> > > > > turn on slot power on the pcie root port ...
> > > > 
> > > > I'm not sure I understand what the situation is, and how to trigger it.
> > > > Could you clarify pls?
> > > > 
> > > > > I'd suggest to just revert to pcie native hotplug for 6.2.
> > > > 
> > > > Hmm that kind of change seems even riskier to me. I think I'll try with
> > > > Igor's patches.
> > > 
> > > Why would it be risky ? PCIE native hotplug is what we've used in
> > > QEMU for years & years, until 6.1 enabled the buggy ACPI hotplug.
> > > The behaviour of the current PCIE native hotplug impl is a known
> > > quantity.
> > > 
> > > Regards,
> > > Daniel
> > 
> > For example we might regress some of the bugs that the switch to ACPI fixed back to
> > 6.0 state. There were a bunch of these. For example it should be
> > possible for guests to disable native and use ACPI then, but isn't.
> 
> Of course there were bugs fixed by switching to ACPI, but we'd
> lived with native hotplug in production and the majority of
> the time it worked as users needed. The bugs were edge cases
> essentially only affecting a small subset of users.
> 
> The switch to ACPI broke the out of the box configuration for
> used by OpenStack. That's not an edge case, that's a serious
> impact.

Right. It's pretty easy for downstreams to switch back if they want to,
though.

> > I'm very willing to consider the switch back to native by default
> > but given the timing doing big changes like that at the last
> > minute seems unusual.
> 
> I consider it to be fixing a serious regression by going back
> to a known working safe impl, that has been used in production
> successfully for a long time. We know there are bugs with
> native hotplug, but they're *known* problems.

Are you familiar with the issues or are just arguing generally?  From my
POV they made native too unreliable to be useful.  It's great that the
narrow usecase of openstack managed not to hit any of the races
involved, but I think it's more a question of luck.  This until these
specific patches, but they are only in v2 out of rfc status just today.

> The unsual thing about timing is having a major functional
> regression identified in the previous release and then not
> even having patches propposed to fix it until after soft
> freeze for the next release arrives :-(
> 
> It doesn't give a feeling of confidence, but makes me
> wonder what other *unknown* problems we're liable to hit
> still.
> 
> Regards,
> Daniel

Right. And this applies to both approaches.  So I thought hard about the
balance here, and am inclined to go with a rough consensus opinion.

I don't see any reasons or in fact anyone objecting strongly to Igor's
patches, so I'm taking these. These are just bugfixes.

But if we also reach a rough consensus on others and e.g. Igor acks
Gerd's patch to revert to native then I think I'll merge it, it's
a close thing.

> -- 
> |: 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 0/6] RfC: try improve native hotplug for pcie root ports

[Gerd Hoffmann]

  Hi,

> involved, but I think it's more a question of luck.  This until these
> specific patches, but they are only in v2 out of rfc status just today.

Never posted a formal non-rfc version because I had no pending changes.
Maybe I should have done that ...

v2 has no functional changes, it only resolves a conflict,
so effectively the same thing as the rfc series.

take care,
  Gerd

Re: [PATCH 0/6] RfC: try improve native hotplug for pcie root ports

[Gerd Hoffmann]

On Thu, Nov 11, 2021 at 10:39:59AM -0500, Michael S. Tsirkin wrote:
> On Thu, Nov 11, 2021 at 01:09:05PM +0100, Gerd Hoffmann wrote:
> >   Hi,
> > 
> > > When the acpihp driver is used the linux kernel will just call the aml
> > > methods and I suspect the pci device will stay invisible then because
> > > nobody flips the slot power control bit (with native-hotplug=on, for
> > > native-hotplug=off this isn't a problem of course).
> > 
> > Hmm, on a quick smoke test with both patch series (mine + igors) applied
> > everything seems to work fine on a quick glance.  Dunno why.  Maybe the
> > pcieport driver turns on slot power even in case pciehp is not active.

Digged deeper.  Updating power status is handled by the plug() callback,
which is never called in case acpi hotplug is active.  The guest seems
to never touch slot power control either, so it's working fine.  Still
feels a bit fragile though.

> Well power and hotplug capabilities are mostly unrelated, right?

At least they are separate slot capabilities.  The linux pciehp driver
checks whenever the power control is present before using it, so having
PwrCtrl- HotPlug+ seems to be a valid combination.

We even have an option for that: pcie-root-port.power_controller_present

So flipping that to off in case apci hotplug is active should make sure
we never run into trouble with pci devices being powered off.

Igor?  Can you add that to your patch series?

> I feel switching to native so late would be inappropriate, looks more
> like a feature than a bugfix. Given that - we need Igor's patches.
> Given that - would you say I should apply yours?

I think when setting power_controller_present=off for acpi hotplug it is
safe to merge both mine and igor's.

take care,
  Gerd

Re: [PATCH 0/6] RfC: try improve native hotplug for pcie root ports

[Igor Mammedov]

On Fri, 12 Nov 2021 12:15:28 +0100
Gerd Hoffmann <kraxel@redhat.com> wrote:

> On Thu, Nov 11, 2021 at 10:39:59AM -0500, Michael S. Tsirkin wrote:
> > On Thu, Nov 11, 2021 at 01:09:05PM +0100, Gerd Hoffmann wrote:  
> > >   Hi,
> > >   
> > > > When the acpihp driver is used the linux kernel will just call the aml
> > > > methods and I suspect the pci device will stay invisible then because
> > > > nobody flips the slot power control bit (with native-hotplug=on, for
> > > > native-hotplug=off this isn't a problem of course).  
> > > 
> > > Hmm, on a quick smoke test with both patch series (mine + igors) applied
> > > everything seems to work fine on a quick glance.  Dunno why.  Maybe the
> > > pcieport driver turns on slot power even in case pciehp is not active.  
> 
> Digged deeper.  Updating power status is handled by the plug() callback,
> which is never called in case acpi hotplug is active.  The guest seems
> to never touch slot power control either, so it's working fine.  Still
> feels a bit fragile though.
> 
> > Well power and hotplug capabilities are mostly unrelated, right?  
> 
> At least they are separate slot capabilities.  The linux pciehp driver
> checks whenever the power control is present before using it, so having
> PwrCtrl- HotPlug+ seems to be a valid combination.
> 
> We even have an option for that: pcie-root-port.power_controller_present
> 
> So flipping that to off in case apci hotplug is active should make sure
> we never run into trouble with pci devices being powered off.
> 
> Igor?  Can you add that to your patch series?

Sorry, saw it too late.
I'll test idea with my set of guests to see if there are any adverse effects.


> > I feel switching to native so late would be inappropriate, looks more
> > like a feature than a bugfix. Given that - we need Igor's patches.
> > Given that - would you say I should apply yours?  
> 
> I think when setting power_controller_present=off for acpi hotplug it is
> safe to merge both mine and igor's.
> 
> take care,
>   Gerd
> 

Re: [PATCH 0/6] RfC: try improve native hotplug for pcie root ports

[Michael S. Tsirkin]

On Fri, Nov 12, 2021 at 12:15:28PM +0100, Gerd Hoffmann wrote:
> On Thu, Nov 11, 2021 at 10:39:59AM -0500, Michael S. Tsirkin wrote:
> > On Thu, Nov 11, 2021 at 01:09:05PM +0100, Gerd Hoffmann wrote:
> > >   Hi,
> > > 
> > > > When the acpihp driver is used the linux kernel will just call the aml
> > > > methods and I suspect the pci device will stay invisible then because
> > > > nobody flips the slot power control bit (with native-hotplug=on, for
> > > > native-hotplug=off this isn't a problem of course).
> > > 
> > > Hmm, on a quick smoke test with both patch series (mine + igors) applied
> > > everything seems to work fine on a quick glance.  Dunno why.  Maybe the
> > > pcieport driver turns on slot power even in case pciehp is not active.
> 
> Digged deeper.  Updating power status is handled by the plug() callback,
> which is never called in case acpi hotplug is active.  The guest seems
> to never touch slot power control either, so it's working fine.  Still
> feels a bit fragile though.
> 
> > Well power and hotplug capabilities are mostly unrelated, right?
> 
> At least they are separate slot capabilities.  The linux pciehp driver
> checks whenever the power control is present before using it, so having
> PwrCtrl- HotPlug+ seems to be a valid combination.
> 
> We even have an option for that: pcie-root-port.power_controller_present
> 
> So flipping that to off in case apci hotplug is active should make sure
> we never run into trouble with pci devices being powered off.
> 
> Igor?  Can you add that to your patch series?
> 
> > I feel switching to native so late would be inappropriate, looks more
> > like a feature than a bugfix. Given that - we need Igor's patches.
> > Given that - would you say I should apply yours?
> 
> I think when setting power_controller_present=off for acpi hotplug it is
> safe to merge both mine and igor's.
> 
> take care,
>   Gerd

So this did not surface yet but I guess we can do this as
a patch on top, either of you can post it.

-- 
MST

Re: [PATCH 3/6] pcie: add power indicator blink check

[Michael S. Tsirkin]

On Mon, Oct 11, 2021 at 02:05:01PM +0200, Gerd Hoffmann wrote:
> Refuse to push the attention button in case the guest is busy with some
> hotplug operation (as indicated by the power indicator blinking).
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>

Can't we do better and press the button later after
indicator stops blinking?

> ---
>  hw/pci/pcie.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> index 4a52c250615e..c455f92e16bf 100644
> --- a/hw/pci/pcie.c
> +++ b/hw/pci/pcie.c
> @@ -506,6 +506,7 @@ void pcie_cap_slot_unplug_request_cb(HotplugHandler *hotplug_dev,
>      PCIDevice *hotplug_pdev = PCI_DEVICE(hotplug_dev);
>      uint8_t *exp_cap = hotplug_pdev->config + hotplug_pdev->exp.exp_cap;
>      uint32_t sltcap = pci_get_word(exp_cap + PCI_EXP_SLTCAP);
> +    uint16_t sltctl = pci_get_word(exp_cap + PCI_EXP_SLTCTL);
>  
>      /* Check if hot-unplug is disabled on the slot */
>      if ((sltcap & PCI_EXP_SLTCAP_HPC) == 0) {
> @@ -521,6 +522,12 @@ void pcie_cap_slot_unplug_request_cb(HotplugHandler *hotplug_dev,
>          return;
>      }
>  
> +    if ((sltctl & PCI_EXP_SLTCTL_PIC) == PCI_EXP_SLTCTL_PWR_IND_BLINK) {
> +        error_setg(errp, "Hot-unplug failed: "
> +                   "guest is busy (power indicator blinking)");
> +        return;
> +    }
> +
>      dev->pending_deleted_event = true;
>  
>      /* In case user cancel the operation of multi-function hot-add,
> -- 
> 2.31.1

Re: [PATCH 3/6] pcie: add power indicator blink check

[Gerd Hoffmann]

On Mon, Nov 15, 2021 at 06:29:18AM -0500, Michael S. Tsirkin wrote:
> On Mon, Oct 11, 2021 at 02:05:01PM +0200, Gerd Hoffmann wrote:
> > Refuse to push the attention button in case the guest is busy with some
> > hotplug operation (as indicated by the power indicator blinking).
> > 
> > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> 
> Can't we do better and press the button later after
> indicator stops blinking?

I don't think this is a good idea.  acpi hotplug doesn't do automatic
retries either, and IMHO the behavior of acpi and native hotplug should
be as close as possible so management apps do not need to handle these
cases differently.

Specifically OpenStack will continue re-trying device_del commands
(mentioned by Daniel a few weeks ago) as long as it doesn't receive
the completion event, so things should work just fine as-is.

Beside that the typical workflow would be that the guest completes
device shutdown, then turns off power (-> qemu unplugs device and
sends completion event now), then turns off the power indicator led.
So in most cases the reason to press the button is gone when the
indicator stops blinking ...

take care,
  Gerd