On Tue, 4 Apr 2023 18:00:06 +0200
Yu Zhang <yu.zh...@ionos.com> wrote:
> > this patch targets corner case of early boot where
> > guest hasn't initialized ACPI subsystem yet and 'broken'
> > management asking to unplug device too early which leads
> > to device stuck in being unplugged state due to regression
> > in QEMU.
> > However, It doesn't apply to fully booted guest.
>
> by adding a few debug lines I see that in
> acpi_pcihp_device_unplug_request_cb(),
>
> pdev->qdev.pending_deleted_event = true;
>
> was executed, which then directly triggered the error in:
If you do repeat unplug request right away after the 1st one,
then getting this error is expected behavior
(as guest needs time to react and unplug device).
> void qmp_device_del(const char *id, Error **errp)
> {
> DeviceState *dev = find_device_state(id, errp);
> if (dev != NULL) {
> if (dev->pending_deleted_event &&
> (dev->pending_deleted_expires_ms == 0 ||
> dev->pending_deleted_expires_ms >
> qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL))) {
> error_setg(errp, "Device %s is already in the "
> "process of unplug", id);
> return;
> }
>
> qdev_unplug(dev, errp);
> }
> }
>
> In QEMU code, there are 6 lines where this flag is changed:
>
> hw/core/qdev.c:564: dev->pending_deleted_event = false;
> hw/core/qdev.c:601: dev->pending_deleted_event = true;
> hw/acpi/pcihp.c:219: qdev->pending_deleted_event = false;
> hw/acpi/pcihp.c:359: pdev->qdev.pending_deleted_event = true;
> hw/pci/pcie.c:516: dev->qdev.pending_deleted_event = false;
> hw/pci/pcie.c:573: dev->pending_deleted_event = true;
>
> Considering the complexity of the code, the logic for setting and clearing
> this flag
> seems not quite straightforward. I doubt that the setting of
> pending_deleted_event in
> acpi_pcihp_device_unplug_request_cb() is the appropriate approach to
> accomplish its target.
It's true that pending_deleted_event is abused by failover and later by
pci hotplug.
see comment 15 where Paolo suggest how to fix it
https://bugzilla.redhat.com/show_bug.cgi?id=1878659#c15
and than see comment 17, explaining that cleaning up pending_deleted_event
usage won't change current behavior.
Anyways, clean up patches are welcome if you wish to follow up
on Paolo's suggestion.
> On Tue, Apr 4, 2023 at 2:25 PM Igor Mammedov <imamm...@redhat.com> wrote:
>
> > On Tue, 4 Apr 2023 08:45:54 +0200
> > Jinpu Wang <jinpu.w...@ionos.com> wrote:
> >
> > > Hi Yu,
> > >
> > > On Mon, Apr 3, 2023 at 6:59 PM Yu Zhang <yu.zh...@ionos.com> wrote:
> > > >
> > > > Dear Laurent,
> > > >
> > > > Thank you for your quick reply. We used qemu-7.1, but it is
> > reproducible with qemu from v6.2 to the recent v8.0 release candidates.
> > > > I found that it's introduced by the commit 9323f892b39 (between
> > v6.2.0-rc2 and v6.2.0-rc3).
> > > >
> > > > If it doesn't break anything else, it suffices to remove the line
> > below from acpi_pcihp_device_unplug_request_cb():
> > > >
> > > > pdev->qdev.pending_deleted_event = true;
> > > >
> > > > but you may have a reason to keep it. First of all, I'll open a bug in
> > the bug tracker and let you know.
> > > >
> > > > Best regards,
> > > > Yu Zhang
> > > This patch from Igor Mammedov seems relevant,
> > >
> > https://lore.kernel.org/qemu-devel/20230403131833-mutt-send-email-...@kernel.org/T/#t
> >
> > this patch targets corner case of early boot where
> > guest hasn't initialized ACPI subsystem yet and 'broken'
> > management asking to unplug device too early which leads
> > to device stuck in being unplugged state due to regression
> > in QEMU.
> > However, It doesn't apply to fully booted guest.
> >
> > [...]
> >
> > > >> > The purpose is for detecting the end of the PCI device hot-unplug.
> > However, we feel the
> > > >> > error confusing. How is it possible that a disk "is already in the
> > process of unplug"
> > > >> > during the first hot-unplug attempt? So far as I know, the issue
> > was also encountered by
> > > >> > libvirt, but they simply ignored it:
> > > >> >
> > > >> > https://bugzilla.redhat.com/show_bug.cgi?id=1878659
> > > >> > <https://bugzilla.redhat.com/show_bug.cgi?id=1878659>
> > see my other reply email/BZ comment 17.
> >
> > [...]
> >
> >
