On Wed, Apr 05, 2023 at 11:42:56AM +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.
First, thanks for this fix. I'm going to test it with the libvirt-based reproducer from here[1]. [1] https://gitlab.com/libvirt/libvirt/-/issues/309. > Allowing pending delete blocking expire brings ACPI PCI hotplug A small nit-pick, if you don't mind (as discussed on #qemu, OFTC IRC): Please rephrase the commit summary-line to "acpi: pcihp: allow repeating hot-unplug requests". The phrase "Allowing pending delete blocking expire" reads unclear to me. Specifically, I'm not sure how to parse the line "pending delete blocking". I don't know the internals of AHCI to be able to suggest a rephrasing of this bit. All I know is that the core idea here is to allow reissuing of hot-unplug requests. Hence I suggested to rename the commit summary-line. :-) > 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> > CC: m...@redhat.com > CC: anisi...@redhat.com > CC: jus...@redhat.com > CC: kra...@redhat.com > --- > v3: > * fix typo in comment > * move CC to the main commit message body > v2: > * change timeout to 1ms > * add comment to expire usage > * massage commit message to be a bit more clear > --- > hw/acpi/pcihp.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c > index dcfb779a7a..cdd6f775a1 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. And than followup qmp_device_del() calls remain > + * 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 > > -- /kashyap
