* [PATCH wave 1 00/10] support QEMU's "SMRAM at default SMBASE" feature
@ 2019-09-24 11:34 Laszlo Ersek
2019-09-24 11:34 ` [PATCH wave 1 01/10] OvmfPkg: introduce PcdQ35SmramAtDefaultSmbase Laszlo Ersek
` (11 more replies)
0 siblings, 12 replies; 21+ messages in thread
From: Laszlo Ersek @ 2019-09-24 11:34 UTC (permalink / raw)
To: edk2-devel-groups-io
Cc: Ard Biesheuvel, Boris Ostrovsky, Brijesh Singh, Igor Mammedov,
Jiewen Yao, Joao M Martins, Jordan Justen, Jun Nakajima,
Michael Kinney, Paolo Bonzini, Phillip Goerl, Yingwen Chen
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1512
Repo: https://github.com/lersek/edk2.git
Branch: smram_at_default_smbase_bz_1512_wave_1
This is the firmware-side patch series for the QEMU feature that Igor is
proposing in
[Qemu-devel] [PATCH 0/2]
q35: mch: allow to lock down 128K RAM at default SMBASE address
posted at
http://mid.mail-archive.com/20190917130708.10281-1-imammedo@redhat.com
https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg03538.html
https://edk2.groups.io/g/devel/message/47364
This OVMF patch series is supposed to be the first "wave" of patch sets,
working towards VCPU hotplug with SMM enabled.
We want the hot-plugged VCPU to execute its very first instruction from
SMRAM, running in SMM, for protection from OS and PCI DMA interference.
In the (long!) discussion at
http://mid.mail-archive.com/20190905174503.2acaa46a@redhat.com
https://edk2.groups.io/g/devel/message/46910
https://bugzilla.tianocore.org/show_bug.cgi?id=1512#c9
we considered diverging from the specs and changing the default SMBASE
to an address different from 0x3_0000, so that it would point into
SMRAM. However, Igor had the idea to keep the default SMBASE intact, and
replace the underlying system RAM (128 KB of it) with lockable SMRAM.
This conforms to the language of the Intel SDM, namely, "[t]he actual
physical location of the SMRAM can be in system memory or in a separate
RAM memory". When firmware locks this QEMU-specific SMRAM, the latter
completely disappears from the address space of code that runs outside
of SMM (including PCI devices doing DMA). We call the remaining MMIO
area a "black hole".
The present patch set detects the QEMU feature, locks the SMRAM at the
right times, and informs firmware and OS components to stay away form
the SMRAM at the default SMBASE.
I've tested the set extensively:
http://mid.mail-archive.com/c18f1ada-3eca-d5f1-da4f-e74181c5a862@redhat.com
https://edk2.groups.io/g/devel/message/47864
Going forward, if I understand correctly, the plan is to populate the
new SMRAM with the hotplug SMI handler. (This would likely be put in
place by OvmfPkg/PlatformPei, from NASM source code. The
SmmRelocateBases() function in UefiCpuPkg/PiSmmCpuDxeSmm backs up the
original contents before, and restores them after, the initial SMBASE
relocation of the cold-plugged VCPUs. Thus the hotplug SMI handler would
survive the initial relocation of the cold-plugged VCPUs.)
Further, when a VCPU is hot-plugged, we seem to intend to raise an SMI
for it (perhaps internally to QEMU?), which will remain pending until
the VCPU is launched with INIT-SIPI-SIPI. Once the INIT-SIPI-SIPI is
sent, the VCPU will immediately enter SMM, and run the SMI handler in
the locked SMRAM at the default SMBASE. At RSM, it may continue at the
vector requested in the INIT-SIPI-SIPI.
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Joao M Martins <joao.m.martins@oracle.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Jun Nakajima <jun.nakajima@intel.com>
Cc: Michael Kinney <michael.d.kinney@intel.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Phillip Goerl <phillip.goerl@oracle.com>
Cc: Yingwen Chen <yingwen.chen@intel.com>
Thanks
Laszlo
Laszlo Ersek (10):
OvmfPkg: introduce PcdQ35SmramAtDefaultSmbase
OvmfPkg/IndustryStandard: increase vertical whitespace in Q35 macro
defs
OvmfPkg/IndustryStandard: add MCH_DEFAULT_SMBASE* register macros
OvmfPkg/PlatformPei: factor out Q35BoardVerification()
OvmfPkg/PlatformPei: detect SMRAM at default SMBASE (skeleton)
OvmfPkg/PlatformPei: assert there's no permanent PEI RAM at default
SMBASE
OvmfPkg/PlatformPei: reserve the SMRAM at the default SMBASE, if it
exists
OvmfPkg/SEV: don't manage the lifecycle of the SMRAM at the default
SMBASE
OvmfPkg/SmmAccess: close and lock SMRAM at default SMBASE
OvmfPkg/PlatformPei: detect SMRAM at default SMBASE (for real)
OvmfPkg/Include/IndustryStandard/Q35MchIch9.h | 106 +++++++++++---------
OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c | 21 +++-
OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf | 4 +
OvmfPkg/OvmfPkg.dec | 6 ++
OvmfPkg/OvmfPkgIa32.dsc | 1 +
OvmfPkg/OvmfPkgIa32X64.dsc | 1 +
OvmfPkg/OvmfPkgX64.dsc | 1 +
OvmfPkg/PlatformPei/AmdSev.c | 24 ++++-
OvmfPkg/PlatformPei/MemDetect.c | 87 +++++++++++++---
OvmfPkg/PlatformPei/Platform.c | 24 +++++
OvmfPkg/PlatformPei/Platform.h | 7 ++
OvmfPkg/PlatformPei/PlatformPei.inf | 1 +
OvmfPkg/SmmAccess/SmmAccess2Dxe.c | 7 ++
OvmfPkg/SmmAccess/SmmAccess2Dxe.inf | 1 +
OvmfPkg/SmmAccess/SmmAccessPei.c | 6 ++
OvmfPkg/SmmAccess/SmmAccessPei.inf | 1 +
OvmfPkg/SmmAccess/SmramInternal.c | 25 +++++
OvmfPkg/SmmAccess/SmramInternal.h | 8 ++
18 files changed, 260 insertions(+), 71 deletions(-)
--
2.19.1.3.g30247aa5d201
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH wave 1 01/10] OvmfPkg: introduce PcdQ35SmramAtDefaultSmbase
2019-09-24 11:34 [PATCH wave 1 00/10] support QEMU's "SMRAM at default SMBASE" feature Laszlo Ersek
@ 2019-09-24 11:34 ` Laszlo Ersek
2019-09-24 11:34 ` [PATCH wave 1 02/10] OvmfPkg/IndustryStandard: increase vertical whitespace in Q35 macro defs Laszlo Ersek
` (10 subsequent siblings)
11 siblings, 0 replies; 21+ messages in thread
From: Laszlo Ersek @ 2019-09-24 11:34 UTC (permalink / raw)
To: edk2-devel-groups-io
Cc: Ard Biesheuvel, Boris Ostrovsky, Brijesh Singh, Igor Mammedov,
Jiewen Yao, Joao M Martins, Jordan Justen, Jun Nakajima,
Michael Kinney, Paolo Bonzini, Phillip Goerl, Yingwen Chen
For supporting VCPU hotplug with SMM enabled/required, QEMU offers the
(dynamically detectable) feature called "SMRAM at default SMBASE". When
the feature is enabled, the firmware can lock down the 128 KB range
starting at the default SMBASE; that is, the [0x3_0000, 0x4_FFFF]
interval. The goal is to shield the very first SMI handler of the
hotplugged VCPU from OS influence.
Multiple modules in OVMF will have to inter-operate for locking down this
range. Introduce a dynamic PCD that will reflect the feature (to be
negotiated by PlatformPei), for coordination between drivers.
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Joao M Martins <joao.m.martins@oracle.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Jun Nakajima <jun.nakajima@intel.com>
Cc: Michael Kinney <michael.d.kinney@intel.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Phillip Goerl <phillip.goerl@oracle.com>
Cc: Yingwen Chen <yingwen.chen@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1512
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
OvmfPkg/OvmfPkg.dec | 6 ++++++
OvmfPkg/OvmfPkgIa32.dsc | 1 +
OvmfPkg/OvmfPkgIa32X64.dsc | 1 +
OvmfPkg/OvmfPkgX64.dsc | 1 +
4 files changed, 9 insertions(+)
diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
index d5fee805ef4a..57034c784f88 100644
--- a/OvmfPkg/OvmfPkg.dec
+++ b/OvmfPkg/OvmfPkg.dec
@@ -257,6 +257,12 @@ [PcdsDynamic, PcdsDynamicEx]
# This PCD is only accessed if PcdSmmSmramRequire is TRUE (see below).
gUefiOvmfPkgTokenSpaceGuid.PcdQ35TsegMbytes|8|UINT16|0x20
+ ## Set to TRUE by PlatformPei if the Q35 board supports the "SMRAM at default
+ # SMBASE" feature.
+ #
+ # This PCD is only accessed if PcdSmmSmramRequire is TRUE (see below).
+ gUefiOvmfPkgTokenSpaceGuid.PcdQ35SmramAtDefaultSmbase|FALSE|BOOLEAN|0x34
+
[PcdsFeatureFlag]
gUefiOvmfPkgTokenSpaceGuid.PcdQemuBootOrderPciTranslation|TRUE|BOOLEAN|0x1c
gUefiOvmfPkgTokenSpaceGuid.PcdQemuBootOrderMmioTranslation|FALSE|BOOLEAN|0x1d
diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
index 66e944436a69..8a970a40cc62 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -561,6 +561,7 @@ [PcdsDynamicDefault]
!if $(SMM_REQUIRE) == TRUE
gUefiOvmfPkgTokenSpaceGuid.PcdQ35TsegMbytes|8
+ gUefiOvmfPkgTokenSpaceGuid.PcdQ35SmramAtDefaultSmbase|FALSE
gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode|0x01
gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmApSyncTimeout|100000
!endif
diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index 51c2bfb44f14..2a3ba9ec2ca1 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -573,6 +573,7 @@ [PcdsDynamicDefault]
!if $(SMM_REQUIRE) == TRUE
gUefiOvmfPkgTokenSpaceGuid.PcdQ35TsegMbytes|8
+ gUefiOvmfPkgTokenSpaceGuid.PcdQ35SmramAtDefaultSmbase|FALSE
gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode|0x01
gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmApSyncTimeout|100000
!endif
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index ba7a75884490..17a135a8edfe 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -572,6 +572,7 @@ [PcdsDynamicDefault]
!if $(SMM_REQUIRE) == TRUE
gUefiOvmfPkgTokenSpaceGuid.PcdQ35TsegMbytes|8
+ gUefiOvmfPkgTokenSpaceGuid.PcdQ35SmramAtDefaultSmbase|FALSE
gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode|0x01
gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmApSyncTimeout|100000
!endif
--
2.19.1.3.g30247aa5d201
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH wave 1 02/10] OvmfPkg/IndustryStandard: increase vertical whitespace in Q35 macro defs
2019-09-24 11:34 [PATCH wave 1 00/10] support QEMU's "SMRAM at default SMBASE" feature Laszlo Ersek
2019-09-24 11:34 ` [PATCH wave 1 01/10] OvmfPkg: introduce PcdQ35SmramAtDefaultSmbase Laszlo Ersek
@ 2019-09-24 11:34 ` Laszlo Ersek
2019-09-24 11:44 ` [edk2-devel] " Philippe Mathieu-Daudé
2019-09-24 11:34 ` [PATCH wave 1 03/10] OvmfPkg/IndustryStandard: add MCH_DEFAULT_SMBASE* register macros Laszlo Ersek
` (9 subsequent siblings)
11 siblings, 1 reply; 21+ messages in thread
From: Laszlo Ersek @ 2019-09-24 11:34 UTC (permalink / raw)
To: edk2-devel-groups-io
Cc: Ard Biesheuvel, Boris Ostrovsky, Brijesh Singh, Igor Mammedov,
Jiewen Yao, Joao M Martins, Jordan Justen, Jun Nakajima,
Michael Kinney, Paolo Bonzini, Phillip Goerl, Yingwen Chen
In a subsequent patch, we'll introduce new DRAM controller macros in
"Q35MchIch9.h". Their names are too long for the currently available
vertical whitespace, so increase the latter first.
There is no functional change in this patch ("git show -b" displays
nothing).
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Joao M Martins <joao.m.martins@oracle.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Jun Nakajima <jun.nakajima@intel.com>
Cc: Michael Kinney <michael.d.kinney@intel.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Phillip Goerl <phillip.goerl@oracle.com>
Cc: Yingwen Chen <yingwen.chen@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1512
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
OvmfPkg/Include/IndustryStandard/Q35MchIch9.h | 100 ++++++++++----------
1 file changed, 50 insertions(+), 50 deletions(-)
diff --git a/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h b/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h
index 391cb4622226..614699ab38f1 100644
--- a/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h
+++ b/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h
@@ -27,56 +27,56 @@
//
#define DRAMC_REGISTER_Q35(Offset) PCI_LIB_ADDRESS (0, 0, 0, (Offset))
-#define MCH_EXT_TSEG_MB 0x50
-#define MCH_EXT_TSEG_MB_QUERY 0xFFFF
-
-#define MCH_GGC 0x52
-#define MCH_GGC_IVD BIT1
-
-#define MCH_PCIEXBAR_LOW 0x60
-#define MCH_PCIEXBAR_LOWMASK 0x0FFFFFFF
-#define MCH_PCIEXBAR_BUS_FF 0
-#define MCH_PCIEXBAR_EN BIT0
-
-#define MCH_PCIEXBAR_HIGH 0x64
-#define MCH_PCIEXBAR_HIGHMASK 0xFFFFFFF0
-
-#define MCH_PAM0 0x90
-#define MCH_PAM1 0x91
-#define MCH_PAM2 0x92
-#define MCH_PAM3 0x93
-#define MCH_PAM4 0x94
-#define MCH_PAM5 0x95
-#define MCH_PAM6 0x96
-
-#define MCH_SMRAM 0x9D
-#define MCH_SMRAM_D_LCK BIT4
-#define MCH_SMRAM_G_SMRAME BIT3
-
-#define MCH_ESMRAMC 0x9E
-#define MCH_ESMRAMC_H_SMRAME BIT7
-#define MCH_ESMRAMC_E_SMERR BIT6
-#define MCH_ESMRAMC_SM_CACHE BIT5
-#define MCH_ESMRAMC_SM_L1 BIT4
-#define MCH_ESMRAMC_SM_L2 BIT3
-#define MCH_ESMRAMC_TSEG_EXT (BIT2 | BIT1)
-#define MCH_ESMRAMC_TSEG_8MB BIT2
-#define MCH_ESMRAMC_TSEG_2MB BIT1
-#define MCH_ESMRAMC_TSEG_1MB 0
-#define MCH_ESMRAMC_TSEG_MASK (BIT2 | BIT1)
-#define MCH_ESMRAMC_T_EN BIT0
-
-#define MCH_GBSM 0xA4
-#define MCH_GBSM_MB_SHIFT 20
-
-#define MCH_BGSM 0xA8
-#define MCH_BGSM_MB_SHIFT 20
-
-#define MCH_TSEGMB 0xAC
-#define MCH_TSEGMB_MB_SHIFT 20
-
-#define MCH_TOLUD 0xB0
-#define MCH_TOLUD_MB_SHIFT 4
+#define MCH_EXT_TSEG_MB 0x50
+#define MCH_EXT_TSEG_MB_QUERY 0xFFFF
+
+#define MCH_GGC 0x52
+#define MCH_GGC_IVD BIT1
+
+#define MCH_PCIEXBAR_LOW 0x60
+#define MCH_PCIEXBAR_LOWMASK 0x0FFFFFFF
+#define MCH_PCIEXBAR_BUS_FF 0
+#define MCH_PCIEXBAR_EN BIT0
+
+#define MCH_PCIEXBAR_HIGH 0x64
+#define MCH_PCIEXBAR_HIGHMASK 0xFFFFFFF0
+
+#define MCH_PAM0 0x90
+#define MCH_PAM1 0x91
+#define MCH_PAM2 0x92
+#define MCH_PAM3 0x93
+#define MCH_PAM4 0x94
+#define MCH_PAM5 0x95
+#define MCH_PAM6 0x96
+
+#define MCH_SMRAM 0x9D
+#define MCH_SMRAM_D_LCK BIT4
+#define MCH_SMRAM_G_SMRAME BIT3
+
+#define MCH_ESMRAMC 0x9E
+#define MCH_ESMRAMC_H_SMRAME BIT7
+#define MCH_ESMRAMC_E_SMERR BIT6
+#define MCH_ESMRAMC_SM_CACHE BIT5
+#define MCH_ESMRAMC_SM_L1 BIT4
+#define MCH_ESMRAMC_SM_L2 BIT3
+#define MCH_ESMRAMC_TSEG_EXT (BIT2 | BIT1)
+#define MCH_ESMRAMC_TSEG_8MB BIT2
+#define MCH_ESMRAMC_TSEG_2MB BIT1
+#define MCH_ESMRAMC_TSEG_1MB 0
+#define MCH_ESMRAMC_TSEG_MASK (BIT2 | BIT1)
+#define MCH_ESMRAMC_T_EN BIT0
+
+#define MCH_GBSM 0xA4
+#define MCH_GBSM_MB_SHIFT 20
+
+#define MCH_BGSM 0xA8
+#define MCH_BGSM_MB_SHIFT 20
+
+#define MCH_TSEGMB 0xAC
+#define MCH_TSEGMB_MB_SHIFT 20
+
+#define MCH_TOLUD 0xB0
+#define MCH_TOLUD_MB_SHIFT 4
//
// B/D/F/Type: 0/0x1f/0/PCI
--
2.19.1.3.g30247aa5d201
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH wave 1 03/10] OvmfPkg/IndustryStandard: add MCH_DEFAULT_SMBASE* register macros
2019-09-24 11:34 [PATCH wave 1 00/10] support QEMU's "SMRAM at default SMBASE" feature Laszlo Ersek
2019-09-24 11:34 ` [PATCH wave 1 01/10] OvmfPkg: introduce PcdQ35SmramAtDefaultSmbase Laszlo Ersek
2019-09-24 11:34 ` [PATCH wave 1 02/10] OvmfPkg/IndustryStandard: increase vertical whitespace in Q35 macro defs Laszlo Ersek
@ 2019-09-24 11:34 ` Laszlo Ersek
2019-09-24 11:34 ` [PATCH wave 1 04/10] OvmfPkg/PlatformPei: factor out Q35BoardVerification() Laszlo Ersek
` (8 subsequent siblings)
11 siblings, 0 replies; 21+ messages in thread
From: Laszlo Ersek @ 2019-09-24 11:34 UTC (permalink / raw)
To: edk2-devel-groups-io
Cc: Ard Biesheuvel, Boris Ostrovsky, Brijesh Singh, Igor Mammedov,
Jiewen Yao, Joao M Martins, Jordan Justen, Jun Nakajima,
Michael Kinney, Paolo Bonzini, Phillip Goerl, Yingwen Chen
In Intel datasheet 316966-002 (the "q35 spec"), Table 5-1 "DRAM Controller
Register Address Map (D0:F0)" leaves the byte register at config space
offset 0x9C unused.
On QEMU's Q35 board, for detecting the "SMRAM at default SMBASE" feature,
firmware is expected to write MCH_DEFAULT_SMBASE_QUERY (0xFF) to offset
MCH_DEFAULT_SMBASE_CTL (0x9C), and read back the register. If the value is
MCH_DEFAULT_SMBASE_IN_RAM (0x01), then the feature is available, and the
range mentioned below is open (accessible to code running outside of SMM).
Then, once firmware writes MCH_DEFAULT_SMBASE_LCK (0x02) to the register,
the MCH_DEFAULT_SMBASE_SIZE (128KB) range at 0x3_0000 (SMM_DEFAULT_SMBASE)
gets closed and locked down, and the register becomes read-only. The area
is reopened, and the register becomes read/write, at platform reset.
Add the above-listed macros to "Q35MchIch9.h".
(There are some other unused offsets in Table 5-1; for example we had
scavenged 0x50 for implementing the extended TSEG feature. 0x9C is the
first byte-wide register standing in isolation after 0x50.)
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Joao M Martins <joao.m.martins@oracle.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Jun Nakajima <jun.nakajima@intel.com>
Cc: Michael Kinney <michael.d.kinney@intel.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Phillip Goerl <phillip.goerl@oracle.com>
Cc: Yingwen Chen <yingwen.chen@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1512
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
OvmfPkg/Include/IndustryStandard/Q35MchIch9.h | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h b/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h
index 614699ab38f1..eac57b10d77e 100644
--- a/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h
+++ b/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h
@@ -49,6 +49,12 @@
#define MCH_PAM5 0x95
#define MCH_PAM6 0x96
+#define MCH_DEFAULT_SMBASE_CTL 0x9C
+#define MCH_DEFAULT_SMBASE_QUERY 0xFF
+#define MCH_DEFAULT_SMBASE_IN_RAM 0x01
+#define MCH_DEFAULT_SMBASE_LCK 0x02
+#define MCH_DEFAULT_SMBASE_SIZE SIZE_128KB
+
#define MCH_SMRAM 0x9D
#define MCH_SMRAM_D_LCK BIT4
#define MCH_SMRAM_G_SMRAME BIT3
--
2.19.1.3.g30247aa5d201
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH wave 1 04/10] OvmfPkg/PlatformPei: factor out Q35BoardVerification()
2019-09-24 11:34 [PATCH wave 1 00/10] support QEMU's "SMRAM at default SMBASE" feature Laszlo Ersek
` (2 preceding siblings ...)
2019-09-24 11:34 ` [PATCH wave 1 03/10] OvmfPkg/IndustryStandard: add MCH_DEFAULT_SMBASE* register macros Laszlo Ersek
@ 2019-09-24 11:34 ` Laszlo Ersek
2019-09-24 11:41 ` [edk2-devel] " Philippe Mathieu-Daudé
2019-09-24 11:35 ` [PATCH wave 1 05/10] OvmfPkg/PlatformPei: detect SMRAM at default SMBASE (skeleton) Laszlo Ersek
` (7 subsequent siblings)
11 siblings, 1 reply; 21+ messages in thread
From: Laszlo Ersek @ 2019-09-24 11:34 UTC (permalink / raw)
To: edk2-devel-groups-io
Cc: Ard Biesheuvel, Boris Ostrovsky, Brijesh Singh, Igor Mammedov,
Jiewen Yao, Joao M Martins, Jordan Justen, Jun Nakajima,
Michael Kinney, Paolo Bonzini, Phillip Goerl, Yingwen Chen
Before adding another SMM-related, and therefore Q35-only, dynamically
detectable feature, extract the current board type check from
Q35TsegMbytesInitialization() to a standalone function.
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Joao M Martins <joao.m.martins@oracle.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Jun Nakajima <jun.nakajima@intel.com>
Cc: Michael Kinney <michael.d.kinney@intel.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Phillip Goerl <phillip.goerl@oracle.com>
Cc: Yingwen Chen <yingwen.chen@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1512
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
OvmfPkg/PlatformPei/MemDetect.c | 13 +----------
OvmfPkg/PlatformPei/Platform.c | 23 ++++++++++++++++++++
2 files changed, 24 insertions(+), 12 deletions(-)
diff --git a/OvmfPkg/PlatformPei/MemDetect.c b/OvmfPkg/PlatformPei/MemDetect.c
index d451989f31c9..58b171fba1c8 100644
--- a/OvmfPkg/PlatformPei/MemDetect.c
+++ b/OvmfPkg/PlatformPei/MemDetect.c
@@ -53,18 +53,7 @@ Q35TsegMbytesInitialization (
UINT16 ExtendedTsegMbytes;
RETURN_STATUS PcdStatus;
- if (mHostBridgeDevId != INTEL_Q35_MCH_DEVICE_ID) {
- DEBUG ((
- DEBUG_ERROR,
- "%a: no TSEG (SMRAM) on host bridge DID=0x%04x; "
- "only DID=0x%04x (Q35) is supported\n",
- __FUNCTION__,
- mHostBridgeDevId,
- INTEL_Q35_MCH_DEVICE_ID
- ));
- ASSERT (FALSE);
- CpuDeadLoop ();
- }
+ ASSERT (mHostBridgeDevId == INTEL_Q35_MCH_DEVICE_ID);
//
// Check if QEMU offers an extended TSEG.
diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
index 3ba2459872d9..ca6d37cb1549 100644
--- a/OvmfPkg/PlatformPei/Platform.c
+++ b/OvmfPkg/PlatformPei/Platform.c
@@ -563,6 +563,28 @@ S3Verification (
}
+VOID
+Q35BoardVerification (
+ VOID
+ )
+{
+ if (mHostBridgeDevId == INTEL_Q35_MCH_DEVICE_ID) {
+ return;
+ }
+
+ DEBUG ((
+ DEBUG_ERROR,
+ "%a: no TSEG (SMRAM) on host bridge DID=0x%04x; "
+ "only DID=0x%04x (Q35) is supported\n",
+ __FUNCTION__,
+ mHostBridgeDevId,
+ INTEL_Q35_MCH_DEVICE_ID
+ ));
+ ASSERT (FALSE);
+ CpuDeadLoop ();
+}
+
+
/**
Fetch the number of boot CPUs from QEMU and expose it to UefiCpuPkg modules.
Set the mMaxCpuCount variable.
@@ -646,6 +668,7 @@ InitializePlatform (
mHostBridgeDevId = PciRead16 (OVMF_HOSTBRIDGE_DID);
if (FeaturePcdGet (PcdSmmSmramRequire)) {
+ Q35BoardVerification ();
Q35TsegMbytesInitialization ();
}
--
2.19.1.3.g30247aa5d201
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH wave 1 05/10] OvmfPkg/PlatformPei: detect SMRAM at default SMBASE (skeleton)
2019-09-24 11:34 [PATCH wave 1 00/10] support QEMU's "SMRAM at default SMBASE" feature Laszlo Ersek
` (3 preceding siblings ...)
2019-09-24 11:34 ` [PATCH wave 1 04/10] OvmfPkg/PlatformPei: factor out Q35BoardVerification() Laszlo Ersek
@ 2019-09-24 11:35 ` Laszlo Ersek
2019-09-24 11:35 ` [PATCH wave 1 06/10] OvmfPkg/PlatformPei: assert there's no permanent PEI RAM at default SMBASE Laszlo Ersek
` (6 subsequent siblings)
11 siblings, 0 replies; 21+ messages in thread
From: Laszlo Ersek @ 2019-09-24 11:35 UTC (permalink / raw)
To: edk2-devel-groups-io
Cc: Ard Biesheuvel, Boris Ostrovsky, Brijesh Singh, Igor Mammedov,
Jiewen Yao, Joao M Martins, Jordan Justen, Jun Nakajima,
Michael Kinney, Paolo Bonzini, Phillip Goerl, Yingwen Chen
Introduce the Q35SmramAtDefaultSmbaseInitialization() function for
detecting the "SMRAM at default SMBASE" feature.
For now, the function is only a skeleton, so that we can gradually build
upon the result while the result is hard-coded as FALSE. The actual
detection will occur in a later patch.
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Joao M Martins <joao.m.martins@oracle.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Jun Nakajima <jun.nakajima@intel.com>
Cc: Michael Kinney <michael.d.kinney@intel.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Phillip Goerl <phillip.goerl@oracle.com>
Cc: Yingwen Chen <yingwen.chen@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1512
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
OvmfPkg/PlatformPei/PlatformPei.inf | 1 +
OvmfPkg/PlatformPei/Platform.h | 7 +++++++
OvmfPkg/PlatformPei/MemDetect.c | 18 ++++++++++++++++++
OvmfPkg/PlatformPei/Platform.c | 1 +
4 files changed, 27 insertions(+)
diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf b/OvmfPkg/PlatformPei/PlatformPei.inf
index d9fd9c8f05b3..2b0c2c886929 100644
--- a/OvmfPkg/PlatformPei/PlatformPei.inf
+++ b/OvmfPkg/PlatformPei/PlatformPei.inf
@@ -84,6 +84,7 @@ [Pcd]
gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio64Size
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfDecompressionScratchEnd
gUefiOvmfPkgTokenSpaceGuid.PcdQ35TsegMbytes
+ gUefiOvmfPkgTokenSpaceGuid.PcdQ35SmramAtDefaultSmbase
gEfiMdePkgTokenSpaceGuid.PcdGuidedExtractHandlerTableAddress
gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize
gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize
diff --git a/OvmfPkg/PlatformPei/Platform.h b/OvmfPkg/PlatformPei/Platform.h
index 2f3cebcd3a6a..43f20f067f22 100644
--- a/OvmfPkg/PlatformPei/Platform.h
+++ b/OvmfPkg/PlatformPei/Platform.h
@@ -52,6 +52,11 @@ Q35TsegMbytesInitialization (
VOID
);
+VOID
+Q35SmramAtDefaultSmbaseInitialization (
+ VOID
+ );
+
EFI_STATUS
PublishPeiMemory (
VOID
@@ -119,6 +124,8 @@ extern UINT32 mMaxCpuCount;
extern UINT16 mHostBridgeDevId;
+extern BOOLEAN mQ35SmramAtDefaultSmbase;
+
extern UINT32 mQemuUc32Base;
#endif // _PLATFORM_PEI_H_INCLUDED_
diff --git a/OvmfPkg/PlatformPei/MemDetect.c b/OvmfPkg/PlatformPei/MemDetect.c
index 58b171fba1c8..2bc1c46dffc2 100644
--- a/OvmfPkg/PlatformPei/MemDetect.c
+++ b/OvmfPkg/PlatformPei/MemDetect.c
@@ -43,6 +43,8 @@ STATIC UINT32 mS3AcpiReservedMemorySize;
STATIC UINT16 mQ35TsegMbytes;
+BOOLEAN mQ35SmramAtDefaultSmbase;
+
UINT32 mQemuUc32Base;
VOID
@@ -90,6 +92,22 @@ Q35TsegMbytesInitialization (
}
+VOID
+Q35SmramAtDefaultSmbaseInitialization (
+ VOID
+ )
+{
+ RETURN_STATUS PcdStatus;
+
+ ASSERT (mHostBridgeDevId == INTEL_Q35_MCH_DEVICE_ID);
+
+ mQ35SmramAtDefaultSmbase = FALSE;
+ PcdStatus = PcdSetBoolS (PcdQ35SmramAtDefaultSmbase,
+ mQ35SmramAtDefaultSmbase);
+ ASSERT_RETURN_ERROR (PcdStatus);
+}
+
+
VOID
QemuUc32BaseInitialization (
VOID
diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
index ca6d37cb1549..6989712b0645 100644
--- a/OvmfPkg/PlatformPei/Platform.c
+++ b/OvmfPkg/PlatformPei/Platform.c
@@ -670,6 +670,7 @@ InitializePlatform (
if (FeaturePcdGet (PcdSmmSmramRequire)) {
Q35BoardVerification ();
Q35TsegMbytesInitialization ();
+ Q35SmramAtDefaultSmbaseInitialization ();
}
PublishPeiMemory ();
--
2.19.1.3.g30247aa5d201
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH wave 1 06/10] OvmfPkg/PlatformPei: assert there's no permanent PEI RAM at default SMBASE
2019-09-24 11:34 [PATCH wave 1 00/10] support QEMU's "SMRAM at default SMBASE" feature Laszlo Ersek
` (4 preceding siblings ...)
2019-09-24 11:35 ` [PATCH wave 1 05/10] OvmfPkg/PlatformPei: detect SMRAM at default SMBASE (skeleton) Laszlo Ersek
@ 2019-09-24 11:35 ` Laszlo Ersek
2019-09-24 11:35 ` [PATCH wave 1 07/10] OvmfPkg/PlatformPei: reserve the SMRAM at the default SMBASE, if it exists Laszlo Ersek
` (5 subsequent siblings)
11 siblings, 0 replies; 21+ messages in thread
From: Laszlo Ersek @ 2019-09-24 11:35 UTC (permalink / raw)
To: edk2-devel-groups-io
Cc: Ard Biesheuvel, Boris Ostrovsky, Brijesh Singh, Igor Mammedov,
Jiewen Yao, Joao M Martins, Jordan Justen, Jun Nakajima,
Michael Kinney, Paolo Bonzini, Phillip Goerl, Yingwen Chen
The permanent PEI RAM that is published on the normal boot path starts
strictly above MEMFD_BASE_ADDRESS (8 MB -- see the FDF files), regardless
of whether PEI decompression will be necessary on S3 resume due to
SMM_REQUIRE. Therefore the normal boot permanent PEI RAM never overlaps
with the SMRAM at the default SMBASE (192 KB).
The S3 resume permanent PEI RAM is strictly above the normal boot one.
Therefore the no-overlap statement holds true on the S3 resume path as
well.
Assert the no-overlap condition commonly for both boot paths in
PublishPeiMemory().
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Joao M Martins <joao.m.martins@oracle.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Jun Nakajima <jun.nakajima@intel.com>
Cc: Michael Kinney <michael.d.kinney@intel.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Phillip Goerl <phillip.goerl@oracle.com>
Cc: Yingwen Chen <yingwen.chen@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1512
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
OvmfPkg/PlatformPei/MemDetect.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/OvmfPkg/PlatformPei/MemDetect.c b/OvmfPkg/PlatformPei/MemDetect.c
index 2bc1c46dffc2..4879ee87b797 100644
--- a/OvmfPkg/PlatformPei/MemDetect.c
+++ b/OvmfPkg/PlatformPei/MemDetect.c
@@ -17,6 +17,7 @@ Module Name:
#include <IndustryStandard/I440FxPiix4.h>
#include <IndustryStandard/Q35MchIch9.h>
#include <PiPei.h>
+#include <Register/Intel/SmramSaveStateMap.h>
//
// The Library classes this module consumes
@@ -626,6 +627,15 @@ PublishPeiMemory (
}
}
+ //
+ // MEMFD_BASE_ADDRESS separates the SMRAM at the default SMBASE from the
+ // normal boot permanent PEI RAM. Regarding the S3 boot path, the S3
+ // permanent PEI RAM is located even higher.
+ //
+ if (FeaturePcdGet (PcdSmmSmramRequire) && mQ35SmramAtDefaultSmbase) {
+ ASSERT (SMM_DEFAULT_SMBASE + MCH_DEFAULT_SMBASE_SIZE <= MemoryBase);
+ }
+
//
// Publish this memory to the PEI Core
//
--
2.19.1.3.g30247aa5d201
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH wave 1 07/10] OvmfPkg/PlatformPei: reserve the SMRAM at the default SMBASE, if it exists
2019-09-24 11:34 [PATCH wave 1 00/10] support QEMU's "SMRAM at default SMBASE" feature Laszlo Ersek
` (5 preceding siblings ...)
2019-09-24 11:35 ` [PATCH wave 1 06/10] OvmfPkg/PlatformPei: assert there's no permanent PEI RAM at default SMBASE Laszlo Ersek
@ 2019-09-24 11:35 ` Laszlo Ersek
2019-09-24 11:35 ` [PATCH wave 1 08/10] OvmfPkg/SEV: don't manage the lifecycle of the SMRAM at the default SMBASE Laszlo Ersek
` (4 subsequent siblings)
11 siblings, 0 replies; 21+ messages in thread
From: Laszlo Ersek @ 2019-09-24 11:35 UTC (permalink / raw)
To: edk2-devel-groups-io
Cc: Ard Biesheuvel, Boris Ostrovsky, Brijesh Singh, Igor Mammedov,
Jiewen Yao, Joao M Martins, Jordan Justen, Jun Nakajima,
Michael Kinney, Paolo Bonzini, Phillip Goerl, Yingwen Chen
The 128KB SMRAM at the default SMBASE will be used for protecting the
initial SMI handler for hot-plugged VCPUs. After platform reset, the SMRAM
in question is open (and looks just like RAM). When BDS signals
EFI_DXE_MM_READY_TO_LOCK_PROTOCOL (and so TSEG is locked down), we're
going to lock the SMRAM at the default SMBASE too.
For this, we have to reserve said SMRAM area as early as possible, from
components in PEI, DXE, and OS runtime.
* QemuInitializeRam() currently produces a single resource descriptor HOB,
for exposing the system RAM available under 1GB. This occurs during both
normal boot and S3 resume identically (the latter only for the sake of
CpuMpPei borrowing low RAM for the AP startup vector).
But, the SMRAM at the default SMBASE falls in the middle of the current
system RAM HOB. Split the HOB, and cover the SMRAM with a reserved
memory HOB in the middle. CpuMpPei (via MpInitLib) skips reserved memory
HOBs.
* InitializeRamRegions() is responsible for producing memory allocation
HOBs, carving out parts of the resource descriptor HOBs produced in
QemuInitializeRam(). Allocate the above-introduced reserved memory
region in full, similarly to how we treat TSEG, so that DXE and the OS
avoid the locked SMRAM (black hole) in this area.
(Note that these allocations only occur on the normal boot path, as they
matter for the UEFI memory map, which cannot be changed during S3
resume.)
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Joao M Martins <joao.m.martins@oracle.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Jun Nakajima <jun.nakajima@intel.com>
Cc: Michael Kinney <michael.d.kinney@intel.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Phillip Goerl <phillip.goerl@oracle.com>
Cc: Yingwen Chen <yingwen.chen@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1512
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
OvmfPkg/PlatformPei/MemDetect.c | 37 ++++++++++++++++++--
1 file changed, 35 insertions(+), 2 deletions(-)
diff --git a/OvmfPkg/PlatformPei/MemDetect.c b/OvmfPkg/PlatformPei/MemDetect.c
index 4879ee87b797..8fdc9c2ed7c9 100644
--- a/OvmfPkg/PlatformPei/MemDetect.c
+++ b/OvmfPkg/PlatformPei/MemDetect.c
@@ -646,6 +646,28 @@ PublishPeiMemory (
}
+STATIC
+VOID
+QemuInitializeRamBelow1gb (
+ VOID
+ )
+{
+ if (FeaturePcdGet (PcdSmmSmramRequire) && mQ35SmramAtDefaultSmbase) {
+ AddMemoryRangeHob (0, SMM_DEFAULT_SMBASE);
+ AddReservedMemoryBaseSizeHob (SMM_DEFAULT_SMBASE, MCH_DEFAULT_SMBASE_SIZE,
+ TRUE /* Cacheable */);
+ STATIC_ASSERT (
+ SMM_DEFAULT_SMBASE + MCH_DEFAULT_SMBASE_SIZE < BASE_512KB + BASE_128KB,
+ "end of SMRAM at default SMBASE ends at, or exceeds, 640KB"
+ );
+ AddMemoryRangeHob (SMM_DEFAULT_SMBASE + MCH_DEFAULT_SMBASE_SIZE,
+ BASE_512KB + BASE_128KB);
+ } else {
+ AddMemoryRangeHob (0, BASE_512KB + BASE_128KB);
+ }
+}
+
+
/**
Peform Memory Detection for QEMU / KVM
@@ -690,12 +712,12 @@ QemuInitializeRam (
// allocation HOBs, and to honor preexistent memory allocation HOBs when
// looking for an area to borrow.
//
- AddMemoryRangeHob (0, BASE_512KB + BASE_128KB);
+ QemuInitializeRamBelow1gb ();
} else {
//
// Create memory HOBs
//
- AddMemoryRangeHob (0, BASE_512KB + BASE_128KB);
+ QemuInitializeRamBelow1gb ();
if (FeaturePcdGet (PcdSmmSmramRequire)) {
UINT32 TsegSize;
@@ -861,6 +883,17 @@ InitializeRamRegions (
TsegSize,
EfiReservedMemoryType
);
+ //
+ // Similarly, allocate away the (already reserved) SMRAM at the default
+ // SMBASE, if it exists.
+ //
+ if (mQ35SmramAtDefaultSmbase) {
+ BuildMemoryAllocationHob (
+ SMM_DEFAULT_SMBASE,
+ MCH_DEFAULT_SMBASE_SIZE,
+ EfiReservedMemoryType
+ );
+ }
}
}
}
--
2.19.1.3.g30247aa5d201
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH wave 1 08/10] OvmfPkg/SEV: don't manage the lifecycle of the SMRAM at the default SMBASE
2019-09-24 11:34 [PATCH wave 1 00/10] support QEMU's "SMRAM at default SMBASE" feature Laszlo Ersek
` (6 preceding siblings ...)
2019-09-24 11:35 ` [PATCH wave 1 07/10] OvmfPkg/PlatformPei: reserve the SMRAM at the default SMBASE, if it exists Laszlo Ersek
@ 2019-09-24 11:35 ` Laszlo Ersek
2019-09-24 11:35 ` [PATCH wave 1 09/10] OvmfPkg/SmmAccess: close and lock SMRAM at " Laszlo Ersek
` (3 subsequent siblings)
11 siblings, 0 replies; 21+ messages in thread
From: Laszlo Ersek @ 2019-09-24 11:35 UTC (permalink / raw)
To: edk2-devel-groups-io
Cc: Ard Biesheuvel, Boris Ostrovsky, Brijesh Singh, Igor Mammedov,
Jiewen Yao, Joao M Martins, Jordan Justen, Jun Nakajima,
Michael Kinney, Paolo Bonzini, Phillip Goerl, Yingwen Chen
When OVMF runs in a SEV guest, the initial SMM Save State Map is
(1) allocated as EfiBootServicesData type memory in OvmfPkg/PlatformPei,
function AmdSevInitialize(), for preventing unintended information
sharing with the hypervisor;
(2) decrypted in AmdSevDxe;
(3) re-encrypted in OvmfPkg/Library/SmmCpuFeaturesLib, function
SmmCpuFeaturesSmmRelocationComplete(), which is called by
PiSmmCpuDxeSmm right after initial SMBASE relocation;
(4) released to DXE at the same location.
The SMRAM at the default SMBASE is a superset of the initial Save State
Map. The reserved memory allocation in InitializeRamRegions(), from the
previous patch, must override the allocating and freeing in (1) and (4),
respectively. (Note: the decrypting and re-encrypting in (2) and (3) are
unaffected.)
In AmdSevInitialize(), only assert the containment of the initial Save
State Map, in the larger area already allocated by InitializeRamRegions().
In SmmCpuFeaturesSmmRelocationComplete(), preserve the allocation of the
initial Save State Map into OS runtime, as part of the allocation done by
InitializeRamRegions(). Only assert containment.
These changes only affect the normal boot path (the UEFI memory map is
untouched during S3 resume).
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Joao M Martins <joao.m.martins@oracle.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Jun Nakajima <jun.nakajima@intel.com>
Cc: Michael Kinney <michael.d.kinney@intel.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Phillip Goerl <phillip.goerl@oracle.com>
Cc: Yingwen Chen <yingwen.chen@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1512
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf | 4 ++++
OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c | 21 +++++++++++++++--
OvmfPkg/PlatformPei/AmdSev.c | 24 ++++++++++++++++----
3 files changed, 42 insertions(+), 7 deletions(-)
diff --git a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
index dd316f2b1bd8..97a10afb6e27 100644
--- a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
+++ b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
@@ -30,5 +30,9 @@ [LibraryClasses]
BaseMemoryLib
DebugLib
MemEncryptSevLib
+ PcdLib
SmmServicesTableLib
UefiBootServicesTableLib
+
+[Pcd]
+ gUefiOvmfPkgTokenSpaceGuid.PcdQ35SmramAtDefaultSmbase
diff --git a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
index 0bfdeda78d33..7ef7ed98342e 100644
--- a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
+++ b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
@@ -6,14 +6,17 @@
SPDX-License-Identifier: BSD-2-Clause-Patent
**/
+#include <IndustryStandard/Q35MchIch9.h>
#include <Library/BaseLib.h>
#include <Library/BaseMemoryLib.h>
#include <Library/DebugLib.h>
#include <Library/MemEncryptSevLib.h>
+#include <Library/PcdLib.h>
#include <Library/SmmCpuFeaturesLib.h>
#include <Library/SmmServicesTableLib.h>
#include <Library/UefiBootServicesTableLib.h>
#include <PiSmm.h>
+#include <Register/Intel/SmramSaveStateMap.h>
#include <Register/QemuSmramSaveStateMap.h>
//
@@ -215,8 +218,22 @@ SmmCpuFeaturesSmmRelocationComplete (
ZeroMem ((VOID *)MapPagesBase, EFI_PAGES_TO_SIZE (MapPagesCount));
- Status = gBS->FreePages (MapPagesBase, MapPagesCount);
- ASSERT_EFI_ERROR (Status);
+ if (PcdGetBool (PcdQ35SmramAtDefaultSmbase)) {
+ //
+ // The initial SMRAM Save State Map has been covered as part of a larger
+ // reserved memory allocation in PlatformPei's InitializeRamRegions(). That
+ // allocation is supposed to survive into OS runtime; we must not release
+ // any part of it. Only re-assert the containment here.
+ //
+ ASSERT (SMM_DEFAULT_SMBASE <= MapPagesBase);
+ ASSERT (
+ (MapPagesBase + EFI_PAGES_TO_SIZE (MapPagesCount) <=
+ SMM_DEFAULT_SMBASE + MCH_DEFAULT_SMBASE_SIZE)
+ );
+ } else {
+ Status = gBS->FreePages (MapPagesBase, MapPagesCount);
+ ASSERT_EFI_ERROR (Status);
+ }
}
/**
diff --git a/OvmfPkg/PlatformPei/AmdSev.c b/OvmfPkg/PlatformPei/AmdSev.c
index 2ae8126ccf8a..e484f4b311fe 100644
--- a/OvmfPkg/PlatformPei/AmdSev.c
+++ b/OvmfPkg/PlatformPei/AmdSev.c
@@ -9,6 +9,7 @@
//
// The package level header files this module uses
//
+#include <IndustryStandard/Q35MchIch9.h>
#include <Library/DebugLib.h>
#include <Library/HobLib.h>
#include <Library/MemEncryptSevLib.h>
@@ -16,6 +17,7 @@
#include <PiPei.h>
#include <Register/Amd/Cpuid.h>
#include <Register/Cpuid.h>
+#include <Register/Intel/SmramSaveStateMap.h>
#include "Platform.h"
@@ -83,10 +85,22 @@ AmdSevInitialize (
);
ASSERT_RETURN_ERROR (LocateMapStatus);
- BuildMemoryAllocationHob (
- MapPagesBase, // BaseAddress
- EFI_PAGES_TO_SIZE (MapPagesCount), // Length
- EfiBootServicesData // MemoryType
- );
+ if (mQ35SmramAtDefaultSmbase) {
+ //
+ // The initial SMRAM Save State Map has been covered as part of a larger
+ // reserved memory allocation in InitializeRamRegions().
+ //
+ ASSERT (SMM_DEFAULT_SMBASE <= MapPagesBase);
+ ASSERT (
+ (MapPagesBase + EFI_PAGES_TO_SIZE (MapPagesCount) <=
+ SMM_DEFAULT_SMBASE + MCH_DEFAULT_SMBASE_SIZE)
+ );
+ } else {
+ BuildMemoryAllocationHob (
+ MapPagesBase, // BaseAddress
+ EFI_PAGES_TO_SIZE (MapPagesCount), // Length
+ EfiBootServicesData // MemoryType
+ );
+ }
}
}
--
2.19.1.3.g30247aa5d201
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH wave 1 09/10] OvmfPkg/SmmAccess: close and lock SMRAM at default SMBASE
2019-09-24 11:34 [PATCH wave 1 00/10] support QEMU's "SMRAM at default SMBASE" feature Laszlo Ersek
` (7 preceding siblings ...)
2019-09-24 11:35 ` [PATCH wave 1 08/10] OvmfPkg/SEV: don't manage the lifecycle of the SMRAM at the default SMBASE Laszlo Ersek
@ 2019-09-24 11:35 ` Laszlo Ersek
2019-09-24 11:35 ` [PATCH wave 1 10/10] OvmfPkg/PlatformPei: detect SMRAM at default SMBASE (for real) Laszlo Ersek
` (2 subsequent siblings)
11 siblings, 0 replies; 21+ messages in thread
From: Laszlo Ersek @ 2019-09-24 11:35 UTC (permalink / raw)
To: edk2-devel-groups-io
Cc: Ard Biesheuvel, Boris Ostrovsky, Brijesh Singh, Igor Mammedov,
Jiewen Yao, Joao M Martins, Jordan Justen, Jun Nakajima,
Michael Kinney, Paolo Bonzini, Phillip Goerl, Yingwen Chen
During normal boot, when EFI_DXE_SMM_READY_TO_LOCK_PROTOCOL is installed
by platform BDS, the SMM IPL locks SMRAM (TSEG) through
EFI_SMM_ACCESS2_PROTOCOL.Lock(). See SmmIplReadyToLockEventNotify() in
"MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c".
During S3 resume, S3Resume2Pei locks SMRAM (TSEG) through
PEI_SMM_ACCESS_PPI.Lock(), before executing the boot script. See
S3ResumeExecuteBootScript() in
"UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c".
Those are precisely the places where the SMRAM at the default SMBASE
should be locked too. Add such an action to SmramAccessLock().
Notes:
- The SMRAM at the default SMBASE doesn't support the "closed and
unlocked" state (and so it can't be closed without locking it, and it
cannot be opened after closing it).
- The SMRAM at the default SMBASE isn't (and shouldn't) be exposed with
another EFI_SMRAM_DESCRIPTOR in the GetCapabilities() members of
EFI_SMM_ACCESS2_PROTOCOL / PEI_SMM_ACCESS_PPI. That's because the SMRAM
in question is not "general purpose"; it's only QEMU's solution to
protect the initial SMI handler from the OS, when a VCPU is hot-plugged.
Consequently, the state of the SMRAM at the default SMBASE is not
reflected in the "OpenState" / "LockState" fields of the protocol and
PPI.
- An alternative to extending SmramAccessLock() would be to register an
EFI_DXE_SMM_READY_TO_LOCK_PROTOCOL notify in SmmAccess2Dxe (for locking
at normal boot), and an EDKII_S3_SMM_INIT_DONE_GUID PPI notify in
SmmAccessPei (for locking at S3 resume).
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Joao M Martins <joao.m.martins@oracle.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Jun Nakajima <jun.nakajima@intel.com>
Cc: Michael Kinney <michael.d.kinney@intel.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Phillip Goerl <phillip.goerl@oracle.com>
Cc: Yingwen Chen <yingwen.chen@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1512
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
OvmfPkg/SmmAccess/SmmAccess2Dxe.inf | 1 +
OvmfPkg/SmmAccess/SmmAccessPei.inf | 1 +
OvmfPkg/SmmAccess/SmramInternal.h | 8 +++++++
OvmfPkg/SmmAccess/SmmAccess2Dxe.c | 7 ++++++
OvmfPkg/SmmAccess/SmmAccessPei.c | 6 +++++
OvmfPkg/SmmAccess/SmramInternal.c | 25 ++++++++++++++++++++
6 files changed, 48 insertions(+)
diff --git a/OvmfPkg/SmmAccess/SmmAccess2Dxe.inf b/OvmfPkg/SmmAccess/SmmAccess2Dxe.inf
index 7ced6b4e7ff4..d86381d0fbe2 100644
--- a/OvmfPkg/SmmAccess/SmmAccess2Dxe.inf
+++ b/OvmfPkg/SmmAccess/SmmAccess2Dxe.inf
@@ -49,6 +49,7 @@ [FeaturePcd]
gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire
[Pcd]
+ gUefiOvmfPkgTokenSpaceGuid.PcdQ35SmramAtDefaultSmbase
gUefiOvmfPkgTokenSpaceGuid.PcdQ35TsegMbytes
[Depex]
diff --git a/OvmfPkg/SmmAccess/SmmAccessPei.inf b/OvmfPkg/SmmAccess/SmmAccessPei.inf
index d73a029cc790..1698c4ce6c92 100644
--- a/OvmfPkg/SmmAccess/SmmAccessPei.inf
+++ b/OvmfPkg/SmmAccess/SmmAccessPei.inf
@@ -54,6 +54,7 @@ [FeaturePcd]
gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire
[Pcd]
+ gUefiOvmfPkgTokenSpaceGuid.PcdQ35SmramAtDefaultSmbase
gUefiOvmfPkgTokenSpaceGuid.PcdQ35TsegMbytes
[Ppis]
diff --git a/OvmfPkg/SmmAccess/SmramInternal.h b/OvmfPkg/SmmAccess/SmramInternal.h
index 74d962b4ecae..a4d8827adfe4 100644
--- a/OvmfPkg/SmmAccess/SmramInternal.h
+++ b/OvmfPkg/SmmAccess/SmramInternal.h
@@ -38,6 +38,14 @@ InitQ35TsegMbytes (
VOID
);
+/**
+ Save PcdQ35SmramAtDefaultSmbase into mQ35SmramAtDefaultSmbase.
+**/
+VOID
+InitQ35SmramAtDefaultSmbase (
+ VOID
+ );
+
/**
Read the MCH_SMRAM and ESMRAMC registers, and update the LockState and
OpenState fields in the PEI_SMM_ACCESS_PPI / EFI_SMM_ACCESS2_PROTOCOL object,
diff --git a/OvmfPkg/SmmAccess/SmmAccess2Dxe.c b/OvmfPkg/SmmAccess/SmmAccess2Dxe.c
index e098f6f15f77..3691a6cd1f10 100644
--- a/OvmfPkg/SmmAccess/SmmAccess2Dxe.c
+++ b/OvmfPkg/SmmAccess/SmmAccess2Dxe.c
@@ -145,6 +145,13 @@ SmmAccess2DxeEntryPoint (
InitQ35TsegMbytes ();
GetStates (&mAccess2.LockState, &mAccess2.OpenState);
+
+ //
+ // SmramAccessLock() depends on "mQ35SmramAtDefaultSmbase"; init the latter
+ // just before exposing the former via EFI_SMM_ACCESS2_PROTOCOL.Lock().
+ //
+ InitQ35SmramAtDefaultSmbase ();
+
return gBS->InstallMultipleProtocolInterfaces (&ImageHandle,
&gEfiSmmAccess2ProtocolGuid, &mAccess2,
NULL);
diff --git a/OvmfPkg/SmmAccess/SmmAccessPei.c b/OvmfPkg/SmmAccess/SmmAccessPei.c
index d67850651c58..c8bbc17e907a 100644
--- a/OvmfPkg/SmmAccess/SmmAccessPei.c
+++ b/OvmfPkg/SmmAccess/SmmAccessPei.c
@@ -372,6 +372,12 @@ SmmAccessPeiEntryPoint (
CopyMem (GuidHob, &SmramMap[DescIdxSmmS3ResumeState],
sizeof SmramMap[DescIdxSmmS3ResumeState]);
+ //
+ // SmramAccessLock() depends on "mQ35SmramAtDefaultSmbase"; init the latter
+ // just before exposing the former via PEI_SMM_ACCESS_PPI.Lock().
+ //
+ InitQ35SmramAtDefaultSmbase ();
+
//
// We're done. The next step should succeed, but even if it fails, we can't
// roll back the above BuildGuidHob() allocation, because PEI doesn't support
diff --git a/OvmfPkg/SmmAccess/SmramInternal.c b/OvmfPkg/SmmAccess/SmramInternal.c
index 09657d0f9b0f..0b07dc667b3f 100644
--- a/OvmfPkg/SmmAccess/SmramInternal.c
+++ b/OvmfPkg/SmmAccess/SmramInternal.c
@@ -21,6 +21,12 @@
//
UINT16 mQ35TsegMbytes;
+//
+// The value of PcdQ35SmramAtDefaultSmbase is saved into this variable at
+// module startup.
+//
+STATIC BOOLEAN mQ35SmramAtDefaultSmbase;
+
/**
Save PcdQ35TsegMbytes into mQ35TsegMbytes.
**/
@@ -32,6 +38,17 @@ InitQ35TsegMbytes (
mQ35TsegMbytes = PcdGet16 (PcdQ35TsegMbytes);
}
+/**
+ Save PcdQ35SmramAtDefaultSmbase into mQ35SmramAtDefaultSmbase.
+**/
+VOID
+InitQ35SmramAtDefaultSmbase (
+ VOID
+ )
+{
+ mQ35SmramAtDefaultSmbase = PcdGetBool (PcdQ35SmramAtDefaultSmbase);
+}
+
/**
Read the MCH_SMRAM and ESMRAMC registers, and update the LockState and
OpenState fields in the PEI_SMM_ACCESS_PPI / EFI_SMM_ACCESS2_PROTOCOL object,
@@ -125,6 +142,14 @@ SmramAccessLock (
PciOr8 (DRAMC_REGISTER_Q35 (MCH_ESMRAMC), MCH_ESMRAMC_T_EN);
PciOr8 (DRAMC_REGISTER_Q35 (MCH_SMRAM), MCH_SMRAM_D_LCK);
+ //
+ // Close & lock the SMRAM at the default SMBASE, if it exists.
+ //
+ if (mQ35SmramAtDefaultSmbase) {
+ PciWrite8 (DRAMC_REGISTER_Q35 (MCH_DEFAULT_SMBASE_CTL),
+ MCH_DEFAULT_SMBASE_LCK);
+ }
+
GetStates (LockState, OpenState);
if (*OpenState || !*LockState) {
return EFI_DEVICE_ERROR;
--
2.19.1.3.g30247aa5d201
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH wave 1 10/10] OvmfPkg/PlatformPei: detect SMRAM at default SMBASE (for real)
2019-09-24 11:34 [PATCH wave 1 00/10] support QEMU's "SMRAM at default SMBASE" feature Laszlo Ersek
` (8 preceding siblings ...)
2019-09-24 11:35 ` [PATCH wave 1 09/10] OvmfPkg/SmmAccess: close and lock SMRAM at " Laszlo Ersek
@ 2019-09-24 11:35 ` Laszlo Ersek
2019-09-26 8:46 ` [edk2-devel] [PATCH wave 1 00/10] support QEMU's "SMRAM at default SMBASE" feature Yao, Jiewen
2019-09-27 11:35 ` Igor Mammedov
11 siblings, 0 replies; 21+ messages in thread
From: Laszlo Ersek @ 2019-09-24 11:35 UTC (permalink / raw)
To: edk2-devel-groups-io
Cc: Ard Biesheuvel, Boris Ostrovsky, Brijesh Singh, Igor Mammedov,
Jiewen Yao, Joao M Martins, Jordan Justen, Jun Nakajima,
Michael Kinney, Paolo Bonzini, Phillip Goerl, Yingwen Chen
Now that the SMRAM at the default SMBASE is honored everywhere necessary,
implement the actual detection. The (simple) steps are described in
previous patch "OvmfPkg/IndustryStandard: add MCH_DEFAULT_SMBASE* register
macros".
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Joao M Martins <joao.m.martins@oracle.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Jun Nakajima <jun.nakajima@intel.com>
Cc: Michael Kinney <michael.d.kinney@intel.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Phillip Goerl <phillip.goerl@oracle.com>
Cc: Yingwen Chen <yingwen.chen@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1512
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
OvmfPkg/PlatformPei/MemDetect.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/OvmfPkg/PlatformPei/MemDetect.c b/OvmfPkg/PlatformPei/MemDetect.c
index 8fdc9c2ed7c9..3364193b4952 100644
--- a/OvmfPkg/PlatformPei/MemDetect.c
+++ b/OvmfPkg/PlatformPei/MemDetect.c
@@ -98,14 +98,23 @@ Q35SmramAtDefaultSmbaseInitialization (
VOID
)
{
+ UINTN CtlReg;
+ UINT8 CtlRegVal;
RETURN_STATUS PcdStatus;
ASSERT (mHostBridgeDevId == INTEL_Q35_MCH_DEVICE_ID);
- mQ35SmramAtDefaultSmbase = FALSE;
+ CtlReg = DRAMC_REGISTER_Q35 (MCH_DEFAULT_SMBASE_CTL);
+ PciWrite8 (CtlReg, MCH_DEFAULT_SMBASE_QUERY);
+ CtlRegVal = PciRead8 (CtlReg);
+
+ mQ35SmramAtDefaultSmbase = (BOOLEAN)(CtlRegVal == MCH_DEFAULT_SMBASE_IN_RAM);
PcdStatus = PcdSetBoolS (PcdQ35SmramAtDefaultSmbase,
mQ35SmramAtDefaultSmbase);
ASSERT_RETURN_ERROR (PcdStatus);
+ if (mQ35SmramAtDefaultSmbase) {
+ DEBUG ((DEBUG_INFO, "%a: SMRAM at default SMBASE found\n", __FUNCTION__));
+ }
}
--
2.19.1.3.g30247aa5d201
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [edk2-devel] [PATCH wave 1 04/10] OvmfPkg/PlatformPei: factor out Q35BoardVerification()
2019-09-24 11:34 ` [PATCH wave 1 04/10] OvmfPkg/PlatformPei: factor out Q35BoardVerification() Laszlo Ersek
@ 2019-09-24 11:41 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 21+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-09-24 11:41 UTC (permalink / raw)
To: devel, lersek
Cc: Ard Biesheuvel, Boris Ostrovsky, Brijesh Singh, Igor Mammedov,
Jiewen Yao, Joao M Martins, Jordan Justen, Jun Nakajima,
Michael Kinney, Paolo Bonzini, Phillip Goerl, Yingwen Chen
On 9/24/19 1:34 PM, Laszlo Ersek wrote:
> Before adding another SMM-related, and therefore Q35-only, dynamically
> detectable feature, extract the current board type check from
> Q35TsegMbytesInitialization() to a standalone function.
>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Cc: Brijesh Singh <brijesh.singh@amd.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Joao M Martins <joao.m.martins@oracle.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Jun Nakajima <jun.nakajima@intel.com>
> Cc: Michael Kinney <michael.d.kinney@intel.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Phillip Goerl <phillip.goerl@oracle.com>
> Cc: Yingwen Chen <yingwen.chen@intel.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1512
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
> OvmfPkg/PlatformPei/MemDetect.c | 13 +----------
> OvmfPkg/PlatformPei/Platform.c | 23 ++++++++++++++++++++
> 2 files changed, 24 insertions(+), 12 deletions(-)
>
> diff --git a/OvmfPkg/PlatformPei/MemDetect.c b/OvmfPkg/PlatformPei/MemDetect.c
> index d451989f31c9..58b171fba1c8 100644
> --- a/OvmfPkg/PlatformPei/MemDetect.c
> +++ b/OvmfPkg/PlatformPei/MemDetect.c
> @@ -53,18 +53,7 @@ Q35TsegMbytesInitialization (
> UINT16 ExtendedTsegMbytes;
> RETURN_STATUS PcdStatus;
>
> - if (mHostBridgeDevId != INTEL_Q35_MCH_DEVICE_ID) {
> - DEBUG ((
> - DEBUG_ERROR,
> - "%a: no TSEG (SMRAM) on host bridge DID=0x%04x; "
> - "only DID=0x%04x (Q35) is supported\n",
> - __FUNCTION__,
> - mHostBridgeDevId,
> - INTEL_Q35_MCH_DEVICE_ID
> - ));
> - ASSERT (FALSE);
> - CpuDeadLoop ();
> - }
> + ASSERT (mHostBridgeDevId == INTEL_Q35_MCH_DEVICE_ID);
>
> //
> // Check if QEMU offers an extended TSEG.
> diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
> index 3ba2459872d9..ca6d37cb1549 100644
> --- a/OvmfPkg/PlatformPei/Platform.c
> +++ b/OvmfPkg/PlatformPei/Platform.c
> @@ -563,6 +563,28 @@ S3Verification (
> }
>
>
> +VOID
> +Q35BoardVerification (
> + VOID
> + )
> +{
> + if (mHostBridgeDevId == INTEL_Q35_MCH_DEVICE_ID) {
> + return;
> + }
> +
> + DEBUG ((
> + DEBUG_ERROR,
> + "%a: no TSEG (SMRAM) on host bridge DID=0x%04x; "
> + "only DID=0x%04x (Q35) is supported\n",
> + __FUNCTION__,
> + mHostBridgeDevId,
> + INTEL_Q35_MCH_DEVICE_ID
> + ));
> + ASSERT (FALSE);
> + CpuDeadLoop ();
> +}
> +
> +
> /**
> Fetch the number of boot CPUs from QEMU and expose it to UefiCpuPkg modules.
> Set the mMaxCpuCount variable.
> @@ -646,6 +668,7 @@ InitializePlatform (
> mHostBridgeDevId = PciRead16 (OVMF_HOSTBRIDGE_DID);
>
> if (FeaturePcdGet (PcdSmmSmramRequire)) {
> + Q35BoardVerification ();
> Q35TsegMbytesInitialization ();
> }
>
>
Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [edk2-devel] [PATCH wave 1 02/10] OvmfPkg/IndustryStandard: increase vertical whitespace in Q35 macro defs
2019-09-24 11:34 ` [PATCH wave 1 02/10] OvmfPkg/IndustryStandard: increase vertical whitespace in Q35 macro defs Laszlo Ersek
@ 2019-09-24 11:44 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 21+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-09-24 11:44 UTC (permalink / raw)
To: devel, lersek
Cc: Ard Biesheuvel, Boris Ostrovsky, Brijesh Singh, Igor Mammedov,
Jiewen Yao, Joao M Martins, Jordan Justen, Jun Nakajima,
Michael Kinney, Paolo Bonzini, Phillip Goerl, Yingwen Chen
On 9/24/19 1:34 PM, Laszlo Ersek wrote:
> In a subsequent patch, we'll introduce new DRAM controller macros in
> "Q35MchIch9.h". Their names are too long for the currently available
> vertical whitespace, so increase the latter first.
>
> There is no functional change in this patch ("git show -b" displays
> nothing).
>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Cc: Brijesh Singh <brijesh.singh@amd.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Joao M Martins <joao.m.martins@oracle.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Jun Nakajima <jun.nakajima@intel.com>
> Cc: Michael Kinney <michael.d.kinney@intel.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Phillip Goerl <phillip.goerl@oracle.com>
> Cc: Yingwen Chen <yingwen.chen@intel.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1512
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
> OvmfPkg/Include/IndustryStandard/Q35MchIch9.h | 100 ++++++++++----------
> 1 file changed, 50 insertions(+), 50 deletions(-)
>
> diff --git a/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h b/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h
> index 391cb4622226..614699ab38f1 100644
> --- a/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h
> +++ b/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h
> @@ -27,56 +27,56 @@
> //
> #define DRAMC_REGISTER_Q35(Offset) PCI_LIB_ADDRESS (0, 0, 0, (Offset))
>
> -#define MCH_EXT_TSEG_MB 0x50
> -#define MCH_EXT_TSEG_MB_QUERY 0xFFFF
> -
> -#define MCH_GGC 0x52
> -#define MCH_GGC_IVD BIT1
> -
> -#define MCH_PCIEXBAR_LOW 0x60
> -#define MCH_PCIEXBAR_LOWMASK 0x0FFFFFFF
> -#define MCH_PCIEXBAR_BUS_FF 0
> -#define MCH_PCIEXBAR_EN BIT0
> -
> -#define MCH_PCIEXBAR_HIGH 0x64
> -#define MCH_PCIEXBAR_HIGHMASK 0xFFFFFFF0
> -
> -#define MCH_PAM0 0x90
> -#define MCH_PAM1 0x91
> -#define MCH_PAM2 0x92
> -#define MCH_PAM3 0x93
> -#define MCH_PAM4 0x94
> -#define MCH_PAM5 0x95
> -#define MCH_PAM6 0x96
> -
> -#define MCH_SMRAM 0x9D
> -#define MCH_SMRAM_D_LCK BIT4
> -#define MCH_SMRAM_G_SMRAME BIT3
> -
> -#define MCH_ESMRAMC 0x9E
> -#define MCH_ESMRAMC_H_SMRAME BIT7
> -#define MCH_ESMRAMC_E_SMERR BIT6
> -#define MCH_ESMRAMC_SM_CACHE BIT5
> -#define MCH_ESMRAMC_SM_L1 BIT4
> -#define MCH_ESMRAMC_SM_L2 BIT3
> -#define MCH_ESMRAMC_TSEG_EXT (BIT2 | BIT1)
> -#define MCH_ESMRAMC_TSEG_8MB BIT2
> -#define MCH_ESMRAMC_TSEG_2MB BIT1
> -#define MCH_ESMRAMC_TSEG_1MB 0
> -#define MCH_ESMRAMC_TSEG_MASK (BIT2 | BIT1)
> -#define MCH_ESMRAMC_T_EN BIT0
> -
> -#define MCH_GBSM 0xA4
> -#define MCH_GBSM_MB_SHIFT 20
> -
> -#define MCH_BGSM 0xA8
> -#define MCH_BGSM_MB_SHIFT 20
> -
> -#define MCH_TSEGMB 0xAC
> -#define MCH_TSEGMB_MB_SHIFT 20
> -
> -#define MCH_TOLUD 0xB0
> -#define MCH_TOLUD_MB_SHIFT 4
> +#define MCH_EXT_TSEG_MB 0x50
> +#define MCH_EXT_TSEG_MB_QUERY 0xFFFF
> +
> +#define MCH_GGC 0x52
> +#define MCH_GGC_IVD BIT1
> +
> +#define MCH_PCIEXBAR_LOW 0x60
> +#define MCH_PCIEXBAR_LOWMASK 0x0FFFFFFF
> +#define MCH_PCIEXBAR_BUS_FF 0
> +#define MCH_PCIEXBAR_EN BIT0
> +
> +#define MCH_PCIEXBAR_HIGH 0x64
> +#define MCH_PCIEXBAR_HIGHMASK 0xFFFFFFF0
> +
> +#define MCH_PAM0 0x90
> +#define MCH_PAM1 0x91
> +#define MCH_PAM2 0x92
> +#define MCH_PAM3 0x93
> +#define MCH_PAM4 0x94
> +#define MCH_PAM5 0x95
> +#define MCH_PAM6 0x96
> +
> +#define MCH_SMRAM 0x9D
> +#define MCH_SMRAM_D_LCK BIT4
> +#define MCH_SMRAM_G_SMRAME BIT3
> +
> +#define MCH_ESMRAMC 0x9E
> +#define MCH_ESMRAMC_H_SMRAME BIT7
> +#define MCH_ESMRAMC_E_SMERR BIT6
> +#define MCH_ESMRAMC_SM_CACHE BIT5
> +#define MCH_ESMRAMC_SM_L1 BIT4
> +#define MCH_ESMRAMC_SM_L2 BIT3
> +#define MCH_ESMRAMC_TSEG_EXT (BIT2 | BIT1)
> +#define MCH_ESMRAMC_TSEG_8MB BIT2
> +#define MCH_ESMRAMC_TSEG_2MB BIT1
> +#define MCH_ESMRAMC_TSEG_1MB 0
> +#define MCH_ESMRAMC_TSEG_MASK (BIT2 | BIT1)
> +#define MCH_ESMRAMC_T_EN BIT0
> +
> +#define MCH_GBSM 0xA4
> +#define MCH_GBSM_MB_SHIFT 20
> +
> +#define MCH_BGSM 0xA8
> +#define MCH_BGSM_MB_SHIFT 20
> +
> +#define MCH_TSEGMB 0xAC
> +#define MCH_TSEGMB_MB_SHIFT 20
> +
> +#define MCH_TOLUD 0xB0
> +#define MCH_TOLUD_MB_SHIFT 4
>
> //
> // B/D/F/Type: 0/0x1f/0/PCI
>
Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>
Tested-by: Philippe Mathieu-Daude <philmd@redhat.com>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [edk2-devel] [PATCH wave 1 00/10] support QEMU's "SMRAM at default SMBASE" feature
2019-09-24 11:34 [PATCH wave 1 00/10] support QEMU's "SMRAM at default SMBASE" feature Laszlo Ersek
` (9 preceding siblings ...)
2019-09-24 11:35 ` [PATCH wave 1 10/10] OvmfPkg/PlatformPei: detect SMRAM at default SMBASE (for real) Laszlo Ersek
@ 2019-09-26 8:46 ` Yao, Jiewen
2019-09-26 14:51 ` Laszlo Ersek
2019-09-27 11:35 ` Igor Mammedov
11 siblings, 1 reply; 21+ messages in thread
From: Yao, Jiewen @ 2019-09-26 8:46 UTC (permalink / raw)
To: devel@edk2.groups.io, lersek@redhat.com
Cc: Ard Biesheuvel, Boris Ostrovsky, Brijesh Singh, Igor Mammedov,
Joao M Martins, Justen, Jordan L, Nakajima, Jun,
Kinney, Michael D, Paolo Bonzini, Phillip Goerl, Chen, Yingwen
Hi Laszlo
May I know if you want to support legacy BIOS?
The legacy BIOS memory map is reported in E820, which is different with UEFI memory map.
In OvmfPkg\Csm\LegacyBiosDxe\LegacyBootSupport.c, LegacyBiosBuildE820() will hardcode below 640K address to be OS usage without consulting UEFI memory map.
//
// First entry is 0 to (640k - EBDA)
//
ACCESS_PAGE0_CODE (
E820Table[0].BaseAddr = 0;
E820Table[0].Length = (UINT64) ((*(UINT16 *) (UINTN)0x40E) << 4);
E820Table[0].Type = EfiAcpiAddressRangeMemory;
);
If we want to support this feature in legacy BIOS, we also need reserve the black hole here.
Thank you
Yao Jiewen
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek
> Sent: Tuesday, September 24, 2019 7:35 PM
> To: edk2-devel-groups-io <devel@edk2.groups.io>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Boris Ostrovsky
> <boris.ostrovsky@oracle.com>; Brijesh Singh <brijesh.singh@amd.com>; Igor
> Mammedov <imammedo@redhat.com>; Yao, Jiewen <jiewen.yao@intel.com>;
> Joao M Martins <joao.m.martins@oracle.com>; Justen, Jordan L
> <jordan.l.justen@intel.com>; Nakajima, Jun <jun.nakajima@intel.com>; Kinney,
> Michael D <michael.d.kinney@intel.com>; Paolo Bonzini
> <pbonzini@redhat.com>; Phillip Goerl <phillip.goerl@oracle.com>; Chen,
> Yingwen <yingwen.chen@intel.com>
> Subject: [edk2-devel] [PATCH wave 1 00/10] support QEMU's "SMRAM at
> default SMBASE" feature
>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1512
> Repo: https://github.com/lersek/edk2.git
> Branch: smram_at_default_smbase_bz_1512_wave_1
>
> This is the firmware-side patch series for the QEMU feature that Igor is
> proposing in
>
> [Qemu-devel] [PATCH 0/2]
> q35: mch: allow to lock down 128K RAM at default SMBASE address
>
> posted at
>
> http://mid.mail-archive.com/20190917130708.10281-1-
> imammedo@redhat.com
> https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg03538.html
> https://edk2.groups.io/g/devel/message/47364
>
> This OVMF patch series is supposed to be the first "wave" of patch sets,
> working towards VCPU hotplug with SMM enabled.
>
> We want the hot-plugged VCPU to execute its very first instruction from
> SMRAM, running in SMM, for protection from OS and PCI DMA interference.
> In the (long!) discussion at
>
> http://mid.mail-archive.com/20190905174503.2acaa46a@redhat.com
> https://edk2.groups.io/g/devel/message/46910
> https://bugzilla.tianocore.org/show_bug.cgi?id=1512#c9
>
> we considered diverging from the specs and changing the default SMBASE
> to an address different from 0x3_0000, so that it would point into
> SMRAM. However, Igor had the idea to keep the default SMBASE intact, and
> replace the underlying system RAM (128 KB of it) with lockable SMRAM.
> This conforms to the language of the Intel SDM, namely, "[t]he actual
> physical location of the SMRAM can be in system memory or in a separate
> RAM memory". When firmware locks this QEMU-specific SMRAM, the latter
> completely disappears from the address space of code that runs outside
> of SMM (including PCI devices doing DMA). We call the remaining MMIO
> area a "black hole".
>
> The present patch set detects the QEMU feature, locks the SMRAM at the
> right times, and informs firmware and OS components to stay away form
> the SMRAM at the default SMBASE.
>
> I've tested the set extensively:
>
> http://mid.mail-archive.com/c18f1ada-3eca-d5f1-da4f-
> e74181c5a862@redhat.com
> https://edk2.groups.io/g/devel/message/47864
>
> Going forward, if I understand correctly, the plan is to populate the
> new SMRAM with the hotplug SMI handler. (This would likely be put in
> place by OvmfPkg/PlatformPei, from NASM source code. The
> SmmRelocateBases() function in UefiCpuPkg/PiSmmCpuDxeSmm backs up the
> original contents before, and restores them after, the initial SMBASE
> relocation of the cold-plugged VCPUs. Thus the hotplug SMI handler would
> survive the initial relocation of the cold-plugged VCPUs.)
>
> Further, when a VCPU is hot-plugged, we seem to intend to raise an SMI
> for it (perhaps internally to QEMU?), which will remain pending until
> the VCPU is launched with INIT-SIPI-SIPI. Once the INIT-SIPI-SIPI is
> sent, the VCPU will immediately enter SMM, and run the SMI handler in
> the locked SMRAM at the default SMBASE. At RSM, it may continue at the
> vector requested in the INIT-SIPI-SIPI.
>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Cc: Brijesh Singh <brijesh.singh@amd.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Joao M Martins <joao.m.martins@oracle.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Jun Nakajima <jun.nakajima@intel.com>
> Cc: Michael Kinney <michael.d.kinney@intel.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Phillip Goerl <phillip.goerl@oracle.com>
> Cc: Yingwen Chen <yingwen.chen@intel.com>
>
> Thanks
> Laszlo
>
> Laszlo Ersek (10):
> OvmfPkg: introduce PcdQ35SmramAtDefaultSmbase
> OvmfPkg/IndustryStandard: increase vertical whitespace in Q35 macro
> defs
> OvmfPkg/IndustryStandard: add MCH_DEFAULT_SMBASE* register macros
> OvmfPkg/PlatformPei: factor out Q35BoardVerification()
> OvmfPkg/PlatformPei: detect SMRAM at default SMBASE (skeleton)
> OvmfPkg/PlatformPei: assert there's no permanent PEI RAM at default
> SMBASE
> OvmfPkg/PlatformPei: reserve the SMRAM at the default SMBASE, if it
> exists
> OvmfPkg/SEV: don't manage the lifecycle of the SMRAM at the default
> SMBASE
> OvmfPkg/SmmAccess: close and lock SMRAM at default SMBASE
> OvmfPkg/PlatformPei: detect SMRAM at default SMBASE (for real)
>
> OvmfPkg/Include/IndustryStandard/Q35MchIch9.h | 106 +++++++++++----
> -----
> OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c | 21 +++-
> OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf | 4 +
> OvmfPkg/OvmfPkg.dec | 6 ++
> OvmfPkg/OvmfPkgIa32.dsc | 1 +
> OvmfPkg/OvmfPkgIa32X64.dsc | 1 +
> OvmfPkg/OvmfPkgX64.dsc | 1 +
> OvmfPkg/PlatformPei/AmdSev.c | 24 ++++-
> OvmfPkg/PlatformPei/MemDetect.c | 87 +++++++++++++---
> OvmfPkg/PlatformPei/Platform.c | 24 +++++
> OvmfPkg/PlatformPei/Platform.h | 7 ++
> OvmfPkg/PlatformPei/PlatformPei.inf | 1 +
> OvmfPkg/SmmAccess/SmmAccess2Dxe.c | 7 ++
> OvmfPkg/SmmAccess/SmmAccess2Dxe.inf | 1 +
> OvmfPkg/SmmAccess/SmmAccessPei.c | 6 ++
> OvmfPkg/SmmAccess/SmmAccessPei.inf | 1 +
> OvmfPkg/SmmAccess/SmramInternal.c | 25 +++++
> OvmfPkg/SmmAccess/SmramInternal.h | 8 ++
> 18 files changed, 260 insertions(+), 71 deletions(-)
>
> --
> 2.19.1.3.g30247aa5d201
>
>
> -=-=-=-=-=-=
> Groups.io Links: You receive all messages sent to this group.
>
> View/Reply Online (#47924): https://edk2.groups.io/g/devel/message/47924
> Mute This Topic: https://groups.io/mt/34274934/1772286
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub [jiewen.yao@intel.com]
> -=-=-=-=-=-=
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [edk2-devel] [PATCH wave 1 00/10] support QEMU's "SMRAM at default SMBASE" feature
2019-09-26 8:46 ` [edk2-devel] [PATCH wave 1 00/10] support QEMU's "SMRAM at default SMBASE" feature Yao, Jiewen
@ 2019-09-26 14:51 ` Laszlo Ersek
2019-09-27 1:14 ` Yao, Jiewen
0 siblings, 1 reply; 21+ messages in thread
From: Laszlo Ersek @ 2019-09-26 14:51 UTC (permalink / raw)
To: Yao, Jiewen, devel@edk2.groups.io
Cc: Ard Biesheuvel, Boris Ostrovsky, Brijesh Singh, Igor Mammedov,
Joao M Martins, Justen, Jordan L, Nakajima, Jun,
Kinney, Michael D, Paolo Bonzini, Phillip Goerl, Chen, Yingwen
On 09/26/19 10:46, Yao, Jiewen wrote:
> Hi Laszlo
> May I know if you want to support legacy BIOS?
No, thanks.
The end-goal is to implement VCPU hotplug at OS runtime, when the OS is
booted with OVMF, Secure Boot is enabled, and the firmware is protected
with SMM.
In other words, the goal is to support VCPU hotplug when there is a
privilege barrier between the firmware and the OS. (The whole complexity
of the feature arises because a hotplugged VCPU is a risk for that barrier.)
With legacy BIOS, there is no such barrier, and VCPU hotplug already
works with SeaBIOS.
Furthermore, if you build OVMF *without* "-D SMM_REQUIRE", then there is
no such barrier again, and VCPU hotplug already works well, in that case
too. (I had tested it extensively, in RHBZ#1454803.)
In effect, I consider "-D SMM_REQUIRE" and "-D CSM_ENABLE" mutually
exclusive, for OVMF. And the present work is only relevant for "-D
SMM_REQUIRE".
We could perhaps add an "!error" directive to the OVMF DSC files,
conditional on both flags being enabled (SMM_REQUIRE and CSM_ENABLE). I
might propose such a patch in the future, but until it becomes a
practical problem, I don't want to spend cycles on it.
Thanks
Laszlo
> The legacy BIOS memory map is reported in E820, which is different with UEFI memory map.
>
> In OvmfPkg\Csm\LegacyBiosDxe\LegacyBootSupport.c, LegacyBiosBuildE820() will hardcode below 640K address to be OS usage without consulting UEFI memory map.
>
> //
> // First entry is 0 to (640k - EBDA)
> //
> ACCESS_PAGE0_CODE (
> E820Table[0].BaseAddr = 0;
> E820Table[0].Length = (UINT64) ((*(UINT16 *) (UINTN)0x40E) << 4);
> E820Table[0].Type = EfiAcpiAddressRangeMemory;
> );
>
> If we want to support this feature in legacy BIOS, we also need reserve the black hole here.
>
> Thank you
> Yao Jiewen
>
>
>> -----Original Message-----
>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek
>> Sent: Tuesday, September 24, 2019 7:35 PM
>> To: edk2-devel-groups-io <devel@edk2.groups.io>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Boris Ostrovsky
>> <boris.ostrovsky@oracle.com>; Brijesh Singh <brijesh.singh@amd.com>; Igor
>> Mammedov <imammedo@redhat.com>; Yao, Jiewen <jiewen.yao@intel.com>;
>> Joao M Martins <joao.m.martins@oracle.com>; Justen, Jordan L
>> <jordan.l.justen@intel.com>; Nakajima, Jun <jun.nakajima@intel.com>; Kinney,
>> Michael D <michael.d.kinney@intel.com>; Paolo Bonzini
>> <pbonzini@redhat.com>; Phillip Goerl <phillip.goerl@oracle.com>; Chen,
>> Yingwen <yingwen.chen@intel.com>
>> Subject: [edk2-devel] [PATCH wave 1 00/10] support QEMU's "SMRAM at
>> default SMBASE" feature
>>
>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1512
>> Repo: https://github.com/lersek/edk2.git
>> Branch: smram_at_default_smbase_bz_1512_wave_1
>>
>> This is the firmware-side patch series for the QEMU feature that Igor is
>> proposing in
>>
>> [Qemu-devel] [PATCH 0/2]
>> q35: mch: allow to lock down 128K RAM at default SMBASE address
>>
>> posted at
>>
>> http://mid.mail-archive.com/20190917130708.10281-1-
>> imammedo@redhat.com
>> https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg03538.html
>> https://edk2.groups.io/g/devel/message/47364
>>
>> This OVMF patch series is supposed to be the first "wave" of patch sets,
>> working towards VCPU hotplug with SMM enabled.
>>
>> We want the hot-plugged VCPU to execute its very first instruction from
>> SMRAM, running in SMM, for protection from OS and PCI DMA interference.
>> In the (long!) discussion at
>>
>> http://mid.mail-archive.com/20190905174503.2acaa46a@redhat.com
>> https://edk2.groups.io/g/devel/message/46910
>> https://bugzilla.tianocore.org/show_bug.cgi?id=1512#c9
>>
>> we considered diverging from the specs and changing the default SMBASE
>> to an address different from 0x3_0000, so that it would point into
>> SMRAM. However, Igor had the idea to keep the default SMBASE intact, and
>> replace the underlying system RAM (128 KB of it) with lockable SMRAM.
>> This conforms to the language of the Intel SDM, namely, "[t]he actual
>> physical location of the SMRAM can be in system memory or in a separate
>> RAM memory". When firmware locks this QEMU-specific SMRAM, the latter
>> completely disappears from the address space of code that runs outside
>> of SMM (including PCI devices doing DMA). We call the remaining MMIO
>> area a "black hole".
>>
>> The present patch set detects the QEMU feature, locks the SMRAM at the
>> right times, and informs firmware and OS components to stay away form
>> the SMRAM at the default SMBASE.
>>
>> I've tested the set extensively:
>>
>> http://mid.mail-archive.com/c18f1ada-3eca-d5f1-da4f-
>> e74181c5a862@redhat.com
>> https://edk2.groups.io/g/devel/message/47864
>>
>> Going forward, if I understand correctly, the plan is to populate the
>> new SMRAM with the hotplug SMI handler. (This would likely be put in
>> place by OvmfPkg/PlatformPei, from NASM source code. The
>> SmmRelocateBases() function in UefiCpuPkg/PiSmmCpuDxeSmm backs up the
>> original contents before, and restores them after, the initial SMBASE
>> relocation of the cold-plugged VCPUs. Thus the hotplug SMI handler would
>> survive the initial relocation of the cold-plugged VCPUs.)
>>
>> Further, when a VCPU is hot-plugged, we seem to intend to raise an SMI
>> for it (perhaps internally to QEMU?), which will remain pending until
>> the VCPU is launched with INIT-SIPI-SIPI. Once the INIT-SIPI-SIPI is
>> sent, the VCPU will immediately enter SMM, and run the SMI handler in
>> the locked SMRAM at the default SMBASE. At RSM, it may continue at the
>> vector requested in the INIT-SIPI-SIPI.
>>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>> Cc: Brijesh Singh <brijesh.singh@amd.com>
>> Cc: Igor Mammedov <imammedo@redhat.com>
>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>> Cc: Joao M Martins <joao.m.martins@oracle.com>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Jun Nakajima <jun.nakajima@intel.com>
>> Cc: Michael Kinney <michael.d.kinney@intel.com>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Cc: Phillip Goerl <phillip.goerl@oracle.com>
>> Cc: Yingwen Chen <yingwen.chen@intel.com>
>>
>> Thanks
>> Laszlo
>>
>> Laszlo Ersek (10):
>> OvmfPkg: introduce PcdQ35SmramAtDefaultSmbase
>> OvmfPkg/IndustryStandard: increase vertical whitespace in Q35 macro
>> defs
>> OvmfPkg/IndustryStandard: add MCH_DEFAULT_SMBASE* register macros
>> OvmfPkg/PlatformPei: factor out Q35BoardVerification()
>> OvmfPkg/PlatformPei: detect SMRAM at default SMBASE (skeleton)
>> OvmfPkg/PlatformPei: assert there's no permanent PEI RAM at default
>> SMBASE
>> OvmfPkg/PlatformPei: reserve the SMRAM at the default SMBASE, if it
>> exists
>> OvmfPkg/SEV: don't manage the lifecycle of the SMRAM at the default
>> SMBASE
>> OvmfPkg/SmmAccess: close and lock SMRAM at default SMBASE
>> OvmfPkg/PlatformPei: detect SMRAM at default SMBASE (for real)
>>
>> OvmfPkg/Include/IndustryStandard/Q35MchIch9.h | 106 +++++++++++----
>> -----
>> OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c | 21 +++-
>> OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf | 4 +
>> OvmfPkg/OvmfPkg.dec | 6 ++
>> OvmfPkg/OvmfPkgIa32.dsc | 1 +
>> OvmfPkg/OvmfPkgIa32X64.dsc | 1 +
>> OvmfPkg/OvmfPkgX64.dsc | 1 +
>> OvmfPkg/PlatformPei/AmdSev.c | 24 ++++-
>> OvmfPkg/PlatformPei/MemDetect.c | 87 +++++++++++++---
>> OvmfPkg/PlatformPei/Platform.c | 24 +++++
>> OvmfPkg/PlatformPei/Platform.h | 7 ++
>> OvmfPkg/PlatformPei/PlatformPei.inf | 1 +
>> OvmfPkg/SmmAccess/SmmAccess2Dxe.c | 7 ++
>> OvmfPkg/SmmAccess/SmmAccess2Dxe.inf | 1 +
>> OvmfPkg/SmmAccess/SmmAccessPei.c | 6 ++
>> OvmfPkg/SmmAccess/SmmAccessPei.inf | 1 +
>> OvmfPkg/SmmAccess/SmramInternal.c | 25 +++++
>> OvmfPkg/SmmAccess/SmramInternal.h | 8 ++
>> 18 files changed, 260 insertions(+), 71 deletions(-)
>>
>> --
>> 2.19.1.3.g30247aa5d201
>>
>>
>> -=-=-=-=-=-=
>> Groups.io Links: You receive all messages sent to this group.
>>
>> View/Reply Online (#47924): https://edk2.groups.io/g/devel/message/47924
>> Mute This Topic: https://groups.io/mt/34274934/1772286
>> Group Owner: devel+owner@edk2.groups.io
>> Unsubscribe: https://edk2.groups.io/g/devel/unsub [jiewen.yao@intel.com]
>> -=-=-=-=-=-=
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [edk2-devel] [PATCH wave 1 00/10] support QEMU's "SMRAM at default SMBASE" feature
2019-09-26 14:51 ` Laszlo Ersek
@ 2019-09-27 1:14 ` Yao, Jiewen
2019-10-01 14:43 ` Laszlo Ersek
0 siblings, 1 reply; 21+ messages in thread
From: Yao, Jiewen @ 2019-09-27 1:14 UTC (permalink / raw)
To: Laszlo Ersek, devel@edk2.groups.io
Cc: Ard Biesheuvel, Boris Ostrovsky, Brijesh Singh, Igor Mammedov,
Joao M Martins, Justen, Jordan L, Nakajima, Jun,
Kinney, Michael D, Paolo Bonzini, Phillip Goerl, Chen, Yingwen
Sounds good.
Maybe you can write that info in the commit message. A simple sentence such as :
No CSM support is needed because legacy BIOS boot don't use SMM.
So other people won't have same question in the future.
With the commit message change, the series reviewed-by: jiewen.yao@intel.com
Thank you
Yao Jiewen
> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Thursday, September 26, 2019 10:52 PM
> To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Boris Ostrovsky
> <boris.ostrovsky@oracle.com>; Brijesh Singh <brijesh.singh@amd.com>; Igor
> Mammedov <imammedo@redhat.com>; Joao M Martins
> <joao.m.martins@oracle.com>; Justen, Jordan L <jordan.l.justen@intel.com>;
> Nakajima, Jun <jun.nakajima@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Paolo Bonzini <pbonzini@redhat.com>; Phillip
> Goerl <phillip.goerl@oracle.com>; Chen, Yingwen <yingwen.chen@intel.com>
> Subject: Re: [edk2-devel] [PATCH wave 1 00/10] support QEMU's "SMRAM at
> default SMBASE" feature
>
> On 09/26/19 10:46, Yao, Jiewen wrote:
> > Hi Laszlo
> > May I know if you want to support legacy BIOS?
>
> No, thanks.
>
> The end-goal is to implement VCPU hotplug at OS runtime, when the OS is
> booted with OVMF, Secure Boot is enabled, and the firmware is protected
> with SMM.
>
> In other words, the goal is to support VCPU hotplug when there is a
> privilege barrier between the firmware and the OS. (The whole complexity
> of the feature arises because a hotplugged VCPU is a risk for that barrier.)
>
> With legacy BIOS, there is no such barrier, and VCPU hotplug already
> works with SeaBIOS.
>
> Furthermore, if you build OVMF *without* "-D SMM_REQUIRE", then there is
> no such barrier again, and VCPU hotplug already works well, in that case
> too. (I had tested it extensively, in RHBZ#1454803.)
>
> In effect, I consider "-D SMM_REQUIRE" and "-D CSM_ENABLE" mutually
> exclusive, for OVMF. And the present work is only relevant for "-D
> SMM_REQUIRE".
>
> We could perhaps add an "!error" directive to the OVMF DSC files,
> conditional on both flags being enabled (SMM_REQUIRE and CSM_ENABLE). I
> might propose such a patch in the future, but until it becomes a
> practical problem, I don't want to spend cycles on it.
>
> Thanks
> Laszlo
>
> > The legacy BIOS memory map is reported in E820, which is different with UEFI
> memory map.
> >
> > In OvmfPkg\Csm\LegacyBiosDxe\LegacyBootSupport.c, LegacyBiosBuildE820()
> will hardcode below 640K address to be OS usage without consulting UEFI
> memory map.
> >
> > //
> > // First entry is 0 to (640k - EBDA)
> > //
> > ACCESS_PAGE0_CODE (
> > E820Table[0].BaseAddr = 0;
> > E820Table[0].Length = (UINT64) ((*(UINT16 *) (UINTN)0x40E) << 4);
> > E820Table[0].Type = EfiAcpiAddressRangeMemory;
> > );
> >
> > If we want to support this feature in legacy BIOS, we also need reserve the
> black hole here.
> >
> > Thank you
> > Yao Jiewen
> >
> >
> >> -----Original Message-----
> >> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo
> Ersek
> >> Sent: Tuesday, September 24, 2019 7:35 PM
> >> To: edk2-devel-groups-io <devel@edk2.groups.io>
> >> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Boris Ostrovsky
> >> <boris.ostrovsky@oracle.com>; Brijesh Singh <brijesh.singh@amd.com>; Igor
> >> Mammedov <imammedo@redhat.com>; Yao, Jiewen
> <jiewen.yao@intel.com>;
> >> Joao M Martins <joao.m.martins@oracle.com>; Justen, Jordan L
> >> <jordan.l.justen@intel.com>; Nakajima, Jun <jun.nakajima@intel.com>;
> Kinney,
> >> Michael D <michael.d.kinney@intel.com>; Paolo Bonzini
> >> <pbonzini@redhat.com>; Phillip Goerl <phillip.goerl@oracle.com>; Chen,
> >> Yingwen <yingwen.chen@intel.com>
> >> Subject: [edk2-devel] [PATCH wave 1 00/10] support QEMU's "SMRAM at
> >> default SMBASE" feature
> >>
> >> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1512
> >> Repo: https://github.com/lersek/edk2.git
> >> Branch: smram_at_default_smbase_bz_1512_wave_1
> >>
> >> This is the firmware-side patch series for the QEMU feature that Igor is
> >> proposing in
> >>
> >> [Qemu-devel] [PATCH 0/2]
> >> q35: mch: allow to lock down 128K RAM at default SMBASE address
> >>
> >> posted at
> >>
> >> http://mid.mail-archive.com/20190917130708.10281-1-
> >> imammedo@redhat.com
> >> https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg03538.html
> >> https://edk2.groups.io/g/devel/message/47364
> >>
> >> This OVMF patch series is supposed to be the first "wave" of patch sets,
> >> working towards VCPU hotplug with SMM enabled.
> >>
> >> We want the hot-plugged VCPU to execute its very first instruction from
> >> SMRAM, running in SMM, for protection from OS and PCI DMA interference.
> >> In the (long!) discussion at
> >>
> >> http://mid.mail-archive.com/20190905174503.2acaa46a@redhat.com
> >> https://edk2.groups.io/g/devel/message/46910
> >> https://bugzilla.tianocore.org/show_bug.cgi?id=1512#c9
> >>
> >> we considered diverging from the specs and changing the default SMBASE
> >> to an address different from 0x3_0000, so that it would point into
> >> SMRAM. However, Igor had the idea to keep the default SMBASE intact, and
> >> replace the underlying system RAM (128 KB of it) with lockable SMRAM.
> >> This conforms to the language of the Intel SDM, namely, "[t]he actual
> >> physical location of the SMRAM can be in system memory or in a separate
> >> RAM memory". When firmware locks this QEMU-specific SMRAM, the latter
> >> completely disappears from the address space of code that runs outside
> >> of SMM (including PCI devices doing DMA). We call the remaining MMIO
> >> area a "black hole".
> >>
> >> The present patch set detects the QEMU feature, locks the SMRAM at the
> >> right times, and informs firmware and OS components to stay away form
> >> the SMRAM at the default SMBASE.
> >>
> >> I've tested the set extensively:
> >>
> >> http://mid.mail-archive.com/c18f1ada-3eca-d5f1-da4f-
> >> e74181c5a862@redhat.com
> >> https://edk2.groups.io/g/devel/message/47864
> >>
> >> Going forward, if I understand correctly, the plan is to populate the
> >> new SMRAM with the hotplug SMI handler. (This would likely be put in
> >> place by OvmfPkg/PlatformPei, from NASM source code. The
> >> SmmRelocateBases() function in UefiCpuPkg/PiSmmCpuDxeSmm backs up
> the
> >> original contents before, and restores them after, the initial SMBASE
> >> relocation of the cold-plugged VCPUs. Thus the hotplug SMI handler would
> >> survive the initial relocation of the cold-plugged VCPUs.)
> >>
> >> Further, when a VCPU is hot-plugged, we seem to intend to raise an SMI
> >> for it (perhaps internally to QEMU?), which will remain pending until
> >> the VCPU is launched with INIT-SIPI-SIPI. Once the INIT-SIPI-SIPI is
> >> sent, the VCPU will immediately enter SMM, and run the SMI handler in
> >> the locked SMRAM at the default SMBASE. At RSM, it may continue at the
> >> vector requested in the INIT-SIPI-SIPI.
> >>
> >> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> >> Cc: Brijesh Singh <brijesh.singh@amd.com>
> >> Cc: Igor Mammedov <imammedo@redhat.com>
> >> Cc: Jiewen Yao <jiewen.yao@intel.com>
> >> Cc: Joao M Martins <joao.m.martins@oracle.com>
> >> Cc: Jordan Justen <jordan.l.justen@intel.com>
> >> Cc: Jun Nakajima <jun.nakajima@intel.com>
> >> Cc: Michael Kinney <michael.d.kinney@intel.com>
> >> Cc: Paolo Bonzini <pbonzini@redhat.com>
> >> Cc: Phillip Goerl <phillip.goerl@oracle.com>
> >> Cc: Yingwen Chen <yingwen.chen@intel.com>
> >>
> >> Thanks
> >> Laszlo
> >>
> >> Laszlo Ersek (10):
> >> OvmfPkg: introduce PcdQ35SmramAtDefaultSmbase
> >> OvmfPkg/IndustryStandard: increase vertical whitespace in Q35 macro
> >> defs
> >> OvmfPkg/IndustryStandard: add MCH_DEFAULT_SMBASE* register macros
> >> OvmfPkg/PlatformPei: factor out Q35BoardVerification()
> >> OvmfPkg/PlatformPei: detect SMRAM at default SMBASE (skeleton)
> >> OvmfPkg/PlatformPei: assert there's no permanent PEI RAM at default
> >> SMBASE
> >> OvmfPkg/PlatformPei: reserve the SMRAM at the default SMBASE, if it
> >> exists
> >> OvmfPkg/SEV: don't manage the lifecycle of the SMRAM at the default
> >> SMBASE
> >> OvmfPkg/SmmAccess: close and lock SMRAM at default SMBASE
> >> OvmfPkg/PlatformPei: detect SMRAM at default SMBASE (for real)
> >>
> >> OvmfPkg/Include/IndustryStandard/Q35MchIch9.h | 106
> +++++++++++----
> >> -----
> >> OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c | 21 +++-
> >> OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf | 4 +
> >> OvmfPkg/OvmfPkg.dec | 6 ++
> >> OvmfPkg/OvmfPkgIa32.dsc | 1 +
> >> OvmfPkg/OvmfPkgIa32X64.dsc | 1 +
> >> OvmfPkg/OvmfPkgX64.dsc | 1 +
> >> OvmfPkg/PlatformPei/AmdSev.c | 24 ++++-
> >> OvmfPkg/PlatformPei/MemDetect.c | 87 +++++++++++++---
> >> OvmfPkg/PlatformPei/Platform.c | 24 +++++
> >> OvmfPkg/PlatformPei/Platform.h | 7 ++
> >> OvmfPkg/PlatformPei/PlatformPei.inf | 1 +
> >> OvmfPkg/SmmAccess/SmmAccess2Dxe.c | 7 ++
> >> OvmfPkg/SmmAccess/SmmAccess2Dxe.inf | 1 +
> >> OvmfPkg/SmmAccess/SmmAccessPei.c | 6 ++
> >> OvmfPkg/SmmAccess/SmmAccessPei.inf | 1 +
> >> OvmfPkg/SmmAccess/SmramInternal.c | 25 +++++
> >> OvmfPkg/SmmAccess/SmramInternal.h | 8 ++
> >> 18 files changed, 260 insertions(+), 71 deletions(-)
> >>
> >> --
> >> 2.19.1.3.g30247aa5d201
> >>
> >>
> >> -=-=-=-=-=-=
> >> Groups.io Links: You receive all messages sent to this group.
> >>
> >> View/Reply Online (#47924): https://edk2.groups.io/g/devel/message/47924
> >> Mute This Topic: https://groups.io/mt/34274934/1772286
> >> Group Owner: devel+owner@edk2.groups.io
> >> Unsubscribe: https://edk2.groups.io/g/devel/unsub [jiewen.yao@intel.com]
> >> -=-=-=-=-=-=
> >
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [edk2-devel] [PATCH wave 1 00/10] support QEMU's "SMRAM at default SMBASE" feature
2019-09-24 11:34 [PATCH wave 1 00/10] support QEMU's "SMRAM at default SMBASE" feature Laszlo Ersek
` (10 preceding siblings ...)
2019-09-26 8:46 ` [edk2-devel] [PATCH wave 1 00/10] support QEMU's "SMRAM at default SMBASE" feature Yao, Jiewen
@ 2019-09-27 11:35 ` Igor Mammedov
2019-10-01 15:31 ` Laszlo Ersek
11 siblings, 1 reply; 21+ messages in thread
From: Igor Mammedov @ 2019-09-27 11:35 UTC (permalink / raw)
To: Laszlo Ersek
Cc: devel, Ard Biesheuvel, Boris Ostrovsky, Brijesh Singh, Jiewen Yao,
Joao M Martins, Jordan Justen, Jun Nakajima, Michael Kinney,
Paolo Bonzini, Phillip Goerl, Yingwen Chen
On Tue, 24 Sep 2019 13:34:55 +0200
"Laszlo Ersek" <lersek@redhat.com> wrote:
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1512
> Repo: https://github.com/lersek/edk2.git
> Branch: smram_at_default_smbase_bz_1512_wave_1
>
> This is the firmware-side patch series for the QEMU feature that Igor is
> proposing in
>
> [Qemu-devel] [PATCH 0/2]
> q35: mch: allow to lock down 128K RAM at default SMBASE address
>
> posted at
>
> http://mid.mail-archive.com/20190917130708.10281-1-imammedo@redhat.com
> https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg03538.html
> https://edk2.groups.io/g/devel/message/47364
>
> This OVMF patch series is supposed to be the first "wave" of patch sets,
> working towards VCPU hotplug with SMM enabled.
>
> We want the hot-plugged VCPU to execute its very first instruction from
> SMRAM, running in SMM, for protection from OS and PCI DMA interference.
> In the (long!) discussion at
>
> http://mid.mail-archive.com/20190905174503.2acaa46a@redhat.com
> https://edk2.groups.io/g/devel/message/46910
> https://bugzilla.tianocore.org/show_bug.cgi?id=1512#c9
>
> we considered diverging from the specs and changing the default SMBASE
> to an address different from 0x3_0000, so that it would point into
> SMRAM. However, Igor had the idea to keep the default SMBASE intact, and
> replace the underlying system RAM (128 KB of it) with lockable SMRAM.
> This conforms to the language of the Intel SDM, namely, "[t]he actual
> physical location of the SMRAM can be in system memory or in a separate
> RAM memory". When firmware locks this QEMU-specific SMRAM, the latter
> completely disappears from the address space of code that runs outside
> of SMM (including PCI devices doing DMA). We call the remaining MMIO
> area a "black hole".
>
> The present patch set detects the QEMU feature, locks the SMRAM at the
> right times, and informs firmware and OS components to stay away form
> the SMRAM at the default SMBASE.
>
> I've tested the set extensively:
>
> http://mid.mail-archive.com/c18f1ada-3eca-d5f1-da4f-e74181c5a862@redhat.com
> https://edk2.groups.io/g/devel/message/47864
>
> Going forward, if I understand correctly, the plan is to populate the
> new SMRAM with the hotplug SMI handler. (This would likely be put in
> place by OvmfPkg/PlatformPei, from NASM source code. The
> SmmRelocateBases() function in UefiCpuPkg/PiSmmCpuDxeSmm backs up the
> original contents before, and restores them after, the initial SMBASE
> relocation of the cold-plugged VCPUs. Thus the hotplug SMI handler would
> survive the initial relocation of the cold-plugged VCPUs.)
that's the tricky part, can we safely detect (in SmmRelocateBases)
race condition in case of several hotplugged CPUs are executing
SMI relocation handler at the same time? (crashing system in case
that happens is good enough)
> Further, when a VCPU is hot-plugged, we seem to intend to raise an SMI
> for it (perhaps internally to QEMU?),
I'd rather rise SMI from guest side (using IO port write from
ACPI cpu hotplug handler). Which in typical case (QEMU negotiated
SMI broadcast) will trigger pending SMI on hotplugged CPU as well.
(if it's acceptable I can prepare corresponding QEMU patches)
In case of unicast SMIs, a CPU handling guest ACPI triggered
SMI, can send SMI to hotpluged CPU as an additional step.
> which will remain pending until
> the VCPU is launched with INIT-SIPI-SIPI. Once the INIT-SIPI-SIPI is
> sent, the VCPU will immediately enter SMM, and run the SMI handler in
> the locked SMRAM at the default SMBASE. At RSM, it may continue at the
> vector requested in the INIT-SIPI-SIPI.
Firmware can arbitrate/send INIT-SIPI-SIPI to hotplugged CPUs.
Enumerating hotplugged CPUs, can be done by reusing CPU hotplug
interface described at QEMU/docs/specs/acpi_cpu_hotplug.txt.
(preferably in read-only mode)
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Cc: Brijesh Singh <brijesh.singh@amd.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Joao M Martins <joao.m.martins@oracle.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Jun Nakajima <jun.nakajima@intel.com>
> Cc: Michael Kinney <michael.d.kinney@intel.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Phillip Goerl <phillip.goerl@oracle.com>
> Cc: Yingwen Chen <yingwen.chen@intel.com>
>
> Thanks
> Laszlo
>
> Laszlo Ersek (10):
> OvmfPkg: introduce PcdQ35SmramAtDefaultSmbase
> OvmfPkg/IndustryStandard: increase vertical whitespace in Q35 macro
> defs
> OvmfPkg/IndustryStandard: add MCH_DEFAULT_SMBASE* register macros
> OvmfPkg/PlatformPei: factor out Q35BoardVerification()
> OvmfPkg/PlatformPei: detect SMRAM at default SMBASE (skeleton)
> OvmfPkg/PlatformPei: assert there's no permanent PEI RAM at default
> SMBASE
> OvmfPkg/PlatformPei: reserve the SMRAM at the default SMBASE, if it
> exists
> OvmfPkg/SEV: don't manage the lifecycle of the SMRAM at the default
> SMBASE
> OvmfPkg/SmmAccess: close and lock SMRAM at default SMBASE
> OvmfPkg/PlatformPei: detect SMRAM at default SMBASE (for real)
>
> OvmfPkg/Include/IndustryStandard/Q35MchIch9.h | 106 +++++++++++---------
> OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c | 21 +++-
> OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf | 4 +
> OvmfPkg/OvmfPkg.dec | 6 ++
> OvmfPkg/OvmfPkgIa32.dsc | 1 +
> OvmfPkg/OvmfPkgIa32X64.dsc | 1 +
> OvmfPkg/OvmfPkgX64.dsc | 1 +
> OvmfPkg/PlatformPei/AmdSev.c | 24 ++++-
> OvmfPkg/PlatformPei/MemDetect.c | 87 +++++++++++++---
> OvmfPkg/PlatformPei/Platform.c | 24 +++++
> OvmfPkg/PlatformPei/Platform.h | 7 ++
> OvmfPkg/PlatformPei/PlatformPei.inf | 1 +
> OvmfPkg/SmmAccess/SmmAccess2Dxe.c | 7 ++
> OvmfPkg/SmmAccess/SmmAccess2Dxe.inf | 1 +
> OvmfPkg/SmmAccess/SmmAccessPei.c | 6 ++
> OvmfPkg/SmmAccess/SmmAccessPei.inf | 1 +
> OvmfPkg/SmmAccess/SmramInternal.c | 25 +++++
> OvmfPkg/SmmAccess/SmramInternal.h | 8 ++
> 18 files changed, 260 insertions(+), 71 deletions(-)
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [edk2-devel] [PATCH wave 1 00/10] support QEMU's "SMRAM at default SMBASE" feature
2019-09-27 1:14 ` Yao, Jiewen
@ 2019-10-01 14:43 ` Laszlo Ersek
0 siblings, 0 replies; 21+ messages in thread
From: Laszlo Ersek @ 2019-10-01 14:43 UTC (permalink / raw)
To: Yao, Jiewen
Cc: devel@edk2.groups.io, Ard Biesheuvel, Boris Ostrovsky,
Brijesh Singh, Igor Mammedov, Joao M Martins, Justen, Jordan L,
Nakajima, Jun, Kinney, Michael D, Paolo Bonzini, Phillip Goerl,
Chen, Yingwen, David Woodhouse
(+David)
On 09/27/19 03:14, Yao, Jiewen wrote:
> Sounds good.
>
> Maybe you can write that info in the commit message. A simple sentence such as :
> No CSM support is needed because legacy BIOS boot don't use SMM.
>
> So other people won't have same question in the future.
I've given this more thought. I can't decide what's the best approach,
without David's input.
David, here's the problem statement, stripped down: with a new QEMU
feature that's dynamically detected and enabled by the firmware, the
128KB memory area at 0x3_0000 becomes inaccessible to the OS. (This
subfeature is necessary for enabling the larger "VCPU hotplug with SMM"
feature). Right now, this patch series communcates that fact to the UEFI
OS through the UEFI memory map, but it's not told to a legacy OS through
the E820 memmap.
Which option do you prefer?
(1) The feature depends on building OVMF with -D SMM_REQUIRE. We can say
that -D SMM_REQUIRE and -D CSM_ENABLE are mutually exclusive, and add an
!error directive to the DSC files to catch a build that is passed both
-D flags.
This option is the most convenient for me, but it's likely not flexible
enough for users that have no access to hypervisor configuration (such
as QEMU cmdline options and/or firmware binary choice). They might want
to pick UEFI boot vs. CSM boot "unpredictably".
(2) Jiewen suggested a patch for LegacyBiosBuildE820()
[OvmfPkg/Csm/LegacyBiosDxe/LegacyBootSupport.c], punching a hole into
E820Table[0].
While this would make the CSM honest about the memory map, to legacy
OSes, I strongly doubt legacy OSes would actually *work* with such a
memory map (i.e. with 128 KB missing in the middle of the <=640KB RAM).
It would require a bunch of testing anyway, and it could break with any
newly tried legacy OS.
(3) Modify OvmfPkg/SmmAccess/SmmAccess2Dxe to set up an event
notification function for the "gEfiEventLegacyBootGuid" event group, in
case the feature has been detected in PlatformPei. In the notification
function, log an error message, call ASSERT (FALSE), and call
CpuDeadLoop ().
Advantages:
- when OVMF is built without -D SMM_REQUIRE, SmmAccess2Dxe is not included
- when OVMF is built without -D CSM_REQUIRE, then the event group is
never signaled -- the only EfiSignalEventLegacyBoot() call is in
"OvmfPkg/Csm/LegacyBiosDxe/LegacyBootSupport.c".
- when OVMF is built with both options, but no legacy boot is performed,
the callback does nothing
- the callback catches the invalid constellation and prevents the boot
cleanly. The error message in the OVMF debug log would instruct users to
disable the dynamically detectable feature on the QEMU command line, and
to boot again.
Disadvantages:
- users have to look at the OVMF log
- users may not have the power to change the QEMU configuration -- if
they had that power, they'd simply use SeaBIOS for booting the legacy
OS, not OVMF+CSM.
(4) Identify a CSM build in time for the feature detection (in
PlatformPei), and force-disable the feature (don't even attempt
detecting it from QEMU) if this is a CSM build.
In the DXE phase (and especially in the BDS phase), we can detect a CSM
build, from the presence of gEfiLegacyBiosProtocolGuid in the UEFI
protocol database. However, the protocol is not available in the PEI
phase, which is when this particular QEMU feature has to be enabled.
Therefore we could introduce a dedicated FeaturePCD, which would reflect
-D CSM_ENABLE. When set, the PCD would short-circuit the dynamic
detection in PlatformPei, and pretend that the feature doesn't exist.
This option would allow -D SMM_REQUIRE and -D CSM_ENABLE to co-exist
(like they do now); the latter would only disable the new feature
(regardless of QEMU offering it), not all of SMM. With -D CSM_ENABLE,
users would not lose the protection they get from SMM_REQUIRE when
booting a UEFI OS; they'd only lose VCPU hotplug capability.
My preference is (1), but I never use -D CSM_ENABLE.
David, do you see (or do you foresee) OVMF binaries that are built with
*both* of -D CSM_ENABLE and -D SMM_REQUIRE?
> With the commit message change, the series reviewed-by: jiewen.yao@intel.com
Thank you Jiewen -- it's possible that I'll post a v2, based on David's
answer, and then I'll pick up your R-b for unchanged patches from v1.
Thanks
Laszlo
>
>> -----Original Message-----
>> From: Laszlo Ersek <lersek@redhat.com>
>> Sent: Thursday, September 26, 2019 10:52 PM
>> To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Boris Ostrovsky
>> <boris.ostrovsky@oracle.com>; Brijesh Singh <brijesh.singh@amd.com>; Igor
>> Mammedov <imammedo@redhat.com>; Joao M Martins
>> <joao.m.martins@oracle.com>; Justen, Jordan L <jordan.l.justen@intel.com>;
>> Nakajima, Jun <jun.nakajima@intel.com>; Kinney, Michael D
>> <michael.d.kinney@intel.com>; Paolo Bonzini <pbonzini@redhat.com>; Phillip
>> Goerl <phillip.goerl@oracle.com>; Chen, Yingwen <yingwen.chen@intel.com>
>> Subject: Re: [edk2-devel] [PATCH wave 1 00/10] support QEMU's "SMRAM at
>> default SMBASE" feature
>>
>> On 09/26/19 10:46, Yao, Jiewen wrote:
>>> Hi Laszlo
>>> May I know if you want to support legacy BIOS?
>>
>> No, thanks.
>>
>> The end-goal is to implement VCPU hotplug at OS runtime, when the OS is
>> booted with OVMF, Secure Boot is enabled, and the firmware is protected
>> with SMM.
>>
>> In other words, the goal is to support VCPU hotplug when there is a
>> privilege barrier between the firmware and the OS. (The whole complexity
>> of the feature arises because a hotplugged VCPU is a risk for that barrier.)
>>
>> With legacy BIOS, there is no such barrier, and VCPU hotplug already
>> works with SeaBIOS.
>>
>> Furthermore, if you build OVMF *without* "-D SMM_REQUIRE", then there is
>> no such barrier again, and VCPU hotplug already works well, in that case
>> too. (I had tested it extensively, in RHBZ#1454803.)
>>
>> In effect, I consider "-D SMM_REQUIRE" and "-D CSM_ENABLE" mutually
>> exclusive, for OVMF. And the present work is only relevant for "-D
>> SMM_REQUIRE".
>>
>> We could perhaps add an "!error" directive to the OVMF DSC files,
>> conditional on both flags being enabled (SMM_REQUIRE and CSM_ENABLE). I
>> might propose such a patch in the future, but until it becomes a
>> practical problem, I don't want to spend cycles on it.
>>
>> Thanks
>> Laszlo
>>
>>> The legacy BIOS memory map is reported in E820, which is different with UEFI
>> memory map.
>>>
>>> In OvmfPkg\Csm\LegacyBiosDxe\LegacyBootSupport.c, LegacyBiosBuildE820()
>> will hardcode below 640K address to be OS usage without consulting UEFI
>> memory map.
>>>
>>> //
>>> // First entry is 0 to (640k - EBDA)
>>> //
>>> ACCESS_PAGE0_CODE (
>>> E820Table[0].BaseAddr = 0;
>>> E820Table[0].Length = (UINT64) ((*(UINT16 *) (UINTN)0x40E) << 4);
>>> E820Table[0].Type = EfiAcpiAddressRangeMemory;
>>> );
>>>
>>> If we want to support this feature in legacy BIOS, we also need reserve the
>> black hole here.
>>>
>>> Thank you
>>> Yao Jiewen
>>>
>>>
>>>> -----Original Message-----
>>>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo
>> Ersek
>>>> Sent: Tuesday, September 24, 2019 7:35 PM
>>>> To: edk2-devel-groups-io <devel@edk2.groups.io>
>>>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Boris Ostrovsky
>>>> <boris.ostrovsky@oracle.com>; Brijesh Singh <brijesh.singh@amd.com>; Igor
>>>> Mammedov <imammedo@redhat.com>; Yao, Jiewen
>> <jiewen.yao@intel.com>;
>>>> Joao M Martins <joao.m.martins@oracle.com>; Justen, Jordan L
>>>> <jordan.l.justen@intel.com>; Nakajima, Jun <jun.nakajima@intel.com>;
>> Kinney,
>>>> Michael D <michael.d.kinney@intel.com>; Paolo Bonzini
>>>> <pbonzini@redhat.com>; Phillip Goerl <phillip.goerl@oracle.com>; Chen,
>>>> Yingwen <yingwen.chen@intel.com>
>>>> Subject: [edk2-devel] [PATCH wave 1 00/10] support QEMU's "SMRAM at
>>>> default SMBASE" feature
>>>>
>>>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1512
>>>> Repo: https://github.com/lersek/edk2.git
>>>> Branch: smram_at_default_smbase_bz_1512_wave_1
>>>>
>>>> This is the firmware-side patch series for the QEMU feature that Igor is
>>>> proposing in
>>>>
>>>> [Qemu-devel] [PATCH 0/2]
>>>> q35: mch: allow to lock down 128K RAM at default SMBASE address
>>>>
>>>> posted at
>>>>
>>>> http://mid.mail-archive.com/20190917130708.10281-1-
>>>> imammedo@redhat.com
>>>> https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg03538.html
>>>> https://edk2.groups.io/g/devel/message/47364
>>>>
>>>> This OVMF patch series is supposed to be the first "wave" of patch sets,
>>>> working towards VCPU hotplug with SMM enabled.
>>>>
>>>> We want the hot-plugged VCPU to execute its very first instruction from
>>>> SMRAM, running in SMM, for protection from OS and PCI DMA interference.
>>>> In the (long!) discussion at
>>>>
>>>> http://mid.mail-archive.com/20190905174503.2acaa46a@redhat.com
>>>> https://edk2.groups.io/g/devel/message/46910
>>>> https://bugzilla.tianocore.org/show_bug.cgi?id=1512#c9
>>>>
>>>> we considered diverging from the specs and changing the default SMBASE
>>>> to an address different from 0x3_0000, so that it would point into
>>>> SMRAM. However, Igor had the idea to keep the default SMBASE intact, and
>>>> replace the underlying system RAM (128 KB of it) with lockable SMRAM.
>>>> This conforms to the language of the Intel SDM, namely, "[t]he actual
>>>> physical location of the SMRAM can be in system memory or in a separate
>>>> RAM memory". When firmware locks this QEMU-specific SMRAM, the latter
>>>> completely disappears from the address space of code that runs outside
>>>> of SMM (including PCI devices doing DMA). We call the remaining MMIO
>>>> area a "black hole".
>>>>
>>>> The present patch set detects the QEMU feature, locks the SMRAM at the
>>>> right times, and informs firmware and OS components to stay away form
>>>> the SMRAM at the default SMBASE.
>>>>
>>>> I've tested the set extensively:
>>>>
>>>> http://mid.mail-archive.com/c18f1ada-3eca-d5f1-da4f-
>>>> e74181c5a862@redhat.com
>>>> https://edk2.groups.io/g/devel/message/47864
>>>>
>>>> Going forward, if I understand correctly, the plan is to populate the
>>>> new SMRAM with the hotplug SMI handler. (This would likely be put in
>>>> place by OvmfPkg/PlatformPei, from NASM source code. The
>>>> SmmRelocateBases() function in UefiCpuPkg/PiSmmCpuDxeSmm backs up
>> the
>>>> original contents before, and restores them after, the initial SMBASE
>>>> relocation of the cold-plugged VCPUs. Thus the hotplug SMI handler would
>>>> survive the initial relocation of the cold-plugged VCPUs.)
>>>>
>>>> Further, when a VCPU is hot-plugged, we seem to intend to raise an SMI
>>>> for it (perhaps internally to QEMU?), which will remain pending until
>>>> the VCPU is launched with INIT-SIPI-SIPI. Once the INIT-SIPI-SIPI is
>>>> sent, the VCPU will immediately enter SMM, and run the SMI handler in
>>>> the locked SMRAM at the default SMBASE. At RSM, it may continue at the
>>>> vector requested in the INIT-SIPI-SIPI.
>>>>
>>>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>>> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>>>> Cc: Brijesh Singh <brijesh.singh@amd.com>
>>>> Cc: Igor Mammedov <imammedo@redhat.com>
>>>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>>>> Cc: Joao M Martins <joao.m.martins@oracle.com>
>>>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>>>> Cc: Jun Nakajima <jun.nakajima@intel.com>
>>>> Cc: Michael Kinney <michael.d.kinney@intel.com>
>>>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>>>> Cc: Phillip Goerl <phillip.goerl@oracle.com>
>>>> Cc: Yingwen Chen <yingwen.chen@intel.com>
>>>>
>>>> Thanks
>>>> Laszlo
>>>>
>>>> Laszlo Ersek (10):
>>>> OvmfPkg: introduce PcdQ35SmramAtDefaultSmbase
>>>> OvmfPkg/IndustryStandard: increase vertical whitespace in Q35 macro
>>>> defs
>>>> OvmfPkg/IndustryStandard: add MCH_DEFAULT_SMBASE* register macros
>>>> OvmfPkg/PlatformPei: factor out Q35BoardVerification()
>>>> OvmfPkg/PlatformPei: detect SMRAM at default SMBASE (skeleton)
>>>> OvmfPkg/PlatformPei: assert there's no permanent PEI RAM at default
>>>> SMBASE
>>>> OvmfPkg/PlatformPei: reserve the SMRAM at the default SMBASE, if it
>>>> exists
>>>> OvmfPkg/SEV: don't manage the lifecycle of the SMRAM at the default
>>>> SMBASE
>>>> OvmfPkg/SmmAccess: close and lock SMRAM at default SMBASE
>>>> OvmfPkg/PlatformPei: detect SMRAM at default SMBASE (for real)
>>>>
>>>> OvmfPkg/Include/IndustryStandard/Q35MchIch9.h | 106
>> +++++++++++----
>>>> -----
>>>> OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c | 21 +++-
>>>> OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf | 4 +
>>>> OvmfPkg/OvmfPkg.dec | 6 ++
>>>> OvmfPkg/OvmfPkgIa32.dsc | 1 +
>>>> OvmfPkg/OvmfPkgIa32X64.dsc | 1 +
>>>> OvmfPkg/OvmfPkgX64.dsc | 1 +
>>>> OvmfPkg/PlatformPei/AmdSev.c | 24 ++++-
>>>> OvmfPkg/PlatformPei/MemDetect.c | 87 +++++++++++++---
>>>> OvmfPkg/PlatformPei/Platform.c | 24 +++++
>>>> OvmfPkg/PlatformPei/Platform.h | 7 ++
>>>> OvmfPkg/PlatformPei/PlatformPei.inf | 1 +
>>>> OvmfPkg/SmmAccess/SmmAccess2Dxe.c | 7 ++
>>>> OvmfPkg/SmmAccess/SmmAccess2Dxe.inf | 1 +
>>>> OvmfPkg/SmmAccess/SmmAccessPei.c | 6 ++
>>>> OvmfPkg/SmmAccess/SmmAccessPei.inf | 1 +
>>>> OvmfPkg/SmmAccess/SmramInternal.c | 25 +++++
>>>> OvmfPkg/SmmAccess/SmramInternal.h | 8 ++
>>>> 18 files changed, 260 insertions(+), 71 deletions(-)
>>>>
>>>> --
>>>> 2.19.1.3.g30247aa5d201
>>>>
>>>>
>>>> -=-=-=-=-=-=
>>>> Groups.io Links: You receive all messages sent to this group.
>>>>
>>>> View/Reply Online (#47924): https://edk2.groups.io/g/devel/message/47924
>>>> Mute This Topic: https://groups.io/mt/34274934/1772286
>>>> Group Owner: devel+owner@edk2.groups.io
>>>> Unsubscribe: https://edk2.groups.io/g/devel/unsub [jiewen.yao@intel.com]
>>>> -=-=-=-=-=-=
>>>
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [edk2-devel] [PATCH wave 1 00/10] support QEMU's "SMRAM at default SMBASE" feature
2019-09-27 11:35 ` Igor Mammedov
@ 2019-10-01 15:31 ` Laszlo Ersek
2019-10-04 14:09 ` Igor Mammedov
0 siblings, 1 reply; 21+ messages in thread
From: Laszlo Ersek @ 2019-10-01 15:31 UTC (permalink / raw)
To: Igor Mammedov
Cc: devel, Ard Biesheuvel, Boris Ostrovsky, Brijesh Singh, Jiewen Yao,
Joao M Martins, Jordan Justen, Jun Nakajima, Michael Kinney,
Paolo Bonzini, Phillip Goerl, Yingwen Chen
On 09/27/19 13:35, Igor Mammedov wrote:
> On Tue, 24 Sep 2019 13:34:55 +0200
> "Laszlo Ersek" <lersek@redhat.com> wrote:
>> Going forward, if I understand correctly, the plan is to populate the
>> new SMRAM with the hotplug SMI handler. (This would likely be put in
>> place by OvmfPkg/PlatformPei, from NASM source code. The
>> SmmRelocateBases() function in UefiCpuPkg/PiSmmCpuDxeSmm backs up the
>> original contents before, and restores them after, the initial SMBASE
>> relocation of the cold-plugged VCPUs. Thus the hotplug SMI handler would
>> survive the initial relocation of the cold-plugged VCPUs.)
> that's the tricky part, can we safely detect (in SmmRelocateBases)
> race condition in case of several hotplugged CPUs are executing
> SMI relocation handler at the same time? (crashing system in case
> that happens is good enough)
Wait, there are *two* initial SMI handlers here, one for cold-plugged
CPUs, and another for hot-plugged CPUs. Let's just call them coldplug
and hotplug handlers, for simplicity.
In chronological order, during boot:
(1) OvmfPkg/PlatformPei would put the hotplug handler in place, at the
default SMBASE. This code would lie dormant for a long time.
(2) UefiCpuPkg/PiSmmCpuDxeSmm backs up the current contents of the
default SMBASE area, and installs the coldplug handler. This handler is
then actually used, for relocating SMBASE for the present (cold-plugged)
CPUs. Finally, UefiCpuPkg/PiSmmCpuDxeSmm restores the original contents
of the default SMBASE area. This would in effect re-establish the
hotplug handler from (1).
(3) At OS runtime, a CPU is hotplugged, and then the hotplug handler
runs, at the default SMBASE.
The function SmmRelocateBases() is strictly limited to step (2), and has
nothing to do with hotplug. Therefore it need not deal with any race
conditions related to multi-hotplug.
This is just to clarify that your question applies to the initial SMI
handler (the hotplug one) that is put in place in step (1), and then
used in step (3). The question does not apply to SmmRelocateBases().
With that out of the way, I do think we should be able to detect this,
with a simple -- haha, famous last words! -- compare-and-exchange (LOCK
CMPXCHG); a kind of semaphore.
The initial value of an integer variable in SMRAM should be 1. At the
start of the hotplug initial SMI handler, the CPU should try to swap
that with 0. If it succeeds, proceed. If it fails (the variable's value
is not 1), force a platform reset. Finally, right before the RSM,
restore 1 (swap it back).
(It does not matter if another hotplug CPU starts the relocation in SMM
while the earlier one is left with *only* the RSM instruction in SMM,
immediately after the swap-back.)
Alternatively, if it's friendlier or simpler, we can spin on the initial
LOCK CMPXCHG until it succeeds, executing PAUSE in the loop body. That
should be friendly with KVM too (due to pause loop exiting).
>> Further, when a VCPU is hot-plugged, we seem to intend to raise an SMI
>> for it (perhaps internally to QEMU?),
> I'd rather rise SMI from guest side (using IO port write from
> ACPI cpu hotplug handler). Which in typical case (QEMU negotiated
> SMI broadcast) will trigger pending SMI on hotplugged CPU as well.
> (if it's acceptable I can prepare corresponding QEMU patches)
What if the OS does not send the SMI (from the GPE handler) at once, and
boots the new CPU instead?
Previously, you wrote that the new CPU "won't get access to privileged
SMRAM so OS can't subvert firmware. The next time SMI broadcast is sent
the CPU will use SMI handler at default 30000 SMBASE. It's up to us to
define behavior here (for example relocation handler can put such CPU in
shutdown state)."
How can we discover in the hotplug initial SMI handler at 0x3_0000 that
the CPU executing the code should have been relocated *earlier*, and the
OS just didn't obey the ACPI GPE code? I think a platform reset would be
a proper response, but I don't know how to tell, "this CPU should not be
here".
> In case of unicast SMIs, a CPU handling guest ACPI triggered
> SMI, can send SMI to hotpluged CPU as an additional step.
I think it's OK if we restrict this feature to "broadcast SMI" only.
>
>> which will remain pending until
>> the VCPU is launched with INIT-SIPI-SIPI. Once the INIT-SIPI-SIPI is
>> sent, the VCPU will immediately enter SMM, and run the SMI handler in
>> the locked SMRAM at the default SMBASE. At RSM, it may continue at the
>> vector requested in the INIT-SIPI-SIPI.
> Firmware can arbitrate/send INIT-SIPI-SIPI to hotplugged CPUs.
> Enumerating hotplugged CPUs, can be done by reusing CPU hotplug
> interface described at QEMU/docs/specs/acpi_cpu_hotplug.txt.
> (preferably in read-only mode)
... I'll have to look at that later :)
Thanks
Laszlo
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [edk2-devel] [PATCH wave 1 00/10] support QEMU's "SMRAM at default SMBASE" feature
2019-10-01 15:31 ` Laszlo Ersek
@ 2019-10-04 14:09 ` Igor Mammedov
2019-10-07 9:34 ` Laszlo Ersek
0 siblings, 1 reply; 21+ messages in thread
From: Igor Mammedov @ 2019-10-04 14:09 UTC (permalink / raw)
To: Laszlo Ersek
Cc: devel, Ard Biesheuvel, Boris Ostrovsky, Brijesh Singh, Jiewen Yao,
Joao M Martins, Jordan Justen, Jun Nakajima, Michael Kinney,
Paolo Bonzini, Phillip Goerl, Yingwen Chen
On Tue, 1 Oct 2019 17:31:17 +0200
"Laszlo Ersek" <lersek@redhat.com> wrote:
> On 09/27/19 13:35, Igor Mammedov wrote:
> > On Tue, 24 Sep 2019 13:34:55 +0200
> > "Laszlo Ersek" <lersek@redhat.com> wrote:
>
> >> Going forward, if I understand correctly, the plan is to populate the
> >> new SMRAM with the hotplug SMI handler. (This would likely be put in
> >> place by OvmfPkg/PlatformPei, from NASM source code. The
> >> SmmRelocateBases() function in UefiCpuPkg/PiSmmCpuDxeSmm backs up the
> >> original contents before, and restores them after, the initial SMBASE
> >> relocation of the cold-plugged VCPUs. Thus the hotplug SMI handler would
> >> survive the initial relocation of the cold-plugged VCPUs.)
>
> > that's the tricky part, can we safely detect (in SmmRelocateBases)
> > race condition in case of several hotplugged CPUs are executing
> > SMI relocation handler at the same time? (crashing system in case
> > that happens is good enough)
>
> Wait, there are *two* initial SMI handlers here, one for cold-plugged
> CPUs, and another for hot-plugged CPUs. Let's just call them coldplug
> and hotplug handlers, for simplicity.
>
> In chronological order, during boot:
>
> (1) OvmfPkg/PlatformPei would put the hotplug handler in place, at the
> default SMBASE. This code would lie dormant for a long time.
>
> (2) UefiCpuPkg/PiSmmCpuDxeSmm backs up the current contents of the
> default SMBASE area, and installs the coldplug handler. This handler is
> then actually used, for relocating SMBASE for the present (cold-plugged)
> CPUs. Finally, UefiCpuPkg/PiSmmCpuDxeSmm restores the original contents
> of the default SMBASE area. This would in effect re-establish the
> hotplug handler from (1).
>
> (3) At OS runtime, a CPU is hotplugged, and then the hotplug handler
> runs, at the default SMBASE.
>
> The function SmmRelocateBases() is strictly limited to step (2), and has
> nothing to do with hotplug. Therefore it need not deal with any race
> conditions related to multi-hotplug.
>
> This is just to clarify that your question applies to the initial SMI
> handler (the hotplug one) that is put in place in step (1), and then
> used in step (3). The question does not apply to SmmRelocateBases().
>
>
> With that out of the way, I do think we should be able to detect this,
> with a simple -- haha, famous last words! -- compare-and-exchange (LOCK
> CMPXCHG); a kind of semaphore.
>
> The initial value of an integer variable in SMRAM should be 1. At the
> start of the hotplug initial SMI handler, the CPU should try to swap
> that with 0. If it succeeds, proceed. If it fails (the variable's value
> is not 1), force a platform reset. Finally, right before the RSM,
> restore 1 (swap it back).
>
> (It does not matter if another hotplug CPU starts the relocation in SMM
> while the earlier one is left with *only* the RSM instruction in SMM,
> immediately after the swap-back.)
I don't see why it doesn't matter, let me illustrate the case
I'm talking about.
assuming there are 2 hotplugged CPUs with pending SMI and
stray INIT-SIPI-SIPI in flight to CPU2.
HP CPU1 HP CPU2
save state at 0x30000 -
start executing hotplug SMI handler -
set new SMBASE -
- save state at 0x30000
(overwrites CPU1 set SMBASE to default one)
- .....
-----------zzzzzzz----------
RSM
restores CPU1 with CPU2 saved state
incl. CPU2 SMBASE = either default or
already new CPU2 specific one
semaphore is irrelevant in this case as we will loose CPU1 relocation
at best (assuming hotplug handler does nothing else beside relocation)
and if hotplug handler does something else this case will probably
leave FW in inconsistent state.
Safest way to deal with it would be:
1. wait till all host CPUs are brought into SMM (with hotplug SMI handler = check me in)
2. wait till all hotplugged CPUs are put in well know state
(a host cpu should send INIT-SIPI-SIPI with NOP vector to wake up)
3. a host CPU will serialize hotplugged CPUs relocation by sending
uincast SMI + INIT-SIPI-SIPI NOP vector to wake up the second time
(with hotplug SMI handler = relocate_me)
alternatively we could skip step 3 and do following:
hotplug_SMI_handler()
hp_cpu_check_in_map[apic_id] = 1;
/* wait till another cpu tells me to continue */
while(hp_cpu_check_in_map[apic_id]) ;;
...
host_cpu_hotplug_all_cpus()
wait till all hotpluged CPUs are in hp_cpu_check_in_map;
for(i=0;;) {
if (hp_cpu_check_in_map[i]) {
/* relocate this CPU */
hp_cpu_check_in_map[i] = 0;
}
tell QEMU that FW pulled the CPU in;
/* we can add a flag to QEMU's cpu hotplug MMIO interface to FW do it,
so that legitimate GPE handler would tell OS to online only
firmware vetted CPUs */
}
> Alternatively, if it's friendlier or simpler, we can spin on the initial
> LOCK CMPXCHG until it succeeds, executing PAUSE in the loop body. That
> should be friendly with KVM too (due to pause loop exiting).
>
>
> >> Further, when a VCPU is hot-plugged, we seem to intend to raise an SMI
> >> for it (perhaps internally to QEMU?),
>
> > I'd rather rise SMI from guest side (using IO port write from
> > ACPI cpu hotplug handler). Which in typical case (QEMU negotiated
> > SMI broadcast) will trigger pending SMI on hotplugged CPU as well.
> > (if it's acceptable I can prepare corresponding QEMU patches)
>
> What if the OS does not send the SMI (from the GPE handler) at once, and
> boots the new CPU instead?
>
> Previously, you wrote that the new CPU "won't get access to privileged
> SMRAM so OS can't subvert firmware. The next time SMI broadcast is sent
> the CPU will use SMI handler at default 30000 SMBASE. It's up to us to
> define behavior here (for example relocation handler can put such CPU in
> shutdown state)."
>
> How can we discover in the hotplug initial SMI handler at 0x3_0000 that
> the CPU executing the code should have been relocated *earlier*, and the
> OS just didn't obey the ACPI GPE code? I think a platform reset would be
> a proper response, but I don't know how to tell, "this CPU should not be
> here".
one can ask QEMU about present CPUs (via cpu hotplug interface)
and compare with present CPU state in FW (OS can hide insert
event there but not the presence).
another way is to keep it pinned in relocation SMI handler
until another CPU tells it to continue with relocation.
In absence of SMI, such CPU will continue to run wild but do we
really care about it?
> > In case of unicast SMIs, a CPU handling guest ACPI triggered
> > SMI, can send SMI to hotpluged CPU as an additional step.
>
> I think it's OK if we restrict this feature to "broadcast SMI" only.
>
> >
> >> which will remain pending until
> >> the VCPU is launched with INIT-SIPI-SIPI. Once the INIT-SIPI-SIPI is
> >> sent, the VCPU will immediately enter SMM, and run the SMI handler in
> >> the locked SMRAM at the default SMBASE. At RSM, it may continue at the
> >> vector requested in the INIT-SIPI-SIPI.
> > Firmware can arbitrate/send INIT-SIPI-SIPI to hotplugged CPUs.
> > Enumerating hotplugged CPUs, can be done by reusing CPU hotplug
> > interface described at QEMU/docs/specs/acpi_cpu_hotplug.txt.
> > (preferably in read-only mode)
>
> ... I'll have to look at that later :)
>
> Thanks
> Laszlo
>
>
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [edk2-devel] [PATCH wave 1 00/10] support QEMU's "SMRAM at default SMBASE" feature
2019-10-04 14:09 ` Igor Mammedov
@ 2019-10-07 9:34 ` Laszlo Ersek
0 siblings, 0 replies; 21+ messages in thread
From: Laszlo Ersek @ 2019-10-07 9:34 UTC (permalink / raw)
To: devel, imammedo
Cc: Ard Biesheuvel, Boris Ostrovsky, Brijesh Singh, Jiewen Yao,
Joao M Martins, Jordan Justen, Jun Nakajima, Michael Kinney,
Paolo Bonzini, Phillip Goerl, Yingwen Chen
On 10/04/19 16:09, Igor Mammedov wrote:
> On Tue, 1 Oct 2019 17:31:17 +0200
> "Laszlo Ersek" <lersek@redhat.com> wrote:
>> (It does not matter if another hotplug CPU starts the relocation in SMM
>> while the earlier one is left with *only* the RSM instruction in SMM,
>> immediately after the swap-back.)
> I don't see why it doesn't matter, let me illustrate the case
> I'm talking about.
Right, I'm sorry -- I completely forgot that the RSM instruction is what
makes the newly set SMBASE permanent!
[...]
> Safest way to deal with it would be:
>
> 1. wait till all host CPUs are brought into SMM (with hotplug SMI handler = check me in)
>
> 2. wait till all hotplugged CPUs are put in well know state
> (a host cpu should send INIT-SIPI-SIPI with NOP vector to wake up)
>
> 3. a host CPU will serialize hotplugged CPUs relocation by sending
> uincast SMI + INIT-SIPI-SIPI NOP vector to wake up the second time
> (with hotplug SMI handler = relocate_me)
>
> alternatively we could skip step 3 and do following:
>
> hotplug_SMI_handler()
> hp_cpu_check_in_map[apic_id] = 1;
> /* wait till another cpu tells me to continue */
> while(hp_cpu_check_in_map[apic_id]) ;;
> ...
>
> host_cpu_hotplug_all_cpus()
> wait till all hotpluged CPUs are in hp_cpu_check_in_map;
> for(i=0;;) {
> if (hp_cpu_check_in_map[i]) {
> /* relocate this CPU */
> hp_cpu_check_in_map[i] = 0;
> }
>
> tell QEMU that FW pulled the CPU in;
> /* we can add a flag to QEMU's cpu hotplug MMIO interface to FW do it,
> so that legitimate GPE handler would tell OS to online only
> firmware vetted CPUs */
> }
[...]
>> How can we discover in the hotplug initial SMI handler at 0x3_0000 that
>> the CPU executing the code should have been relocated *earlier*, and the
>> OS just didn't obey the ACPI GPE code? I think a platform reset would be
>> a proper response, but I don't know how to tell, "this CPU should not be
>> here".
>
> one can ask QEMU about present CPUs (via cpu hotplug interface)
> and compare with present CPU state in FW (OS can hide insert
> event there but not the presence).
>
> another way is to keep it pinned in relocation SMI handler
> until another CPU tells it to continue with relocation.
>
> In absence of SMI, such CPU will continue to run wild but do we
> really care about it?
Thanks.
It's clear that, for me, too much hangs in the air right now. I'll have
to get my hands dirty and experiment. I need to learn a lot about this
area of edk2 as well, and experimenting looks like one way.
Let's return to these question once we reach the following state:
- ACPI code generated by QEMU raises a broadcast SMI on VCPU hotplug
- in OVMF, a cold-plugged CPU executes platform code, in a custom SMI
handler
- in OVMF, a hot-plugged CPU executes platform code, in the initial
(default) SMI handler.
Once we get to that point, we can look into further details.
I'll answer under your other email too:
<http://mid.mail-archive.com/20191004133052.20373155@redhat.com>.
Thanks
Laszlo
^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2019-10-07 9:34 UTC | newest]
Thread overview: 21+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-09-24 11:34 [PATCH wave 1 00/10] support QEMU's "SMRAM at default SMBASE" feature Laszlo Ersek
2019-09-24 11:34 ` [PATCH wave 1 01/10] OvmfPkg: introduce PcdQ35SmramAtDefaultSmbase Laszlo Ersek
2019-09-24 11:34 ` [PATCH wave 1 02/10] OvmfPkg/IndustryStandard: increase vertical whitespace in Q35 macro defs Laszlo Ersek
2019-09-24 11:44 ` [edk2-devel] " Philippe Mathieu-Daudé
2019-09-24 11:34 ` [PATCH wave 1 03/10] OvmfPkg/IndustryStandard: add MCH_DEFAULT_SMBASE* register macros Laszlo Ersek
2019-09-24 11:34 ` [PATCH wave 1 04/10] OvmfPkg/PlatformPei: factor out Q35BoardVerification() Laszlo Ersek
2019-09-24 11:41 ` [edk2-devel] " Philippe Mathieu-Daudé
2019-09-24 11:35 ` [PATCH wave 1 05/10] OvmfPkg/PlatformPei: detect SMRAM at default SMBASE (skeleton) Laszlo Ersek
2019-09-24 11:35 ` [PATCH wave 1 06/10] OvmfPkg/PlatformPei: assert there's no permanent PEI RAM at default SMBASE Laszlo Ersek
2019-09-24 11:35 ` [PATCH wave 1 07/10] OvmfPkg/PlatformPei: reserve the SMRAM at the default SMBASE, if it exists Laszlo Ersek
2019-09-24 11:35 ` [PATCH wave 1 08/10] OvmfPkg/SEV: don't manage the lifecycle of the SMRAM at the default SMBASE Laszlo Ersek
2019-09-24 11:35 ` [PATCH wave 1 09/10] OvmfPkg/SmmAccess: close and lock SMRAM at " Laszlo Ersek
2019-09-24 11:35 ` [PATCH wave 1 10/10] OvmfPkg/PlatformPei: detect SMRAM at default SMBASE (for real) Laszlo Ersek
2019-09-26 8:46 ` [edk2-devel] [PATCH wave 1 00/10] support QEMU's "SMRAM at default SMBASE" feature Yao, Jiewen
2019-09-26 14:51 ` Laszlo Ersek
2019-09-27 1:14 ` Yao, Jiewen
2019-10-01 14:43 ` Laszlo Ersek
2019-09-27 11:35 ` Igor Mammedov
2019-10-01 15:31 ` Laszlo Ersek
2019-10-04 14:09 ` Igor Mammedov
2019-10-07 9:34 ` Laszlo Ersek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox