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

[Laszlo Ersek <lersek@redhat.com>]

On 11/23/16 04:44, Jordan Justen wrote:
> 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.

Auto-initializing mSmiBroadcast to FALSE (0) here is required by the C
standard.

I think you are recalling the obscure problem we have with PEIMs on S3
resume, in the SMM-less build only. (And, I'm impressed that you
remember it :)) In that case we re-use the previously decompressed (and
executed) PEI modules from RAM. Then such variables (= of static storage
duration) are not re-initialized when they are dispatched during S3
resume (in the SMM-less build).

However, as far as I remember, they are also not re-set when we provide
an initializer of the form "= FALSE" -- that's again not code in the
binary, just initialized static data. The re-setting only happens (at
SMM-less S3 resume) if we assign values in code (entry point function of
the module, or library constructor).

Anyhow, this module is a runtime DXE driver, not a PEIM, so it is not
affected. During S3 resume, its not (re)dispatched, it plainly survives
in runtime code / data type memory, and all its variables are supposed
to retain their values.

> 
> Does it hurt to actually set it to FALSE?

It does not hurt, but it makes no difference; either in this case, or
for those PEIMs during SMM-less S3 resume.

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

Before posting the QEMU and edk2 series, I tested Windows extensively
with broadcast SMI (using the Ia32X64 build):

{ Windows 7,
  Windows Server 2008 R2,
  Windows 8.1,
  Windows Server 2012 R2,
  Windows 10,
  Windows Server 2016 Tech Preview 4
} x {
  normal boot,
  S3 resume
}

In particular, the Windows 8 family boots *much* faster with broadcast
SMIs. I had KVM-traced Windows 8 boot earlier, and for some reason that
boot is crazy about messing with variables -- the KVM trace was
chock-full of SMIs. A good part of those variable accesses seemed to
come from APs, which made a visible impact (--> choppiness) on the
rotating animation during Windows 8 boot. So the broadcast SMI happens
to fix that mild annoyance as well.

Anyway, now that you are asking, I remember another method to trigger a
variable write from under Windows 8:

https://firmware.intel.com/blog/using-os-indications-uefi
http://answers.microsoft.com/en-us/surface/forum/surfpro-surfgetstart/accessing-windows-8-firmware-settings/c37b0b00-ae8f-4ad8-9ad6-88e070cb4d36
http://www.windowspasswordsrecovery.com/articles/win8/how-to-access-the-boot-menu-and-bios-in-a-windows-8-computer.html

Windows 8 provides a GUI to set the OsIndications UEFI variable. So,
I've just retested it now (with SMI broadcast), and it works fine.
Windows reboots the VM and the setup TUI is entered immediately. From
the log:

-----------
SetBootOrderFromQemu: setting BootOrder: success
[Bds]OsIndication: 0000000000000001
[Bds]=============Begin Load Options Dumping ...=============
  Driver Options:
  SysPrep Options:
  Boot Options:
    Boot0002: UEFI QEMU QEMU CD-ROM  2           0x0001
    Boot0003: Windows Boot Manager               0x0001
    Boot0001: UEFI QEMU QEMU CD-ROM              0x0001
    Boot0000: UiApp              0x0109
    Boot0009: EFI Internal Shell                 0x0001
  PlatformRecovery Options:
    PlatformRecovery0000: Default PlatformRecovery               0x0001
[Bds]=============End Load Options Dumping=============
[Bds] Booting Boot Manager Menu.
-----------

> 
> Series Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>

Thank you!

I'll delay pushing the patches until Michael Tsirkin reviews the
interface spec in the QEMU series on qemu-devel.

Thanks!
Laszlo

> 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