From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 87A84C6FD1D for ; Tue, 4 Apr 2023 16:03:08 +0000 (UTC) Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1pjj5f-0006M5-7E; Tue, 04 Apr 2023 12:00:59 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1pjj5B-000622-Oi for qemu-devel@nongnu.org; Tue, 04 Apr 2023 12:00:31 -0400 Received: from mail-ed1-x530.google.com ([2a00:1450:4864:20::530]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1pjj55-0000sM-N9 for qemu-devel@nongnu.org; Tue, 04 Apr 2023 12:00:28 -0400 Received: by mail-ed1-x530.google.com with SMTP id eg48so132501130edb.13 for ; Tue, 04 Apr 2023 09:00:19 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ionos.com; s=google; t=1680624017; x=1683216017; h=to:subject:message-id:date:from:in-reply-to:references:mime-version :from:to:cc:subject:date:message-id:reply-to; bh=AnbQpTPXZqn/fH6T+i2fN71U7OOpZ9l2Dr+GSiQ/XkQ=; b=Xr3ymvzx9B26FBxCoajHFLAa507jJJ02c8b94QtxRiOcgCEdgTRzZOQvexarLVtcvt ixSEAJYkTtTSMzw4vC76Fst0wMu7m1atAIKqT/jnDYt+ViDsgWMzpHUGPlHr71Ce6ke9 4Va/REVR14Xddua/v3u7dJHbbSqNBMyd5clPeCKWF5yzb/dFz2SWaySzUu0CQhsHezUu g5AbtS2mxqGpL9WHVoy25X0vVV1ZeVpUocwKZbqfzs+oGD5hfLmfIuJMqgfy0Md2VXrw r76KV09RmlwWyV8hb3n9Zqow2s+dOen6JyieGAAZwSCVQQbVs9G8yqc8fY4VRLz6zkAo 5Gdg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1680624017; x=1683216017; h=to:subject:message-id:date:from:in-reply-to:references:mime-version :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=AnbQpTPXZqn/fH6T+i2fN71U7OOpZ9l2Dr+GSiQ/XkQ=; b=s9KWyew8E4ASkVo6yeI6T0/cYQpAtpudIoQUwHRWvtNkaYSRIX+rpjbi6ayZiGfXLv RQ8jaZ2pQXBEIFAdXKtzIcziid7ZMNijoEHLPZIK8zHsT96XBzV7ToYkfzW7YgiDJ8iq 6gjZu73VYLDs8NnJPJol222Ysh6uquSQ1WPn28cn5qUoNX9eIoSoxd00n+flC8OSlhpN 5mvEZ7x8RBjb6KlWMkZhRUGALzd7/uFTArIJEietk2J5KoKL5tBmLvNW/AJaBfHOXuuG BjO8+U94jLF1oxMcMZRcAsRUyYehXRL5IYrWFYosthLbZkK4EPoubHa4J3I5tws8PSIc xogQ== X-Gm-Message-State: AAQBX9cLy1LU0mjZxfbcccg7W0DfLc057eTqw6Pr3WCq3tT49OY3v485 v6WA7/LYd8u1zEpz3ukpgO95sCD4K2OG0Zy7YUY1Qw== X-Google-Smtp-Source: AKy350YMHFX5xF+wuyKDHnQMn21EQ3GVxt3h2MMOFxfxot+XrerUJN8CF7an213Rolp3XNn33f5Pn30RpKtfyCUqmJQ= X-Received: by 2002:a50:d597:0:b0:502:3c99:417f with SMTP id v23-20020a50d597000000b005023c99417fmr1673503edi.6.1680624017137; Tue, 04 Apr 2023 09:00:17 -0700 (PDT) MIME-Version: 1.0 References: <94e21f89-0a3e-701b-7171-7398dff9ce46@redhat.com> <20230404142553.31030bb7@imammedo.users.ipa.redhat.com> In-Reply-To: <20230404142553.31030bb7@imammedo.users.ipa.redhat.com> From: Yu Zhang Date: Tue, 4 Apr 2023 18:00:06 +0200 Message-ID: Subject: Re: an issue for device hot-unplug To: Igor Mammedov , Laurent Vivier , qemu-devel , Jinpu Wang , Elmar Gerdes Content-Type: multipart/alternative; boundary="0000000000008af65305f884c5f8" Received-SPF: permerror client-ip=2a00:1450:4864:20::530; envelope-from=yu.zhang@ionos.com; helo=mail-ed1-x530.google.com X-Spam_score_int: -20 X-Spam_score: -2.1 X-Spam_bar: -- X-Spam_report: (-2.1 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, HTML_MESSAGE=0.001, RCVD_IN_DNSWL_NONE=-0.0001, SPF_HELO_NONE=0.001, T_SPF_PERMERROR=0.01 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org --0000000000008af65305f884c5f8 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable > 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 =3D true; was executed, which then directly triggered the error in: void qmp_device_del(const char *id, Error **errp) { DeviceState *dev =3D find_device_state(id, errp); if (dev !=3D NULL) { if (dev->pending_deleted_event && (dev->pending_deleted_expires_ms =3D=3D 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 =3D false; hw/core/qdev.c:601: dev->pending_deleted_event =3D true; hw/acpi/pcihp.c:219: qdev->pending_deleted_event =3D fal= se; hw/acpi/pcihp.c:359: pdev->qdev.pending_deleted_event =3D true; hw/pci/pcie.c:516: dev->qdev.pending_deleted_event =3D false; hw/pci/pcie.c:573: dev->pending_deleted_event =3D 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. On Tue, Apr 4, 2023 at 2:25=E2=80=AFPM Igor Mammedov = wrote: > On Tue, 4 Apr 2023 08:45:54 +0200 > Jinpu Wang wrote: > > > Hi Yu, > > > > On Mon, Apr 3, 2023 at 6:59=E2=80=AFPM Yu Zhang wr= ote: > > > > > > 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 =3D true; > > > > > > but you may have a reason to keep it. First of all, I'll open a bug i= n > 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-mst@ker= nel.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=3D1878659 > > >> > > see my other reply email/BZ comment 17. > > [...] > > --0000000000008af65305f884c5f8 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
> this patch targets corner case of early boot where> guest hasn't initialized ACPI subsystem yet and 'broken'<= br>> 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 addin= g a few debug lines I see that in acpi_pcihp_device_unplug_request_cb(),
=C2=A0 =C2=A0 pdev->qdev.pending_deleted_event =3D true;

wa= s executed, which then directly triggered the error in:

void qmp_de= vice_del(const char *id, Error **errp)
{
=C2=A0 =C2=A0 DeviceState *d= ev =3D find_device_state(id, errp);
=C2=A0 =C2=A0 if (dev !=3D NULL) {=C2=A0 =C2=A0 =C2=A0 =C2=A0 if (dev->pending_deleted_event &&<= br>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (dev->pending_deleted_expir= es_ms =3D=3D 0 ||
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0dev-&g= t;pending_deleted_expires_ms > qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL))) {=
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 error_setg(errp, "Device= %s is already in the "
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0"process= of unplug", id);
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return;=
=C2=A0 =C2=A0 =C2=A0 =C2=A0 }

=C2=A0 =C2=A0 =C2=A0 =C2=A0 qdev_u= nplug(dev, errp);
=C2=A0 =C2=A0 }
}

In QEMU code, there are 6 = lines where this flag is changed:

hw/core/qdev.c:564: =C2=A0 =C2=A0 = =C2=A0 =C2=A0dev->pending_deleted_event =3D false;
hw/core/qdev.c:601= : =C2=A0 =C2=A0 =C2=A0 =C2=A0dev->pending_deleted_event =3D true;
hw/= acpi/pcihp.c:219: =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0qdev->pending_deleted_event =3D false;
hw/acpi/pcihp.c:3= 59: =C2=A0 =C2=A0pdev->qdev.pending_deleted_event =3D true;
hw/pci/pc= ie.c:516: =C2=A0 =C2=A0 =C2=A0 =C2=A0dev->qdev.pending_deleted_event =3D= false;
hw/pci/pcie.c:573: =C2=A0 =C2=A0dev->pending_deleted_event = =3D true;

Considering the complexity of the code, the logic for sett= ing and clearing this flag
seems not quite straightforward. I doubt tha= t the setting of pending_deleted_event in
acpi_pcihp_device_unplug_requ= est_cb() is the appropriate approach to accomplish its target.


On Tue, Apr 4, 2023 at 2:25=E2=80=AFPM Igor M= ammedov <imamme= do@redhat.com> wrote:
On Tue, 4 Apr 2023 08:45:54 +0200
Jinpu Wang <ji= npu.wang@ionos.com> wrote:

> Hi Yu,
>
> On Mon, Apr 3, 2023 at 6:59=E2=80=AFPM Yu Zhang <yu.zhang@ionos.com> wrote:
> >
> > Dear Laurent,
> >
> > Thank you for your quick reply. We used qemu-7.1, but it is repro= ducible with qemu from v6.2 to the recent v8.0 release candidates.
> > I found that it's introduced by the commit=C2=A0 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():
> >
> >=C2=A0 =C2=A0 =C2=A0pdev->qdev.pending_deleted_event =3D true;<= br> > >
> > 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=C2=A0
> This patch from Igor Mammedov seems relevant,
> https://lo= re.kernel.org/qemu-devel/20230403131833-mutt-send-email-mst@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 h= ot-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, t= he issue was also encountered by
> >> > libvirt, but they simply ignored it:
> >> >
> >> > https://bugzilla.redhat.co= m/show_bug.cgi?id=3D1878659
> >> > <https://bugzilla.redhat.= com/show_bug.cgi?id=3D1878659>
see my other reply email/BZ comment 17.

[...]

--0000000000008af65305f884c5f8--