From 43df61878d948c6d4f1dcf021ff2b0732d2a1cfd Mon Sep 17 00:00:00 2001 From: Laszlo Ersek Date: Wed, 26 Feb 2020 23:11:44 +0100 Subject: [PATCH] OvmfPkg: enable SMM Monarch Election in PiSmmCpuDxeSmm MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit With "PcdCpuSmmEnableBspElection" set to FALSE, PiSmmCpuDxeSmm always considers the processor with index 0 to be the SMM Monarch (a.k.a. the SMM BSP). The SMM Monarch handles the SMI for real, while the other CPUs wait in their SMM loops. In a subsequent patch, we want to set "PcdCpuHotPlugSupport" to TRUE. For that, PiCpuSmmEntry() [UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c] forces us with an ASSERT() to set "PcdCpuSmmEnableBspElection" to TRUE as well. To satisfy that expectation, we can simply remove our current "PcdCpuSmmEnableBspElection|FALSE" setting, and inherit the default TRUE value from "UefiCpuPkg.dec". This causes "mSmmMpSyncData->BspIndex" in PiSmmCpuDxeSmm to lose its static zero value (standing for CPU#0); instead it becomes (-1) in general, and the SMM Monarch is elected anew on every SMI. The default SMM Monarch Election is basically a race -- whichever CPU can flip "mSmmMpSyncData->BspIndex" from (-1) to its own index, becomes king, for handling that SMI. Refer to SmiRendezvous() [UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c]. I consider this non-determinism less than ideal on QEMU/KVM; it would be nice to stick with a "mostly permanent" SMM Monarch even with the Election enabled. We can do that by implementing the PlatformSmmBspElection() API in the SmmCpuPlatformHookLibQemu instance: The IA32 APIC Base MSR can be read on each CPU concurrently, and it will report the BSP bit as set only on the current Boot Service Processor. QEMU marks CPU#0 as the BSP, by default. Elect the current BSP, as reported by QEMU, for the SMM Monarch role. (Note that the QEMU commit history is not entirely consistent on whether QEMU/KVM may mark a CPU with nonzero index as the BSP: - At tag v4.2.0, "target/i386/cpu.c" has a comment saying "We hard-wire the BSP to the first CPU". This comment goes back to commit 6cb2996cef5e ("x86: Extend validity of bsp_to_cpu", 2010-03-04). - Compare commit 9cb11fd7539b ("target-i386: clear bsp bit when designating bsp", 2015-04-02) though, especially considering KVM. Either way, this OvmfPkg patch is *not* dependent on CPU index 0; it just takes the race on every SMI out of the game.) One benefit of using a "mostly permanent" SMM Monarch / BSP is that we can continue testing the SMM CPU synchronization by deterministically entering the firmware on the BSP, vs. on an AP, from Linux guests: $ time taskset -c 0 efibootmgr $ time taskset -c 1 efibootmgr (See .) Cc: Ard Biesheuvel Cc: Igor Mammedov Cc: Jiewen Yao Cc: Jordan Justen Cc: Michael Kinney Cc: Philippe Mathieu-Daudé Suggested-by: Igor Mammedov Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1512#c5 Signed-off-by: Laszlo Ersek Message-Id: <20200226221156.29589-5-lersek@redhat.com> Reviewed-by: Ard Biesheuvel Tested-by: Boris Ostrovsky --- .../SmmCpuPlatformHookLibQemu.c | 9 ++++++++- .../SmmCpuPlatformHookLibQemu.inf | 3 +++ OvmfPkg/OvmfPkgIa32.dsc | 1 - OvmfPkg/OvmfPkgIa32X64.dsc | 1 - OvmfPkg/OvmfPkgX64.dsc | 1 - 5 files changed, 11 insertions(+), 4 deletions(-) diff --git a/OvmfPkg/Library/SmmCpuPlatformHookLibQemu/SmmCpuPlatformHookLibQemu.c b/OvmfPkg/Library/SmmCpuPlatformHookLibQemu/SmmCpuPlatformHookLibQemu.c index 257e1d399c..c88a95c6de 100644 --- a/OvmfPkg/Library/SmmCpuPlatformHookLibQemu/SmmCpuPlatformHookLibQemu.c +++ b/OvmfPkg/Library/SmmCpuPlatformHookLibQemu/SmmCpuPlatformHookLibQemu.c @@ -6,7 +6,10 @@ Copyright (c) 2006 - 2015, Intel Corporation. All rights reserved.
SPDX-License-Identifier: BSD-2-Clause-Patent **/ +#include // AsmReadMsr64() #include +#include // MSR_IA32_APIC_BASE_REGISTER + #include /** @@ -75,7 +78,11 @@ PlatformSmmBspElection ( OUT BOOLEAN *IsBsp ) { - return EFI_NOT_READY; + MSR_IA32_APIC_BASE_REGISTER ApicBaseMsr; + + ApicBaseMsr.Uint64 = AsmReadMsr64 (MSR_IA32_APIC_BASE); + *IsBsp = (BOOLEAN)(ApicBaseMsr.Bits.BSP == 1); + return EFI_SUCCESS; } /** diff --git a/OvmfPkg/Library/SmmCpuPlatformHookLibQemu/SmmCpuPlatformHookLibQemu.inf b/OvmfPkg/Library/SmmCpuPlatformHookLibQemu/SmmCpuPlatformHookLibQemu.inf index 82edeca3d1..413c56fce6 100644 --- a/OvmfPkg/Library/SmmCpuPlatformHookLibQemu/SmmCpuPlatformHookLibQemu.inf +++ b/OvmfPkg/Library/SmmCpuPlatformHookLibQemu/SmmCpuPlatformHookLibQemu.inf @@ -27,3 +27,6 @@ [Packages] MdePkg/MdePkg.dec UefiCpuPkg/UefiCpuPkg.dec + +[LibraryClasses] + BaseLib diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc index c2d727730c..4788f03e9a 100644 --- a/OvmfPkg/OvmfPkgIa32.dsc +++ b/OvmfPkg/OvmfPkgIa32.dsc @@ -437,7 +437,6 @@ !endif !if $(SMM_REQUIRE) == TRUE gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire|TRUE - gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmEnableBspElection|FALSE gEfiMdeModulePkgTokenSpaceGuid.PcdEnableVariableRuntimeCache|FALSE !endif diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc index 7d862232b2..adbbed8e11 100644 --- a/OvmfPkg/OvmfPkgIa32X64.dsc +++ b/OvmfPkg/OvmfPkgIa32X64.dsc @@ -441,7 +441,6 @@ !endif !if $(SMM_REQUIRE) == TRUE gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire|TRUE - gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmEnableBspElection|FALSE gEfiMdeModulePkgTokenSpaceGuid.PcdEnableVariableRuntimeCache|FALSE !endif diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc index c49703c994..02cbc75a12 100644 --- a/OvmfPkg/OvmfPkgX64.dsc +++ b/OvmfPkg/OvmfPkgX64.dsc @@ -441,7 +441,6 @@ !endif !if $(SMM_REQUIRE) == TRUE gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire|TRUE - gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmEnableBspElection|FALSE gEfiMdeModulePkgTokenSpaceGuid.PcdEnableVariableRuntimeCache|FALSE !endif -- 2.39.2