* [PATCH 0/5] OvmfPkg: improve SMM comms security with adaptive MemoryTypeInformation
@ 2020-03-10 22:27 Laszlo Ersek
2020-03-10 22:27 ` [PATCH 1/5] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: drop unused PCDs Laszlo Ersek
` (4 more replies)
0 siblings, 5 replies; 18+ messages in thread
From: Laszlo Ersek @ 2020-03-10 22:27 UTC (permalink / raw)
To: edk2-devel-groups-io
Cc: Ard Biesheuvel, Jordan Justen, Philippe Mathieu-Daudé
Repo: https://pagure.io/lersek/edk2.git
Branch: mem_type_info
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=386
The following "A Tour Beyond BIOS" whitepapers, available at
https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-white-papers
discuss the WSMT ACPI table:
- "Secure SMM Communication"
- "Memory Protection in UEFI BIOS"
- "Memory Map and Practices in UEFI BIOS"
With time, OVMF should install the WSMT ACPI table. I made an effort to
analyze what was missing for that, here:
[edk2-devel] WSMT bits
http://mid.mail-archive.com/3c32815c-3ee9-261a-b473-1be341bdfb0c@redhat.com
https://edk2.groups.io/g/devel/message/55715
The part (or, well, "one part") that OVMF seems to miss is
Assumption/Recommendation #3 from "Secure SMM Communication", so this
series supplies that.
This series overlaps with TianoCore#386 (although the series is
motivated differently from the BZ). I used TianoCore#386 as the BZ
reference in the patches, for simplicity.
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
Thanks
Laszlo
Laszlo Ersek (5):
OvmfPkg/QemuFlashFvbServicesRuntimeDxe: drop unused PCDs
OvmfPkg/QemuFlashFvbServices: factor out
SetPcdFlashNvStorageBaseAddresses
OvmfPkg: set fixed FlashNvStorage base addresses with -D SMM_REQUIRE
OvmfPkg: include FaultTolerantWritePei and VariablePei with -D
SMM_REQUIRE
OvmfPkg: improve SMM comms security with adaptive
MemoryTypeInformation
OvmfPkg/OvmfPkg.fdf.inc | 6 +
OvmfPkg/OvmfPkgIa32.dsc | 6 +
OvmfPkg/OvmfPkgIa32.fdf | 2 +
OvmfPkg/OvmfPkgIa32X64.dsc | 6 +
OvmfPkg/OvmfPkgIa32X64.fdf | 2 +
OvmfPkg/OvmfPkgX64.dsc | 6 +
OvmfPkg/OvmfPkgX64.fdf | 2 +
OvmfPkg/PlatformPei/MemTypeInfo.c | 147 ++++++++++++++++++++
OvmfPkg/PlatformPei/Platform.c | 23 +--
OvmfPkg/PlatformPei/Platform.h | 5 +
OvmfPkg/PlatformPei/PlatformPei.inf | 2 +
OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf | 2 -
OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesSmm.inf | 7 -
OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c | 19 +--
OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.h | 5 +
OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c | 27 ++++
OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceSmm.c | 10 ++
17 files changed, 228 insertions(+), 49 deletions(-)
create mode 100644 OvmfPkg/PlatformPei/MemTypeInfo.c
--
2.19.1.3.g30247aa5d201
^ permalink raw reply [flat|nested] 18+ messages in thread* [PATCH 1/5] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: drop unused PCDs 2020-03-10 22:27 [PATCH 0/5] OvmfPkg: improve SMM comms security with adaptive MemoryTypeInformation Laszlo Ersek @ 2020-03-10 22:27 ` Laszlo Ersek 2020-03-10 22:27 ` [PATCH 2/5] OvmfPkg/QemuFlashFvbServices: factor out SetPcdFlashNvStorageBaseAddresses Laszlo Ersek ` (3 subsequent siblings) 4 siblings, 0 replies; 18+ messages in thread From: Laszlo Ersek @ 2020-03-10 22:27 UTC (permalink / raw) To: edk2-devel-groups-io Cc: Ard Biesheuvel, Jordan Justen, Philippe Mathieu-Daudé The only two OvmfPkg references to "PcdFlashNvStorageVariableBase" are the spurious ones in the runtime DXE driver and the SMM driver INF files of the QEMU flash driver. Remove these references. The flash driver does not access "PcdOvmfFlashNvStorageEventLogBase" either, so remove that from the INF files too. Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> Cc: Jordan Justen <jordan.l.justen@intel.com> Cc: Philippe Mathieu-Daudé <philmd@redhat.com> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=386 Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf | 2 -- OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesSmm.inf | 2 -- 2 files changed, 4 deletions(-) diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf index 8125fd0735a1..72cabba4357d 100644 --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf @@ -76,8 +76,6 @@ [FixedPcd] [Pcd] gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase - gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase - gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageEventLogBase gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64 gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashVariablesEnable diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesSmm.inf b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesSmm.inf index 4715d5fc437e..215daa2ba9fd 100644 --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesSmm.inf +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesSmm.inf @@ -76,8 +76,6 @@ [FixedPcd] [Pcd] gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase - gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase - gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageEventLogBase gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64 gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashVariablesEnable -- 2.19.1.3.g30247aa5d201 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 2/5] OvmfPkg/QemuFlashFvbServices: factor out SetPcdFlashNvStorageBaseAddresses 2020-03-10 22:27 [PATCH 0/5] OvmfPkg: improve SMM comms security with adaptive MemoryTypeInformation Laszlo Ersek 2020-03-10 22:27 ` [PATCH 1/5] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: drop unused PCDs Laszlo Ersek @ 2020-03-10 22:27 ` Laszlo Ersek 2020-03-10 22:27 ` [PATCH 3/5] OvmfPkg: set fixed FlashNvStorage base addresses with -D SMM_REQUIRE Laszlo Ersek ` (2 subsequent siblings) 4 siblings, 0 replies; 18+ messages in thread From: Laszlo Ersek @ 2020-03-10 22:27 UTC (permalink / raw) To: edk2-devel-groups-io Cc: Ard Biesheuvel, Jordan Justen, Philippe Mathieu-Daudé Extract the dynamic setting of the - PcdFlashNvStorageVariableBase64 - PcdFlashNvStorageFtwWorkingBase - PcdFlashNvStorageFtwSpareBase addresses to a helper function. For now, the helper function is identical (duplicated) between the SMM flash driver and the runtime DXE flash driver. In subsequent patches, this will change. Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> Cc: Jordan Justen <jordan.l.justen@intel.com> Cc: Philippe Mathieu-Daudé <philmd@redhat.com> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=386 Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.h | 5 ++++ OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c | 19 +------------- OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c | 27 ++++++++++++++++++++ OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceSmm.c | 27 ++++++++++++++++++++ 4 files changed, 60 insertions(+), 18 deletions(-) diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.h b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.h index a12577182d66..d064aee6ef55 100644 --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.h +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.h @@ -190,4 +190,9 @@ MarkIoMemoryRangeForRuntimeAccess ( IN UINTN Length ); +VOID +SetPcdFlashNvStorageBaseAddresses ( + VOID + ); + #endif diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c index b7b99129a80e..76a7853de04d 100644 --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c @@ -1051,24 +1051,7 @@ FvbInitialize ( MarkIoMemoryRangeForRuntimeAccess (BaseAddress, Length); - // - // Set several PCD values to point to flash - // - PcdStatus = PcdSet64S ( - PcdFlashNvStorageVariableBase64, - (UINTN) PcdGet32 (PcdOvmfFlashNvStorageVariableBase) - ); - ASSERT_RETURN_ERROR (PcdStatus); - PcdStatus = PcdSet32S ( - PcdFlashNvStorageFtwWorkingBase, - PcdGet32 (PcdOvmfFlashNvStorageFtwWorkingBase) - ); - ASSERT_RETURN_ERROR (PcdStatus); - PcdStatus = PcdSet32S ( - PcdFlashNvStorageFtwSpareBase, - PcdGet32 (PcdOvmfFlashNvStorageFtwSpareBase) - ); - ASSERT_RETURN_ERROR (PcdStatus); + SetPcdFlashNvStorageBaseAddresses (); FwhInstance = (EFI_FW_VOL_INSTANCE *) ( diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c index 69b20916bc7c..e60978fa127b 100644 --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c @@ -216,3 +216,30 @@ MarkIoMemoryRangeForRuntimeAccess ( return Status; } + +VOID +SetPcdFlashNvStorageBaseAddresses ( + VOID + ) +{ + RETURN_STATUS PcdStatus; + + // + // Set several PCD values to point to flash + // + PcdStatus = PcdSet64S ( + PcdFlashNvStorageVariableBase64, + (UINTN) PcdGet32 (PcdOvmfFlashNvStorageVariableBase) + ); + ASSERT_RETURN_ERROR (PcdStatus); + PcdStatus = PcdSet32S ( + PcdFlashNvStorageFtwWorkingBase, + PcdGet32 (PcdOvmfFlashNvStorageFtwWorkingBase) + ); + ASSERT_RETURN_ERROR (PcdStatus); + PcdStatus = PcdSet32S ( + PcdFlashNvStorageFtwSpareBase, + PcdGet32 (PcdOvmfFlashNvStorageFtwSpareBase) + ); + ASSERT_RETURN_ERROR (PcdStatus); +} diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceSmm.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceSmm.c index 1b74fc17b1ee..33bc3e1137be 100644 --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceSmm.c +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceSmm.c @@ -74,3 +74,30 @@ MarkIoMemoryRangeForRuntimeAccess ( return EFI_SUCCESS; } + +VOID +SetPcdFlashNvStorageBaseAddresses ( + VOID + ) +{ + RETURN_STATUS PcdStatus; + + // + // Set several PCD values to point to flash + // + PcdStatus = PcdSet64S ( + PcdFlashNvStorageVariableBase64, + (UINTN) PcdGet32 (PcdOvmfFlashNvStorageVariableBase) + ); + ASSERT_RETURN_ERROR (PcdStatus); + PcdStatus = PcdSet32S ( + PcdFlashNvStorageFtwWorkingBase, + PcdGet32 (PcdOvmfFlashNvStorageFtwWorkingBase) + ); + ASSERT_RETURN_ERROR (PcdStatus); + PcdStatus = PcdSet32S ( + PcdFlashNvStorageFtwSpareBase, + PcdGet32 (PcdOvmfFlashNvStorageFtwSpareBase) + ); + ASSERT_RETURN_ERROR (PcdStatus); +} -- 2.19.1.3.g30247aa5d201 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 3/5] OvmfPkg: set fixed FlashNvStorage base addresses with -D SMM_REQUIRE 2020-03-10 22:27 [PATCH 0/5] OvmfPkg: improve SMM comms security with adaptive MemoryTypeInformation Laszlo Ersek 2020-03-10 22:27 ` [PATCH 1/5] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: drop unused PCDs Laszlo Ersek 2020-03-10 22:27 ` [PATCH 2/5] OvmfPkg/QemuFlashFvbServices: factor out SetPcdFlashNvStorageBaseAddresses Laszlo Ersek @ 2020-03-10 22:27 ` Laszlo Ersek 2020-03-11 15:44 ` [edk2-devel] " Leif Lindholm 2020-03-10 22:27 ` [PATCH 4/5] OvmfPkg: include FaultTolerantWritePei and VariablePei " Laszlo Ersek 2020-03-10 22:27 ` [PATCH 5/5] OvmfPkg: improve SMM comms security with adaptive MemoryTypeInformation Laszlo Ersek 4 siblings, 1 reply; 18+ messages in thread From: Laszlo Ersek @ 2020-03-10 22:27 UTC (permalink / raw) To: edk2-devel-groups-io Cc: Ard Biesheuvel, Jordan Justen, Philippe Mathieu-Daudé The following flash-related base addresses: - PcdFlashNvStorageVariableBase64, - PcdFlashNvStorageFtwWorkingBase, - PcdFlashNvStorageFtwSpareBase, are always set to constant (invariable) values in the "-D SMM_REQUIRE" build of OVMF. (That's because in the SMM build, actual pflash is a hard requirement, and the RAM-based emulation is never available.) Set said PCDs statically, at build. This will allow us to depend on their values in the PEI phase. When SMM_REQUIRE is FALSE, this change has no effect (confirmed by report file comparison). When SMM_REQUIRE is TRUE, the report file shows the following changes: - "PcdOvmfFlashNvStorageFtwSpareBase" and "PcdOvmfFlashNvStorageFtwWorkingBase" are no longer consumed by any module directly, - for "PcdFlashNvStorageFtwSpareBase", "PcdFlashNvStorageFtwWorkingBase" and "PcdFlashNvStorageVariableBase64", the access method changes from DYN to FIXED, - for the latter PCDs, the zero (dynamic default) values are replaced with the desired constants. Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> Cc: Jordan Justen <jordan.l.justen@intel.com> Cc: Philippe Mathieu-Daudé <philmd@redhat.com> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=386 Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- OvmfPkg/OvmfPkgIa32.dsc | 2 ++ OvmfPkg/OvmfPkgIa32X64.dsc | 2 ++ OvmfPkg/OvmfPkgX64.dsc | 2 ++ OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesSmm.inf | 5 ----- OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceSmm.c | 19 +------------------ OvmfPkg/OvmfPkg.fdf.inc | 6 ++++++ 6 files changed, 13 insertions(+), 23 deletions(-) diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc index 8916255df4df..9bc47b01bb6a 100644 --- a/OvmfPkg/OvmfPkgIa32.dsc +++ b/OvmfPkg/OvmfPkgIa32.dsc @@ -541,9 +541,11 @@ [PcdsDynamicDefault] # ($(SMM_REQUIRE) == FALSE) gEfiMdeModulePkgTokenSpaceGuid.PcdEmuVariableNvStoreReserved|0 +!if $(SMM_REQUIRE) == FALSE gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64|0 gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase|0 gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase|0 +!endif gEfiMdeModulePkgTokenSpaceGuid.PcdPciDisableBusEnumeration|FALSE gEfiMdeModulePkgTokenSpaceGuid.PcdVideoHorizontalResolution|800 gEfiMdeModulePkgTokenSpaceGuid.PcdVideoVerticalResolution|600 diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc index 342ff96cc279..234d5426f066 100644 --- a/OvmfPkg/OvmfPkgIa32X64.dsc +++ b/OvmfPkg/OvmfPkgIa32X64.dsc @@ -546,9 +546,11 @@ [PcdsDynamicDefault] # ($(SMM_REQUIRE) == FALSE) gEfiMdeModulePkgTokenSpaceGuid.PcdEmuVariableNvStoreReserved|0 +!if $(SMM_REQUIRE) == FALSE gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64|0 gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase|0 gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase|0 +!endif gEfiMdeModulePkgTokenSpaceGuid.PcdPciDisableBusEnumeration|FALSE gEfiMdeModulePkgTokenSpaceGuid.PcdVideoHorizontalResolution|800 gEfiMdeModulePkgTokenSpaceGuid.PcdVideoVerticalResolution|600 diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc index 1fb2de5e0121..4d8a39a12ee1 100644 --- a/OvmfPkg/OvmfPkgX64.dsc +++ b/OvmfPkg/OvmfPkgX64.dsc @@ -545,9 +545,11 @@ [PcdsDynamicDefault] # ($(SMM_REQUIRE) == FALSE) gEfiMdeModulePkgTokenSpaceGuid.PcdEmuVariableNvStoreReserved|0 +!if $(SMM_REQUIRE) == FALSE gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64|0 gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase|0 gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase|0 +!endif gEfiMdeModulePkgTokenSpaceGuid.PcdPciDisableBusEnumeration|FALSE gEfiMdeModulePkgTokenSpaceGuid.PcdVideoHorizontalResolution|800 gEfiMdeModulePkgTokenSpaceGuid.PcdVideoVerticalResolution|600 diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesSmm.inf b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesSmm.inf index 215daa2ba9fd..41926914c8a3 100644 --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesSmm.inf +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesSmm.inf @@ -66,17 +66,12 @@ [FixedPcd] gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageVariableBase - gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageFtwWorkingBase - gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageFtwSpareBase gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageEventLogSize gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFdBaseAddress gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFirmwareFdSize gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFirmwareBlockSize [Pcd] - gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase - gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase - gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64 gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashVariablesEnable [FeaturePcd] diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceSmm.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceSmm.c index 33bc3e1137be..2717930f75e3 100644 --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceSmm.c +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceSmm.c @@ -80,24 +80,7 @@ SetPcdFlashNvStorageBaseAddresses ( VOID ) { - RETURN_STATUS PcdStatus; - // - // Set several PCD values to point to flash + // Do nothing. // - PcdStatus = PcdSet64S ( - PcdFlashNvStorageVariableBase64, - (UINTN) PcdGet32 (PcdOvmfFlashNvStorageVariableBase) - ); - ASSERT_RETURN_ERROR (PcdStatus); - PcdStatus = PcdSet32S ( - PcdFlashNvStorageFtwWorkingBase, - PcdGet32 (PcdOvmfFlashNvStorageFtwWorkingBase) - ); - ASSERT_RETURN_ERROR (PcdStatus); - PcdStatus = PcdSet32S ( - PcdFlashNvStorageFtwSpareBase, - PcdGet32 (PcdOvmfFlashNvStorageFtwSpareBase) - ); - ASSERT_RETURN_ERROR (PcdStatus); } diff --git a/OvmfPkg/OvmfPkg.fdf.inc b/OvmfPkg/OvmfPkg.fdf.inc index 66e0e4d270f5..35fd454b97ab 100644 --- a/OvmfPkg/OvmfPkg.fdf.inc +++ b/OvmfPkg/OvmfPkg.fdf.inc @@ -82,4 +82,10 @@ SET gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageFtwSpareBase = gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageFtwWorkingBase + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize SET gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize = $(VARS_SPARE_SIZE) +!if $(SMM_REQUIRE) == TRUE +SET gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64 = gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageVariableBase +SET gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase = gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageFtwWorkingBase +SET gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase = gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageFtwSpareBase +!endif + DEFINE MEMFD_BASE_ADDRESS = 0x800000 -- 2.19.1.3.g30247aa5d201 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [edk2-devel] [PATCH 3/5] OvmfPkg: set fixed FlashNvStorage base addresses with -D SMM_REQUIRE 2020-03-10 22:27 ` [PATCH 3/5] OvmfPkg: set fixed FlashNvStorage base addresses with -D SMM_REQUIRE Laszlo Ersek @ 2020-03-11 15:44 ` Leif Lindholm 2020-03-11 16:14 ` Laszlo Ersek 0 siblings, 1 reply; 18+ messages in thread From: Leif Lindholm @ 2020-03-11 15:44 UTC (permalink / raw) To: devel, lersek; +Cc: Ard Biesheuvel, Jordan Justen, Philippe Mathieu-Daudé One comment, not on this patch but prompted by it: On Tue, Mar 10, 2020 at 23:27:37 +0100, Laszlo Ersek wrote: > diff --git a/OvmfPkg/OvmfPkg.fdf.inc b/OvmfPkg/OvmfPkg.fdf.inc > index 66e0e4d270f5..35fd454b97ab 100644 > --- a/OvmfPkg/OvmfPkg.fdf.inc > +++ b/OvmfPkg/OvmfPkg.fdf.inc > @@ -82,4 +82,10 @@ I was surprised at not seeing the section header here, so had a look at the file, noticed it doesn't have any. And that all files that include it do it by: [Defines] !include OvmfPkg.fdf.inc That looks a bit error-prone and inflexible - could we move/copy the header into this file? / Leif > SET gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageFtwSpareBase = gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageFtwWorkingBase + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize > SET gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize = $(VARS_SPARE_SIZE) > > +!if $(SMM_REQUIRE) == TRUE > +SET gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64 = gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageVariableBase > +SET gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase = gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageFtwWorkingBase > +SET gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase = gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageFtwSpareBase > +!endif > + > DEFINE MEMFD_BASE_ADDRESS = 0x800000 > -- > 2.19.1.3.g30247aa5d201 > > > > > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [edk2-devel] [PATCH 3/5] OvmfPkg: set fixed FlashNvStorage base addresses with -D SMM_REQUIRE 2020-03-11 15:44 ` [edk2-devel] " Leif Lindholm @ 2020-03-11 16:14 ` Laszlo Ersek 2020-03-11 16:20 ` Leif Lindholm 0 siblings, 1 reply; 18+ messages in thread From: Laszlo Ersek @ 2020-03-11 16:14 UTC (permalink / raw) To: Leif Lindholm, devel Cc: Ard Biesheuvel, Jordan Justen, Philippe Mathieu-Daudé On 03/11/20 16:44, Leif Lindholm wrote: > One comment, not on this patch but prompted by it: > > On Tue, Mar 10, 2020 at 23:27:37 +0100, Laszlo Ersek wrote: >> diff --git a/OvmfPkg/OvmfPkg.fdf.inc b/OvmfPkg/OvmfPkg.fdf.inc >> index 66e0e4d270f5..35fd454b97ab 100644 >> --- a/OvmfPkg/OvmfPkg.fdf.inc >> +++ b/OvmfPkg/OvmfPkg.fdf.inc >> @@ -82,4 +82,10 @@ > > I was surprised at not seeing the section header here, so had a look > at the file, noticed it doesn't have any. And that all files that > include it do it by: > > [Defines] > !include OvmfPkg.fdf.inc > > That looks a bit error-prone and inflexible - could we move/copy the > header into this file? No, please let us not -- I strive to keep all FDF and DSC include files under OvmfPkg header-free. It gives more flexibility to the includer. A recent example of this was my request for NetworkPkg to expose its include snippets header-less, for DSC files. Please see the "!include NetworkPkg/..." directives in the OVMF DSC files; those are also by design header-less: NetworkPkg/NetworkComponents.dsc.inc NetworkPkg/NetworkDefines.dsc.inc NetworkPkg/NetworkLibs.dsc.inc NetworkPkg/NetworkPcds.dsc.inc NetworkPkg does offer (as a convenience) more "canned" includes too, but those were not flexible enough for OVMF. Same for the other FDF include files under OvmfPkg: - DecomprScratchEnd.fdf.inc - VarStore.fdf.inc An example of where this is actively being put to use is "VarStore.fdf.inc": it is included both under [FD.OVMF], and [FD.OVMF_VARS]. So the treatment of the include files is consistent (I'd not want some includes with headers, and some without). I realize the include files under ArmVirtPkg do not follow this pattern (they contain headers); however, out of the files: - ArmVirt.dsc.inc - ArmVirtQemuFvMain.fdf.inc - ArmVirtRules.fdf.inc - VarStore.fdf.inc the first and the third contain *multiple* headers, so the application area is arguably different. Thanks, Laszlo > > / > Leif > >> SET gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageFtwSpareBase = gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageFtwWorkingBase + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize >> SET gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize = $(VARS_SPARE_SIZE) >> >> +!if $(SMM_REQUIRE) == TRUE >> +SET gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64 = gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageVariableBase >> +SET gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase = gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageFtwWorkingBase >> +SET gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase = gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageFtwSpareBase >> +!endif >> + >> DEFINE MEMFD_BASE_ADDRESS = 0x800000 >> -- >> 2.19.1.3.g30247aa5d201 >> >> >> >> >> > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [edk2-devel] [PATCH 3/5] OvmfPkg: set fixed FlashNvStorage base addresses with -D SMM_REQUIRE 2020-03-11 16:14 ` Laszlo Ersek @ 2020-03-11 16:20 ` Leif Lindholm 2020-03-11 16:41 ` Laszlo Ersek 0 siblings, 1 reply; 18+ messages in thread From: Leif Lindholm @ 2020-03-11 16:20 UTC (permalink / raw) To: devel, lersek; +Cc: Ard Biesheuvel, Jordan Justen, Philippe Mathieu-Daudé On Wed, Mar 11, 2020 at 17:14:53 +0100, Laszlo Ersek wrote: > On 03/11/20 16:44, Leif Lindholm wrote: > > One comment, not on this patch but prompted by it: > > > > On Tue, Mar 10, 2020 at 23:27:37 +0100, Laszlo Ersek wrote: > >> diff --git a/OvmfPkg/OvmfPkg.fdf.inc b/OvmfPkg/OvmfPkg.fdf.inc > >> index 66e0e4d270f5..35fd454b97ab 100644 > >> --- a/OvmfPkg/OvmfPkg.fdf.inc > >> +++ b/OvmfPkg/OvmfPkg.fdf.inc > >> @@ -82,4 +82,10 @@ > > > > I was surprised at not seeing the section header here, so had a look > > at the file, noticed it doesn't have any. And that all files that > > include it do it by: > > > > [Defines] > > !include OvmfPkg.fdf.inc > > > > That looks a bit error-prone and inflexible - could we move/copy the > > header into this file? > > No, please let us not -- I strive to keep all FDF and DSC include files > under OvmfPkg header-free. It gives more flexibility to the includer. I see your point. The generic name suggested to me that it might be *intended* to hold multiple sections, and currently just happened not to. However, to follow a rule of least surprise... > A recent example of this was my request for NetworkPkg to expose its > include snippets header-less, for DSC files. Please see the "!include > NetworkPkg/..." directives in the OVMF DSC files; those are also by > design header-less: > > NetworkPkg/NetworkComponents.dsc.inc > NetworkPkg/NetworkDefines.dsc.inc > NetworkPkg/NetworkLibs.dsc.inc > NetworkPkg/NetworkPcds.dsc.inc ...could OvmfPkg use a similar naming scheme to this? That would also remove the drawback of not having the section name as part of the hunk header, as you'd have it anyway immediately above as part of the file name? Regards, Leif > NetworkPkg does offer (as a convenience) more "canned" includes too, but > those were not flexible enough for OVMF. > > Same for the other FDF include files under OvmfPkg: > - DecomprScratchEnd.fdf.inc > - VarStore.fdf.inc > > An example of where this is actively being put to use is > "VarStore.fdf.inc": it is included both under [FD.OVMF], and [FD.OVMF_VARS]. > > So the treatment of the include files is consistent (I'd not want some > includes with headers, and some without). > > > I realize the include files under ArmVirtPkg do not follow this pattern > (they contain headers); however, out of the files: > > - ArmVirt.dsc.inc > - ArmVirtQemuFvMain.fdf.inc > - ArmVirtRules.fdf.inc > - VarStore.fdf.inc > > the first and the third contain *multiple* headers, so the application > area is arguably different. > > Thanks, > Laszlo > > > > > > / > > Leif > > > >> SET gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageFtwSpareBase = gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageFtwWorkingBase + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize > >> SET gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize = $(VARS_SPARE_SIZE) > >> > >> +!if $(SMM_REQUIRE) == TRUE > >> +SET gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64 = gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageVariableBase > >> +SET gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase = gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageFtwWorkingBase > >> +SET gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase = gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageFtwSpareBase > >> +!endif > >> + > >> DEFINE MEMFD_BASE_ADDRESS = 0x800000 > >> -- > >> 2.19.1.3.g30247aa5d201 > >> > >> > >> > >> > >> > > > > > > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [edk2-devel] [PATCH 3/5] OvmfPkg: set fixed FlashNvStorage base addresses with -D SMM_REQUIRE 2020-03-11 16:20 ` Leif Lindholm @ 2020-03-11 16:41 ` Laszlo Ersek 2020-03-12 14:20 ` Leif Lindholm 0 siblings, 1 reply; 18+ messages in thread From: Laszlo Ersek @ 2020-03-11 16:41 UTC (permalink / raw) To: Leif Lindholm, devel Cc: Ard Biesheuvel, Jordan Justen, Philippe Mathieu-Daudé On 03/11/20 17:20, Leif Lindholm wrote: > On Wed, Mar 11, 2020 at 17:14:53 +0100, Laszlo Ersek wrote: >> On 03/11/20 16:44, Leif Lindholm wrote: >>> One comment, not on this patch but prompted by it: >>> >>> On Tue, Mar 10, 2020 at 23:27:37 +0100, Laszlo Ersek wrote: >>>> diff --git a/OvmfPkg/OvmfPkg.fdf.inc b/OvmfPkg/OvmfPkg.fdf.inc >>>> index 66e0e4d270f5..35fd454b97ab 100644 >>>> --- a/OvmfPkg/OvmfPkg.fdf.inc >>>> +++ b/OvmfPkg/OvmfPkg.fdf.inc >>>> @@ -82,4 +82,10 @@ >>> >>> I was surprised at not seeing the section header here, so had a look >>> at the file, noticed it doesn't have any. And that all files that >>> include it do it by: >>> >>> [Defines] >>> !include OvmfPkg.fdf.inc >>> >>> That looks a bit error-prone and inflexible - could we move/copy the >>> header into this file? >> >> No, please let us not -- I strive to keep all FDF and DSC include >> files under OvmfPkg header-free. It gives more flexibility to the >> includer. > > I see your point. The generic name suggested to me that it might be > *intended* to hold multiple sections, and currently just happened not > to. > > However, to follow a rule of least surprise... > >> A recent example of this was my request for NetworkPkg to expose its >> include snippets header-less, for DSC files. Please see the "!include >> NetworkPkg/..." directives in the OVMF DSC files; those are also by >> design header-less: >> >> NetworkPkg/NetworkComponents.dsc.inc >> NetworkPkg/NetworkDefines.dsc.inc >> NetworkPkg/NetworkLibs.dsc.inc >> NetworkPkg/NetworkPcds.dsc.inc > > ...could OvmfPkg use a similar naming scheme to this? > > That would also remove the drawback of not having the section name as > part of the hunk header, as you'd have it anyway immediately above as > part of the file name? There are three FDF include files: (1) DecomprScratchEnd.fdf.inc (included under [FV.FVMAIN_COMPACT], per commit 9beac0d847bf9): ## @file # This FDF include file computes the end of the scratch buffer used in # DecompressMemFvs() [OvmfPkg/Sec/SecMain.c]. It is based on the decompressed # (ie. original) size of the LZMA-compressed section of the one FFS file in # the FVMAIN_COMPACT firmware volume. # # Copyright (C) 2015, Red Hat, Inc. # # SPDX-License-Identifier: BSD-2-Clause-Patent ## This include file contains DEFINE and SET statements (for setting macros and PCDs, accordingly). I don't think either a "Defines" or "Pcds" suffix would apply. (2) OvmfPkg.fdf.inc (included under [Defines]): ## @file # FDF include file that defines the main macros and sets the dependent PCDs. # # Copyright (C) 2014, Red Hat, Inc. # Copyright (c) 2006 - 2013, Intel Corporation. All rights reserved.<BR> # # SPDX-License-Identifier: BSD-2-Clause-Patent # ## Same situation (both DEFINE and SET statements). (3) VarStore.fdf.inc (included under [FD.OVMF] and [FD.OVMF_VARS]): ## @file # FDF include file with Layout Regions that define an empty variable store. # # Copyright (C) 2014, Red Hat, Inc. # Copyright (c) 2006 - 2013, Intel Corporation. All rights reserved.<BR> # # SPDX-License-Identifier: BSD-2-Clause-Patent # ## I guess we could rename this to "VarStoreLayoutRegions.fdf.inc" -- but would that really help? I'm not against introducing helpful name suffixes, I just can't find anything that really fits and significantly improves upon the current names. I vaguely remember racking my brain when I was introducing these files, for better names, and this is what I had come up with. :) Thanks, Laszlo ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [edk2-devel] [PATCH 3/5] OvmfPkg: set fixed FlashNvStorage base addresses with -D SMM_REQUIRE 2020-03-11 16:41 ` Laszlo Ersek @ 2020-03-12 14:20 ` Leif Lindholm 2020-03-12 22:19 ` Laszlo Ersek 0 siblings, 1 reply; 18+ messages in thread From: Leif Lindholm @ 2020-03-12 14:20 UTC (permalink / raw) To: Laszlo Ersek Cc: devel, Ard Biesheuvel, Jordan Justen, Philippe Mathieu-Daudé On Wed, Mar 11, 2020 at 17:41:04 +0100, Laszlo Ersek wrote: > >> A recent example of this was my request for NetworkPkg to expose its > >> include snippets header-less, for DSC files. Please see the "!include > >> NetworkPkg/..." directives in the OVMF DSC files; those are also by > >> design header-less: > >> > >> NetworkPkg/NetworkComponents.dsc.inc > >> NetworkPkg/NetworkDefines.dsc.inc > >> NetworkPkg/NetworkLibs.dsc.inc > >> NetworkPkg/NetworkPcds.dsc.inc > > > > ...could OvmfPkg use a similar naming scheme to this? > > > > That would also remove the drawback of not having the section name as > > part of the hunk header, as you'd have it anyway immediately above as > > part of the file name? > > There are three FDF include files: > > (1) DecomprScratchEnd.fdf.inc (included under [FV.FVMAIN_COMPACT], per > commit 9beac0d847bf9): > > ## @file > # This FDF include file computes the end of the scratch buffer used in > # DecompressMemFvs() [OvmfPkg/Sec/SecMain.c]. It is based on the decompressed > # (ie. original) size of the LZMA-compressed section of the one FFS file in > # the FVMAIN_COMPACT firmware volume. > # > # Copyright (C) 2015, Red Hat, Inc. > # > # SPDX-License-Identifier: BSD-2-Clause-Patent > ## > > This include file contains DEFINE and SET statements (for setting macros > and PCDs, accordingly). I don't think either a "Defines" or "Pcds" > suffix would apply. Would FvmainCompactScratchEnd.fdf.inc be an option? > (2) OvmfPkg.fdf.inc (included under [Defines]): > > ## @file > # FDF include file that defines the main macros and sets the dependent PCDs. > # > # Copyright (C) 2014, Red Hat, Inc. > # Copyright (c) 2006 - 2013, Intel Corporation. All rights reserved.<BR> > # > # SPDX-License-Identifier: BSD-2-Clause-Patent > # > ## > > Same situation (both DEFINE and SET statements). However, it *needs* to be included inside the [Defines] section, so there is no need to get philosofical: It's OvmfPkgDefines.fdf.inc (or OvmfDefines.fdf.inc). This aligns 100% with the NetworkPkg example. > (3) VarStore.fdf.inc (included under [FD.OVMF] and [FD.OVMF_VARS]): > > ## @file > # FDF include file with Layout Regions that define an empty variable store. > # > # Copyright (C) 2014, Red Hat, Inc. > # Copyright (c) 2006 - 2013, Intel Corporation. All rights reserved.<BR> > # > # SPDX-License-Identifier: BSD-2-Clause-Patent > # > ## > > I guess we could rename this to "VarStoreLayoutRegions.fdf.inc" -- but > would that really help? OK, so this one is different because it can be included in more than one location. Also, it's not the sort of thing you need to touch if you're not already in the sixth circle of hell. > I'm not against introducing helpful name suffixes, I just can't find > anything that really fits and significantly improves upon the current > names. I vaguely remember racking my brain when I was introducing these > files, for better names, and this is what I had come up with. :) Sure, this all predates the NetworkPkg fragments (or really any fragments). It would just be nice to be able to start aligning this across packages. Splitting up the ARM fragment files is also totally on the table here. / Leif ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [edk2-devel] [PATCH 3/5] OvmfPkg: set fixed FlashNvStorage base addresses with -D SMM_REQUIRE 2020-03-12 14:20 ` Leif Lindholm @ 2020-03-12 22:19 ` Laszlo Ersek 0 siblings, 0 replies; 18+ messages in thread From: Laszlo Ersek @ 2020-03-12 22:19 UTC (permalink / raw) To: Leif Lindholm Cc: devel, Ard Biesheuvel, Jordan Justen, Philippe Mathieu-Daudé On 03/12/20 15:20, Leif Lindholm wrote: > On Wed, Mar 11, 2020 at 17:41:04 +0100, Laszlo Ersek wrote: >>>> A recent example of this was my request for NetworkPkg to expose its >>>> include snippets header-less, for DSC files. Please see the "!include >>>> NetworkPkg/..." directives in the OVMF DSC files; those are also by >>>> design header-less: >>>> >>>> NetworkPkg/NetworkComponents.dsc.inc >>>> NetworkPkg/NetworkDefines.dsc.inc >>>> NetworkPkg/NetworkLibs.dsc.inc >>>> NetworkPkg/NetworkPcds.dsc.inc >>> >>> ...could OvmfPkg use a similar naming scheme to this? >>> >>> That would also remove the drawback of not having the section name as >>> part of the hunk header, as you'd have it anyway immediately above as >>> part of the file name? >> >> There are three FDF include files: >> >> (1) DecomprScratchEnd.fdf.inc (included under [FV.FVMAIN_COMPACT], per >> commit 9beac0d847bf9): >> >> ## @file >> # This FDF include file computes the end of the scratch buffer used in >> # DecompressMemFvs() [OvmfPkg/Sec/SecMain.c]. It is based on the decompressed >> # (ie. original) size of the LZMA-compressed section of the one FFS file in >> # the FVMAIN_COMPACT firmware volume. >> # >> # Copyright (C) 2015, Red Hat, Inc. >> # >> # SPDX-License-Identifier: BSD-2-Clause-Patent >> ## >> >> This include file contains DEFINE and SET statements (for setting macros >> and PCDs, accordingly). I don't think either a "Defines" or "Pcds" >> suffix would apply. > > Would FvmainCompactScratchEnd.fdf.inc be an option? > >> (2) OvmfPkg.fdf.inc (included under [Defines]): >> >> ## @file >> # FDF include file that defines the main macros and sets the dependent PCDs. >> # >> # Copyright (C) 2014, Red Hat, Inc. >> # Copyright (c) 2006 - 2013, Intel Corporation. All rights reserved.<BR> >> # >> # SPDX-License-Identifier: BSD-2-Clause-Patent >> # >> ## >> >> Same situation (both DEFINE and SET statements). > > However, it *needs* to be included inside the [Defines] section, so > there is no need to get philosofical: > It's OvmfPkgDefines.fdf.inc (or OvmfDefines.fdf.inc). > This aligns 100% with the NetworkPkg example. > >> (3) VarStore.fdf.inc (included under [FD.OVMF] and [FD.OVMF_VARS]): >> >> ## @file >> # FDF include file with Layout Regions that define an empty variable store. >> # >> # Copyright (C) 2014, Red Hat, Inc. >> # Copyright (c) 2006 - 2013, Intel Corporation. All rights reserved.<BR> >> # >> # SPDX-License-Identifier: BSD-2-Clause-Patent >> # >> ## >> >> I guess we could rename this to "VarStoreLayoutRegions.fdf.inc" -- but >> would that really help? > > OK, so this one is different because it can be included in more than > one location. Also, it's not the sort of thing you need to touch if > you're not already in the sixth circle of hell. > >> I'm not against introducing helpful name suffixes, I just can't find >> anything that really fits and significantly improves upon the current >> names. I vaguely remember racking my brain when I was introducing these >> files, for better names, and this is what I had come up with. :) > > Sure, this all predates the NetworkPkg fragments (or really any > fragments). It would just be nice to be able to start aligning this > across packages. Splitting up the ARM fragment files is also totally > on the table here. > > / > Leif > I'll send a patch for (1) and (2). Thanks Laszlo ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 4/5] OvmfPkg: include FaultTolerantWritePei and VariablePei with -D SMM_REQUIRE 2020-03-10 22:27 [PATCH 0/5] OvmfPkg: improve SMM comms security with adaptive MemoryTypeInformation Laszlo Ersek ` (2 preceding siblings ...) 2020-03-10 22:27 ` [PATCH 3/5] OvmfPkg: set fixed FlashNvStorage base addresses with -D SMM_REQUIRE Laszlo Ersek @ 2020-03-10 22:27 ` Laszlo Ersek 2020-03-10 22:27 ` [PATCH 5/5] OvmfPkg: improve SMM comms security with adaptive MemoryTypeInformation Laszlo Ersek 4 siblings, 0 replies; 18+ messages in thread From: Laszlo Ersek @ 2020-03-10 22:27 UTC (permalink / raw) To: edk2-devel-groups-io Cc: Ard Biesheuvel, Jordan Justen, Philippe Mathieu-Daudé FaultTolerantWritePei consumes: - PcdFlashNvStorageFtwWorkingBase, - PcdFlashNvStorageFtwSpareBase. VariablePei consumes: - PcdFlashNvStorageVariableBase64. Due to the previous patches in this series, the above PCDs are available in the PEI phase, in the SMM_REQUIRE build. FaultTolerantWritePei produces a GUID-ed HOB with FAULT_TOLERANT_WRITE_LAST_WRITE_DATA as contents. It also installs a Null PPI that carries the same gEdkiiFaultTolerantWriteGuid as the HOB. VariablePei depends on the Null PPI mentioned above with a DEPEX, consumes the HOB (which is safe due to the DEPEX), and produces EFI_PEI_READ_ONLY_VARIABLE2_PPI. This enables read-only access to non-volatile UEFI variables in the PEI phase, in the SMM_REQUIRE build. For now, the DxeLoadCore() function in "MdeModulePkg/Core/DxeIplPeim/DxeLoad.c" will not access the "MemoryTypeInformation" variable, because OVMF's PlatformPei always produces the MemoryTypeInformation HOB. (Note: when the boot mode is BOOT_ON_S3_RESUME, PlatformPei doesn't build the HOB, but that's in sync with DxeLoadCore() also not looking for either the HOB or the UEFI variable.) Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> Cc: Jordan Justen <jordan.l.justen@intel.com> Cc: Philippe Mathieu-Daudé <philmd@redhat.com> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=386 Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- OvmfPkg/OvmfPkgIa32.dsc | 2 ++ OvmfPkg/OvmfPkgIa32X64.dsc | 2 ++ OvmfPkg/OvmfPkgX64.dsc | 2 ++ OvmfPkg/OvmfPkgIa32.fdf | 2 ++ OvmfPkg/OvmfPkgIa32X64.fdf | 2 ++ OvmfPkg/OvmfPkgX64.fdf | 2 ++ 6 files changed, 12 insertions(+) diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc index 9bc47b01bb6a..4c9ff419c462 100644 --- a/OvmfPkg/OvmfPkgIa32.dsc +++ b/OvmfPkg/OvmfPkgIa32.dsc @@ -638,6 +638,8 @@ [Components] !endif } !if $(SMM_REQUIRE) == TRUE + MdeModulePkg/Universal/FaultTolerantWritePei/FaultTolerantWritePei.inf + MdeModulePkg/Universal/Variable/Pei/VariablePei.inf OvmfPkg/SmmAccess/SmmAccessPei.inf !endif UefiCpuPkg/CpuMpPei/CpuMpPei.inf diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc index 234d5426f066..56681e9e4580 100644 --- a/OvmfPkg/OvmfPkgIa32X64.dsc +++ b/OvmfPkg/OvmfPkgIa32X64.dsc @@ -649,6 +649,8 @@ [Components.IA32] !endif } !if $(SMM_REQUIRE) == TRUE + MdeModulePkg/Universal/FaultTolerantWritePei/FaultTolerantWritePei.inf + MdeModulePkg/Universal/Variable/Pei/VariablePei.inf OvmfPkg/SmmAccess/SmmAccessPei.inf !endif UefiCpuPkg/CpuMpPei/CpuMpPei.inf diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc index 4d8a39a12ee1..0e74b6f4d26c 100644 --- a/OvmfPkg/OvmfPkgX64.dsc +++ b/OvmfPkg/OvmfPkgX64.dsc @@ -648,6 +648,8 @@ [Components] !endif } !if $(SMM_REQUIRE) == TRUE + MdeModulePkg/Universal/FaultTolerantWritePei/FaultTolerantWritePei.inf + MdeModulePkg/Universal/Variable/Pei/VariablePei.inf OvmfPkg/SmmAccess/SmmAccessPei.inf !endif UefiCpuPkg/CpuMpPei/CpuMpPei.inf diff --git a/OvmfPkg/OvmfPkgIa32.fdf b/OvmfPkg/OvmfPkgIa32.fdf index 54417b2bed4d..f1a15de365fd 100644 --- a/OvmfPkg/OvmfPkgIa32.fdf +++ b/OvmfPkg/OvmfPkgIa32.fdf @@ -155,6 +155,8 @@ [FV.PEIFV] INF MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf INF UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf !if $(SMM_REQUIRE) == TRUE +INF MdeModulePkg/Universal/FaultTolerantWritePei/FaultTolerantWritePei.inf +INF MdeModulePkg/Universal/Variable/Pei/VariablePei.inf INF OvmfPkg/SmmAccess/SmmAccessPei.inf !endif INF UefiCpuPkg/CpuMpPei/CpuMpPei.inf diff --git a/OvmfPkg/OvmfPkgIa32X64.fdf b/OvmfPkg/OvmfPkgIa32X64.fdf index 889a5aceea4d..9e2eb78230e9 100644 --- a/OvmfPkg/OvmfPkgIa32X64.fdf +++ b/OvmfPkg/OvmfPkgIa32X64.fdf @@ -155,6 +155,8 @@ [FV.PEIFV] INF MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf INF UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf !if $(SMM_REQUIRE) == TRUE +INF MdeModulePkg/Universal/FaultTolerantWritePei/FaultTolerantWritePei.inf +INF MdeModulePkg/Universal/Variable/Pei/VariablePei.inf INF OvmfPkg/SmmAccess/SmmAccessPei.inf !endif INF UefiCpuPkg/CpuMpPei/CpuMpPei.inf diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf index 889a5aceea4d..9e2eb78230e9 100644 --- a/OvmfPkg/OvmfPkgX64.fdf +++ b/OvmfPkg/OvmfPkgX64.fdf @@ -155,6 +155,8 @@ [FV.PEIFV] INF MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf INF UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf !if $(SMM_REQUIRE) == TRUE +INF MdeModulePkg/Universal/FaultTolerantWritePei/FaultTolerantWritePei.inf +INF MdeModulePkg/Universal/Variable/Pei/VariablePei.inf INF OvmfPkg/SmmAccess/SmmAccessPei.inf !endif INF UefiCpuPkg/CpuMpPei/CpuMpPei.inf -- 2.19.1.3.g30247aa5d201 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 5/5] OvmfPkg: improve SMM comms security with adaptive MemoryTypeInformation 2020-03-10 22:27 [PATCH 0/5] OvmfPkg: improve SMM comms security with adaptive MemoryTypeInformation Laszlo Ersek ` (3 preceding siblings ...) 2020-03-10 22:27 ` [PATCH 4/5] OvmfPkg: include FaultTolerantWritePei and VariablePei " Laszlo Ersek @ 2020-03-10 22:27 ` Laszlo Ersek 2020-03-11 16:00 ` [edk2-devel] " Leif Lindholm 4 siblings, 1 reply; 18+ messages in thread From: Laszlo Ersek @ 2020-03-10 22:27 UTC (permalink / raw) To: edk2-devel-groups-io Cc: Ard Biesheuvel, Jordan Justen, Philippe Mathieu-Daudé * In the Intel whitepaper: --v-- A Tour Beyond BIOS -- Secure SMM Communication https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Security-White-Papers https://github.com/tianocore-docs/Docs/raw/master/White_Papers/A_Tour_Beyond_BIOS_Secure_SMM_Communication.pdf --^-- bullet#3 in section "Assumption and Recommendation", and bullet#4 in "Call for action", recommend enabling the (adaptive) Memory Type Information feature. * In the Intel whitepaper: --v-- A Tour Beyond BIOS -- Memory Map and Practices in UEFI BIOS https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-white-papers https://github.com/tianocore-docs/Docs/raw/master/White_Papers/A_Tour_Beyond_BIOS_Memory_Map_And_Practices_in_UEFI_BIOS_V2.pdf --^-- figure#6 describes the Memory Type Information feature in detail; namely as a feedback loop between the Platform PEIM, the DXE IPL PEIM, the DXE Core, and BDS. Implement the missing PlatformPei functionality in OvmfPkg, for fulfilling the Secure SMM Communication recommendation. In the longer term, OVMF should install the WSMT ACPI table, and this patch contributes to that. Notes: - the step in figure#6 where the UEFI variable is copied into the HOB is covered by the DXE IPL PEIM, in the DxeLoadCore() function, - "PcdResetOnMemoryTypeInformationChange" must be reverted to the DEC default TRUE value, because both whitepapers indicate that BDS needs to reset the system if the Memory Type Information changes. Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> Cc: Jordan Justen <jordan.l.justen@intel.com> Cc: Philippe Mathieu-Daudé <philmd@redhat.com> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=386 Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- OvmfPkg/OvmfPkgIa32.dsc | 2 + OvmfPkg/OvmfPkgIa32X64.dsc | 2 + OvmfPkg/OvmfPkgX64.dsc | 2 + OvmfPkg/PlatformPei/PlatformPei.inf | 2 + OvmfPkg/PlatformPei/Platform.h | 5 + OvmfPkg/PlatformPei/MemTypeInfo.c | 147 ++++++++++++++++++++ OvmfPkg/PlatformPei/Platform.c | 23 +-- 7 files changed, 161 insertions(+), 22 deletions(-) diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc index 4c9ff419c462..02ca17db8b2a 100644 --- a/OvmfPkg/OvmfPkgIa32.dsc +++ b/OvmfPkg/OvmfPkgIa32.dsc @@ -448,7 +448,9 @@ [PcdsFeatureFlag] [PcdsFixedAtBuild] gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeMemorySize|1 +!if $(SMM_REQUIRE) == FALSE gEfiMdeModulePkgTokenSpaceGuid.PcdResetOnMemoryTypeInformationChange|FALSE +!endif gEfiMdePkgTokenSpaceGuid.PcdMaximumGuidedExtractHandler|0x10 !if ($(FD_SIZE_IN_KB) == 1024) || ($(FD_SIZE_IN_KB) == 2048) gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000 diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc index 56681e9e4580..d08cf558c6aa 100644 --- a/OvmfPkg/OvmfPkgIa32X64.dsc +++ b/OvmfPkg/OvmfPkgIa32X64.dsc @@ -452,7 +452,9 @@ [PcdsFeatureFlag] [PcdsFixedAtBuild] gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeMemorySize|1 +!if $(SMM_REQUIRE) == FALSE gEfiMdeModulePkgTokenSpaceGuid.PcdResetOnMemoryTypeInformationChange|FALSE +!endif gEfiMdePkgTokenSpaceGuid.PcdMaximumGuidedExtractHandler|0x10 !if ($(FD_SIZE_IN_KB) == 1024) || ($(FD_SIZE_IN_KB) == 2048) gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000 diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc index 0e74b6f4d26c..b2dccc40a865 100644 --- a/OvmfPkg/OvmfPkgX64.dsc +++ b/OvmfPkg/OvmfPkgX64.dsc @@ -452,7 +452,9 @@ [PcdsFeatureFlag] [PcdsFixedAtBuild] gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeMemorySize|1 +!if $(SMM_REQUIRE) == FALSE gEfiMdeModulePkgTokenSpaceGuid.PcdResetOnMemoryTypeInformationChange|FALSE +!endif gEfiMdePkgTokenSpaceGuid.PcdMaximumGuidedExtractHandler|0x10 !if ($(FD_SIZE_IN_KB) == 1024) || ($(FD_SIZE_IN_KB) == 2048) gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000 diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf b/OvmfPkg/PlatformPei/PlatformPei.inf index c51a6176aa2e..8531c63995c1 100644 --- a/OvmfPkg/PlatformPei/PlatformPei.inf +++ b/OvmfPkg/PlatformPei/PlatformPei.inf @@ -30,6 +30,7 @@ [Sources] FeatureControl.c Fv.c MemDetect.c + MemTypeInfo.c Platform.c Platform.h Xen.c @@ -112,6 +113,7 @@ [FeaturePcd] [Ppis] gEfiPeiMasterBootModePpiGuid gEfiPeiMpServicesPpiGuid + gEfiPeiReadOnlyVariable2PpiGuid [Depex] TRUE diff --git a/OvmfPkg/PlatformPei/Platform.h b/OvmfPkg/PlatformPei/Platform.h index 43f20f067f22..0484ec9e6b4c 100644 --- a/OvmfPkg/PlatformPei/Platform.h +++ b/OvmfPkg/PlatformPei/Platform.h @@ -82,6 +82,11 @@ PeiFvInitialization ( VOID ); +VOID +MemTypeInfoInitialization ( + VOID + ); + VOID InstallFeatureControlCallback ( VOID diff --git a/OvmfPkg/PlatformPei/MemTypeInfo.c b/OvmfPkg/PlatformPei/MemTypeInfo.c new file mode 100644 index 000000000000..c709236a457a --- /dev/null +++ b/OvmfPkg/PlatformPei/MemTypeInfo.c @@ -0,0 +1,147 @@ +/** @file + Produce a default memory type information HOB unless we can determine, from + the existence of the "MemoryTypeInformation" variable, that the DXE IPL PEIM + will produce the HOB. + + Copyright (C) 2017-2020, Red Hat, Inc. + + SPDX-License-Identifier: BSD-2-Clause-Patent +**/ + +#include <Guid/MemoryTypeInformation.h> +#include <Library/BaseLib.h> +#include <Library/DebugLib.h> +#include <Library/HobLib.h> +#include <Library/PcdLib.h> +#include <Library/PeiServicesLib.h> +#include <Ppi/ReadOnlyVariable2.h> +#include <Uefi/UefiMultiPhase.h> + +#include "Platform.h" + +STATIC EFI_MEMORY_TYPE_INFORMATION mDefaultMemoryTypeInformation[] = { + { EfiACPIMemoryNVS, 0x004 }, + { EfiACPIReclaimMemory, 0x008 }, + { EfiReservedMemoryType, 0x004 }, + { EfiRuntimeServicesData, 0x024 }, + { EfiRuntimeServicesCode, 0x030 }, + { EfiBootServicesCode, 0x180 }, + { EfiBootServicesData, 0xF00 }, + { EfiMaxMemoryType, 0x000 } +}; + +STATIC +VOID +BuildMemTypeInfoHob ( + VOID + ) +{ + BuildGuidDataHob ( + &gEfiMemoryTypeInformationGuid, + mDefaultMemoryTypeInformation, + sizeof mDefaultMemoryTypeInformation + ); + DEBUG ((DEBUG_INFO, "%a: default memory type information HOB built\n", + __FUNCTION__)); +} + +/** + Notification function called when EFI_PEI_READ_ONLY_VARIABLE2_PPI becomes + available. + + @param[in] PeiServices Indirect reference to the PEI Services Table. + @param[in] NotifyDescriptor Address of the notification descriptor data + structure. + @param[in] Ppi Address of the PPI that was installed. + + @return Status of the notification. The status code returned from this + function is ignored. +**/ +STATIC +EFI_STATUS +EFIAPI +OnReadOnlyVariable2Available ( + IN EFI_PEI_SERVICES **PeiServices, + IN EFI_PEI_NOTIFY_DESCRIPTOR *NotifyDescriptor, + IN VOID *Ppi + ) +{ + EFI_PEI_READ_ONLY_VARIABLE2_PPI *ReadOnlyVariable2; + UINTN DataSize; + EFI_STATUS Status; + + DEBUG ((DEBUG_VERBOSE, "%a\n", __FUNCTION__)); + + // + // Check if the "MemoryTypeInformation" variable exists, in the + // gEfiMemoryTypeInformationGuid namespace. + // + ReadOnlyVariable2 = Ppi; + DataSize = 0; + Status = ReadOnlyVariable2->GetVariable ( + ReadOnlyVariable2, + EFI_MEMORY_TYPE_INFORMATION_VARIABLE_NAME, + &gEfiMemoryTypeInformationGuid, + NULL, + &DataSize, + NULL + ); + switch (Status) { + case EFI_BUFFER_TOO_SMALL: + // + // The variable exists; the DXE IPL PEIM will build the HOB from it. + // + break; + case EFI_NOT_FOUND: + // + // The variable does not exist; install the default memory type information + // HOB. + // + BuildMemTypeInfoHob (); + break; + default: + DEBUG ((DEBUG_ERROR, "%a: unexpected: GetVariable(): %r\n", __FUNCTION__, + Status)); + ASSERT (FALSE); + CpuDeadLoop (); + break; + } + + return EFI_SUCCESS; +} + +// +// Notification object for registering the callback, for when +// EFI_PEI_READ_ONLY_VARIABLE2_PPI becomes available. +// +STATIC CONST EFI_PEI_NOTIFY_DESCRIPTOR mReadOnlyVariable2Notify = { + (EFI_PEI_PPI_DESCRIPTOR_NOTIFY_DISPATCH | + EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST), // Flags + &gEfiPeiReadOnlyVariable2PpiGuid, // Guid + OnReadOnlyVariable2Available // Notify +}; + +VOID +MemTypeInfoInitialization ( + VOID + ) +{ + EFI_STATUS Status; + + if (!FeaturePcdGet (PcdSmmSmramRequire)) { + // + // EFI_PEI_READ_ONLY_VARIABLE2_PPI will never be available; install + // the default memory type information HOB right away. + // + BuildMemTypeInfoHob (); + return; + } + + Status = PeiServicesNotifyPpi (&mReadOnlyVariable2Notify); + if (EFI_ERROR (Status)) { + DEBUG ((DEBUG_ERROR, "%a: failed to set up R/O Variable 2 callback: %r\n", + __FUNCTION__, Status)); + ASSERT (FALSE); + CpuDeadLoop (); + } +} diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c index 473af102783a..587ca68fc210 100644 --- a/OvmfPkg/PlatformPei/Platform.c +++ b/OvmfPkg/PlatformPei/Platform.c @@ -28,7 +28,6 @@ #include <Library/QemuFwCfgLib.h> #include <Library/QemuFwCfgS3Lib.h> #include <Library/ResourcePublicationLib.h> -#include <Guid/MemoryTypeInformation.h> #include <Ppi/MasterBootMode.h> #include <IndustryStandard/I440FxPiix4.h> #include <IndustryStandard/Pci22.h> @@ -39,18 +38,6 @@ #include "Platform.h" #include "Cmos.h" -EFI_MEMORY_TYPE_INFORMATION mDefaultMemoryTypeInformation[] = { - { EfiACPIMemoryNVS, 0x004 }, - { EfiACPIReclaimMemory, 0x008 }, - { EfiReservedMemoryType, 0x004 }, - { EfiRuntimeServicesData, 0x024 }, - { EfiRuntimeServicesCode, 0x030 }, - { EfiBootServicesCode, 0x180 }, - { EfiBootServicesData, 0xF00 }, - { EfiMaxMemoryType, 0x000 } -}; - - EFI_PEI_PPI_DESCRIPTOR mPpiBootMode[] = { { EFI_PEI_PPI_DESCRIPTOR_PPI | EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST, @@ -162,15 +149,6 @@ MemMapInitialization ( PciIoBase = 0xC000; PciIoSize = 0x4000; - // - // Create Memory Type Information HOB - // - BuildGuidDataHob ( - &gEfiMemoryTypeInformationGuid, - mDefaultMemoryTypeInformation, - sizeof(mDefaultMemoryTypeInformation) - ); - // // Video memory + Legacy BIOS region // @@ -811,6 +789,7 @@ InitializePlatform ( ReserveEmuVariableNvStore (); } PeiFvInitialization (); + MemTypeInfoInitialization (); MemMapInitialization (); NoexecDxeInitialization (); } -- 2.19.1.3.g30247aa5d201 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [edk2-devel] [PATCH 5/5] OvmfPkg: improve SMM comms security with adaptive MemoryTypeInformation 2020-03-10 22:27 ` [PATCH 5/5] OvmfPkg: improve SMM comms security with adaptive MemoryTypeInformation Laszlo Ersek @ 2020-03-11 16:00 ` Leif Lindholm 2020-03-11 16:22 ` Laszlo Ersek 0 siblings, 1 reply; 18+ messages in thread From: Leif Lindholm @ 2020-03-11 16:00 UTC (permalink / raw) To: devel, lersek; +Cc: Ard Biesheuvel, Jordan Justen, Philippe Mathieu-Daudé On Tue, Mar 10, 2020 at 23:27:39 +0100, Laszlo Ersek wrote: > * In the Intel whitepaper: > > --v-- > A Tour Beyond BIOS -- Secure SMM Communication > > https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Security-White-Papers > https://github.com/tianocore-docs/Docs/raw/master/White_Papers/A_Tour_Beyond_BIOS_Secure_SMM_Communication.pdf > --^-- > > bullet#3 in section "Assumption and Recommendation", and bullet#4 in "Call > for action", recommend enabling the (adaptive) Memory Type Information > feature. > > * In the Intel whitepaper: > > --v-- > A Tour Beyond BIOS -- Memory Map and Practices in UEFI BIOS > > https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-white-papers > https://github.com/tianocore-docs/Docs/raw/master/White_Papers/A_Tour_Beyond_BIOS_Memory_Map_And_Practices_in_UEFI_BIOS_V2.pdf > --^-- > > figure#6 describes the Memory Type Information feature in detail; namely > as a feedback loop between the Platform PEIM, the DXE IPL PEIM, the DXE > Core, and BDS. > > Implement the missing PlatformPei functionality in OvmfPkg, for fulfilling > the Secure SMM Communication recommendation. > > In the longer term, OVMF should install the WSMT ACPI table, and this > patch contributes to that. > > Notes: > > - the step in figure#6 where the UEFI variable is copied into the HOB is > covered by the DXE IPL PEIM, in the DxeLoadCore() function, > > - "PcdResetOnMemoryTypeInformationChange" must be reverted to the DEC > default TRUE value, because both whitepapers indicate that BDS needs to > reset the system if the Memory Type Information changes. > > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> > Cc: Jordan Justen <jordan.l.justen@intel.com> > Cc: Philippe Mathieu-Daudé <philmd@redhat.com> > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=386 > Signed-off-by: Laszlo Ersek <lersek@redhat.com> > --- > OvmfPkg/OvmfPkgIa32.dsc | 2 + > OvmfPkg/OvmfPkgIa32X64.dsc | 2 + > OvmfPkg/OvmfPkgX64.dsc | 2 + > OvmfPkg/PlatformPei/PlatformPei.inf | 2 + > OvmfPkg/PlatformPei/Platform.h | 5 + > OvmfPkg/PlatformPei/MemTypeInfo.c | 147 ++++++++++++++++++++ > OvmfPkg/PlatformPei/Platform.c | 23 +-- > 7 files changed, 161 insertions(+), 22 deletions(-) > > diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc > index 4c9ff419c462..02ca17db8b2a 100644 > --- a/OvmfPkg/OvmfPkgIa32.dsc > +++ b/OvmfPkg/OvmfPkgIa32.dsc > @@ -448,7 +448,9 @@ [PcdsFeatureFlag] > > [PcdsFixedAtBuild] > gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeMemorySize|1 > +!if $(SMM_REQUIRE) == FALSE > gEfiMdeModulePkgTokenSpaceGuid.PcdResetOnMemoryTypeInformationChange|FALSE > +!endif > gEfiMdePkgTokenSpaceGuid.PcdMaximumGuidedExtractHandler|0x10 > !if ($(FD_SIZE_IN_KB) == 1024) || ($(FD_SIZE_IN_KB) == 2048) > gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000 > diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc > index 56681e9e4580..d08cf558c6aa 100644 > --- a/OvmfPkg/OvmfPkgIa32X64.dsc > +++ b/OvmfPkg/OvmfPkgIa32X64.dsc > @@ -452,7 +452,9 @@ [PcdsFeatureFlag] > > [PcdsFixedAtBuild] > gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeMemorySize|1 > +!if $(SMM_REQUIRE) == FALSE > gEfiMdeModulePkgTokenSpaceGuid.PcdResetOnMemoryTypeInformationChange|FALSE > +!endif > gEfiMdePkgTokenSpaceGuid.PcdMaximumGuidedExtractHandler|0x10 > !if ($(FD_SIZE_IN_KB) == 1024) || ($(FD_SIZE_IN_KB) == 2048) > gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000 > diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc > index 0e74b6f4d26c..b2dccc40a865 100644 > --- a/OvmfPkg/OvmfPkgX64.dsc > +++ b/OvmfPkg/OvmfPkgX64.dsc > @@ -452,7 +452,9 @@ [PcdsFeatureFlag] > > [PcdsFixedAtBuild] > gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeMemorySize|1 > +!if $(SMM_REQUIRE) == FALSE > gEfiMdeModulePkgTokenSpaceGuid.PcdResetOnMemoryTypeInformationChange|FALSE > +!endif > gEfiMdePkgTokenSpaceGuid.PcdMaximumGuidedExtractHandler|0x10 > !if ($(FD_SIZE_IN_KB) == 1024) || ($(FD_SIZE_IN_KB) == 2048) > gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000 > diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf b/OvmfPkg/PlatformPei/PlatformPei.inf > index c51a6176aa2e..8531c63995c1 100644 > --- a/OvmfPkg/PlatformPei/PlatformPei.inf > +++ b/OvmfPkg/PlatformPei/PlatformPei.inf > @@ -30,6 +30,7 @@ [Sources] > FeatureControl.c > Fv.c > MemDetect.c > + MemTypeInfo.c > Platform.c > Platform.h > Xen.c > @@ -112,6 +113,7 @@ [FeaturePcd] > [Ppis] > gEfiPeiMasterBootModePpiGuid > gEfiPeiMpServicesPpiGuid > + gEfiPeiReadOnlyVariable2PpiGuid > > [Depex] > TRUE > diff --git a/OvmfPkg/PlatformPei/Platform.h b/OvmfPkg/PlatformPei/Platform.h > index 43f20f067f22..0484ec9e6b4c 100644 > --- a/OvmfPkg/PlatformPei/Platform.h > +++ b/OvmfPkg/PlatformPei/Platform.h > @@ -82,6 +82,11 @@ PeiFvInitialization ( > VOID > ); > > +VOID > +MemTypeInfoInitialization ( > + VOID > + ); > + > VOID > InstallFeatureControlCallback ( > VOID > diff --git a/OvmfPkg/PlatformPei/MemTypeInfo.c b/OvmfPkg/PlatformPei/MemTypeInfo.c > new file mode 100644 > index 000000000000..c709236a457a > --- /dev/null > +++ b/OvmfPkg/PlatformPei/MemTypeInfo.c > @@ -0,0 +1,147 @@ > +/** @file > + Produce a default memory type information HOB unless we can determine, from > + the existence of the "MemoryTypeInformation" variable, that the DXE IPL PEIM > + will produce the HOB. > + > + Copyright (C) 2017-2020, Red Hat, Inc. > + > + SPDX-License-Identifier: BSD-2-Clause-Patent > +**/ > + > +#include <Guid/MemoryTypeInformation.h> > +#include <Library/BaseLib.h> > +#include <Library/DebugLib.h> > +#include <Library/HobLib.h> > +#include <Library/PcdLib.h> > +#include <Library/PeiServicesLib.h> > +#include <Ppi/ReadOnlyVariable2.h> > +#include <Uefi/UefiMultiPhase.h> > + > +#include "Platform.h" > + > +STATIC EFI_MEMORY_TYPE_INFORMATION mDefaultMemoryTypeInformation[] = { > + { EfiACPIMemoryNVS, 0x004 }, > + { EfiACPIReclaimMemory, 0x008 }, > + { EfiReservedMemoryType, 0x004 }, > + { EfiRuntimeServicesData, 0x024 }, > + { EfiRuntimeServicesCode, 0x030 }, > + { EfiBootServicesCode, 0x180 }, > + { EfiBootServicesData, 0xF00 }, > + { EfiMaxMemoryType, 0x000 } > +}; Could you add a comment as to where these page counts come from? Oh, right, they're just moved across from OvmfPkg/PlatformPei/Platform.c. It *would* be nice if that could be cleaned up at the same time... / Leif > + > +STATIC > +VOID > +BuildMemTypeInfoHob ( > + VOID > + ) > +{ > + BuildGuidDataHob ( > + &gEfiMemoryTypeInformationGuid, > + mDefaultMemoryTypeInformation, > + sizeof mDefaultMemoryTypeInformation > + ); > + DEBUG ((DEBUG_INFO, "%a: default memory type information HOB built\n", > + __FUNCTION__)); > +} > + > +/** > + Notification function called when EFI_PEI_READ_ONLY_VARIABLE2_PPI becomes > + available. > + > + @param[in] PeiServices Indirect reference to the PEI Services Table. > + @param[in] NotifyDescriptor Address of the notification descriptor data > + structure. > + @param[in] Ppi Address of the PPI that was installed. > + > + @return Status of the notification. The status code returned from this > + function is ignored. > +**/ > +STATIC > +EFI_STATUS > +EFIAPI > +OnReadOnlyVariable2Available ( > + IN EFI_PEI_SERVICES **PeiServices, > + IN EFI_PEI_NOTIFY_DESCRIPTOR *NotifyDescriptor, > + IN VOID *Ppi > + ) > +{ > + EFI_PEI_READ_ONLY_VARIABLE2_PPI *ReadOnlyVariable2; > + UINTN DataSize; > + EFI_STATUS Status; > + > + DEBUG ((DEBUG_VERBOSE, "%a\n", __FUNCTION__)); > + > + // > + // Check if the "MemoryTypeInformation" variable exists, in the > + // gEfiMemoryTypeInformationGuid namespace. > + // > + ReadOnlyVariable2 = Ppi; > + DataSize = 0; > + Status = ReadOnlyVariable2->GetVariable ( > + ReadOnlyVariable2, > + EFI_MEMORY_TYPE_INFORMATION_VARIABLE_NAME, > + &gEfiMemoryTypeInformationGuid, > + NULL, > + &DataSize, > + NULL > + ); > + switch (Status) { > + case EFI_BUFFER_TOO_SMALL: > + // > + // The variable exists; the DXE IPL PEIM will build the HOB from it. > + // > + break; > + case EFI_NOT_FOUND: > + // > + // The variable does not exist; install the default memory type information > + // HOB. > + // > + BuildMemTypeInfoHob (); > + break; > + default: > + DEBUG ((DEBUG_ERROR, "%a: unexpected: GetVariable(): %r\n", __FUNCTION__, > + Status)); > + ASSERT (FALSE); > + CpuDeadLoop (); > + break; > + } > + > + return EFI_SUCCESS; > +} > + > +// > +// Notification object for registering the callback, for when > +// EFI_PEI_READ_ONLY_VARIABLE2_PPI becomes available. > +// > +STATIC CONST EFI_PEI_NOTIFY_DESCRIPTOR mReadOnlyVariable2Notify = { > + (EFI_PEI_PPI_DESCRIPTOR_NOTIFY_DISPATCH | > + EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST), // Flags > + &gEfiPeiReadOnlyVariable2PpiGuid, // Guid > + OnReadOnlyVariable2Available // Notify > +}; > + > +VOID > +MemTypeInfoInitialization ( > + VOID > + ) > +{ > + EFI_STATUS Status; > + > + if (!FeaturePcdGet (PcdSmmSmramRequire)) { > + // > + // EFI_PEI_READ_ONLY_VARIABLE2_PPI will never be available; install > + // the default memory type information HOB right away. > + // > + BuildMemTypeInfoHob (); > + return; > + } > + > + Status = PeiServicesNotifyPpi (&mReadOnlyVariable2Notify); > + if (EFI_ERROR (Status)) { > + DEBUG ((DEBUG_ERROR, "%a: failed to set up R/O Variable 2 callback: %r\n", > + __FUNCTION__, Status)); > + ASSERT (FALSE); > + CpuDeadLoop (); > + } > +} > diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c > index 473af102783a..587ca68fc210 100644 > --- a/OvmfPkg/PlatformPei/Platform.c > +++ b/OvmfPkg/PlatformPei/Platform.c > @@ -28,7 +28,6 @@ > #include <Library/QemuFwCfgLib.h> > #include <Library/QemuFwCfgS3Lib.h> > #include <Library/ResourcePublicationLib.h> > -#include <Guid/MemoryTypeInformation.h> > #include <Ppi/MasterBootMode.h> > #include <IndustryStandard/I440FxPiix4.h> > #include <IndustryStandard/Pci22.h> > @@ -39,18 +38,6 @@ > #include "Platform.h" > #include "Cmos.h" > > -EFI_MEMORY_TYPE_INFORMATION mDefaultMemoryTypeInformation[] = { > - { EfiACPIMemoryNVS, 0x004 }, > - { EfiACPIReclaimMemory, 0x008 }, > - { EfiReservedMemoryType, 0x004 }, > - { EfiRuntimeServicesData, 0x024 }, > - { EfiRuntimeServicesCode, 0x030 }, > - { EfiBootServicesCode, 0x180 }, > - { EfiBootServicesData, 0xF00 }, > - { EfiMaxMemoryType, 0x000 } > -}; > - > - > EFI_PEI_PPI_DESCRIPTOR mPpiBootMode[] = { > { > EFI_PEI_PPI_DESCRIPTOR_PPI | EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST, > @@ -162,15 +149,6 @@ MemMapInitialization ( > PciIoBase = 0xC000; > PciIoSize = 0x4000; > > - // > - // Create Memory Type Information HOB > - // > - BuildGuidDataHob ( > - &gEfiMemoryTypeInformationGuid, > - mDefaultMemoryTypeInformation, > - sizeof(mDefaultMemoryTypeInformation) > - ); > - > // > // Video memory + Legacy BIOS region > // > @@ -811,6 +789,7 @@ InitializePlatform ( > ReserveEmuVariableNvStore (); > } > PeiFvInitialization (); > + MemTypeInfoInitialization (); > MemMapInitialization (); > NoexecDxeInitialization (); > } > -- > 2.19.1.3.g30247aa5d201 > > > > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [edk2-devel] [PATCH 5/5] OvmfPkg: improve SMM comms security with adaptive MemoryTypeInformation 2020-03-11 16:00 ` [edk2-devel] " Leif Lindholm @ 2020-03-11 16:22 ` Laszlo Ersek 2020-03-11 19:36 ` Leif Lindholm 0 siblings, 1 reply; 18+ messages in thread From: Laszlo Ersek @ 2020-03-11 16:22 UTC (permalink / raw) To: Leif Lindholm, devel Cc: Ard Biesheuvel, Jordan Justen, Philippe Mathieu-Daudé On 03/11/20 17:00, Leif Lindholm wrote: > On Tue, Mar 10, 2020 at 23:27:39 +0100, Laszlo Ersek wrote: >> * In the Intel whitepaper: >> >> --v-- >> A Tour Beyond BIOS -- Secure SMM Communication >> >> https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Security-White-Papers >> https://github.com/tianocore-docs/Docs/raw/master/White_Papers/A_Tour_Beyond_BIOS_Secure_SMM_Communication.pdf >> --^-- >> >> bullet#3 in section "Assumption and Recommendation", and bullet#4 in "Call >> for action", recommend enabling the (adaptive) Memory Type Information >> feature. >> >> * In the Intel whitepaper: >> >> --v-- >> A Tour Beyond BIOS -- Memory Map and Practices in UEFI BIOS >> >> https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-white-papers >> https://github.com/tianocore-docs/Docs/raw/master/White_Papers/A_Tour_Beyond_BIOS_Memory_Map_And_Practices_in_UEFI_BIOS_V2.pdf >> --^-- >> >> figure#6 describes the Memory Type Information feature in detail; namely >> as a feedback loop between the Platform PEIM, the DXE IPL PEIM, the DXE >> Core, and BDS. >> >> Implement the missing PlatformPei functionality in OvmfPkg, for fulfilling >> the Secure SMM Communication recommendation. >> >> In the longer term, OVMF should install the WSMT ACPI table, and this >> patch contributes to that. >> >> Notes: >> >> - the step in figure#6 where the UEFI variable is copied into the HOB is >> covered by the DXE IPL PEIM, in the DxeLoadCore() function, >> >> - "PcdResetOnMemoryTypeInformationChange" must be reverted to the DEC >> default TRUE value, because both whitepapers indicate that BDS needs to >> reset the system if the Memory Type Information changes. >> >> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> Cc: Jordan Justen <jordan.l.justen@intel.com> >> Cc: Philippe Mathieu-Daudé <philmd@redhat.com> >> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=386 >> Signed-off-by: Laszlo Ersek <lersek@redhat.com> >> --- >> OvmfPkg/OvmfPkgIa32.dsc | 2 + >> OvmfPkg/OvmfPkgIa32X64.dsc | 2 + >> OvmfPkg/OvmfPkgX64.dsc | 2 + >> OvmfPkg/PlatformPei/PlatformPei.inf | 2 + >> OvmfPkg/PlatformPei/Platform.h | 5 + >> OvmfPkg/PlatformPei/MemTypeInfo.c | 147 ++++++++++++++++++++ >> OvmfPkg/PlatformPei/Platform.c | 23 +-- >> 7 files changed, 161 insertions(+), 22 deletions(-) >> >> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc >> index 4c9ff419c462..02ca17db8b2a 100644 >> --- a/OvmfPkg/OvmfPkgIa32.dsc >> +++ b/OvmfPkg/OvmfPkgIa32.dsc >> @@ -448,7 +448,9 @@ [PcdsFeatureFlag] >> >> [PcdsFixedAtBuild] >> gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeMemorySize|1 >> +!if $(SMM_REQUIRE) == FALSE >> gEfiMdeModulePkgTokenSpaceGuid.PcdResetOnMemoryTypeInformationChange|FALSE >> +!endif >> gEfiMdePkgTokenSpaceGuid.PcdMaximumGuidedExtractHandler|0x10 >> !if ($(FD_SIZE_IN_KB) == 1024) || ($(FD_SIZE_IN_KB) == 2048) >> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000 >> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc >> index 56681e9e4580..d08cf558c6aa 100644 >> --- a/OvmfPkg/OvmfPkgIa32X64.dsc >> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc >> @@ -452,7 +452,9 @@ [PcdsFeatureFlag] >> >> [PcdsFixedAtBuild] >> gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeMemorySize|1 >> +!if $(SMM_REQUIRE) == FALSE >> gEfiMdeModulePkgTokenSpaceGuid.PcdResetOnMemoryTypeInformationChange|FALSE >> +!endif >> gEfiMdePkgTokenSpaceGuid.PcdMaximumGuidedExtractHandler|0x10 >> !if ($(FD_SIZE_IN_KB) == 1024) || ($(FD_SIZE_IN_KB) == 2048) >> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000 >> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc >> index 0e74b6f4d26c..b2dccc40a865 100644 >> --- a/OvmfPkg/OvmfPkgX64.dsc >> +++ b/OvmfPkg/OvmfPkgX64.dsc >> @@ -452,7 +452,9 @@ [PcdsFeatureFlag] >> >> [PcdsFixedAtBuild] >> gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeMemorySize|1 >> +!if $(SMM_REQUIRE) == FALSE >> gEfiMdeModulePkgTokenSpaceGuid.PcdResetOnMemoryTypeInformationChange|FALSE >> +!endif >> gEfiMdePkgTokenSpaceGuid.PcdMaximumGuidedExtractHandler|0x10 >> !if ($(FD_SIZE_IN_KB) == 1024) || ($(FD_SIZE_IN_KB) == 2048) >> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000 >> diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf b/OvmfPkg/PlatformPei/PlatformPei.inf >> index c51a6176aa2e..8531c63995c1 100644 >> --- a/OvmfPkg/PlatformPei/PlatformPei.inf >> +++ b/OvmfPkg/PlatformPei/PlatformPei.inf >> @@ -30,6 +30,7 @@ [Sources] >> FeatureControl.c >> Fv.c >> MemDetect.c >> + MemTypeInfo.c >> Platform.c >> Platform.h >> Xen.c >> @@ -112,6 +113,7 @@ [FeaturePcd] >> [Ppis] >> gEfiPeiMasterBootModePpiGuid >> gEfiPeiMpServicesPpiGuid >> + gEfiPeiReadOnlyVariable2PpiGuid >> >> [Depex] >> TRUE >> diff --git a/OvmfPkg/PlatformPei/Platform.h b/OvmfPkg/PlatformPei/Platform.h >> index 43f20f067f22..0484ec9e6b4c 100644 >> --- a/OvmfPkg/PlatformPei/Platform.h >> +++ b/OvmfPkg/PlatformPei/Platform.h >> @@ -82,6 +82,11 @@ PeiFvInitialization ( >> VOID >> ); >> >> +VOID >> +MemTypeInfoInitialization ( >> + VOID >> + ); >> + >> VOID >> InstallFeatureControlCallback ( >> VOID >> diff --git a/OvmfPkg/PlatformPei/MemTypeInfo.c b/OvmfPkg/PlatformPei/MemTypeInfo.c >> new file mode 100644 >> index 000000000000..c709236a457a >> --- /dev/null >> +++ b/OvmfPkg/PlatformPei/MemTypeInfo.c >> @@ -0,0 +1,147 @@ >> +/** @file >> + Produce a default memory type information HOB unless we can determine, from >> + the existence of the "MemoryTypeInformation" variable, that the DXE IPL PEIM >> + will produce the HOB. >> + >> + Copyright (C) 2017-2020, Red Hat, Inc. >> + >> + SPDX-License-Identifier: BSD-2-Clause-Patent >> +**/ >> + >> +#include <Guid/MemoryTypeInformation.h> >> +#include <Library/BaseLib.h> >> +#include <Library/DebugLib.h> >> +#include <Library/HobLib.h> >> +#include <Library/PcdLib.h> >> +#include <Library/PeiServicesLib.h> >> +#include <Ppi/ReadOnlyVariable2.h> >> +#include <Uefi/UefiMultiPhase.h> >> + >> +#include "Platform.h" >> + >> +STATIC EFI_MEMORY_TYPE_INFORMATION mDefaultMemoryTypeInformation[] = { >> + { EfiACPIMemoryNVS, 0x004 }, >> + { EfiACPIReclaimMemory, 0x008 }, >> + { EfiReservedMemoryType, 0x004 }, >> + { EfiRuntimeServicesData, 0x024 }, >> + { EfiRuntimeServicesCode, 0x030 }, >> + { EfiBootServicesCode, 0x180 }, >> + { EfiBootServicesData, 0xF00 }, >> + { EfiMaxMemoryType, 0x000 } >> +}; > > Could you add a comment as to where these page counts come from? > Oh, right, they're just moved across from OvmfPkg/PlatformPei/Platform.c. > > It *would* be nice if that could be cleaned up at the same time... Sorry, I don't understand -- what kind of cleanup do you have in mind precisely? The table is not copied, but moved from the original place, so I'm unsure what's left in "Platform.c" to clean up. Regarding the origin of the table, it's ancient: - 49ba9447c92d ("Add initial version of Open Virtual Machine Firmware (OVMF) platform.", 2009-05-27) - 991d95636264 ("Update OVMF Platform PEI to declare IO and MMIO resources. Update default memory type information to reduce EFI Memory Map fragmentation.", 2010-07-16) - 55cdb67acb3d ("OVMF: Support greater than 2GB of memory", 2010-10-13) (BTW I have a patch as a separate work item in the queue for this -- I'm going to remove the EfiBootServicesCode / EfiBootServicesData entries from this table, per Jiewen's recommendation / explanation in the "WSMT bits" thread: https://edk2.groups.io/g/devel/message/55712 http://mid.mail-archive.com/a5e71131-65dc-8b85-481a-d35011512987@redhat.com But that's a separate patch, for later.) Thank you! Laszlo > > / > Leif > >> + >> +STATIC >> +VOID >> +BuildMemTypeInfoHob ( >> + VOID >> + ) >> +{ >> + BuildGuidDataHob ( >> + &gEfiMemoryTypeInformationGuid, >> + mDefaultMemoryTypeInformation, >> + sizeof mDefaultMemoryTypeInformation >> + ); >> + DEBUG ((DEBUG_INFO, "%a: default memory type information HOB built\n", >> + __FUNCTION__)); >> +} >> + >> +/** >> + Notification function called when EFI_PEI_READ_ONLY_VARIABLE2_PPI becomes >> + available. >> + >> + @param[in] PeiServices Indirect reference to the PEI Services Table. >> + @param[in] NotifyDescriptor Address of the notification descriptor data >> + structure. >> + @param[in] Ppi Address of the PPI that was installed. >> + >> + @return Status of the notification. The status code returned from this >> + function is ignored. >> +**/ >> +STATIC >> +EFI_STATUS >> +EFIAPI >> +OnReadOnlyVariable2Available ( >> + IN EFI_PEI_SERVICES **PeiServices, >> + IN EFI_PEI_NOTIFY_DESCRIPTOR *NotifyDescriptor, >> + IN VOID *Ppi >> + ) >> +{ >> + EFI_PEI_READ_ONLY_VARIABLE2_PPI *ReadOnlyVariable2; >> + UINTN DataSize; >> + EFI_STATUS Status; >> + >> + DEBUG ((DEBUG_VERBOSE, "%a\n", __FUNCTION__)); >> + >> + // >> + // Check if the "MemoryTypeInformation" variable exists, in the >> + // gEfiMemoryTypeInformationGuid namespace. >> + // >> + ReadOnlyVariable2 = Ppi; >> + DataSize = 0; >> + Status = ReadOnlyVariable2->GetVariable ( >> + ReadOnlyVariable2, >> + EFI_MEMORY_TYPE_INFORMATION_VARIABLE_NAME, >> + &gEfiMemoryTypeInformationGuid, >> + NULL, >> + &DataSize, >> + NULL >> + ); >> + switch (Status) { >> + case EFI_BUFFER_TOO_SMALL: >> + // >> + // The variable exists; the DXE IPL PEIM will build the HOB from it. >> + // >> + break; >> + case EFI_NOT_FOUND: >> + // >> + // The variable does not exist; install the default memory type information >> + // HOB. >> + // >> + BuildMemTypeInfoHob (); >> + break; >> + default: >> + DEBUG ((DEBUG_ERROR, "%a: unexpected: GetVariable(): %r\n", __FUNCTION__, >> + Status)); >> + ASSERT (FALSE); >> + CpuDeadLoop (); >> + break; >> + } >> + >> + return EFI_SUCCESS; >> +} >> + >> +// >> +// Notification object for registering the callback, for when >> +// EFI_PEI_READ_ONLY_VARIABLE2_PPI becomes available. >> +// >> +STATIC CONST EFI_PEI_NOTIFY_DESCRIPTOR mReadOnlyVariable2Notify = { >> + (EFI_PEI_PPI_DESCRIPTOR_NOTIFY_DISPATCH | >> + EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST), // Flags >> + &gEfiPeiReadOnlyVariable2PpiGuid, // Guid >> + OnReadOnlyVariable2Available // Notify >> +}; >> + >> +VOID >> +MemTypeInfoInitialization ( >> + VOID >> + ) >> +{ >> + EFI_STATUS Status; >> + >> + if (!FeaturePcdGet (PcdSmmSmramRequire)) { >> + // >> + // EFI_PEI_READ_ONLY_VARIABLE2_PPI will never be available; install >> + // the default memory type information HOB right away. >> + // >> + BuildMemTypeInfoHob (); >> + return; >> + } >> + >> + Status = PeiServicesNotifyPpi (&mReadOnlyVariable2Notify); >> + if (EFI_ERROR (Status)) { >> + DEBUG ((DEBUG_ERROR, "%a: failed to set up R/O Variable 2 callback: %r\n", >> + __FUNCTION__, Status)); >> + ASSERT (FALSE); >> + CpuDeadLoop (); >> + } >> +} >> diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c >> index 473af102783a..587ca68fc210 100644 >> --- a/OvmfPkg/PlatformPei/Platform.c >> +++ b/OvmfPkg/PlatformPei/Platform.c >> @@ -28,7 +28,6 @@ >> #include <Library/QemuFwCfgLib.h> >> #include <Library/QemuFwCfgS3Lib.h> >> #include <Library/ResourcePublicationLib.h> >> -#include <Guid/MemoryTypeInformation.h> >> #include <Ppi/MasterBootMode.h> >> #include <IndustryStandard/I440FxPiix4.h> >> #include <IndustryStandard/Pci22.h> >> @@ -39,18 +38,6 @@ >> #include "Platform.h" >> #include "Cmos.h" >> >> -EFI_MEMORY_TYPE_INFORMATION mDefaultMemoryTypeInformation[] = { >> - { EfiACPIMemoryNVS, 0x004 }, >> - { EfiACPIReclaimMemory, 0x008 }, >> - { EfiReservedMemoryType, 0x004 }, >> - { EfiRuntimeServicesData, 0x024 }, >> - { EfiRuntimeServicesCode, 0x030 }, >> - { EfiBootServicesCode, 0x180 }, >> - { EfiBootServicesData, 0xF00 }, >> - { EfiMaxMemoryType, 0x000 } >> -}; >> - >> - >> EFI_PEI_PPI_DESCRIPTOR mPpiBootMode[] = { >> { >> EFI_PEI_PPI_DESCRIPTOR_PPI | EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST, >> @@ -162,15 +149,6 @@ MemMapInitialization ( >> PciIoBase = 0xC000; >> PciIoSize = 0x4000; >> >> - // >> - // Create Memory Type Information HOB >> - // >> - BuildGuidDataHob ( >> - &gEfiMemoryTypeInformationGuid, >> - mDefaultMemoryTypeInformation, >> - sizeof(mDefaultMemoryTypeInformation) >> - ); >> - >> // >> // Video memory + Legacy BIOS region >> // >> @@ -811,6 +789,7 @@ InitializePlatform ( >> ReserveEmuVariableNvStore (); >> } >> PeiFvInitialization (); >> + MemTypeInfoInitialization (); >> MemMapInitialization (); >> NoexecDxeInitialization (); >> } >> -- >> 2.19.1.3.g30247aa5d201 >> >> >> >> > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [edk2-devel] [PATCH 5/5] OvmfPkg: improve SMM comms security with adaptive MemoryTypeInformation 2020-03-11 16:22 ` Laszlo Ersek @ 2020-03-11 19:36 ` Leif Lindholm 2020-03-12 0:30 ` Laszlo Ersek 0 siblings, 1 reply; 18+ messages in thread From: Leif Lindholm @ 2020-03-11 19:36 UTC (permalink / raw) To: Laszlo Ersek Cc: devel, Ard Biesheuvel, Jordan Justen, Philippe Mathieu-Daudé On Wed, Mar 11, 2020 at 17:22:47 +0100, Laszlo Ersek wrote: > >> +STATIC EFI_MEMORY_TYPE_INFORMATION mDefaultMemoryTypeInformation[] = { > >> + { EfiACPIMemoryNVS, 0x004 }, > >> + { EfiACPIReclaimMemory, 0x008 }, > >> + { EfiReservedMemoryType, 0x004 }, > >> + { EfiRuntimeServicesData, 0x024 }, > >> + { EfiRuntimeServicesCode, 0x030 }, > >> + { EfiBootServicesCode, 0x180 }, > >> + { EfiBootServicesData, 0xF00 }, > >> + { EfiMaxMemoryType, 0x000 } > >> +}; > > > > Could you add a comment as to where these page counts come from? > > Oh, right, they're just moved across from OvmfPkg/PlatformPei/Platform.c. > > > > It *would* be nice if that could be cleaned up at the same time... > > Sorry, I don't understand -- what kind of cleanup do you have in mind > precisely? The table is not copied, but moved from the original place, > so I'm unsure what's left in "Platform.c" to clean up. Not left to clean up there, but something to consider addressing when moving it here. Yes, it's just a move, so we could argue it doesn't need fixing - but it's a struct with a bunch of live-coded numerical values completely without explanation. "I'd rather not" is an acceptable answer, but I figured I should point it out. / Leif > Regarding the origin of the table, it's ancient: > > - 49ba9447c92d ("Add initial version of Open Virtual Machine Firmware > (OVMF) platform.", 2009-05-27) > - 991d95636264 ("Update OVMF Platform PEI to declare IO and MMIO > resources. Update default memory type information to > reduce EFI Memory Map fragmentation.", 2010-07-16) > - 55cdb67acb3d ("OVMF: Support greater than 2GB of memory", 2010-10-13) > > (BTW I have a patch as a separate work item in the queue for this -- I'm > going to remove the EfiBootServicesCode / EfiBootServicesData entries > from this table, per Jiewen's recommendation / explanation in the "WSMT > bits" thread: > > https://edk2.groups.io/g/devel/message/55712 > http://mid.mail-archive.com/a5e71131-65dc-8b85-481a-d35011512987@redhat.com > > But that's a separate patch, for later.) > > Thank you! > Laszlo > > > > > / > > Leif > > > >> + > >> +STATIC > >> +VOID > >> +BuildMemTypeInfoHob ( > >> + VOID > >> + ) > >> +{ > >> + BuildGuidDataHob ( > >> + &gEfiMemoryTypeInformationGuid, > >> + mDefaultMemoryTypeInformation, > >> + sizeof mDefaultMemoryTypeInformation > >> + ); > >> + DEBUG ((DEBUG_INFO, "%a: default memory type information HOB built\n", > >> + __FUNCTION__)); > >> +} > >> + > >> +/** > >> + Notification function called when EFI_PEI_READ_ONLY_VARIABLE2_PPI becomes > >> + available. > >> + > >> + @param[in] PeiServices Indirect reference to the PEI Services Table. > >> + @param[in] NotifyDescriptor Address of the notification descriptor data > >> + structure. > >> + @param[in] Ppi Address of the PPI that was installed. > >> + > >> + @return Status of the notification. The status code returned from this > >> + function is ignored. > >> +**/ > >> +STATIC > >> +EFI_STATUS > >> +EFIAPI > >> +OnReadOnlyVariable2Available ( > >> + IN EFI_PEI_SERVICES **PeiServices, > >> + IN EFI_PEI_NOTIFY_DESCRIPTOR *NotifyDescriptor, > >> + IN VOID *Ppi > >> + ) > >> +{ > >> + EFI_PEI_READ_ONLY_VARIABLE2_PPI *ReadOnlyVariable2; > >> + UINTN DataSize; > >> + EFI_STATUS Status; > >> + > >> + DEBUG ((DEBUG_VERBOSE, "%a\n", __FUNCTION__)); > >> + > >> + // > >> + // Check if the "MemoryTypeInformation" variable exists, in the > >> + // gEfiMemoryTypeInformationGuid namespace. > >> + // > >> + ReadOnlyVariable2 = Ppi; > >> + DataSize = 0; > >> + Status = ReadOnlyVariable2->GetVariable ( > >> + ReadOnlyVariable2, > >> + EFI_MEMORY_TYPE_INFORMATION_VARIABLE_NAME, > >> + &gEfiMemoryTypeInformationGuid, > >> + NULL, > >> + &DataSize, > >> + NULL > >> + ); > >> + switch (Status) { > >> + case EFI_BUFFER_TOO_SMALL: > >> + // > >> + // The variable exists; the DXE IPL PEIM will build the HOB from it. > >> + // > >> + break; > >> + case EFI_NOT_FOUND: > >> + // > >> + // The variable does not exist; install the default memory type information > >> + // HOB. > >> + // > >> + BuildMemTypeInfoHob (); > >> + break; > >> + default: > >> + DEBUG ((DEBUG_ERROR, "%a: unexpected: GetVariable(): %r\n", __FUNCTION__, > >> + Status)); > >> + ASSERT (FALSE); > >> + CpuDeadLoop (); > >> + break; > >> + } > >> + > >> + return EFI_SUCCESS; > >> +} > >> + > >> +// > >> +// Notification object for registering the callback, for when > >> +// EFI_PEI_READ_ONLY_VARIABLE2_PPI becomes available. > >> +// > >> +STATIC CONST EFI_PEI_NOTIFY_DESCRIPTOR mReadOnlyVariable2Notify = { > >> + (EFI_PEI_PPI_DESCRIPTOR_NOTIFY_DISPATCH | > >> + EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST), // Flags > >> + &gEfiPeiReadOnlyVariable2PpiGuid, // Guid > >> + OnReadOnlyVariable2Available // Notify > >> +}; > >> + > >> +VOID > >> +MemTypeInfoInitialization ( > >> + VOID > >> + ) > >> +{ > >> + EFI_STATUS Status; > >> + > >> + if (!FeaturePcdGet (PcdSmmSmramRequire)) { > >> + // > >> + // EFI_PEI_READ_ONLY_VARIABLE2_PPI will never be available; install > >> + // the default memory type information HOB right away. > >> + // > >> + BuildMemTypeInfoHob (); > >> + return; > >> + } > >> + > >> + Status = PeiServicesNotifyPpi (&mReadOnlyVariable2Notify); > >> + if (EFI_ERROR (Status)) { > >> + DEBUG ((DEBUG_ERROR, "%a: failed to set up R/O Variable 2 callback: %r\n", > >> + __FUNCTION__, Status)); > >> + ASSERT (FALSE); > >> + CpuDeadLoop (); > >> + } > >> +} > >> diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c > >> index 473af102783a..587ca68fc210 100644 > >> --- a/OvmfPkg/PlatformPei/Platform.c > >> +++ b/OvmfPkg/PlatformPei/Platform.c > >> @@ -28,7 +28,6 @@ > >> #include <Library/QemuFwCfgLib.h> > >> #include <Library/QemuFwCfgS3Lib.h> > >> #include <Library/ResourcePublicationLib.h> > >> -#include <Guid/MemoryTypeInformation.h> > >> #include <Ppi/MasterBootMode.h> > >> #include <IndustryStandard/I440FxPiix4.h> > >> #include <IndustryStandard/Pci22.h> > >> @@ -39,18 +38,6 @@ > >> #include "Platform.h" > >> #include "Cmos.h" > >> > >> -EFI_MEMORY_TYPE_INFORMATION mDefaultMemoryTypeInformation[] = { > >> - { EfiACPIMemoryNVS, 0x004 }, > >> - { EfiACPIReclaimMemory, 0x008 }, > >> - { EfiReservedMemoryType, 0x004 }, > >> - { EfiRuntimeServicesData, 0x024 }, > >> - { EfiRuntimeServicesCode, 0x030 }, > >> - { EfiBootServicesCode, 0x180 }, > >> - { EfiBootServicesData, 0xF00 }, > >> - { EfiMaxMemoryType, 0x000 } > >> -}; > >> - > >> - > >> EFI_PEI_PPI_DESCRIPTOR mPpiBootMode[] = { > >> { > >> EFI_PEI_PPI_DESCRIPTOR_PPI | EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST, > >> @@ -162,15 +149,6 @@ MemMapInitialization ( > >> PciIoBase = 0xC000; > >> PciIoSize = 0x4000; > >> > >> - // > >> - // Create Memory Type Information HOB > >> - // > >> - BuildGuidDataHob ( > >> - &gEfiMemoryTypeInformationGuid, > >> - mDefaultMemoryTypeInformation, > >> - sizeof(mDefaultMemoryTypeInformation) > >> - ); > >> - > >> // > >> // Video memory + Legacy BIOS region > >> // > >> @@ -811,6 +789,7 @@ InitializePlatform ( > >> ReserveEmuVariableNvStore (); > >> } > >> PeiFvInitialization (); > >> + MemTypeInfoInitialization (); > >> MemMapInitialization (); > >> NoexecDxeInitialization (); > >> } > >> -- > >> 2.19.1.3.g30247aa5d201 > >> > >> > >> > >> > > > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [edk2-devel] [PATCH 5/5] OvmfPkg: improve SMM comms security with adaptive MemoryTypeInformation 2020-03-11 19:36 ` Leif Lindholm @ 2020-03-12 0:30 ` Laszlo Ersek 2020-03-12 10:40 ` Leif Lindholm 0 siblings, 1 reply; 18+ messages in thread From: Laszlo Ersek @ 2020-03-12 0:30 UTC (permalink / raw) To: Leif Lindholm Cc: devel, Ard Biesheuvel, Jordan Justen, Philippe Mathieu-Daudé On 03/11/20 20:36, Leif Lindholm wrote: > On Wed, Mar 11, 2020 at 17:22:47 +0100, Laszlo Ersek wrote: >>>> +STATIC EFI_MEMORY_TYPE_INFORMATION mDefaultMemoryTypeInformation[] = { >>>> + { EfiACPIMemoryNVS, 0x004 }, >>>> + { EfiACPIReclaimMemory, 0x008 }, >>>> + { EfiReservedMemoryType, 0x004 }, >>>> + { EfiRuntimeServicesData, 0x024 }, >>>> + { EfiRuntimeServicesCode, 0x030 }, >>>> + { EfiBootServicesCode, 0x180 }, >>>> + { EfiBootServicesData, 0xF00 }, >>>> + { EfiMaxMemoryType, 0x000 } >>>> +}; >>> >>> Could you add a comment as to where these page counts come from? >>> Oh, right, they're just moved across from OvmfPkg/PlatformPei/Platform.c. >>> >>> It *would* be nice if that could be cleaned up at the same time... >> >> Sorry, I don't understand -- what kind of cleanup do you have in mind >> precisely? The table is not copied, but moved from the original place, >> so I'm unsure what's left in "Platform.c" to clean up. > > Not left to clean up there, but something to consider addressing when > moving it here. Yes, it's just a move, so we could argue it doesn't > need fixing - but it's a struct with a bunch of live-coded numerical > values completely without explanation. > > "I'd rather not" is an acceptable answer, but I figured I should point > it out. Good point! Yet, I'd rather not :) Long read ahead: This table is used for priming the memory type BINs in the DXE Core. After this set, in non-SMM builds, the functionality will remain the same (the table stays in effect for every boot); in SMM builds, the table is only a starting point for the feedback loop. What's important is that the numbers in the table are entirely ad-hoc. They were last updated in commit 991d95636264, in 2010. They capture a set of BIN hints that made sense at the time, for *some* (now unknown) workloads / boot paths. That's the important trait of this table: it made sense at some point in time, for some use case. That's the property we should not regress. So let's consider the possible ways to improve the table. (1) Given that in SMM builds, the table will function only as a starting point for the feedback loop, start using two tables. A new one (for the SMM build) with nice numbers (everything 0x1, or everyting 0x1000, whatever), and keep the old one for the non-SMM build. Unfortunately, this "improvement" is a net negative, because then we'd have *more* stuff, on top of the *current* dump-of-obscure-numbers. (2) Keep the one table, but replace the values with nicer looking numbers (see examples above). Unfortunately, larger numbers could waste memory (stuck in BINs and hence in the UEFI memmap) for some boots, and smaller page counts would increase memmap fragmentation. We might cause some (at best, superficial) regressions. And we'd lose the property "this table made sense at some point in time" -- because the new ad-hoc numbers wouldn't even be rooted in measurements. (3) Actually measure various boots and try to derive new page counts from those. This is something I'm not prepared to do. The memory needs (considering the various memory types too), depend on a bunch of stuff: - ACPI tables generated by QEMU (influences AcpiNVS, AcpiReclaim, and even Reserved -- some opcodes in QEMU's ACPI linker/loader script require the production of S3 boot script opcodes). For example if the QEMU command line specifies the vmgenid device, then the S3 boot script stuff applies. - S3 support enabled/disabled in general on the QEMU cmdline. - OS bootloader footprint. This approach would uphold the property "has been useful at some point in time, for some workloads" -- but it's too much time to research, and it's anyway obviated by the dynamic / adaptive approach that this series enables for OVMF (in the SMM build). (4) OK, so let's not touch the numeric values, but move them out of the table? (4a) Introduce macros. Not a good idea, as these numbers are never referenced anywhere else. The following: #define MTI_RT_DATA_PAGE_COUNT 0x024 ... { EfiRuntimeServicesData, MTI_RT_DATA_PAGE_COUNT }, is not one bit more readable or expressive, but is more wasteful, than the current { EfiRuntimeServicesData, 0x024 }, (4b) Introduce PCDs. Even worse: it elevates these ad-hoc numbers to the OvmfPkg.dec file, without any plan or intent whatsoever to ever update the constants, or to reference them in any other modules, or to override them in any of the locations where PCDs can be overridden (DSC file, FDF file, and for dynamic access PCDs: C code). These numbers are basically a state-dump from a time when they had been found somewhat useful. They still work acceptably, and without an interest in (3), I wouldn't like to touch them with a ten foot pole. :) Thanks, Laszlo ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [edk2-devel] [PATCH 5/5] OvmfPkg: improve SMM comms security with adaptive MemoryTypeInformation 2020-03-12 0:30 ` Laszlo Ersek @ 2020-03-12 10:40 ` Leif Lindholm 2020-03-12 21:19 ` Laszlo Ersek 0 siblings, 1 reply; 18+ messages in thread From: Leif Lindholm @ 2020-03-12 10:40 UTC (permalink / raw) To: Laszlo Ersek Cc: devel, Ard Biesheuvel, Jordan Justen, Philippe Mathieu-Daudé On Thu, Mar 12, 2020 at 01:30:10 +0100, Laszlo Ersek wrote: > On 03/11/20 20:36, Leif Lindholm wrote: > > On Wed, Mar 11, 2020 at 17:22:47 +0100, Laszlo Ersek wrote: > >>>> +STATIC EFI_MEMORY_TYPE_INFORMATION mDefaultMemoryTypeInformation[] = { > >>>> + { EfiACPIMemoryNVS, 0x004 }, > >>>> + { EfiACPIReclaimMemory, 0x008 }, > >>>> + { EfiReservedMemoryType, 0x004 }, > >>>> + { EfiRuntimeServicesData, 0x024 }, > >>>> + { EfiRuntimeServicesCode, 0x030 }, > >>>> + { EfiBootServicesCode, 0x180 }, > >>>> + { EfiBootServicesData, 0xF00 }, > >>>> + { EfiMaxMemoryType, 0x000 } > >>>> +}; > >>> > >>> Could you add a comment as to where these page counts come from? > >>> Oh, right, they're just moved across from OvmfPkg/PlatformPei/Platform.c. > >>> > >>> It *would* be nice if that could be cleaned up at the same time... > >> > >> Sorry, I don't understand -- what kind of cleanup do you have in mind > >> precisely? The table is not copied, but moved from the original place, > >> so I'm unsure what's left in "Platform.c" to clean up. > > > > Not left to clean up there, but something to consider addressing when > > moving it here. Yes, it's just a move, so we could argue it doesn't > > need fixing - but it's a struct with a bunch of live-coded numerical > > values completely without explanation. > > > > "I'd rather not" is an acceptable answer, but I figured I should point > > it out. > > Good point! > > Yet, I'd rather not :) Long read ahead: > > This table is used for priming the memory type BINs in the DXE Core. > After this set, in non-SMM builds, the functionality will remain the > same (the table stays in effect for every boot); in SMM builds, the > table is only a starting point for the feedback loop. > > What's important is that the numbers in the table are entirely ad-hoc. > They were last updated in commit 991d95636264, in 2010. They capture a > set of BIN hints that made sense at the time, for *some* (now unknown) > workloads / boot paths. That's the important trait of this table: it > made sense at some point in time, for some use case. That's the property > we should not regress. > > So let's consider the possible ways to improve the table. There is fixing and there is improving. The preceding paragraph as a comment block would prevent the next person who comes across it going "Where the $EXPLETIVE did these numbers come from?". (Adding the preceding paragraph as well would have saved me another minute of grepping, but that is more down to the fact that this is a repeating pattern implemented differently for different platforms - for most ARM platforms partly hidden away in EmbeddedPkg: https://github.com/tianocore/edk2/blob/master/EmbeddedPkg/EmbeddedPkg.dec#L104) > (1) Given that in SMM builds, the table will function only as a starting > point for the feedback loop, start using two tables. A new one (for the > SMM build) with nice numbers (everything 0x1, or everyting 0x1000, > whatever), and keep the old one for the non-SMM build. > > Unfortunately, this "improvement" is a net negative, because then we'd > have *more* stuff, on top of the *current* dump-of-obscure-numbers. > > (2) Keep the one table, but replace the values with nicer looking > numbers (see examples above). Unfortunately, larger numbers could waste > memory (stuck in BINs and hence in the UEFI memmap) for some boots, and > smaller page counts would increase memmap fragmentation. > > We might cause some (at best, superficial) regressions. And we'd lose > the property "this table made sense at some point in time" -- because > the new ad-hoc numbers wouldn't even be rooted in measurements. > > (3) Actually measure various boots and try to derive new page counts > from those. > > This is something I'm not prepared to do. The memory needs (considering > the various memory types too), depend on a bunch of stuff: > > - ACPI tables generated by QEMU (influences AcpiNVS, AcpiReclaim, and > even Reserved -- some opcodes in QEMU's ACPI linker/loader script > require the production of S3 boot script opcodes). For example if the > QEMU command line specifies the vmgenid device, then the S3 boot script > stuff applies. > > - S3 support enabled/disabled in general on the QEMU cmdline. > > - OS bootloader footprint. - Separately loaded drivers (including Option ROMs?), making these numers impossible to precisely determine statically. > This approach would uphold the property "has been useful at some point > in time, for some workloads" -- but it's too much time to research, and > it's anyway obviated by the dynamic / adaptive approach that this series > enables for OVMF (in the SMM build). > > (4) OK, so let's not touch the numeric values, but move them out of the > table? > > (4a) Introduce macros. > > Not a good idea, as these numbers are never referenced anywhere else. > The following: > > #define MTI_RT_DATA_PAGE_COUNT 0x024 > ... > { EfiRuntimeServicesData, MTI_RT_DATA_PAGE_COUNT }, > > is not one bit more readable or expressive, but is more wasteful, than > the current > > { EfiRuntimeServicesData, 0x024 }, > > (4b) Introduce PCDs. > > Even worse: it elevates these ad-hoc numbers to the OvmfPkg.dec file, > without any plan or intent whatsoever to ever update the constants, or > to reference them in any other modules, or to override them in any of > the locations where PCDs can be overridden (DSC file, FDF file, and for > dynamic access PCDs: C code). See EmbeddedPkg. Basically, all of the variants you enumerate exist in the tree(s) today. > These numbers are basically a state-dump from a time when they had been > found somewhat useful. They still work acceptably, and without an > interest in (3), I wouldn't like to touch them with a ten foot pole. :) Sure. So what I'm *actually* after is. (5) *Somehow* standardise how platforms build up the HOB I think this means *hiding* BuildGuidDataHob() behind some higher-level functions, backed by some market-segment (or market-segment:architecture tuple) specific defaults. But for this patch, if you could add the archaeology bit in a comment block, I think that would be useful for whatever next lost soul stumbles upon it. With or without that, for the series: Acked-by: Leif Lindholm <leif@nuviainc.com> ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [edk2-devel] [PATCH 5/5] OvmfPkg: improve SMM comms security with adaptive MemoryTypeInformation 2020-03-12 10:40 ` Leif Lindholm @ 2020-03-12 21:19 ` Laszlo Ersek 0 siblings, 0 replies; 18+ messages in thread From: Laszlo Ersek @ 2020-03-12 21:19 UTC (permalink / raw) To: Leif Lindholm Cc: devel, Ard Biesheuvel, Jordan Justen, Philippe Mathieu-Daudé On 03/12/20 11:40, Leif Lindholm wrote: > On Thu, Mar 12, 2020 at 01:30:10 +0100, Laszlo Ersek wrote: >> On 03/11/20 20:36, Leif Lindholm wrote: >>> On Wed, Mar 11, 2020 at 17:22:47 +0100, Laszlo Ersek wrote: >>>>>> +STATIC EFI_MEMORY_TYPE_INFORMATION mDefaultMemoryTypeInformation[] = { >>>>>> + { EfiACPIMemoryNVS, 0x004 }, >>>>>> + { EfiACPIReclaimMemory, 0x008 }, >>>>>> + { EfiReservedMemoryType, 0x004 }, >>>>>> + { EfiRuntimeServicesData, 0x024 }, >>>>>> + { EfiRuntimeServicesCode, 0x030 }, >>>>>> + { EfiBootServicesCode, 0x180 }, >>>>>> + { EfiBootServicesData, 0xF00 }, >>>>>> + { EfiMaxMemoryType, 0x000 } >>>>>> +}; >>>>> >>>>> Could you add a comment as to where these page counts come from? >>>>> Oh, right, they're just moved across from OvmfPkg/PlatformPei/Platform.c. >>>>> >>>>> It *would* be nice if that could be cleaned up at the same time... >>>> >>>> Sorry, I don't understand -- what kind of cleanup do you have in mind >>>> precisely? The table is not copied, but moved from the original place, >>>> so I'm unsure what's left in "Platform.c" to clean up. >>> >>> Not left to clean up there, but something to consider addressing when >>> moving it here. Yes, it's just a move, so we could argue it doesn't >>> need fixing - but it's a struct with a bunch of live-coded numerical >>> values completely without explanation. >>> >>> "I'd rather not" is an acceptable answer, but I figured I should point >>> it out. >> >> Good point! >> >> Yet, I'd rather not :) Long read ahead: >> >> This table is used for priming the memory type BINs in the DXE Core. >> After this set, in non-SMM builds, the functionality will remain the >> same (the table stays in effect for every boot); in SMM builds, the >> table is only a starting point for the feedback loop. >> >> What's important is that the numbers in the table are entirely ad-hoc. >> They were last updated in commit 991d95636264, in 2010. They capture a >> set of BIN hints that made sense at the time, for *some* (now unknown) >> workloads / boot paths. That's the important trait of this table: it >> made sense at some point in time, for some use case. That's the property >> we should not regress. >> >> So let's consider the possible ways to improve the table. > > There is fixing and there is improving. > > The preceding paragraph as a comment block would prevent the next > person who comes across it going "Where the $EXPLETIVE did these > numbers come from?". > > (Adding the preceding paragraph as well would have saved me another > minute of grepping, but that is more down to the fact that this is a > repeating pattern implemented differently for different platforms - > for most ARM platforms partly hidden away in EmbeddedPkg: > https://github.com/tianocore/edk2/blob/master/EmbeddedPkg/EmbeddedPkg.dec#L104) > >> (1) Given that in SMM builds, the table will function only as a starting >> point for the feedback loop, start using two tables. A new one (for the >> SMM build) with nice numbers (everything 0x1, or everyting 0x1000, >> whatever), and keep the old one for the non-SMM build. >> >> Unfortunately, this "improvement" is a net negative, because then we'd >> have *more* stuff, on top of the *current* dump-of-obscure-numbers. >> >> (2) Keep the one table, but replace the values with nicer looking >> numbers (see examples above). Unfortunately, larger numbers could waste >> memory (stuck in BINs and hence in the UEFI memmap) for some boots, and >> smaller page counts would increase memmap fragmentation. >> >> We might cause some (at best, superficial) regressions. And we'd lose >> the property "this table made sense at some point in time" -- because >> the new ad-hoc numbers wouldn't even be rooted in measurements. >> >> (3) Actually measure various boots and try to derive new page counts >> from those. >> >> This is something I'm not prepared to do. The memory needs (considering >> the various memory types too), depend on a bunch of stuff: >> >> - ACPI tables generated by QEMU (influences AcpiNVS, AcpiReclaim, and >> even Reserved -- some opcodes in QEMU's ACPI linker/loader script >> require the production of S3 boot script opcodes). For example if the >> QEMU command line specifies the vmgenid device, then the S3 boot script >> stuff applies. >> >> - S3 support enabled/disabled in general on the QEMU cmdline. >> >> - OS bootloader footprint. > > - Separately loaded drivers (including Option ROMs?), making these > numers impossible to precisely determine statically. > >> This approach would uphold the property "has been useful at some point >> in time, for some workloads" -- but it's too much time to research, and >> it's anyway obviated by the dynamic / adaptive approach that this series >> enables for OVMF (in the SMM build). >> >> (4) OK, so let's not touch the numeric values, but move them out of the >> table? >> >> (4a) Introduce macros. >> >> Not a good idea, as these numbers are never referenced anywhere else. >> The following: >> >> #define MTI_RT_DATA_PAGE_COUNT 0x024 >> ... >> { EfiRuntimeServicesData, MTI_RT_DATA_PAGE_COUNT }, >> >> is not one bit more readable or expressive, but is more wasteful, than >> the current >> >> { EfiRuntimeServicesData, 0x024 }, >> >> (4b) Introduce PCDs. >> >> Even worse: it elevates these ad-hoc numbers to the OvmfPkg.dec file, >> without any plan or intent whatsoever to ever update the constants, or >> to reference them in any other modules, or to override them in any of >> the locations where PCDs can be overridden (DSC file, FDF file, and for >> dynamic access PCDs: C code). > > See EmbeddedPkg. > > Basically, all of the variants you enumerate exist in the tree(s) > today. > >> These numbers are basically a state-dump from a time when they had been >> found somewhat useful. They still work acceptably, and without an >> interest in (3), I wouldn't like to touch them with a ten foot pole. :) > > Sure. > > So what I'm *actually* after is. > > (5) *Somehow* standardise how platforms build up the HOB > > I think this means *hiding* BuildGuidDataHob() behind some > higher-level functions, backed by some market-segment (or > market-segment:architecture tuple) specific defaults. > > > But for this patch, if you could add the archaeology bit in a comment > block, I think that would be useful for whatever next lost soul > stumbles upon it. > > With or without that, for the series: > Acked-by: Leif Lindholm <leif@nuviainc.com> > Merged as-is, in commit range 7d325f93e190..d42fdd6f8384, via <https://github.com/tianocore/edk2/pull/442>. I am going to submit a separate patch with the suggested comment. Thank you! Laszlo ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2020-03-12 22:20 UTC | newest] Thread overview: 18+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-03-10 22:27 [PATCH 0/5] OvmfPkg: improve SMM comms security with adaptive MemoryTypeInformation Laszlo Ersek 2020-03-10 22:27 ` [PATCH 1/5] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: drop unused PCDs Laszlo Ersek 2020-03-10 22:27 ` [PATCH 2/5] OvmfPkg/QemuFlashFvbServices: factor out SetPcdFlashNvStorageBaseAddresses Laszlo Ersek 2020-03-10 22:27 ` [PATCH 3/5] OvmfPkg: set fixed FlashNvStorage base addresses with -D SMM_REQUIRE Laszlo Ersek 2020-03-11 15:44 ` [edk2-devel] " Leif Lindholm 2020-03-11 16:14 ` Laszlo Ersek 2020-03-11 16:20 ` Leif Lindholm 2020-03-11 16:41 ` Laszlo Ersek 2020-03-12 14:20 ` Leif Lindholm 2020-03-12 22:19 ` Laszlo Ersek 2020-03-10 22:27 ` [PATCH 4/5] OvmfPkg: include FaultTolerantWritePei and VariablePei " Laszlo Ersek 2020-03-10 22:27 ` [PATCH 5/5] OvmfPkg: improve SMM comms security with adaptive MemoryTypeInformation Laszlo Ersek 2020-03-11 16:00 ` [edk2-devel] " Leif Lindholm 2020-03-11 16:22 ` Laszlo Ersek 2020-03-11 19:36 ` Leif Lindholm 2020-03-12 0:30 ` Laszlo Ersek 2020-03-12 10:40 ` Leif Lindholm 2020-03-12 21:19 ` Laszlo Ersek
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox