From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wr1-f65.google.com (mail-wr1-f65.google.com [209.85.221.65]) by mx.groups.io with SMTP id smtpd.web10.14199.1583155962061229168 for ; Mon, 02 Mar 2020 05:32:42 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@linaro.org header.s=google header.b=l8GHSYc2; spf=pass (domain: linaro.org, ip: 209.85.221.65, mailfrom: ard.biesheuvel@linaro.org) Received: by mail-wr1-f65.google.com with SMTP id e10so11157296wrr.10 for ; Mon, 02 Mar 2020 05:32:41 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=tGu/oM9201bukZeIMyaJuDgJ0VUqJQM/CbAk3ckXTdY=; b=l8GHSYc24Wsyk1MlT1Hw7gaIxJ0gMPJIS3xsdAAcfnj98nbnB06vVpvhS2sWeMwf4c G/MliOFxWNlx8MxypH3FRqp26wTE5NYn0475sHoA500Ej/x9enYKqS7f3FTZaAxCZ6T4 SeA6Xj6tJwdSxUbSQsNAX7RZHBUn2+Q5pY57h34TRhr9Li3cdoPpTAR8r4Os7xe8lBLD R+zpsSHybu2Poo26sqo8HDdw6il/AGWMU5cYcNs2IXGG12lu9x0rxYgebeb0tNeaPGDS Rq8T/wlonYgmulfM+Jq0HUjb9ZLoxdi07jrfDhK3q/g8+Ka99+A6tJxLRCmckHzAbDoH jxNQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=tGu/oM9201bukZeIMyaJuDgJ0VUqJQM/CbAk3ckXTdY=; b=Kk5mMCV6obz595+Z+HIPKv8NbXteOPks5OLMbNsTxw3w0LD692WtYZGl/VMLSM2Jpf 6vxhfncc2WXKRXjh5P/Y2lU6Ywv/KIIuqYfHf55OhtIShXK48X4xCflt5BGDNeF/FdwL Soe+7b20/1dDI6EZbH102YlW9GvWmS8khAq23EIrg3niuvk+VeqSsvzhpUu82w9hsLTj /nilytvp8fFtSdP2oQxLhknhHflz7QESDeRCNDuSa9zZXLm5nXFlJVtmAG6AUCYEvBxs XxkKAGkqFdP55rekMWnADy8mDNw5mYaZlSUlVlp/O3Kg9zARQS+onm/+IWts3oJCRD7d +vZQ== X-Gm-Message-State: APjAAAUtPJGvcEPKZmM1VReFgBxAsR/NaEI5ULlZQxFaR0HVPwYmaxED GCToBnZGXV+TfYMxEr7XnkZaj/U78brzFYrlEtJi6Q== X-Google-Smtp-Source: APXvYqw8In7TCim/WYJaVgE1u1tr0825sEIYxT7Oo5pi8iTQry3gCPnwnCbUc4BKAVgRm8ztVcAMwOwAzvCHDS4Enq0= X-Received: by 2002:adf:f84a:: with SMTP id d10mr22833243wrq.208.1583155960521; Mon, 02 Mar 2020 05:32:40 -0800 (PST) MIME-Version: 1.0 References: <20200226221156.29589-1-lersek@redhat.com> <20200226221156.29589-5-lersek@redhat.com> In-Reply-To: <20200226221156.29589-5-lersek@redhat.com> From: "Ard Biesheuvel" Date: Mon, 2 Mar 2020 14:32:29 +0100 Message-ID: Subject: Re: [PATCH v2 04/16] OvmfPkg: enable SMM Monarch Election in PiSmmCpuDxeSmm To: Laszlo Ersek Cc: edk2-devel-groups-io , Igor Mammedov , Jiewen Yao , Jordan Justen , Michael Kinney , =?UTF-8?Q?Philippe_Mathieu=2DDaud=C3=A9?= Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Wed, 26 Feb 2020 at 23:12, Laszlo Ersek wrote: > > With "PcdCpuSmmEnableBspElection" set to FALSE, PiSmmCpuDxeSmm always > considers the processor with index 0 to be the SMM Monarch (a.k.a. the SM= M > 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 Electio= n > 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. QEM= U > 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 6cb2996cef5= e > ("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 ca= n > continue testing the SMM CPU synchronization by deterministically enterin= g > 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=C3=A9 > Suggested-by: Igor Mammedov > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3D1512#c5 > Signed-off-by: Laszlo Ersek > Acked-by: Ard Biesheuvel Reviewed-by: Ard Biesheuvel (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) =3D=3D TRUE > gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire|TRUE > - gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmEnableBspElection|FALSE > gEfiMdeModulePkgTokenSpaceGuid.PcdEnableVariableRuntimeCache|FALSE > !endif > > [PcdsFixedAtBuild] > gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeMemorySize|1 > gEfiMdeModulePkgTokenSpaceGuid.PcdResetOnMemoryTypeInformationChange|F= ALSE > gEfiMdePkgTokenSpaceGuid.PcdMaximumGuidedExtractHandler|0x10 > !if ($(FD_SIZE_IN_KB) =3D=3D 1024) || ($(FD_SIZE_IN_KB) =3D=3D 2048) > gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000 > gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x2800 > !if $(NETWORK_TLS_ENABLE) =3D=3D FALSE > # match PcdFlashNvStorageVariableSize purely for convenience > gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0xe000 > !endif > !endif > !if $(FD_SIZE_IN_KB) =3D=3D 4096 > gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x8400 > gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x8400 > !if $(NETWORK_TLS_ENABLE) =3D=3D 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) =3D=3D TRUE > gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire|TRUE > - gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmEnableBspElection|FALSE > gEfiMdeModulePkgTokenSpaceGuid.PcdEnableVariableRuntimeCache|FALSE > !endif > > [PcdsFixedAtBuild] > gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeMemorySize|1 > gEfiMdeModulePkgTokenSpaceGuid.PcdResetOnMemoryTypeInformationChange|F= ALSE > gEfiMdePkgTokenSpaceGuid.PcdMaximumGuidedExtractHandler|0x10 > !if ($(FD_SIZE_IN_KB) =3D=3D 1024) || ($(FD_SIZE_IN_KB) =3D=3D 2048) > gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000 > gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x2800 > !if $(NETWORK_TLS_ENABLE) =3D=3D FALSE > # match PcdFlashNvStorageVariableSize purely for convenience > gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0xe000 > !endif > !endif > !if $(FD_SIZE_IN_KB) =3D=3D 4096 > gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x8400 > gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x8400 > !if $(NETWORK_TLS_ENABLE) =3D=3D 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) =3D=3D TRUE > gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire|TRUE > - gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmEnableBspElection|FALSE > gEfiMdeModulePkgTokenSpaceGuid.PcdEnableVariableRuntimeCache|FALSE > !endif > > [PcdsFixedAtBuild] > gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeMemorySize|1 > gEfiMdeModulePkgTokenSpaceGuid.PcdResetOnMemoryTypeInformationChange|F= ALSE > gEfiMdePkgTokenSpaceGuid.PcdMaximumGuidedExtractHandler|0x10 > !if ($(FD_SIZE_IN_KB) =3D=3D 1024) || ($(FD_SIZE_IN_KB) =3D=3D 2048) > gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000 > gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x2800 > !if $(NETWORK_TLS_ENABLE) =3D=3D FALSE > # match PcdFlashNvStorageVariableSize purely for convenience > gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0xe000 > !endif > !endif > !if $(FD_SIZE_IN_KB) =3D=3D 4096 > gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x8400 > gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x8400 > !if $(NETWORK_TLS_ENABLE) =3D=3D FALSE > # match PcdFlashNvStorageVariableSize purely for convenience > gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0x40000 > !endif > diff --git a/OvmfPkg/Library/SmmCpuPlatformHookLibQemu/SmmCpuPlatformHook= LibQemu.inf b/OvmfPkg/Library/SmmCpuPlatformHookLibQemu/SmmCpuPlatformHookL= ibQemu.inf > index 82edeca3d12d..413c56fce6e1 100644 > --- a/OvmfPkg/Library/SmmCpuPlatformHookLibQemu/SmmCpuPlatformHookLibQemu= .inf > +++ b/OvmfPkg/Library/SmmCpuPlatformHookLibQemu/SmmCpuPlatformHookLibQemu= .inf > @@ -8,22 +8,25 @@ > > [Defines] > INF_VERSION =3D 1.29 > BASE_NAME =3D SmmCpuPlatformHookLibQemu > FILE_GUID =3D 154D6D26-54B8-45BC-BA3A-CBAA20C02A6= A > MODULE_TYPE =3D DXE_DRIVER > VERSION_STRING =3D 1.0 > LIBRARY_CLASS =3D SmmCpuPlatformHookLib > > # > # The following information is for reference only and not required by th= e build > # tools. > # > # VALID_ARCHITECTURES =3D IA32 X64 > # > > [Sources] > SmmCpuPlatformHookLibQemu.c > > [Packages] > MdePkg/MdePkg.dec > UefiCpuPkg/UefiCpuPkg.dec > + > +[LibraryClasses] > + BaseLib > diff --git a/OvmfPkg/Library/SmmCpuPlatformHookLibQemu/SmmCpuPlatformHook= LibQemu.c b/OvmfPkg/Library/SmmCpuPlatformHookLibQemu/SmmCpuPlatformHookLib= Qemu.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.
> SPDX-License-Identifier: BSD-2-Clause-Patent > > **/ > +#include // AsmReadMsr64() > #include > +#include // MSR_IA32_APIC_BASE_REGIS= TER > + > #include > > /** > Checks if platform produces a valid SMI. > > This function checks if platform produces a valid SMI. This function i= s > 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 functio= n > executes on is elected to be the SMM BSP. FA= LSE: > the CPU this function executes on is to be S= MM 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 hardw= are > init sequence to enable the determination is= yet to > be done, or the function chooses not to do B= SP > election and will let SMM CPU driver to use = its > default BSP election process. > @retval EFI_DEVICE_ERROR The function cannot determine whether this C= PU > 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 =3D AsmReadMsr64 (MSR_IA32_APIC_BASE); > + *IsBsp =3D (BOOLEAN)(ApicBaseMsr.Bits.BSP =3D=3D 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, et= c). > > @retval EFI_SUCCESS The platform page table attribute from the ad= dress > is determined. > @retval EFI_UNSUPPORTED The platform does not support getting page ta= ble > attribute for the address. > > **/ > EFI_STATUS > EFIAPI > GetPlatformPageTableAttribute ( > -- > 2.19.1.3.g30247aa5d201 > >