public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [PATCH 0/4] OvmfPkg/PlatformPei: rewrite MemTypeInfo HOB production logic
@ 2020-05-08 12:16 Laszlo Ersek
  2020-05-08 12:16 ` [PATCH 1/4] OvmfPkg/PlatformPei: don't track BS Code/Data in default MemTypeInfo HOB Laszlo Ersek
                   ` (6 more replies)
  0 siblings, 7 replies; 11+ messages in thread
From: Laszlo Ersek @ 2020-05-08 12:16 UTC (permalink / raw)
  To: edk2-devel-groups-io
  Cc: Ard Biesheuvel, Jiewen Yao, Jordan Justen,
	Philippe Mathieu-Daudé

Ref:    https://bugzilla.tianocore.org/show_bug.cgi?id=2706
Repo:   https://pagure.io/lersek/edk2.git
Branch: memtypeinfo_rework

Please find the problem statement and the solution outline in the
Bugzilla ticket linked above.

Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Philippe Mathieu-Daudé <philmd@redhat.com>

Thanks,
Laszlo

Laszlo Ersek (4):
  OvmfPkg/PlatformPei: don't track BS Code/Data in default MemTypeInfo
    HOB
  OvmfPkg/PlatformPei: rewrite MemTypeInfo HOB production logic
  OvmfPkg/PlatformPei: extract memory type info defaults to PCDs
  OvmfPkg/PlatformPei: increase memory type info defaults

 OvmfPkg/OvmfPkgIa32.dsc             |  12 ++
 OvmfPkg/OvmfPkgIa32X64.dsc          |  13 ++
 OvmfPkg/OvmfPkgX64.dsc              |  12 ++
 OvmfPkg/PlatformPei/MemTypeInfo.c   | 184 +++++++++++++-------
 OvmfPkg/PlatformPei/PlatformPei.inf |   6 +
 5 files changed, 166 insertions(+), 61 deletions(-)

-- 
2.19.1.3.g30247aa5d201


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 1/4] OvmfPkg/PlatformPei: don't track BS Code/Data in default MemTypeInfo HOB
  2020-05-08 12:16 [PATCH 0/4] OvmfPkg/PlatformPei: rewrite MemTypeInfo HOB production logic Laszlo Ersek
@ 2020-05-08 12:16 ` Laszlo Ersek
  2020-05-15 17:55   ` Philippe Mathieu-Daudé
  2020-05-08 12:16 ` [PATCH 2/4] OvmfPkg/PlatformPei: rewrite MemTypeInfo HOB production logic Laszlo Ersek
                   ` (5 subsequent siblings)
  6 siblings, 1 reply; 11+ messages in thread
From: Laszlo Ersek @ 2020-05-08 12:16 UTC (permalink / raw)
  To: edk2-devel-groups-io
  Cc: Ard Biesheuvel, Jiewen Yao, Jordan Justen,
	Philippe Mathieu-Daudé

In commit d42fdd6f8384 ("OvmfPkg: improve SMM comms security with adaptive
MemoryTypeInformation", 2020-03-12), we enabled the boot-to-boot tracking
of the usages of various UEFI memory types.

Both whitepapers listed in that commit recommend that BS Code/Data type
memory *not* be tracked. This recommendation was confirmed by Jiewen in
the following two messages as well:

[1] https://edk2.groups.io/g/devel/message/55741
    http://mid.mail-archive.com/74D8A39837DF1E4DA445A8C0B3885C503F97B579@shsmsx102.ccr.corp.intel.com

[2] https://edk2.groups.io/g/devel/message/55749
    http://mid.mail-archive.com/74D8A39837DF1E4DA445A8C0B3885C503F97BDC5@shsmsx102.ccr.corp.intel.com

While tracking BS Code/Data type memory has one benefit (it de-fragments
the UEFI memory map), the downsides outweigh it. Spikes in BS Data type
memory usage are not uncommon in particular, and they may have the
following consequences:

- such reboots during normal boot that look "spurious" to the end user,
  and have no SMM security benefit,

- a large BS Data record in MemoryTypeInformation may cause issues when
  the DXE Core tries to prime the according bin(s), but the system's RAM
  size has been reduced meanwhile.

Removing the BS Code/Data entries from MemoryTypeInformation leads to a
bit more fragmentation in the UEFI memory map, but that should be
harmless.

Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2706
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/PlatformPei/MemTypeInfo.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/OvmfPkg/PlatformPei/MemTypeInfo.c b/OvmfPkg/PlatformPei/MemTypeInfo.c
index 863c6f382680..8100a2db7d44 100644
--- a/OvmfPkg/PlatformPei/MemTypeInfo.c
+++ b/OvmfPkg/PlatformPei/MemTypeInfo.c
@@ -31,8 +31,6 @@ STATIC EFI_MEMORY_TYPE_INFORMATION mDefaultMemoryTypeInformation[] = {
   { EfiReservedMemoryType,  0x004 },
   { EfiRuntimeServicesData, 0x024 },
   { EfiRuntimeServicesCode, 0x030 },
-  { EfiBootServicesCode,    0x180 },
-  { EfiBootServicesData,    0xF00 },
   { EfiMaxMemoryType,       0x000 }
 };
 
-- 
2.19.1.3.g30247aa5d201



^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 2/4] OvmfPkg/PlatformPei: rewrite MemTypeInfo HOB production logic
  2020-05-08 12:16 [PATCH 0/4] OvmfPkg/PlatformPei: rewrite MemTypeInfo HOB production logic Laszlo Ersek
  2020-05-08 12:16 ` [PATCH 1/4] OvmfPkg/PlatformPei: don't track BS Code/Data in default MemTypeInfo HOB Laszlo Ersek
@ 2020-05-08 12:16 ` Laszlo Ersek
  2020-05-08 12:16 ` [PATCH 3/4] OvmfPkg/PlatformPei: extract memory type info defaults to PCDs Laszlo Ersek
                   ` (4 subsequent siblings)
  6 siblings, 0 replies; 11+ messages in thread
From: Laszlo Ersek @ 2020-05-08 12:16 UTC (permalink / raw)
  To: edk2-devel-groups-io
  Cc: Ard Biesheuvel, Jiewen Yao, Jordan Justen,
	Philippe Mathieu-Daudé

The previous patch has no effect -- i.e., it cannot stop the tracking of
BS Code/Data in MemTypeInfo -- if the virtual machine already has a
MemoryTypeInformation UEFI variable.

In that case, our current logic allows the DXE IPL PEIM to translate the
UEFI variable to the HOB, and that translation is verbatim. If the
variable already contains records for BS Code/Data, the issues listed in
the previous patch persist for the virtual machine.

For this reason, *always* install PlatformPei's own MemTypeInfo HOB. This
prevents the DXE IPL PEIM's variable-to-HOB translation.

In PlatformPei, consume the records in the MemoryTypeInformation UEFI
variable as hints:

- Ignore all memory types for which we wouldn't by default install records
  in the HOB. This hides BS Code/Data from any existent
  MemoryTypeInformation variable.

- For the memory types that our defaults cover, enable the records in the
  UEFI variable to increase (and *only* to increase) the page counts.

  This lets the MemoryTypeInformation UEFI variable function as designed,
  but it eliminates a reboot when such a new OVMF binary is deployed (a)
  that has higher memory consumption than tracked by the virtual machine's
  UEFI variable previously, *but* (b) whose defaults also reflect those
  higher page counts.

Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2706
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/PlatformPei/MemTypeInfo.c | 161 ++++++++++++++------
 1 file changed, 114 insertions(+), 47 deletions(-)

diff --git a/OvmfPkg/PlatformPei/MemTypeInfo.c b/OvmfPkg/PlatformPei/MemTypeInfo.c
index 8100a2db7d44..d287fb9d7df0 100644
--- a/OvmfPkg/PlatformPei/MemTypeInfo.c
+++ b/OvmfPkg/PlatformPei/MemTypeInfo.c
@@ -1,7 +1,5 @@
 /** @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.
+  Produce the memory type information HOB.
 
   Copyright (C) 2017-2020, Red Hat, Inc.
 
@@ -25,7 +23,7 @@
 // of BIN hints that made sense at a particular time, for some (now likely
 // unknown) workloads / boot paths.
 //
-STATIC EFI_MEMORY_TYPE_INFORMATION mDefaultMemoryTypeInformation[] = {
+STATIC EFI_MEMORY_TYPE_INFORMATION mMemoryTypeInformation[] = {
   { EfiACPIMemoryNVS,       0x004 },
   { EfiACPIReclaimMemory,   0x008 },
   { EfiReservedMemoryType,  0x004 },
@@ -42,11 +40,117 @@ BuildMemTypeInfoHob (
 {
   BuildGuidDataHob (
     &gEfiMemoryTypeInformationGuid,
-    mDefaultMemoryTypeInformation,
-    sizeof mDefaultMemoryTypeInformation
+    mMemoryTypeInformation,
+    sizeof mMemoryTypeInformation
     );
-  DEBUG ((DEBUG_INFO, "%a: default memory type information HOB built\n",
-    __FUNCTION__));
+}
+
+/**
+  Refresh the mMemoryTypeInformation array (which we'll turn into the
+  MemoryTypeInformation HOB) from the MemoryTypeInformation UEFI variable.
+
+  Normally, the DXE IPL PEIM builds the HOB from the UEFI variable. But it does
+  so *transparently*. Instead, we consider the UEFI variable as a list of
+  hints, for updating our HOB defaults:
+
+  - Record types not covered in mMemoryTypeInformation are ignored. In
+    particular, this hides record types from the UEFI variable that may lead to
+    reboots without benefiting SMM security, such as EfiBootServicesData.
+
+  - Records that would lower the defaults in mMemoryTypeInformation are also
+    ignored.
+
+  @param[in] ReadOnlyVariable2  The EFI_PEI_READ_ONLY_VARIABLE2_PPI used for
+                                retrieving the MemoryTypeInformation UEFI
+                                variable.
+**/
+STATIC
+VOID
+RefreshMemTypeInfo (
+  IN EFI_PEI_READ_ONLY_VARIABLE2_PPI *ReadOnlyVariable2
+  )
+{
+  UINTN                       DataSize;
+  EFI_MEMORY_TYPE_INFORMATION Entries[EfiMaxMemoryType + 1];
+  EFI_STATUS                  Status;
+  UINTN                       NumEntries;
+  UINTN                       HobRecordIdx;
+
+  //
+  // Read the MemoryTypeInformation UEFI variable from the
+  // gEfiMemoryTypeInformationGuid namespace.
+  //
+  DataSize = sizeof Entries;
+  Status = ReadOnlyVariable2->GetVariable (
+                                ReadOnlyVariable2,
+                                EFI_MEMORY_TYPE_INFORMATION_VARIABLE_NAME,
+                                &gEfiMemoryTypeInformationGuid,
+                                NULL,
+                                &DataSize,
+                                Entries
+                                );
+  if (EFI_ERROR (Status)) {
+    //
+    // If the UEFI variable does not exist (EFI_NOT_FOUND), we can't use it for
+    // udpating mMemoryTypeInformation.
+    //
+    // If the UEFI variable exists but Entries is too small to hold it
+    // (EFI_BUFFER_TOO_SMALL), then the variable contents are arguably invalid.
+    // That's because Entries has room for every distinct EFI_MEMORY_TYPE,
+    // including the terminator record with EfiMaxMemoryType. Thus, we can't
+    // use the UEFI variable for updating mMemoryTypeInformation.
+    //
+    // If the UEFI variable couldn't be read for some other reason, we
+    // similarly can't use it for udpating mMemoryTypeInformation.
+    //
+    DEBUG ((DEBUG_ERROR, "%a: GetVariable(): %r\n", __FUNCTION__, Status));
+    return;
+  }
+
+  //
+  // Sanity-check the UEFI variable size against the record size.
+  //
+  if (DataSize % sizeof Entries[0] != 0) {
+    DEBUG ((DEBUG_ERROR, "%a: invalid UEFI variable size %Lu\n", __FUNCTION__,
+      (UINT64)DataSize));
+    return;
+  }
+  NumEntries = DataSize / sizeof Entries[0];
+
+  //
+  // For each record in mMemoryTypeInformation, except the terminator record,
+  // look up the first match (if any) in the UEFI variable, based on the memory
+  // type.
+  //
+  for (HobRecordIdx = 0;
+       HobRecordIdx < ARRAY_SIZE (mMemoryTypeInformation) - 1;
+       HobRecordIdx++) {
+    EFI_MEMORY_TYPE_INFORMATION *HobRecord;
+    UINTN                       Idx;
+    EFI_MEMORY_TYPE_INFORMATION *VariableRecord;
+
+    HobRecord = &mMemoryTypeInformation[HobRecordIdx];
+
+    for (Idx = 0; Idx < NumEntries; Idx++) {
+      VariableRecord = &Entries[Idx];
+
+      if (VariableRecord->Type == HobRecord->Type) {
+        break;
+      }
+    }
+
+    //
+    // If there is a match, allow the UEFI variable to increase NumberOfPages.
+    //
+    if (Idx < NumEntries &&
+        HobRecord->NumberOfPages < VariableRecord->NumberOfPages) {
+      DEBUG ((DEBUG_VERBOSE, "%a: Type 0x%x: NumberOfPages 0x%x -> 0x%x\n",
+        __FUNCTION__, HobRecord->Type, HobRecord->NumberOfPages,
+        VariableRecord->NumberOfPages));
+
+      HobRecord->NumberOfPages = VariableRecord->NumberOfPages;
+    }
+  }
 }
 
 /**
@@ -70,47 +174,10 @@ OnReadOnlyVariable2Available (
   IN VOID                       *Ppi
   )
 {
-  EFI_PEI_READ_ONLY_VARIABLE2_PPI *ReadOnlyVariable2;
-  UINTN                           DataSize;
-  EFI_STATUS                      Status;
-
   DEBUG ((DEBUG_VERBOSE, "%a\n", __FUNCTION__));
 
-  //
-  // Check if the "MemoryTypeInformation" variable exists, in the
-  // gEfiMemoryTypeInformationGuid namespace.
-  //
-  ReadOnlyVariable2 = Ppi;
-  DataSize = 0;
-  Status = ReadOnlyVariable2->GetVariable (
-                                ReadOnlyVariable2,
-                                EFI_MEMORY_TYPE_INFORMATION_VARIABLE_NAME,
-                                &gEfiMemoryTypeInformationGuid,
-                                NULL,
-                                &DataSize,
-                                NULL
-                                );
-  switch (Status) {
-  case EFI_BUFFER_TOO_SMALL:
-    //
-    // The variable exists; the DXE IPL PEIM will build the HOB from it.
-    //
-    break;
-  case EFI_NOT_FOUND:
-    //
-    // The variable does not exist; install the default memory type information
-    // HOB.
-    //
-    BuildMemTypeInfoHob ();
-    break;
-  default:
-    DEBUG ((DEBUG_ERROR, "%a: unexpected: GetVariable(): %r\n", __FUNCTION__,
-      Status));
-    ASSERT (FALSE);
-    CpuDeadLoop ();
-    break;
-  }
-
+  RefreshMemTypeInfo (Ppi);
+  BuildMemTypeInfoHob ();
   return EFI_SUCCESS;
 }
 
-- 
2.19.1.3.g30247aa5d201



^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 3/4] OvmfPkg/PlatformPei: extract memory type info defaults to PCDs
  2020-05-08 12:16 [PATCH 0/4] OvmfPkg/PlatformPei: rewrite MemTypeInfo HOB production logic Laszlo Ersek
  2020-05-08 12:16 ` [PATCH 1/4] OvmfPkg/PlatformPei: don't track BS Code/Data in default MemTypeInfo HOB Laszlo Ersek
  2020-05-08 12:16 ` [PATCH 2/4] OvmfPkg/PlatformPei: rewrite MemTypeInfo HOB production logic Laszlo Ersek
@ 2020-05-08 12:16 ` Laszlo Ersek
  2020-05-15 17:40   ` Philippe Mathieu-Daudé
  2020-05-08 12:16 ` [PATCH 4/4] OvmfPkg/PlatformPei: increase memory type info defaults Laszlo Ersek
                   ` (3 subsequent siblings)
  6 siblings, 1 reply; 11+ messages in thread
From: Laszlo Ersek @ 2020-05-08 12:16 UTC (permalink / raw)
  To: edk2-devel-groups-io
  Cc: Ard Biesheuvel, Jiewen Yao, Jordan Justen,
	Philippe Mathieu-Daudé

Some OvmfPkg modules already depend on "EmbeddedPkg.dec"; thus, replace
the open-coded memory type info defaults in the source code with the
EmbeddedPkg PCDs that stand for the same purpose. Consequently, platform
builders can override these values with the "--pcd" option of "build",
without source code updates.

While at it, sort the memory type names alphabetically.

Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2706
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/OvmfPkgIa32.dsc             | 12 +++++++++++
 OvmfPkg/OvmfPkgIa32X64.dsc          | 13 ++++++++++++
 OvmfPkg/OvmfPkgX64.dsc              | 12 +++++++++++
 OvmfPkg/PlatformPei/PlatformPei.inf |  6 ++++++
 OvmfPkg/PlatformPei/MemTypeInfo.c   | 21 +++++++++-----------
 5 files changed, 52 insertions(+), 12 deletions(-)

diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
index 7c8b51f43b66..a1981f8085a6 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -532,6 +532,18 @@ [PcdsFixedAtBuild]
   gEfiSourceLevelDebugPkgTokenSpaceGuid.PcdDebugLoadImageMethod|0x2
 !endif
 
+  #
+  # The NumberOfPages values below are ad-hoc. They are updated sporadically at
+  # best (please refer to git-blame for past updates). The values capture a set
+  # of BIN hints that made sense at a particular time, for some (now likely
+  # unknown) workloads / boot paths.
+  #
+  gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIMemoryNVS|0x4
+  gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIReclaimMemory|0x8
+  gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiReservedMemoryType|0x4
+  gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiRuntimeServicesCode|0x30
+  gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiRuntimeServicesData|0x24
+
   #
   # Network Pcds
   #
diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index a0596c44168c..c1950e6aa915 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -536,6 +536,19 @@ [PcdsFixedAtBuild]
   gEfiSourceLevelDebugPkgTokenSpaceGuid.PcdDebugLoadImageMethod|0x2
 !endif
 
+[PcdsFixedAtBuild.IA32]
+  #
+  # The NumberOfPages values below are ad-hoc. They are updated sporadically at
+  # best (please refer to git-blame for past updates). The values capture a set
+  # of BIN hints that made sense at a particular time, for some (now likely
+  # unknown) workloads / boot paths.
+  #
+  gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIMemoryNVS|0x4
+  gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIReclaimMemory|0x8
+  gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiReservedMemoryType|0x4
+  gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiRuntimeServicesCode|0x30
+  gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiRuntimeServicesData|0x24
+
 [PcdsFixedAtBuild.X64]
   #
   # Network Pcds
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index 2e764b6b7233..bc7e15f749ca 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -536,6 +536,18 @@ [PcdsFixedAtBuild]
   gEfiSourceLevelDebugPkgTokenSpaceGuid.PcdDebugLoadImageMethod|0x2
 !endif
 
+  #
+  # The NumberOfPages values below are ad-hoc. They are updated sporadically at
+  # best (please refer to git-blame for past updates). The values capture a set
+  # of BIN hints that made sense at a particular time, for some (now likely
+  # unknown) workloads / boot paths.
+  #
+  gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIMemoryNVS|0x4
+  gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIReclaimMemory|0x8
+  gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiReservedMemoryType|0x4
+  gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiRuntimeServicesCode|0x30
+  gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiRuntimeServicesData|0x24
+
   #
   # Network Pcds
   #
diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf b/OvmfPkg/PlatformPei/PlatformPei.inf
index e72ef7963d97..ff397b3ee9d7 100644
--- a/OvmfPkg/PlatformPei/PlatformPei.inf
+++ b/OvmfPkg/PlatformPei/PlatformPei.inf
@@ -37,6 +37,7 @@ [Sources]
   Xen.h
 
 [Packages]
+  EmbeddedPkg/EmbeddedPkg.dec
   MdePkg/MdePkg.dec
   MdeModulePkg/MdeModulePkg.dec
   SecurityPkg/SecurityPkg.dec
@@ -105,6 +106,11 @@ [Pcd]
 
 [FixedPcd]
   gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress
+  gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIMemoryNVS
+  gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIReclaimMemory
+  gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiReservedMemoryType
+  gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiRuntimeServicesCode
+  gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiRuntimeServicesData
 
 [FeaturePcd]
   gUefiOvmfPkgTokenSpaceGuid.PcdCsmEnable
diff --git a/OvmfPkg/PlatformPei/MemTypeInfo.c b/OvmfPkg/PlatformPei/MemTypeInfo.c
index d287fb9d7df0..f3ce2b6865e1 100644
--- a/OvmfPkg/PlatformPei/MemTypeInfo.c
+++ b/OvmfPkg/PlatformPei/MemTypeInfo.c
@@ -17,19 +17,16 @@
 
 #include "Platform.h"
 
-//
-// The NumberOfPages values below are ad-hoc. They are updated sporadically at
-// best (please refer to git-blame for past updates). The values capture a set
-// of BIN hints that made sense at a particular time, for some (now likely
-// unknown) workloads / boot paths.
-//
+#define MEMORY_TYPE_INFO_DEFAULT(Type) \
+  { Type, FixedPcdGet32 (PcdMemoryType ## Type) }
+
 STATIC EFI_MEMORY_TYPE_INFORMATION mMemoryTypeInformation[] = {
-  { EfiACPIMemoryNVS,       0x004 },
-  { EfiACPIReclaimMemory,   0x008 },
-  { EfiReservedMemoryType,  0x004 },
-  { EfiRuntimeServicesData, 0x024 },
-  { EfiRuntimeServicesCode, 0x030 },
-  { EfiMaxMemoryType,       0x000 }
+  MEMORY_TYPE_INFO_DEFAULT (EfiACPIMemoryNVS),
+  MEMORY_TYPE_INFO_DEFAULT (EfiACPIReclaimMemory),
+  MEMORY_TYPE_INFO_DEFAULT (EfiReservedMemoryType),
+  MEMORY_TYPE_INFO_DEFAULT (EfiRuntimeServicesCode),
+  MEMORY_TYPE_INFO_DEFAULT (EfiRuntimeServicesData),
+  { EfiMaxMemoryType, 0 }
 };
 
 STATIC
-- 
2.19.1.3.g30247aa5d201



^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 4/4] OvmfPkg/PlatformPei: increase memory type info defaults
  2020-05-08 12:16 [PATCH 0/4] OvmfPkg/PlatformPei: rewrite MemTypeInfo HOB production logic Laszlo Ersek
                   ` (2 preceding siblings ...)
  2020-05-08 12:16 ` [PATCH 3/4] OvmfPkg/PlatformPei: extract memory type info defaults to PCDs Laszlo Ersek
@ 2020-05-08 12:16 ` Laszlo Ersek
  2020-05-12 15:19 ` [edk2-devel] [PATCH 0/4] OvmfPkg/PlatformPei: rewrite MemTypeInfo HOB production logic Laszlo Ersek
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 11+ messages in thread
From: Laszlo Ersek @ 2020-05-08 12:16 UTC (permalink / raw)
  To: edk2-devel-groups-io
  Cc: Ard Biesheuvel, Jiewen Yao, Jordan Justen,
	Philippe Mathieu-Daudé

Any new OVMF binary (containing commit d42fdd6f8384, and built with
SMM_REQUIRE) is likely to reboot during its first boot, regardless of
whether the variable store is logically empty, or it contains a
MemoryTypeInformation variable from an earlier OVMF binary.

This "reboot on first boot after OVMF upgrade" occurs despite having
eliminated BS Code/Data tracking in earlier parts of this series. Meaning
that we've outgrown the bins of those memory types too that matter for SMM
security.

Eliminating said reboot will make an upgrade to edk2-stable202005 more
comfortable for users. Increase the defaults empirically. (The total
doesn't exceed 3MB by much.)

Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2706
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/OvmfPkgIa32.dsc    | 10 +++++-----
 OvmfPkg/OvmfPkgIa32X64.dsc | 10 +++++-----
 OvmfPkg/OvmfPkgX64.dsc     | 10 +++++-----
 3 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
index a1981f8085a6..cb01d16f7ffc 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -538,11 +538,11 @@ [PcdsFixedAtBuild]
   # of BIN hints that made sense at a particular time, for some (now likely
   # unknown) workloads / boot paths.
   #
-  gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIMemoryNVS|0x4
-  gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIReclaimMemory|0x8
-  gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiReservedMemoryType|0x4
-  gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiRuntimeServicesCode|0x30
-  gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiRuntimeServicesData|0x24
+  gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIMemoryNVS|0x80
+  gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIReclaimMemory|0x10
+  gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiReservedMemoryType|0x80
+  gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiRuntimeServicesCode|0x100
+  gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiRuntimeServicesData|0x100
 
   #
   # Network Pcds
diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index c1950e6aa915..ce4b44e548dd 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -543,11 +543,11 @@ [PcdsFixedAtBuild.IA32]
   # of BIN hints that made sense at a particular time, for some (now likely
   # unknown) workloads / boot paths.
   #
-  gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIMemoryNVS|0x4
-  gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIReclaimMemory|0x8
-  gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiReservedMemoryType|0x4
-  gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiRuntimeServicesCode|0x30
-  gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiRuntimeServicesData|0x24
+  gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIMemoryNVS|0x80
+  gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIReclaimMemory|0x10
+  gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiReservedMemoryType|0x80
+  gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiRuntimeServicesCode|0x100
+  gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiRuntimeServicesData|0x100
 
 [PcdsFixedAtBuild.X64]
   #
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index bc7e15f749ca..abd4230e6a9d 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -542,11 +542,11 @@ [PcdsFixedAtBuild]
   # of BIN hints that made sense at a particular time, for some (now likely
   # unknown) workloads / boot paths.
   #
-  gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIMemoryNVS|0x4
-  gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIReclaimMemory|0x8
-  gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiReservedMemoryType|0x4
-  gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiRuntimeServicesCode|0x30
-  gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiRuntimeServicesData|0x24
+  gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIMemoryNVS|0x80
+  gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIReclaimMemory|0x10
+  gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiReservedMemoryType|0x80
+  gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiRuntimeServicesCode|0x100
+  gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiRuntimeServicesData|0x100
 
   #
   # Network Pcds
-- 
2.19.1.3.g30247aa5d201


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [edk2-devel] [PATCH 0/4] OvmfPkg/PlatformPei: rewrite MemTypeInfo HOB production logic
  2020-05-08 12:16 [PATCH 0/4] OvmfPkg/PlatformPei: rewrite MemTypeInfo HOB production logic Laszlo Ersek
                   ` (3 preceding siblings ...)
  2020-05-08 12:16 ` [PATCH 4/4] OvmfPkg/PlatformPei: increase memory type info defaults Laszlo Ersek
@ 2020-05-12 15:19 ` Laszlo Ersek
  2020-05-15 10:10   ` Laszlo Ersek
  2020-05-15 17:19 ` Ard Biesheuvel
  2020-05-18 16:09 ` [edk2-devel] " Laszlo Ersek
  6 siblings, 1 reply; 11+ messages in thread
From: Laszlo Ersek @ 2020-05-12 15:19 UTC (permalink / raw)
  To: edk2-devel-groups-io
  Cc: Ard Biesheuvel, Jiewen Yao, Jordan Justen,
	Philippe Mathieu-Daudé, Liming Gao,
	Leif Lindholm (Nuvia address), Michael Kinney, Andrew Fish

A meta-note:

On 05/08/20 14:16, Laszlo Ersek wrote:
> Ref:    https://bugzilla.tianocore.org/show_bug.cgi?id=2706
> Repo:   https://pagure.io/lersek/edk2.git
> Branch: memtypeinfo_rework
> 
> Please find the problem statement and the solution outline in the
> Bugzilla ticket linked above.

I consider this patch series a regression fix. (The BZ has the
"regression" keyword set.)

It's not yet time for me to ping reviewers for ACKs (the patch set has
been on the list for 4 days only), just saying that, knowing that the
edk2-stable202005 Soft Feature Freeze will be announced on the 15th, I
consider this set suitable for merging during the Hard Feature Freeze too.

Thanks,
Laszlo

> 
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> 
> Thanks,
> Laszlo
> 
> Laszlo Ersek (4):
>   OvmfPkg/PlatformPei: don't track BS Code/Data in default MemTypeInfo
>     HOB
>   OvmfPkg/PlatformPei: rewrite MemTypeInfo HOB production logic
>   OvmfPkg/PlatformPei: extract memory type info defaults to PCDs
>   OvmfPkg/PlatformPei: increase memory type info defaults
> 
>  OvmfPkg/OvmfPkgIa32.dsc             |  12 ++
>  OvmfPkg/OvmfPkgIa32X64.dsc          |  13 ++
>  OvmfPkg/OvmfPkgX64.dsc              |  12 ++
>  OvmfPkg/PlatformPei/MemTypeInfo.c   | 184 +++++++++++++-------
>  OvmfPkg/PlatformPei/PlatformPei.inf |   6 +
>  5 files changed, 166 insertions(+), 61 deletions(-)
> 


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [edk2-devel] [PATCH 0/4] OvmfPkg/PlatformPei: rewrite MemTypeInfo HOB production logic
  2020-05-12 15:19 ` [edk2-devel] [PATCH 0/4] OvmfPkg/PlatformPei: rewrite MemTypeInfo HOB production logic Laszlo Ersek
@ 2020-05-15 10:10   ` Laszlo Ersek
  0 siblings, 0 replies; 11+ messages in thread
From: Laszlo Ersek @ 2020-05-15 10:10 UTC (permalink / raw)
  To: Ard Biesheuvel, Jiewen Yao
  Cc: edk2-devel-groups-io, Jordan Justen, Philippe Mathieu-Daudé,
	Liming Gao, Leif Lindholm (Nuvia address), Michael Kinney,
	Andrew Fish

Hi Jiewen, Ard,

On 05/12/20 17:19, Laszlo Ersek wrote:
> A meta-note:
> 
> On 05/08/20 14:16, Laszlo Ersek wrote:
>> Ref:    https://bugzilla.tianocore.org/show_bug.cgi?id=2706
>> Repo:   https://pagure.io/lersek/edk2.git
>> Branch: memtypeinfo_rework
>>
>> Please find the problem statement and the solution outline in the
>> Bugzilla ticket linked above.
> 
> I consider this patch series a regression fix. (The BZ has the
> "regression" keyword set.)
> 
> It's not yet time for me to ping reviewers for ACKs (the patch set has
> been on the list for 4 days only), just saying that, knowing that the
> edk2-stable202005 Soft Feature Freeze will be announced on the 15th, I
> consider this set suitable for merging during the Hard Feature Freeze too.

Jiewen -- do you have any feedback?

If not: Ard, can you please ACK this series?

Thanks!
Laszlo


>> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
>>
>> Thanks,
>> Laszlo
>>
>> Laszlo Ersek (4):
>>   OvmfPkg/PlatformPei: don't track BS Code/Data in default MemTypeInfo
>>     HOB
>>   OvmfPkg/PlatformPei: rewrite MemTypeInfo HOB production logic
>>   OvmfPkg/PlatformPei: extract memory type info defaults to PCDs
>>   OvmfPkg/PlatformPei: increase memory type info defaults
>>
>>  OvmfPkg/OvmfPkgIa32.dsc             |  12 ++
>>  OvmfPkg/OvmfPkgIa32X64.dsc          |  13 ++
>>  OvmfPkg/OvmfPkgX64.dsc              |  12 ++
>>  OvmfPkg/PlatformPei/MemTypeInfo.c   | 184 +++++++++++++-------
>>  OvmfPkg/PlatformPei/PlatformPei.inf |   6 +
>>  5 files changed, 166 insertions(+), 61 deletions(-)
>>
> 
> 
> 
> 


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 0/4] OvmfPkg/PlatformPei: rewrite MemTypeInfo HOB production logic
  2020-05-08 12:16 [PATCH 0/4] OvmfPkg/PlatformPei: rewrite MemTypeInfo HOB production logic Laszlo Ersek
                   ` (4 preceding siblings ...)
  2020-05-12 15:19 ` [edk2-devel] [PATCH 0/4] OvmfPkg/PlatformPei: rewrite MemTypeInfo HOB production logic Laszlo Ersek
@ 2020-05-15 17:19 ` Ard Biesheuvel
  2020-05-18 16:09 ` [edk2-devel] " Laszlo Ersek
  6 siblings, 0 replies; 11+ messages in thread
From: Ard Biesheuvel @ 2020-05-15 17:19 UTC (permalink / raw)
  To: Laszlo Ersek, edk2-devel-groups-io
  Cc: Jiewen Yao, Jordan Justen, Philippe Mathieu-Daudé

On 5/8/20 2:16 PM, Laszlo Ersek wrote:
> Ref:    https://bugzilla.tianocore.org/show_bug.cgi?id=2706
> Repo:   https://pagure.io/lersek/edk2.git
> Branch: memtypeinfo_rework
> 
> Please find the problem statement and the solution outline in the
> Bugzilla ticket linked above.
> 
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> 
> Thanks,
> Laszlo
> 
> Laszlo Ersek (4):
>    OvmfPkg/PlatformPei: don't track BS Code/Data in default MemTypeInfo
>      HOB
>    OvmfPkg/PlatformPei: rewrite MemTypeInfo HOB production logic
>    OvmfPkg/PlatformPei: extract memory type info defaults to PCDs
>    OvmfPkg/PlatformPei: increase memory type info defaults
> 

For the series,

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@arm.com>

>   OvmfPkg/OvmfPkgIa32.dsc             |  12 ++
>   OvmfPkg/OvmfPkgIa32X64.dsc          |  13 ++
>   OvmfPkg/OvmfPkgX64.dsc              |  12 ++
>   OvmfPkg/PlatformPei/MemTypeInfo.c   | 184 +++++++++++++-------
>   OvmfPkg/PlatformPei/PlatformPei.inf |   6 +
>   5 files changed, 166 insertions(+), 61 deletions(-)
> 


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 3/4] OvmfPkg/PlatformPei: extract memory type info defaults to PCDs
  2020-05-08 12:16 ` [PATCH 3/4] OvmfPkg/PlatformPei: extract memory type info defaults to PCDs Laszlo Ersek
@ 2020-05-15 17:40   ` Philippe Mathieu-Daudé
  0 siblings, 0 replies; 11+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-05-15 17:40 UTC (permalink / raw)
  To: Laszlo Ersek, edk2-devel-groups-io
  Cc: Ard Biesheuvel, Jiewen Yao, Jordan Justen

On 5/8/20 2:16 PM, Laszlo Ersek wrote:
> Some OvmfPkg modules already depend on "EmbeddedPkg.dec"; thus, replace
> the open-coded memory type info defaults in the source code with the
> EmbeddedPkg PCDs that stand for the same purpose. Consequently, platform
> builders can override these values with the "--pcd" option of "build",
> without source code updates.
> 
> While at it, sort the memory type names alphabetically.
> 
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2706
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>   OvmfPkg/OvmfPkgIa32.dsc             | 12 +++++++++++
>   OvmfPkg/OvmfPkgIa32X64.dsc          | 13 ++++++++++++
>   OvmfPkg/OvmfPkgX64.dsc              | 12 +++++++++++
>   OvmfPkg/PlatformPei/PlatformPei.inf |  6 ++++++
>   OvmfPkg/PlatformPei/MemTypeInfo.c   | 21 +++++++++-----------
>   5 files changed, 52 insertions(+), 12 deletions(-)
> 
> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
> index 7c8b51f43b66..a1981f8085a6 100644
> --- a/OvmfPkg/OvmfPkgIa32.dsc
> +++ b/OvmfPkg/OvmfPkgIa32.dsc
> @@ -532,6 +532,18 @@ [PcdsFixedAtBuild]
>     gEfiSourceLevelDebugPkgTokenSpaceGuid.PcdDebugLoadImageMethod|0x2
>   !endif
>   
> +  #
> +  # The NumberOfPages values below are ad-hoc. They are updated sporadically at
> +  # best (please refer to git-blame for past updates). The values capture a set
> +  # of BIN hints that made sense at a particular time, for some (now likely
> +  # unknown) workloads / boot paths.
> +  #
> +  gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIMemoryNVS|0x4
> +  gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIReclaimMemory|0x8
> +  gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiReservedMemoryType|0x4
> +  gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiRuntimeServicesCode|0x30
> +  gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiRuntimeServicesData|0x24
> +
>     #
>     # Network Pcds
>     #
> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
> index a0596c44168c..c1950e6aa915 100644
> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
> @@ -536,6 +536,19 @@ [PcdsFixedAtBuild]
>     gEfiSourceLevelDebugPkgTokenSpaceGuid.PcdDebugLoadImageMethod|0x2
>   !endif
>   
> +[PcdsFixedAtBuild.IA32]
> +  #
> +  # The NumberOfPages values below are ad-hoc. They are updated sporadically at
> +  # best (please refer to git-blame for past updates). The values capture a set
> +  # of BIN hints that made sense at a particular time, for some (now likely
> +  # unknown) workloads / boot paths.
> +  #
> +  gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIMemoryNVS|0x4
> +  gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIReclaimMemory|0x8
> +  gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiReservedMemoryType|0x4
> +  gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiRuntimeServicesCode|0x30
> +  gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiRuntimeServicesData|0x24
> +
>   [PcdsFixedAtBuild.X64]
>     #
>     # Network Pcds
> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> index 2e764b6b7233..bc7e15f749ca 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -536,6 +536,18 @@ [PcdsFixedAtBuild]
>     gEfiSourceLevelDebugPkgTokenSpaceGuid.PcdDebugLoadImageMethod|0x2
>   !endif
>   
> +  #
> +  # The NumberOfPages values below are ad-hoc. They are updated sporadically at
> +  # best (please refer to git-blame for past updates). The values capture a set
> +  # of BIN hints that made sense at a particular time, for some (now likely
> +  # unknown) workloads / boot paths.
> +  #
> +  gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIMemoryNVS|0x4
> +  gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIReclaimMemory|0x8
> +  gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiReservedMemoryType|0x4
> +  gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiRuntimeServicesCode|0x30
> +  gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiRuntimeServicesData|0x24
> +
>     #
>     # Network Pcds
>     #
> diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf b/OvmfPkg/PlatformPei/PlatformPei.inf
> index e72ef7963d97..ff397b3ee9d7 100644
> --- a/OvmfPkg/PlatformPei/PlatformPei.inf
> +++ b/OvmfPkg/PlatformPei/PlatformPei.inf
> @@ -37,6 +37,7 @@ [Sources]
>     Xen.h
>   
>   [Packages]
> +  EmbeddedPkg/EmbeddedPkg.dec
>     MdePkg/MdePkg.dec
>     MdeModulePkg/MdeModulePkg.dec
>     SecurityPkg/SecurityPkg.dec
> @@ -105,6 +106,11 @@ [Pcd]
>   
>   [FixedPcd]
>     gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress
> +  gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIMemoryNVS
> +  gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIReclaimMemory
> +  gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiReservedMemoryType
> +  gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiRuntimeServicesCode
> +  gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiRuntimeServicesData
>   
>   [FeaturePcd]
>     gUefiOvmfPkgTokenSpaceGuid.PcdCsmEnable
> diff --git a/OvmfPkg/PlatformPei/MemTypeInfo.c b/OvmfPkg/PlatformPei/MemTypeInfo.c
> index d287fb9d7df0..f3ce2b6865e1 100644
> --- a/OvmfPkg/PlatformPei/MemTypeInfo.c
> +++ b/OvmfPkg/PlatformPei/MemTypeInfo.c
> @@ -17,19 +17,16 @@
>   
>   #include "Platform.h"
>   
> -//
> -// The NumberOfPages values below are ad-hoc. They are updated sporadically at
> -// best (please refer to git-blame for past updates). The values capture a set
> -// of BIN hints that made sense at a particular time, for some (now likely
> -// unknown) workloads / boot paths.
> -//
> +#define MEMORY_TYPE_INFO_DEFAULT(Type) \
> +  { Type, FixedPcdGet32 (PcdMemoryType ## Type) }
> +
>   STATIC EFI_MEMORY_TYPE_INFORMATION mMemoryTypeInformation[] = {
> -  { EfiACPIMemoryNVS,       0x004 },
> -  { EfiACPIReclaimMemory,   0x008 },
> -  { EfiReservedMemoryType,  0x004 },
> -  { EfiRuntimeServicesData, 0x024 },
> -  { EfiRuntimeServicesCode, 0x030 },
> -  { EfiMaxMemoryType,       0x000 }
> +  MEMORY_TYPE_INFO_DEFAULT (EfiACPIMemoryNVS),
> +  MEMORY_TYPE_INFO_DEFAULT (EfiACPIReclaimMemory),
> +  MEMORY_TYPE_INFO_DEFAULT (EfiReservedMemoryType),
> +  MEMORY_TYPE_INFO_DEFAULT (EfiRuntimeServicesCode),
> +  MEMORY_TYPE_INFO_DEFAULT (EfiRuntimeServicesData),
> +  { EfiMaxMemoryType, 0 }
>   };
>   
>   STATIC
> 

Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/4] OvmfPkg/PlatformPei: don't track BS Code/Data in default MemTypeInfo HOB
  2020-05-08 12:16 ` [PATCH 1/4] OvmfPkg/PlatformPei: don't track BS Code/Data in default MemTypeInfo HOB Laszlo Ersek
@ 2020-05-15 17:55   ` Philippe Mathieu-Daudé
  0 siblings, 0 replies; 11+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-05-15 17:55 UTC (permalink / raw)
  To: Laszlo Ersek, edk2-devel-groups-io
  Cc: Ard Biesheuvel, Jiewen Yao, Jordan Justen

On 5/8/20 2:16 PM, Laszlo Ersek wrote:
> In commit d42fdd6f8384 ("OvmfPkg: improve SMM comms security with adaptive
> MemoryTypeInformation", 2020-03-12), we enabled the boot-to-boot tracking
> of the usages of various UEFI memory types.
> 
> Both whitepapers listed in that commit recommend that BS Code/Data type
> memory *not* be tracked. This recommendation was confirmed by Jiewen in
> the following two messages as well:
> 
> [1] https://edk2.groups.io/g/devel/message/55741
>      http://mid.mail-archive.com/74D8A39837DF1E4DA445A8C0B3885C503F97B579@shsmsx102.ccr.corp.intel.com
> 
> [2] https://edk2.groups.io/g/devel/message/55749
>      http://mid.mail-archive.com/74D8A39837DF1E4DA445A8C0B3885C503F97BDC5@shsmsx102.ccr.corp.intel.com
> 
> While tracking BS Code/Data type memory has one benefit (it de-fragments
> the UEFI memory map), the downsides outweigh it. Spikes in BS Data type
> memory usage are not uncommon in particular, and they may have the
> following consequences:
> 
> - such reboots during normal boot that look "spurious" to the end user,
>    and have no SMM security benefit,
> 
> - a large BS Data record in MemoryTypeInformation may cause issues when
>    the DXE Core tries to prime the according bin(s), but the system's RAM
>    size has been reduced meanwhile.
> 
> Removing the BS Code/Data entries from MemoryTypeInformation leads to a
> bit more fragmentation in the UEFI memory map, but that should be
> harmless.
> 
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2706
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>   OvmfPkg/PlatformPei/MemTypeInfo.c | 2 --
>   1 file changed, 2 deletions(-)
> 
> diff --git a/OvmfPkg/PlatformPei/MemTypeInfo.c b/OvmfPkg/PlatformPei/MemTypeInfo.c
> index 863c6f382680..8100a2db7d44 100644
> --- a/OvmfPkg/PlatformPei/MemTypeInfo.c
> +++ b/OvmfPkg/PlatformPei/MemTypeInfo.c
> @@ -31,8 +31,6 @@ STATIC EFI_MEMORY_TYPE_INFORMATION mDefaultMemoryTypeInformation[] = {
>     { EfiReservedMemoryType,  0x004 },
>     { EfiRuntimeServicesData, 0x024 },
>     { EfiRuntimeServicesCode, 0x030 },
> -  { EfiBootServicesCode,    0x180 },
> -  { EfiBootServicesData,    0xF00 },
>     { EfiMaxMemoryType,       0x000 }
>   };
>   
> 

Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [edk2-devel] [PATCH 0/4] OvmfPkg/PlatformPei: rewrite MemTypeInfo HOB production logic
  2020-05-08 12:16 [PATCH 0/4] OvmfPkg/PlatformPei: rewrite MemTypeInfo HOB production logic Laszlo Ersek
                   ` (5 preceding siblings ...)
  2020-05-15 17:19 ` Ard Biesheuvel
@ 2020-05-18 16:09 ` Laszlo Ersek
  6 siblings, 0 replies; 11+ messages in thread
From: Laszlo Ersek @ 2020-05-18 16:09 UTC (permalink / raw)
  To: edk2-devel-groups-io
  Cc: Ard Biesheuvel, Jiewen Yao, Jordan Justen,
	Philippe Mathieu-Daudé

On 05/08/20 14:16, Laszlo Ersek wrote:
> Ref:    https://bugzilla.tianocore.org/show_bug.cgi?id=2706
> Repo:   https://pagure.io/lersek/edk2.git
> Branch: memtypeinfo_rework
> 
> Please find the problem statement and the solution outline in the
> Bugzilla ticket linked above.
> 
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> 
> Thanks,
> Laszlo
> 
> Laszlo Ersek (4):
>   OvmfPkg/PlatformPei: don't track BS Code/Data in default MemTypeInfo
>     HOB
>   OvmfPkg/PlatformPei: rewrite MemTypeInfo HOB production logic
>   OvmfPkg/PlatformPei: extract memory type info defaults to PCDs
>   OvmfPkg/PlatformPei: increase memory type info defaults
> 
>  OvmfPkg/OvmfPkgIa32.dsc             |  12 ++
>  OvmfPkg/OvmfPkgIa32X64.dsc          |  13 ++
>  OvmfPkg/OvmfPkgX64.dsc              |  12 ++
>  OvmfPkg/PlatformPei/MemTypeInfo.c   | 184 +++++++++++++-------
>  OvmfPkg/PlatformPei/PlatformPei.inf |   6 +
>  5 files changed, 166 insertions(+), 61 deletions(-)
> 

Thank you all for the reviews. Merged in commit range
9099dcbd61c8..7b6327ff03bb via <https://github.com/tianocore/edk2/pull/629>.

Laszlo


^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2020-05-18 16:10 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-05-08 12:16 [PATCH 0/4] OvmfPkg/PlatformPei: rewrite MemTypeInfo HOB production logic Laszlo Ersek
2020-05-08 12:16 ` [PATCH 1/4] OvmfPkg/PlatformPei: don't track BS Code/Data in default MemTypeInfo HOB Laszlo Ersek
2020-05-15 17:55   ` Philippe Mathieu-Daudé
2020-05-08 12:16 ` [PATCH 2/4] OvmfPkg/PlatformPei: rewrite MemTypeInfo HOB production logic Laszlo Ersek
2020-05-08 12:16 ` [PATCH 3/4] OvmfPkg/PlatformPei: extract memory type info defaults to PCDs Laszlo Ersek
2020-05-15 17:40   ` Philippe Mathieu-Daudé
2020-05-08 12:16 ` [PATCH 4/4] OvmfPkg/PlatformPei: increase memory type info defaults Laszlo Ersek
2020-05-12 15:19 ` [edk2-devel] [PATCH 0/4] OvmfPkg/PlatformPei: rewrite MemTypeInfo HOB production logic Laszlo Ersek
2020-05-15 10:10   ` Laszlo Ersek
2020-05-15 17:19 ` Ard Biesheuvel
2020-05-18 16:09 ` [edk2-devel] " Laszlo Ersek

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox