Re: [PATCH v2 4/4] OvmfPkg/SmmControl2Dxe: select broadcast SMI if available

[Jordan Justen <jordan.l.justen@intel.com>]

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.fan@intel.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Michael Kinney <michael.d.kinney@intel.com>
> Cc: Paolo Bonzini <pbonzini@redhat.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:
>     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.justen@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