Re: [PATCH v2] acpi: pcihp: make pending delete blocking action expire

["Michael S. Tsirkin" <mst@redhat.com>]

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" <mst@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 <imammedo@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: mst@redhat.com
> > > CC: anisinha@redhat.com
> > > CC: jusual@redhat.com
> > > CC: kraxel@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  
> >