[PATCH 0/5] OvmfPkg: improve SMM comms security with adaptive MemoryTypeInformation

[PATCH 0/5] OvmfPkg: improve SMM comms security with adaptive MemoryTypeInformation

[Laszlo Ersek]

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

[PATCH 1/5] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: drop unused PCDs

[Laszlo Ersek]

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

[PATCH 2/5] OvmfPkg/QemuFlashFvbServices: factor out SetPcdFlashNvStorageBaseAddresses

[Laszlo Ersek]

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

[PATCH 3/5] OvmfPkg: set fixed FlashNvStorage base addresses with -D SMM_REQUIRE

[Laszlo Ersek]

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

[PATCH 4/5] OvmfPkg: include FaultTolerantWritePei and VariablePei with -D SMM_REQUIRE

[Laszlo Ersek]

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

[PATCH 5/5] OvmfPkg: improve SMM comms security with adaptive MemoryTypeInformation

[Laszlo Ersek]

* 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

Re: [edk2-devel] [PATCH 3/5] OvmfPkg: set fixed FlashNvStorage base addresses with -D SMM_REQUIRE

[Leif Lindholm]

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

Re: [edk2-devel] [PATCH 5/5] OvmfPkg: improve SMM comms security with adaptive MemoryTypeInformation

[Leif Lindholm]

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

Re: [edk2-devel] [PATCH 3/5] OvmfPkg: set fixed FlashNvStorage base addresses with -D SMM_REQUIRE

[Laszlo Ersek]

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

Re: [edk2-devel] [PATCH 3/5] OvmfPkg: set fixed FlashNvStorage base addresses with -D SMM_REQUIRE

[Leif Lindholm]

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

Re: [edk2-devel] [PATCH 5/5] OvmfPkg: improve SMM comms security with adaptive MemoryTypeInformation

[Laszlo Ersek]

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

Re: [edk2-devel] [PATCH 3/5] OvmfPkg: set fixed FlashNvStorage base addresses with -D SMM_REQUIRE

[Laszlo Ersek]

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

Re: [edk2-devel] [PATCH 5/5] OvmfPkg: improve SMM comms security with adaptive MemoryTypeInformation

[Leif Lindholm]

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

Re: [edk2-devel] [PATCH 5/5] OvmfPkg: improve SMM comms security with adaptive MemoryTypeInformation

[Laszlo Ersek]

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

Re: [edk2-devel] [PATCH 5/5] OvmfPkg: improve SMM comms security with adaptive MemoryTypeInformation

[Leif Lindholm]

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>

Re: [edk2-devel] [PATCH 3/5] OvmfPkg: set fixed FlashNvStorage base addresses with -D SMM_REQUIRE

[Leif Lindholm]

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

Re: [edk2-devel] [PATCH 5/5] OvmfPkg: improve SMM comms security with adaptive MemoryTypeInformation

[Laszlo Ersek]

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

Re: [edk2-devel] [PATCH 3/5] OvmfPkg: set fixed FlashNvStorage base addresses with -D SMM_REQUIRE

[Laszlo Ersek]

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