From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by mx.groups.io with SMTP id smtpd.web11.43163.1674127641337396016 for ; Thu, 19 Jan 2023 03:27:21 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=sbz21g3C; spf=pass (domain: kernel.org, ip: 139.178.84.217, mailfrom: ardb@kernel.org) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id 8AC7561B7F for ; Thu, 19 Jan 2023 11:27:20 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 7ED6EC43396 for ; Thu, 19 Jan 2023 11:27:19 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1674127639; bh=G8Vvz/06GQu4eDEwG7HI79nAOlvwgW5PPnq+QMABPmk=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=sbz21g3C3MQqkNP6NoeC15xbIlJzrfdrKlIHbEDUsMxuqWGSRku1grMjYHP+exivJ dsPZYSZz+KPUEFDZp0rrZxmfLMVU/TxFU6xdl46/rhobq2f5VzxDGxmPLDDR6Mn6Im IFO502u7nk1gu22fqQtnP+87o+LsmisO9KGno9CASw0/M0ZiEMFf4nbLGPJeXiwEeB szP89uQ7sWtUQ0HBK1uQdNLu9JLXhYakjOBz04dgr6qI7jy8msMsYqPdSYW6cNv5rI HtITnVMks9apYaEUNMjGWMcpQZfalkHJwwWMbybgDnPUGcoIIPoUnIsYiJsHfJr7IM LVvaWX6ndlBMg== Received: by mail-lj1-f178.google.com with SMTP id z7so1783023ljz.4 for ; Thu, 19 Jan 2023 03:27:19 -0800 (PST) X-Gm-Message-State: AFqh2kp3FQ08f6nCndEm0y9H/GlnitBTGv9bgnTEUvN2R1LZUI06gesC N1Hc5JVue6ndK85b6zDnuGOvr9blZ1Lj1BmZkJc= X-Google-Smtp-Source: AMrXdXtPGZvgZI6vx9msnSSmE5zzztgoQ3zvnFNJeVr2H1AMukl6svGWUHNFV2aApjwo/gC4863gMqBimu9NkCNsaGc= X-Received: by 2002:a2e:b173:0:b0:27f:bc58:3924 with SMTP id a19-20020a2eb173000000b0027fbc583924mr877684ljm.352.1674127637384; Thu, 19 Jan 2023 03:27:17 -0800 (PST) MIME-Version: 1.0 References: <20230119110131.91923-1-lersek@redhat.com> <20230119110131.91923-3-lersek@redhat.com> In-Reply-To: <20230119110131.91923-3-lersek@redhat.com> From: "Ard Biesheuvel" Date: Thu, 19 Jan 2023 12:27:06 +0100 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH v3 2/2] OvmfPkg/PlatformInitLib: catch QEMU's CPU hotplug reg block regression To: Laszlo Ersek Cc: devel@edk2.groups.io, Ard Biesheuvel , Brijesh Singh , Erdem Aktas , Gerd Hoffmann , James Bottomley , Jiewen Yao , Jordan Justen , Michael Brown , Min Xu , Oliver Steffen , Sebastien Boeuf , Tom Lendacky Content-Type: text/plain; charset="UTF-8" On Thu, 19 Jan 2023 at 12:01, Laszlo Ersek wrote: > > In QEMU v5.1.0, the CPU hotplug register block misbehaves: the negotiation > protocol is (effectively) broken such that it suggests that switching from > the legacy interface to the modern interface works, but in reality the > switch never happens. The symptom has been witnessed when using TCG > acceleration; KVM seems to mask the issue. The issue persists with the > following (latest) stable QEMU releases: v5.2.0, v6.2.0, v7.2.0. Currently > there is no stable release that addresses the problem. > > The QEMU bug confuses the Present and Possible counting in function > PlatformMaxCpuCountInitialization(), in > "OvmfPkg/Library/PlatformInitLib/Platform.c". OVMF ends up with Present=0 > Possible=1. This in turn further confuses MpInitLib in UefiCpuPkg (hence > firmware-time multiprocessing will be broken). Worse, CPU hot(un)plug with > SMI will be summarily broken in OvmfPkg/CpuHotplugSmm, which (considering > the privilege level of SMM) is not that great. > > Detect the issue in PlatformCpuCountBugCheck(), and print an error message > and *hang* if the issue is present. > > Users willing to take risks can override the hang with the experimental > QEMU command line option > > -fw_cfg name=opt/org.tianocore/X-Cpuhp-Bugcheck-Override,string=yes > > (The "-fw_cfg" QEMU option itself is not experimental; its above argument, > as far it concerns the firmware, is experimental.) > > The problem was originally reported by Ard [0]. We analyzed it at [1] and > [2]. A QEMU patch was sent at [3]; now merged as commit dab30fbef389 > ("acpi: cpuhp: fix guest-visible maximum access size to the legacy reg > block", 2023-01-08), to be included in QEMU v8.0.0. > > [0] https://bugzilla.tianocore.org/show_bug.cgi?id=4234#c2 > > [1] https://bugzilla.tianocore.org/show_bug.cgi?id=4234#c3 > > [2] IO port write width clamping differs between TCG and KVM > http://mid.mail-archive.com/aaedee84-d3ed-a4f9-21e7-d221a28d1683@redhat.com > https://lists.gnu.org/archive/html/qemu-devel/2023-01/msg00199.html > > [3] acpi: cpuhp: fix guest-visible maximum access size to the legacy reg block > http://mid.mail-archive.com/20230104090138.214862-1-lersek@redhat.com > https://lists.gnu.org/archive/html/qemu-devel/2023-01/msg00278.html > > NOTE: PlatformInitLib is used in the following platform DSCs: > > OvmfPkg/AmdSev/AmdSevX64.dsc > OvmfPkg/CloudHv/CloudHvX64.dsc > OvmfPkg/IntelTdx/IntelTdxX64.dsc > OvmfPkg/Microvm/MicrovmX64.dsc > OvmfPkg/OvmfPkgIa32.dsc > OvmfPkg/OvmfPkgIa32X64.dsc > OvmfPkg/OvmfPkgX64.dsc > > but I can only test this change with the last three platforms, running on > QEMU. > > Test results: > > TCG QEMU OVMF override result > patched patched > --- ------- ------- -------- -------------------------------------- > 0 0 0 0 CPU counts OK (KVM masks the QEMU bug) > 0 0 1 0 CPU counts OK (KVM masks the QEMU bug) > 0 1 0 0 CPU counts OK (QEMU fix, but KVM masks > the QEMU bug anyway) > 0 1 1 0 CPU counts OK (QEMU fix, but KVM masks > the QEMU bug anyway) > 1 0 0 0 boot with broken CPU counts (original > QEMU bug) > 1 0 1 0 broken CPU count caught (boot hangs) > 1 0 1 1 broken CPU count caught, bug check > overridden, boot continues > 1 1 0 0 CPU counts OK (QEMU fix) > 1 1 1 0 CPU counts OK (QEMU fix) > > Cc: Ard Biesheuvel > Cc: Brijesh Singh > Cc: Erdem Aktas > Cc: Gerd Hoffmann > Cc: James Bottomley > Cc: Jiewen Yao > Cc: Jordan Justen > Cc: Michael Brown > Cc: Min Xu > Cc: Oliver Steffen > Cc: Sebastien Boeuf > Cc: Tom Lendacky > Bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=4250 > Signed-off-by: Laszlo Ersek Thanks a lot for taking the time and investing the effort. I'm quite happy that we have this 'escape hatch' now, which we could arguably use temporarily in the VS2019 platform CI until its QEMU binary gets updated, right? For the series, Reviewed-by: Ard Biesheuvel > --- > > Notes: > v3: > > - reimplement the bug check in the factored out function > PlatformCpuCountBugCheck() > > - add override [Ard, Gerd, Michael] > > - assert "0 < Present == BootCpuCount <= Possible" whenever > PlatformCpuCountBugCheck() returns > > - update commit message > > - update test matrix in commit message > > - (re)test all "OVMF patched = 1" cases (sigh) > > v2: > > - V1 was at > . > > - Repo: , branch: > cpuhp-reg-catch-4250-v2 > > - Remove KVM as a proposed workaround from the error message, because in > the QEMU discussion, we had found that the KVM accelerator's behavior > in QEMU (masking the problem) was not right, and that a fix for that > had been in progress for quite some time. > > - Add the QEMU commit hash to the commit message, the code comment, and > the error message. > > - Pick up Gerd's R-b; add Oliver to the Cc list. > > OvmfPkg/Library/PlatformInitLib/Platform.c | 87 ++++++++++++++++++++ > 1 file changed, 87 insertions(+) > > diff --git a/OvmfPkg/Library/PlatformInitLib/Platform.c b/OvmfPkg/Library/PlatformInitLib/Platform.c > index d1be5c2d7970..9fee6e481038 100644 > --- a/OvmfPkg/Library/PlatformInitLib/Platform.c > +++ b/OvmfPkg/Library/PlatformInitLib/Platform.c > @@ -36,6 +36,9 @@ > > #include > > +#define CPUHP_BUGCHECK_OVERRIDE_FWCFG_FILE \ > + "opt/org.tianocore/X-Cpuhp-Bugcheck-Override" > + > VOID > EFIAPI > PlatformAddIoMemoryBaseSizeHob ( > @@ -437,6 +440,87 @@ PlatformCpuCountBugCheck ( > { > ASSERT (*BootCpuCount > 0); > > + // > + // Sanity check: we need at least 1 present CPU (CPU#0 is always present). > + // > + // The legacy-to-modern switching of the CPU hotplug register block got broken > + // (for TCG) in QEMU v5.1.0. Refer to "IO port write width clamping differs > + // between TCG and KVM" at > + // > + // or at > + // . > + // > + // QEMU received the fix in commit dab30fbef389 ("acpi: cpuhp: fix > + // guest-visible maximum access size to the legacy reg block", 2023-01-08), to > + // be included in QEMU v8.0.0. > + // > + // If we're affected by this QEMU bug, then we must not continue: it confuses > + // the multiprocessing in UefiCpuPkg/Library/MpInitLib, and breaks CPU > + // hot(un)plug with SMI in OvmfPkg/CpuHotplugSmm. > + // > + if (*Present == 0) { > + UINTN Idx; > + STATIC CONST CHAR8 *CONST Message[] = { > + "Broken CPU hotplug register block found. Update QEMU to version 8+, or", > + "to a stable release with commit dab30fbef389 backported. Refer to", > + ".", > + "Consequences of the QEMU bug may include, but are not limited to:", > + "- all firmware logic, dependent on the CPU hotplug register block,", > + " being confused, for example, multiprocessing-related logic;", > + "- guest OS data loss, including filesystem corruption, due to crash or", > + " hang during ACPI S3 resume;", > + "- SMM privilege escalation, by a malicious guest OS or 3rd partty UEFI", > + " agent, against the platform firmware.", > + "These symptoms need not necessarily be limited to the QEMU user", > + "attempting to hot(un)plug a CPU.", > + "The firmware will now stop (hang) deliberately, in order to prevent the", > + "above symptoms.", > + "You can forcibly override the hang, *at your own risk*, with the", > + "following *experimental* QEMU command line option:", > + " -fw_cfg name=" CPUHP_BUGCHECK_OVERRIDE_FWCFG_FILE ",string=yes", > + "Please only report such bugs that you can reproduce *without* the", > + "override.", > + }; > + RETURN_STATUS ParseStatus; > + BOOLEAN Override; > + > + DEBUG (( > + DEBUG_ERROR, > + "%a: Present=%u Possible=%u\n", > + __FUNCTION__, > + *Present, > + *Possible > + )); > + for (Idx = 0; Idx < ARRAY_SIZE (Message); ++Idx) { > + DEBUG ((DEBUG_ERROR, "%a: %a\n", __FUNCTION__, Message[Idx])); > + } > + > + ParseStatus = QemuFwCfgParseBool ( > + CPUHP_BUGCHECK_OVERRIDE_FWCFG_FILE, > + &Override > + ); > + if (!RETURN_ERROR (ParseStatus) && Override) { > + DEBUG (( > + DEBUG_WARN, > + "%a: \"%a\" active. You've been warned.\n", > + __FUNCTION__, > + CPUHP_BUGCHECK_OVERRIDE_FWCFG_FILE > + )); > + // > + // The bug is in QEMU v5.1.0+, where we're not affected by the QEMU v2.7 > + // reset bug, so BootCpuCount from fw_cfg is reliable. Assume a fully > + // populated topology, like when the modern CPU hotplug interface is > + // unavailable. > + // > + *Present = *BootCpuCount; > + *Possible = *BootCpuCount; > + return; > + } > + > + ASSERT (FALSE); > + CpuDeadLoop (); > + } > + > // > // Sanity check: fw_cfg and the modern CPU hotplug interface should expose the > // same boot CPU count. > @@ -596,6 +680,9 @@ PlatformMaxCpuCountInitialization ( > } while (Selected > 0); > > PlatformCpuCountBugCheck (&BootCpuCount, &Present, &Possible); > + ASSERT (Present > 0); > + ASSERT (Present <= Possible); > + ASSERT (BootCpuCount == Present); > > MaxCpuCount = Possible; > }