On Wed, Apr 05, 2023 at 11:38:56AM +0200, Igor Mammedov wrote: > On Wed, 5 Apr 2023 04:47:48 -0400 > "Michael S. Tsirkin" <m...@redhat.com> wrote: > > > On Wed, Apr 05, 2023 at 10:34:44AM +0200, Igor Mammedov wrote: > > > with Q35 using ACPI PCI hotplug by default, user's request to unplug > > > device is ignored when it's issued before guest OS has been booted. > > > And any additional attempt to request device hot-unplug afterwards > > > results in following error: > > > > > > "Device XYZ is already in the process of unplug" > > > > > > arguably it can be considered as a regression introduced by [2], > > > before which it was possible to issue unplug request multiple > > > times. > > > > > > Allowing pending delete blocking expire brings ACPI PCI hotplug > > > on par with native PCIe unplug behavior [1] and allows user > > > to repeat unplug requests at propper times. > > > Set expire timeout to arbitrary 1msec so user won't be able to > > > flood guest with SCI interrupts by calling device_del in tight loop. > > > > > > PS: > > > ACPI spec doesn't mandate what OSPM can do with GPEx.status > > > bits set before it's booted => it's impl. depended. > > > Status bits may be retained (I tested with one Windows version) > > > or cleared (Linux since 2.6 kernel times) during guest's ACPI > > > subsystem initialization. > > > Clearing status bits (though not wrong per se) hides the unplug > > > event from guest, and it's upto user to repeat device_del later > > > when guest is able to handle unplug requests. > > > > > > 1) 18416c62e3 ("pcie: expire pending delete") > > > 2) > > > Fixes: cce8944cc9ef ("qdev-monitor: Forbid repeated device_del") > > > Signed-off-by: Igor Mammedov <imamm...@redhat.com> > > > > I feel a real solution is to detect guest handling the > > event such as clearing GPE and allowing resending > > the interrupt then. > > I did consider preserving(masking clearing attempt) of GPE0.sts[] > IF en[] hasn't been enabled ever (i.e. should help with unplug at > the 1st boot). But that won't work across reboots (depends on > reboot kind) and it's twisting rules wrt spec (platform(QEMU/fw) > may set status bits, but it's upto OSPM to decide what to do with > them (when and how clear or ignore them). > > > A similar strategy should be possible with the attention > > button. > > > > > This patch is more of a band-aid - it is possible that guest > > rebooted and so user knows a new device_del is required, > > and we arbitrarily reject that. Right? > You lost me here. Can you elaborate?
I request device deletion but guest was rebooting losing state. I want to request deletion again but the request will fail until 1ms passes. 1ms is not a lot but I really don't want management to learn weird tricks like waiting for 1ms. > > > > This is arguably a regression but not in this release yes? > > So I don't think it needs to block qemu release. > > > > > > > --- > > > v2: > > > * change timeout to 1ms > > > * add comment to expire usage > > > * massage commit message to be a bit more clear > > > > > > CC: m...@redhat.com > > > CC: anisi...@redhat.com > > > CC: jus...@redhat.com > > > CC: kra...@redhat.com > > > > It's helpful to have CC before --- so backporters know whom to CC, too. > > ok, I'll fix it up and respin > > > > > > --- > > > hw/acpi/pcihp.c | 10 ++++++++++ > > > 1 file changed, 10 insertions(+) > > > > > > diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c > > > index dcfb779a7a..5daa732a33 100644 > > > --- a/hw/acpi/pcihp.c > > > +++ b/hw/acpi/pcihp.c > > > @@ -357,6 +357,16 @@ void > > > acpi_pcihp_device_unplug_request_cb(HotplugHandler *hotplug_dev, > > > * acpi_pcihp_eject_slot() when the operation is completed. > > > */ > > > pdev->qdev.pending_deleted_event = true; > > > + /* if unplug was requested before OSPM is initialized, > > > + * linux kernel will clear GPE0.sts[] bits during boot, which > > > effectively > > > + * hides unplug event. BAnd than followup qmp_device_del() calls > > > remain > > > > BAnd? > > > > > + * blocked by above flag permanently. > > > + * Unblock qmp_device_del() by setting expire limit, so user can > > > + * repeat unplug request later when OSPM has been booted. > > > + */ > > > + pdev->qdev.pending_deleted_expires_ms = > > > + qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL); /* 1 msec */ > > > + > > > s->acpi_pcihp_pci_status[bsel].down |= (1U << slot); > > > acpi_send_event(DEVICE(hotplug_dev), ACPI_PCI_HOTPLUG_STATUS); > > > } > > > -- > > > 2.39.1 > >