[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] acpi: pcihp: make pending delete expire in 5sec
From: |
Michael S. Tsirkin |
Subject: |
Re: [PATCH] acpi: pcihp: make pending delete expire in 5sec |
Date: |
Tue, 4 Apr 2023 08:46:15 -0400 |
On Tue, Apr 04, 2023 at 10:28:07AM +0200, Igor Mammedov wrote:
> On Mon, 3 Apr 2023 13:23:45 -0400
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
> > On Mon, Apr 03, 2023 at 06:16:18PM +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 expire brings ACPI PCI hotplug on par
> > > with native PCIe unplug behavior [1] which in its turn refers
> > > back to ACPI PCI hotplug ability to repeat unplug requests.
> > >
> > > PS:
> > > >From ACPI point of view, unplug request sets PCI hotplug status
> > > bit in GPE0 block. However depending on OSPM, status bits may
> > > be retained (Windows) or cleared (Linux) during guest's ACPI
> > > subsystem initialization, and as result Linux guest looses
> > > plug/unplug event (no SCI generated) if plug/unplug has
> > > happend before guest OS initialized GPE registers handling.
> > > I couldn't find any restrictions wrt OPM clearing GPE status
> > > bits ACPI spec.
> > > Hence a fallback approach is to let user repeat unplug request
> > > later at the time when guest OS has booted.
> > >
> > > 1) 18416c62e3 ("pcie: expire pending delete")
> > > 2)
> > > Fixes: cce8944cc9ef ("qdev-monitor: Forbid repeated device_del")
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> >
> > A bit concerned about how this interacts with failover,
> > and 5sec is a lot of time that I hoped we'd avoid with acpi.
> > Any better ideas of catching such misbehaving guests?
>
> It shouldn't affect affect failover, pending_delete is not
> cleared after all (only device removal should do that).
> So all patch does is allowing to reissue unplug request
> in case it was lost, delay here doesn't mean much
> (do you have any preference wrt specific value)?
I'd prefer immediately.
> As for 'misbehaving' - I tried to find justification
> for it in spec, but I couldn't.
> Essentially it's upto OSPM to clear or not GPE status
> bits at startup (linux was doing it since forever),
> depending on guest's ability to handle hotplug events
> at boot time.
>
> It's more a user error, ACPI hotplug does imply booted
> guest for it to function properly. So it's fine to
> loose unplug event at boot time. What QEMU does wrong is
> preventing follow up unplug requests.
>
> >
> > Also at this point I do not know why we deny hotplug
> > pending_deleted_event in qdev core.
> > Commit log says:
> >
> > Device unplug can be done asynchronously. Thus, sending the second
> > device_del before the previous unplug is complete may lead to
> > unexpected results. On PCIe devices, this cancels the hot-unplug
> > process.
> >
> > so it's a work around for an issue in pcie hotplug (and maybe shpc
> > too?). Maybe we should have put that check in pcie/shpc and
> > leave acpi along?
> >
> >
> >
> >
> > > ---
> > > CC: mst@redhat.com
> > > CC: anisinha@redhat.com
> > > CC: jusual@redhat.com
> > > CC: kraxel@redhat.com
> > > ---
> > > hw/acpi/pcihp.c | 2 ++
> > > 1 file changed, 2 insertions(+)
> > >
> > > diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> > > index dcfb779a7a..cd4f9fee0a 100644
> > > --- a/hw/acpi/pcihp.c
> > > +++ b/hw/acpi/pcihp.c
> > > @@ -357,6 +357,8 @@ 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;
> > > + pdev->qdev.pending_deleted_expires_ms =
> > > + qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + 5000; /* 5 secs */
> > > s->acpi_pcihp_pci_status[bsel].down |= (1U << slot);
> > > acpi_send_event(DEVICE(hotplug_dev), ACPI_PCI_HOTPLUG_STATUS);
> > > }
> > > --
> > > 2.39.1
> >
- Re: [PATCH] acpi: pcihp: make pending delete expire in 5sec, (continued)
- Re: [PATCH] acpi: pcihp: make pending delete expire in 5sec, Michael S. Tsirkin, 2023/04/03
- Re: [PATCH] acpi: pcihp: make pending delete expire in 5sec, Igor Mammedov, 2023/04/04
- Re: [PATCH] acpi: pcihp: make pending delete expire in 5sec, Gerd Hoffmann, 2023/04/04
- Re: [PATCH] acpi: pcihp: make pending delete expire in 5sec, Igor Mammedov, 2023/04/06
- Re: [PATCH] acpi: pcihp: make pending delete expire in 5sec, Gerd Hoffmann, 2023/04/06
Re: [PATCH] acpi: pcihp: make pending delete expire in 5sec, Igor Mammedov, 2023/04/04
Re: [PATCH] acpi: pcihp: make pending delete expire in 5sec, Michael S. Tsirkin, 2023/04/04
Re: [PATCH] acpi: pcihp: make pending delete expire in 5sec, Igor Mammedov, 2023/04/05
Re: [PATCH] acpi: pcihp: make pending delete expire in 5sec, Michael S. Tsirkin, 2023/04/05
Re: [PATCH] acpi: pcihp: make pending delete expire in 5sec, Igor Mammedov, 2023/04/05
Re: [PATCH] acpi: pcihp: make pending delete expire in 5sec, Michael S. Tsirkin, 2023/04/05
Re: [PATCH] acpi: pcihp: make pending delete expire in 5sec, Igor Mammedov, 2023/04/05
Re: [PATCH] acpi: pcihp: make pending delete expire in 5sec, Michael S. Tsirkin, 2023/04/05