* [PATCH v3 0/2] OvmfPkg: broadcast SMIs and dynamically revert to traditional AP sync mode
@ 2017-01-31 11:02 Laszlo Ersek
2017-01-31 11:02 ` [PATCH v3 1/2] OvmfPkg: dynamic defaults for PcdCpuSmmApSyncTimeout, PcdCpuSmmSyncMode Laszlo Ersek
2017-01-31 11:02 ` [PATCH v3 2/2] OvmfPkg/SmmControl2Dxe: select broadcast SMI if available Laszlo Ersek
0 siblings, 2 replies; 4+ messages in thread
From: Laszlo Ersek @ 2017-01-31 11:02 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Jordan Justen
Previous version (v2):
<https://lists.01.org/pipermail/edk2-devel/2016-November/005003.html>.
In this version (again), OVMF negotiates the broadcast SMI feature with
QEMU dynamically. If the feature is available, OVMF selects it, and also
re-sets the UefiCpuPkg PCDs that are related to BSP-AP synchronization
to their "UefiCpuPkg.dec" defaults.
The difference relative to v2 is that for the SMI features, QEMU gained
a dedicated fw_cfg-based negotiation interface, replacing the previously
proposed (somewhat ad-hoc) ICH9_APM_STS-based interface. The QEMU
commits implementing and documenting the new interface are listed in
patch #2. (But, I also commented patch #2 heavily.)
Patch for patch:
- v2 1/4 was committed separately (b43dd22981b7,
"UefiCpuPkg/PiSmmCpuDxeSmm: dynamic PcdCpuSmmApSyncTimeout,
PcdCpuSmmSyncMode", 2016-11-17),
- v2 2/4 is now carried forward without changes, as patch #1 (I've
picked up Jordan's R-b),
- the rest of v2 is replaced with patch #2.
I formatted this series with 7 lines of context, to make it an easier
read.
Repo: https://github.com/lersek/edk2/
Branch: broadcast_smi_v3
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=230
Cc: Jordan Justen <jordan.l.justen@intel.com>
Thanks,
Laszlo
Laszlo Ersek (2):
OvmfPkg: dynamic defaults for PcdCpuSmmApSyncTimeout,
PcdCpuSmmSyncMode
OvmfPkg/SmmControl2Dxe: select broadcast SMI if available
OvmfPkg/OvmfPkgIa32.dsc | 7 +-
OvmfPkg/OvmfPkgIa32X64.dsc | 7 +-
OvmfPkg/OvmfPkgX64.dsc | 7 +-
OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.inf | 8 +
OvmfPkg/SmmControl2Dxe/SmiFeatures.h | 49 +++
OvmfPkg/SmmControl2Dxe/SmiFeatures.c | 324 ++++++++++++++++++++
OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c | 21 ++
7 files changed, 417 insertions(+), 6 deletions(-)
create mode 100644 OvmfPkg/SmmControl2Dxe/SmiFeatures.h
create mode 100644 OvmfPkg/SmmControl2Dxe/SmiFeatures.c
--
2.9.3
^ permalink raw reply [flat|nested] 4+ messages in thread* [PATCH v3 1/2] OvmfPkg: dynamic defaults for PcdCpuSmmApSyncTimeout, PcdCpuSmmSyncMode 2017-01-31 11:02 [PATCH v3 0/2] OvmfPkg: broadcast SMIs and dynamically revert to traditional AP sync mode Laszlo Ersek @ 2017-01-31 11:02 ` Laszlo Ersek 2017-01-31 11:02 ` [PATCH v3 2/2] OvmfPkg/SmmControl2Dxe: select broadcast SMI if available Laszlo Ersek 1 sibling, 0 replies; 4+ messages in thread From: Laszlo Ersek @ 2017-01-31 11:02 UTC (permalink / raw) To: edk2-devel-01; +Cc: Jordan Justen Move the platform-specific default values for these PCDs from the [PcdsFixedAtBuild] / [PcdsFixedAtBuild.X64] sections to the [PcdsDynamicDefault] section. Cc: Jordan Justen <jordan.l.justen@intel.com> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=230 Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Laszlo Ersek <lersek@redhat.com> Reviewed-by: Jordan Justen <jordan.l.justen@intel.com> --- Notes: v3: - no changes - pick up Jordan's R-b - trim CC list v2: - new in v2 OvmfPkg/OvmfPkgIa32.dsc | 7 +++++-- OvmfPkg/OvmfPkgIa32X64.dsc | 7 +++++-- OvmfPkg/OvmfPkgX64.dsc | 7 +++++-- 3 files changed, 15 insertions(+), 6 deletions(-) diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc index 43f63cf585cb..993547d4859e 100644 --- a/OvmfPkg/OvmfPkgIa32.dsc +++ b/OvmfPkg/OvmfPkgIa32.dsc @@ -441,16 +441,14 @@ [PcdsFixedAtBuild] !endif !ifndef $(USE_OLD_SHELL) gEfiIntelFrameworkModulePkgTokenSpaceGuid.PcdShellFile|{ 0x83, 0xA5, 0x04, 0x7C, 0x3E, 0x9E, 0x1C, 0x4F, 0xAD, 0x65, 0xE0, 0x52, 0x68, 0xD0, 0xB4, 0xD1 } !endif !if $(SMM_REQUIRE) == TRUE - gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode|0x01 - gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmApSyncTimeout|100000 gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmStackSize|0x4000 !endif !if $(SECURE_BOOT_ENABLE) == TRUE gEfiSecurityPkgTokenSpaceGuid.PcdOptionRomImageVerificationPolicy|0x00 !endif @@ -495,14 +493,19 @@ [PcdsDynamicDefault] gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack|FALSE gEfiMdeModulePkgTokenSpaceGuid.PcdPropertiesTableEnable|FALSE # UefiCpuPkg PCDs related to initial AP bringup and general AP management. gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber|64 gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds|50000 +!if $(SMM_REQUIRE) == TRUE + gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode|0x01 + gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmApSyncTimeout|100000 +!endif + ################################################################################ # # Components Section - list of all EDK II Modules needed by this Platform. # ################################################################################ [Components] OvmfPkg/ResetVector/ResetVector.inf diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc index 2760533f420a..f36604ecb4d8 100644 --- a/OvmfPkg/OvmfPkgIa32X64.dsc +++ b/OvmfPkg/OvmfPkgIa32X64.dsc @@ -447,16 +447,14 @@ [PcdsFixedAtBuild.X64] !endif !ifndef $(USE_OLD_SHELL) gEfiIntelFrameworkModulePkgTokenSpaceGuid.PcdShellFile|{ 0x83, 0xA5, 0x04, 0x7C, 0x3E, 0x9E, 0x1C, 0x4F, 0xAD, 0x65, 0xE0, 0x52, 0x68, 0xD0, 0xB4, 0xD1 } !endif !if $(SMM_REQUIRE) == TRUE - gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode|0x01 - gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmApSyncTimeout|100000 gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmStackSize|0x4000 !endif !if $(SECURE_BOOT_ENABLE) == TRUE gEfiSecurityPkgTokenSpaceGuid.PcdOptionRomImageVerificationPolicy|0x00 !endif @@ -503,14 +501,19 @@ [PcdsDynamicDefault] gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack|FALSE gEfiMdeModulePkgTokenSpaceGuid.PcdPropertiesTableEnable|FALSE # UefiCpuPkg PCDs related to initial AP bringup and general AP management. gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber|64 gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds|50000 +!if $(SMM_REQUIRE) == TRUE + gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode|0x01 + gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmApSyncTimeout|100000 +!endif + ################################################################################ # # Components Section - list of all EDK II Modules needed by this Platform. # ################################################################################ [Components.IA32] OvmfPkg/ResetVector/ResetVector.inf diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc index bd9b56952481..c5bf1a672b1e 100644 --- a/OvmfPkg/OvmfPkgX64.dsc +++ b/OvmfPkg/OvmfPkgX64.dsc @@ -446,16 +446,14 @@ [PcdsFixedAtBuild] !endif !ifndef $(USE_OLD_SHELL) gEfiIntelFrameworkModulePkgTokenSpaceGuid.PcdShellFile|{ 0x83, 0xA5, 0x04, 0x7C, 0x3E, 0x9E, 0x1C, 0x4F, 0xAD, 0x65, 0xE0, 0x52, 0x68, 0xD0, 0xB4, 0xD1 } !endif !if $(SMM_REQUIRE) == TRUE - gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode|0x01 - gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmApSyncTimeout|100000 gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmStackSize|0x4000 !endif !if $(SECURE_BOOT_ENABLE) == TRUE gEfiSecurityPkgTokenSpaceGuid.PcdOptionRomImageVerificationPolicy|0x00 !endif @@ -502,14 +500,19 @@ [PcdsDynamicDefault] gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack|FALSE gEfiMdeModulePkgTokenSpaceGuid.PcdPropertiesTableEnable|FALSE # UefiCpuPkg PCDs related to initial AP bringup and general AP management. gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber|64 gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds|50000 +!if $(SMM_REQUIRE) == TRUE + gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode|0x01 + gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmApSyncTimeout|100000 +!endif + ################################################################################ # # Components Section - list of all EDK II Modules needed by this Platform. # ################################################################################ [Components] OvmfPkg/ResetVector/ResetVector.inf -- 2.9.3 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH v3 2/2] OvmfPkg/SmmControl2Dxe: select broadcast SMI if available 2017-01-31 11:02 [PATCH v3 0/2] OvmfPkg: broadcast SMIs and dynamically revert to traditional AP sync mode Laszlo Ersek 2017-01-31 11:02 ` [PATCH v3 1/2] OvmfPkg: dynamic defaults for PcdCpuSmmApSyncTimeout, PcdCpuSmmSyncMode Laszlo Ersek @ 2017-01-31 11:02 ` Laszlo Ersek [not found] ` <148646137918.13729.17315316362028292158@jljusten-ivb> 1 sibling, 1 reply; 4+ messages in thread From: Laszlo Ersek @ 2017-01-31 11:02 UTC (permalink / raw) To: edk2-devel-01; +Cc: Jordan Justen When writing to IO port 0xB2 (ICH9_APM_CNT), QEMU by default injects an SMI only on the VCPU that is writing the port. This has exposed corner cases and strange behavior with edk2 code, which generally expects a software SMI to affect all CPUs at once. We've experienced instability despite the fact that OVMF sets PcdCpuSmmApSyncTimeout and PcdCpuSmmSyncMode differently from the UefiCpuPkg defaults, such that they match QEMU's unicast SMIs better. (Refer to edk2 commits 9b1e378811ff and bb0f18b0bce6.) Using the new fw_cfg-based SMI feature negotiation in QEMU (see commits 50de920b372b "hw/isa/lpc_ich9: add SMI feature negotiation via fw_cfg" and 5ce45c7a2b15 "hw/isa/lpc_ich9: add broadcast SMI feature"), we can ask QEMU to broadcast SMIs. Extensive testing from earlier proves that broadcast SMIs are only reliable if we use the UefiCpuPkg defaults for the above PCDs. With those settings however, the broadcast is very reliable -- the most reliable configuration encountered thus far. Therefore negotiate broadcast SMIs with QEMU, and if the negotiation is successful, dynamically revert the PCDs to the UefiCpuPkg defaults. Setting the PCDs in this module is safe: - only PiSmmCpuDxeSmm consumes them, - PiSmmCpuDxeSmm is a DXE_SMM_DRIVER, launched by the SMM_CORE (MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf), - the SMM_CORE is launched by the SMM IPL runtime DXE driver (MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf), - the SMM IPL has a DEPEX on EFI_SMM_CONTROL2_PROTOCOL, - OvmfPkg/SmmControl2Dxe produces that protocol. The end result is that PiSmmCpuDxeSmm cannot be dispatched before SmmControl2Dxe installs EFI_SMM_CONTROL2_PROTOCOL and returns from its entry point. Hence we can set the PCD's consumed by PiSmmCpuDxeSmm in SmmControl2Dxe. Cc: Jordan Justen <jordan.l.justen@intel.com> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=230 Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- Notes: v3: - replace the ICH9_APM_STS-based feature negotiation with the new writeable fw_cfg kind OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.inf | 8 + OvmfPkg/SmmControl2Dxe/SmiFeatures.h | 49 +++ OvmfPkg/SmmControl2Dxe/SmiFeatures.c | 324 ++++++++++++++++++++ OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c | 21 ++ 4 files changed, 402 insertions(+) diff --git a/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.inf b/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.inf index 0e9f98c2871c..31c80bd4448c 100644 --- a/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.inf +++ b/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.inf @@ -35,32 +35,40 @@ [Defines] # # The following information is for reference only and not required by the build tools. # # VALID_ARCHITECTURES = IA32 X64 # [Sources] + SmiFeatures.h + SmiFeatures.c SmmControl2Dxe.c [Packages] MdePkg/MdePkg.dec OvmfPkg/OvmfPkg.dec + UefiCpuPkg/UefiCpuPkg.dec [LibraryClasses] BaseLib DebugLib IoLib + MemoryAllocationLib PcdLib PciLib QemuFwCfgLib UefiBootServicesTableLib UefiDriverEntryPoint [Protocols] gEfiS3SaveStateProtocolGuid ## SOMETIMES_CONSUMES gEfiSmmControl2ProtocolGuid ## PRODUCES +[Pcd] + gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmApSyncTimeout ## SOMETIMES_PRODUCES + gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode ## SOMETIMES_PRODUCES + [FeaturePcd] gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire [Depex] TRUE diff --git a/OvmfPkg/SmmControl2Dxe/SmiFeatures.h b/OvmfPkg/SmmControl2Dxe/SmiFeatures.h new file mode 100644 index 000000000000..9d5f1dbcb57e --- /dev/null +++ b/OvmfPkg/SmmControl2Dxe/SmiFeatures.h @@ -0,0 +1,49 @@ +/**@file + Negotiate SMI features with QEMU, and configure UefiCpuPkg/PiSmmCpuDxeSmm + accordingly. + + Copyright (C) 2016-2017, Red Hat, Inc. + + This program and the accompanying materials are licensed and made available + under the terms and conditions of the BSD License which accompanies this + distribution. The full text of the license may be found at + http://opensource.org/licenses/bsd-license.php + + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, WITHOUT + WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. +**/ + +#ifndef __SMI_FEATURES_H__ +#define __SMI_FEATURES_H__ + +#include <Protocol/S3SaveState.h> + +/** + Negotiate SMI features with QEMU. + + @retval FALSE If SMI feature negotiation is not supported by QEMU. This is + not an error, it just means that SaveSmiFeatures() should not + be called. + + @retval TRUE SMI feature negotiation is supported, and it has completed + successfully as well. (Failure to negotiate is a fatal error + and the function never returns in that case.) +**/ +BOOLEAN +NegotiateSmiFeatures ( + VOID + ); + +/** + Append a boot script fragment that will re-select the previously negotiated + SMI features during S3 resume. + + @param[in] S3SaveState The EFI_S3_SAVE_STATE_PROTOCOL instance to append to + the S3 boot script with. +**/ +VOID +SaveSmiFeatures ( + IN EFI_S3_SAVE_STATE_PROTOCOL *S3SaveState + ); + +#endif diff --git a/OvmfPkg/SmmControl2Dxe/SmiFeatures.c b/OvmfPkg/SmmControl2Dxe/SmiFeatures.c new file mode 100644 index 000000000000..e070969065c0 --- /dev/null +++ b/OvmfPkg/SmmControl2Dxe/SmiFeatures.c @@ -0,0 +1,324 @@ +/**@file + Negotiate SMI features with QEMU, and configure UefiCpuPkg/PiSmmCpuDxeSmm + accordingly. + + Copyright (C) 2016-2017, Red Hat, Inc. + + This program and the accompanying materials are licensed and made available + under the terms and conditions of the BSD License which accompanies this + distribution. The full text of the license may be found at + http://opensource.org/licenses/bsd-license.php + + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, WITHOUT + WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. +**/ + +#include <Library/BaseLib.h> +#include <Library/DebugLib.h> +#include <Library/MemoryAllocationLib.h> +#include <Library/PcdLib.h> +#include <Library/QemuFwCfgLib.h> + +#include "SmiFeatures.h" + +// +// The following bit value stands for "broadcast SMI" in the +// "etc/smi/supported-features" and "etc/smi/requested-features" fw_cfg files. +// +#define ICH9_LPC_SMI_F_BROADCAST BIT0 + +// +// Provides a scratch buffer (allocated in EfiReservedMemoryType type memory) +// for the S3 boot script fragment to write to and read from. The buffer +// captures a combined fw_cfg item selection + write command using the DMA +// access method. Note that we don't trust the runtime OS to preserve the +// contents of the buffer, the boot script will first rewrite it. +// +#pragma pack (1) +typedef struct { + FW_CFG_DMA_ACCESS Access; + UINT64 Features; +} SCRATCH_BUFFER; +#pragma pack () + +// +// These carry the selector keys of the "etc/smi/requested-features" and +// "etc/smi/features-ok" fw_cfg files from NegotiateSmiFeatures() to +// SaveSmiFeatures(). +// +STATIC FIRMWARE_CONFIG_ITEM mRequestedFeaturesItem; +STATIC FIRMWARE_CONFIG_ITEM mFeaturesOkItem; + +// +// Carries the negotiated SMI features from NegotiateSmiFeatures() to +// SaveSmiFeatures(). +// +STATIC UINT64 mSmiFeatures; + +/** + Negotiate SMI features with QEMU. + + @retval FALSE If SMI feature negotiation is not supported by QEMU. This is + not an error, it just means that SaveSmiFeatures() should not + be called. + + @retval TRUE SMI feature negotiation is supported, and it has completed + successfully as well. (Failure to negotiate is a fatal error + and the function never returns in that case.) +**/ +BOOLEAN +NegotiateSmiFeatures ( + VOID + ) +{ + FIRMWARE_CONFIG_ITEM SupportedFeaturesItem; + UINTN SupportedFeaturesSize; + UINTN RequestedFeaturesSize; + UINTN FeaturesOkSize; + + // + // Look up the fw_cfg files used for feature negotiation. The selector keys + // of "etc/smi/requested-features" and "etc/smi/features-ok" are saved + // statically. If the files are missing, then QEMU doesn't support SMI + // feature negotiation. + // + if (RETURN_ERROR (QemuFwCfgFindFile ("etc/smi/supported-features", + &SupportedFeaturesItem, &SupportedFeaturesSize)) || + RETURN_ERROR (QemuFwCfgFindFile ("etc/smi/requested-features", + &mRequestedFeaturesItem, &RequestedFeaturesSize)) || + RETURN_ERROR (QemuFwCfgFindFile ("etc/smi/features-ok", + &mFeaturesOkItem, &FeaturesOkSize))) { + DEBUG ((DEBUG_INFO, "%a: SMI feature negotiation unavailable\n", + __FUNCTION__)); + return FALSE; + } + + // + // If the files are present but their sizes disagree with us, that's a fatal + // error (we can't trust the behavior of SMIs either way). + // + if (SupportedFeaturesSize != sizeof mSmiFeatures || + RequestedFeaturesSize != sizeof mSmiFeatures || + FeaturesOkSize != sizeof (UINT8)) { + DEBUG ((DEBUG_ERROR, "%a: size mismatch in feature negotiation\n", + __FUNCTION__)); + goto FatalError; + } + + // + // Get the features supported by the host. + // + QemuFwCfgSelectItem (SupportedFeaturesItem); + QemuFwCfgReadBytes (sizeof mSmiFeatures, &mSmiFeatures); + + // + // We want broadcast SMI and nothing else. + // + mSmiFeatures &= ICH9_LPC_SMI_F_BROADCAST; + QemuFwCfgSelectItem (mRequestedFeaturesItem); + QemuFwCfgWriteBytes (sizeof mSmiFeatures, &mSmiFeatures); + + // + // Invoke feature validation in QEMU. If the selection is accepted, the + // features will be locked down. If the selection is rejected, feature + // negotiation remains open; however we don't know what to do in that case, + // so that's a fatal error. + // + QemuFwCfgSelectItem (mFeaturesOkItem); + if (QemuFwCfgRead8 () != 1) { + DEBUG ((DEBUG_ERROR, "%a: negotiation failed for feature bitmap 0x%Lx\n", + __FUNCTION__, mSmiFeatures)); + goto FatalError; + } + + if ((mSmiFeatures & ICH9_LPC_SMI_F_BROADCAST) == 0) { + // + // If we can't get broadcast SMIs from QEMU, that's acceptable too, + // although not optimal. + // + DEBUG ((DEBUG_INFO, "%a: SMI broadcast unavailable\n", __FUNCTION__)); + } else { + // + // Configure the traditional AP sync / SMI delivery mode for + // PiSmmCpuDxeSmm. Effectively, restore the UefiCpuPkg defaults, from which + // the original QEMU behavior (i.e., unicast SMI) used to differ. + // + if (RETURN_ERROR (PcdSet64S (PcdCpuSmmApSyncTimeout, 1000000)) || + RETURN_ERROR (PcdSet8S (PcdCpuSmmSyncMode, 0x00))) { + DEBUG ((DEBUG_ERROR, "%a: PiSmmCpuDxeSmm PCD configuration failed\n", + __FUNCTION__)); + goto FatalError; + } + DEBUG ((DEBUG_INFO, "%a: using SMI broadcast\n", __FUNCTION__)); + } + + // + // Negotiation successful (although we may not have gotten the optimal + // feature set). + // + return TRUE; + +FatalError: + ASSERT (FALSE); + CpuDeadLoop (); + // + // Keep the compiler happy. + // + return FALSE; +} + +/** + Append a boot script fragment that will re-select the previously negotiated + SMI features during S3 resume. + + @param[in] S3SaveState The EFI_S3_SAVE_STATE_PROTOCOL instance to append to + the S3 boot script with. +**/ +VOID +SaveSmiFeatures ( + IN EFI_S3_SAVE_STATE_PROTOCOL *S3SaveState + ) +{ + SCRATCH_BUFFER *ScratchBuffer; + EFI_STATUS Status; + UINT64 AccessAddress; + UINT32 ControlPollData; + UINT32 ControlPollMask; + UINT16 FeaturesOkItemAsUint16; + UINT8 FeaturesOkData; + UINT8 FeaturesOkMask; + + ScratchBuffer = AllocateReservedPool (sizeof *ScratchBuffer); + if (ScratchBuffer == NULL) { + DEBUG ((DEBUG_ERROR, "%a: scratch buffer allocation failed\n", + __FUNCTION__)); + goto FatalError; + } + + // + // Populate the scratch buffer with a select + write fw_cfg DMA command that + // will write the negotiated feature bitmap into + // "etc/smi/requested-features". + // + ScratchBuffer->Access.Control = SwapBytes32 ( + (UINT32)mRequestedFeaturesItem << 16 | + FW_CFG_DMA_CTL_SELECT | + FW_CFG_DMA_CTL_WRITE + ); + ScratchBuffer->Access.Length = SwapBytes32 ( + (UINT32)sizeof ScratchBuffer->Features); + ScratchBuffer->Access.Address = SwapBytes64 ( + (UINTN)&ScratchBuffer->Features); + ScratchBuffer->Features = mSmiFeatures; + + // + // Copy the scratch buffer into the boot script. When replayed, this + // EFI_BOOT_SCRIPT_MEM_WRITE_OPCODE will restore the current contents of the + // scratch buffer, in-place. + // + Status = S3SaveState->Write ( + S3SaveState, // This + EFI_BOOT_SCRIPT_MEM_WRITE_OPCODE, // OpCode + EfiBootScriptWidthUint8, // Width + (UINT64)(UINTN)ScratchBuffer, // Address + sizeof *ScratchBuffer, // Count + (VOID*)ScratchBuffer // Buffer + ); + if (EFI_ERROR (Status)) { + DEBUG ((DEBUG_ERROR, "%a:%d: EFI_BOOT_SCRIPT_MEM_WRITE_OPCODE: %r\n", + __FUNCTION__, __LINE__, Status)); + goto FatalError; + } + + // + // Append an opcode that will write the address of the scratch buffer to the + // fw_cfg DMA address register, which consists of two 32-bit IO ports. The + // second (highest address, least significant) write will start the transfer. + // + AccessAddress = SwapBytes64 ((UINTN)&ScratchBuffer->Access); + Status = S3SaveState->Write ( + S3SaveState, // This + EFI_BOOT_SCRIPT_IO_WRITE_OPCODE, // OpCode + EfiBootScriptWidthUint32, // Width + (UINT64)0x514, // Address + (UINTN)2, // Count + &AccessAddress // Buffer + ); + if (EFI_ERROR (Status)) { + DEBUG ((DEBUG_ERROR, "%a:%d: EFI_BOOT_SCRIPT_IO_WRITE_OPCODE: %r\n", + __FUNCTION__, __LINE__, Status)); + goto FatalError; + } + + // + // The EFI_BOOT_SCRIPT_MEM_POLL_OPCODE will wait until the Control word reads + // as zero (transfer complete). As timeout we use MAX_UINT64 * 100ns, which + // is approximately 58494 years. + // + ControlPollData = 0; + ControlPollMask = MAX_UINT32; + Status = S3SaveState->Write ( + S3SaveState, // This + EFI_BOOT_SCRIPT_MEM_POLL_OPCODE, // OpCode + EfiBootScriptWidthUint32, // Width + (UINT64)(UINTN)&ScratchBuffer->Access.Control, // Address + &ControlPollData, // Data + &ControlPollMask, // DataMask + MAX_UINT64 // Delay + ); + if (EFI_ERROR (Status)) { + DEBUG ((DEBUG_ERROR, "%a:%d: EFI_BOOT_SCRIPT_MEM_POLL_OPCODE: %r\n", + __FUNCTION__, __LINE__, Status)); + goto FatalError; + } + + // + // Select the "etc/smi/features-ok" fw_cfg file, which invokes the feature + // validation & lockdown. (The validation succeeded at first boot.) + // + FeaturesOkItemAsUint16 = (UINT16)mFeaturesOkItem; + Status = S3SaveState->Write ( + S3SaveState, // This + EFI_BOOT_SCRIPT_IO_WRITE_OPCODE, // OpCode + EfiBootScriptWidthUint16, // Width + (UINT64)0x510, // Address + (UINTN)1, // Count + &FeaturesOkItemAsUint16 // Buffer + ); + if (EFI_ERROR (Status)) { + DEBUG ((DEBUG_ERROR, "%a:%d: EFI_BOOT_SCRIPT_IO_WRITE_OPCODE: %r\n", + __FUNCTION__, __LINE__, Status)); + goto FatalError; + } + + // + // Read the contents (one byte) of "etc/smi/features-ok". If the value is + // one, we're good. Otherwise, continue reading the data port: QEMU returns 0 + // past the end of the fw_cfg item, so this will hang the resume process, + // which matches our intent. + // + FeaturesOkData = 1; + FeaturesOkMask = MAX_UINT8; + Status = S3SaveState->Write ( + S3SaveState, // This + EFI_BOOT_SCRIPT_IO_POLL_OPCODE, // OpCode + EfiBootScriptWidthUint8, // Width + (UINT64)(UINTN)0x511, // Address + &FeaturesOkData, // Data + &FeaturesOkMask, // DataMask + MAX_UINT64 // Delay + ); + if (EFI_ERROR (Status)) { + DEBUG ((DEBUG_ERROR, "%a:%d: EFI_BOOT_SCRIPT_IO_POLL_OPCODE: %r\n", + __FUNCTION__, __LINE__, Status)); + goto FatalError; + } + + DEBUG ((DEBUG_VERBOSE, "%a: ScratchBuffer@%p\n", __FUNCTION__, + (VOID *)ScratchBuffer)); + return; + +FatalError: + ASSERT (FALSE); + CpuDeadLoop (); +} diff --git a/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c b/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c index 6c03e17a3a8d..f31646d73461 100644 --- a/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c +++ b/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c @@ -32,14 +32,16 @@ #include <Library/PcdLib.h> #include <Library/PciLib.h> #include <Library/QemuFwCfgLib.h> #include <Library/UefiBootServicesTableLib.h> #include <Protocol/S3SaveState.h> #include <Protocol/SmmControl2.h> +#include "SmiFeatures.h" + // // Forward declaration. // STATIC VOID EFIAPI OnS3SaveStateInstalled ( @@ -52,14 +54,21 @@ OnS3SaveStateInstalled ( // only used to carry information from the entry point function to the // S3SaveState protocol installation callback, strictly before the runtime // phase. // STATIC UINTN mSmiEnable; // +// Captures whether SMI feature negotiation is supported. The variable is only +// used to carry this information from the entry point function to the +// S3SaveState protocol installation callback. +// +STATIC BOOLEAN mSmiFeatureNegotiation; + +// // Event signaled when an S3SaveState protocol interface is installed. // STATIC EFI_EVENT mS3SaveStateInstalled; /** Invokes SMI activation from either the preboot or runtime environment. @@ -225,14 +234,19 @@ SmmControl2DxeEntryPoint ( IoWrite32 (mSmiEnable, SmiEnableVal & ~(UINT32)ICH9_SMI_EN_GBL_SMI_EN); if (IoRead32 (mSmiEnable) != SmiEnableVal) { DEBUG ((EFI_D_ERROR, "%a: failed to lock down GBL_SMI_EN\n", __FUNCTION__)); goto FatalError; } + // + // QEMU can inject SMIs in different ways, negotiate our preferences. + // + mSmiFeatureNegotiation = NegotiateSmiFeatures (); + if (QemuFwCfgS3Enabled ()) { VOID *Registration; // // On S3 resume the above register settings have to be repeated. Register a // protocol notify callback that, when boot script saving becomes // available, saves operations equivalent to the above to the boot script. @@ -359,11 +373,18 @@ OnS3SaveStateInstalled ( DEBUG ((EFI_D_ERROR, "%a: EFI_BOOT_SCRIPT_PCI_CONFIG_READ_WRITE_OPCODE: %r\n", __FUNCTION__, Status)); ASSERT (FALSE); CpuDeadLoop (); } + // + // Append a boot script fragment that re-selects the negotiated SMI features. + // + if (mSmiFeatureNegotiation) { + SaveSmiFeatures (S3SaveState); + } + DEBUG ((EFI_D_VERBOSE, "%a: boot script fragment saved\n", __FUNCTION__)); gBS->CloseEvent (Event); mS3SaveStateInstalled = NULL; } -- 2.9.3 ^ permalink raw reply related [flat|nested] 4+ messages in thread
[parent not found: <148646137918.13729.17315316362028292158@jljusten-ivb>]
* Re: [PATCH v3 2/2] OvmfPkg/SmmControl2Dxe: select broadcast SMI if available [not found] ` <148646137918.13729.17315316362028292158@jljusten-ivb> @ 2017-02-07 11:32 ` Laszlo Ersek 0 siblings, 0 replies; 4+ messages in thread From: Laszlo Ersek @ 2017-02-07 11:32 UTC (permalink / raw) To: Jordan Justen; +Cc: edk2-devel-01 On 02/07/17 10:56, Jordan Justen wrote: > On 2017-01-31 03:02:21, Laszlo Ersek wrote: >> When writing to IO port 0xB2 (ICH9_APM_CNT), QEMU by default injects an >> SMI only on the VCPU that is writing the port. > > I'm still annoyed that qemu didn't follow chipset hardware in handling > the b2 port. I know this is long passed any possibility of doing > anything about it, but it is also my last chance to complain. :) It was difficult to come up with a QEMU design that everyone was willing to accept (it was a tough community effort, really -- sort of walking a fine line), and then getting the QEMU patches right (as defined by QEMU maintainers) was again super difficult to me, despite the patches' small size. So yes, I can (quite selfishly :)) agree that "just doing the broadcast" would have been better. I'm much relieved that the feature seems to be converging finally, on both sides. > >> This has exposed corner >> cases and strange behavior with edk2 code, which generally expects a >> software SMI to affect all CPUs at once. We've experienced instability >> despite the fact that OVMF sets PcdCpuSmmApSyncTimeout and >> PcdCpuSmmSyncMode differently from the UefiCpuPkg defaults, such that they >> match QEMU's unicast SMIs better. (Refer to edk2 commits 9b1e378811ff and >> bb0f18b0bce6.) >> >> Using the new fw_cfg-based SMI feature negotiation in QEMU (see commits >> 50de920b372b "hw/isa/lpc_ich9: add SMI feature negotiation via fw_cfg" and >> 5ce45c7a2b15 "hw/isa/lpc_ich9: add broadcast SMI feature"), we can ask >> QEMU to broadcast SMIs. Extensive testing from earlier proves that >> broadcast SMIs are only reliable if we use the UefiCpuPkg defaults for the >> above PCDs. With those settings however, the broadcast is very reliable -- >> the most reliable configuration encountered thus far. > > Given my experience with SMI's we could see more unexpected corner > cases. It seems that SMM is all corner cases. :) Processor mode changes, multiprocessing, and security impact. What could go wrong! :) > > Series Reviewed-by: Jordan Justen <jordan.l.justen@intel.com> \o/ Hooray! Thank you! Pushed as 7c609a144b66..a316d7ac91d3. Cheers! Laszlo > >> >> Therefore negotiate broadcast SMIs with QEMU, and if the negotiation is >> successful, dynamically revert the PCDs to the UefiCpuPkg defaults. >> >> Setting the PCDs in this module is safe: >> >> - only PiSmmCpuDxeSmm consumes them, >> >> - PiSmmCpuDxeSmm is a DXE_SMM_DRIVER, launched by the SMM_CORE >> (MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf), >> >> - the SMM_CORE is launched by the SMM IPL runtime DXE driver >> (MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf), >> >> - the SMM IPL has a DEPEX on EFI_SMM_CONTROL2_PROTOCOL, >> >> - OvmfPkg/SmmControl2Dxe produces that protocol. >> >> The end result is that PiSmmCpuDxeSmm cannot be dispatched before >> SmmControl2Dxe installs EFI_SMM_CONTROL2_PROTOCOL and returns from its >> entry point. Hence we can set the PCD's consumed by PiSmmCpuDxeSmm in >> SmmControl2Dxe. >> >> Cc: Jordan Justen <jordan.l.justen@intel.com> >> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=230 >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Laszlo Ersek <lersek@redhat.com> >> --- >> >> Notes: >> v3: >> - replace the ICH9_APM_STS-based feature negotiation with the new >> writeable fw_cfg kind >> >> OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.inf | 8 + >> OvmfPkg/SmmControl2Dxe/SmiFeatures.h | 49 +++ >> OvmfPkg/SmmControl2Dxe/SmiFeatures.c | 324 ++++++++++++++++++++ >> OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c | 21 ++ >> 4 files changed, 402 insertions(+) >> >> diff --git a/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.inf b/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.inf >> index 0e9f98c2871c..31c80bd4448c 100644 >> --- a/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.inf >> +++ b/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.inf >> @@ -35,32 +35,40 @@ [Defines] >> # >> # The following information is for reference only and not required by the build tools. >> # >> # VALID_ARCHITECTURES = IA32 X64 >> # >> >> [Sources] >> + SmiFeatures.h >> + SmiFeatures.c >> SmmControl2Dxe.c >> >> [Packages] >> MdePkg/MdePkg.dec >> OvmfPkg/OvmfPkg.dec >> + UefiCpuPkg/UefiCpuPkg.dec >> >> [LibraryClasses] >> BaseLib >> DebugLib >> IoLib >> + MemoryAllocationLib >> PcdLib >> PciLib >> QemuFwCfgLib >> UefiBootServicesTableLib >> UefiDriverEntryPoint >> >> [Protocols] >> gEfiS3SaveStateProtocolGuid ## SOMETIMES_CONSUMES >> gEfiSmmControl2ProtocolGuid ## PRODUCES >> >> +[Pcd] >> + gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmApSyncTimeout ## SOMETIMES_PRODUCES >> + gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode ## SOMETIMES_PRODUCES >> + >> [FeaturePcd] >> gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire >> >> [Depex] >> TRUE >> diff --git a/OvmfPkg/SmmControl2Dxe/SmiFeatures.h b/OvmfPkg/SmmControl2Dxe/SmiFeatures.h >> new file mode 100644 >> index 000000000000..9d5f1dbcb57e >> --- /dev/null >> +++ b/OvmfPkg/SmmControl2Dxe/SmiFeatures.h >> @@ -0,0 +1,49 @@ >> +/**@file >> + Negotiate SMI features with QEMU, and configure UefiCpuPkg/PiSmmCpuDxeSmm >> + accordingly. >> + >> + Copyright (C) 2016-2017, Red Hat, Inc. >> + >> + This program and the accompanying materials are licensed and made available >> + under the terms and conditions of the BSD License which accompanies this >> + distribution. The full text of the license may be found at >> + http://opensource.org/licenses/bsd-license.php >> + >> + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, WITHOUT >> + WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. >> +**/ >> + >> +#ifndef __SMI_FEATURES_H__ >> +#define __SMI_FEATURES_H__ >> + >> +#include <Protocol/S3SaveState.h> >> + >> +/** >> + Negotiate SMI features with QEMU. >> + >> + @retval FALSE If SMI feature negotiation is not supported by QEMU. This is >> + not an error, it just means that SaveSmiFeatures() should not >> + be called. >> + >> + @retval TRUE SMI feature negotiation is supported, and it has completed >> + successfully as well. (Failure to negotiate is a fatal error >> + and the function never returns in that case.) >> +**/ >> +BOOLEAN >> +NegotiateSmiFeatures ( >> + VOID >> + ); >> + >> +/** >> + Append a boot script fragment that will re-select the previously negotiated >> + SMI features during S3 resume. >> + >> + @param[in] S3SaveState The EFI_S3_SAVE_STATE_PROTOCOL instance to append to >> + the S3 boot script with. >> +**/ >> +VOID >> +SaveSmiFeatures ( >> + IN EFI_S3_SAVE_STATE_PROTOCOL *S3SaveState >> + ); >> + >> +#endif >> diff --git a/OvmfPkg/SmmControl2Dxe/SmiFeatures.c b/OvmfPkg/SmmControl2Dxe/SmiFeatures.c >> new file mode 100644 >> index 000000000000..e070969065c0 >> --- /dev/null >> +++ b/OvmfPkg/SmmControl2Dxe/SmiFeatures.c >> @@ -0,0 +1,324 @@ >> +/**@file >> + Negotiate SMI features with QEMU, and configure UefiCpuPkg/PiSmmCpuDxeSmm >> + accordingly. >> + >> + Copyright (C) 2016-2017, Red Hat, Inc. >> + >> + This program and the accompanying materials are licensed and made available >> + under the terms and conditions of the BSD License which accompanies this >> + distribution. The full text of the license may be found at >> + http://opensource.org/licenses/bsd-license.php >> + >> + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, WITHOUT >> + WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. >> +**/ >> + >> +#include <Library/BaseLib.h> >> +#include <Library/DebugLib.h> >> +#include <Library/MemoryAllocationLib.h> >> +#include <Library/PcdLib.h> >> +#include <Library/QemuFwCfgLib.h> >> + >> +#include "SmiFeatures.h" >> + >> +// >> +// The following bit value stands for "broadcast SMI" in the >> +// "etc/smi/supported-features" and "etc/smi/requested-features" fw_cfg files. >> +// >> +#define ICH9_LPC_SMI_F_BROADCAST BIT0 >> + >> +// >> +// Provides a scratch buffer (allocated in EfiReservedMemoryType type memory) >> +// for the S3 boot script fragment to write to and read from. The buffer >> +// captures a combined fw_cfg item selection + write command using the DMA >> +// access method. Note that we don't trust the runtime OS to preserve the >> +// contents of the buffer, the boot script will first rewrite it. >> +// >> +#pragma pack (1) >> +typedef struct { >> + FW_CFG_DMA_ACCESS Access; >> + UINT64 Features; >> +} SCRATCH_BUFFER; >> +#pragma pack () >> + >> +// >> +// These carry the selector keys of the "etc/smi/requested-features" and >> +// "etc/smi/features-ok" fw_cfg files from NegotiateSmiFeatures() to >> +// SaveSmiFeatures(). >> +// >> +STATIC FIRMWARE_CONFIG_ITEM mRequestedFeaturesItem; >> +STATIC FIRMWARE_CONFIG_ITEM mFeaturesOkItem; >> + >> +// >> +// Carries the negotiated SMI features from NegotiateSmiFeatures() to >> +// SaveSmiFeatures(). >> +// >> +STATIC UINT64 mSmiFeatures; >> + >> +/** >> + Negotiate SMI features with QEMU. >> + >> + @retval FALSE If SMI feature negotiation is not supported by QEMU. This is >> + not an error, it just means that SaveSmiFeatures() should not >> + be called. >> + >> + @retval TRUE SMI feature negotiation is supported, and it has completed >> + successfully as well. (Failure to negotiate is a fatal error >> + and the function never returns in that case.) >> +**/ >> +BOOLEAN >> +NegotiateSmiFeatures ( >> + VOID >> + ) >> +{ >> + FIRMWARE_CONFIG_ITEM SupportedFeaturesItem; >> + UINTN SupportedFeaturesSize; >> + UINTN RequestedFeaturesSize; >> + UINTN FeaturesOkSize; >> + >> + // >> + // Look up the fw_cfg files used for feature negotiation. The selector keys >> + // of "etc/smi/requested-features" and "etc/smi/features-ok" are saved >> + // statically. If the files are missing, then QEMU doesn't support SMI >> + // feature negotiation. >> + // >> + if (RETURN_ERROR (QemuFwCfgFindFile ("etc/smi/supported-features", >> + &SupportedFeaturesItem, &SupportedFeaturesSize)) || >> + RETURN_ERROR (QemuFwCfgFindFile ("etc/smi/requested-features", >> + &mRequestedFeaturesItem, &RequestedFeaturesSize)) || >> + RETURN_ERROR (QemuFwCfgFindFile ("etc/smi/features-ok", >> + &mFeaturesOkItem, &FeaturesOkSize))) { >> + DEBUG ((DEBUG_INFO, "%a: SMI feature negotiation unavailable\n", >> + __FUNCTION__)); >> + return FALSE; >> + } >> + >> + // >> + // If the files are present but their sizes disagree with us, that's a fatal >> + // error (we can't trust the behavior of SMIs either way). >> + // >> + if (SupportedFeaturesSize != sizeof mSmiFeatures || >> + RequestedFeaturesSize != sizeof mSmiFeatures || >> + FeaturesOkSize != sizeof (UINT8)) { >> + DEBUG ((DEBUG_ERROR, "%a: size mismatch in feature negotiation\n", >> + __FUNCTION__)); >> + goto FatalError; >> + } >> + >> + // >> + // Get the features supported by the host. >> + // >> + QemuFwCfgSelectItem (SupportedFeaturesItem); >> + QemuFwCfgReadBytes (sizeof mSmiFeatures, &mSmiFeatures); >> + >> + // >> + // We want broadcast SMI and nothing else. >> + // >> + mSmiFeatures &= ICH9_LPC_SMI_F_BROADCAST; >> + QemuFwCfgSelectItem (mRequestedFeaturesItem); >> + QemuFwCfgWriteBytes (sizeof mSmiFeatures, &mSmiFeatures); >> + >> + // >> + // Invoke feature validation in QEMU. If the selection is accepted, the >> + // features will be locked down. If the selection is rejected, feature >> + // negotiation remains open; however we don't know what to do in that case, >> + // so that's a fatal error. >> + // >> + QemuFwCfgSelectItem (mFeaturesOkItem); >> + if (QemuFwCfgRead8 () != 1) { >> + DEBUG ((DEBUG_ERROR, "%a: negotiation failed for feature bitmap 0x%Lx\n", >> + __FUNCTION__, mSmiFeatures)); >> + goto FatalError; >> + } >> + >> + if ((mSmiFeatures & ICH9_LPC_SMI_F_BROADCAST) == 0) { >> + // >> + // If we can't get broadcast SMIs from QEMU, that's acceptable too, >> + // although not optimal. >> + // >> + DEBUG ((DEBUG_INFO, "%a: SMI broadcast unavailable\n", __FUNCTION__)); >> + } else { >> + // >> + // Configure the traditional AP sync / SMI delivery mode for >> + // PiSmmCpuDxeSmm. Effectively, restore the UefiCpuPkg defaults, from which >> + // the original QEMU behavior (i.e., unicast SMI) used to differ. >> + // >> + if (RETURN_ERROR (PcdSet64S (PcdCpuSmmApSyncTimeout, 1000000)) || >> + RETURN_ERROR (PcdSet8S (PcdCpuSmmSyncMode, 0x00))) { >> + DEBUG ((DEBUG_ERROR, "%a: PiSmmCpuDxeSmm PCD configuration failed\n", >> + __FUNCTION__)); >> + goto FatalError; >> + } >> + DEBUG ((DEBUG_INFO, "%a: using SMI broadcast\n", __FUNCTION__)); >> + } >> + >> + // >> + // Negotiation successful (although we may not have gotten the optimal >> + // feature set). >> + // >> + return TRUE; >> + >> +FatalError: >> + ASSERT (FALSE); >> + CpuDeadLoop (); >> + // >> + // Keep the compiler happy. >> + // >> + return FALSE; >> +} >> + >> +/** >> + Append a boot script fragment that will re-select the previously negotiated >> + SMI features during S3 resume. >> + >> + @param[in] S3SaveState The EFI_S3_SAVE_STATE_PROTOCOL instance to append to >> + the S3 boot script with. >> +**/ >> +VOID >> +SaveSmiFeatures ( >> + IN EFI_S3_SAVE_STATE_PROTOCOL *S3SaveState >> + ) >> +{ >> + SCRATCH_BUFFER *ScratchBuffer; >> + EFI_STATUS Status; >> + UINT64 AccessAddress; >> + UINT32 ControlPollData; >> + UINT32 ControlPollMask; >> + UINT16 FeaturesOkItemAsUint16; >> + UINT8 FeaturesOkData; >> + UINT8 FeaturesOkMask; >> + >> + ScratchBuffer = AllocateReservedPool (sizeof *ScratchBuffer); >> + if (ScratchBuffer == NULL) { >> + DEBUG ((DEBUG_ERROR, "%a: scratch buffer allocation failed\n", >> + __FUNCTION__)); >> + goto FatalError; >> + } >> + >> + // >> + // Populate the scratch buffer with a select + write fw_cfg DMA command that >> + // will write the negotiated feature bitmap into >> + // "etc/smi/requested-features". >> + // >> + ScratchBuffer->Access.Control = SwapBytes32 ( >> + (UINT32)mRequestedFeaturesItem << 16 | >> + FW_CFG_DMA_CTL_SELECT | >> + FW_CFG_DMA_CTL_WRITE >> + ); >> + ScratchBuffer->Access.Length = SwapBytes32 ( >> + (UINT32)sizeof ScratchBuffer->Features); >> + ScratchBuffer->Access.Address = SwapBytes64 ( >> + (UINTN)&ScratchBuffer->Features); >> + ScratchBuffer->Features = mSmiFeatures; >> + >> + // >> + // Copy the scratch buffer into the boot script. When replayed, this >> + // EFI_BOOT_SCRIPT_MEM_WRITE_OPCODE will restore the current contents of the >> + // scratch buffer, in-place. >> + // >> + Status = S3SaveState->Write ( >> + S3SaveState, // This >> + EFI_BOOT_SCRIPT_MEM_WRITE_OPCODE, // OpCode >> + EfiBootScriptWidthUint8, // Width >> + (UINT64)(UINTN)ScratchBuffer, // Address >> + sizeof *ScratchBuffer, // Count >> + (VOID*)ScratchBuffer // Buffer >> + ); >> + if (EFI_ERROR (Status)) { >> + DEBUG ((DEBUG_ERROR, "%a:%d: EFI_BOOT_SCRIPT_MEM_WRITE_OPCODE: %r\n", >> + __FUNCTION__, __LINE__, Status)); >> + goto FatalError; >> + } >> + >> + // >> + // Append an opcode that will write the address of the scratch buffer to the >> + // fw_cfg DMA address register, which consists of two 32-bit IO ports. The >> + // second (highest address, least significant) write will start the transfer. >> + // >> + AccessAddress = SwapBytes64 ((UINTN)&ScratchBuffer->Access); >> + Status = S3SaveState->Write ( >> + S3SaveState, // This >> + EFI_BOOT_SCRIPT_IO_WRITE_OPCODE, // OpCode >> + EfiBootScriptWidthUint32, // Width >> + (UINT64)0x514, // Address >> + (UINTN)2, // Count >> + &AccessAddress // Buffer >> + ); >> + if (EFI_ERROR (Status)) { >> + DEBUG ((DEBUG_ERROR, "%a:%d: EFI_BOOT_SCRIPT_IO_WRITE_OPCODE: %r\n", >> + __FUNCTION__, __LINE__, Status)); >> + goto FatalError; >> + } >> + >> + // >> + // The EFI_BOOT_SCRIPT_MEM_POLL_OPCODE will wait until the Control word reads >> + // as zero (transfer complete). As timeout we use MAX_UINT64 * 100ns, which >> + // is approximately 58494 years. >> + // >> + ControlPollData = 0; >> + ControlPollMask = MAX_UINT32; >> + Status = S3SaveState->Write ( >> + S3SaveState, // This >> + EFI_BOOT_SCRIPT_MEM_POLL_OPCODE, // OpCode >> + EfiBootScriptWidthUint32, // Width >> + (UINT64)(UINTN)&ScratchBuffer->Access.Control, // Address >> + &ControlPollData, // Data >> + &ControlPollMask, // DataMask >> + MAX_UINT64 // Delay >> + ); >> + if (EFI_ERROR (Status)) { >> + DEBUG ((DEBUG_ERROR, "%a:%d: EFI_BOOT_SCRIPT_MEM_POLL_OPCODE: %r\n", >> + __FUNCTION__, __LINE__, Status)); >> + goto FatalError; >> + } >> + >> + // >> + // Select the "etc/smi/features-ok" fw_cfg file, which invokes the feature >> + // validation & lockdown. (The validation succeeded at first boot.) >> + // >> + FeaturesOkItemAsUint16 = (UINT16)mFeaturesOkItem; >> + Status = S3SaveState->Write ( >> + S3SaveState, // This >> + EFI_BOOT_SCRIPT_IO_WRITE_OPCODE, // OpCode >> + EfiBootScriptWidthUint16, // Width >> + (UINT64)0x510, // Address >> + (UINTN)1, // Count >> + &FeaturesOkItemAsUint16 // Buffer >> + ); >> + if (EFI_ERROR (Status)) { >> + DEBUG ((DEBUG_ERROR, "%a:%d: EFI_BOOT_SCRIPT_IO_WRITE_OPCODE: %r\n", >> + __FUNCTION__, __LINE__, Status)); >> + goto FatalError; >> + } >> + >> + // >> + // Read the contents (one byte) of "etc/smi/features-ok". If the value is >> + // one, we're good. Otherwise, continue reading the data port: QEMU returns 0 >> + // past the end of the fw_cfg item, so this will hang the resume process, >> + // which matches our intent. >> + // >> + FeaturesOkData = 1; >> + FeaturesOkMask = MAX_UINT8; >> + Status = S3SaveState->Write ( >> + S3SaveState, // This >> + EFI_BOOT_SCRIPT_IO_POLL_OPCODE, // OpCode >> + EfiBootScriptWidthUint8, // Width >> + (UINT64)(UINTN)0x511, // Address >> + &FeaturesOkData, // Data >> + &FeaturesOkMask, // DataMask >> + MAX_UINT64 // Delay >> + ); >> + if (EFI_ERROR (Status)) { >> + DEBUG ((DEBUG_ERROR, "%a:%d: EFI_BOOT_SCRIPT_IO_POLL_OPCODE: %r\n", >> + __FUNCTION__, __LINE__, Status)); >> + goto FatalError; >> + } >> + >> + DEBUG ((DEBUG_VERBOSE, "%a: ScratchBuffer@%p\n", __FUNCTION__, >> + (VOID *)ScratchBuffer)); >> + return; >> + >> +FatalError: >> + ASSERT (FALSE); >> + CpuDeadLoop (); >> +} >> diff --git a/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c b/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c >> index 6c03e17a3a8d..f31646d73461 100644 >> --- a/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c >> +++ b/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c >> @@ -32,14 +32,16 @@ >> #include <Library/PcdLib.h> >> #include <Library/PciLib.h> >> #include <Library/QemuFwCfgLib.h> >> #include <Library/UefiBootServicesTableLib.h> >> #include <Protocol/S3SaveState.h> >> #include <Protocol/SmmControl2.h> >> >> +#include "SmiFeatures.h" >> + >> // >> // Forward declaration. >> // >> STATIC >> VOID >> EFIAPI >> OnS3SaveStateInstalled ( >> @@ -52,14 +54,21 @@ OnS3SaveStateInstalled ( >> // only used to carry information from the entry point function to the >> // S3SaveState protocol installation callback, strictly before the runtime >> // phase. >> // >> STATIC UINTN mSmiEnable; >> >> // >> +// Captures whether SMI feature negotiation is supported. The variable is only >> +// used to carry this information from the entry point function to the >> +// S3SaveState protocol installation callback. >> +// >> +STATIC BOOLEAN mSmiFeatureNegotiation; >> + >> +// >> // Event signaled when an S3SaveState protocol interface is installed. >> // >> STATIC EFI_EVENT mS3SaveStateInstalled; >> >> /** >> Invokes SMI activation from either the preboot or runtime environment. >> >> @@ -225,14 +234,19 @@ SmmControl2DxeEntryPoint ( >> IoWrite32 (mSmiEnable, SmiEnableVal & ~(UINT32)ICH9_SMI_EN_GBL_SMI_EN); >> if (IoRead32 (mSmiEnable) != SmiEnableVal) { >> DEBUG ((EFI_D_ERROR, "%a: failed to lock down GBL_SMI_EN\n", >> __FUNCTION__)); >> goto FatalError; >> } >> >> + // >> + // QEMU can inject SMIs in different ways, negotiate our preferences. >> + // >> + mSmiFeatureNegotiation = NegotiateSmiFeatures (); >> + >> if (QemuFwCfgS3Enabled ()) { >> VOID *Registration; >> >> // >> // On S3 resume the above register settings have to be repeated. Register a >> // protocol notify callback that, when boot script saving becomes >> // available, saves operations equivalent to the above to the boot script. >> @@ -359,11 +373,18 @@ OnS3SaveStateInstalled ( >> DEBUG ((EFI_D_ERROR, >> "%a: EFI_BOOT_SCRIPT_PCI_CONFIG_READ_WRITE_OPCODE: %r\n", __FUNCTION__, >> Status)); >> ASSERT (FALSE); >> CpuDeadLoop (); >> } >> >> + // >> + // Append a boot script fragment that re-selects the negotiated SMI features. >> + // >> + if (mSmiFeatureNegotiation) { >> + SaveSmiFeatures (S3SaveState); >> + } >> + >> DEBUG ((EFI_D_VERBOSE, "%a: boot script fragment saved\n", __FUNCTION__)); >> gBS->CloseEvent (Event); >> mS3SaveStateInstalled = NULL; >> } >> -- >> 2.9.3 >> ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-02-07 11:32 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-01-31 11:02 [PATCH v3 0/2] OvmfPkg: broadcast SMIs and dynamically revert to traditional AP sync mode Laszlo Ersek
2017-01-31 11:02 ` [PATCH v3 1/2] OvmfPkg: dynamic defaults for PcdCpuSmmApSyncTimeout, PcdCpuSmmSyncMode Laszlo Ersek
2017-01-31 11:02 ` [PATCH v3 2/2] OvmfPkg/SmmControl2Dxe: select broadcast SMI if available Laszlo Ersek
[not found] ` <148646137918.13729.17315316362028292158@jljusten-ivb>
2017-02-07 11:32 ` Laszlo Ersek
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox