[PATCH 00/14] OvmfPkg: add the Variable PEIM, defragment the UEFI memmap

[PATCH 00/14] OvmfPkg: add the Variable PEIM, defragment the UEFI memmap

[Laszlo Ersek]

This series implements the ideas discussed in the thread

  [edk2] memory type information HOB / UEFI memmap defrag
  https://lists.01.org/pipermail/edk2-devel/2017-February/007576.html

I'm seeing good results, with approx. 20-24 UEFI memmap entries removed.

The benefits of the patch set are automatically enabled for SMM_REQUIRE
builds.

For non-SMM builds (which are the default), another build flag is being
introduced, namely MEM_VARSTORE_EMU_ENABLE. This build flag controls the
reserved memory / NvVars file based variable emulation. It remains
enabled by default (keeping the current status quo). For benefiting from
the Variable PEIM and the dependent UEFI memmap defragmentation,
MEM_VARSTORE_EMU_ENABLE has to be *disabled*. There are two reasons for
this:

- The PEI phase FTW and Variable modules need immediate (at-startup)
  access to the variable store, but the reserved memory based emulation
  depends on dynamic allocation, plus moving the dynamic pflash
  detection to PEI is out of question.

- Even more importantly, reading actual variable contents from the
  non-pflash varstore would require, in PEI, similar hackery that
  currently happens in BDS -- that's not going to happen.

In fact one of the longer term goals with MEM_VARSTORE_EMU_ENABLE is to
identify what we'll rip out once we finally decide to drop the reserved
memory / NvVars file based emulation.

A summary of build modes:

* default build: works as before, with the Variable PEIM and UEFI memmap
  defragmentation remaining unavailable.

* -D SMM_REQUIRE: works as before, with the Variable PEIM and UEFI
  memmap defragmentation enabled.

  As a reminder, this build is inherently incompatible with QEMU's
  "-bios" and "-L" parameters, which will trigger the ASSERT() in
  QemuFlashInitialize()
  [OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c], added in commit
  b963ec494c48 ("OvmfPkg: QemuFlashFvbServicesRuntimeDxe: adhere to -D
  SMM_REQUIRE", 2015-11-30).

* -D MEM_VARSTORE_EMU_ENABLE=FALSE: eliminates the reserved memory /
  NvVars based variable emulation, turns pflash into a hard requirement,
  and enables the Variable PEIM and the UEFI memmap defragmentation.

  If such a build is executed with -bios or -L, then the FTW and
  Variable PEIMs will temporarily see a well-formed, but empty varstore
  (straight from "OvmfPkg/VarStore.fdf.inc"), and then
  QemuFlashFvbServicesRuntimeDxe will trip a new ASSERT(), in patch #6,
  that is similar to the one added in commit b963ec494c48 (see above).

Anatomy of the series:

* Patches 01 through 04 clean up some innocent warts I noticed while
  working on the feature.

* Patches 05 through 07 introduce the new build option and its basic
  effect, the disabling of reserved memory based variable emulation.

* Patches 08 through 12 include the FTW and Variable PEIMs.

* Patch 13 enables UEFI memmap defragmentation.

* Patch 14 updates the README file.

The variable PEIM was independently requested in
<https://bugzilla.tianocore.org/show_bug.cgi?id=386> earlier, but
without a good upstreamable use case, I disagreed with its inclusion.
The memmap defragmentation is a good use case however.

Also, I didn't lie about "downstream pressure" in my previous patch set;
I wrote this one at Sunday/Monday night (and now it's Tues/Wed night).
So one could consider this personal diligence. :)

The series conforms to the multi-line function call syntax outlined in
<https://bugzilla.tianocore.org/show_bug.cgi?id=425>.

Repo:   https://github.com/lersek/edk2.git
Branch: memmap_defrag

CC: Jordan Justen <jordan.l.justen@intel.com>

Thanks
Laszlo

Laszlo Ersek (14):
  OvmfPkg/EmuVariableFvbRuntimeDxe: always format an auth varstore
    header
  OvmfPkg: remove gUefiOvmfPkgTokenSpaceGuid.PcdSecureBootEnable
  OvmfPkg/PlatformPei: remove unused PcdVariableStoreSize dependency
  OvmfPkg/PlatformPei: don't allocate reserved mem varstore if
    SMM_REQUIRE
  OvmfPkg: introduce PcdMemVarstoreEmuEnable feature flag
  OvmfPkg/QemuFlashFvbServicesRuntimeDxe: check PcdMemVarstoreEmuEnable
  OvmfPkg: conditionally disable reserved memory varstore emulation at
    build
  OvmfPkg: resolve PcdLib for all PEIMs individually
  OvmfPkg: resolve PcdLib for PEIMs to PeiPcdLib by default
  OvmfPkg: introduce FlashNvStorageAddressLib
  OvmfPkg: include FaultTolerantWritePei and VariablePei
  OvmfPkg/QemuFlashFvbServicesRuntimeDxe: don't set flash PCDs if SMM or
    no-emu
  OvmfPkg/PlatformPei: remedy UEFI memory map fragmentation
  OvmfPkg/README: document MEM_VARSTORE_EMU_ENABLE and memmap defrag

 OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c                                |  79 +---------
 OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.inf                              |   3 -
 OvmfPkg/Library/FlashNvStorageAddressLib/FlashNvStorageAddressLib.c   |  53 +++++++
 OvmfPkg/Library/FlashNvStorageAddressLib/FlashNvStorageAddressLib.inf |  48 +++++++
 OvmfPkg/OvmfPkg.dec                                                   |   7 +-
 OvmfPkg/OvmfPkgIa32.dsc                                               |  43 +++---
 OvmfPkg/OvmfPkgIa32.fdf                                               |   8 +-
 OvmfPkg/OvmfPkgIa32X64.dsc                                            |  43 +++---
 OvmfPkg/OvmfPkgIa32X64.fdf                                            |   8 +-
 OvmfPkg/OvmfPkgX64.dsc                                                |  43 +++---
 OvmfPkg/OvmfPkgX64.fdf                                                |   8 +-
 OvmfPkg/PlatformPei/MemTypeInfo.c                                     | 151 ++++++++++++++++++++
 OvmfPkg/PlatformPei/Platform.c                                        |  28 +---
 OvmfPkg/PlatformPei/Platform.h                                        |   5 +
 OvmfPkg/PlatformPei/PlatformPei.inf                                   |   4 +-
 OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf      |   1 +
 OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesSmm.inf             |   1 +
 OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c               |  58 +++++---
 OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c                    |   1 +
 OvmfPkg/README                                                        |  10 ++
 20 files changed, 425 insertions(+), 177 deletions(-)
 create mode 100644 OvmfPkg/Library/FlashNvStorageAddressLib/FlashNvStorageAddressLib.inf
 create mode 100644 OvmfPkg/Library/FlashNvStorageAddressLib/FlashNvStorageAddressLib.c
 create mode 100644 OvmfPkg/PlatformPei/MemTypeInfo.c

-- 
2.9.3

[PATCH 01/14] OvmfPkg/EmuVariableFvbRuntimeDxe: always format an auth varstore header

[Laszlo Ersek]

In this patch, we extend commit d92eaabefbe0 ("OvmfPkg: simplify
VARIABLE_STORE_HEADER generation", 2016-02-05) to
EmuVariableFvbRuntimeDxe.

Cc: Jordan Justen <jordan.l.justen@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.inf |  3 -
 OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c   | 79 ++------------------
 2 files changed, 5 insertions(+), 77 deletions(-)

diff --git a/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.inf b/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.inf
index 4d4827decb52..69b3c9972a76 100644
--- a/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.inf
+++ b/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.inf
@@ -68,9 +68,6 @@ [Pcd]
   gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase
   gEfiMdeModulePkgTokenSpaceGuid.PcdEmuVariableNvStoreReserved
 
-[FeaturePcd]
-  gUefiOvmfPkgTokenSpaceGuid.PcdSecureBootEnable
-
 [Depex]
   TRUE
 
diff --git a/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c b/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c
index dec6d4af50df..7a6d3153ec8c 100644
--- a/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c
+++ b/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c
@@ -626,75 +626,6 @@ InitializeFvAndVariableStoreHeaders (
   )
 {
   //
-  // Templates for standard (non-authenticated) variable FV header
-  //
-  STATIC FVB_FV_HDR_AND_VARS_TEMPLATE FvAndVarTemplate = {
-    { // EFI_FIRMWARE_VOLUME_HEADER FvHdr;
-      // UINT8                     ZeroVector[16];
-      { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 },
-
-      // EFI_GUID                  FileSystemGuid;
-      EFI_SYSTEM_NV_DATA_FV_GUID,
-
-      // UINT64                    FvLength;
-      EMU_FVB_SIZE,
-
-      // UINT32                    Signature;
-      EFI_FVH_SIGNATURE,
-
-      // EFI_FVB_ATTRIBUTES_2      Attributes;
-      0x4feff,
-
-      // UINT16                    HeaderLength;
-      EMU_FV_HEADER_LENGTH,
-
-      // UINT16                    Checksum;
-      0,
-
-      // UINT16                    ExtHeaderOffset;
-      0,
-
-      // UINT8                     Reserved[1];
-      {0},
-
-      // UINT8                     Revision;
-      EFI_FVH_REVISION,
-
-      // EFI_FV_BLOCK_MAP_ENTRY    BlockMap[1];
-      { 
-        {
-          2, // UINT32 NumBlocks;
-          EMU_FVB_BLOCK_SIZE  // UINT32 Length;
-        }
-      }
-    },
-    // EFI_FV_BLOCK_MAP_ENTRY     EndBlockMap;
-    { 0, 0 }, // End of block map
-    { // VARIABLE_STORE_HEADER      VarHdr;
-      // EFI_GUID  Signature;
-      EFI_VARIABLE_GUID,
-
-      // UINT32  Size;
-      (
-        FixedPcdGet32 (PcdVariableStoreSize) -
-        OFFSET_OF (FVB_FV_HDR_AND_VARS_TEMPLATE, VarHdr)
-      ),
-
-      // UINT8   Format;
-      VARIABLE_STORE_FORMATTED,
-
-      // UINT8   State;
-      VARIABLE_STORE_HEALTHY,
-
-      // UINT16  Reserved;
-      0,
-
-      // UINT32  Reserved1;
-      0
-    }
-  };
-
-  //
   // Templates for authenticated variable FV header
   //
   STATIC FVB_FV_HDR_AND_VARS_TEMPLATE FvAndAuthenticatedVarTemplate = {
@@ -768,11 +699,11 @@ InitializeFvAndVariableStoreHeaders (
   //
   // Copy the template structure into the location
   //
-  if (FeaturePcdGet (PcdSecureBootEnable) == FALSE) {
-    CopyMem (Ptr, (VOID*)&FvAndVarTemplate, sizeof (FvAndVarTemplate));
-  } else {
-    CopyMem (Ptr, (VOID*)&FvAndAuthenticatedVarTemplate, sizeof (FvAndAuthenticatedVarTemplate));
-  }
+  CopyMem (
+    Ptr,
+    &FvAndAuthenticatedVarTemplate,
+    sizeof FvAndAuthenticatedVarTemplate
+    );
 
   //
   // Update the checksum for the FV header
-- 
2.9.3

[PATCH 02/14] OvmfPkg: remove gUefiOvmfPkgTokenSpaceGuid.PcdSecureBootEnable

[Laszlo Ersek]

This PCD is no longer used.

Cc: Jordan Justen <jordan.l.justen@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/OvmfPkg.dec        | 1 -
 OvmfPkg/OvmfPkgIa32.dsc    | 3 ---
 OvmfPkg/OvmfPkgIa32X64.dsc | 3 ---
 OvmfPkg/OvmfPkgX64.dsc     | 3 ---
 4 files changed, 10 deletions(-)

diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
index 3490f7ca7c07..5627be0bab0a 100644
--- a/OvmfPkg/OvmfPkg.dec
+++ b/OvmfPkg/OvmfPkg.dec
@@ -136,7 +136,6 @@ [PcdsDynamic, PcdsDynamicEx]
   gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio64Size|0x0|UINT64|0x27
 
 [PcdsFeatureFlag]
-  gUefiOvmfPkgTokenSpaceGuid.PcdSecureBootEnable|FALSE|BOOLEAN|3
   gUefiOvmfPkgTokenSpaceGuid.PcdQemuBootOrderPciTranslation|TRUE|BOOLEAN|0x1c
   gUefiOvmfPkgTokenSpaceGuid.PcdQemuBootOrderMmioTranslation|FALSE|BOOLEAN|0x1d
 
diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
index 0796b0db816b..6d24ef113e64 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -381,9 +381,6 @@ [PcdsFeatureFlag]
   gEfiMdeModulePkgTokenSpaceGuid.PcdConOutGopSupport|TRUE
   gEfiMdeModulePkgTokenSpaceGuid.PcdConOutUgaSupport|FALSE
   gEfiMdeModulePkgTokenSpaceGuid.PcdInstallAcpiSdtProtocol|TRUE
-!if $(SECURE_BOOT_ENABLE) == TRUE
-  gUefiOvmfPkgTokenSpaceGuid.PcdSecureBootEnable|TRUE
-!endif
 !if $(SMM_REQUIRE) == TRUE
   gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire|TRUE
   gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmEnableBspElection|FALSE
diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index 71ac62f023b5..e9d5322d35cc 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -386,9 +386,6 @@ [PcdsFeatureFlag]
   gEfiMdeModulePkgTokenSpaceGuid.PcdConOutGopSupport|TRUE
   gEfiMdeModulePkgTokenSpaceGuid.PcdConOutUgaSupport|FALSE
   gEfiMdeModulePkgTokenSpaceGuid.PcdInstallAcpiSdtProtocol|TRUE
-!if $(SECURE_BOOT_ENABLE) == TRUE
-  gUefiOvmfPkgTokenSpaceGuid.PcdSecureBootEnable|TRUE
-!endif
 !if $(SMM_REQUIRE) == TRUE
   gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire|TRUE
   gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmEnableBspElection|FALSE
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index 2ceb31d7ffd5..65ed44e827fc 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -386,9 +386,6 @@ [PcdsFeatureFlag]
   gEfiMdeModulePkgTokenSpaceGuid.PcdConOutGopSupport|TRUE
   gEfiMdeModulePkgTokenSpaceGuid.PcdConOutUgaSupport|FALSE
   gEfiMdeModulePkgTokenSpaceGuid.PcdInstallAcpiSdtProtocol|TRUE
-!if $(SECURE_BOOT_ENABLE) == TRUE
-  gUefiOvmfPkgTokenSpaceGuid.PcdSecureBootEnable|TRUE
-!endif
 !if $(SMM_REQUIRE) == TRUE
   gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire|TRUE
   gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmEnableBspElection|FALSE
-- 
2.9.3

[PATCH 03/14] OvmfPkg/PlatformPei: remove unused PcdVariableStoreSize dependency

[Laszlo Ersek]

Cc: Jordan Justen <jordan.l.justen@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/PlatformPei/PlatformPei.inf | 1 -
 1 file changed, 1 deletion(-)

diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf b/OvmfPkg/PlatformPei/PlatformPei.inf
index 53c6dd445a0e..a1e12c1fc7e2 100644
--- a/OvmfPkg/PlatformPei/PlatformPei.inf
+++ b/OvmfPkg/PlatformPei/PlatformPei.inf
@@ -84,7 +84,6 @@ [Pcd]
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfDecompressionScratchEnd
   gUefiOvmfPkgTokenSpaceGuid.PcdQ35TsegMbytes
   gEfiMdePkgTokenSpaceGuid.PcdGuidedExtractHandlerTableAddress
-  gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize
   gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize
   gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize
   gEfiMdeModulePkgTokenSpaceGuid.PcdEmuVariableNvStoreReserved
-- 
2.9.3

[PATCH 04/14] OvmfPkg/PlatformPei: don't allocate reserved mem varstore if SMM_REQUIRE

[Laszlo Ersek]

For the emulated variable store, PlatformPei allocates reserved memory (as
early as possible, so that the address remains the same during reboot),
and PcdEmuVariableNvStoreReserved carries the address to
EmuVariableFvbRuntimeDxe.

However, EmuVariableFvbRuntimeDxe is excluded from the SMM_REQUIRE build,
and then noone consumes PcdEmuVariableNvStoreReserved. Don't waste
reserved memory whenever that's the case.

(Even a dynamic default for PcdEmuVariableNvStoreReserved would be
unnecessary; but that way the PcdSet64S() call in the
ReserveEmuVariableNvStore() function doesn't compile.)

Cc: Jordan Justen <jordan.l.justen@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/OvmfPkgIa32.dsc        | 3 +++
 OvmfPkg/OvmfPkgIa32X64.dsc     | 3 +++
 OvmfPkg/OvmfPkgX64.dsc         | 3 +++
 OvmfPkg/PlatformPei/Platform.c | 4 +++-
 4 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
index 6d24ef113e64..8e88744087e0 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -469,7 +469,10 @@ [PcdsFixedAtBuild]
 ################################################################################
 
 [PcdsDynamicDefault]
+  # only set when
+  #   ($(SMM_REQUIRE) == FALSE)
   gEfiMdeModulePkgTokenSpaceGuid.PcdEmuVariableNvStoreReserved|0
+
   gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64|0
   gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase|0
   gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase|0
diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index e9d5322d35cc..18348d8e9c58 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -475,7 +475,10 @@ [PcdsFixedAtBuild.X64]
 ################################################################################
 
 [PcdsDynamicDefault]
+  # only set when
+  #   ($(SMM_REQUIRE) == FALSE)
   gEfiMdeModulePkgTokenSpaceGuid.PcdEmuVariableNvStoreReserved|0
+
   gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64|0
   gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase|0
   gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase|0
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index 65ed44e827fc..76dd72a0d0d0 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -474,7 +474,10 @@ [PcdsFixedAtBuild]
 ################################################################################
 
 [PcdsDynamicDefault]
+  # only set when
+  #   ($(SMM_REQUIRE) == FALSE)
   gEfiMdeModulePkgTokenSpaceGuid.PcdEmuVariableNvStoreReserved|0
+
   gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64|0
   gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase|0
   gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase|0
diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
index 77a8a16c15b8..041eb9d189b4 100644
--- a/OvmfPkg/PlatformPei/Platform.c
+++ b/OvmfPkg/PlatformPei/Platform.c
@@ -661,7 +661,9 @@ InitializePlatform (
   mHostBridgeDevId = PciRead16 (OVMF_HOSTBRIDGE_DID);
 
   if (mBootMode != BOOT_ON_S3_RESUME) {
-    ReserveEmuVariableNvStore ();
+    if (!FeaturePcdGet (PcdSmmSmramRequire)) {
+      ReserveEmuVariableNvStore ();
+    }
     PeiFvInitialization ();
     MemMapInitialization ();
     NoexecDxeInitialization ();
-- 
2.9.3

[PATCH 05/14] OvmfPkg: introduce PcdMemVarstoreEmuEnable feature flag

[Laszlo Ersek]

... and the corresponding MEM_VARSTORE_EMU_ENABLE build define, which
defaults to TRUE.

Setting the build flag to FALSE will later enable the exclusion of the
dynamically allocated, emulated, in-memory varstore.

Cc: Jordan Justen <jordan.l.justen@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/OvmfPkg.dec        | 6 ++++++
 OvmfPkg/OvmfPkgIa32.dsc    | 4 ++++
 OvmfPkg/OvmfPkgIa32X64.dsc | 4 ++++
 OvmfPkg/OvmfPkgX64.dsc     | 4 ++++
 4 files changed, 18 insertions(+)

diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
index 5627be0bab0a..3c39a3b79894 100644
--- a/OvmfPkg/OvmfPkg.dec
+++ b/OvmfPkg/OvmfPkg.dec
@@ -148,3 +148,9 @@ [PcdsFeatureFlag]
   #  runtime OS from tampering with firmware structures (special memory ranges
   #  used by OVMF, the varstore pflash chip, LockBox etc).
   gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire|FALSE|BOOLEAN|0x1e
+
+  ## This feature flag reports whether in-memory (that is, non-flash) variable
+  #  emulation is enabled. Note that with PcdSmmSmramRequire set to TRUE, this
+  #  setting is irrelevant, as SMM/SMRAM support always requires flash
+  #  variables.
+  gUefiOvmfPkgTokenSpaceGuid.PcdMemVarstoreEmuEnable|TRUE|BOOLEAN|3
diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
index 8e88744087e0..c3d7d146ecb7 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -39,6 +39,7 @@ [Defines]
   DEFINE HTTP_BOOT_ENABLE        = FALSE
   DEFINE SMM_REQUIRE             = FALSE
   DEFINE TLS_ENABLE              = FALSE
+  DEFINE MEM_VARSTORE_EMU_ENABLE = TRUE
 
 [BuildOptions]
   GCC:*_UNIXGCC_*_CC_FLAGS             = -DMDEPKG_NDEBUG
@@ -385,6 +386,9 @@ [PcdsFeatureFlag]
   gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire|TRUE
   gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmEnableBspElection|FALSE
 !endif
+!if $(MEM_VARSTORE_EMU_ENABLE) == FALSE
+  gUefiOvmfPkgTokenSpaceGuid.PcdMemVarstoreEmuEnable|FALSE
+!endif
 
 [PcdsFixedAtBuild]
   gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeMemorySize|1
diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index 18348d8e9c58..55a6c66e986d 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -39,6 +39,7 @@ [Defines]
   DEFINE HTTP_BOOT_ENABLE        = FALSE
   DEFINE SMM_REQUIRE             = FALSE
   DEFINE TLS_ENABLE              = FALSE
+  DEFINE MEM_VARSTORE_EMU_ENABLE = TRUE
 
 [BuildOptions]
   GCC:*_UNIXGCC_*_CC_FLAGS             = -DMDEPKG_NDEBUG
@@ -390,6 +391,9 @@ [PcdsFeatureFlag]
   gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire|TRUE
   gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmEnableBspElection|FALSE
 !endif
+!if $(MEM_VARSTORE_EMU_ENABLE) == FALSE
+  gUefiOvmfPkgTokenSpaceGuid.PcdMemVarstoreEmuEnable|FALSE
+!endif
 
 [PcdsFixedAtBuild]
   gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeMemorySize|1
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index 76dd72a0d0d0..400c96ee4b0f 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -39,6 +39,7 @@ [Defines]
   DEFINE HTTP_BOOT_ENABLE        = FALSE
   DEFINE SMM_REQUIRE             = FALSE
   DEFINE TLS_ENABLE              = FALSE
+  DEFINE MEM_VARSTORE_EMU_ENABLE = TRUE
 
 [BuildOptions]
   GCC:*_UNIXGCC_*_CC_FLAGS             = -DMDEPKG_NDEBUG
@@ -390,6 +391,9 @@ [PcdsFeatureFlag]
   gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire|TRUE
   gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmEnableBspElection|FALSE
 !endif
+!if $(MEM_VARSTORE_EMU_ENABLE) == FALSE
+  gUefiOvmfPkgTokenSpaceGuid.PcdMemVarstoreEmuEnable|FALSE
+!endif
 
 [PcdsFixedAtBuild]
   gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeMemorySize|1
-- 
2.9.3

[PATCH 06/14] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: check PcdMemVarstoreEmuEnable

[Laszlo Ersek]

This patch parallels commit b963ec494c48 ("OvmfPkg:
QemuFlashFvbServicesRuntimeDxe: adhere to -D SMM_REQUIRE", 2015-11-30) in
that if QemuFlashDetected() fails -- because flash is not found --, not
only SMM_REQUIRE=TRUE is a wrong build to execute, but
MEM_VARSTORE_EMU_ENABLE=FALSE as well.

Cc: Jordan Justen <jordan.l.justen@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf | 1 +
 OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesSmm.inf        | 1 +
 OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c               | 1 +
 3 files changed, 3 insertions(+)

diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf
index c0dda75bf75f..cf30d5c8f021 100644
--- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf
+++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf
@@ -87,6 +87,7 @@ [Pcd]
 
 [FeaturePcd]
   gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire
+  gUefiOvmfPkgTokenSpaceGuid.PcdMemVarstoreEmuEnable
 
 [Depex]
   TRUE
diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesSmm.inf b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesSmm.inf
index ba2d3679a46d..d5aa393dbc0b 100644
--- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesSmm.inf
+++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesSmm.inf
@@ -86,6 +86,7 @@ [Pcd]
 
 [FeaturePcd]
   gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire
+  gUefiOvmfPkgTokenSpaceGuid.PcdMemVarstoreEmuEnable
 
 [Depex]
   TRUE
diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c
index 5677b5ee119c..60bc7a84bd6b 100644
--- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c
+++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c
@@ -246,6 +246,7 @@ QemuFlashInitialize (
 
   if (!QemuFlashDetected ()) {
     ASSERT (!FeaturePcdGet (PcdSmmSmramRequire));
+    ASSERT (FeaturePcdGet (PcdMemVarstoreEmuEnable));
     return EFI_WRITE_PROTECTED;
   }
 
-- 
2.9.3

[PATCH 07/14] OvmfPkg: conditionally disable reserved memory varstore emulation at build

[Laszlo Ersek]

(All of the below is only relevant for SMM_REQUIRE=FALSE.)

For the emulated variable store, PlatformPei allocates reserved memory (as
early as possible, so that the address remains the same during reboot),
and PcdEmuVariableNvStoreReserved carries the address to
EmuVariableFvbRuntimeDxe.

In addition, QemuFlashFvbServicesRuntimeDxe is always launched before
EmuVariableFvbRuntimeDxe, so that if flash variables are available,
QemuFlashFvbServicesRuntimeDxe can set PcdFlashNvStorageVariableBase64
first, and EmuVariableFvbRuntimeDxe can exit early. This ordering is
currently enforced by adding QemuFlashFvbServicesRuntimeDxe to the APRIORI
DXE file.

All of this is unnecessary when MEM_VARSTORE_EMU_ENABLE is set to FALSE.
In such a build,

- (almost) remove the dynamic default for PcdEmuVariableNvStoreReserved
  (we can't really do this because the PcdSet64() in
  ReserveEmuVariableNvStore() wouldn't compile),

- prevent the reserved memory allocation and PCD setting in PlatformPei,

- exclude EmuVariableFvbRuntimeDxe,

- and drop QemuFlashFvbServicesRuntimeDxe from the APRIORI DXE file (since
  it doesn't have to beat EmuVariableFvbRuntimeDxe in setting
  PcdFlashNvStorageVariableBase64 any longer).

Cc: Jordan Justen <jordan.l.justen@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/OvmfPkgIa32.dsc             | 4 +++-
 OvmfPkg/OvmfPkgIa32X64.dsc          | 4 +++-
 OvmfPkg/OvmfPkgX64.dsc              | 4 +++-
 OvmfPkg/OvmfPkgIa32.fdf             | 4 +++-
 OvmfPkg/OvmfPkgIa32X64.fdf          | 4 +++-
 OvmfPkg/OvmfPkgX64.fdf              | 4 +++-
 OvmfPkg/PlatformPei/PlatformPei.inf | 1 +
 OvmfPkg/PlatformPei/Platform.c      | 3 ++-
 8 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
index c3d7d146ecb7..416868ab51fb 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -474,7 +474,7 @@ [PcdsFixedAtBuild]
 
 [PcdsDynamicDefault]
   # only set when
-  #   ($(SMM_REQUIRE) == FALSE)
+  #   (($(SMM_REQUIRE) == FALSE) && ($(MEM_VARSTORE_EMU_ENABLE) == TRUE))
   gEfiMdeModulePkgTokenSpaceGuid.PcdEmuVariableNvStoreReserved|0
 
   gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64|0
@@ -840,10 +840,12 @@ [Components]
   # Variable driver stack (non-SMM)
   #
   OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf
+!if $(MEM_VARSTORE_EMU_ENABLE) == TRUE
   OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.inf {
     <LibraryClasses>
       PlatformFvbLib|OvmfPkg/Library/EmuVariableFvbLib/EmuVariableFvbLib.inf
   }
+!endif
   MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteDxe.inf
   MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf {
     <LibraryClasses>
diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index 55a6c66e986d..ca661fb09410 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -480,7 +480,7 @@ [PcdsFixedAtBuild.X64]
 
 [PcdsDynamicDefault]
   # only set when
-  #   ($(SMM_REQUIRE) == FALSE)
+  #   (($(SMM_REQUIRE) == FALSE) && ($(MEM_VARSTORE_EMU_ENABLE) == TRUE))
   gEfiMdeModulePkgTokenSpaceGuid.PcdEmuVariableNvStoreReserved|0
 
   gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64|0
@@ -849,10 +849,12 @@ [Components.X64]
   # Variable driver stack (non-SMM)
   #
   OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf
+!if $(MEM_VARSTORE_EMU_ENABLE) == TRUE
   OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.inf {
     <LibraryClasses>
       PlatformFvbLib|OvmfPkg/Library/EmuVariableFvbLib/EmuVariableFvbLib.inf
   }
+!endif
   MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteDxe.inf
   MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf {
     <LibraryClasses>
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index 400c96ee4b0f..0205ca987aee 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -479,7 +479,7 @@ [PcdsFixedAtBuild]
 
 [PcdsDynamicDefault]
   # only set when
-  #   ($(SMM_REQUIRE) == FALSE)
+  #   (($(SMM_REQUIRE) == FALSE) && ($(MEM_VARSTORE_EMU_ENABLE) == TRUE))
   gEfiMdeModulePkgTokenSpaceGuid.PcdEmuVariableNvStoreReserved|0
 
   gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64|0
@@ -847,10 +847,12 @@ [Components]
   # Variable driver stack (non-SMM)
   #
   OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf
+!if $(MEM_VARSTORE_EMU_ENABLE) == TRUE
   OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.inf {
     <LibraryClasses>
       PlatformFvbLib|OvmfPkg/Library/EmuVariableFvbLib/EmuVariableFvbLib.inf
   }
+!endif
   MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteDxe.inf
   MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf {
     <LibraryClasses>
diff --git a/OvmfPkg/OvmfPkgIa32.fdf b/OvmfPkg/OvmfPkgIa32.fdf
index 09c165882c3f..ce6641f534d0 100644
--- a/OvmfPkg/OvmfPkgIa32.fdf
+++ b/OvmfPkg/OvmfPkgIa32.fdf
@@ -190,7 +190,7 @@ [FV.DXEFV]
 APRIORI DXE {
   INF  MdeModulePkg/Universal/DevicePathDxe/DevicePathDxe.inf
   INF  MdeModulePkg/Universal/PCD/Dxe/Pcd.inf
-!if $(SMM_REQUIRE) == FALSE
+!if ($(SMM_REQUIRE) == FALSE) && ($(MEM_VARSTORE_EMU_ENABLE) == TRUE)
   INF  OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf
 !endif
 }
@@ -376,7 +376,9 @@ [FV.DXEFV]
 # Variable driver stack (non-SMM)
 #
 INF  OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf
+!if $(MEM_VARSTORE_EMU_ENABLE) == TRUE
 INF  OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.inf
+!endif
 INF  MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteDxe.inf
 INF  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
 !endif
diff --git a/OvmfPkg/OvmfPkgIa32X64.fdf b/OvmfPkg/OvmfPkgIa32X64.fdf
index 5233314139bc..c28a3080bee7 100644
--- a/OvmfPkg/OvmfPkgIa32X64.fdf
+++ b/OvmfPkg/OvmfPkgIa32X64.fdf
@@ -190,7 +190,7 @@ [FV.DXEFV]
 APRIORI DXE {
   INF  MdeModulePkg/Universal/DevicePathDxe/DevicePathDxe.inf
   INF  MdeModulePkg/Universal/PCD/Dxe/Pcd.inf
-!if $(SMM_REQUIRE) == FALSE
+!if ($(SMM_REQUIRE) == FALSE) && ($(MEM_VARSTORE_EMU_ENABLE) == TRUE)
   INF  OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf
 !endif
 }
@@ -376,7 +376,9 @@ [FV.DXEFV]
 # Variable driver stack (non-SMM)
 #
 INF  OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf
+!if $(MEM_VARSTORE_EMU_ENABLE) == TRUE
 INF  OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.inf
+!endif
 INF  MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteDxe.inf
 INF  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
 !endif
diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf
index 36150101e784..00c539ca5e4a 100644
--- a/OvmfPkg/OvmfPkgX64.fdf
+++ b/OvmfPkg/OvmfPkgX64.fdf
@@ -190,7 +190,7 @@ [FV.DXEFV]
 APRIORI DXE {
   INF  MdeModulePkg/Universal/DevicePathDxe/DevicePathDxe.inf
   INF  MdeModulePkg/Universal/PCD/Dxe/Pcd.inf
-!if $(SMM_REQUIRE) == FALSE
+!if ($(SMM_REQUIRE) == FALSE) && ($(MEM_VARSTORE_EMU_ENABLE) == TRUE)
   INF  OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf
 !endif
 }
@@ -376,7 +376,9 @@ [FV.DXEFV]
 # Variable driver stack (non-SMM)
 #
 INF  OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf
+!if $(MEM_VARSTORE_EMU_ENABLE) == TRUE
 INF  OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.inf
+!endif
 INF  MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteDxe.inf
 INF  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
 !endif
diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf b/OvmfPkg/PlatformPei/PlatformPei.inf
index a1e12c1fc7e2..9c1ffa9815c1 100644
--- a/OvmfPkg/PlatformPei/PlatformPei.inf
+++ b/OvmfPkg/PlatformPei/PlatformPei.inf
@@ -103,6 +103,7 @@ [FixedPcd]
 
 [FeaturePcd]
   gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire
+  gUefiOvmfPkgTokenSpaceGuid.PcdMemVarstoreEmuEnable
 
 [Ppis]
   gEfiPeiMasterBootModePpiGuid
diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
index 041eb9d189b4..ec449b422eda 100644
--- a/OvmfPkg/PlatformPei/Platform.c
+++ b/OvmfPkg/PlatformPei/Platform.c
@@ -661,7 +661,8 @@ InitializePlatform (
   mHostBridgeDevId = PciRead16 (OVMF_HOSTBRIDGE_DID);
 
   if (mBootMode != BOOT_ON_S3_RESUME) {
-    if (!FeaturePcdGet (PcdSmmSmramRequire)) {
+    if (!FeaturePcdGet (PcdSmmSmramRequire) &&
+        FeaturePcdGet (PcdMemVarstoreEmuEnable)) {
       ReserveEmuVariableNvStore ();
     }
     PeiFvInitialization ();
-- 
2.9.3

[PATCH 08/14] OvmfPkg: resolve PcdLib for all PEIMs individually

[Laszlo Ersek]

Currently the default (module type independent) PcdLib resolution is to
BasePcdLibNull.inf, which is inherited by all PEIMs. In the next patch,
we'll flip the PEIM default resolution to PeiPcdLib.inf, but in order to
keep that patch both correct and simple to review, we should spell out the
Null resolution for those two PEIMs (ReportStatusCodeRouterPei and
StatusCodeHandlerPei) that are now the only ones that don't specify an
explicit resolution.

Cc: Jordan Justen <jordan.l.justen@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/OvmfPkgIa32.dsc    | 10 ++++++++--
 OvmfPkg/OvmfPkgIa32X64.dsc | 10 ++++++++--
 OvmfPkg/OvmfPkgX64.dsc     | 10 ++++++++--
 3 files changed, 24 insertions(+), 6 deletions(-)

diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
index 416868ab51fb..19932b874550 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -537,8 +537,14 @@ [Components]
     <LibraryClasses>
       PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
   }
-  MdeModulePkg/Universal/ReportStatusCodeRouter/Pei/ReportStatusCodeRouterPei.inf
-  MdeModulePkg/Universal/StatusCodeHandler/Pei/StatusCodeHandlerPei.inf
+  MdeModulePkg/Universal/ReportStatusCodeRouter/Pei/ReportStatusCodeRouterPei.inf {
+    <LibraryClasses>
+      PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
+  }
+  MdeModulePkg/Universal/StatusCodeHandler/Pei/StatusCodeHandlerPei.inf {
+    <LibraryClasses>
+      PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
+  }
   MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf {
     <LibraryClasses>
       PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf
diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index ca661fb09410..da34a2663f68 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -545,8 +545,14 @@ [Components.IA32]
     <LibraryClasses>
       PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
   }
-  MdeModulePkg/Universal/ReportStatusCodeRouter/Pei/ReportStatusCodeRouterPei.inf
-  MdeModulePkg/Universal/StatusCodeHandler/Pei/StatusCodeHandlerPei.inf
+  MdeModulePkg/Universal/ReportStatusCodeRouter/Pei/ReportStatusCodeRouterPei.inf {
+    <LibraryClasses>
+      PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
+  }
+  MdeModulePkg/Universal/StatusCodeHandler/Pei/StatusCodeHandlerPei.inf {
+    <LibraryClasses>
+      PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
+  }
   MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf {
     <LibraryClasses>
       PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index 0205ca987aee..a16555e12ec6 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -544,8 +544,14 @@ [Components]
     <LibraryClasses>
       PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
   }
-  MdeModulePkg/Universal/ReportStatusCodeRouter/Pei/ReportStatusCodeRouterPei.inf
-  MdeModulePkg/Universal/StatusCodeHandler/Pei/StatusCodeHandlerPei.inf
+  MdeModulePkg/Universal/ReportStatusCodeRouter/Pei/ReportStatusCodeRouterPei.inf {
+    <LibraryClasses>
+      PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
+  }
+  MdeModulePkg/Universal/StatusCodeHandler/Pei/StatusCodeHandlerPei.inf {
+    <LibraryClasses>
+      PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
+  }
   MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf {
     <LibraryClasses>
       PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf
-- 
2.9.3

[PATCH 09/14] OvmfPkg: resolve PcdLib for PEIMs to PeiPcdLib by default

[Laszlo Ersek]

In the previous patch we had to add two explicit Null resolutions, but
here we can remove five PeiPcdLib ones, after setting the default to it.

Cc: Jordan Justen <jordan.l.justen@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/OvmfPkgIa32.dsc    | 22 +++++---------------
 OvmfPkg/OvmfPkgIa32X64.dsc | 22 +++++---------------
 OvmfPkg/OvmfPkgX64.dsc     | 22 +++++---------------
 3 files changed, 15 insertions(+), 51 deletions(-)

diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
index 19932b874550..0d073e3341ba 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -238,6 +238,7 @@ [LibraryClasses.common.PEIM]
   CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLib.inf
   MpInitLib|UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf
   QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/PeiQemuFwCfgS3LibFwCfg.inf
+  PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf
 
 [LibraryClasses.common.DXE_CORE]
   HobLib|MdePkg/Library/DxeCoreHobLib/DxeCoreHobLib.inf
@@ -545,32 +546,19 @@ [Components]
     <LibraryClasses>
       PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
   }
-  MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf {
-    <LibraryClasses>
-      PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf
-  }
+  MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
 
-  OvmfPkg/PlatformPei/PlatformPei.inf {
-    <LibraryClasses>
-      PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf
-  }
+  OvmfPkg/PlatformPei/PlatformPei.inf
   UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf {
     <LibraryClasses>
-      PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf
 !if $(SMM_REQUIRE) == TRUE
       LockBoxLib|MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxPeiLib.inf
 !endif
   }
 !if $(SMM_REQUIRE) == TRUE
-  OvmfPkg/SmmAccess/SmmAccessPei.inf {
-    <LibraryClasses>
-      PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf
-  }
+  OvmfPkg/SmmAccess/SmmAccessPei.inf
 !endif
-  UefiCpuPkg/CpuMpPei/CpuMpPei.inf {
-    <LibraryClasses>
-      PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf
-  }
+  UefiCpuPkg/CpuMpPei/CpuMpPei.inf
 
   #
   # DXE Phase modules
diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index da34a2663f68..20810acbe030 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -243,6 +243,7 @@ [LibraryClasses.common.PEIM]
   CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLib.inf
   MpInitLib|UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf
   QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/PeiQemuFwCfgS3LibFwCfg.inf
+  PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf
 
 [LibraryClasses.common.DXE_CORE]
   HobLib|MdePkg/Library/DxeCoreHobLib/DxeCoreHobLib.inf
@@ -553,32 +554,19 @@ [Components.IA32]
     <LibraryClasses>
       PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
   }
-  MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf {
-    <LibraryClasses>
-      PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf
-  }
+  MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
 
-  OvmfPkg/PlatformPei/PlatformPei.inf {
-    <LibraryClasses>
-      PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf
-  }
+  OvmfPkg/PlatformPei/PlatformPei.inf
   UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf {
     <LibraryClasses>
-      PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf
 !if $(SMM_REQUIRE) == TRUE
       LockBoxLib|MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxPeiLib.inf
 !endif
   }
 !if $(SMM_REQUIRE) == TRUE
-  OvmfPkg/SmmAccess/SmmAccessPei.inf {
-    <LibraryClasses>
-      PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf
-  }
+  OvmfPkg/SmmAccess/SmmAccessPei.inf
 !endif
-  UefiCpuPkg/CpuMpPei/CpuMpPei.inf {
-    <LibraryClasses>
-      PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf
-  }
+  UefiCpuPkg/CpuMpPei/CpuMpPei.inf
 
 [Components.X64]
   #
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index a16555e12ec6..844581d2c417 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -243,6 +243,7 @@ [LibraryClasses.common.PEIM]
   CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLib.inf
   MpInitLib|UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf
   QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/PeiQemuFwCfgS3LibFwCfg.inf
+  PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf
 
 [LibraryClasses.common.DXE_CORE]
   HobLib|MdePkg/Library/DxeCoreHobLib/DxeCoreHobLib.inf
@@ -552,32 +553,19 @@ [Components]
     <LibraryClasses>
       PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
   }
-  MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf {
-    <LibraryClasses>
-      PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf
-  }
+  MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
 
-  OvmfPkg/PlatformPei/PlatformPei.inf {
-    <LibraryClasses>
-      PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf
-  }
+  OvmfPkg/PlatformPei/PlatformPei.inf
   UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf {
     <LibraryClasses>
-      PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf
 !if $(SMM_REQUIRE) == TRUE
       LockBoxLib|MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxPeiLib.inf
 !endif
   }
 !if $(SMM_REQUIRE) == TRUE
-  OvmfPkg/SmmAccess/SmmAccessPei.inf {
-    <LibraryClasses>
-      PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf
-  }
+  OvmfPkg/SmmAccess/SmmAccessPei.inf
 !endif
-  UefiCpuPkg/CpuMpPei/CpuMpPei.inf {
-    <LibraryClasses>
-      PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf
-  }
+  UefiCpuPkg/CpuMpPei/CpuMpPei.inf
 
   #
   # DXE Phase modules
-- 
2.9.3

[PATCH 10/14] OvmfPkg: introduce FlashNvStorageAddressLib

[Laszlo Ersek]

An OVMF binary built with

  -D SMM_REQUIRE

or with

  -D MEM_VARSTORE_EMU_ENABLE=FALSE

requires flash to be present at the expected (build-time determined)
location; falling back to the emulated varstore is not possible.

In such builds, we can replace the settings of the
- varstore,
- FTW working block,
- and FTW spare area
address PCDs in QemuFlashFvbServicesRuntimeDxe with identical settings in
a new plug-in (NULL class) library, to be linked into variable-related
PEIMs.

This will enable such builds to access variables during the PEI phase,
without dynamic flash detection and without ordering constraints against
other PEIMs.

Cc: Jordan Justen <jordan.l.justen@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/Library/FlashNvStorageAddressLib/FlashNvStorageAddressLib.inf | 48 ++++++++++++++++++
 OvmfPkg/Library/FlashNvStorageAddressLib/FlashNvStorageAddressLib.c   | 53 ++++++++++++++++++++
 2 files changed, 101 insertions(+)

diff --git a/OvmfPkg/Library/FlashNvStorageAddressLib/FlashNvStorageAddressLib.inf b/OvmfPkg/Library/FlashNvStorageAddressLib/FlashNvStorageAddressLib.inf
new file mode 100644
index 000000000000..f79194f80de9
--- /dev/null
+++ b/OvmfPkg/Library/FlashNvStorageAddressLib/FlashNvStorageAddressLib.inf
@@ -0,0 +1,48 @@
+## @file
+#
+# A hook-in library for variable-related PEIMs, in order to set
+# - gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64,
+# - gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase,
+# - gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase,
+# from their gUefiOvmfPkgTokenSpaceGuid counterparts, just before those PEIMs
+# consume them.
+#
+# Copyright (C) 2017, Red Hat, Inc.
+#
+# This program and the accompanying materials are licensed and made available
+# under the terms and conditions of the BSD License which accompanies this
+# distribution. The full text of the license may be found at
+# http://opensource.org/licenses/bsd-license.php
+#
+# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, WITHOUT
+# WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+#
+##
+
+[Defines]
+  INF_VERSION                    = 1.25
+  BASE_NAME                      = FlashNvStorageAddressLib
+  FILE_GUID                      = 5FF5A9F9-D01E-49EC-9A17-1682FC85122F
+  MODULE_TYPE                    = BASE
+  VERSION_STRING                 = 1.0
+  LIBRARY_CLASS                  = FlashNvStorageAddressLib|PEIM
+  CONSTRUCTOR                    = SetFlashNvStorageAddresses
+
+[Sources]
+  FlashNvStorageAddressLib.c
+
+[Packages]
+  MdePkg/MdePkg.dec
+  MdeModulePkg/MdeModulePkg.dec
+  OvmfPkg/OvmfPkg.dec
+
+[LibraryClasses]
+  PcdLib
+
+[Pcd]
+  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase
+  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase
+  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64
+  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageFtwSpareBase
+  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageFtwWorkingBase
+  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageVariableBase
diff --git a/OvmfPkg/Library/FlashNvStorageAddressLib/FlashNvStorageAddressLib.c b/OvmfPkg/Library/FlashNvStorageAddressLib/FlashNvStorageAddressLib.c
new file mode 100644
index 000000000000..dc1280cc23f1
--- /dev/null
+++ b/OvmfPkg/Library/FlashNvStorageAddressLib/FlashNvStorageAddressLib.c
@@ -0,0 +1,53 @@
+/** @file
+
+  A hook-in library for variable-related PEIMs, in order to set
+  - gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64,
+  - gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase,
+  - gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase,
+  from their gUefiOvmfPkgTokenSpaceGuid counterparts, just before those PEIMs
+  consume them.
+
+  Copyright (C) 2017, Red Hat, Inc.
+
+  This program and the accompanying materials are licensed and made available
+  under the terms and conditions of the BSD License which accompanies this
+  distribution. The full text of the license may be found at
+  http://opensource.org/licenses/bsd-license.php
+
+  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, WITHOUT
+  WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+
+**/
+
+#include <Library/PcdLib.h>
+
+RETURN_STATUS
+EFIAPI
+SetFlashNvStorageAddresses (
+  VOID
+  )
+{
+  RETURN_STATUS PcdStatus;
+
+  PcdStatus = PcdSet64S (
+                PcdFlashNvStorageVariableBase64,
+                PcdGet32 (PcdOvmfFlashNvStorageVariableBase)
+                );
+  if (RETURN_ERROR (PcdStatus)) {
+    return PcdStatus;
+  }
+
+  PcdStatus = PcdSet32S (
+                PcdFlashNvStorageFtwWorkingBase,
+                PcdGet32 (PcdOvmfFlashNvStorageFtwWorkingBase)
+                );
+  if (RETURN_ERROR (PcdStatus)) {
+    return PcdStatus;
+  }
+
+  PcdStatus = PcdSet32S (
+                PcdFlashNvStorageFtwSpareBase,
+                PcdGet32 (PcdOvmfFlashNvStorageFtwSpareBase)
+                );
+  return PcdStatus;
+}
-- 
2.9.3

[PATCH 11/14] OvmfPkg: include FaultTolerantWritePei and VariablePei

[Laszlo Ersek]

... but only for the

  -D SMM_REQUIRE

and

  -D MEM_VARSTORE_EMU_ENABLE=FALSE

builds, where

- FaultTolerantWritePei can immediately consume
  PcdFlashNvStorageFtwWorkingBase and PcdFlashNvStorageFtwSpareBase,

- and VariablePei can immediately consume PcdFlashNvStorageVariableBase64,

with the help of FlashNvStorageAddressLib, plus variables can actually be
read from some variable store (namely, the pflash chip).

FaultTolerantWritePei produces a GUID data HOB with
FAULT_TOLERANT_WRITE_LAST_WRITE_DATA as contents. It also installs a Null
PPI that carries, as GUID, the same gEdkiiFaultTolerantWriteGuid as the
GUID data 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.

Because of the above serialization via the Null PPI, it suffices to link
FlashNvStorageAddressLib into FaultTolerantWritePei only.

Cc: Jordan Justen <jordan.l.justen@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=386
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/OvmfPkgIa32.dsc    | 7 +++++++
 OvmfPkg/OvmfPkgIa32X64.dsc | 7 +++++++
 OvmfPkg/OvmfPkgX64.dsc     | 7 +++++++
 OvmfPkg/OvmfPkgIa32.fdf    | 4 ++++
 OvmfPkg/OvmfPkgIa32X64.fdf | 4 ++++
 OvmfPkg/OvmfPkgX64.fdf     | 4 ++++
 6 files changed, 33 insertions(+)

diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
index 0d073e3341ba..ff6c397b287b 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -559,6 +559,13 @@ [Components]
   OvmfPkg/SmmAccess/SmmAccessPei.inf
 !endif
   UefiCpuPkg/CpuMpPei/CpuMpPei.inf
+!if ($(SMM_REQUIRE) == TRUE) || ($(MEM_VARSTORE_EMU_ENABLE) == FALSE)
+  MdeModulePkg/Universal/FaultTolerantWritePei/FaultTolerantWritePei.inf {
+    <LibraryClasses>
+      NULL|OvmfPkg/Library/FlashNvStorageAddressLib/FlashNvStorageAddressLib.inf
+  }
+  MdeModulePkg/Universal/Variable/Pei/VariablePei.inf
+!endif
 
   #
   # DXE Phase modules
diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index 20810acbe030..42c366030a7c 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -567,6 +567,13 @@ [Components.IA32]
   OvmfPkg/SmmAccess/SmmAccessPei.inf
 !endif
   UefiCpuPkg/CpuMpPei/CpuMpPei.inf
+!if ($(SMM_REQUIRE) == TRUE) || ($(MEM_VARSTORE_EMU_ENABLE) == FALSE)
+  MdeModulePkg/Universal/FaultTolerantWritePei/FaultTolerantWritePei.inf {
+    <LibraryClasses>
+      NULL|OvmfPkg/Library/FlashNvStorageAddressLib/FlashNvStorageAddressLib.inf
+  }
+  MdeModulePkg/Universal/Variable/Pei/VariablePei.inf
+!endif
 
 [Components.X64]
   #
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index 844581d2c417..9bcb6ae4377b 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -566,6 +566,13 @@ [Components]
   OvmfPkg/SmmAccess/SmmAccessPei.inf
 !endif
   UefiCpuPkg/CpuMpPei/CpuMpPei.inf
+!if ($(SMM_REQUIRE) == TRUE) || ($(MEM_VARSTORE_EMU_ENABLE) == FALSE)
+  MdeModulePkg/Universal/FaultTolerantWritePei/FaultTolerantWritePei.inf {
+    <LibraryClasses>
+      NULL|OvmfPkg/Library/FlashNvStorageAddressLib/FlashNvStorageAddressLib.inf
+  }
+  MdeModulePkg/Universal/Variable/Pei/VariablePei.inf
+!endif
 
   #
   # DXE Phase modules
diff --git a/OvmfPkg/OvmfPkgIa32.fdf b/OvmfPkg/OvmfPkgIa32.fdf
index ce6641f534d0..1d00feabf50a 100644
--- a/OvmfPkg/OvmfPkgIa32.fdf
+++ b/OvmfPkg/OvmfPkgIa32.fdf
@@ -164,6 +164,10 @@ [FV.PEIFV]
 INF  OvmfPkg/SmmAccess/SmmAccessPei.inf
 !endif
 INF  UefiCpuPkg/CpuMpPei/CpuMpPei.inf
+!if ($(SMM_REQUIRE) == TRUE) || ($(MEM_VARSTORE_EMU_ENABLE) == FALSE)
+INF  MdeModulePkg/Universal/FaultTolerantWritePei/FaultTolerantWritePei.inf
+INF  MdeModulePkg/Universal/Variable/Pei/VariablePei.inf
+!endif
 
 ################################################################################
 
diff --git a/OvmfPkg/OvmfPkgIa32X64.fdf b/OvmfPkg/OvmfPkgIa32X64.fdf
index c28a3080bee7..33aff848e66e 100644
--- a/OvmfPkg/OvmfPkgIa32X64.fdf
+++ b/OvmfPkg/OvmfPkgIa32X64.fdf
@@ -164,6 +164,10 @@ [FV.PEIFV]
 INF  OvmfPkg/SmmAccess/SmmAccessPei.inf
 !endif
 INF  UefiCpuPkg/CpuMpPei/CpuMpPei.inf
+!if ($(SMM_REQUIRE) == TRUE) || ($(MEM_VARSTORE_EMU_ENABLE) == FALSE)
+INF  MdeModulePkg/Universal/FaultTolerantWritePei/FaultTolerantWritePei.inf
+INF  MdeModulePkg/Universal/Variable/Pei/VariablePei.inf
+!endif
 
 ################################################################################
 
diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf
index 00c539ca5e4a..259e4419480c 100644
--- a/OvmfPkg/OvmfPkgX64.fdf
+++ b/OvmfPkg/OvmfPkgX64.fdf
@@ -164,6 +164,10 @@ [FV.PEIFV]
 INF  OvmfPkg/SmmAccess/SmmAccessPei.inf
 !endif
 INF  UefiCpuPkg/CpuMpPei/CpuMpPei.inf
+!if ($(SMM_REQUIRE) == TRUE) || ($(MEM_VARSTORE_EMU_ENABLE) == FALSE)
+INF  MdeModulePkg/Universal/FaultTolerantWritePei/FaultTolerantWritePei.inf
+INF  MdeModulePkg/Universal/Variable/Pei/VariablePei.inf
+!endif
 
 ################################################################################
 
-- 
2.9.3

[PATCH 12/14] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: don't set flash PCDs if SMM or no-emu

[Laszlo Ersek]

... only check them. When OVMF is built with

  -D SMM_REQUIRE

or with

  -D MEM_VARSTORE_EMU_ENABLE=FALSE

then the PCDs are set during the PEI phase, as part of
FaultTolerantWritePei's startup (in FlashNvStorageAddressLib's
constructor).

Cc: Jordan Justen <jordan.l.justen@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c | 58 ++++++++++++++------
 1 file changed, 40 insertions(+), 18 deletions(-)

diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c
index ff27c1100c01..62419a8fdf08 100644
--- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c
+++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c
@@ -1093,24 +1093,46 @@ FvbInitialize (
 
   MarkMemoryRangeForRuntimeAccess (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);
+  if (!FeaturePcdGet (PcdSmmSmramRequire) &&
+      FeaturePcdGet (PcdMemVarstoreEmuEnable)) {
+    //
+    // This build is suitable for both flash and in-memory emulated variables,
+    // and we happen to have found flash. Set several PCD values to point to
+    // flash.
+    //
+    PcdStatus = PcdSet64S (
+                  PcdFlashNvStorageVariableBase64,
+                  PcdGet32 (PcdOvmfFlashNvStorageVariableBase)
+                  );
+    ASSERT_RETURN_ERROR (PcdStatus);
+    PcdStatus = PcdSet32S (
+                  PcdFlashNvStorageFtwWorkingBase,
+                  PcdGet32 (PcdOvmfFlashNvStorageFtwWorkingBase)
+                  );
+    ASSERT_RETURN_ERROR (PcdStatus);
+    PcdStatus = PcdSet32S (
+                  PcdFlashNvStorageFtwSpareBase,
+                  PcdGet32 (PcdOvmfFlashNvStorageFtwSpareBase)
+                  );
+    ASSERT_RETURN_ERROR (PcdStatus);
+  } else {
+    //
+    // This build is suitable for flash variables only. Double-check several
+    // PCDs that point to the flash.
+    //
+    ASSERT (
+      (PcdGet64 (PcdFlashNvStorageVariableBase64) ==
+       PcdGet32 (PcdOvmfFlashNvStorageVariableBase))
+      );
+    ASSERT (
+      (PcdGet32 (PcdFlashNvStorageFtwWorkingBase) ==
+       PcdGet32 (PcdOvmfFlashNvStorageFtwWorkingBase))
+      );
+    ASSERT (
+      (PcdGet32 (PcdFlashNvStorageFtwSpareBase) ==
+       PcdGet32 (PcdOvmfFlashNvStorageFtwSpareBase))
+      );
+  }
 
   FwhInstance = (EFI_FW_VOL_INSTANCE *)
     (
-- 
2.9.3

[PATCH 13/14] OvmfPkg/PlatformPei: remedy UEFI memory map fragmentation

[Laszlo Ersek]

The Memory Type Information HOB is used for sizing the allocation bins for
the various memory types. If the PEI phase does not produce the HOB, and
it includes the VariablePei driver, then the DXE IPL PEIM will itself
build the HOB, from the "MemoryTypeInformation" non-volatile variable.

(The HOB is consumed in the DxeLoadCore() function, and it is ignored if
the boot mode is BOOT_ON_S3_RESUME. Accordingly, we already don't build
the HOB in InitializePlatform() during S3 resume; MemMapInitialization()
isn't called.)

In the BDS phase, BmSetMemoryTypeInformationVariable() reads the variable
(if it exists) under all boot modes different from
BOOT_WITH_DEFAULT_SETTINGS, and (re-)sets the variable if it doesn't
exist, or the counts of the pages allocated during boot have changed,
relative to what the variable predicted.

In effect this creates a feedback loop between BDS and the next boot's
PEI, making sure the memory allocation bins are sized large enough in
advance. Ultimately, for BOOT_WITH_FULL_CONFIGURATION, as a special case
of the above, this measures the maximum boot memory requirement per UEFI
memory type, and over time decreases fragmentation in the UEFI memory map.

We continue creating our (constant) Memory Type Information HOB in
OvmfPkg/PlatformPei -- which prevents the above feedback loop -- except in
one case: when OVMF is built with SMM_REQUIRE=TRUE or
MEM_VARSTORE_EMU_ENABLE=FALSE (that is, when a flash-based varstore is
guaranteed), and the "MemoryTypeInformation" variable exists (that is,
when the virtual machine has been booted at least once). This lets the OS
installer see a somewhat fragmented memory map at first boot, but further
boots should witness defragmented maps. In practice the difference seems
to be 20-24 entries in the UEFI memory map.

In the longer term this should also serve as basis for S4 enablement. For
now, we keep the PcdResetOnMemoryTypeInformationChange|FALSE setting in
the OVMF DSC files, dating back to commit 7709cf48e432 ("DuetPkg, OvmfPkg,
UnixPkg: Remove unnecessary reset during boot", 2010-12-06).

Cc: Jordan Justen <jordan.l.justen@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/PlatformPei/PlatformPei.inf |   2 +
 OvmfPkg/PlatformPei/Platform.h      |   5 +
 OvmfPkg/PlatformPei/MemTypeInfo.c   | 151 ++++++++++++++++++++
 OvmfPkg/PlatformPei/Platform.c      |  23 +--
 4 files changed, 159 insertions(+), 22 deletions(-)

diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf b/OvmfPkg/PlatformPei/PlatformPei.inf
index 9c1ffa9815c1..21f3a5ea4909 100644
--- a/OvmfPkg/PlatformPei/PlatformPei.inf
+++ b/OvmfPkg/PlatformPei/PlatformPei.inf
@@ -33,6 +33,7 @@ [Sources]
   FeatureControl.c
   Fv.c
   MemDetect.c
+  MemTypeInfo.c
   Platform.c
   Xen.c
 
@@ -108,6 +109,7 @@ [FeaturePcd]
 [Ppis]
   gEfiPeiMasterBootModePpiGuid
   gEfiPeiMpServicesPpiGuid
+  gEfiPeiReadOnlyVariable2PpiGuid
 
 [Depex]
   TRUE
diff --git a/OvmfPkg/PlatformPei/Platform.h b/OvmfPkg/PlatformPei/Platform.h
index 18f42c3f0ea8..64a87da4a70a 100644
--- a/OvmfPkg/PlatformPei/Platform.h
+++ b/OvmfPkg/PlatformPei/Platform.h
@@ -74,6 +74,11 @@ PeiFvInitialization (
   );
 
 VOID
+MemTypeInfoInitialization (
+  VOID
+  );
+
+VOID
 InstallFeatureControlCallback (
   VOID
   );
diff --git a/OvmfPkg/PlatformPei/MemTypeInfo.c b/OvmfPkg/PlatformPei/MemTypeInfo.c
new file mode 100644
index 000000000000..46ed9aaf8f31
--- /dev/null
+++ b/OvmfPkg/PlatformPei/MemTypeInfo.c
@@ -0,0 +1,151 @@
+/**@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, Red Hat, Inc.
+
+  This program and the accompanying materials are licensed and made available
+  under the terms and conditions of the BSD License which accompanies this
+  distribution.  The full text of the license may be found at
+  http://opensource.org/licenses/bsd-license.php
+
+  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, WITHOUT
+  WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+**/
+
+#include <Guid/MemoryTypeInformation.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: %a\n", gEfiCallerBaseName, __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
+                                );
+  if (Status == EFI_BUFFER_TOO_SMALL) {
+    //
+    // The variable exists; the DXE IPL PEIM will build the HOB from it.
+    //
+    return EFI_SUCCESS;
+  }
+  //
+  // Install the default memory type information HOB.
+  //
+  BuildMemTypeInfoHob ();
+  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) &&
+      FeaturePcdGet (PcdMemVarstoreEmuEnable)) {
+    //
+    // 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
+      ));
+    //
+    // Install the default HOB as a last resort.
+    //
+    BuildMemTypeInfoHob ();
+  }
+}
diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
index ec449b422eda..11a7a05fb1d7 100644
--- a/OvmfPkg/PlatformPei/Platform.c
+++ b/OvmfPkg/PlatformPei/Platform.c
@@ -34,7 +34,6 @@
 #include <Library/QemuFwCfgLib.h>
 #include <Library/QemuFwCfgS3Lib.h>
 #include <Library/ResourcePublicationLib.h>
-#include <Guid/MemoryTypeInformation.h>
 #include <Ppi/MasterBootMode.h>
 #include <IndustryStandard/Pci22.h>
 #include <OvmfPlatforms.h>
@@ -42,18 +41,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,
@@ -166,15 +153,6 @@ MemMapInitialization (
   PciIoSize = 0x4000;
 
   //
-  // Create Memory Type Information HOB
-  //
-  BuildGuidDataHob (
-    &gEfiMemoryTypeInformationGuid,
-    mDefaultMemoryTypeInformation,
-    sizeof(mDefaultMemoryTypeInformation)
-    );
-
-  //
   // Video memory + Legacy BIOS region
   //
   AddIoMemoryRangeHob (0x0A0000, BASE_1MB);
@@ -666,6 +644,7 @@ InitializePlatform (
       ReserveEmuVariableNvStore ();
     }
     PeiFvInitialization ();
+    MemTypeInfoInitialization ();
     MemMapInitialization ();
     NoexecDxeInitialization ();
   }
-- 
2.9.3

[PATCH 14/14] OvmfPkg/README: document MEM_VARSTORE_EMU_ENABLE and memmap defrag

[Laszlo Ersek]

Cc: Jordan Justen <jordan.l.justen@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/README | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/OvmfPkg/README b/OvmfPkg/README
index 304e69fbe545..79b6eb6255c3 100644
--- a/OvmfPkg/README
+++ b/OvmfPkg/README
@@ -68,13 +68,21 @@ https://github.com/tianocore/tianocore.github.io/wiki/How%20to%20build%20OVMF
     * Run qemu with: -pflash path/to/OVMF.fd
     * Note that this option is required for running SecureBoot-enabled builds
       (-D SECURE_BOOT_ENABLE).
+    * When a commitment to this option (i.e., -pflash) can be made, it is
+      recommended to build OVMF with -D MEM_VARSTORE_EMU_ENABLE=FALSE. Said
+      build setting enables UEFI memory map defragmentation across boots of the
+      virtual machine.
   - Option 2: Use QEMU -bios parameter
     * Note that UEFI variables will be partially emulated, and non-volatile
       variables may lose their contents after a reboot
+    * The build option -D MEM_VARSTORE_EMU_ENABLE=FALSE is incompatible with
+      the QEMU -bios option.
     * Run qemu with: -bios path/to/OVMF.fd
   - Option 3: Use QEMU -L parameter
     * Note that UEFI variables will be partially emulated, and non-volatile
       variables may lose their contents after a reboot
+    * The build option -D MEM_VARSTORE_EMU_ENABLE=FALSE is incompatible with
+      the QEMU -L option.
     * Either copy, rename or symlink OVMF.fd => bios.bin
     * Use the QEMU -L parameter to specify the directory where the bios.bin
       file is located.
@@ -139,6 +147,8 @@ during boot (even in RELEASE builds). Both the naming of the flag (SMM_REQUIRE,
 instead of SMM_ENABLE), and this behavior are consistent with the goal
 described above: this is supposed to be a security feature, and fallbacks are
 not allowed. Similarly, a pflash-backed variable store is a requirement.
+(Accordingly, SMM_REQUIRE automatically enables all benefits of
+MEM_VARSTORE_EMU_ENABLE=FALSE).
 
 QEMU should be started with the options listed below (in addition to any other
 guest-specific flags). The command line should be gradually composed from the
-- 
2.9.3

Re: [PATCH 00/14] OvmfPkg: add the Variable PEIM, defragment the UEFI memmap

[Jordan Justen]

On 2017-03-14 16:32:32, Laszlo Ersek wrote:
> This series implements the ideas discussed in the thread
> 
>   [edk2] memory type information HOB / UEFI memmap defrag
>   https://lists.01.org/pipermail/edk2-devel/2017-February/007576.html
> 
> I'm seeing good results, with approx. 20-24 UEFI memmap entries removed.
> 
> The benefits of the patch set are automatically enabled for SMM_REQUIRE
> builds.
> 
> For non-SMM builds (which are the default), another build flag is being
> introduced, namely MEM_VARSTORE_EMU_ENABLE. This build flag controls the
> reserved memory / NvVars file based variable emulation. It remains
> enabled by default (keeping the current status quo). For benefiting from
> the Variable PEIM and the dependent UEFI memmap defragmentation,
> MEM_VARSTORE_EMU_ENABLE has to be *disabled*. There are two reasons for
> this:
> 
> - The PEI phase FTW and Variable modules need immediate (at-startup)
>   access to the variable store, but the reserved memory based emulation
>   depends on dynamic allocation, plus moving the dynamic pflash
>   detection to PEI is out of question.
> 
> - Even more importantly, reading actual variable contents from the
>   non-pflash varstore would require, in PEI, similar hackery that
>   currently happens in BDS -- that's not going to happen.

What prevents us from enabling the PEI variable stack running using
the memory based "emu fvb"?

> In fact one of the longer term goals with MEM_VARSTORE_EMU_ENABLE is to
> identify what we'll rip out once we finally decide to drop the reserved
> memory / NvVars file based emulation.

I think what we could consider ripping out is the disk save feature.
Since QEMU/KVM (at least in the past) seem to preserve RAM contents
across reboot, I think the memory based buffers should continue to be
a fallback for when users don't get pflash setup correctly.

That said, I'm not sure we should decide to rip out the disk save
feature just yet, unless you think it can simplify things greatly.

-Jordan

> A summary of build modes:
> 
> * default build: works as before, with the Variable PEIM and UEFI memmap
>   defragmentation remaining unavailable.
> 
> * -D SMM_REQUIRE: works as before, with the Variable PEIM and UEFI
>   memmap defragmentation enabled.
> 
>   As a reminder, this build is inherently incompatible with QEMU's
>   "-bios" and "-L" parameters, which will trigger the ASSERT() in
>   QemuFlashInitialize()
>   [OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c], added in commit
>   b963ec494c48 ("OvmfPkg: QemuFlashFvbServicesRuntimeDxe: adhere to -D
>   SMM_REQUIRE", 2015-11-30).
> 
> * -D MEM_VARSTORE_EMU_ENABLE=FALSE: eliminates the reserved memory /
>   NvVars based variable emulation, turns pflash into a hard requirement,
>   and enables the Variable PEIM and the UEFI memmap defragmentation.
> 
>   If such a build is executed with -bios or -L, then the FTW and
>   Variable PEIMs will temporarily see a well-formed, but empty varstore
>   (straight from "OvmfPkg/VarStore.fdf.inc"), and then
>   QemuFlashFvbServicesRuntimeDxe will trip a new ASSERT(), in patch #6,
>   that is similar to the one added in commit b963ec494c48 (see above).
> 
> Anatomy of the series:
> 
> * Patches 01 through 04 clean up some innocent warts I noticed while
>   working on the feature.
> 
> * Patches 05 through 07 introduce the new build option and its basic
>   effect, the disabling of reserved memory based variable emulation.
> 
> * Patches 08 through 12 include the FTW and Variable PEIMs.
> 
> * Patch 13 enables UEFI memmap defragmentation.
> 
> * Patch 14 updates the README file.
> 
> The variable PEIM was independently requested in
> <https://bugzilla.tianocore.org/show_bug.cgi?id=386> earlier, but
> without a good upstreamable use case, I disagreed with its inclusion.
> The memmap defragmentation is a good use case however.
> 
> Also, I didn't lie about "downstream pressure" in my previous patch set;
> I wrote this one at Sunday/Monday night (and now it's Tues/Wed night).
> So one could consider this personal diligence. :)
> 
> The series conforms to the multi-line function call syntax outlined in
> <https://bugzilla.tianocore.org/show_bug.cgi?id=425>.
> 
> Repo:   https://github.com/lersek/edk2.git
> Branch: memmap_defrag
> 
> CC: Jordan Justen <jordan.l.justen@intel.com>
> 
> Thanks
> Laszlo
> 
> Laszlo Ersek (14):
>   OvmfPkg/EmuVariableFvbRuntimeDxe: always format an auth varstore
>     header
>   OvmfPkg: remove gUefiOvmfPkgTokenSpaceGuid.PcdSecureBootEnable
>   OvmfPkg/PlatformPei: remove unused PcdVariableStoreSize dependency
>   OvmfPkg/PlatformPei: don't allocate reserved mem varstore if
>     SMM_REQUIRE
>   OvmfPkg: introduce PcdMemVarstoreEmuEnable feature flag
>   OvmfPkg/QemuFlashFvbServicesRuntimeDxe: check PcdMemVarstoreEmuEnable
>   OvmfPkg: conditionally disable reserved memory varstore emulation at
>     build
>   OvmfPkg: resolve PcdLib for all PEIMs individually
>   OvmfPkg: resolve PcdLib for PEIMs to PeiPcdLib by default
>   OvmfPkg: introduce FlashNvStorageAddressLib
>   OvmfPkg: include FaultTolerantWritePei and VariablePei
>   OvmfPkg/QemuFlashFvbServicesRuntimeDxe: don't set flash PCDs if SMM or
>     no-emu
>   OvmfPkg/PlatformPei: remedy UEFI memory map fragmentation
>   OvmfPkg/README: document MEM_VARSTORE_EMU_ENABLE and memmap defrag
> 
>  OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c                                |  79 +---------
>  OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.inf                              |   3 -
>  OvmfPkg/Library/FlashNvStorageAddressLib/FlashNvStorageAddressLib.c   |  53 +++++++
>  OvmfPkg/Library/FlashNvStorageAddressLib/FlashNvStorageAddressLib.inf |  48 +++++++
>  OvmfPkg/OvmfPkg.dec                                                   |   7 +-
>  OvmfPkg/OvmfPkgIa32.dsc                                               |  43 +++---
>  OvmfPkg/OvmfPkgIa32.fdf                                               |   8 +-
>  OvmfPkg/OvmfPkgIa32X64.dsc                                            |  43 +++---
>  OvmfPkg/OvmfPkgIa32X64.fdf                                            |   8 +-
>  OvmfPkg/OvmfPkgX64.dsc                                                |  43 +++---
>  OvmfPkg/OvmfPkgX64.fdf                                                |   8 +-
>  OvmfPkg/PlatformPei/MemTypeInfo.c                                     | 151 ++++++++++++++++++++
>  OvmfPkg/PlatformPei/Platform.c                                        |  28 +---
>  OvmfPkg/PlatformPei/Platform.h                                        |   5 +
>  OvmfPkg/PlatformPei/PlatformPei.inf                                   |   4 +-
>  OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf      |   1 +
>  OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesSmm.inf             |   1 +
>  OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c               |  58 +++++---
>  OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c                    |   1 +
>  OvmfPkg/README                                                        |  10 ++
>  20 files changed, 425 insertions(+), 177 deletions(-)
>  create mode 100644 OvmfPkg/Library/FlashNvStorageAddressLib/FlashNvStorageAddressLib.inf
>  create mode 100644 OvmfPkg/Library/FlashNvStorageAddressLib/FlashNvStorageAddressLib.c
>  create mode 100644 OvmfPkg/PlatformPei/MemTypeInfo.c
> 
> -- 
> 2.9.3
> 

Re: [PATCH 00/14] OvmfPkg: add the Variable PEIM, defragment the UEFI memmap

[Laszlo Ersek]

On 03/16/17 19:51, Jordan Justen wrote:
> On 2017-03-14 16:32:32, Laszlo Ersek wrote:
>> This series implements the ideas discussed in the thread
>>
>>   [edk2] memory type information HOB / UEFI memmap defrag
>>   https://lists.01.org/pipermail/edk2-devel/2017-February/007576.html
>>
>> I'm seeing good results, with approx. 20-24 UEFI memmap entries removed.
>>
>> The benefits of the patch set are automatically enabled for SMM_REQUIRE
>> builds.
>>
>> For non-SMM builds (which are the default), another build flag is being
>> introduced, namely MEM_VARSTORE_EMU_ENABLE. This build flag controls the
>> reserved memory / NvVars file based variable emulation. It remains
>> enabled by default (keeping the current status quo). For benefiting from
>> the Variable PEIM and the dependent UEFI memmap defragmentation,
>> MEM_VARSTORE_EMU_ENABLE has to be *disabled*. There are two reasons for
>> this:
>>
>> - The PEI phase FTW and Variable modules need immediate (at-startup)
>>   access to the variable store, but the reserved memory based emulation
>>   depends on dynamic allocation, plus moving the dynamic pflash
>>   detection to PEI is out of question.
>>
>> - Even more importantly, reading actual variable contents from the
>>   non-pflash varstore would require, in PEI, similar hackery that
>>   currently happens in BDS -- that's not going to happen.
> 
> What prevents us from enabling the PEI variable stack running using
> the memory based "emu fvb"?

(1) PlatformPei would have to dynamically detect whether the pflash
address range behaves as flash or ROM. Same as in the current runtime
DXE driver, QemuFlashFvbServicesRuntimeDxe. Code duplication or quite a
bit of code movement, for factoring it out and sharing it.

(2) If it behaves as ROM, the memory block has to be allocated (like
now). If we're not just rebooting within the VM, that block of memory is
empty. The FTW and variable PEIMs will look for headers in there, so it
must be formatted.

(3) The address of the dynamic allocation has to be propagated to the
FTW and variable PEIMs. This introduces an ordering constraint between
PlatformPei and the FTW & variable PEIMs that we'd have to satisfy somehow.

(4) After all of the above, no variables would be found in the allocated
/ formatted memory block, unless we were just rebooting within the VM.
(I assume we wouldn't want to reload \NvVars from FAT in PEI...)
Considering the current use case, all fresh VM startups would produce
the default memory type info HOB, and wouldn't remedy the memory map
fragmentation. And in the longer term, considering S4, when the VM goes
down for S4 / hibernation, the VM actually goes away (QEMU exits), and
S4 resume would likely not work with a different / fragmented memmap (or
it would be obscurely different if you warm-rebooted once before).

It is technically solvable, but the benefit is very little; it would
just perpetuate the current broken emulation.

>> In fact one of the longer term goals with MEM_VARSTORE_EMU_ENABLE is to
>> identify what we'll rip out once we finally decide to drop the reserved
>> memory / NvVars file based emulation.
> 
> I think what we could consider ripping out is the disk save feature.
> Since QEMU/KVM (at least in the past) seem to preserve RAM contents
> across reboot, I think the memory based buffers should continue to be
> a fallback for when users don't get pflash setup correctly.

(a) The memory preservation works across reboot, yes, but it is not
useful for the memory type information HOB, and consequently (in the
longer term) for S4. The memory type information HOB should carry
information (== implement a long-term maximum search) over several cold
boots (= different QEMU instances) of the same virtual machine.

At every normal boot, the emulated varstore will be empty (and
well-formed only because we'd have to do that manually too, see (2)
above), and any access to it within PEI will be useless.

(b) If users don't set up pflash correctly, the default build will
continue to work for them without any changes at all. This covers Xen as
well.

The strict pflash requirements are only live if users build OVMF with
either one of the following non-default switches:

  -D SMM_REQUIRE

or

  -D MEM_VARSTORE_EMU_ENABLE=FALSE

Both of these imply "I know what I'm doing".

I see no benefit in enabling the FTW & Variable PEIMs without a
persistent flash varstore. I do see it require a whole lot of work.

If someone wants the FTW & Variable PEIMs real hard under the above (IMO
useless) circumstances, they're welcome to generalize the code / make it
dynamic on top of this series.

> That said, I'm not sure we should decide to rip out the disk save
> feature just yet, unless you think it can simplify things greatly.

The disk save (\NvVars) feature is irrelevant for this series. That
feature is based off the "PcdOvmfFlashVariablesEnable" PCD, which is set
by QemuFlashFvbServicesRuntimeDxe, and consumed by Platform BDS.

When OVMF is built with either of the above build switches,
PcdOvmfFlashVariablesEnable is guaranteed to end up as TRUE, so the
\NvVars code is always (dynamically) disabled in those builds.

I don't want to prevent users from continuing to *use* the emulated
varstore in the default build. However, that emulation is obscurely
broken, and has been so for 7 years. QEMU 1.6 (with pflash) has been out
for the second half of that time. I certainly want to stop *developing*
features for the emulated variable store. (If someone does want to, on
top of this first-line enablement, I won't stand in their way, of course.)

Thanks
Laszlo

> 
> -Jordan
> 
>> A summary of build modes:
>>
>> * default build: works as before, with the Variable PEIM and UEFI memmap
>>   defragmentation remaining unavailable.
>>
>> * -D SMM_REQUIRE: works as before, with the Variable PEIM and UEFI
>>   memmap defragmentation enabled.
>>
>>   As a reminder, this build is inherently incompatible with QEMU's
>>   "-bios" and "-L" parameters, which will trigger the ASSERT() in
>>   QemuFlashInitialize()
>>   [OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c], added in commit
>>   b963ec494c48 ("OvmfPkg: QemuFlashFvbServicesRuntimeDxe: adhere to -D
>>   SMM_REQUIRE", 2015-11-30).
>>
>> * -D MEM_VARSTORE_EMU_ENABLE=FALSE: eliminates the reserved memory /
>>   NvVars based variable emulation, turns pflash into a hard requirement,
>>   and enables the Variable PEIM and the UEFI memmap defragmentation.
>>
>>   If such a build is executed with -bios or -L, then the FTW and
>>   Variable PEIMs will temporarily see a well-formed, but empty varstore
>>   (straight from "OvmfPkg/VarStore.fdf.inc"), and then
>>   QemuFlashFvbServicesRuntimeDxe will trip a new ASSERT(), in patch #6,
>>   that is similar to the one added in commit b963ec494c48 (see above).
>>
>> Anatomy of the series:
>>
>> * Patches 01 through 04 clean up some innocent warts I noticed while
>>   working on the feature.
>>
>> * Patches 05 through 07 introduce the new build option and its basic
>>   effect, the disabling of reserved memory based variable emulation.
>>
>> * Patches 08 through 12 include the FTW and Variable PEIMs.
>>
>> * Patch 13 enables UEFI memmap defragmentation.
>>
>> * Patch 14 updates the README file.
>>
>> The variable PEIM was independently requested in
>> <https://bugzilla.tianocore.org/show_bug.cgi?id=386> earlier, but
>> without a good upstreamable use case, I disagreed with its inclusion.
>> The memmap defragmentation is a good use case however.
>>
>> Also, I didn't lie about "downstream pressure" in my previous patch set;
>> I wrote this one at Sunday/Monday night (and now it's Tues/Wed night).
>> So one could consider this personal diligence. :)
>>
>> The series conforms to the multi-line function call syntax outlined in
>> <https://bugzilla.tianocore.org/show_bug.cgi?id=425>.
>>
>> Repo:   https://github.com/lersek/edk2.git
>> Branch: memmap_defrag
>>
>> CC: Jordan Justen <jordan.l.justen@intel.com>
>>
>> Thanks
>> Laszlo
>>
>> Laszlo Ersek (14):
>>   OvmfPkg/EmuVariableFvbRuntimeDxe: always format an auth varstore
>>     header
>>   OvmfPkg: remove gUefiOvmfPkgTokenSpaceGuid.PcdSecureBootEnable
>>   OvmfPkg/PlatformPei: remove unused PcdVariableStoreSize dependency
>>   OvmfPkg/PlatformPei: don't allocate reserved mem varstore if
>>     SMM_REQUIRE
>>   OvmfPkg: introduce PcdMemVarstoreEmuEnable feature flag
>>   OvmfPkg/QemuFlashFvbServicesRuntimeDxe: check PcdMemVarstoreEmuEnable
>>   OvmfPkg: conditionally disable reserved memory varstore emulation at
>>     build
>>   OvmfPkg: resolve PcdLib for all PEIMs individually
>>   OvmfPkg: resolve PcdLib for PEIMs to PeiPcdLib by default
>>   OvmfPkg: introduce FlashNvStorageAddressLib
>>   OvmfPkg: include FaultTolerantWritePei and VariablePei
>>   OvmfPkg/QemuFlashFvbServicesRuntimeDxe: don't set flash PCDs if SMM or
>>     no-emu
>>   OvmfPkg/PlatformPei: remedy UEFI memory map fragmentation
>>   OvmfPkg/README: document MEM_VARSTORE_EMU_ENABLE and memmap defrag
>>
>>  OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c                                |  79 +---------
>>  OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.inf                              |   3 -
>>  OvmfPkg/Library/FlashNvStorageAddressLib/FlashNvStorageAddressLib.c   |  53 +++++++
>>  OvmfPkg/Library/FlashNvStorageAddressLib/FlashNvStorageAddressLib.inf |  48 +++++++
>>  OvmfPkg/OvmfPkg.dec                                                   |   7 +-
>>  OvmfPkg/OvmfPkgIa32.dsc                                               |  43 +++---
>>  OvmfPkg/OvmfPkgIa32.fdf                                               |   8 +-
>>  OvmfPkg/OvmfPkgIa32X64.dsc                                            |  43 +++---
>>  OvmfPkg/OvmfPkgIa32X64.fdf                                            |   8 +-
>>  OvmfPkg/OvmfPkgX64.dsc                                                |  43 +++---
>>  OvmfPkg/OvmfPkgX64.fdf                                                |   8 +-
>>  OvmfPkg/PlatformPei/MemTypeInfo.c                                     | 151 ++++++++++++++++++++
>>  OvmfPkg/PlatformPei/Platform.c                                        |  28 +---
>>  OvmfPkg/PlatformPei/Platform.h                                        |   5 +
>>  OvmfPkg/PlatformPei/PlatformPei.inf                                   |   4 +-
>>  OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf      |   1 +
>>  OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesSmm.inf             |   1 +
>>  OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c               |  58 +++++---
>>  OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c                    |   1 +
>>  OvmfPkg/README                                                        |  10 ++
>>  20 files changed, 425 insertions(+), 177 deletions(-)
>>  create mode 100644 OvmfPkg/Library/FlashNvStorageAddressLib/FlashNvStorageAddressLib.inf
>>  create mode 100644 OvmfPkg/Library/FlashNvStorageAddressLib/FlashNvStorageAddressLib.c
>>  create mode 100644 OvmfPkg/PlatformPei/MemTypeInfo.c
>>
>> -- 
>> 2.9.3
>>

Re: [PATCH 00/14] OvmfPkg: add the Variable PEIM, defragment the UEFI memmap

[Laszlo Ersek]

Jordan,

On 03/16/17 22:22, Laszlo Ersek wrote:
> On 03/16/17 19:51, Jordan Justen wrote:
>> On 2017-03-14 16:32:32, Laszlo Ersek wrote:
>>> This series implements the ideas discussed in the thread
>>>
>>>   [edk2] memory type information HOB / UEFI memmap defrag
>>>   https://lists.01.org/pipermail/edk2-devel/2017-February/007576.html
>>>
>>> I'm seeing good results, with approx. 20-24 UEFI memmap entries removed.
>>>
>>> The benefits of the patch set are automatically enabled for SMM_REQUIRE
>>> builds.
>>>
>>> For non-SMM builds (which are the default), another build flag is being
>>> introduced, namely MEM_VARSTORE_EMU_ENABLE. This build flag controls the
>>> reserved memory / NvVars file based variable emulation. It remains
>>> enabled by default (keeping the current status quo). For benefiting from
>>> the Variable PEIM and the dependent UEFI memmap defragmentation,
>>> MEM_VARSTORE_EMU_ENABLE has to be *disabled*. There are two reasons for
>>> this:
>>>
>>> - The PEI phase FTW and Variable modules need immediate (at-startup)
>>>   access to the variable store, but the reserved memory based emulation
>>>   depends on dynamic allocation, plus moving the dynamic pflash
>>>   detection to PEI is out of question.
>>>
>>> - Even more importantly, reading actual variable contents from the
>>>   non-pflash varstore would require, in PEI, similar hackery that
>>>   currently happens in BDS -- that's not going to happen.
>>
>> What prevents us from enabling the PEI variable stack running using
>> the memory based "emu fvb"?
> 
> (1) PlatformPei would have to dynamically detect whether the pflash
> address range behaves as flash or ROM. Same as in the current runtime
> DXE driver, QemuFlashFvbServicesRuntimeDxe. Code duplication or quite a
> bit of code movement, for factoring it out and sharing it.
> 
> (2) If it behaves as ROM, the memory block has to be allocated (like
> now). If we're not just rebooting within the VM, that block of memory is
> empty. The FTW and variable PEIMs will look for headers in there, so it
> must be formatted.
> 
> (3) The address of the dynamic allocation has to be propagated to the
> FTW and variable PEIMs. This introduces an ordering constraint between
> PlatformPei and the FTW & variable PEIMs that we'd have to satisfy somehow.
> 
> (4) After all of the above, no variables would be found in the allocated
> / formatted memory block, unless we were just rebooting within the VM.
> (I assume we wouldn't want to reload \NvVars from FAT in PEI...)
> Considering the current use case, all fresh VM startups would produce
> the default memory type info HOB, and wouldn't remedy the memory map
> fragmentation. And in the longer term, considering S4, when the VM goes
> down for S4 / hibernation, the VM actually goes away (QEMU exits), and
> S4 resume would likely not work with a different / fragmented memmap (or
> it would be obscurely different if you warm-rebooted once before).
> 
> It is technically solvable, but the benefit is very little; it would
> just perpetuate the current broken emulation.
> 
>>> In fact one of the longer term goals with MEM_VARSTORE_EMU_ENABLE is to
>>> identify what we'll rip out once we finally decide to drop the reserved
>>> memory / NvVars file based emulation.
>>
>> I think what we could consider ripping out is the disk save feature.
>> Since QEMU/KVM (at least in the past) seem to preserve RAM contents
>> across reboot, I think the memory based buffers should continue to be
>> a fallback for when users don't get pflash setup correctly.
> 
> (a) The memory preservation works across reboot, yes, but it is not
> useful for the memory type information HOB, and consequently (in the
> longer term) for S4. The memory type information HOB should carry
> information (== implement a long-term maximum search) over several cold
> boots (= different QEMU instances) of the same virtual machine.
> 
> At every normal boot, the emulated varstore will be empty (and
> well-formed only because we'd have to do that manually too, see (2)
> above), and any access to it within PEI will be useless.
> 
> (b) If users don't set up pflash correctly, the default build will
> continue to work for them without any changes at all. This covers Xen as
> well.
> 
> The strict pflash requirements are only live if users build OVMF with
> either one of the following non-default switches:
> 
>   -D SMM_REQUIRE
> 
> or
> 
>   -D MEM_VARSTORE_EMU_ENABLE=FALSE
> 
> Both of these imply "I know what I'm doing".
> 
> I see no benefit in enabling the FTW & Variable PEIMs without a
> persistent flash varstore. I do see it require a whole lot of work.
> 
> If someone wants the FTW & Variable PEIMs real hard under the above (IMO
> useless) circumstances, they're welcome to generalize the code / make it
> dynamic on top of this series.
> 
>> That said, I'm not sure we should decide to rip out the disk save
>> feature just yet, unless you think it can simplify things greatly.
> 
> The disk save (\NvVars) feature is irrelevant for this series. That
> feature is based off the "PcdOvmfFlashVariablesEnable" PCD, which is set
> by QemuFlashFvbServicesRuntimeDxe, and consumed by Platform BDS.
> 
> When OVMF is built with either of the above build switches,
> PcdOvmfFlashVariablesEnable is guaranteed to end up as TRUE, so the
> \NvVars code is always (dynamically) disabled in those builds.
> 
> I don't want to prevent users from continuing to *use* the emulated
> varstore in the default build. However, that emulation is obscurely
> broken, and has been so for 7 years. QEMU 1.6 (with pflash) has been out
> for the second half of that time. I certainly want to stop *developing*
> features for the emulated variable store. (If someone does want to, on
> top of this first-line enablement, I won't stand in their way, of course.)

ping -- what do you think of my arguments above, please?

While this series is not urgent for me at the moment, flushing it (as
in, committing it) would certainly ease my load.

If you disagree with the above points, I'll have to postpone this work
indefinitely. (The downstream churn has arrived.)

Thanks,
Laszlo

>>> A summary of build modes:
>>>
>>> * default build: works as before, with the Variable PEIM and UEFI memmap
>>>   defragmentation remaining unavailable.
>>>
>>> * -D SMM_REQUIRE: works as before, with the Variable PEIM and UEFI
>>>   memmap defragmentation enabled.
>>>
>>>   As a reminder, this build is inherently incompatible with QEMU's
>>>   "-bios" and "-L" parameters, which will trigger the ASSERT() in
>>>   QemuFlashInitialize()
>>>   [OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c], added in commit
>>>   b963ec494c48 ("OvmfPkg: QemuFlashFvbServicesRuntimeDxe: adhere to -D
>>>   SMM_REQUIRE", 2015-11-30).
>>>
>>> * -D MEM_VARSTORE_EMU_ENABLE=FALSE: eliminates the reserved memory /
>>>   NvVars based variable emulation, turns pflash into a hard requirement,
>>>   and enables the Variable PEIM and the UEFI memmap defragmentation.
>>>
>>>   If such a build is executed with -bios or -L, then the FTW and
>>>   Variable PEIMs will temporarily see a well-formed, but empty varstore
>>>   (straight from "OvmfPkg/VarStore.fdf.inc"), and then
>>>   QemuFlashFvbServicesRuntimeDxe will trip a new ASSERT(), in patch #6,
>>>   that is similar to the one added in commit b963ec494c48 (see above).
>>>
>>> Anatomy of the series:
>>>
>>> * Patches 01 through 04 clean up some innocent warts I noticed while
>>>   working on the feature.
>>>
>>> * Patches 05 through 07 introduce the new build option and its basic
>>>   effect, the disabling of reserved memory based variable emulation.
>>>
>>> * Patches 08 through 12 include the FTW and Variable PEIMs.
>>>
>>> * Patch 13 enables UEFI memmap defragmentation.
>>>
>>> * Patch 14 updates the README file.
>>>
>>> The variable PEIM was independently requested in
>>> <https://bugzilla.tianocore.org/show_bug.cgi?id=386> earlier, but
>>> without a good upstreamable use case, I disagreed with its inclusion.
>>> The memmap defragmentation is a good use case however.
>>>
>>> Also, I didn't lie about "downstream pressure" in my previous patch set;
>>> I wrote this one at Sunday/Monday night (and now it's Tues/Wed night).
>>> So one could consider this personal diligence. :)
>>>
>>> The series conforms to the multi-line function call syntax outlined in
>>> <https://bugzilla.tianocore.org/show_bug.cgi?id=425>.
>>>
>>> Repo:   https://github.com/lersek/edk2.git
>>> Branch: memmap_defrag
>>>
>>> CC: Jordan Justen <jordan.l.justen@intel.com>
>>>
>>> Thanks
>>> Laszlo
>>>
>>> Laszlo Ersek (14):
>>>   OvmfPkg/EmuVariableFvbRuntimeDxe: always format an auth varstore
>>>     header
>>>   OvmfPkg: remove gUefiOvmfPkgTokenSpaceGuid.PcdSecureBootEnable
>>>   OvmfPkg/PlatformPei: remove unused PcdVariableStoreSize dependency
>>>   OvmfPkg/PlatformPei: don't allocate reserved mem varstore if
>>>     SMM_REQUIRE
>>>   OvmfPkg: introduce PcdMemVarstoreEmuEnable feature flag
>>>   OvmfPkg/QemuFlashFvbServicesRuntimeDxe: check PcdMemVarstoreEmuEnable
>>>   OvmfPkg: conditionally disable reserved memory varstore emulation at
>>>     build
>>>   OvmfPkg: resolve PcdLib for all PEIMs individually
>>>   OvmfPkg: resolve PcdLib for PEIMs to PeiPcdLib by default
>>>   OvmfPkg: introduce FlashNvStorageAddressLib
>>>   OvmfPkg: include FaultTolerantWritePei and VariablePei
>>>   OvmfPkg/QemuFlashFvbServicesRuntimeDxe: don't set flash PCDs if SMM or
>>>     no-emu
>>>   OvmfPkg/PlatformPei: remedy UEFI memory map fragmentation
>>>   OvmfPkg/README: document MEM_VARSTORE_EMU_ENABLE and memmap defrag
>>>
>>>  OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c                                |  79 +---------
>>>  OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.inf                              |   3 -
>>>  OvmfPkg/Library/FlashNvStorageAddressLib/FlashNvStorageAddressLib.c   |  53 +++++++
>>>  OvmfPkg/Library/FlashNvStorageAddressLib/FlashNvStorageAddressLib.inf |  48 +++++++
>>>  OvmfPkg/OvmfPkg.dec                                                   |   7 +-
>>>  OvmfPkg/OvmfPkgIa32.dsc                                               |  43 +++---
>>>  OvmfPkg/OvmfPkgIa32.fdf                                               |   8 +-
>>>  OvmfPkg/OvmfPkgIa32X64.dsc                                            |  43 +++---
>>>  OvmfPkg/OvmfPkgIa32X64.fdf                                            |   8 +-
>>>  OvmfPkg/OvmfPkgX64.dsc                                                |  43 +++---
>>>  OvmfPkg/OvmfPkgX64.fdf                                                |   8 +-
>>>  OvmfPkg/PlatformPei/MemTypeInfo.c                                     | 151 ++++++++++++++++++++
>>>  OvmfPkg/PlatformPei/Platform.c                                        |  28 +---
>>>  OvmfPkg/PlatformPei/Platform.h                                        |   5 +
>>>  OvmfPkg/PlatformPei/PlatformPei.inf                                   |   4 +-
>>>  OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf      |   1 +
>>>  OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesSmm.inf             |   1 +
>>>  OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c               |  58 +++++---
>>>  OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c                    |   1 +
>>>  OvmfPkg/README                                                        |  10 ++
>>>  20 files changed, 425 insertions(+), 177 deletions(-)
>>>  create mode 100644 OvmfPkg/Library/FlashNvStorageAddressLib/FlashNvStorageAddressLib.inf
>>>  create mode 100644 OvmfPkg/Library/FlashNvStorageAddressLib/FlashNvStorageAddressLib.c
>>>  create mode 100644 OvmfPkg/PlatformPei/MemTypeInfo.c
>>>
>>> -- 
>>> 2.9.3
>>>
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> 

Re: [PATCH 00/14] OvmfPkg: add the Variable PEIM, defragment the UEFI memmap

[Jordan Justen]

On 2017-03-22 09:58:24, Laszlo Ersek wrote:
> Jordan,
> 
> On 03/16/17 22:22, Laszlo Ersek wrote:
> > On 03/16/17 19:51, Jordan Justen wrote:
> >> On 2017-03-14 16:32:32, Laszlo Ersek wrote:
> >>> This series implements the ideas discussed in the thread
> >>>
> >>>   [edk2] memory type information HOB / UEFI memmap defrag
> >>>   https://lists.01.org/pipermail/edk2-devel/2017-February/007576.html
> >>>
> >>> I'm seeing good results, with approx. 20-24 UEFI memmap entries removed.
> >>>
> >>> The benefits of the patch set are automatically enabled for SMM_REQUIRE
> >>> builds.
> >>>
> >>> For non-SMM builds (which are the default), another build flag is being
> >>> introduced, namely MEM_VARSTORE_EMU_ENABLE. This build flag controls the
> >>> reserved memory / NvVars file based variable emulation. It remains
> >>> enabled by default (keeping the current status quo). For benefiting from
> >>> the Variable PEIM and the dependent UEFI memmap defragmentation,
> >>> MEM_VARSTORE_EMU_ENABLE has to be *disabled*. There are two reasons for
> >>> this:
> >>>
> >>> - The PEI phase FTW and Variable modules need immediate (at-startup)
> >>>   access to the variable store, but the reserved memory based emulation
> >>>   depends on dynamic allocation, plus moving the dynamic pflash
> >>>   detection to PEI is out of question.
> >>>
> >>> - Even more importantly, reading actual variable contents from the
> >>>   non-pflash varstore would require, in PEI, similar hackery that
> >>>   currently happens in BDS -- that's not going to happen.
> >>
> >> What prevents us from enabling the PEI variable stack running using
> >> the memory based "emu fvb"?
> > 
> > (1) PlatformPei would have to dynamically detect whether the pflash
> > address range behaves as flash or ROM. Same as in the current runtime
> > DXE driver, QemuFlashFvbServicesRuntimeDxe. Code duplication or quite a
> > bit of code movement, for factoring it out and sharing it.
> > 
> > (2) If it behaves as ROM, the memory block has to be allocated (like
> > now). If we're not just rebooting within the VM, that block of memory is
> > empty. The FTW and variable PEIMs will look for headers in there, so it
> > must be formatted.
> > 
> > (3) The address of the dynamic allocation has to be propagated to the
> > FTW and variable PEIMs. This introduces an ordering constraint between
> > PlatformPei and the FTW & variable PEIMs that we'd have to satisfy somehow.
> > 
> > (4) After all of the above, no variables would be found in the allocated
> > / formatted memory block, unless we were just rebooting within the VM.
> > (I assume we wouldn't want to reload \NvVars from FAT in PEI...)
> > Considering the current use case, all fresh VM startups would produce
> > the default memory type info HOB, and wouldn't remedy the memory map
> > fragmentation. And in the longer term, considering S4, when the VM goes
> > down for S4 / hibernation, the VM actually goes away (QEMU exits), and
> > S4 resume would likely not work with a different / fragmented memmap (or
> > it would be obscurely different if you warm-rebooted once before).
> > 
> > It is technically solvable, but the benefit is very little; it would
> > just perpetuate the current broken emulation.
> > 
> >>> In fact one of the longer term goals with MEM_VARSTORE_EMU_ENABLE is to
> >>> identify what we'll rip out once we finally decide to drop the reserved
> >>> memory / NvVars file based emulation.
> >>
> >> I think what we could consider ripping out is the disk save feature.
> >> Since QEMU/KVM (at least in the past) seem to preserve RAM contents
> >> across reboot, I think the memory based buffers should continue to be
> >> a fallback for when users don't get pflash setup correctly.
> > 
> > (a) The memory preservation works across reboot, yes, but it is not
> > useful for the memory type information HOB, and consequently (in the
> > longer term) for S4. The memory type information HOB should carry
> > information (== implement a long-term maximum search) over several cold
> > boots (= different QEMU instances) of the same virtual machine.
> > 
> > At every normal boot, the emulated varstore will be empty (and
> > well-formed only because we'd have to do that manually too, see (2)
> > above), and any access to it within PEI will be useless.
> > 
> > (b) If users don't set up pflash correctly, the default build will
> > continue to work for them without any changes at all. This covers Xen as
> > well.
> > 
> > The strict pflash requirements are only live if users build OVMF with
> > either one of the following non-default switches:
> > 
> >   -D SMM_REQUIRE
> > 
> > or
> > 
> >   -D MEM_VARSTORE_EMU_ENABLE=FALSE
> > 
> > Both of these imply "I know what I'm doing".
> > 
> > I see no benefit in enabling the FTW & Variable PEIMs without a
> > persistent flash varstore. I do see it require a whole lot of work.
> > 
> > If someone wants the FTW & Variable PEIMs real hard under the above (IMO
> > useless) circumstances, they're welcome to generalize the code / make it
> > dynamic on top of this series.
> > 
> >> That said, I'm not sure we should decide to rip out the disk save
> >> feature just yet, unless you think it can simplify things greatly.
> > 
> > The disk save (\NvVars) feature is irrelevant for this series. That
> > feature is based off the "PcdOvmfFlashVariablesEnable" PCD, which is set
> > by QemuFlashFvbServicesRuntimeDxe, and consumed by Platform BDS.
> > 
> > When OVMF is built with either of the above build switches,
> > PcdOvmfFlashVariablesEnable is guaranteed to end up as TRUE, so the
> > \NvVars code is always (dynamically) disabled in those builds.
> > 
> > I don't want to prevent users from continuing to *use* the emulated
> > varstore in the default build. However, that emulation is obscurely
> > broken, and has been so for 7 years. QEMU 1.6 (with pflash) has been out
> > for the second half of that time. I certainly want to stop *developing*
> > features for the emulated variable store. (If someone does want to, on
> > top of this first-line enablement, I won't stand in their way, of course.)
> 
> ping -- what do you think of my arguments above, please?

I guess I'd like to see what it would take to enable it for the EMU
FVB path. I hope I can find some time to look at it this weekend.

Regarding depending more on flash by default, is it correct that Xen
is the only significant thing preventing it? I mean, looking at it
from the qemu/kvm side of things, I would say we could discuss just
building that way by default at this point. I still think we would
want a failure path friendlier than a blind crash, which still leaves
some dependence on EMU FVB so we could boot far enough for a user
visible crash message.

-Jordan

> While this series is not urgent for me at the moment, flushing it (as
> in, committing it) would certainly ease my load.
> 
> If you disagree with the above points, I'll have to postpone this work
> indefinitely. (The downstream churn has arrived.)
> 
> Thanks,
> Laszlo
> 
> >>> A summary of build modes:
> >>>
> >>> * default build: works as before, with the Variable PEIM and UEFI memmap
> >>>   defragmentation remaining unavailable.
> >>>
> >>> * -D SMM_REQUIRE: works as before, with the Variable PEIM and UEFI
> >>>   memmap defragmentation enabled.
> >>>
> >>>   As a reminder, this build is inherently incompatible with QEMU's
> >>>   "-bios" and "-L" parameters, which will trigger the ASSERT() in
> >>>   QemuFlashInitialize()
> >>>   [OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c], added in commit
> >>>   b963ec494c48 ("OvmfPkg: QemuFlashFvbServicesRuntimeDxe: adhere to -D
> >>>   SMM_REQUIRE", 2015-11-30).
> >>>
> >>> * -D MEM_VARSTORE_EMU_ENABLE=FALSE: eliminates the reserved memory /
> >>>   NvVars based variable emulation, turns pflash into a hard requirement,
> >>>   and enables the Variable PEIM and the UEFI memmap defragmentation.
> >>>
> >>>   If such a build is executed with -bios or -L, then the FTW and
> >>>   Variable PEIMs will temporarily see a well-formed, but empty varstore
> >>>   (straight from "OvmfPkg/VarStore.fdf.inc"), and then
> >>>   QemuFlashFvbServicesRuntimeDxe will trip a new ASSERT(), in patch #6,
> >>>   that is similar to the one added in commit b963ec494c48 (see above).
> >>>
> >>> Anatomy of the series:
> >>>
> >>> * Patches 01 through 04 clean up some innocent warts I noticed while
> >>>   working on the feature.
> >>>
> >>> * Patches 05 through 07 introduce the new build option and its basic
> >>>   effect, the disabling of reserved memory based variable emulation.
> >>>
> >>> * Patches 08 through 12 include the FTW and Variable PEIMs.
> >>>
> >>> * Patch 13 enables UEFI memmap defragmentation.
> >>>
> >>> * Patch 14 updates the README file.
> >>>
> >>> The variable PEIM was independently requested in
> >>> <https://bugzilla.tianocore.org/show_bug.cgi?id=386> earlier, but
> >>> without a good upstreamable use case, I disagreed with its inclusion.
> >>> The memmap defragmentation is a good use case however.
> >>>
> >>> Also, I didn't lie about "downstream pressure" in my previous patch set;
> >>> I wrote this one at Sunday/Monday night (and now it's Tues/Wed night).
> >>> So one could consider this personal diligence. :)
> >>>
> >>> The series conforms to the multi-line function call syntax outlined in
> >>> <https://bugzilla.tianocore.org/show_bug.cgi?id=425>.
> >>>
> >>> Repo:   https://github.com/lersek/edk2.git
> >>> Branch: memmap_defrag
> >>>
> >>> CC: Jordan Justen <jordan.l.justen@intel.com>
> >>>
> >>> Thanks
> >>> Laszlo
> >>>
> >>> Laszlo Ersek (14):
> >>>   OvmfPkg/EmuVariableFvbRuntimeDxe: always format an auth varstore
> >>>     header
> >>>   OvmfPkg: remove gUefiOvmfPkgTokenSpaceGuid.PcdSecureBootEnable
> >>>   OvmfPkg/PlatformPei: remove unused PcdVariableStoreSize dependency
> >>>   OvmfPkg/PlatformPei: don't allocate reserved mem varstore if
> >>>     SMM_REQUIRE
> >>>   OvmfPkg: introduce PcdMemVarstoreEmuEnable feature flag
> >>>   OvmfPkg/QemuFlashFvbServicesRuntimeDxe: check PcdMemVarstoreEmuEnable
> >>>   OvmfPkg: conditionally disable reserved memory varstore emulation at
> >>>     build
> >>>   OvmfPkg: resolve PcdLib for all PEIMs individually
> >>>   OvmfPkg: resolve PcdLib for PEIMs to PeiPcdLib by default
> >>>   OvmfPkg: introduce FlashNvStorageAddressLib
> >>>   OvmfPkg: include FaultTolerantWritePei and VariablePei
> >>>   OvmfPkg/QemuFlashFvbServicesRuntimeDxe: don't set flash PCDs if SMM or
> >>>     no-emu
> >>>   OvmfPkg/PlatformPei: remedy UEFI memory map fragmentation
> >>>   OvmfPkg/README: document MEM_VARSTORE_EMU_ENABLE and memmap defrag
> >>>
> >>>  OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c                                |  79 +---------
> >>>  OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.inf                              |   3 -
> >>>  OvmfPkg/Library/FlashNvStorageAddressLib/FlashNvStorageAddressLib.c   |  53 +++++++
> >>>  OvmfPkg/Library/FlashNvStorageAddressLib/FlashNvStorageAddressLib.inf |  48 +++++++
> >>>  OvmfPkg/OvmfPkg.dec                                                   |   7 +-
> >>>  OvmfPkg/OvmfPkgIa32.dsc                                               |  43 +++---
> >>>  OvmfPkg/OvmfPkgIa32.fdf                                               |   8 +-
> >>>  OvmfPkg/OvmfPkgIa32X64.dsc                                            |  43 +++---
> >>>  OvmfPkg/OvmfPkgIa32X64.fdf                                            |   8 +-
> >>>  OvmfPkg/OvmfPkgX64.dsc                                                |  43 +++---
> >>>  OvmfPkg/OvmfPkgX64.fdf                                                |   8 +-
> >>>  OvmfPkg/PlatformPei/MemTypeInfo.c                                     | 151 ++++++++++++++++++++
> >>>  OvmfPkg/PlatformPei/Platform.c                                        |  28 +---
> >>>  OvmfPkg/PlatformPei/Platform.h                                        |   5 +
> >>>  OvmfPkg/PlatformPei/PlatformPei.inf                                   |   4 +-
> >>>  OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf      |   1 +
> >>>  OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesSmm.inf             |   1 +
> >>>  OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c               |  58 +++++---
> >>>  OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c                    |   1 +
> >>>  OvmfPkg/README                                                        |  10 ++
> >>>  20 files changed, 425 insertions(+), 177 deletions(-)
> >>>  create mode 100644 OvmfPkg/Library/FlashNvStorageAddressLib/FlashNvStorageAddressLib.inf
> >>>  create mode 100644 OvmfPkg/Library/FlashNvStorageAddressLib/FlashNvStorageAddressLib.c
> >>>  create mode 100644 OvmfPkg/PlatformPei/MemTypeInfo.c
> >>>
> >>> -- 
> >>> 2.9.3
> >>>
> > 
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel
> > 
> 

Re: [PATCH 00/14] OvmfPkg: add the Variable PEIM, defragment the UEFI memmap

[Laszlo Ersek]

On 03/24/17 08:38, Jordan Justen wrote:
> On 2017-03-22 09:58:24, Laszlo Ersek wrote:
>> Jordan,
>>
>> On 03/16/17 22:22, Laszlo Ersek wrote:
>>> On 03/16/17 19:51, Jordan Justen wrote:
>>>> On 2017-03-14 16:32:32, Laszlo Ersek wrote:
>>>>> This series implements the ideas discussed in the thread
>>>>>
>>>>>   [edk2] memory type information HOB / UEFI memmap defrag
>>>>>   https://lists.01.org/pipermail/edk2-devel/2017-February/007576.html
>>>>>
>>>>> I'm seeing good results, with approx. 20-24 UEFI memmap entries removed.
>>>>>
>>>>> The benefits of the patch set are automatically enabled for SMM_REQUIRE
>>>>> builds.
>>>>>
>>>>> For non-SMM builds (which are the default), another build flag is being
>>>>> introduced, namely MEM_VARSTORE_EMU_ENABLE. This build flag controls the
>>>>> reserved memory / NvVars file based variable emulation. It remains
>>>>> enabled by default (keeping the current status quo). For benefiting from
>>>>> the Variable PEIM and the dependent UEFI memmap defragmentation,
>>>>> MEM_VARSTORE_EMU_ENABLE has to be *disabled*. There are two reasons for
>>>>> this:
>>>>>
>>>>> - The PEI phase FTW and Variable modules need immediate (at-startup)
>>>>>   access to the variable store, but the reserved memory based emulation
>>>>>   depends on dynamic allocation, plus moving the dynamic pflash
>>>>>   detection to PEI is out of question.
>>>>>
>>>>> - Even more importantly, reading actual variable contents from the
>>>>>   non-pflash varstore would require, in PEI, similar hackery that
>>>>>   currently happens in BDS -- that's not going to happen.
>>>>
>>>> What prevents us from enabling the PEI variable stack running using
>>>> the memory based "emu fvb"?
>>>
>>> (1) PlatformPei would have to dynamically detect whether the pflash
>>> address range behaves as flash or ROM. Same as in the current runtime
>>> DXE driver, QemuFlashFvbServicesRuntimeDxe. Code duplication or quite a
>>> bit of code movement, for factoring it out and sharing it.
>>>
>>> (2) If it behaves as ROM, the memory block has to be allocated (like
>>> now). If we're not just rebooting within the VM, that block of memory is
>>> empty. The FTW and variable PEIMs will look for headers in there, so it
>>> must be formatted.
>>>
>>> (3) The address of the dynamic allocation has to be propagated to the
>>> FTW and variable PEIMs. This introduces an ordering constraint between
>>> PlatformPei and the FTW & variable PEIMs that we'd have to satisfy somehow.
>>>
>>> (4) After all of the above, no variables would be found in the allocated
>>> / formatted memory block, unless we were just rebooting within the VM.
>>> (I assume we wouldn't want to reload \NvVars from FAT in PEI...)
>>> Considering the current use case, all fresh VM startups would produce
>>> the default memory type info HOB, and wouldn't remedy the memory map
>>> fragmentation. And in the longer term, considering S4, when the VM goes
>>> down for S4 / hibernation, the VM actually goes away (QEMU exits), and
>>> S4 resume would likely not work with a different / fragmented memmap (or
>>> it would be obscurely different if you warm-rebooted once before).
>>>
>>> It is technically solvable, but the benefit is very little; it would
>>> just perpetuate the current broken emulation.
>>>
>>>>> In fact one of the longer term goals with MEM_VARSTORE_EMU_ENABLE is to
>>>>> identify what we'll rip out once we finally decide to drop the reserved
>>>>> memory / NvVars file based emulation.
>>>>
>>>> I think what we could consider ripping out is the disk save feature.
>>>> Since QEMU/KVM (at least in the past) seem to preserve RAM contents
>>>> across reboot, I think the memory based buffers should continue to be
>>>> a fallback for when users don't get pflash setup correctly.
>>>
>>> (a) The memory preservation works across reboot, yes, but it is not
>>> useful for the memory type information HOB, and consequently (in the
>>> longer term) for S4. The memory type information HOB should carry
>>> information (== implement a long-term maximum search) over several cold
>>> boots (= different QEMU instances) of the same virtual machine.
>>>
>>> At every normal boot, the emulated varstore will be empty (and
>>> well-formed only because we'd have to do that manually too, see (2)
>>> above), and any access to it within PEI will be useless.
>>>
>>> (b) If users don't set up pflash correctly, the default build will
>>> continue to work for them without any changes at all. This covers Xen as
>>> well.
>>>
>>> The strict pflash requirements are only live if users build OVMF with
>>> either one of the following non-default switches:
>>>
>>>   -D SMM_REQUIRE
>>>
>>> or
>>>
>>>   -D MEM_VARSTORE_EMU_ENABLE=FALSE
>>>
>>> Both of these imply "I know what I'm doing".
>>>
>>> I see no benefit in enabling the FTW & Variable PEIMs without a
>>> persistent flash varstore. I do see it require a whole lot of work.
>>>
>>> If someone wants the FTW & Variable PEIMs real hard under the above (IMO
>>> useless) circumstances, they're welcome to generalize the code / make it
>>> dynamic on top of this series.
>>>
>>>> That said, I'm not sure we should decide to rip out the disk save
>>>> feature just yet, unless you think it can simplify things greatly.
>>>
>>> The disk save (\NvVars) feature is irrelevant for this series. That
>>> feature is based off the "PcdOvmfFlashVariablesEnable" PCD, which is set
>>> by QemuFlashFvbServicesRuntimeDxe, and consumed by Platform BDS.
>>>
>>> When OVMF is built with either of the above build switches,
>>> PcdOvmfFlashVariablesEnable is guaranteed to end up as TRUE, so the
>>> \NvVars code is always (dynamically) disabled in those builds.
>>>
>>> I don't want to prevent users from continuing to *use* the emulated
>>> varstore in the default build. However, that emulation is obscurely
>>> broken, and has been so for 7 years. QEMU 1.6 (with pflash) has been out
>>> for the second half of that time. I certainly want to stop *developing*
>>> features for the emulated variable store. (If someone does want to, on
>>> top of this first-line enablement, I won't stand in their way, of course.)
>>
>> ping -- what do you think of my arguments above, please?
> 
> I guess I'd like to see what it would take to enable it for the EMU
> FVB path. I hope I can find some time to look at it this weekend.
> 
> Regarding depending more on flash by default, is it correct that Xen
> is the only significant thing preventing it?

That is my understanding, yes.

Thanks,
Laszlo

> I mean, looking at it
> from the qemu/kvm side of things, I would say we could discuss just
> building that way by default at this point. I still think we would
> want a failure path friendlier than a blind crash, which still leaves
> some dependence on EMU FVB so we could boot far enough for a user
> visible crash message.
> 
> -Jordan
> 
>> While this series is not urgent for me at the moment, flushing it (as
>> in, committing it) would certainly ease my load.
>>
>> If you disagree with the above points, I'll have to postpone this work
>> indefinitely. (The downstream churn has arrived.)
>>
>> Thanks,
>> Laszlo
>>
>>>>> A summary of build modes:
>>>>>
>>>>> * default build: works as before, with the Variable PEIM and UEFI memmap
>>>>>   defragmentation remaining unavailable.
>>>>>
>>>>> * -D SMM_REQUIRE: works as before, with the Variable PEIM and UEFI
>>>>>   memmap defragmentation enabled.
>>>>>
>>>>>   As a reminder, this build is inherently incompatible with QEMU's
>>>>>   "-bios" and "-L" parameters, which will trigger the ASSERT() in
>>>>>   QemuFlashInitialize()
>>>>>   [OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c], added in commit
>>>>>   b963ec494c48 ("OvmfPkg: QemuFlashFvbServicesRuntimeDxe: adhere to -D
>>>>>   SMM_REQUIRE", 2015-11-30).
>>>>>
>>>>> * -D MEM_VARSTORE_EMU_ENABLE=FALSE: eliminates the reserved memory /
>>>>>   NvVars based variable emulation, turns pflash into a hard requirement,
>>>>>   and enables the Variable PEIM and the UEFI memmap defragmentation.
>>>>>
>>>>>   If such a build is executed with -bios or -L, then the FTW and
>>>>>   Variable PEIMs will temporarily see a well-formed, but empty varstore
>>>>>   (straight from "OvmfPkg/VarStore.fdf.inc"), and then
>>>>>   QemuFlashFvbServicesRuntimeDxe will trip a new ASSERT(), in patch #6,
>>>>>   that is similar to the one added in commit b963ec494c48 (see above).
>>>>>
>>>>> Anatomy of the series:
>>>>>
>>>>> * Patches 01 through 04 clean up some innocent warts I noticed while
>>>>>   working on the feature.
>>>>>
>>>>> * Patches 05 through 07 introduce the new build option and its basic
>>>>>   effect, the disabling of reserved memory based variable emulation.
>>>>>
>>>>> * Patches 08 through 12 include the FTW and Variable PEIMs.
>>>>>
>>>>> * Patch 13 enables UEFI memmap defragmentation.
>>>>>
>>>>> * Patch 14 updates the README file.
>>>>>
>>>>> The variable PEIM was independently requested in
>>>>> <https://bugzilla.tianocore.org/show_bug.cgi?id=386> earlier, but
>>>>> without a good upstreamable use case, I disagreed with its inclusion.
>>>>> The memmap defragmentation is a good use case however.
>>>>>
>>>>> Also, I didn't lie about "downstream pressure" in my previous patch set;
>>>>> I wrote this one at Sunday/Monday night (and now it's Tues/Wed night).
>>>>> So one could consider this personal diligence. :)
>>>>>
>>>>> The series conforms to the multi-line function call syntax outlined in
>>>>> <https://bugzilla.tianocore.org/show_bug.cgi?id=425>.
>>>>>
>>>>> Repo:   https://github.com/lersek/edk2.git
>>>>> Branch: memmap_defrag
>>>>>
>>>>> CC: Jordan Justen <jordan.l.justen@intel.com>
>>>>>
>>>>> Thanks
>>>>> Laszlo
>>>>>
>>>>> Laszlo Ersek (14):
>>>>>   OvmfPkg/EmuVariableFvbRuntimeDxe: always format an auth varstore
>>>>>     header
>>>>>   OvmfPkg: remove gUefiOvmfPkgTokenSpaceGuid.PcdSecureBootEnable
>>>>>   OvmfPkg/PlatformPei: remove unused PcdVariableStoreSize dependency
>>>>>   OvmfPkg/PlatformPei: don't allocate reserved mem varstore if
>>>>>     SMM_REQUIRE
>>>>>   OvmfPkg: introduce PcdMemVarstoreEmuEnable feature flag
>>>>>   OvmfPkg/QemuFlashFvbServicesRuntimeDxe: check PcdMemVarstoreEmuEnable
>>>>>   OvmfPkg: conditionally disable reserved memory varstore emulation at
>>>>>     build
>>>>>   OvmfPkg: resolve PcdLib for all PEIMs individually
>>>>>   OvmfPkg: resolve PcdLib for PEIMs to PeiPcdLib by default
>>>>>   OvmfPkg: introduce FlashNvStorageAddressLib
>>>>>   OvmfPkg: include FaultTolerantWritePei and VariablePei
>>>>>   OvmfPkg/QemuFlashFvbServicesRuntimeDxe: don't set flash PCDs if SMM or
>>>>>     no-emu
>>>>>   OvmfPkg/PlatformPei: remedy UEFI memory map fragmentation
>>>>>   OvmfPkg/README: document MEM_VARSTORE_EMU_ENABLE and memmap defrag
>>>>>
>>>>>  OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c                                |  79 +---------
>>>>>  OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.inf                              |   3 -
>>>>>  OvmfPkg/Library/FlashNvStorageAddressLib/FlashNvStorageAddressLib.c   |  53 +++++++
>>>>>  OvmfPkg/Library/FlashNvStorageAddressLib/FlashNvStorageAddressLib.inf |  48 +++++++
>>>>>  OvmfPkg/OvmfPkg.dec                                                   |   7 +-
>>>>>  OvmfPkg/OvmfPkgIa32.dsc                                               |  43 +++---
>>>>>  OvmfPkg/OvmfPkgIa32.fdf                                               |   8 +-
>>>>>  OvmfPkg/OvmfPkgIa32X64.dsc                                            |  43 +++---
>>>>>  OvmfPkg/OvmfPkgIa32X64.fdf                                            |   8 +-
>>>>>  OvmfPkg/OvmfPkgX64.dsc                                                |  43 +++---
>>>>>  OvmfPkg/OvmfPkgX64.fdf                                                |   8 +-
>>>>>  OvmfPkg/PlatformPei/MemTypeInfo.c                                     | 151 ++++++++++++++++++++
>>>>>  OvmfPkg/PlatformPei/Platform.c                                        |  28 +---
>>>>>  OvmfPkg/PlatformPei/Platform.h                                        |   5 +
>>>>>  OvmfPkg/PlatformPei/PlatformPei.inf                                   |   4 +-
>>>>>  OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf      |   1 +
>>>>>  OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesSmm.inf             |   1 +
>>>>>  OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c               |  58 +++++---
>>>>>  OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c                    |   1 +
>>>>>  OvmfPkg/README                                                        |  10 ++
>>>>>  20 files changed, 425 insertions(+), 177 deletions(-)
>>>>>  create mode 100644 OvmfPkg/Library/FlashNvStorageAddressLib/FlashNvStorageAddressLib.inf
>>>>>  create mode 100644 OvmfPkg/Library/FlashNvStorageAddressLib/FlashNvStorageAddressLib.c
>>>>>  create mode 100644 OvmfPkg/PlatformPei/MemTypeInfo.c
>>>>>
>>>>> -- 
>>>>> 2.9.3
>>>>>
>>>
>>> _______________________________________________
>>> edk2-devel mailing list
>>> edk2-devel@lists.01.org
>>> https://lists.01.org/mailman/listinfo/edk2-devel
>>>
>>