[PATCH v2 0/8] OvmfPkg: add the Variable PEIM, defragment the UEFI memmap

[PATCH v2 0/8] OvmfPkg: add the Variable PEIM, defragment the UEFI memmap

[Laszlo Ersek]

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

This version is identical to the v1 seres I posted in March. Some
patches of the v1 series have been committed. I've been rebasing and
using the rest since, and am now posting it as v2. References:

* https://bugzilla.tianocore.org/show_bug.cgi?id=386

* http://mid.mail-archive.com/20170314233246.17864-1-lersek@redhat.com
  https://lists.01.org/pipermail/edk2-devel/2017-March/008525.html

* http://mid.mail-archive.com/20170327080544.24748-1-jordan.l.justen@intel.com
  https://lists.01.org/pipermail/edk2-devel/2017-March/009049.html

Cc: Anthony Perard <anthony.perard@citrix.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Julien Grall <julien.grall@linaro.org>

Thanks
Laszlo

Laszlo Ersek (8):
  OvmfPkg: introduce PcdMemVarstoreEmuEnable feature flag
  OvmfPkg/QemuFlashFvbServicesRuntimeDxe: check PcdMemVarstoreEmuEnable
  OvmfPkg: conditionally disable reserved memory varstore emulation at
    build
  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/Library/FlashNvStorageAddressLib/FlashNvStorageAddressLib.c   |  53 +++++++
 OvmfPkg/Library/FlashNvStorageAddressLib/FlashNvStorageAddressLib.inf |  48 +++++++
 OvmfPkg/OvmfPkg.dec                                                   |   6 +
 OvmfPkg/OvmfPkgIa32.dsc                                               |  15 +-
 OvmfPkg/OvmfPkgIa32.fdf                                               |   8 +-
 OvmfPkg/OvmfPkgIa32X64.dsc                                            |  15 +-
 OvmfPkg/OvmfPkgIa32X64.fdf                                            |   8 +-
 OvmfPkg/OvmfPkgX64.dsc                                                |  15 +-
 OvmfPkg/OvmfPkgX64.fdf                                                |   8 +-
 OvmfPkg/PlatformPei/MemTypeInfo.c                                     | 151 ++++++++++++++++++++
 OvmfPkg/PlatformPei/Platform.c                                        |  26 +---
 OvmfPkg/PlatformPei/Platform.h                                        |   5 +
 OvmfPkg/PlatformPei/PlatformPei.inf                                   |   3 +
 OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf      |   1 +
 OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesSmm.inf             |   1 +
 OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c               |  58 +++++---
 OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c                    |   1 +
 OvmfPkg/README                                                        |  10 ++
 18 files changed, 385 insertions(+), 47 deletions(-)
 create mode 100644 OvmfPkg/Library/FlashNvStorageAddressLib/FlashNvStorageAddressLib.c
 create mode 100644 OvmfPkg/Library/FlashNvStorageAddressLib/FlashNvStorageAddressLib.inf
 create mode 100644 OvmfPkg/PlatformPei/MemTypeInfo.c

-- 
2.14.1.3.gb7cf6e02401b

[PATCH v2 1/8] 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: Anthony Perard <anthony.perard@citrix.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Julien Grall <julien.grall@linaro.org>
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 27bcfc141e5a..f03dd7e27d6b 100644
--- a/OvmfPkg/OvmfPkg.dec
+++ b/OvmfPkg/OvmfPkg.dec
@@ -150,3 +150,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 9d23f8c162e4..443da553d0a3 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
 
   #
   # Flash size selection. Setting FD_SIZE_IN_KB on the command line directly to
@@ -410,6 +411,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 a9c667fed8b0..0fc81743bac4 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
 
   #
   # Flash size selection. Setting FD_SIZE_IN_KB on the command line directly to
@@ -415,6 +416,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 abf570512a38..db33be4bc0b7 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
 
   #
   # Flash size selection. Setting FD_SIZE_IN_KB on the command line directly to
@@ -415,6 +416,9 @@ [PcdsFeatureFlag]
   gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire|TRUE
   gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmEnableBspElection|FALSE
 !endif
+!if $(MEM_VARSTORE_EMU_ENABLE) == FALSE
+  gUefiOvmfPkgTokenSpaceGuid.PcdMemVarstoreEmuEnable|FALSE
+!endif
 
 [PcdsFixedAtBuild]
   gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeMemorySize|1
-- 
2.14.1.3.gb7cf6e02401b

Re: [PATCH v2 1/8] OvmfPkg: introduce PcdMemVarstoreEmuEnable feature flag

[Ard Biesheuvel]

On 30 November 2017 at 16:30, Laszlo Ersek <lersek@redhat.com> wrote:
> ... 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: Anthony Perard <anthony.perard@citrix.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Julien Grall <julien.grall@linaro.org>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> ---
>  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 27bcfc141e5a..f03dd7e27d6b 100644
> --- a/OvmfPkg/OvmfPkg.dec
> +++ b/OvmfPkg/OvmfPkg.dec
> @@ -150,3 +150,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 9d23f8c162e4..443da553d0a3 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
>
>    #
>    # Flash size selection. Setting FD_SIZE_IN_KB on the command line directly to
> @@ -410,6 +411,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 a9c667fed8b0..0fc81743bac4 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
>
>    #
>    # Flash size selection. Setting FD_SIZE_IN_KB on the command line directly to
> @@ -415,6 +416,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 abf570512a38..db33be4bc0b7 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
>
>    #
>    # Flash size selection. Setting FD_SIZE_IN_KB on the command line directly to
> @@ -415,6 +416,9 @@ [PcdsFeatureFlag]
>    gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire|TRUE
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmEnableBspElection|FALSE
>  !endif
> +!if $(MEM_VARSTORE_EMU_ENABLE) == FALSE
> +  gUefiOvmfPkgTokenSpaceGuid.PcdMemVarstoreEmuEnable|FALSE
> +!endif
>
>  [PcdsFixedAtBuild]
>    gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeMemorySize|1
> --
> 2.14.1.3.gb7cf6e02401b
>
>

[PATCH v2 2/8] 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: Anthony Perard <anthony.perard@citrix.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Julien Grall <julien.grall@linaro.org>
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.14.1.3.gb7cf6e02401b

Re: [PATCH v2 2/8] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: check PcdMemVarstoreEmuEnable

[Ard Biesheuvel]

On 30 November 2017 at 16:30, Laszlo Ersek <lersek@redhat.com> wrote:
> 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: Anthony Perard <anthony.perard@citrix.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Julien Grall <julien.grall@linaro.org>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> ---
>  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.14.1.3.gb7cf6e02401b
>
>

[PATCH v2 3/8] 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: Anthony Perard <anthony.perard@citrix.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Julien Grall <julien.grall@linaro.org>
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 443da553d0a3..dd6be0de0445 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -502,7 +502,7 @@ [PcdsFixedAtBuild]
 
 [PcdsDynamicDefault]
   # only set when
-  #   ($(SMM_REQUIRE) == FALSE)
+  #   (($(SMM_REQUIRE) == FALSE) && ($(MEM_VARSTORE_EMU_ENABLE) == TRUE))
   gEfiMdeModulePkgTokenSpaceGuid.PcdEmuVariableNvStoreReserved|0
 
   gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64|0
@@ -871,10 +871,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 0fc81743bac4..84c578ac22a4 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -508,7 +508,7 @@ [PcdsFixedAtBuild.X64]
 
 [PcdsDynamicDefault]
   # only set when
-  #   ($(SMM_REQUIRE) == FALSE)
+  #   (($(SMM_REQUIRE) == FALSE) && ($(MEM_VARSTORE_EMU_ENABLE) == TRUE))
   gEfiMdeModulePkgTokenSpaceGuid.PcdEmuVariableNvStoreReserved|0
 
   gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64|0
@@ -881,10 +881,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 db33be4bc0b7..b5d385101411 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -507,7 +507,7 @@ [PcdsFixedAtBuild]
 
 [PcdsDynamicDefault]
   # only set when
-  #   ($(SMM_REQUIRE) == FALSE)
+  #   (($(SMM_REQUIRE) == FALSE) && ($(MEM_VARSTORE_EMU_ENABLE) == TRUE))
   gEfiMdeModulePkgTokenSpaceGuid.PcdEmuVariableNvStoreReserved|0
 
   gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64|0
@@ -879,10 +879,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 ba980834d720..50a2db897bbb 100644
--- a/OvmfPkg/OvmfPkgIa32.fdf
+++ b/OvmfPkg/OvmfPkgIa32.fdf
@@ -191,7 +191,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
 }
@@ -375,7 +375,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 72ac82e76b7b..efa01734b576 100644
--- a/OvmfPkg/OvmfPkgIa32X64.fdf
+++ b/OvmfPkg/OvmfPkgIa32X64.fdf
@@ -192,7 +192,7 @@ [FV.DXEFV]
   INF  MdeModulePkg/Universal/DevicePathDxe/DevicePathDxe.inf
   INF  MdeModulePkg/Universal/PCD/Dxe/Pcd.inf
   INF  OvmfPkg/AmdSevDxe/AmdSevDxe.inf
-!if $(SMM_REQUIRE) == FALSE
+!if ($(SMM_REQUIRE) == FALSE) && ($(MEM_VARSTORE_EMU_ENABLE) == TRUE)
   INF  OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf
 !endif
 }
@@ -382,7 +382,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 2fc17810eb23..d7a5ea97bda8 100644
--- a/OvmfPkg/OvmfPkgX64.fdf
+++ b/OvmfPkg/OvmfPkgX64.fdf
@@ -192,7 +192,7 @@ [FV.DXEFV]
   INF  MdeModulePkg/Universal/DevicePathDxe/DevicePathDxe.inf
   INF  MdeModulePkg/Universal/PCD/Dxe/Pcd.inf
   INF  OvmfPkg/AmdSevDxe/AmdSevDxe.inf
-!if $(SMM_REQUIRE) == FALSE
+!if ($(SMM_REQUIRE) == FALSE) && ($(MEM_VARSTORE_EMU_ENABLE) == TRUE)
   INF  OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf
 !endif
 }
@@ -382,7 +382,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 de7434d93dc0..4b8626cb2a27 100644
--- a/OvmfPkg/PlatformPei/PlatformPei.inf
+++ b/OvmfPkg/PlatformPei/PlatformPei.inf
@@ -108,6 +108,7 @@ [FixedPcd]
 
 [FeaturePcd]
   gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire
+  gUefiOvmfPkgTokenSpaceGuid.PcdMemVarstoreEmuEnable
 
 [Ppis]
   gEfiPeiMasterBootModePpiGuid
diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
index 5a78668126b4..34e7e903fc70 100644
--- a/OvmfPkg/PlatformPei/Platform.c
+++ b/OvmfPkg/PlatformPei/Platform.c
@@ -664,7 +664,8 @@ InitializePlatform (
   }
 
   if (mBootMode != BOOT_ON_S3_RESUME) {
-    if (!FeaturePcdGet (PcdSmmSmramRequire)) {
+    if (!FeaturePcdGet (PcdSmmSmramRequire) &&
+        FeaturePcdGet (PcdMemVarstoreEmuEnable)) {
       ReserveEmuVariableNvStore ();
     }
     PeiFvInitialization ();
-- 
2.14.1.3.gb7cf6e02401b

Re: [PATCH v2 3/8] OvmfPkg: conditionally disable reserved memory varstore emulation at build

[Ard Biesheuvel]

On 30 November 2017 at 16:30, Laszlo Ersek <lersek@redhat.com> wrote:
> (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),
>

If that is the only concern, and the value is irrelevant, you could
make it a patchable PCD instead

> - 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: Anthony Perard <anthony.perard@citrix.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Julien Grall <julien.grall@linaro.org>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> ---
>  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 443da553d0a3..dd6be0de0445 100644
> --- a/OvmfPkg/OvmfPkgIa32.dsc
> +++ b/OvmfPkg/OvmfPkgIa32.dsc
> @@ -502,7 +502,7 @@ [PcdsFixedAtBuild]
>
>  [PcdsDynamicDefault]
>    # only set when
> -  #   ($(SMM_REQUIRE) == FALSE)
> +  #   (($(SMM_REQUIRE) == FALSE) && ($(MEM_VARSTORE_EMU_ENABLE) == TRUE))
>    gEfiMdeModulePkgTokenSpaceGuid.PcdEmuVariableNvStoreReserved|0
>
>    gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64|0
> @@ -871,10 +871,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 0fc81743bac4..84c578ac22a4 100644
> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
> @@ -508,7 +508,7 @@ [PcdsFixedAtBuild.X64]
>
>  [PcdsDynamicDefault]
>    # only set when
> -  #   ($(SMM_REQUIRE) == FALSE)
> +  #   (($(SMM_REQUIRE) == FALSE) && ($(MEM_VARSTORE_EMU_ENABLE) == TRUE))
>    gEfiMdeModulePkgTokenSpaceGuid.PcdEmuVariableNvStoreReserved|0
>
>    gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64|0
> @@ -881,10 +881,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 db33be4bc0b7..b5d385101411 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -507,7 +507,7 @@ [PcdsFixedAtBuild]
>
>  [PcdsDynamicDefault]
>    # only set when
> -  #   ($(SMM_REQUIRE) == FALSE)
> +  #   (($(SMM_REQUIRE) == FALSE) && ($(MEM_VARSTORE_EMU_ENABLE) == TRUE))
>    gEfiMdeModulePkgTokenSpaceGuid.PcdEmuVariableNvStoreReserved|0
>
>    gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64|0
> @@ -879,10 +879,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 ba980834d720..50a2db897bbb 100644
> --- a/OvmfPkg/OvmfPkgIa32.fdf
> +++ b/OvmfPkg/OvmfPkgIa32.fdf
> @@ -191,7 +191,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
>  }
> @@ -375,7 +375,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 72ac82e76b7b..efa01734b576 100644
> --- a/OvmfPkg/OvmfPkgIa32X64.fdf
> +++ b/OvmfPkg/OvmfPkgIa32X64.fdf
> @@ -192,7 +192,7 @@ [FV.DXEFV]
>    INF  MdeModulePkg/Universal/DevicePathDxe/DevicePathDxe.inf
>    INF  MdeModulePkg/Universal/PCD/Dxe/Pcd.inf
>    INF  OvmfPkg/AmdSevDxe/AmdSevDxe.inf
> -!if $(SMM_REQUIRE) == FALSE
> +!if ($(SMM_REQUIRE) == FALSE) && ($(MEM_VARSTORE_EMU_ENABLE) == TRUE)
>    INF  OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf
>  !endif
>  }
> @@ -382,7 +382,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 2fc17810eb23..d7a5ea97bda8 100644
> --- a/OvmfPkg/OvmfPkgX64.fdf
> +++ b/OvmfPkg/OvmfPkgX64.fdf
> @@ -192,7 +192,7 @@ [FV.DXEFV]
>    INF  MdeModulePkg/Universal/DevicePathDxe/DevicePathDxe.inf
>    INF  MdeModulePkg/Universal/PCD/Dxe/Pcd.inf
>    INF  OvmfPkg/AmdSevDxe/AmdSevDxe.inf
> -!if $(SMM_REQUIRE) == FALSE
> +!if ($(SMM_REQUIRE) == FALSE) && ($(MEM_VARSTORE_EMU_ENABLE) == TRUE)
>    INF  OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf
>  !endif
>  }
> @@ -382,7 +382,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 de7434d93dc0..4b8626cb2a27 100644
> --- a/OvmfPkg/PlatformPei/PlatformPei.inf
> +++ b/OvmfPkg/PlatformPei/PlatformPei.inf
> @@ -108,6 +108,7 @@ [FixedPcd]
>
>  [FeaturePcd]
>    gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire
> +  gUefiOvmfPkgTokenSpaceGuid.PcdMemVarstoreEmuEnable
>
>  [Ppis]
>    gEfiPeiMasterBootModePpiGuid
> diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
> index 5a78668126b4..34e7e903fc70 100644
> --- a/OvmfPkg/PlatformPei/Platform.c
> +++ b/OvmfPkg/PlatformPei/Platform.c
> @@ -664,7 +664,8 @@ InitializePlatform (
>    }
>
>    if (mBootMode != BOOT_ON_S3_RESUME) {
> -    if (!FeaturePcdGet (PcdSmmSmramRequire)) {
> +    if (!FeaturePcdGet (PcdSmmSmramRequire) &&
> +        FeaturePcdGet (PcdMemVarstoreEmuEnable)) {
>        ReserveEmuVariableNvStore ();
>      }
>      PeiFvInitialization ();
> --
> 2.14.1.3.gb7cf6e02401b
>
>

Re: [PATCH v2 3/8] OvmfPkg: conditionally disable reserved memory varstore emulation at build

[Laszlo Ersek]

On 12/01/17 09:51, Ard Biesheuvel wrote:
> On 30 November 2017 at 16:30, Laszlo Ersek <lersek@redhat.com> wrote:
>> (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),
>>
> 
> If that is the only concern, and the value is irrelevant, you could
> make it a patchable PCD instead

This sounds interesting; I've never used patchable PCDs. Can you please
elaborate?

Do you mean that for (SMM_REQUIRE==TRUE ||
MEM_VARSTORE_EMU_ENABLE==FALSE), the DSC file should list
PcdEmuVariableNvStoreReserved somewhere else?

Does PcdSet work on patchable PCDs?

Thanks!
Laszlo

Re: [PATCH v2 3/8] OvmfPkg: conditionally disable reserved memory varstore emulation at build

[Ard Biesheuvel]

On 1 December 2017 at 10:52, Laszlo Ersek <lersek@redhat.com> wrote:
> On 12/01/17 09:51, Ard Biesheuvel wrote:
>> On 30 November 2017 at 16:30, Laszlo Ersek <lersek@redhat.com> wrote:
>>> (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),
>>>
>>
>> If that is the only concern, and the value is irrelevant, you could
>> make it a patchable PCD instead
>
> This sounds interesting; I've never used patchable PCDs. Can you please
> elaborate?
>
> Do you mean that for (SMM_REQUIRE==TRUE ||
> MEM_VARSTORE_EMU_ENABLE==FALSE), the DSC file should list
> PcdEmuVariableNvStoreReserved somewhere else?
>

Yes.

> Does PcdSet work on patchable PCDs?
>

Yes. It's a bit icky because the value does not actually propagate to
other modules, but that doesn't matter in this case.

Re: [PATCH v2 3/8] OvmfPkg: conditionally disable reserved memory varstore emulation at build

[Laszlo Ersek]

On 12/01/17 11:53, Ard Biesheuvel wrote:
> On 1 December 2017 at 10:52, Laszlo Ersek <lersek@redhat.com> wrote:
>> On 12/01/17 09:51, Ard Biesheuvel wrote:
>>> On 30 November 2017 at 16:30, Laszlo Ersek <lersek@redhat.com> wrote:
>>>> (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),
>>>>
>>>
>>> If that is the only concern, and the value is irrelevant, you could
>>> make it a patchable PCD instead
>>
>> This sounds interesting; I've never used patchable PCDs. Can you please
>> elaborate?
>>
>> Do you mean that for (SMM_REQUIRE==TRUE ||
>> MEM_VARSTORE_EMU_ENABLE==FALSE), the DSC file should list
>> PcdEmuVariableNvStoreReserved somewhere else?
>>
> 
> Yes.
> 
>> Does PcdSet work on patchable PCDs?
>>
> 
> Yes. It's a bit icky because the value does not actually propagate to
> other modules, but that doesn't matter in this case.

Hm, this sounds like a recipe. We have two sets of affected PCDs:

- PcdEmuVariableNvStoreReserved --> in the "flash required" builds, make
it patchable, don't bother setting a good value at build time, and the
PcdSet calls will compile (but won't ever be reached due to code logic)

- PcdFlashNvStorageVariableBase64, PcdFlashNvStorageFtwWorkingBase,
PcdFlashNvStorageFtwSpareBase: in the "flash required" builds, make them
patchable, *try* to set the right default values at build time, with SET
commands, and the PcdSet calls will compile (but won't ever be reached
due to code logic)

That could let us drop FlashNvStorageAddressLib. (On patch #6, only the
commit message would change.)

Thanks,
Laszlo

Re: [PATCH v2 3/8] OvmfPkg: conditionally disable reserved memory varstore emulation at build

[Ard Biesheuvel]

On 1 December 2017 at 11:41, Laszlo Ersek <lersek@redhat.com> wrote:
> On 12/01/17 11:53, Ard Biesheuvel wrote:
>> On 1 December 2017 at 10:52, Laszlo Ersek <lersek@redhat.com> wrote:
>>> On 12/01/17 09:51, Ard Biesheuvel wrote:
>>>> On 30 November 2017 at 16:30, Laszlo Ersek <lersek@redhat.com> wrote:
>>>>> (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),
>>>>>
>>>>
>>>> If that is the only concern, and the value is irrelevant, you could
>>>> make it a patchable PCD instead
>>>
>>> This sounds interesting; I've never used patchable PCDs. Can you please
>>> elaborate?
>>>
>>> Do you mean that for (SMM_REQUIRE==TRUE ||
>>> MEM_VARSTORE_EMU_ENABLE==FALSE), the DSC file should list
>>> PcdEmuVariableNvStoreReserved somewhere else?
>>>
>>
>> Yes.
>>
>>> Does PcdSet work on patchable PCDs?
>>>
>>
>> Yes. It's a bit icky because the value does not actually propagate to
>> other modules, but that doesn't matter in this case.
>
> Hm, this sounds like a recipe. We have two sets of affected PCDs:
>
> - PcdEmuVariableNvStoreReserved --> in the "flash required" builds, make
> it patchable, don't bother setting a good value at build time, and the
> PcdSet calls will compile (but won't ever be reached due to code logic)
>
> - PcdFlashNvStorageVariableBase64, PcdFlashNvStorageFtwWorkingBase,
> PcdFlashNvStorageFtwSpareBase: in the "flash required" builds, make them
> patchable, *try* to set the right default values at build time, with SET
> commands, and the PcdSet calls will compile (but won't ever be reached
> due to code logic)
>
> That could let us drop FlashNvStorageAddressLib. (On patch #6, only the
> commit message would change.)
>

Yes, something like that. I strongly prefer a NULL library class
resolution over A PRIORI, but having neither is even better.

[PATCH v2 4/8] 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: Anthony Perard <anthony.perard@citrix.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Julien Grall <julien.grall@linaro.org>
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.14.1.3.gb7cf6e02401b

Re: [PATCH v2 4/8] OvmfPkg: introduce FlashNvStorageAddressLib

[Ard Biesheuvel]

On 30 November 2017 at 16:30, Laszlo Ersek <lersek@redhat.com> wrote:
> 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.
>

Why can't we set these at build time in this case?

> Cc: Anthony Perard <anthony.perard@citrix.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Julien Grall <julien.grall@linaro.org>
> 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.14.1.3.gb7cf6e02401b
>
>

Re: [PATCH v2 4/8] OvmfPkg: introduce FlashNvStorageAddressLib

[Laszlo Ersek]

On 12/01/17 09:55, Ard Biesheuvel wrote:
> On 30 November 2017 at 16:30, Laszlo Ersek <lersek@redhat.com> wrote:
>> 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.
>>
> 
> Why can't we set these at build time in this case?

Hmm, let me think... Oh yes, I think I remember.

The PCDs that are used as *source* here (i.e., passed as arguments to
PcdGet32()) *are* set at build time. However, they take some calculation
at build time too, and they are set in the "OvmfPkg/OvmfPkg.fdf.inc"
file, with explicit SET statments.

This is because you can build OVMF for a 1MB, 2MB, or 4MB unified flash
image size, and the "source PCDs" in question depend on them.
Furthermore, these "source PCDs" are fixed-at-build.

I think I couldn't find a way to set the defaults for the "target",
dynamic, PCDs, from the build-time-calculated "source PCDs". The usual
dynamic-default setting in the DSC does not work (because the default
value is not a numeric constant), and, as far as I remember, when I
tried the SET command on the dynamic ("target") PCDs as well, it simply
didn't work.

The SET statements *could* work if the target PCDs could be made
(conditionally) fixed-at-build as well, but then we run into the same
build problem of ReserveEmuVariableNvStore() not compiling.

In short:

- the "target" PCDs have to remain dynamic, or else
ReserveEmuVariableNvStore() won't compile,

- as long as the "target" PCDs are dynamic, I cannot set their defaults
at build time *from* the "source" fixed-at-build PCDs, which are
calculated at each build separately, with the SET statement.

Thanks
Laszlo


> 
>> Cc: Anthony Perard <anthony.perard@citrix.com>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Julien Grall <julien.grall@linaro.org>
>> 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.14.1.3.gb7cf6e02401b
>>
>>

Re: [PATCH v2 4/8] OvmfPkg: introduce FlashNvStorageAddressLib

[Laszlo Ersek]

On 12/01/17 12:03, Laszlo Ersek wrote:
> On 12/01/17 09:55, Ard Biesheuvel wrote:
>> On 30 November 2017 at 16:30, Laszlo Ersek <lersek@redhat.com> wrote:
>>> 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.
>>>
>>
>> Why can't we set these at build time in this case?
> 
> Hmm, let me think... Oh yes, I think I remember.
> 
> The PCDs that are used as *source* here (i.e., passed as arguments to
> PcdGet32()) *are* set at build time. However, they take some calculation
> at build time too, and they are set in the "OvmfPkg/OvmfPkg.fdf.inc"
> file, with explicit SET statments.
> 
> This is because you can build OVMF for a 1MB, 2MB, or 4MB unified flash
> image size, and the "source PCDs" in question depend on them.
> Furthermore, these "source PCDs" are fixed-at-build.
> 
> I think I couldn't find a way to set the defaults for the "target",
> dynamic, PCDs, from the build-time-calculated "source PCDs". The usual
> dynamic-default setting in the DSC does not work (because the default
> value is not a numeric constant), and, as far as I remember, when I
> tried the SET command on the dynamic ("target") PCDs as well, it simply
> didn't work.
> 
> The SET statements *could* work if the target PCDs could be made
> (conditionally) fixed-at-build as well, but then we run into the same
> build problem of ReserveEmuVariableNvStore() not compiling.
> 
> In short:
> 
> - the "target" PCDs have to remain dynamic, or else
> ReserveEmuVariableNvStore() won't compile,

correction -- ReserveEmuVariableNvStore() is totally irrelevant here;
what I meant here were the PcdSet calls for these "target" PCDs that
wouldn't compile in QemuFlashFvbServicesRuntimeDxe. (See patch 6.)

Thanks,
Laszlo

> 
> - as long as the "target" PCDs are dynamic, I cannot set their defaults
> at build time *from* the "source" fixed-at-build PCDs, which are
> calculated at each build separately, with the SET statement.
> 
> Thanks
> Laszlo
> 
> 
>>
>>> Cc: Anthony Perard <anthony.perard@citrix.com>
>>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>>> Cc: Julien Grall <julien.grall@linaro.org>
>>> 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.14.1.3.gb7cf6e02401b
>>>
>>>
> 

[PATCH v2 5/8] 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: Anthony Perard <anthony.perard@citrix.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Julien Grall <julien.grall@linaro.org>
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 dd6be0de0445..e5268f64f381 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -592,6 +592,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 84c578ac22a4..2ba567f45306 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -600,6 +600,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 b5d385101411..28db76c630d7 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -599,6 +599,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 50a2db897bbb..d697d9151f8e 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 efa01734b576..d70b43198ab8 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 d7a5ea97bda8..bb46a409d9d8 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.14.1.3.gb7cf6e02401b

[PATCH v2 6/8] 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: Anthony Perard <anthony.perard@citrix.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Julien Grall <julien.grall@linaro.org>
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 558b395dff4a..5d5a7580454a 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.14.1.3.gb7cf6e02401b

[PATCH v2 7/8] 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: Anthony Perard <anthony.perard@citrix.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Julien Grall <julien.grall@linaro.org>
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 4b8626cb2a27..062cc083c698 100644
--- a/OvmfPkg/PlatformPei/PlatformPei.inf
+++ b/OvmfPkg/PlatformPei/PlatformPei.inf
@@ -34,6 +34,7 @@ [Sources]
   FeatureControl.c
   Fv.c
   MemDetect.c
+  MemTypeInfo.c
   Platform.c
   Xen.c
 
@@ -113,6 +114,7 @@ [FeaturePcd]
 [Ppis]
   gEfiPeiMasterBootModePpiGuid
   gEfiPeiMpServicesPpiGuid
+  gEfiPeiReadOnlyVariable2PpiGuid
 
 [Depex]
   TRUE
diff --git a/OvmfPkg/PlatformPei/Platform.h b/OvmfPkg/PlatformPei/Platform.h
index f942e61bb4f9..5847b46a308d 100644
--- a/OvmfPkg/PlatformPei/Platform.h
+++ b/OvmfPkg/PlatformPei/Platform.h
@@ -78,6 +78,11 @@ PeiFvInitialization (
   VOID
   );
 
+VOID
+MemTypeInfoInitialization (
+  VOID
+  );
+
 VOID
 InstallFeatureControlCallback (
   VOID
diff --git a/OvmfPkg/PlatformPei/MemTypeInfo.c b/OvmfPkg/PlatformPei/MemTypeInfo.c
new file mode 100644
index 000000000000..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 34e7e903fc70..1a45531359a6 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,
@@ -165,15 +152,6 @@ MemMapInitialization (
   PciIoBase = 0xC000;
   PciIoSize = 0x4000;
 
-  //
-  // Create Memory Type Information HOB
-  //
-  BuildGuidDataHob (
-    &gEfiMemoryTypeInformationGuid,
-    mDefaultMemoryTypeInformation,
-    sizeof(mDefaultMemoryTypeInformation)
-    );
-
   //
   // Video memory + Legacy BIOS region
   //
@@ -669,6 +647,7 @@ InitializePlatform (
       ReserveEmuVariableNvStore ();
     }
     PeiFvInitialization ();
+    MemTypeInfoInitialization ();
     MemMapInitialization ();
     NoexecDxeInitialization ();
   }
-- 
2.14.1.3.gb7cf6e02401b

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

[Laszlo Ersek]

Cc: Anthony Perard <anthony.perard@citrix.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Julien Grall <julien.grall@linaro.org>
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 00fb71848200..cdd8dbea7f18 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.14.1.3.gb7cf6e02401b

Re: [PATCH v2 0/8] OvmfPkg: add the Variable PEIM, defragment the UEFI memmap

[Jordan Justen]

As I recall, this enables a modest (non-essential) improvement in
fragmentation, at the cost of more ways to configure OVMF to hang
during boot. (With info available via debugcon.)

I would prefer if were able to always fallback and continue boot.
Later, after GOP has started we could visibly warn the user that they
haven't properly enabled SMM and/or flash.

If we had a way to do this, I would be in favor of displaying an error
message and delaying for 30 seconds before booting if flash has not
been enabled. (Or, if SMM was enabled in the build, but not set on the
command line.)

At that point, I think we could start to transition to making flash an
boot requirement for OVMF. But first we need to be able to visibly
warn the user why they are not able to boot.

-Jordan

On 2017-11-30 08:30:21, Laszlo Ersek wrote:
> Repo:   https://github.com/lersek/edk2.git
> Branch: memmap_defrag_v2
> 
> This version is identical to the v1 seres I posted in March. Some
> patches of the v1 series have been committed. I've been rebasing and
> using the rest since, and am now posting it as v2. References:
> 
> * https://bugzilla.tianocore.org/show_bug.cgi?id=386
> 
> * http://mid.mail-archive.com/20170314233246.17864-1-lersek@redhat.com
>   https://lists.01.org/pipermail/edk2-devel/2017-March/008525.html
> 
> * http://mid.mail-archive.com/20170327080544.24748-1-jordan.l.justen@intel.com
>   https://lists.01.org/pipermail/edk2-devel/2017-March/009049.html
> 
> Cc: Anthony Perard <anthony.perard@citrix.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Julien Grall <julien.grall@linaro.org>
> 
> Thanks
> Laszlo
> 
> Laszlo Ersek (8):
>   OvmfPkg: introduce PcdMemVarstoreEmuEnable feature flag
>   OvmfPkg/QemuFlashFvbServicesRuntimeDxe: check PcdMemVarstoreEmuEnable
>   OvmfPkg: conditionally disable reserved memory varstore emulation at
>     build
>   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/Library/FlashNvStorageAddressLib/FlashNvStorageAddressLib.c   |  53 +++++++
>  OvmfPkg/Library/FlashNvStorageAddressLib/FlashNvStorageAddressLib.inf |  48 +++++++
>  OvmfPkg/OvmfPkg.dec                                                   |   6 +
>  OvmfPkg/OvmfPkgIa32.dsc                                               |  15 +-
>  OvmfPkg/OvmfPkgIa32.fdf                                               |   8 +-
>  OvmfPkg/OvmfPkgIa32X64.dsc                                            |  15 +-
>  OvmfPkg/OvmfPkgIa32X64.fdf                                            |   8 +-
>  OvmfPkg/OvmfPkgX64.dsc                                                |  15 +-
>  OvmfPkg/OvmfPkgX64.fdf                                                |   8 +-
>  OvmfPkg/PlatformPei/MemTypeInfo.c                                     | 151 ++++++++++++++++++++
>  OvmfPkg/PlatformPei/Platform.c                                        |  26 +---
>  OvmfPkg/PlatformPei/Platform.h                                        |   5 +
>  OvmfPkg/PlatformPei/PlatformPei.inf                                   |   3 +
>  OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf      |   1 +
>  OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesSmm.inf             |   1 +
>  OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c               |  58 +++++---
>  OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c                    |   1 +
>  OvmfPkg/README                                                        |  10 ++
>  18 files changed, 385 insertions(+), 47 deletions(-)
>  create mode 100644 OvmfPkg/Library/FlashNvStorageAddressLib/FlashNvStorageAddressLib.c
>  create mode 100644 OvmfPkg/Library/FlashNvStorageAddressLib/FlashNvStorageAddressLib.inf
>  create mode 100644 OvmfPkg/PlatformPei/MemTypeInfo.c
> 
> -- 
> 2.14.1.3.gb7cf6e02401b
> 

Re: [PATCH v2 0/8] OvmfPkg: add the Variable PEIM, defragment the UEFI memmap

[Ard Biesheuvel]

On 30 November 2017 at 19:00, Jordan Justen <jordan.l.justen@intel.com> wrote:
> As I recall, this enables a modest (non-essential) improvement in
> fragmentation, at the cost of more ways to configure OVMF to hang
> during boot. (With info available via debugcon.)
>
> I would prefer if were able to always fallback and continue boot.
> Later, after GOP has started we could visibly warn the user that they
> haven't properly enabled SMM and/or flash.
>
> If we had a way to do this, I would be in favor of displaying an error
> message and delaying for 30 seconds before booting if flash has not
> been enabled. (Or, if SMM was enabled in the build, but not set on the
> command line.)
>
> At that point, I think we could start to transition to making flash an
> boot requirement for OVMF. But first we need to be able to visibly
> warn the user why they are not able to boot.
>

That doesn't argue against this series, does it? AFAICT it only argues
against building OVMF with a non-default value for
MEM_VARSTORE_EMU_ENABLE


> On 2017-11-30 08:30:21, Laszlo Ersek wrote:
>> Repo:   https://github.com/lersek/edk2.git
>> Branch: memmap_defrag_v2
>>
>> This version is identical to the v1 seres I posted in March. Some
>> patches of the v1 series have been committed. I've been rebasing and
>> using the rest since, and am now posting it as v2. References:
>>
>> * https://bugzilla.tianocore.org/show_bug.cgi?id=386
>>
>> * http://mid.mail-archive.com/20170314233246.17864-1-lersek@redhat.com
>>   https://lists.01.org/pipermail/edk2-devel/2017-March/008525.html
>>
>> * http://mid.mail-archive.com/20170327080544.24748-1-jordan.l.justen@intel.com
>>   https://lists.01.org/pipermail/edk2-devel/2017-March/009049.html
>>
>> Cc: Anthony Perard <anthony.perard@citrix.com>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Julien Grall <julien.grall@linaro.org>
>>
>> Thanks
>> Laszlo
>>
>> Laszlo Ersek (8):
>>   OvmfPkg: introduce PcdMemVarstoreEmuEnable feature flag
>>   OvmfPkg/QemuFlashFvbServicesRuntimeDxe: check PcdMemVarstoreEmuEnable
>>   OvmfPkg: conditionally disable reserved memory varstore emulation at
>>     build
>>   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/Library/FlashNvStorageAddressLib/FlashNvStorageAddressLib.c   |  53 +++++++
>>  OvmfPkg/Library/FlashNvStorageAddressLib/FlashNvStorageAddressLib.inf |  48 +++++++
>>  OvmfPkg/OvmfPkg.dec                                                   |   6 +
>>  OvmfPkg/OvmfPkgIa32.dsc                                               |  15 +-
>>  OvmfPkg/OvmfPkgIa32.fdf                                               |   8 +-
>>  OvmfPkg/OvmfPkgIa32X64.dsc                                            |  15 +-
>>  OvmfPkg/OvmfPkgIa32X64.fdf                                            |   8 +-
>>  OvmfPkg/OvmfPkgX64.dsc                                                |  15 +-
>>  OvmfPkg/OvmfPkgX64.fdf                                                |   8 +-
>>  OvmfPkg/PlatformPei/MemTypeInfo.c                                     | 151 ++++++++++++++++++++
>>  OvmfPkg/PlatformPei/Platform.c                                        |  26 +---
>>  OvmfPkg/PlatformPei/Platform.h                                        |   5 +
>>  OvmfPkg/PlatformPei/PlatformPei.inf                                   |   3 +
>>  OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf      |   1 +
>>  OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesSmm.inf             |   1 +
>>  OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c               |  58 +++++---
>>  OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c                    |   1 +
>>  OvmfPkg/README                                                        |  10 ++
>>  18 files changed, 385 insertions(+), 47 deletions(-)
>>  create mode 100644 OvmfPkg/Library/FlashNvStorageAddressLib/FlashNvStorageAddressLib.c
>>  create mode 100644 OvmfPkg/Library/FlashNvStorageAddressLib/FlashNvStorageAddressLib.inf
>>  create mode 100644 OvmfPkg/PlatformPei/MemTypeInfo.c
>>
>> --
>> 2.14.1.3.gb7cf6e02401b
>>

Re: [PATCH v2 0/8] OvmfPkg: add the Variable PEIM, defragment the UEFI memmap

[Laszlo Ersek]

On 12/01/17 09:42, Ard Biesheuvel wrote:
> On 30 November 2017 at 19:00, Jordan Justen <jordan.l.justen@intel.com> wrote:
>> As I recall, this enables a modest (non-essential) improvement in
>> fragmentation, at the cost of more ways to configure OVMF to hang
>> during boot. (With info available via debugcon.)
>>
>> I would prefer if were able to always fallback and continue boot.
>> Later, after GOP has started we could visibly warn the user that they
>> haven't properly enabled SMM and/or flash.
>>
>> If we had a way to do this, I would be in favor of displaying an error
>> message and delaying for 30 seconds before booting if flash has not
>> been enabled. (Or, if SMM was enabled in the build, but not set on the
>> command line.)
>>
>> At that point, I think we could start to transition to making flash an
>> boot requirement for OVMF. But first we need to be able to visibly
>> warn the user why they are not able to boot.
>>
> 
> That doesn't argue against this series, does it? AFAICT it only argues
> against building OVMF with a non-default value for
> MEM_VARSTORE_EMU_ENABLE

... which is explicitly documented in patch 8.

Re: [PATCH v2 0/8] OvmfPkg: add the Variable PEIM, defragment the UEFI memmap

[Laszlo Ersek]

On 11/30/17 20:00, Jordan Justen wrote:
> As I recall, this enables a modest (non-essential) improvement in
> fragmentation,

My testing from 2 minutes ago shows that the UEFI memmap (as dumped by
the Linux guest) goes from 82 entries to 56 entries. I consider the
defragmentation a QoS question, and the patch set an avenue to further
features like S4.

> at the cost of more ways to configure OVMF to hang
> during boot. (With info available via debugcon.)

Yes.

The configuration is static, the default is sane, and distros can pretty
well decide if they want it or not, same as with SMM_REQUIRE. (Some
package providers offer multiple builds.)

> I would prefer if were able to always fallback and continue boot.

Sure, I would prefer that too. Can someone write it?

- Flash detection in the PEI phase, using the SMM_REQUIRE build, would
require OVMF to raise an SMI, enter SMM, do the detection, and propagate
the result.

- Assuming it works, all we get with it is that OVMF guests launched
with the "-bios" flag (including all OVMF/Xen guests) will get a PEI
phase r/o variable driver that has *zero* chance at returning actual
variable contents (because it cannot read the \NvVars file from the ESP,
even if there is such a partition and/or file.)

Is the above worth it, for users and packagers that consciously set
MEM_VARSTORE_EMU_ENABLE to FALSE at build, and then expect the binary to
boot with "-bios"?

> Later, after GOP has started we could visibly warn the user that they
> haven't properly enabled SMM and/or flash.

GOPs are provided by UEFI_DRIVER modules, generally. SMM/flash are
required much earlier than that.

Perhaps we could use status codes, and report them to the serial port.
Then I assume the complaint would be that (a) users don't understand
status codes, (b) users don't look at the serial port.

> If we had a way to do this, I would be in favor of displaying an error
> message and delaying for 30 seconds before booting if flash has not
> been enabled. (Or, if SMM was enabled in the build, but not set on the
> command line.)

An SMM build *must not* boot on a misconfigured QEMU.

> At that point, I think we could start to transition to making flash an
> boot requirement for OVMF. But first we need to be able to visibly
> warn the user why they are not able to boot.

I don't think this is a reasonable requirement.

First, the series does not make flash a requirement for OVMF, it allows
packagers to make that choice. Which they already have to make if they
consider SMM_REQUIRE.

Second, up till very recent commit d1de487dd2e7 ("MdeModulePkg/BdsDxe:
fall back to a Boot Manager Menu loop before hanging", 2017-11-22), OVMF
(and edk2) wouldn't report a boot failure to the console that was caused
by *much higher level* problems, such as booting a traditional BIOS / OS
disk image without having a CSM and a built-in shell. And when I tried
to introduce edk2-specific status codes for emitting more detail about
the boot process to the user, to the system console, I was asked to talk
to PIWG for standardizing the status codes first.

So on one hand, you want me to report very low level boot requirement
violations through a later, highly abstracted interface; and on the
other hand, when I try to report details of the *same* high abstraction
level, I'm directed to a standards body. I guess I give up. Thanks for
your time.

Laszlo

> 
> -Jordan
> 
> On 2017-11-30 08:30:21, Laszlo Ersek wrote:
>> Repo:   https://github.com/lersek/edk2.git
>> Branch: memmap_defrag_v2
>>
>> This version is identical to the v1 seres I posted in March. Some
>> patches of the v1 series have been committed. I've been rebasing and
>> using the rest since, and am now posting it as v2. References:
>>
>> * https://bugzilla.tianocore.org/show_bug.cgi?id=386
>>
>> * http://mid.mail-archive.com/20170314233246.17864-1-lersek@redhat.com
>>   https://lists.01.org/pipermail/edk2-devel/2017-March/008525.html
>>
>> * http://mid.mail-archive.com/20170327080544.24748-1-jordan.l.justen@intel.com
>>   https://lists.01.org/pipermail/edk2-devel/2017-March/009049.html
>>
>> Cc: Anthony Perard <anthony.perard@citrix.com>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Julien Grall <julien.grall@linaro.org>
>>
>> Thanks
>> Laszlo
>>
>> Laszlo Ersek (8):
>>   OvmfPkg: introduce PcdMemVarstoreEmuEnable feature flag
>>   OvmfPkg/QemuFlashFvbServicesRuntimeDxe: check PcdMemVarstoreEmuEnable
>>   OvmfPkg: conditionally disable reserved memory varstore emulation at
>>     build
>>   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/Library/FlashNvStorageAddressLib/FlashNvStorageAddressLib.c   |  53 +++++++
>>  OvmfPkg/Library/FlashNvStorageAddressLib/FlashNvStorageAddressLib.inf |  48 +++++++
>>  OvmfPkg/OvmfPkg.dec                                                   |   6 +
>>  OvmfPkg/OvmfPkgIa32.dsc                                               |  15 +-
>>  OvmfPkg/OvmfPkgIa32.fdf                                               |   8 +-
>>  OvmfPkg/OvmfPkgIa32X64.dsc                                            |  15 +-
>>  OvmfPkg/OvmfPkgIa32X64.fdf                                            |   8 +-
>>  OvmfPkg/OvmfPkgX64.dsc                                                |  15 +-
>>  OvmfPkg/OvmfPkgX64.fdf                                                |   8 +-
>>  OvmfPkg/PlatformPei/MemTypeInfo.c                                     | 151 ++++++++++++++++++++
>>  OvmfPkg/PlatformPei/Platform.c                                        |  26 +---
>>  OvmfPkg/PlatformPei/Platform.h                                        |   5 +
>>  OvmfPkg/PlatformPei/PlatformPei.inf                                   |   3 +
>>  OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf      |   1 +
>>  OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesSmm.inf             |   1 +
>>  OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c               |  58 +++++---
>>  OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c                    |   1 +
>>  OvmfPkg/README                                                        |  10 ++
>>  18 files changed, 385 insertions(+), 47 deletions(-)
>>  create mode 100644 OvmfPkg/Library/FlashNvStorageAddressLib/FlashNvStorageAddressLib.c
>>  create mode 100644 OvmfPkg/Library/FlashNvStorageAddressLib/FlashNvStorageAddressLib.inf
>>  create mode 100644 OvmfPkg/PlatformPei/MemTypeInfo.c
>>
>> -- 
>> 2.14.1.3.gb7cf6e02401b
>>