Re: [PATCH] acpi: pcihp: make pending delete expire in 5sec

[Michael S. Tsirkin]

On Tue, Apr 04, 2023 at 07:40:40PM +0530, Ani Sinha wrote:
> 
> 
> On Tue, Apr 4, 2023 at 7:34 PM Igor Mammedov <imammedo@redhat.com> wrote:
> 
>     On Tue, 4 Apr 2023 08:46:15 -0400
>     "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
>     > 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.
> 
>     ok, lets use 1ms then, I'd rather reuse the preexisting
>     pending_deleted_expires_ms machinery instead of
>     special-casing immediate repeat.
> 
> 
> Sounds good to me.
>  

OK but please add a comment explaining what's going on.


> 
>     >
>     > > 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   
>     > > >   
>     >
> 
>