[PATCH 0/3] OvmfPkg: broadcast SMIs and revert to traditional AP sync mode

[PATCH 0/3] OvmfPkg: broadcast SMIs and revert to traditional AP sync mode

[Laszlo Ersek]

Spurred by Jiewen's and Jeff's recent SMM-related work, Paolo and I
stress-tested OVMF's SMM a little bit, and with S3 it turns out to be
less stable than ideal.

Paolo agreed / suggested that we add a broadcast SMI mode to QEMU, and
utilize it from OVMF:

  https://www.mail-archive.com/edk2-devel@lists.01.org/msg19669.html

I sent the QEMU patch a few minutes (or hours?) ago:

  http://lists.nongnu.org/archive/html/qemu-devel/2016-11/msg02687.html

and these are the OVMF patches that put the QEMU functionality to use.

With these patches (and with Jeff's just-committed series for
<https://bugzilla.tianocore.org/show_bug.cgi?id=216>) I see stable and
reliable behavior with SMM+S3. Please see

  https://lists.01.org/pipermail/edk2-devel/2016-November/004682.html

for details.

Repo:   https://github.com/lersek/edk2/
Branch: broadcast_smi
BZ:     https://bugzilla.tianocore.org/show_bug.cgi?id=230

Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>

Thanks
Laszlo

Laszlo Ersek (3):
  OvmfPkg/SmmControl2Dxe: select broadcast SMI
  OvmfPkg: revert "any AP in SMM should not wait for the BSP for more
    than 100 ms"
  OvmfPkg: revert "use relaxed AP SMM synchronization mode"

 OvmfPkg/OvmfPkgIa32.dsc                       |  2 --
 OvmfPkg/OvmfPkgIa32X64.dsc                    |  2 --
 OvmfPkg/OvmfPkgX64.dsc                        |  2 --
 OvmfPkg/Include/IndustryStandard/Q35MchIch9.h |  6 ++++--
 OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c       | 18 +++++++++++++++++-
 5 files changed, 21 insertions(+), 9 deletions(-)

-- 
2.9.2

[PATCH 1/3] OvmfPkg/SmmControl2Dxe: select broadcast SMI

[Laszlo Ersek]

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.

The QEMU patch

  hw/isa/lpc_ich9: inject SMI on all VCPUs if APM_STS == 'Q'

adds a side-channel / backdoor to QEMU that enables the firmware to
request a broadcast SMI; namely, by setting IO port 0xB3 (ICH9_APM_STS) to
value 0x51 ('Q') first. (The default behavior cannot be changed because
SeaBIOS depends on it.) Utilize this feature in OVMF's
EFI_SMM_CONTROL2_PROTOCOL.Trigger() method.

The change has no effect on QEMUs that lack the above patch.

Cc: Jordan Justen <jordan.l.justen@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>
---
 OvmfPkg/Include/IndustryStandard/Q35MchIch9.h |  6 ++++--
 OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c       | 18 +++++++++++++++++-
 2 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h b/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h
index 4dc2c39901c1..acc05f84b18c 100644
--- a/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h
+++ b/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h
@@ -91,8 +91,10 @@
 //
 // IO ports
 //
-#define ICH9_APM_CNT 0xB2
-#define ICH9_APM_STS 0xB3
+#define ICH9_APM_CNT                    0xB2
+
+#define ICH9_APM_STS                    0xB3
+#define QEMU_ICH9_APM_STS_BROADCAST_SMI   'Q'
 
 //
 // IO ports relative to PMBASE
diff --git a/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c b/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c
index 82549b0a7e35..2c987c0cc558 100644
--- a/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c
+++ b/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c
@@ -107,10 +107,26 @@ SmmControl2DxeTrigger (
   // report about hardware status, while this register is fully governed by
   // software.
   //
+  // On the QEMU platform, we use the status register to request a "broadcast"
+  // SMI; i.e., to bring all VCPUs into SMM at once. Edk2 handles the case when
+  // the SMI is raised only on the processor that calls Trigger(), but
+  //
+  // - if the processor executing Trigger() is an AP, then the resultant AP-BSP
+  //   synchronization is time consuming and computation-hungry,
+  //
+  // - edk2 modules that deal with SMIs generally expect / prefer a software
+  //   SMI to affect all CPUs at once; unicast SMIs have exposed obscure corner
+  //   cases.
+  //
+  // Note that we exploit the fact that the SMM_CORE never passes a non-NULL
+  // DataPort pointer (that is, we exploit that it doesn't care about the
+  // status register value).
+  //
   // Write to the status register first, as this won't trigger the SMI just
   // yet. Then write to the control register.
   //
-  IoWrite8 (ICH9_APM_STS, DataPort    == NULL ? 0 : *DataPort);
+  ASSERT (DataPort == NULL);
+  IoWrite8 (ICH9_APM_STS, QEMU_ICH9_APM_STS_BROADCAST_SMI);
   IoWrite8 (ICH9_APM_CNT, CommandPort == NULL ? 0 : *CommandPort);
   return EFI_SUCCESS;
 }
-- 
2.9.2

[PATCH 2/3] OvmfPkg: revert "any AP in SMM should not wait for the BSP for more than 100 ms"

[Laszlo Ersek]

This reverts commit bb0f18b0bce682f6780af997de45bcef146c11ca ("OvmfPkg:
any AP in SMM should not wait for the BSP for more than 100 ms"). That
commit said,

> NOTE: once QEMU becomes capable of synchronous broadcast SMIs, this
> patch and the previous one ("OvmfPkg: use relaxed AP SMM synchronization
> mode") should be reverted, and SmmControl2DxeTrigger() should be
> adjusted instead.

Cc: Jordan Justen <jordan.l.justen@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>
---
 OvmfPkg/OvmfPkgIa32.dsc    | 1 -
 OvmfPkg/OvmfPkgIa32X64.dsc | 1 -
 OvmfPkg/OvmfPkgX64.dsc     | 1 -
 3 files changed, 3 deletions(-)

diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
index 3f4d42d85642..422b73c22c0e 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -434,7 +434,6 @@ [PcdsFixedAtBuild]
 
 !if $(SMM_REQUIRE) == TRUE
   gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode|0x01
-  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmApSyncTimeout|100000
   gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmStackSize|0x4000
 !endif
 
diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index 568847531a56..c04486537ef0 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -440,7 +440,6 @@ [PcdsFixedAtBuild.X64]
 
 !if $(SMM_REQUIRE) == TRUE
   gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode|0x01
-  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmApSyncTimeout|100000
   gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmStackSize|0x4000
 !endif
 
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index dcf64b9d6aef..230a26ba576a 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -439,7 +439,6 @@ [PcdsFixedAtBuild]
 
 !if $(SMM_REQUIRE) == TRUE
   gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode|0x01
-  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmApSyncTimeout|100000
   gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmStackSize|0x4000
 !endif
 
-- 
2.9.2

[PATCH 3/3] OvmfPkg: revert "use relaxed AP SMM synchronization mode"

[Laszlo Ersek]

This reverts commit 9b1e378811ff730fe4e5bbbba294ea74ef76a915 ("OvmfPkg:
use relaxed AP SMM synchronization mode").

With QEMU's broadcast SMI ability, that patch is superfluous. Reverting
the AP sync mode to the "traditional" default even eliminates some obscure
corner case issues; see the discussion rooted at
<https://lists.01.org/pipermail/edk2-devel/2016-November/004487.html>.

Cc: Jordan Justen <jordan.l.justen@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>
---
 OvmfPkg/OvmfPkgIa32.dsc    | 1 -
 OvmfPkg/OvmfPkgIa32X64.dsc | 1 -
 OvmfPkg/OvmfPkgX64.dsc     | 1 -
 3 files changed, 3 deletions(-)

diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
index 422b73c22c0e..6f3b0234f2cc 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -433,7 +433,6 @@ [PcdsFixedAtBuild]
 !endif
 
 !if $(SMM_REQUIRE) == TRUE
-  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode|0x01
   gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmStackSize|0x4000
 !endif
 
diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index c04486537ef0..729f81511720 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -439,7 +439,6 @@ [PcdsFixedAtBuild.X64]
 !endif
 
 !if $(SMM_REQUIRE) == TRUE
-  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode|0x01
   gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmStackSize|0x4000
 !endif
 
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index 230a26ba576a..c440fc1b264e 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -438,7 +438,6 @@ [PcdsFixedAtBuild]
 !endif
 
 !if $(SMM_REQUIRE) == TRUE
-  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode|0x01
   gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmStackSize|0x4000
 !endif
 
-- 
2.9.2

Re: [PATCH 0/3] OvmfPkg: broadcast SMIs and revert to traditional AP sync mode

[Laszlo Ersek]

On 11/15/16 03:43, Laszlo Ersek wrote:
> Spurred by Jiewen's and Jeff's recent SMM-related work, Paolo and I
> stress-tested OVMF's SMM a little bit, and with S3 it turns out to be
> less stable than ideal.
> 
> Paolo agreed / suggested that we add a broadcast SMI mode to QEMU, and
> utilize it from OVMF:
> 
>   https://www.mail-archive.com/edk2-devel@lists.01.org/msg19669.html
> 
> I sent the QEMU patch a few minutes (or hours?) ago:
> 
>   http://lists.nongnu.org/archive/html/qemu-devel/2016-11/msg02687.html
> 
> and these are the OVMF patches that put the QEMU functionality to use.
> 
> With these patches (and with Jeff's just-committed series for
> <https://bugzilla.tianocore.org/show_bug.cgi?id=216>) I see stable and
> reliable behavior with SMM+S3. Please see
> 
>   https://lists.01.org/pipermail/edk2-devel/2016-November/004682.html
> 
> for details.
> 
> Repo:   https://github.com/lersek/edk2/
> Branch: broadcast_smi
> BZ:     https://bugzilla.tianocore.org/show_bug.cgi?id=230
> 
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> 
> Thanks
> Laszlo
> 
> Laszlo Ersek (3):
>   OvmfPkg/SmmControl2Dxe: select broadcast SMI
>   OvmfPkg: revert "any AP in SMM should not wait for the BSP for more
>     than 100 ms"
>   OvmfPkg: revert "use relaxed AP SMM synchronization mode"
> 
>  OvmfPkg/OvmfPkgIa32.dsc                       |  2 --
>  OvmfPkg/OvmfPkgIa32X64.dsc                    |  2 --
>  OvmfPkg/OvmfPkgX64.dsc                        |  2 --
>  OvmfPkg/Include/IndustryStandard/Q35MchIch9.h |  6 ++++--
>  OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c       | 18 +++++++++++++++++-
>  5 files changed, 21 insertions(+), 9 deletions(-)
> 

Self-NAK, will send a new version.