On 2016-11-18 05:52:49, 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. 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 feature negotiation specified in QEMU's
> "docs/specs/q35-apm-sts.txt", 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: Jeff Fan <jeff....@intel.com>
> Cc: Jordan Justen <jordan.l.jus...@intel.com>
> Cc: Michael Kinney <michael.d.kin...@intel.com>
> Cc: Paolo Bonzini <pbonz...@redhat.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=230
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek <ler...@redhat.com>
> ---
>
> Notes:
> v2:
> - negotiate the broadcast SMI feature
> - make the S3 boot script re-select it if it's available at first boot
> - set PiSmmCpuDxeSmm's PCD's dynamically
>
> OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.inf | 5 ++
> OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c | 73 +++++++++++++++++++-
> 2 files changed, 75 insertions(+), 3 deletions(-)
>
> diff --git a/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.inf
> b/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.inf
> index 0e9f98c2871c..c28832435eed 100644
> --- a/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.inf
> +++ b/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.inf
> @@ -44,6 +44,7 @@ [Sources]
> [Packages]
> MdePkg/MdePkg.dec
> OvmfPkg/OvmfPkg.dec
> + UefiCpuPkg/UefiCpuPkg.dec
>
> [LibraryClasses]
> BaseLib
> @@ -59,6 +60,10 @@ [Protocols]
> gEfiS3SaveStateProtocolGuid ## SOMETIMES_CONSUMES
> gEfiSmmControl2ProtocolGuid ## PRODUCES
>
> +[Pcd]
> + gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmApSyncTimeout ## SOMETIMES_PRODUCES
> + gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode ## SOMETIMES_PRODUCES
> +
> [FeaturePcd]
> gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire
>
> diff --git a/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c
> b/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c
> index 82549b0a7e35..6f05797979de 100644
> --- a/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c
> +++ b/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c
> @@ -56,6 +56,15 @@ OnS3SaveStateInstalled (
> STATIC UINTN mSmiEnable;
> //
> +// The indicator whether we have negotiated with QEMU to broadcast the SMI to
> +// all VCPUs whenever we write to ICH9_APM_CNT in our
> +// EFI_SMM_CONTROL2_PROTOCOL.Trigger() implementation. This variable is only
> +// used to carry information from the entry point function to the S3SaveState
> +// protocol installation callback.
> +//
> +STATIC BOOLEAN mSmiBroadcast;
I think you are relying on uninitialized static data being zero'd.
Does it hurt to actually set it to FALSE?
I'm glad we'll be using a mechanism that broadcasts to all the
processors like the real hardware. It is a bit unfortunate that it
doesn't go through the b2 port for it.
Did you get a chance to test variable writes on Windows with this
change? I know Windows can be more picky about port accesses...
Series Reviewed-by: Jordan Justen <jordan.l.jus...@intel.com>
Thanks!
> +
> +//
> // Event signaled when an S3SaveState protocol interface is installed.
> //
> STATIC EFI_EVENT mS3SaveStateInstalled;
> @@ -107,10 +116,10 @@ SmmControl2DxeTrigger (
> // report about hardware status, while this register is fully governed by
> // software.
> //
> - // Write to the status register first, as this won't trigger the SMI just
> - // yet. Then write to the control register.
> + // QEMU utilizes the status register for feature negotiation, therefore we
> + // can't accept external data.
> //
> - IoWrite8 (ICH9_APM_STS, DataPort == NULL ? 0 : *DataPort);
> + ASSERT (DataPort == NULL);
> IoWrite8 (ICH9_APM_CNT, CommandPort == NULL ? 0 : *CommandPort);
> return EFI_SUCCESS;
> }
> @@ -177,6 +186,7 @@ SmmControl2DxeEntryPoint (
> {
> UINT32 PmBase;
> UINT32 SmiEnableVal;
> + UINT8 ApmStatusVal;
> EFI_STATUS Status;
>
> //
> @@ -229,6 +239,43 @@ SmmControl2DxeEntryPoint (
> goto FatalError;
> }
>
> + //
> + // Negotiate broadcast SMI with QEMU.
> + //
> + IoWrite8 (ICH9_APM_STS, QEMU_ICH9_APM_STS_GET_SET_FEAT);
> + ApmStatusVal = IoRead8 (ICH9_APM_STS);
> + if ((ApmStatusVal & QEMU_ICH9_APM_STS_GET_SET_FEAT) != 0) {
> + DEBUG ((DEBUG_VERBOSE, "%a: SMI feature negotiation unavailable\n",
> + __FUNCTION__));
> + IoWrite8 (ICH9_APM_STS, 0);
> + } else if ((ApmStatusVal & QEMU_ICH9_APM_STS_F_BCAST_SMI) == 0) {
> + DEBUG ((DEBUG_VERBOSE, "%a: SMI broadcast unavailable\n", __FUNCTION__));
> + } else {
> + //
> + // Request the broadcast feature, and nothing else. Check for
> confirmation.
> + //
> + IoWrite8 (ICH9_APM_STS, QEMU_ICH9_APM_STS_F_BCAST_SMI);
> + ApmStatusVal = IoRead8 (ICH9_APM_STS);
> + if ((ApmStatusVal & QEMU_ICH9_APM_STS_GET_SET_FEAT) != 0) {
> + DEBUG ((DEBUG_VERBOSE, "%a: failed to negotiate SMI broadcast\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;
> + }
> + mSmiBroadcast = TRUE;
> + DEBUG ((DEBUG_INFO, "%a: using SMI broadcast\n", __FUNCTION__));
> + }
> + }
> +
> if (QemuFwCfgS3Enabled ()) {
> VOID *Registration;
>
> @@ -360,6 +407,26 @@ OnS3SaveStateInstalled (
> CpuDeadLoop ();
> }
>
> + if (mSmiBroadcast) {
> + UINT8 ApmStatusVal;
> +
> + ApmStatusVal = QEMU_ICH9_APM_STS_F_BCAST_SMI;
> + Status = S3SaveState->Write (
> + S3SaveState,
> + EFI_BOOT_SCRIPT_IO_WRITE_OPCODE,
> + EfiBootScriptWidthUint8,
> + (UINT64)ICH9_APM_STS,
> + (UINTN)1,
> + &ApmStatusVal
> + );
> + if (EFI_ERROR (Status)) {
> + DEBUG ((DEBUG_ERROR, "%a: EFI_BOOT_SCRIPT_IO_WRITE_OPCODE: %r\n",
> + __FUNCTION__, Status));
> + ASSERT (FALSE);
> + CpuDeadLoop ();
> + }
> + }
> +
> DEBUG ((EFI_D_VERBOSE, "%a: boot script fragment saved\n", __FUNCTION__));
> gBS->CloseEvent (Event);
> mS3SaveStateInstalled = NULL;
> --
> 2.9.2
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
