[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3] acpi: pcihp: make pending delete blocking action expire
From: |
Kashyap Chamarthy |
Subject: |
Re: [PATCH v3] acpi: pcihp: make pending delete blocking action expire |
Date: |
Tue, 18 Apr 2023 10:49:25 +0200 |
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 <imammedo@redhat.com>
> CC: mst@redhat.com
> CC: anisinha@redhat.com
> CC: jusual@redhat.com
> CC: kraxel@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