On Wed, 26 Feb 2020 at 23:12, Laszlo Ersek <ler...@redhat.com> wrote:
>
> 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
> <https://github.com/tianocore/tianocore.github.io/wiki/Testing-SMM-with-QEMU,-KVM-and-libvirt#uefi-variable-access-test>.)
>
> Cc: Ard Biesheuvel <ard.biesheu...@linaro.org>
> Cc: Igor Mammedov <imamm...@redhat.com>
> Cc: Jiewen Yao <jiewen....@intel.com>
> Cc: Jordan Justen <jordan.l.jus...@intel.com>
> Cc: Michael Kinney <michael.d.kin...@intel.com>
> Cc: Philippe Mathieu-Daudé <phi...@redhat.com>
> Suggested-by: Igor Mammedov <imamm...@redhat.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1512#c5
> Signed-off-by: Laszlo Ersek <ler...@redhat.com>
> Acked-by: Ard Biesheuvel <ard.biesheu...@linaro.org>
Reviewed-by: Ard Biesheuvel <ard.biesheu...@linaro.org>
(I checked the code below versus the intent below, which looks to me
to be in agreement.)
> ---
>
> Notes:
> v2:
>
> - Pick up Ard's Acked-by, which is conditional on approval from Intel
> reviewers on Cc. (I'd like to save Ard the churn of re-acking
> unmodified patches.)
>
> OvmfPkg/OvmfPkgIa32.dsc | 1 -
> OvmfPkg/OvmfPkgIa32X64.dsc | 1 -
> OvmfPkg/OvmfPkgX64.dsc | 1 -
> OvmfPkg/Library/SmmCpuPlatformHookLibQemu/SmmCpuPlatformHookLibQemu.inf | 3
> +++
> OvmfPkg/Library/SmmCpuPlatformHookLibQemu/SmmCpuPlatformHookLibQemu.c | 9
> ++++++++-
> 5 files changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
> index 813995fefad8..60d8af185b9c 100644
> --- a/OvmfPkg/OvmfPkgIa32.dsc
> +++ b/OvmfPkg/OvmfPkgIa32.dsc
> @@ -414,45 +414,44 @@ [LibraryClasses.common.SMM_CORE]
> !endif
> PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf
>
>
> ################################################################################
> #
> # Pcd Section - list of all EDK II PCD Entries defined by this Platform.
> #
>
> ################################################################################
> [PcdsFeatureFlag]
> gEfiMdeModulePkgTokenSpaceGuid.PcdHiiOsRuntimeSupport|FALSE
> gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeUseSerial|FALSE
> gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeUseMemory|TRUE
> gEfiMdeModulePkgTokenSpaceGuid.PcdDxeIplSupportUefiDecompress|FALSE
> gEfiMdeModulePkgTokenSpaceGuid.PcdDxeIplSwitchToLongMode|FALSE
> gEfiMdeModulePkgTokenSpaceGuid.PcdConOutGopSupport|TRUE
> gEfiMdeModulePkgTokenSpaceGuid.PcdConOutUgaSupport|FALSE
> gEfiMdeModulePkgTokenSpaceGuid.PcdInstallAcpiSdtProtocol|TRUE
> !ifdef $(CSM_ENABLE)
> gUefiOvmfPkgTokenSpaceGuid.PcdCsmEnable|TRUE
> !endif
> !if $(SMM_REQUIRE) == TRUE
> gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire|TRUE
> - gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmEnableBspElection|FALSE
> gEfiMdeModulePkgTokenSpaceGuid.PcdEnableVariableRuntimeCache|FALSE
> !endif
>
> [PcdsFixedAtBuild]
> gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeMemorySize|1
> gEfiMdeModulePkgTokenSpaceGuid.PcdResetOnMemoryTypeInformationChange|FALSE
> gEfiMdePkgTokenSpaceGuid.PcdMaximumGuidedExtractHandler|0x10
> !if ($(FD_SIZE_IN_KB) == 1024) || ($(FD_SIZE_IN_KB) == 2048)
> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000
> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x2800
> !if $(NETWORK_TLS_ENABLE) == FALSE
> # match PcdFlashNvStorageVariableSize purely for convenience
> gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0xe000
> !endif
> !endif
> !if $(FD_SIZE_IN_KB) == 4096
> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x8400
> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x8400
> !if $(NETWORK_TLS_ENABLE) == FALSE
> # match PcdFlashNvStorageVariableSize purely for convenience
> gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0x40000
> !endif
> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
> index a256c7084a7e..be6bc7bd88a7 100644
> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
> @@ -419,45 +419,44 @@ [LibraryClasses.common.SMM_CORE]
> !endif
> PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf
>
>
> ################################################################################
> #
> # Pcd Section - list of all EDK II PCD Entries defined by this Platform.
> #
>
> ################################################################################
> [PcdsFeatureFlag]
> gEfiMdeModulePkgTokenSpaceGuid.PcdHiiOsRuntimeSupport|FALSE
> gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeUseSerial|FALSE
> gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeUseMemory|TRUE
> gEfiMdeModulePkgTokenSpaceGuid.PcdDxeIplSupportUefiDecompress|FALSE
> gEfiMdeModulePkgTokenSpaceGuid.PcdDxeIplSwitchToLongMode|TRUE
> gEfiMdeModulePkgTokenSpaceGuid.PcdConOutGopSupport|TRUE
> gEfiMdeModulePkgTokenSpaceGuid.PcdConOutUgaSupport|FALSE
> gEfiMdeModulePkgTokenSpaceGuid.PcdInstallAcpiSdtProtocol|TRUE
> !ifdef $(CSM_ENABLE)
> gUefiOvmfPkgTokenSpaceGuid.PcdCsmEnable|TRUE
> !endif
> !if $(SMM_REQUIRE) == TRUE
> gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire|TRUE
> - gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmEnableBspElection|FALSE
> gEfiMdeModulePkgTokenSpaceGuid.PcdEnableVariableRuntimeCache|FALSE
> !endif
>
> [PcdsFixedAtBuild]
> gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeMemorySize|1
> gEfiMdeModulePkgTokenSpaceGuid.PcdResetOnMemoryTypeInformationChange|FALSE
> gEfiMdePkgTokenSpaceGuid.PcdMaximumGuidedExtractHandler|0x10
> !if ($(FD_SIZE_IN_KB) == 1024) || ($(FD_SIZE_IN_KB) == 2048)
> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000
> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x2800
> !if $(NETWORK_TLS_ENABLE) == FALSE
> # match PcdFlashNvStorageVariableSize purely for convenience
> gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0xe000
> !endif
> !endif
> !if $(FD_SIZE_IN_KB) == 4096
> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x8400
> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x8400
> !if $(NETWORK_TLS_ENABLE) == FALSE
> # match PcdFlashNvStorageVariableSize purely for convenience
> gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0x40000
> !endif
> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> index 78079b9f8e13..e258c474b60d 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -419,45 +419,44 @@ [LibraryClasses.common.SMM_CORE]
> !endif
> PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf
>
>
> ################################################################################
> #
> # Pcd Section - list of all EDK II PCD Entries defined by this Platform.
> #
>
> ################################################################################
> [PcdsFeatureFlag]
> gEfiMdeModulePkgTokenSpaceGuid.PcdHiiOsRuntimeSupport|FALSE
> gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeUseSerial|FALSE
> gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeUseMemory|TRUE
> gEfiMdeModulePkgTokenSpaceGuid.PcdDxeIplSupportUefiDecompress|FALSE
> gEfiMdeModulePkgTokenSpaceGuid.PcdDxeIplSwitchToLongMode|FALSE
> gEfiMdeModulePkgTokenSpaceGuid.PcdConOutGopSupport|TRUE
> gEfiMdeModulePkgTokenSpaceGuid.PcdConOutUgaSupport|FALSE
> gEfiMdeModulePkgTokenSpaceGuid.PcdInstallAcpiSdtProtocol|TRUE
> !ifdef $(CSM_ENABLE)
> gUefiOvmfPkgTokenSpaceGuid.PcdCsmEnable|TRUE
> !endif
> !if $(SMM_REQUIRE) == TRUE
> gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire|TRUE
> - gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmEnableBspElection|FALSE
> gEfiMdeModulePkgTokenSpaceGuid.PcdEnableVariableRuntimeCache|FALSE
> !endif
>
> [PcdsFixedAtBuild]
> gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeMemorySize|1
> gEfiMdeModulePkgTokenSpaceGuid.PcdResetOnMemoryTypeInformationChange|FALSE
> gEfiMdePkgTokenSpaceGuid.PcdMaximumGuidedExtractHandler|0x10
> !if ($(FD_SIZE_IN_KB) == 1024) || ($(FD_SIZE_IN_KB) == 2048)
> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000
> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x2800
> !if $(NETWORK_TLS_ENABLE) == FALSE
> # match PcdFlashNvStorageVariableSize purely for convenience
> gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0xe000
> !endif
> !endif
> !if $(FD_SIZE_IN_KB) == 4096
> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x8400
> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x8400
> !if $(NETWORK_TLS_ENABLE) == FALSE
> # match PcdFlashNvStorageVariableSize purely for convenience
> gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0x40000
> !endif
> diff --git
> a/OvmfPkg/Library/SmmCpuPlatformHookLibQemu/SmmCpuPlatformHookLibQemu.inf
> b/OvmfPkg/Library/SmmCpuPlatformHookLibQemu/SmmCpuPlatformHookLibQemu.inf
> index 82edeca3d12d..413c56fce6e1 100644
> --- a/OvmfPkg/Library/SmmCpuPlatformHookLibQemu/SmmCpuPlatformHookLibQemu.inf
> +++ b/OvmfPkg/Library/SmmCpuPlatformHookLibQemu/SmmCpuPlatformHookLibQemu.inf
> @@ -8,22 +8,25 @@
>
> [Defines]
> INF_VERSION = 1.29
> BASE_NAME = SmmCpuPlatformHookLibQemu
> FILE_GUID = 154D6D26-54B8-45BC-BA3A-CBAA20C02A6A
> MODULE_TYPE = DXE_DRIVER
> VERSION_STRING = 1.0
> LIBRARY_CLASS = SmmCpuPlatformHookLib
>
> #
> # The following information is for reference only and not required by the
> build
> # tools.
> #
> # VALID_ARCHITECTURES = IA32 X64
> #
>
> [Sources]
> SmmCpuPlatformHookLibQemu.c
>
> [Packages]
> MdePkg/MdePkg.dec
> UefiCpuPkg/UefiCpuPkg.dec
> +
> +[LibraryClasses]
> + BaseLib
> diff --git
> a/OvmfPkg/Library/SmmCpuPlatformHookLibQemu/SmmCpuPlatformHookLibQemu.c
> b/OvmfPkg/Library/SmmCpuPlatformHookLibQemu/SmmCpuPlatformHookLibQemu.c
> index 257e1d399cc6..c88a95c6deff 100644
> --- a/OvmfPkg/Library/SmmCpuPlatformHookLibQemu/SmmCpuPlatformHookLibQemu.c
> +++ b/OvmfPkg/Library/SmmCpuPlatformHookLibQemu/SmmCpuPlatformHookLibQemu.c
> @@ -1,31 +1,34 @@
> /** @file
> SMM CPU Platform Hook library instance for QEMU.
>
> Copyright (c) 2020, Red Hat, Inc.
> Copyright (c) 2006 - 2015, Intel Corporation. All rights reserved.<BR>
> SPDX-License-Identifier: BSD-2-Clause-Patent
>
> **/
> +#include <Library/BaseLib.h> // AsmReadMsr64()
> #include <PiSmm.h>
> +#include <Register/Intel/ArchitecturalMsr.h> // MSR_IA32_APIC_BASE_REGISTER
> +
> #include <Library/SmmCpuPlatformHookLib.h>
>
> /**
> Checks if platform produces a valid SMI.
>
> This function checks if platform produces a valid SMI. This function is
> called at SMM entry to detect if this is a spurious SMI. This function
> must be implemented in an MP safe way because it is called by multiple CPU
> threads.
>
> @retval TRUE There is a valid SMI
> @retval FALSE There is no valid SMI
>
> **/
> BOOLEAN
> EFIAPI
> PlatformValidSmi (
> VOID
> )
> {
> return TRUE;
> }
> @@ -56,45 +59,49 @@ ClearTopLevelSmiStatus (
>
> @param IsBsp Output parameter. TRUE: the CPU this function
> executes on is elected to be the SMM BSP. FALSE:
> the CPU this function executes on is to be SMM
> AP.
>
> @retval EFI_SUCCESS The function executes successfully.
> @retval EFI_NOT_READY The function does not determine whether this CPU
> should be BSP or AP. This may occur if hardware
> init sequence to enable the determination is yet
> to
> be done, or the function chooses not to do BSP
> election and will let SMM CPU driver to use its
> default BSP election process.
> @retval EFI_DEVICE_ERROR The function cannot determine whether this CPU
> should be BSP or AP due to hardware error.
>
> **/
> EFI_STATUS
> EFIAPI
> 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;
> }
>
> /**
> Get platform page table attribute.
>
> This function gets page table attribute of platform.
>
> @param Address Input parameter. Obtain the page table entries
> attribute on this address.
> @param PageSize Output parameter. The size of the page.
> @param NumOfPages Output parameter. Number of page.
> @param PageAttribute Output parameter. Paging Attributes (WB, UC, etc).
>
> @retval EFI_SUCCESS The platform page table attribute from the address
> is determined.
> @retval EFI_UNSUPPORTED The platform does not support getting page table
> attribute for the address.
>
> **/
> EFI_STATUS
> EFIAPI
> GetPlatformPageTableAttribute (
> --
> 2.19.1.3.g30247aa5d201
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#55191): https://edk2.groups.io/g/devel/message/55191
Mute This Topic: https://groups.io/mt/71575177/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
