Re: [PATCH v3 2/2] OvmfPkg/SmmControl2Dxe: select broadcast SMI if available

[Laszlo Ersek <lersek@redhat.com>]

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
>>