[PATCH 0/1] OvmfPkg/PlatformInitLib: catch QEMU's CPU hotplug reg block regression

[PATCH 0/1] OvmfPkg/PlatformInitLib: catch QEMU's CPU hotplug reg block regression

[Laszlo Ersek]

Repo:       https://pagure.io/lersek/edk2.git
Branch:     cpuhp-reg-catch-4250
Test build: https://github.com/tianocore/edk2/pull/3853
Bugzilla:   https://bugzilla.tianocore.org/show_bug.cgi?id=4250

NOTE: the test build linked above (in the github.com CI env) *failed*.
That's because the CI environment is affected by this very QEMU bug!

Namely, the following checks failed -- due to the intentional hang
introduced in this patch:

- PlatformCI_OvmfPkg_Ubuntu_GCC5_PR (12 tests)
- PlatformCI_OvmfPkg_Windows_VS2019_PR (11 tests)

(The Build_VS2019_TARGET_CODE_COVERAGE and
Build_GCC5_TARGET_CODE_COVERAGE tests also failed, but those seem bogus
to me.)

All 12+11=23 PlatformCI_OvmfPkg_* checks failed due to timeout, and the
firmware logs of the DEBUG and NOOPT builds end with:

> INFO - PlatformMaxCpuCountInitialization: Broken CPU hotplug register block: Present=0 Possible=1.
> INFO - PlatformMaxCpuCountInitialization: Switch QEMU's acceleration from TCG to KVM, or update QEMU.
> INFO - PlatformMaxCpuCountInitialization: Refer to <https://bugzilla.tianocore.org/show_bug.cgi?id=4250>.
> INFO - ASSERT /home/vsts/work/1/s/OvmfPkg/Library/PlatformInitLib/Platform.c(574): ((BOOLEAN)(0==1))

So I'm not proposing to merge this immediately; something must be fixed
first:

- use KVM in the CI env, or

- delay this patch until my QEMU fix is merged, and a new release is
  made, and the new QEMU release is packaged by the distros, and the new
  distro packages are picked up by github.com CI.

Suggestions?

Thanks,
Laszlo

Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Sebastien Boeuf <sebastien.boeuf@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>

Laszlo Ersek (1):
  OvmfPkg/PlatformInitLib: catch QEMU's CPU hotplug reg block regression

 OvmfPkg/Library/PlatformInitLib/Platform.c | 34 ++++++++++++++++++++
 1 file changed, 34 insertions(+)

[PATCH 1/1] OvmfPkg/PlatformInitLib: catch QEMU's CPU hotplug reg block regression

[Laszlo Ersek]

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 PlatformMaxCpuCountInitialization(), and print an
error message and *hang* if the issue is present.

The problem was originally reported by Ard [0]. We analyzed it at [1] and
[2]. A QEMU patch was sent at [3].

[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     result
       patched  patched
  ---  -------  -------  -------------------------------------------------
  0    0        0        CPU counts OK (KVM masks the QEMU bug)
  0    0        1        CPU counts OK (KVM masks the QEMU bug)
  0    1        0        CPU counts OK (QEMU fix, but KVM masks the QEMU
                         bug anyway)
  0    1        1        CPU counts OK (QEMU fix, but KVM masks the QEMU
                         bug anyway)
  1    0        0        boot with broken CPU counts (original QEMU bug)
  1    0        1        broken CPU count caught (boot hangs)
  1    1        0        CPU counts OK (QEMU fix)
  1    1        1        CPU counts OK (QEMU fix)

Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Sebastien Boeuf <sebastien.boeuf@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=4250
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/Library/PlatformInitLib/Platform.c | 34 ++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/OvmfPkg/Library/PlatformInitLib/Platform.c b/OvmfPkg/Library/PlatformInitLib/Platform.c
index 3e13c5d4b34f..034282df5aab 100644
--- a/OvmfPkg/Library/PlatformInitLib/Platform.c
+++ b/OvmfPkg/Library/PlatformInitLib/Platform.c
@@ -541,6 +541,40 @@ PlatformMaxCpuCountInitialization (
         ASSERT (Selected == Possible || Selected == 0);
       } while (Selected > 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
+      // <http://mid.mail-archive.com/aaedee84-d3ed-a4f9-21e7-d221a28d1683@redhat.com>
+      // or at
+      // <https://lists.gnu.org/archive/html/qemu-devel/2023-01/msg00199.html>.
+      //
+      // A fix was submitted after QEMU v7.2.0: "[PATCH] acpi: cpuhp: fix
+      // guest-visible maximum access size to the legacy reg block".
+      //
+      // 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) {
+        DEBUG ((
+          DEBUG_ERROR,
+          "%a: Broken CPU hotplug register block: Present=%u Possible=%u.\n"
+          "%a: Switch QEMU's acceleration from TCG to KVM, or update QEMU.\n"
+          "%a: Refer to "
+          "<https://bugzilla.tianocore.org/show_bug.cgi?id=4250>.\n",
+          __FUNCTION__,
+          Present,
+          Possible,
+          __FUNCTION__,
+          __FUNCTION__
+          ));
+        ASSERT (FALSE);
+        CpuDeadLoop ();
+      }
+
       //
       // Sanity check: fw_cfg and the modern CPU hotplug interface should
       // return the same boot CPU count.

Re: [PATCH 1/1] OvmfPkg/PlatformInitLib: catch QEMU's CPU hotplug reg block regression

[Gerd Hoffmann]

On Wed, Jan 04, 2023 at 04:12:34PM +0100, 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 PlatformMaxCpuCountInitialization(), and print an
> error message and *hang* if the issue is present.
> 
> The problem was originally reported by Ard [0]. We analyzed it at [1] and
> [2]. A QEMU patch was sent at [3].
> 
> [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     result
>        patched  patched
>   ---  -------  -------  -------------------------------------------------
>   0    0        0        CPU counts OK (KVM masks the QEMU bug)
>   0    0        1        CPU counts OK (KVM masks the QEMU bug)
>   0    1        0        CPU counts OK (QEMU fix, but KVM masks the QEMU
>                          bug anyway)
>   0    1        1        CPU counts OK (QEMU fix, but KVM masks the QEMU
>                          bug anyway)
>   1    0        0        boot with broken CPU counts (original QEMU bug)
>   1    0        1        broken CPU count caught (boot hangs)
>   1    1        0        CPU counts OK (QEMU fix)
>   1    1        1        CPU counts OK (QEMU fix)
> 
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Brijesh Singh <brijesh.singh@amd.com>
> Cc: Erdem Aktas <erdemaktas@google.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: James Bottomley <jejb@linux.ibm.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Min Xu <min.m.xu@intel.com>
> Cc: Sebastien Boeuf <sebastien.boeuf@intel.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=4250
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>

Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>

Re: [edk2-devel] [PATCH 0/1] OvmfPkg/PlatformInitLib: catch QEMU's CPU hotplug reg block regression

[Michael D Kinney]

Hi Laszlo,

Unit test code coverage was enabled just a couple days ago in CI.  Looks like you uncovered a corner case that was missed that should not generate a CI failure.

I have asked Gua to investigate.

Thanks,

Mike

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek
> Sent: Wednesday, January 4, 2023 7:13 AM
> To: devel@edk2.groups.io
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>; Brijesh Singh <brijesh.singh@amd.com>; Aktas, Erdem <erdemaktas@google.com>; Gerd
> Hoffmann <kraxel@redhat.com>; James Bottomley <jejb@linux.ibm.com>; Yao, Jiewen <jiewen.yao@intel.com>; Justen, Jordan L
> <jordan.l.justen@intel.com>; Xu, Min M <min.m.xu@intel.com>; Boeuf, Sebastien <sebastien.boeuf@intel.com>; Tom Lendacky
> <thomas.lendacky@amd.com>
> Subject: [edk2-devel] [PATCH 0/1] OvmfPkg/PlatformInitLib: catch QEMU's CPU hotplug reg block regression
> 
> Repo:       https://pagure.io/lersek/edk2.git
> Branch:     cpuhp-reg-catch-4250
> Test build: https://github.com/tianocore/edk2/pull/3853
> Bugzilla:   https://bugzilla.tianocore.org/show_bug.cgi?id=4250
> 
> NOTE: the test build linked above (in the github.com CI env) *failed*.
> That's because the CI environment is affected by this very QEMU bug!
> 
> Namely, the following checks failed -- due to the intentional hang
> introduced in this patch:
> 
> - PlatformCI_OvmfPkg_Ubuntu_GCC5_PR (12 tests)
> - PlatformCI_OvmfPkg_Windows_VS2019_PR (11 tests)
> 
> (The Build_VS2019_TARGET_CODE_COVERAGE and
> Build_GCC5_TARGET_CODE_COVERAGE tests also failed, but those seem bogus
> to me.)
> 
> All 12+11=23 PlatformCI_OvmfPkg_* checks failed due to timeout, and the
> firmware logs of the DEBUG and NOOPT builds end with:
> 
> > INFO - PlatformMaxCpuCountInitialization: Broken CPU hotplug register block: Present=0 Possible=1.
> > INFO - PlatformMaxCpuCountInitialization: Switch QEMU's acceleration from TCG to KVM, or update QEMU.
> > INFO - PlatformMaxCpuCountInitialization: Refer to <https://bugzilla.tianocore.org/show_bug.cgi?id=4250>.
> > INFO - ASSERT /home/vsts/work/1/s/OvmfPkg/Library/PlatformInitLib/Platform.c(574): ((BOOLEAN)(0==1))
> 
> So I'm not proposing to merge this immediately; something must be fixed
> first:
> 
> - use KVM in the CI env, or
> 
> - delay this patch until my QEMU fix is merged, and a new release is
>   made, and the new QEMU release is packaged by the distros, and the new
>   distro packages are picked up by github.com CI.
> 
> Suggestions?
> 
> Thanks,
> Laszlo
> 
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Brijesh Singh <brijesh.singh@amd.com>
> Cc: Erdem Aktas <erdemaktas@google.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: James Bottomley <jejb@linux.ibm.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Min Xu <min.m.xu@intel.com>
> Cc: Sebastien Boeuf <sebastien.boeuf@intel.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> 
> Laszlo Ersek (1):
>   OvmfPkg/PlatformInitLib: catch QEMU's CPU hotplug reg block regression
> 
>  OvmfPkg/Library/PlatformInitLib/Platform.c | 34 ++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> 
> 
> 
> 

Re: [edk2-devel] [PATCH 0/1] OvmfPkg/PlatformInitLib: catch QEMU's CPU hotplug reg block regression

[Guo, Gua]

[-- Attachment #1: Type: text/plain, Size: 5122 bytes --]

@Kinney, Michael D<mailto:michael.d.kinney@intel.com> I create a PR to fix it. Could you help me for code review and push it to unlock the failure ?
URL: https://github.com/tianocore/edk2/pull/3842/



-----Original Message-----
From: Kinney, Michael D <michael.d.kinney@intel.com>
Sent: Thursday, January 5, 2023 7:35 AM
To: devel@edk2.groups.io; lersek@redhat.com; Guo, Gua <gua.guo@intel.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>; Brijesh Singh <brijesh.singh@amd.com>; Aktas, Erdem <erdemaktas@google.com>; Gerd Hoffmann <kraxel@redhat.com>; James Bottomley <jejb@linux.ibm.com>; Yao, Jiewen <jiewen.yao@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>; Xu, Min M <min.m.xu@intel.com>; Boeuf, Sebastien <sebastien.boeuf@intel.com>; Tom Lendacky <thomas.lendacky@amd.com>; Kinney, Michael D <michael.d.kinney@intel.com>
Subject: RE: [edk2-devel] [PATCH 0/1] OvmfPkg/PlatformInitLib: catch QEMU's CPU hotplug reg block regression



Hi Laszlo,



Unit test code coverage was enabled just a couple days ago in CI.  Looks like you uncovered a corner case that was missed that should not generate a CI failure.



I have asked Gua to investigate.



Thanks,



Mike



> -----Original Message-----

> From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of Laszlo

> Ersek

> Sent: Wednesday, January 4, 2023 7:13 AM

> To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>

> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org<mailto:ardb+tianocore@kernel.org>>; Brijesh Singh

> <brijesh.singh@amd.com<mailto:brijesh.singh@amd.com>>; Aktas, Erdem <erdemaktas@google.com<mailto:erdemaktas@google.com>>; Gerd

> Hoffmann <kraxel@redhat.com<mailto:kraxel@redhat.com>>; James Bottomley <jejb@linux.ibm.com<mailto:jejb@linux.ibm.com>>;

> Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Justen, Jordan L

> <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>>; Xu, Min M <min.m.xu@intel.com<mailto:min.m.xu@intel.com>>; Boeuf,

> Sebastien <sebastien.boeuf@intel.com<mailto:sebastien.boeuf@intel.com>>; Tom Lendacky

> <thomas.lendacky@amd.com<mailto:thomas.lendacky@amd.com>>

> Subject: [edk2-devel] [PATCH 0/1] OvmfPkg/PlatformInitLib: catch

> QEMU's CPU hotplug reg block regression

>

> Repo:       https://pagure.io/lersek/edk2.git

> Branch:     cpuhp-reg-catch-4250

> Test build: https://github.com/tianocore/edk2/pull/3853

> Bugzilla:   https://bugzilla.tianocore.org/show_bug.cgi?id=4250

>

> NOTE: the test build linked above (in the github.com CI env) *failed*.

> That's because the CI environment is affected by this very QEMU bug!

>

> Namely, the following checks failed -- due to the intentional hang

> introduced in this patch:

>

> - PlatformCI_OvmfPkg_Ubuntu_GCC5_PR (12 tests)

> - PlatformCI_OvmfPkg_Windows_VS2019_PR (11 tests)

>

> (The Build_VS2019_TARGET_CODE_COVERAGE and

> Build_GCC5_TARGET_CODE_COVERAGE tests also failed, but those seem

> bogus to me.)

>

> All 12+11=23 PlatformCI_OvmfPkg_* checks failed due to timeout, and

> the firmware logs of the DEBUG and NOOPT builds end with:

>

> > INFO - PlatformMaxCpuCountInitialization: Broken CPU hotplug register block: Present=0 Possible=1.

> > INFO - PlatformMaxCpuCountInitialization: Switch QEMU's acceleration from TCG to KVM, or update QEMU.

> > INFO - PlatformMaxCpuCountInitialization: Refer to <https://bugzilla.tianocore.org/show_bug.cgi?id=4250>.

> > INFO - ASSERT

> > /home/vsts/work/1/s/OvmfPkg/Library/PlatformInitLib/Platform.c(574):

> > ((BOOLEAN)(0==1))

>

> So I'm not proposing to merge this immediately; something must be

> fixed

> first:

>

> - use KVM in the CI env, or

>

> - delay this patch until my QEMU fix is merged, and a new release is

>   made, and the new QEMU release is packaged by the distros, and the new

>   distro packages are picked up by github.com CI.

>

> Suggestions?

>

> Thanks,

> Laszlo

>

> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org<mailto:ardb+tianocore@kernel.org>>

> Cc: Brijesh Singh <brijesh.singh@amd.com<mailto:brijesh.singh@amd.com>>

> Cc: Erdem Aktas <erdemaktas@google.com<mailto:erdemaktas@google.com>>

> Cc: Gerd Hoffmann <kraxel@redhat.com<mailto:kraxel@redhat.com>>

> Cc: James Bottomley <jejb@linux.ibm.com<mailto:jejb@linux.ibm.com>>

> Cc: Jiewen Yao <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>

> Cc: Jordan Justen <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>>

> Cc: Min Xu <min.m.xu@intel.com<mailto:min.m.xu@intel.com>>

> Cc: Sebastien Boeuf <sebastien.boeuf@intel.com<mailto:sebastien.boeuf@intel.com>>

> Cc: Tom Lendacky <thomas.lendacky@amd.com<mailto:thomas.lendacky@amd.com>>

>

> Laszlo Ersek (1):

>   OvmfPkg/PlatformInitLib: catch QEMU's CPU hotplug reg block

> regression

>

>  OvmfPkg/Library/PlatformInitLib/Platform.c | 34 ++++++++++++++++++++

>  1 file changed, 34 insertions(+)

>

>

>

> 

>



[-- Attachment #2: Type: text/html, Size: 13530 bytes --]

Re: [edk2-devel] [PATCH 0/1] OvmfPkg/PlatformInitLib: catch QEMU's CPU hotplug reg block regression

[Laszlo Ersek]

Hi Mike,

On 1/5/23 00:34, Kinney, Michael D wrote:
> Hi Laszlo,
> 
> Unit test code coverage was enabled just a couple days ago in CI.  Looks like you uncovered a corner case that was missed that should not generate a CI failure.
> 
> I have asked Gua to investigate.

the CI env is healthy, as far as I can tell. It's just that this patch
catches a grave bug in QEMU, and refuses continue booting if the bug is
present (intentionally). The issue is that currently there's no released
QEMU version with the fix, so if this patch is merged immediately into
OVMF, then OVMF will hang every test case in CI. (Every test scenario
that boots OVMF one way or another.)

Practically, this patch must be postponed until QEMU can be upgraded in
the CI env -- it turns out though that the latter could be possible much
earlier than I had hoped.

Thanks!
Laszlo



> 
> Thanks,
> 
> Mike
> 
>> -----Original Message-----
>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek
>> Sent: Wednesday, January 4, 2023 7:13 AM
>> To: devel@edk2.groups.io
>> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>; Brijesh Singh <brijesh.singh@amd.com>; Aktas, Erdem <erdemaktas@google.com>; Gerd
>> Hoffmann <kraxel@redhat.com>; James Bottomley <jejb@linux.ibm.com>; Yao, Jiewen <jiewen.yao@intel.com>; Justen, Jordan L
>> <jordan.l.justen@intel.com>; Xu, Min M <min.m.xu@intel.com>; Boeuf, Sebastien <sebastien.boeuf@intel.com>; Tom Lendacky
>> <thomas.lendacky@amd.com>
>> Subject: [edk2-devel] [PATCH 0/1] OvmfPkg/PlatformInitLib: catch QEMU's CPU hotplug reg block regression
>>
>> Repo:       https://pagure.io/lersek/edk2.git
>> Branch:     cpuhp-reg-catch-4250
>> Test build: https://github.com/tianocore/edk2/pull/3853
>> Bugzilla:   https://bugzilla.tianocore.org/show_bug.cgi?id=4250
>>
>> NOTE: the test build linked above (in the github.com CI env) *failed*.
>> That's because the CI environment is affected by this very QEMU bug!
>>
>> Namely, the following checks failed -- due to the intentional hang
>> introduced in this patch:
>>
>> - PlatformCI_OvmfPkg_Ubuntu_GCC5_PR (12 tests)
>> - PlatformCI_OvmfPkg_Windows_VS2019_PR (11 tests)
>>
>> (The Build_VS2019_TARGET_CODE_COVERAGE and
>> Build_GCC5_TARGET_CODE_COVERAGE tests also failed, but those seem bogus
>> to me.)
>>
>> All 12+11=23 PlatformCI_OvmfPkg_* checks failed due to timeout, and the
>> firmware logs of the DEBUG and NOOPT builds end with:
>>
>>> INFO - PlatformMaxCpuCountInitialization: Broken CPU hotplug register block: Present=0 Possible=1.
>>> INFO - PlatformMaxCpuCountInitialization: Switch QEMU's acceleration from TCG to KVM, or update QEMU.
>>> INFO - PlatformMaxCpuCountInitialization: Refer to <https://bugzilla.tianocore.org/show_bug.cgi?id=4250>.
>>> INFO - ASSERT /home/vsts/work/1/s/OvmfPkg/Library/PlatformInitLib/Platform.c(574): ((BOOLEAN)(0==1))
>>
>> So I'm not proposing to merge this immediately; something must be fixed
>> first:
>>
>> - use KVM in the CI env, or
>>
>> - delay this patch until my QEMU fix is merged, and a new release is
>>   made, and the new QEMU release is packaged by the distros, and the new
>>   distro packages are picked up by github.com CI.
>>
>> Suggestions?
>>
>> Thanks,
>> Laszlo
>>
>> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
>> Cc: Brijesh Singh <brijesh.singh@amd.com>
>> Cc: Erdem Aktas <erdemaktas@google.com>
>> Cc: Gerd Hoffmann <kraxel@redhat.com>
>> Cc: James Bottomley <jejb@linux.ibm.com>
>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Min Xu <min.m.xu@intel.com>
>> Cc: Sebastien Boeuf <sebastien.boeuf@intel.com>
>> Cc: Tom Lendacky <thomas.lendacky@amd.com>
>>
>> Laszlo Ersek (1):
>>   OvmfPkg/PlatformInitLib: catch QEMU's CPU hotplug reg block regression
>>
>>  OvmfPkg/Library/PlatformInitLib/Platform.c | 34 ++++++++++++++++++++
>>  1 file changed, 34 insertions(+)
>>
>>
>>
>> 
>>
> 

Re: [edk2-devel] [PATCH 0/1] OvmfPkg/PlatformInitLib: catch QEMU's CPU hotplug reg block regression

[Laszlo Ersek]

ugh, unless you meant the Build_VS2019_TARGET_CODE_COVERAGE and
Build_GCC5_TARGET_CODE_COVERAGE test failures, which I mentioned in
passing -- I agree that those don't look healthy.

Thanks
Laszlo

On 1/5/23 16:15, Laszlo Ersek wrote:
> Hi Mike,
> 
> On 1/5/23 00:34, Kinney, Michael D wrote:
>> Hi Laszlo,
>>
>> Unit test code coverage was enabled just a couple days ago in CI.  Looks like you uncovered a corner case that was missed that should not generate a CI failure.
>>
>> I have asked Gua to investigate.
> 
> the CI env is healthy, as far as I can tell. It's just that this patch
> catches a grave bug in QEMU, and refuses continue booting if the bug is
> present (intentionally). The issue is that currently there's no released
> QEMU version with the fix, so if this patch is merged immediately into
> OVMF, then OVMF will hang every test case in CI. (Every test scenario
> that boots OVMF one way or another.)
> 
> Practically, this patch must be postponed until QEMU can be upgraded in
> the CI env -- it turns out though that the latter could be possible much
> earlier than I had hoped.
> 
> Thanks!
> Laszlo
> 
> 
> 
>>
>> Thanks,
>>
>> Mike
>>
>>> -----Original Message-----
>>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek
>>> Sent: Wednesday, January 4, 2023 7:13 AM
>>> To: devel@edk2.groups.io
>>> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>; Brijesh Singh <brijesh.singh@amd.com>; Aktas, Erdem <erdemaktas@google.com>; Gerd
>>> Hoffmann <kraxel@redhat.com>; James Bottomley <jejb@linux.ibm.com>; Yao, Jiewen <jiewen.yao@intel.com>; Justen, Jordan L
>>> <jordan.l.justen@intel.com>; Xu, Min M <min.m.xu@intel.com>; Boeuf, Sebastien <sebastien.boeuf@intel.com>; Tom Lendacky
>>> <thomas.lendacky@amd.com>
>>> Subject: [edk2-devel] [PATCH 0/1] OvmfPkg/PlatformInitLib: catch QEMU's CPU hotplug reg block regression
>>>
>>> Repo:       https://pagure.io/lersek/edk2.git
>>> Branch:     cpuhp-reg-catch-4250
>>> Test build: https://github.com/tianocore/edk2/pull/3853
>>> Bugzilla:   https://bugzilla.tianocore.org/show_bug.cgi?id=4250
>>>
>>> NOTE: the test build linked above (in the github.com CI env) *failed*.
>>> That's because the CI environment is affected by this very QEMU bug!
>>>
>>> Namely, the following checks failed -- due to the intentional hang
>>> introduced in this patch:
>>>
>>> - PlatformCI_OvmfPkg_Ubuntu_GCC5_PR (12 tests)
>>> - PlatformCI_OvmfPkg_Windows_VS2019_PR (11 tests)
>>>
>>> (The Build_VS2019_TARGET_CODE_COVERAGE and
>>> Build_GCC5_TARGET_CODE_COVERAGE tests also failed, but those seem bogus
>>> to me.)
>>>
>>> All 12+11=23 PlatformCI_OvmfPkg_* checks failed due to timeout, and the
>>> firmware logs of the DEBUG and NOOPT builds end with:
>>>
>>>> INFO - PlatformMaxCpuCountInitialization: Broken CPU hotplug register block: Present=0 Possible=1.
>>>> INFO - PlatformMaxCpuCountInitialization: Switch QEMU's acceleration from TCG to KVM, or update QEMU.
>>>> INFO - PlatformMaxCpuCountInitialization: Refer to <https://bugzilla.tianocore.org/show_bug.cgi?id=4250>.
>>>> INFO - ASSERT /home/vsts/work/1/s/OvmfPkg/Library/PlatformInitLib/Platform.c(574): ((BOOLEAN)(0==1))
>>>
>>> So I'm not proposing to merge this immediately; something must be fixed
>>> first:
>>>
>>> - use KVM in the CI env, or
>>>
>>> - delay this patch until my QEMU fix is merged, and a new release is
>>>   made, and the new QEMU release is packaged by the distros, and the new
>>>   distro packages are picked up by github.com CI.
>>>
>>> Suggestions?
>>>
>>> Thanks,
>>> Laszlo
>>>
>>> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
>>> Cc: Brijesh Singh <brijesh.singh@amd.com>
>>> Cc: Erdem Aktas <erdemaktas@google.com>
>>> Cc: Gerd Hoffmann <kraxel@redhat.com>
>>> Cc: James Bottomley <jejb@linux.ibm.com>
>>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>>> Cc: Min Xu <min.m.xu@intel.com>
>>> Cc: Sebastien Boeuf <sebastien.boeuf@intel.com>
>>> Cc: Tom Lendacky <thomas.lendacky@amd.com>
>>>
>>> Laszlo Ersek (1):
>>>   OvmfPkg/PlatformInitLib: catch QEMU's CPU hotplug reg block regression
>>>
>>>  OvmfPkg/Library/PlatformInitLib/Platform.c | 34 ++++++++++++++++++++
>>>  1 file changed, 34 insertions(+)
>>>
>>>
>>>
>>> 
>>>
>>
> 

Re: [PATCH 0/1] OvmfPkg/PlatformInitLib: catch QEMU's CPU hotplug reg block regression

[Gerd Hoffmann]

  Hi,

> So I'm not proposing to merge this immediately; something must be fixed
> first:
> 
> - use KVM in the CI env, or

Not sure this is an option.  Is nested virtualization supported in azure
pipeline VMs?  If so, is /dev/kvm passed into containers?

> - delay this patch until my QEMU fix is merged, and a new release is
>   made, and the new QEMU release is packaged by the distros, and the new
>   distro packages are picked up by github.com CI.

Well.  There is the pending patch series by Oliver to switch over CI to
use containers, and build using fedora container images.  Putting the
fix on fast track and backport the fix to the fedora packages should be
in the cards, which should get us a working qemu quickly.

That requires updating the fedora containers though.  I think they are
at Fedora 35 still, which is EOL meanwhile, so no more updates ...

Oliver?
CI team?

take care,
  Gerd

Re: [PATCH 0/1] OvmfPkg/PlatformInitLib: catch QEMU's CPU hotplug reg block regression

[Oliver Steffen]

Quoting Gerd Hoffmann (2023-01-05 14:12:22)
>   Hi,
>
> > So I'm not proposing to merge this immediately; something must be fixed
> > first:
> >
> > - use KVM in the CI env, or
>
> Not sure this is an option.  Is nested virtualization supported in azure
> pipeline VMs?  If so, is /dev/kvm passed into containers?

I doubt it, but need to check.

>
> > - delay this patch until my QEMU fix is merged, and a new release is
> >   made, and the new QEMU release is packaged by the distros, and the new
> >   distro packages are picked up by github.com CI.
>
> Well.  There is the pending patch series by Oliver to switch over CI to
> use containers, and build using fedora container images.  Putting the
> fix on fast track and backport the fix to the fedora packages should be
> in the cards, which should get us a working qemu quickly.
>
> That requires updating the fedora containers though.  I think they are
> at Fedora 35 still, which is EOL meanwhile, so no more updates ...

Yes, it is Fedora 35, because we were (are?) not ready for gcc 12 yet.
Currently we are building Qemu is from scratch anyway (instead of
using the Fedora 35 repo) becasue we needed a recent one, see
https://github.com/tianocore/containers/blob/main/Fedora-35/Dockerfile#L75

So if the fixed Qemu is availiabe as source download from download.qemu.org
then this is easy to get into the CI images.

That reminds me to resent the "containerized CI" patches again.

>
> Oliver?
> CI team?
>
> take care,
>   Gerd
>

Oliver

Re: [PATCH 0/1] OvmfPkg/PlatformInitLib: catch QEMU's CPU hotplug reg block regression

[Gerd Hoffmann]

On Thu, Jan 05, 2023 at 02:43:32PM +0100, Oliver Steffen wrote:
> Quoting Gerd Hoffmann (2023-01-05 14:12:22)
> > Not sure this is an option.  Is nested virtualization supported in azure
> > pipeline VMs?  If so, is /dev/kvm passed into containers?
> 
> I doubt it, but need to check.

> > That requires updating the fedora containers though.  I think they are
> > at Fedora 35 still, which is EOL meanwhile, so no more updates ...
> 
> Yes, it is Fedora 35, because we were (are?) not ready for gcc 12 yet.

The gcc12 fix finally landed (129404f6e4395008ac0045e7e627edbba2a1e064),
so yes, we are ready now.

> Currently we are building Qemu is from scratch anyway (instead of
> using the Fedora 35 repo) becasue we needed a recent one, see
> https://github.com/tianocore/containers/blob/main/Fedora-35/Dockerfile#L75

Alternative to self-compiling is using
https://fedoraproject.org/wiki/Virtualization_Preview_Repository
for a more recent qemu version.

take care,
  Gerd

Re: [PATCH 0/1] OvmfPkg/PlatformInitLib: catch QEMU's CPU hotplug reg block regression

[Laszlo Ersek]

On 1/5/23 14:52, Gerd Hoffmann wrote:
> On Thu, Jan 05, 2023 at 02:43:32PM +0100, Oliver Steffen wrote:
>> Quoting Gerd Hoffmann (2023-01-05 14:12:22)
>>> Not sure this is an option.  Is nested virtualization supported in azure
>>> pipeline VMs?  If so, is /dev/kvm passed into containers?
>>
>> I doubt it, but need to check.
> 
>>> That requires updating the fedora containers though.  I think they are
>>> at Fedora 35 still, which is EOL meanwhile, so no more updates ...
>>
>> Yes, it is Fedora 35, because we were (are?) not ready for gcc 12 yet.
> 
> The gcc12 fix finally landed (129404f6e4395008ac0045e7e627edbba2a1e064),
> so yes, we are ready now.
> 
>> Currently we are building Qemu is from scratch anyway (instead of
>> using the Fedora 35 repo) becasue we needed a recent one, see
>> https://github.com/tianocore/containers/blob/main/Fedora-35/Dockerfile#L75
> 
> Alternative to self-compiling is using
> https://fedoraproject.org/wiki/Virtualization_Preview_Repository
> for a more recent qemu version.

Ah, those options all sound great. I didn't expect it was possible to
use self-built (or otherwise "externally sourced") QEMUs in CI!

Thanks!
Laszlo

Re: [PATCH 0/1] OvmfPkg/PlatformInitLib: catch QEMU's CPU hotplug reg block regression

[Oliver Steffen]

Quoting Laszlo Ersek (2023-01-05 16:12:36)
> On 1/5/23 14:52, Gerd Hoffmann wrote:
> > On Thu, Jan 05, 2023 at 02:43:32PM +0100, Oliver Steffen wrote:
> >> Quoting Gerd Hoffmann (2023-01-05 14:12:22)
> >>> Not sure this is an option.  Is nested virtualization supported in azure
> >>> pipeline VMs?  If so, is /dev/kvm passed into containers?
> >>
> >> I doubt it, but need to check.
> >
> >>> That requires updating the fedora containers though.  I think they are
> >>> at Fedora 35 still, which is EOL meanwhile, so no more updates ...
> >>
> >> Yes, it is Fedora 35, because we were (are?) not ready for gcc 12 yet.
> >
> > The gcc12 fix finally landed (129404f6e4395008ac0045e7e627edbba2a1e064),
> > so yes, we are ready now.
> >
> >> Currently we are building Qemu is from scratch anyway (instead of
> >> using the Fedora 35 repo) becasue we needed a recent one, see
> >> https://github.com/tianocore/containers/blob/main/Fedora-35/Dockerfile#L75
> >
> > Alternative to self-compiling is using
> > https://fedoraproject.org/wiki/Virtualization_Preview_Repository
> > for a more recent qemu version.
>
> Ah, those options all sound great. I didn't expect it was possible to
> use self-built (or otherwise "externally sourced") QEMUs in CI!

I would prefer the tar download since it is a rather long lived and
version-stable source. We a trying to pin down the version numbers the
most relevant tools so that they don't "run away"/update without
intent.

I'll also check if we can run the CI with gcc 12.

- Oliver

Re: [edk2-devel] [PATCH 0/1] OvmfPkg/PlatformInitLib: catch QEMU's CPU hotplug reg block regression

[Oliver Steffen]

[-- Attachment #1: Type: text/plain, Size: 1900 bytes --]

On Thu, Jan 5, 2023 at 4:19 PM Oliver Steffen <osteffen@redhat.com> wrote:

> Quoting Laszlo Ersek (2023-01-05 16:12:36)
> > On 1/5/23 14:52, Gerd Hoffmann wrote:
> > > On Thu, Jan 05, 2023 at 02:43:32PM +0100, Oliver Steffen wrote:
> > >> Quoting Gerd Hoffmann (2023-01-05 14:12:22)
> > >>> Not sure this is an option.  Is nested virtualization supported in
> azure
> > >>> pipeline VMs?  If so, is /dev/kvm passed into containers?
> > >>
> > >> I doubt it, but need to check.
> > >
> > >>> That requires updating the fedora containers though.  I think they
> are
> > >>> at Fedora 35 still, which is EOL meanwhile, so no more updates ...
> > >>
> > >> Yes, it is Fedora 35, because we were (are?) not ready for gcc 12 yet.
> > >
> > > The gcc12 fix finally landed
> (129404f6e4395008ac0045e7e627edbba2a1e064),
> > > so yes, we are ready now.
> > >
> > >> Currently we are building Qemu is from scratch anyway (instead of
> > >> using the Fedora 35 repo) becasue we needed a recent one, see
> > >>
> https://github.com/tianocore/containers/blob/main/Fedora-35/Dockerfile#L75
> > >
> > > Alternative to self-compiling is using
> > > https://fedoraproject.org/wiki/Virtualization_Preview_Repository
> > > for a more recent qemu version.
> >
> > Ah, those options all sound great. I didn't expect it was possible to
> > use self-built (or otherwise "externally sourced") QEMUs in CI!
>
> I would prefer the tar download since it is a rather long lived and
> version-stable source. We a trying to pin down the version numbers the
> most relevant tools so that they don't "run away"/update without
> intent.
>
> I'll also check if we can run the CI with gcc 12.
>
Unfortunately, the EDK2 CI as a whole is not ready for gcc 12. RiscV64
builds fail:

  RiscV64/FlushCache.iiii:16: Error: unrecognized opcode `fence.i'

Here we are hitting a downside of the approach of relying on
distro-packages.

-Oliver

[-- Attachment #2: Type: text/html, Size: 3294 bytes --]