[PATCH 0/4] OvmfPkg/PlatformPei: fix two assertion failures with weird RAM sizes

[PATCH 0/4] OvmfPkg/PlatformPei: fix two assertion failures with weird RAM sizes

[Laszlo Ersek]

Repo:   https://github.com/lersek/edk2.git
Branch: exbar_mtrr_rhbz_1666941
Ref:    https://bugzilla.redhat.com/show_bug.cgi?id=1666941
Ref:    https://bugzilla.redhat.com/show_bug.cgi?id=1701710

When booting OVMF on QEMU with "weird" RAM sizes, QEMU's low-RAM split
logic can trigger two assertion failures in OVMF:

- conflict between PCIEXBAR (ECAM) and low-RAM, on q35,

- running out of variable MTRRs when marking the uncacheable MMIO range
  in 32-bit address space, on both i440fx and q35.

This series fixes both issues, by moving around the PCIEXBAR on q35, and
by truncating the size of the uncacheable 32-bit area to a power of two.
The latter idea was inspired by SeaBIOS.

Tested on both machine types, with the following memory sizes (all in
MB): 1025, 2815, 3583, 5120. On i440fx, the X64 build was used (without
SMM). On q35, the IA32 and IA32X64 builds were used (with SMM). Testing
included "/proc/mtrr" verification, 32-bit PCI MMIO aperture
verification, general dmesg checks, and my usual regression tests too
(ACPI S3, UEFI variable services, ...).

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>

Thanks
Laszlo

Laszlo Ersek (4):
  OvmfPkg/PlatformPei: assign PciSize on both i440fx/q35 branches
    explicitly
  OvmfPkg/PlatformPei: hoist PciBase assignment above the i440fx/q35
    branching
  OvmfPkg/PlatformPei: reorder the 32-bit PCI hole vs. the PCIEXBAR on
    q35
  OvmfPkg/PlatformPei: fix MTRR for low-RAM sizes that have many bits
    clear

 OvmfPkg/OvmfPkgIa32.dsc         |  5 +----
 OvmfPkg/OvmfPkgIa32X64.dsc      |  5 +----
 OvmfPkg/OvmfPkgX64.dsc          |  5 +----
 OvmfPkg/PlatformPei/MemDetect.c | 23 +++++++++++++++++---
 OvmfPkg/PlatformPei/Platform.c  | 14 +++++-------
 OvmfPkg/PlatformPei/Platform.h  |  2 ++
 6 files changed, 31 insertions(+), 23 deletions(-)

-- 
2.19.1.3.g30247aa5d201

[PATCH 1/4] OvmfPkg/PlatformPei: assign PciSize on both i440fx/q35 branches explicitly

[Laszlo Ersek]

In the MemMapInitialization() function, we currently have a common
PciSize assignment, shared between i440fx and q35. In order to simplify
the rest of this series, lift and duplicate the assignment identically to
both board-specific branches.

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1666941
Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1701710
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/PlatformPei/Platform.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
index 0876316eefbc..5e0a15484230 100644
--- a/OvmfPkg/PlatformPei/Platform.c
+++ b/OvmfPkg/PlatformPei/Platform.c
@@ -190,8 +190,10 @@ MemMapInitialization (
       ASSERT (TopOfLowRam <= PciExBarBase);
       ASSERT (PciExBarBase <= MAX_UINT32 - SIZE_256MB);
       PciBase = (UINT32)(PciExBarBase + SIZE_256MB);
+      PciSize = 0xFC000000 - PciBase;
     } else {
       PciBase = (TopOfLowRam < BASE_2GB) ? BASE_2GB : TopOfLowRam;
+      PciSize = 0xFC000000 - PciBase;
     }
 
     //
@@ -207,7 +209,6 @@ MemMapInitialization (
     // 0xFED20000    gap                          896 KB
     // 0xFEE00000    LAPIC                          1 MB
     //
-    PciSize = 0xFC000000 - PciBase;
     AddIoMemoryBaseSizeHob (PciBase, PciSize);
     PcdStatus = PcdSet64S (PcdPciMmio32Base, PciBase);
     ASSERT_RETURN_ERROR (PcdStatus);
-- 
2.19.1.3.g30247aa5d201

[PATCH 2/4] OvmfPkg/PlatformPei: hoist PciBase assignment above the i440fx/q35 branching

[Laszlo Ersek]

In the MemMapInitialization() function, we currently assign PciBase
different values, on both branches of the board type check. Hoist the
PciBase assignment from the i440fx branch in front of the "if". This is a
no-op for the i440fx branch. On the q35 branch, we overwrite this value,
hence the change is a no-op on q35 as well.

This is another refactoring for simplifying the rest of this series.

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1666941
Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1701710
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/PlatformPei/Platform.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
index 5e0a15484230..9c013613a1a0 100644
--- a/OvmfPkg/PlatformPei/Platform.c
+++ b/OvmfPkg/PlatformPei/Platform.c
@@ -181,6 +181,7 @@ MemMapInitialization (
 
     TopOfLowRam = GetSystemMemorySizeBelow4gb ();
     PciExBarBase = 0;
+    PciBase = (TopOfLowRam < BASE_2GB) ? BASE_2GB : TopOfLowRam;
     if (mHostBridgeDevId == INTEL_Q35_MCH_DEVICE_ID) {
       //
       // The MMCONFIG area is expected to fall between the top of low RAM and
@@ -192,7 +193,6 @@ MemMapInitialization (
       PciBase = (UINT32)(PciExBarBase + SIZE_256MB);
       PciSize = 0xFC000000 - PciBase;
     } else {
-      PciBase = (TopOfLowRam < BASE_2GB) ? BASE_2GB : TopOfLowRam;
       PciSize = 0xFC000000 - PciBase;
     }
 
-- 
2.19.1.3.g30247aa5d201

[PATCH 3/4] OvmfPkg/PlatformPei: reorder the 32-bit PCI hole vs. the PCIEXBAR on q35

[Laszlo Ersek]

Commit 7b8fe63561b4 ("OvmfPkg: PlatformPei: enable PCIEXBAR (aka MMCONFIG
/ ECAM) on Q35", 2016-03-10) claimed that,

  On Q35 machine types that QEMU intends to support in the long term, QEMU
  never lets the RAM below 4 GB exceed 2 GB.

Alas, this statement came from a misunderstanding that occurred while we
worked out the interface contract. In fact QEMU does allow the 32-bit RAM
extend up to 0xB000_0000 (exclusive), in case the RAM size falls in the
range (0x8000_0000, 0xB000_0000) (i.e., the RAM size is greater than
2048MB and smaller than 2816MB).

In turn, such a RAM size (justifiedly) triggers

  ASSERT (TopOfLowRam <= PciExBarBase);

in MemMapInitialization(), because we placed the 256MB PCIEXBAR at
0x8000_0000 (2GB) exactly, relying on the interface contract. (And, the
32-bit PCI hole would follow the PCIEXBAR, covering the [0x9000_0000,
0xFC00_0000) range.)

In order to fix this, reorder the 32-bit PCI hole against the PCIEXBAR, as
follows:

- start the 32-bit PCI hole where it starts on i440fx as well, that is, at
  2GB or TopOfLowRam, whichever is higher;

- unlike on i440fx, where the 32-bit PCI hole extends up to 0xFC00_0000,
  stop it at 0xE000_0000 on q35,

- place the PCIEXBAR at 0xE000_0000.

(We cannot place the PCIEXBAR at 0xF000_0000 because the 256MB MMIO area
that starts there is not entirely free.)

Before this patch, the 32-bit PCI hole used to only *end* at the same spot
(namely, 0xFC00_0000) between i440fx and q35; now it will only *start* at
the same spot (namely, 2GB or TopOfLowRam, whichever is higher) between
both boards.

On q35, the maximal hole shrinks from

  0xFC00_0000 - 0x9000_0000 = 0x6C00_0000 == 1728 MB

to

  0xE000_0000 - 0x8000_0000 == 1536 MB.

We lose 192 MB of the aperture; however, the aperture is now aligned at
1GB, rather than 256 MB, and so it could fit a 1GB BAR even.

Regarding the minimal hole (triggered by RAM size 2815MB), its size is

  0xE000_0000 - 0xAFF0_0000 = 769 MB

which is not great, but probably better than a failed ASSERT.

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1666941
Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1701710
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/OvmfPkgIa32.dsc        | 5 +----
 OvmfPkg/OvmfPkgIa32X64.dsc     | 5 +----
 OvmfPkg/OvmfPkgX64.dsc         | 5 +----
 OvmfPkg/PlatformPei/Platform.c | 9 ++++-----
 4 files changed, 7 insertions(+), 17 deletions(-)

diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
index 36a0f87258dd..bb55b6fa58e1 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -491,10 +491,7 @@ [PcdsFixedAtBuild]
   # This PCD is used to set the base address of the PCI express hierarchy. It
   # is only consulted when OVMF runs on Q35. In that case it is programmed into
   # the PCIEXBAR register.
-  #
-  # On Q35 machine types that QEMU intends to support in the long term, QEMU
-  # never lets the RAM below 4 GB exceed 2 GB.
-  gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0x80000000
+  gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0xE0000000
 
 !ifdef $(SOURCE_DEBUG_ENABLE)
   gEfiSourceLevelDebugPkgTokenSpaceGuid.PcdDebugLoadImageMethod|0x2
diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index 9b341e17d7ff..06c394a6fb1f 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -496,10 +496,7 @@ [PcdsFixedAtBuild]
   # This PCD is used to set the base address of the PCI express hierarchy. It
   # is only consulted when OVMF runs on Q35. In that case it is programmed into
   # the PCIEXBAR register.
-  #
-  # On Q35 machine types that QEMU intends to support in the long term, QEMU
-  # never lets the RAM below 4 GB exceed 2 GB.
-  gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0x80000000
+  gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0xE0000000
 
 !ifdef $(SOURCE_DEBUG_ENABLE)
   gEfiSourceLevelDebugPkgTokenSpaceGuid.PcdDebugLoadImageMethod|0x2
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index a0f87f74dab9..5e0eb043fab9 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -496,10 +496,7 @@ [PcdsFixedAtBuild]
   # This PCD is used to set the base address of the PCI express hierarchy. It
   # is only consulted when OVMF runs on Q35. In that case it is programmed into
   # the PCIEXBAR register.
-  #
-  # On Q35 machine types that QEMU intends to support in the long term, QEMU
-  # never lets the RAM below 4 GB exceed 2 GB.
-  gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0x80000000
+  gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0xE0000000
 
 !ifdef $(SOURCE_DEBUG_ENABLE)
   gEfiSourceLevelDebugPkgTokenSpaceGuid.PcdDebugLoadImageMethod|0x2
diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
index 9c013613a1a0..fd8eccaf3e50 100644
--- a/OvmfPkg/PlatformPei/Platform.c
+++ b/OvmfPkg/PlatformPei/Platform.c
@@ -184,14 +184,13 @@ MemMapInitialization (
     PciBase = (TopOfLowRam < BASE_2GB) ? BASE_2GB : TopOfLowRam;
     if (mHostBridgeDevId == INTEL_Q35_MCH_DEVICE_ID) {
       //
-      // The MMCONFIG area is expected to fall between the top of low RAM and
-      // the base of the 32-bit PCI host aperture.
+      // The 32-bit PCI host aperture is expected to fall between the top of
+      // low RAM and the base of the MMCONFIG area.
       //
       PciExBarBase = FixedPcdGet64 (PcdPciExpressBaseAddress);
-      ASSERT (TopOfLowRam <= PciExBarBase);
+      ASSERT (PciBase < PciExBarBase);
       ASSERT (PciExBarBase <= MAX_UINT32 - SIZE_256MB);
-      PciBase = (UINT32)(PciExBarBase + SIZE_256MB);
-      PciSize = 0xFC000000 - PciBase;
+      PciSize = (UINT32)(PciExBarBase - PciBase);
     } else {
       PciSize = 0xFC000000 - PciBase;
     }
-- 
2.19.1.3.g30247aa5d201

[PATCH 4/4] OvmfPkg/PlatformPei: fix MTRR for low-RAM sizes that have many bits clear

[Laszlo Ersek]

Assume that we boot OVMF in a QEMU guest with 1025 MB of RAM. The
following assertion will fire:

> ASSERT_EFI_ERROR (Status = Out of Resources)
> ASSERT OvmfPkg/PlatformPei/MemDetect.c(696): !EFI_ERROR (Status)

That's because the range [1025 MB, 4 GB) that we try to mark as
uncacheable with MTRRs has size 3071 MB:

   0x1_0000_0000
  -0x0_4010_0000
  --------------
   0x0_BFF0_0000

The integer that stands for the uncacheable area size has 11 (eleven) bits
set to 1. As a result, covering this size requires 11 variable MTRRs (each
MTRR must cover a naturally aligned, power-of-two sized area). But, if we
need more variable MTRRs than the CPU can muster (such as 8), then
MtrrSetMemoryAttribute() fails, and we refuse to continue booting (which
is justified, in itself).

Unfortunately, this is not difficult to trigger, and the error message is
well-hidden from end-users, in the OVMF debug log. The following
mitigation is inspired by SeaBIOS:

Truncate the uncacheable area size to a power-of-two, while keeping the
end fixed at 4 GB. Such an interval can be covered by just one variable
MTRR.

This may leave such an MMIO gap, between the end of low-RAM and the start
of the uncacheable area, that is marked as WB (through the MTRR default).
Raise the base of the 32-bit PCI MMIO aperture accordingly -- the gap will
not be used for anything.

On Q35, the minimal 32-bit PCI MMIO aperture (triggered by RAM size 2815
MB) shrinks from

  0xE000_0000 - 0xAFF0_0000 = 769 MB

to

  0xE000_0000 - 0xC000_0000 = 512 MB

On i440fx, the minimal 32-bit PCI MMIO aperture (triggered by RAM size
3583 MB) shrinks from

  0xFC00_0000 - 0xDFF0_0000 = 449 MB

to

  0xFC00_0000 - 0xE000_0000 = 448 MB

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1666941
Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1701710
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/PlatformPei/Platform.h  |  2 ++
 OvmfPkg/PlatformPei/MemDetect.c | 23 +++++++++++++++++---
 OvmfPkg/PlatformPei/Platform.c  |  4 +---
 3 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/OvmfPkg/PlatformPei/Platform.h b/OvmfPkg/PlatformPei/Platform.h
index 81af8b71480f..4476ddd871cd 100644
--- a/OvmfPkg/PlatformPei/Platform.h
+++ b/OvmfPkg/PlatformPei/Platform.h
@@ -114,4 +114,6 @@ extern UINT32 mMaxCpuCount;
 
 extern UINT16 mHostBridgeDevId;
 
+extern UINT32 mQemuUc32Base;
+
 #endif // _PLATFORM_PEI_H_INCLUDED_
diff --git a/OvmfPkg/PlatformPei/MemDetect.c b/OvmfPkg/PlatformPei/MemDetect.c
index e890e36408a6..ae73c63d27d5 100644
--- a/OvmfPkg/PlatformPei/MemDetect.c
+++ b/OvmfPkg/PlatformPei/MemDetect.c
@@ -42,6 +42,8 @@ STATIC UINT32 mS3AcpiReservedMemorySize;
 
 STATIC UINT16 mQ35TsegMbytes;
 
+UINT32 mQemuUc32Base;
+
 VOID
 Q35TsegMbytesInitialization (
   VOID
@@ -663,6 +665,8 @@ QemuInitializeRam (
   // cover it exactly.
   //
   if (IsMtrrSupported ()) {
+    UINT32 Uc32Size;
+
     MtrrGetAllMtrrs (&MtrrSettings);
 
     //
@@ -689,11 +693,24 @@ QemuInitializeRam (
 
     //
     // Set memory range from the "top of lower RAM" (RAM below 4GB) to 4GB as
-    // uncacheable
+    // uncacheable. Make sure one variable MTRR suffices by truncating the size
+    // to a whole power of two. This will round the base *up*, and a gap (not
+    // used for either RAM or MMIO) may stay in the middle, marked as
+    // cacheable-by-default.
     //
-    Status = MtrrSetMemoryAttribute (LowerMemorySize,
-               SIZE_4GB - LowerMemorySize, CacheUncacheable);
+    Uc32Size = GetPowerOfTwo32 ((UINT32)(SIZE_4GB - LowerMemorySize));
+    mQemuUc32Base = (UINT32)(SIZE_4GB - Uc32Size);
+    if (mQemuUc32Base != LowerMemorySize) {
+      DEBUG ((DEBUG_VERBOSE, "%a: rounded UC32 base from 0x%x up to 0x%x, for "
+        "an UC32 size of 0x%x\n", __FUNCTION__, (UINT32)LowerMemorySize,
+        mQemuUc32Base, Uc32Size));
+    }
+
+    Status = MtrrSetMemoryAttribute (mQemuUc32Base, Uc32Size,
+               CacheUncacheable);
     ASSERT_EFI_ERROR (Status);
+  } else {
+    mQemuUc32Base = (UINT32)LowerMemorySize;
   }
 }
 
diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
index fd8eccaf3e50..c064b4ed9b8f 100644
--- a/OvmfPkg/PlatformPei/Platform.c
+++ b/OvmfPkg/PlatformPei/Platform.c
@@ -174,14 +174,12 @@ MemMapInitialization (
   AddIoMemoryRangeHob (0x0A0000, BASE_1MB);
 
   if (!mXen) {
-    UINT32  TopOfLowRam;
     UINT64  PciExBarBase;
     UINT32  PciBase;
     UINT32  PciSize;
 
-    TopOfLowRam = GetSystemMemorySizeBelow4gb ();
     PciExBarBase = 0;
-    PciBase = (TopOfLowRam < BASE_2GB) ? BASE_2GB : TopOfLowRam;
+    PciBase = (mQemuUc32Base < BASE_2GB) ? BASE_2GB : mQemuUc32Base;
     if (mHostBridgeDevId == INTEL_Q35_MCH_DEVICE_ID) {
       //
       // The 32-bit PCI host aperture is expected to fall between the top of
-- 
2.19.1.3.g30247aa5d201

Re: [edk2-devel] [PATCH 1/4] OvmfPkg/PlatformPei: assign PciSize on both i440fx/q35 branches explicitly

[Philippe Mathieu-Daudé]

On 5/4/19 2:07 AM, Laszlo Ersek wrote:
> In the MemMapInitialization() function, we currently have a common
> PciSize assignment, shared between i440fx and q35. In order to simplify
> the rest of this series, lift and duplicate the assignment identically to
> both board-specific branches.
> 
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1666941
> Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1701710
> Contributed-under: TianoCore Contribution Agreement 1.1

I guess you can now drop the 'Contributed-under' tag from your git
config. It has been removed from the wiki:
https://github.com/tianocore/tianocore.github.io/wiki/Commit-Message-Format/_compare/03d738e...ee27b0b

> Signed-off-by: Laszlo Ersek <lersek@redhat.com>

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

> ---
>  OvmfPkg/PlatformPei/Platform.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
> index 0876316eefbc..5e0a15484230 100644
> --- a/OvmfPkg/PlatformPei/Platform.c
> +++ b/OvmfPkg/PlatformPei/Platform.c
> @@ -190,8 +190,10 @@ MemMapInitialization (
>        ASSERT (TopOfLowRam <= PciExBarBase);
>        ASSERT (PciExBarBase <= MAX_UINT32 - SIZE_256MB);
>        PciBase = (UINT32)(PciExBarBase + SIZE_256MB);
> +      PciSize = 0xFC000000 - PciBase;
>      } else {
>        PciBase = (TopOfLowRam < BASE_2GB) ? BASE_2GB : TopOfLowRam;
> +      PciSize = 0xFC000000 - PciBase;
>      }
>  
>      //
> @@ -207,7 +209,6 @@ MemMapInitialization (
>      // 0xFED20000    gap                          896 KB
>      // 0xFEE00000    LAPIC                          1 MB
>      //
> -    PciSize = 0xFC000000 - PciBase;
>      AddIoMemoryBaseSizeHob (PciBase, PciSize);
>      PcdStatus = PcdSet64S (PcdPciMmio32Base, PciBase);
>      ASSERT_RETURN_ERROR (PcdStatus);
> 

Re: [edk2-devel] [PATCH 2/4] OvmfPkg/PlatformPei: hoist PciBase assignment above the i440fx/q35 branching

[Philippe Mathieu-Daudé]

On 5/4/19 2:07 AM, Laszlo Ersek wrote:
> In the MemMapInitialization() function, we currently assign PciBase
> different values, on both branches of the board type check. Hoist the
> PciBase assignment from the i440fx branch in front of the "if". This is a
> no-op for the i440fx branch. On the q35 branch, we overwrite this value,
> hence the change is a no-op on q35 as well.
> 
> This is another refactoring for simplifying the rest of this series.
> 
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1666941
> Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1701710
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>

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

> ---
>  OvmfPkg/PlatformPei/Platform.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
> index 5e0a15484230..9c013613a1a0 100644
> --- a/OvmfPkg/PlatformPei/Platform.c
> +++ b/OvmfPkg/PlatformPei/Platform.c
> @@ -181,6 +181,7 @@ MemMapInitialization (
>  
>      TopOfLowRam = GetSystemMemorySizeBelow4gb ();
>      PciExBarBase = 0;
> +    PciBase = (TopOfLowRam < BASE_2GB) ? BASE_2GB : TopOfLowRam;
>      if (mHostBridgeDevId == INTEL_Q35_MCH_DEVICE_ID) {
>        //
>        // The MMCONFIG area is expected to fall between the top of low RAM and
> @@ -192,7 +193,6 @@ MemMapInitialization (
>        PciBase = (UINT32)(PciExBarBase + SIZE_256MB);
>        PciSize = 0xFC000000 - PciBase;
>      } else {
> -      PciBase = (TopOfLowRam < BASE_2GB) ? BASE_2GB : TopOfLowRam;
>        PciSize = 0xFC000000 - PciBase;
>      }
>  
> 

Re: [edk2-devel] [PATCH 1/4] OvmfPkg/PlatformPei: assign PciSize on both i440fx/q35 branches explicitly

[Laszlo Ersek]

On 05/06/19 13:06, Philippe Mathieu-Daudé wrote:
> On 5/4/19 2:07 AM, Laszlo Ersek wrote:
>> In the MemMapInitialization() function, we currently have a common
>> PciSize assignment, shared between i440fx and q35. In order to simplify
>> the rest of this series, lift and duplicate the assignment identically to
>> both board-specific branches.
>>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Cc: Gerd Hoffmann <kraxel@redhat.com>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1666941
>> Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1701710
>> Contributed-under: TianoCore Contribution Agreement 1.1
> 
> I guess you can now drop the 'Contributed-under' tag from your git
> config. It has been removed from the wiki:
> https://github.com/tianocore/tianocore.github.io/wiki/Commit-Message-Format/_compare/03d738e...ee27b0b

Ah, good point; I missed this because the first three patches in the
series originate from Jan 2019. (I didn't post them back then because
they don't solve the whole problem without patch #4.) And in January my
edk2 commit message template file still included the Contributed-under line.

I'll drop the line when I push the patches, or (if a v2 is needed) when
I post v2.

Thanks!
Laszlo

> 
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> 
>> ---
>>  OvmfPkg/PlatformPei/Platform.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
>> index 0876316eefbc..5e0a15484230 100644
>> --- a/OvmfPkg/PlatformPei/Platform.c
>> +++ b/OvmfPkg/PlatformPei/Platform.c
>> @@ -190,8 +190,10 @@ MemMapInitialization (
>>        ASSERT (TopOfLowRam <= PciExBarBase);
>>        ASSERT (PciExBarBase <= MAX_UINT32 - SIZE_256MB);
>>        PciBase = (UINT32)(PciExBarBase + SIZE_256MB);
>> +      PciSize = 0xFC000000 - PciBase;
>>      } else {
>>        PciBase = (TopOfLowRam < BASE_2GB) ? BASE_2GB : TopOfLowRam;
>> +      PciSize = 0xFC000000 - PciBase;
>>      }
>>  
>>      //
>> @@ -207,7 +209,6 @@ MemMapInitialization (
>>      // 0xFED20000    gap                          896 KB
>>      // 0xFEE00000    LAPIC                          1 MB
>>      //
>> -    PciSize = 0xFC000000 - PciBase;
>>      AddIoMemoryBaseSizeHob (PciBase, PciSize);
>>      PcdStatus = PcdSet64S (PcdPciMmio32Base, PciBase);
>>      ASSERT_RETURN_ERROR (PcdStatus);
>>

Re: [edk2-devel] [PATCH 4/4] OvmfPkg/PlatformPei: fix MTRR for low-RAM sizes that have many bits clear

[Philippe Mathieu-Daudé]

On 5/4/19 2:07 AM, Laszlo Ersek wrote:
> Assume that we boot OVMF in a QEMU guest with 1025 MB of RAM. The
> following assertion will fire:
> 
>> ASSERT_EFI_ERROR (Status = Out of Resources)
>> ASSERT OvmfPkg/PlatformPei/MemDetect.c(696): !EFI_ERROR (Status)
> 
> That's because the range [1025 MB, 4 GB) that we try to mark as
> uncacheable with MTRRs has size 3071 MB:
> 
>    0x1_0000_0000
>   -0x0_4010_0000
>   --------------
>    0x0_BFF0_0000
> 
> The integer that stands for the uncacheable area size has 11 (eleven) bits
> set to 1. As a result, covering this size requires 11 variable MTRRs (each
> MTRR must cover a naturally aligned, power-of-two sized area). But, if we
> need more variable MTRRs than the CPU can muster (such as 8), then
> MtrrSetMemoryAttribute() fails, and we refuse to continue booting (which
> is justified, in itself).
> 
> Unfortunately, this is not difficult to trigger, and the error message is
> well-hidden from end-users, in the OVMF debug log. The following
> mitigation is inspired by SeaBIOS:
> 
> Truncate the uncacheable area size to a power-of-two, while keeping the
> end fixed at 4 GB. Such an interval can be covered by just one variable
> MTRR.
> 
> This may leave such an MMIO gap, between the end of low-RAM and the start
> of the uncacheable area, that is marked as WB (through the MTRR default).
> Raise the base of the 32-bit PCI MMIO aperture accordingly -- the gap will
> not be used for anything.

I had to draw it to be sure I understood correctly:

+-------------+               +-------------+  <-- 4GB
|             |               |             |
|             |               |             |
|             |               |  PCI MMIO   |
|             |               |             |
|             |               | uncacheable |
| uncacheable |               |             |
|             |               |             |
|             |       ---->   +-------------+  <-- mQemuUc32Base
|             |      |        |             |      (pow2 aligned)
|             |      |        |     GAP     |
|             |      |        | (cacheable) |
+-------------+  ----         +-------------+  <-- TopOfLowRam
|             |               |             |    (not pow2 aligned)
|             |               |             |
|             |               |             |
|             |               |             |
| LowerMemory |               | LowerMemory |
| (cacheable) |               | (cacheable) |
|             |               |             |
|             |               |             |
|             |               |             |
+-------------+               +-------------+

> On Q35, the minimal 32-bit PCI MMIO aperture (triggered by RAM size 2815
> MB) shrinks from
> 
>   0xE000_0000 - 0xAFF0_0000 = 769 MB
> 
> to
> 
>   0xE000_0000 - 0xC000_0000 = 512 MB
> 
> On i440fx, the minimal 32-bit PCI MMIO aperture (triggered by RAM size
> 3583 MB) shrinks from
> 
>   0xFC00_0000 - 0xDFF0_0000 = 449 MB
> 
> to
> 
>   0xFC00_0000 - 0xE000_0000 = 448 MB
> 
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1666941
> Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1701710
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  OvmfPkg/PlatformPei/Platform.h  |  2 ++
>  OvmfPkg/PlatformPei/MemDetect.c | 23 +++++++++++++++++---
>  OvmfPkg/PlatformPei/Platform.c  |  4 +---
>  3 files changed, 23 insertions(+), 6 deletions(-)
> 
> diff --git a/OvmfPkg/PlatformPei/Platform.h b/OvmfPkg/PlatformPei/Platform.h
> index 81af8b71480f..4476ddd871cd 100644
> --- a/OvmfPkg/PlatformPei/Platform.h
> +++ b/OvmfPkg/PlatformPei/Platform.h
> @@ -114,4 +114,6 @@ extern UINT32 mMaxCpuCount;
>  
>  extern UINT16 mHostBridgeDevId;
>  
> +extern UINT32 mQemuUc32Base;
> +
>  #endif // _PLATFORM_PEI_H_INCLUDED_
> diff --git a/OvmfPkg/PlatformPei/MemDetect.c b/OvmfPkg/PlatformPei/MemDetect.c
> index e890e36408a6..ae73c63d27d5 100644
> --- a/OvmfPkg/PlatformPei/MemDetect.c
> +++ b/OvmfPkg/PlatformPei/MemDetect.c
> @@ -42,6 +42,8 @@ STATIC UINT32 mS3AcpiReservedMemorySize;
>  
>  STATIC UINT16 mQ35TsegMbytes;
>  
> +UINT32 mQemuUc32Base;
> +
>  VOID
>  Q35TsegMbytesInitialization (
>    VOID
> @@ -663,6 +665,8 @@ QemuInitializeRam (
>    // cover it exactly.
>    //
>    if (IsMtrrSupported ()) {
> +    UINT32 Uc32Size;
> +
>      MtrrGetAllMtrrs (&MtrrSettings);
>  
>      //
> @@ -689,11 +693,24 @@ QemuInitializeRam (
>  
>      //
>      // Set memory range from the "top of lower RAM" (RAM below 4GB) to 4GB as
> -    // uncacheable
> +    // uncacheable. Make sure one variable MTRR suffices by truncating the size
> +    // to a whole power of two. This will round the base *up*, and a gap (not
> +    // used for either RAM or MMIO) may stay in the middle, marked as
> +    // cacheable-by-default.
>      //
> -    Status = MtrrSetMemoryAttribute (LowerMemorySize,
> -               SIZE_4GB - LowerMemorySize, CacheUncacheable);
> +    Uc32Size = GetPowerOfTwo32 ((UINT32)(SIZE_4GB - LowerMemorySize));
> +    mQemuUc32Base = (UINT32)(SIZE_4GB - Uc32Size);
> +    if (mQemuUc32Base != LowerMemorySize) {
> +      DEBUG ((DEBUG_VERBOSE, "%a: rounded UC32 base from 0x%x up to 0x%x, for "
> +        "an UC32 size of 0x%x\n", __FUNCTION__, (UINT32)LowerMemorySize,
> +        mQemuUc32Base, Uc32Size));
> +    }
> +
> +    Status = MtrrSetMemoryAttribute (mQemuUc32Base, Uc32Size,
> +               CacheUncacheable);
>      ASSERT_EFI_ERROR (Status);
> +  } else {
> +    mQemuUc32Base = (UINT32)LowerMemorySize;
>    }
>  }
>  
> diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
> index fd8eccaf3e50..c064b4ed9b8f 100644
> --- a/OvmfPkg/PlatformPei/Platform.c
> +++ b/OvmfPkg/PlatformPei/Platform.c
> @@ -174,14 +174,12 @@ MemMapInitialization (
>    AddIoMemoryRangeHob (0x0A0000, BASE_1MB);
>  
>    if (!mXen) {
> -    UINT32  TopOfLowRam;
>      UINT64  PciExBarBase;
>      UINT32  PciBase;
>      UINT32  PciSize;
>  
> -    TopOfLowRam = GetSystemMemorySizeBelow4gb ();
>      PciExBarBase = 0;
> -    PciBase = (TopOfLowRam < BASE_2GB) ? BASE_2GB : TopOfLowRam;
> +    PciBase = (mQemuUc32Base < BASE_2GB) ? BASE_2GB : mQemuUc32Base;
>      if (mHostBridgeDevId == INTEL_Q35_MCH_DEVICE_ID) {
>        //
>        // The 32-bit PCI host aperture is expected to fall between the top of
> 

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

Re: [edk2-devel] [PATCH 4/4] OvmfPkg/PlatformPei: fix MTRR for low-RAM sizes that have many bits clear

[Laszlo Ersek]

Hi Phil,

On 05/08/19 09:33, Philippe Mathieu-Daudé wrote:
> On 5/4/19 2:07 AM, Laszlo Ersek wrote:
>> Assume that we boot OVMF in a QEMU guest with 1025 MB of RAM. The
>> following assertion will fire:
>>
>>> ASSERT_EFI_ERROR (Status = Out of Resources)
>>> ASSERT OvmfPkg/PlatformPei/MemDetect.c(696): !EFI_ERROR (Status)
>>
>> That's because the range [1025 MB, 4 GB) that we try to mark as
>> uncacheable with MTRRs has size 3071 MB:
>>
>>    0x1_0000_0000
>>   -0x0_4010_0000
>>   --------------
>>    0x0_BFF0_0000
>>
>> The integer that stands for the uncacheable area size has 11 (eleven) bits
>> set to 1. As a result, covering this size requires 11 variable MTRRs (each
>> MTRR must cover a naturally aligned, power-of-two sized area). But, if we
>> need more variable MTRRs than the CPU can muster (such as 8), then
>> MtrrSetMemoryAttribute() fails, and we refuse to continue booting (which
>> is justified, in itself).
>>
>> Unfortunately, this is not difficult to trigger, and the error message is
>> well-hidden from end-users, in the OVMF debug log. The following
>> mitigation is inspired by SeaBIOS:
>>
>> Truncate the uncacheable area size to a power-of-two, while keeping the
>> end fixed at 4 GB. Such an interval can be covered by just one variable
>> MTRR.
>>
>> This may leave such an MMIO gap, between the end of low-RAM and the start
>> of the uncacheable area, that is marked as WB (through the MTRR default).
>> Raise the base of the 32-bit PCI MMIO aperture accordingly -- the gap will
>> not be used for anything.
> 
> I had to draw it to be sure I understood correctly:
> 
> +-------------+               +-------------+  <-- 4GB
> |             |               |             |
> |             |               |             |
> |             |               |  PCI MMIO   |
> |             |               |             |
> |             |               | uncacheable |
> | uncacheable |               |             |
> |             |               |             |
> |             |       ---->   +-------------+  <-- mQemuUc32Base
> |             |      |        |             |      (pow2 aligned)
> |             |      |        |     GAP     |
> |             |      |        | (cacheable) |
> +-------------+  ----         +-------------+  <-- TopOfLowRam
> |             |               |             |    (not pow2 aligned)
> |             |               |             |
> |             |               |             |
> |             |               |             |
> | LowerMemory |               | LowerMemory |
> | (cacheable) |               | (cacheable) |
> |             |               |             |
> |             |               |             |
> |             |               |             |
> +-------------+               +-------------+

Correct. "mQemuUc32Base" is not itself a whole power of two, but it is
pow2 aligned, where the alignment must not be smaller than "size" *is*.

This natural alignment is ensured because 4GB is itself a power of two
(2^32). Thus, if "size" is 2^m, then

  mQemuUc32Base == (4GB - size) == (2^32 - 2^m) == 2^m * (2^(32-m) - 1)

and that is divisible by 2^m. (We know for sure that (m < 32).)
Therefore the natural alignment for the base is satisfied.

For example, consider base=3GB, size=1GB. Then m=30.

> 
>> On Q35, the minimal 32-bit PCI MMIO aperture (triggered by RAM size 2815
>> MB) shrinks from
>>
>>   0xE000_0000 - 0xAFF0_0000 = 769 MB
>>
>> to
>>
>>   0xE000_0000 - 0xC000_0000 = 512 MB
>>
>> On i440fx, the minimal 32-bit PCI MMIO aperture (triggered by RAM size
>> 3583 MB) shrinks from
>>
>>   0xFC00_0000 - 0xDFF0_0000 = 449 MB
>>
>> to
>>
>>   0xFC00_0000 - 0xE000_0000 = 448 MB
>>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Cc: Gerd Hoffmann <kraxel@redhat.com>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1666941
>> Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1701710
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>>  OvmfPkg/PlatformPei/Platform.h  |  2 ++
>>  OvmfPkg/PlatformPei/MemDetect.c | 23 +++++++++++++++++---
>>  OvmfPkg/PlatformPei/Platform.c  |  4 +---
>>  3 files changed, 23 insertions(+), 6 deletions(-)
>>
>> diff --git a/OvmfPkg/PlatformPei/Platform.h b/OvmfPkg/PlatformPei/Platform.h
>> index 81af8b71480f..4476ddd871cd 100644
>> --- a/OvmfPkg/PlatformPei/Platform.h
>> +++ b/OvmfPkg/PlatformPei/Platform.h
>> @@ -114,4 +114,6 @@ extern UINT32 mMaxCpuCount;
>>  
>>  extern UINT16 mHostBridgeDevId;
>>  
>> +extern UINT32 mQemuUc32Base;
>> +
>>  #endif // _PLATFORM_PEI_H_INCLUDED_
>> diff --git a/OvmfPkg/PlatformPei/MemDetect.c b/OvmfPkg/PlatformPei/MemDetect.c
>> index e890e36408a6..ae73c63d27d5 100644
>> --- a/OvmfPkg/PlatformPei/MemDetect.c
>> +++ b/OvmfPkg/PlatformPei/MemDetect.c
>> @@ -42,6 +42,8 @@ STATIC UINT32 mS3AcpiReservedMemorySize;
>>  
>>  STATIC UINT16 mQ35TsegMbytes;
>>  
>> +UINT32 mQemuUc32Base;
>> +
>>  VOID
>>  Q35TsegMbytesInitialization (
>>    VOID
>> @@ -663,6 +665,8 @@ QemuInitializeRam (
>>    // cover it exactly.
>>    //
>>    if (IsMtrrSupported ()) {
>> +    UINT32 Uc32Size;
>> +
>>      MtrrGetAllMtrrs (&MtrrSettings);
>>  
>>      //
>> @@ -689,11 +693,24 @@ QemuInitializeRam (
>>  
>>      //
>>      // Set memory range from the "top of lower RAM" (RAM below 4GB) to 4GB as
>> -    // uncacheable
>> +    // uncacheable. Make sure one variable MTRR suffices by truncating the size
>> +    // to a whole power of two. This will round the base *up*, and a gap (not
>> +    // used for either RAM or MMIO) may stay in the middle, marked as
>> +    // cacheable-by-default.
>>      //
>> -    Status = MtrrSetMemoryAttribute (LowerMemorySize,
>> -               SIZE_4GB - LowerMemorySize, CacheUncacheable);
>> +    Uc32Size = GetPowerOfTwo32 ((UINT32)(SIZE_4GB - LowerMemorySize));
>> +    mQemuUc32Base = (UINT32)(SIZE_4GB - Uc32Size);
>> +    if (mQemuUc32Base != LowerMemorySize) {
>> +      DEBUG ((DEBUG_VERBOSE, "%a: rounded UC32 base from 0x%x up to 0x%x, for "
>> +        "an UC32 size of 0x%x\n", __FUNCTION__, (UINT32)LowerMemorySize,
>> +        mQemuUc32Base, Uc32Size));
>> +    }
>> +
>> +    Status = MtrrSetMemoryAttribute (mQemuUc32Base, Uc32Size,
>> +               CacheUncacheable);
>>      ASSERT_EFI_ERROR (Status);
>> +  } else {
>> +    mQemuUc32Base = (UINT32)LowerMemorySize;
>>    }
>>  }
>>  
>> diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
>> index fd8eccaf3e50..c064b4ed9b8f 100644
>> --- a/OvmfPkg/PlatformPei/Platform.c
>> +++ b/OvmfPkg/PlatformPei/Platform.c
>> @@ -174,14 +174,12 @@ MemMapInitialization (
>>    AddIoMemoryRangeHob (0x0A0000, BASE_1MB);
>>  
>>    if (!mXen) {
>> -    UINT32  TopOfLowRam;
>>      UINT64  PciExBarBase;
>>      UINT32  PciBase;
>>      UINT32  PciSize;
>>  
>> -    TopOfLowRam = GetSystemMemorySizeBelow4gb ();
>>      PciExBarBase = 0;
>> -    PciBase = (TopOfLowRam < BASE_2GB) ? BASE_2GB : TopOfLowRam;
>> +    PciBase = (mQemuUc32Base < BASE_2GB) ? BASE_2GB : mQemuUc32Base;
>>      if (mHostBridgeDevId == INTEL_Q35_MCH_DEVICE_ID) {
>>        //
>>        // The 32-bit PCI host aperture is expected to fall between the top of
>>
> 
> Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>
> 

Thank you!
Laszlo

Re: [edk2-devel] [PATCH 0/4] OvmfPkg/PlatformPei: fix two assertion failures with weird RAM sizes

[Laszlo Ersek]

Hi Stewards,

it seems likely that this patch series -- posted on 2019-May-04 -- will
not receive the necessary maintainer A-b's / R-b's before we enter the
soft feature freeze for edk2-stable201905.

Therefore I'd like to state that I consider this series a bugfix series,
not a feature addition. I'm now asking for confirmation that I'll be
allowed to push the series -- dependent on the expected reviewer
feedback of course -- even after 2019-May-17 08:00:00 UTC.

Please also state whether I will need to open a TianoCore BZ for
tracking this work (and to update the commit messages accordingly). I
haven't done that because we already have two public BZs for the issue
in the Red Hat Bugzilla instance, with issue description and analysis.
(RHBZ references are captured in both the blurb below, and in the
patches themselves.)

Thanks
Laszlo

On 05/04/19 02:07, Laszlo Ersek wrote:
> Repo:   https://github.com/lersek/edk2.git
> Branch: exbar_mtrr_rhbz_1666941
> Ref:    https://bugzilla.redhat.com/show_bug.cgi?id=1666941
> Ref:    https://bugzilla.redhat.com/show_bug.cgi?id=1701710
> 
> When booting OVMF on QEMU with "weird" RAM sizes, QEMU's low-RAM split
> logic can trigger two assertion failures in OVMF:
> 
> - conflict between PCIEXBAR (ECAM) and low-RAM, on q35,
> 
> - running out of variable MTRRs when marking the uncacheable MMIO range
>   in 32-bit address space, on both i440fx and q35.
> 
> This series fixes both issues, by moving around the PCIEXBAR on q35, and
> by truncating the size of the uncacheable 32-bit area to a power of two.
> The latter idea was inspired by SeaBIOS.
> 
> Tested on both machine types, with the following memory sizes (all in
> MB): 1025, 2815, 3583, 5120. On i440fx, the X64 build was used (without
> SMM). On q35, the IA32 and IA32X64 builds were used (with SMM). Testing
> included "/proc/mtrr" verification, 32-bit PCI MMIO aperture
> verification, general dmesg checks, and my usual regression tests too
> (ACPI S3, UEFI variable services, ...).
> 
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> 
> Thanks
> Laszlo
> 
> Laszlo Ersek (4):
>   OvmfPkg/PlatformPei: assign PciSize on both i440fx/q35 branches
>     explicitly
>   OvmfPkg/PlatformPei: hoist PciBase assignment above the i440fx/q35
>     branching
>   OvmfPkg/PlatformPei: reorder the 32-bit PCI hole vs. the PCIEXBAR on
>     q35
>   OvmfPkg/PlatformPei: fix MTRR for low-RAM sizes that have many bits
>     clear
> 
>  OvmfPkg/OvmfPkgIa32.dsc         |  5 +----
>  OvmfPkg/OvmfPkgIa32X64.dsc      |  5 +----
>  OvmfPkg/OvmfPkgX64.dsc          |  5 +----
>  OvmfPkg/PlatformPei/MemDetect.c | 23 +++++++++++++++++---
>  OvmfPkg/PlatformPei/Platform.c  | 14 +++++-------
>  OvmfPkg/PlatformPei/Platform.h  |  2 ++
>  6 files changed, 31 insertions(+), 23 deletions(-)
> 

Re: [edk2-devel] [PATCH 0/4] OvmfPkg/PlatformPei: fix two assertion failures with weird RAM sizes

[Leif Lindholm]

On Wed, May 15, 2019 at 12:36:22PM +0200, Laszlo Ersek wrote:
> Hi Stewards,
> 
> it seems likely that this patch series -- posted on 2019-May-04 -- will
> not receive the necessary maintainer A-b's / R-b's before we enter the
> soft feature freeze for edk2-stable201905.
> 
> Therefore I'd like to state that I consider this series a bugfix series,
> not a feature addition. I'm now asking for confirmation that I'll be
> allowed to push the series -- dependent on the expected reviewer
> feedback of course -- even after 2019-May-17 08:00:00 UTC.

Clearly bugfix, and quite contained in scope as well.

> Please also state whether I will need to open a TianoCore BZ for
> tracking this work (and to update the commit messages accordingly). I
> haven't done that because we already have two public BZs for the issue
> in the Red Hat Bugzilla instance, with issue description and analysis.
> (RHBZ references are captured in both the blurb below, and in the
> patches themselves.)

I would lean towards having a TianoCore BZ for anything pushed in the
freeze period.

While I don't expect Red Hat to close its BZ from public access, it
does sort of open up the question of "well, which other BZs do we
consider trustworthy enough to give the same treatment?" that I would
prefer to avoid having to deal with during future freeze periods.

But if you (plural) feel I'm being overly cautious, I don't feel
*that* strongly about it.

Regards,

Leif

> Thanks
> Laszlo
> 
> On 05/04/19 02:07, Laszlo Ersek wrote:
> > Repo:   https://github.com/lersek/edk2.git
> > Branch: exbar_mtrr_rhbz_1666941
> > Ref:    https://bugzilla.redhat.com/show_bug.cgi?id=1666941
> > Ref:    https://bugzilla.redhat.com/show_bug.cgi?id=1701710
> > 
> > When booting OVMF on QEMU with "weird" RAM sizes, QEMU's low-RAM split
> > logic can trigger two assertion failures in OVMF:
> > 
> > - conflict between PCIEXBAR (ECAM) and low-RAM, on q35,
> > 
> > - running out of variable MTRRs when marking the uncacheable MMIO range
> >   in 32-bit address space, on both i440fx and q35.
> > 
> > This series fixes both issues, by moving around the PCIEXBAR on q35, and
> > by truncating the size of the uncacheable 32-bit area to a power of two.
> > The latter idea was inspired by SeaBIOS.
> > 
> > Tested on both machine types, with the following memory sizes (all in
> > MB): 1025, 2815, 3583, 5120. On i440fx, the X64 build was used (without
> > SMM). On q35, the IA32 and IA32X64 builds were used (with SMM). Testing
> > included "/proc/mtrr" verification, 32-bit PCI MMIO aperture
> > verification, general dmesg checks, and my usual regression tests too
> > (ACPI S3, UEFI variable services, ...).
> > 
> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > Cc: Gerd Hoffmann <kraxel@redhat.com>
> > Cc: Jordan Justen <jordan.l.justen@intel.com>
> > 
> > Thanks
> > Laszlo
> > 
> > Laszlo Ersek (4):
> >   OvmfPkg/PlatformPei: assign PciSize on both i440fx/q35 branches
> >     explicitly
> >   OvmfPkg/PlatformPei: hoist PciBase assignment above the i440fx/q35
> >     branching
> >   OvmfPkg/PlatformPei: reorder the 32-bit PCI hole vs. the PCIEXBAR on
> >     q35
> >   OvmfPkg/PlatformPei: fix MTRR for low-RAM sizes that have many bits
> >     clear
> > 
> >  OvmfPkg/OvmfPkgIa32.dsc         |  5 +----
> >  OvmfPkg/OvmfPkgIa32X64.dsc      |  5 +----
> >  OvmfPkg/OvmfPkgX64.dsc          |  5 +----
> >  OvmfPkg/PlatformPei/MemDetect.c | 23 +++++++++++++++++---
> >  OvmfPkg/PlatformPei/Platform.c  | 14 +++++-------
> >  OvmfPkg/PlatformPei/Platform.h  |  2 ++
> >  6 files changed, 31 insertions(+), 23 deletions(-)
> > 
> 

Re: [edk2-devel] [PATCH 0/4] OvmfPkg/PlatformPei: fix two assertion failures with weird RAM sizes

[Laszlo Ersek]

On 05/15/19 13:56, Leif Lindholm wrote:
> On Wed, May 15, 2019 at 12:36:22PM +0200, Laszlo Ersek wrote:
>> Hi Stewards,
>>
>> it seems likely that this patch series -- posted on 2019-May-04 -- will
>> not receive the necessary maintainer A-b's / R-b's before we enter the
>> soft feature freeze for edk2-stable201905.
>>
>> Therefore I'd like to state that I consider this series a bugfix series,
>> not a feature addition. I'm now asking for confirmation that I'll be
>> allowed to push the series -- dependent on the expected reviewer
>> feedback of course -- even after 2019-May-17 08:00:00 UTC.
> 
> Clearly bugfix, and quite contained in scope as well.
> 
>> Please also state whether I will need to open a TianoCore BZ for
>> tracking this work (and to update the commit messages accordingly). I
>> haven't done that because we already have two public BZs for the issue
>> in the Red Hat Bugzilla instance, with issue description and analysis.
>> (RHBZ references are captured in both the blurb below, and in the
>> patches themselves.)
> 
> I would lean towards having a TianoCore BZ for anything pushed in the
> freeze period.
> 
> While I don't expect Red Hat to close its BZ from public access, it
> does sort of open up the question of "well, which other BZs do we
> consider trustworthy enough to give the same treatment?" that I would
> prefer to avoid having to deal with during future freeze periods.

Good point; I've now filed
<https://bugzilla.tianocore.org/show_bug.cgi?id=1814>, and I'll
reference it in the commit messages, when I push the series.

Thanks!
Laszlo

> But if you (plural) feel I'm being overly cautious, I don't feel
> *that* strongly about it.
> 
> Regards,
> 
> Leif
> 
>> Thanks
>> Laszlo
>>
>> On 05/04/19 02:07, Laszlo Ersek wrote:
>>> Repo:   https://github.com/lersek/edk2.git
>>> Branch: exbar_mtrr_rhbz_1666941
>>> Ref:    https://bugzilla.redhat.com/show_bug.cgi?id=1666941
>>> Ref:    https://bugzilla.redhat.com/show_bug.cgi?id=1701710
>>>
>>> When booting OVMF on QEMU with "weird" RAM sizes, QEMU's low-RAM split
>>> logic can trigger two assertion failures in OVMF:
>>>
>>> - conflict between PCIEXBAR (ECAM) and low-RAM, on q35,
>>>
>>> - running out of variable MTRRs when marking the uncacheable MMIO range
>>>   in 32-bit address space, on both i440fx and q35.
>>>
>>> This series fixes both issues, by moving around the PCIEXBAR on q35, and
>>> by truncating the size of the uncacheable 32-bit area to a power of two.
>>> The latter idea was inspired by SeaBIOS.
>>>
>>> Tested on both machine types, with the following memory sizes (all in
>>> MB): 1025, 2815, 3583, 5120. On i440fx, the X64 build was used (without
>>> SMM). On q35, the IA32 and IA32X64 builds were used (with SMM). Testing
>>> included "/proc/mtrr" verification, 32-bit PCI MMIO aperture
>>> verification, general dmesg checks, and my usual regression tests too
>>> (ACPI S3, UEFI variable services, ...).
>>>
>>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> Cc: Gerd Hoffmann <kraxel@redhat.com>
>>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>>>
>>> Thanks
>>> Laszlo
>>>
>>> Laszlo Ersek (4):
>>>   OvmfPkg/PlatformPei: assign PciSize on both i440fx/q35 branches
>>>     explicitly
>>>   OvmfPkg/PlatformPei: hoist PciBase assignment above the i440fx/q35
>>>     branching
>>>   OvmfPkg/PlatformPei: reorder the 32-bit PCI hole vs. the PCIEXBAR on
>>>     q35
>>>   OvmfPkg/PlatformPei: fix MTRR for low-RAM sizes that have many bits
>>>     clear
>>>
>>>  OvmfPkg/OvmfPkgIa32.dsc         |  5 +----
>>>  OvmfPkg/OvmfPkgIa32X64.dsc      |  5 +----
>>>  OvmfPkg/OvmfPkgX64.dsc          |  5 +----
>>>  OvmfPkg/PlatformPei/MemDetect.c | 23 +++++++++++++++++---
>>>  OvmfPkg/PlatformPei/Platform.c  | 14 +++++-------
>>>  OvmfPkg/PlatformPei/Platform.h  |  2 ++
>>>  6 files changed, 31 insertions(+), 23 deletions(-)
>>>
>>

Re: [edk2-devel] [PATCH 0/4] OvmfPkg/PlatformPei: fix two assertion failures with weird RAM sizes

[Michael D Kinney]

Looks like a bug fix to me and is fine for soft freeze period.

To commit during hard freeze, just need to show it is a critical bug.

Mike

> -----Original Message-----
> From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
> Sent: Wednesday, May 15, 2019 4:57 AM
> To: Laszlo Ersek <lersek@redhat.com>
> Cc: edk2-devel-groups-io <devel@edk2.groups.io>; Ard
> Biesheuvel <ard.biesheuvel@linaro.org>; Kinney, Michael
> D <michael.d.kinney@intel.com>; Andrew Fish
> <afish@apple.com>; Gerd Hoffmann <kraxel@redhat.com>;
> Justen, Jordan L <jordan.l.justen@intel.com>
> Subject: Re: [edk2-devel] [PATCH 0/4]
> OvmfPkg/PlatformPei: fix two assertion failures with
> weird RAM sizes
> 
> On Wed, May 15, 2019 at 12:36:22PM +0200, Laszlo Ersek
> wrote:
> > Hi Stewards,
> >
> > it seems likely that this patch series -- posted on
> 2019-May-04 -- will
> > not receive the necessary maintainer A-b's / R-b's
> before we enter the
> > soft feature freeze for edk2-stable201905.
> >
> > Therefore I'd like to state that I consider this
> series a bugfix series,
> > not a feature addition. I'm now asking for
> confirmation that I'll be
> > allowed to push the series -- dependent on the
> expected reviewer
> > feedback of course -- even after 2019-May-17 08:00:00
> UTC.
> 
> Clearly bugfix, and quite contained in scope as well.
> 
> > Please also state whether I will need to open a
> TianoCore BZ for
> > tracking this work (and to update the commit messages
> accordingly). I
> > haven't done that because we already have two public
> BZs for the issue
> > in the Red Hat Bugzilla instance, with issue
> description and analysis.
> > (RHBZ references are captured in both the blurb
> below, and in the
> > patches themselves.)
> 
> I would lean towards having a TianoCore BZ for anything
> pushed in the
> freeze period.
> 
> While I don't expect Red Hat to close its BZ from
> public access, it
> does sort of open up the question of "well, which other
> BZs do we
> consider trustworthy enough to give the same
> treatment?" that I would
> prefer to avoid having to deal with during future
> freeze periods.
> 
> But if you (plural) feel I'm being overly cautious, I
> don't feel
> *that* strongly about it.
> 
> Regards,
> 
> Leif
> 
> > Thanks
> > Laszlo
> >
> > On 05/04/19 02:07, Laszlo Ersek wrote:
> > > Repo:   https://github.com/lersek/edk2.git
> > > Branch: exbar_mtrr_rhbz_1666941
> > > Ref:
> https://bugzilla.redhat.com/show_bug.cgi?id=1666941
> > > Ref:
> https://bugzilla.redhat.com/show_bug.cgi?id=1701710
> > >
> > > When booting OVMF on QEMU with "weird" RAM sizes,
> QEMU's low-RAM split
> > > logic can trigger two assertion failures in OVMF:
> > >
> > > - conflict between PCIEXBAR (ECAM) and low-RAM, on
> q35,
> > >
> > > - running out of variable MTRRs when marking the
> uncacheable MMIO range
> > >   in 32-bit address space, on both i440fx and q35.
> > >
> > > This series fixes both issues, by moving around the
> PCIEXBAR on q35, and
> > > by truncating the size of the uncacheable 32-bit
> area to a power of two.
> > > The latter idea was inspired by SeaBIOS.
> > >
> > > Tested on both machine types, with the following
> memory sizes (all in
> > > MB): 1025, 2815, 3583, 5120. On i440fx, the X64
> build was used (without
> > > SMM). On q35, the IA32 and IA32X64 builds were used
> (with SMM). Testing
> > > included "/proc/mtrr" verification, 32-bit PCI MMIO
> aperture
> > > verification, general dmesg checks, and my usual
> regression tests too
> > > (ACPI S3, UEFI variable services, ...).
> > >
> > > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > > Cc: Gerd Hoffmann <kraxel@redhat.com>
> > > Cc: Jordan Justen <jordan.l.justen@intel.com>
> > >
> > > Thanks
> > > Laszlo
> > >
> > > Laszlo Ersek (4):
> > >   OvmfPkg/PlatformPei: assign PciSize on both
> i440fx/q35 branches
> > >     explicitly
> > >   OvmfPkg/PlatformPei: hoist PciBase assignment
> above the i440fx/q35
> > >     branching
> > >   OvmfPkg/PlatformPei: reorder the 32-bit PCI hole
> vs. the PCIEXBAR on
> > >     q35
> > >   OvmfPkg/PlatformPei: fix MTRR for low-RAM sizes
> that have many bits
> > >     clear
> > >
> > >  OvmfPkg/OvmfPkgIa32.dsc         |  5 +----
> > >  OvmfPkg/OvmfPkgIa32X64.dsc      |  5 +----
> > >  OvmfPkg/OvmfPkgX64.dsc          |  5 +----
> > >  OvmfPkg/PlatformPei/MemDetect.c | 23
> +++++++++++++++++---
> > >  OvmfPkg/PlatformPei/Platform.c  | 14 +++++-------
> > >  OvmfPkg/PlatformPei/Platform.h  |  2 ++
> > >  6 files changed, 31 insertions(+), 23 deletions(-)
> > >
> >

Re: [PATCH 1/4] OvmfPkg/PlatformPei: assign PciSize on both i440fx/q35 branches explicitly

[Ard Biesheuvel]

On Sat, 4 May 2019 at 02:07, Laszlo Ersek <lersek@redhat.com> wrote:
>
> In the MemMapInitialization() function, we currently have a common
> PciSize assignment, shared between i440fx and q35. In order to simplify
> the rest of this series, lift and duplicate the assignment identically to
> both board-specific branches.
>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1666941
> Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1701710
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>

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

> ---
>  OvmfPkg/PlatformPei/Platform.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
> index 0876316eefbc..5e0a15484230 100644
> --- a/OvmfPkg/PlatformPei/Platform.c
> +++ b/OvmfPkg/PlatformPei/Platform.c
> @@ -190,8 +190,10 @@ MemMapInitialization (
>        ASSERT (TopOfLowRam <= PciExBarBase);
>        ASSERT (PciExBarBase <= MAX_UINT32 - SIZE_256MB);
>        PciBase = (UINT32)(PciExBarBase + SIZE_256MB);
> +      PciSize = 0xFC000000 - PciBase;
>      } else {
>        PciBase = (TopOfLowRam < BASE_2GB) ? BASE_2GB : TopOfLowRam;
> +      PciSize = 0xFC000000 - PciBase;
>      }
>
>      //
> @@ -207,7 +209,6 @@ MemMapInitialization (
>      // 0xFED20000    gap                          896 KB
>      // 0xFEE00000    LAPIC                          1 MB
>      //
> -    PciSize = 0xFC000000 - PciBase;
>      AddIoMemoryBaseSizeHob (PciBase, PciSize);
>      PcdStatus = PcdSet64S (PcdPciMmio32Base, PciBase);
>      ASSERT_RETURN_ERROR (PcdStatus);
> --
> 2.19.1.3.g30247aa5d201
>
>

Re: [PATCH 2/4] OvmfPkg/PlatformPei: hoist PciBase assignment above the i440fx/q35 branching

[Ard Biesheuvel]

On Sat, 4 May 2019 at 02:07, Laszlo Ersek <lersek@redhat.com> wrote:
>
> In the MemMapInitialization() function, we currently assign PciBase
> different values, on both branches of the board type check. Hoist the
> PciBase assignment from the i440fx branch in front of the "if". This is a
> no-op for the i440fx branch. On the q35 branch, we overwrite this value,
> hence the change is a no-op on q35 as well.
>
> This is another refactoring for simplifying the rest of this series.
>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1666941
> Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1701710
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>

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

> ---
>  OvmfPkg/PlatformPei/Platform.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
> index 5e0a15484230..9c013613a1a0 100644
> --- a/OvmfPkg/PlatformPei/Platform.c
> +++ b/OvmfPkg/PlatformPei/Platform.c
> @@ -181,6 +181,7 @@ MemMapInitialization (
>
>      TopOfLowRam = GetSystemMemorySizeBelow4gb ();
>      PciExBarBase = 0;
> +    PciBase = (TopOfLowRam < BASE_2GB) ? BASE_2GB : TopOfLowRam;
>      if (mHostBridgeDevId == INTEL_Q35_MCH_DEVICE_ID) {
>        //
>        // The MMCONFIG area is expected to fall between the top of low RAM and
> @@ -192,7 +193,6 @@ MemMapInitialization (
>        PciBase = (UINT32)(PciExBarBase + SIZE_256MB);
>        PciSize = 0xFC000000 - PciBase;
>      } else {
> -      PciBase = (TopOfLowRam < BASE_2GB) ? BASE_2GB : TopOfLowRam;
>        PciSize = 0xFC000000 - PciBase;
>      }
>
> --
> 2.19.1.3.g30247aa5d201
>
>

Re: [edk2-devel] [PATCH 3/4] OvmfPkg/PlatformPei: reorder the 32-bit PCI hole vs. the PCIEXBAR on q35

[Ard Biesheuvel]

On Sat, 4 May 2019 at 02:07, Laszlo Ersek <lersek@redhat.com> wrote:
>
> Commit 7b8fe63561b4 ("OvmfPkg: PlatformPei: enable PCIEXBAR (aka MMCONFIG
> / ECAM) on Q35", 2016-03-10) claimed that,
>
>   On Q35 machine types that QEMU intends to support in the long term, QEMU
>   never lets the RAM below 4 GB exceed 2 GB.
>
> Alas, this statement came from a misunderstanding that occurred while we
> worked out the interface contract. In fact QEMU does allow the 32-bit RAM
> extend up to 0xB000_0000 (exclusive), in case the RAM size falls in the
> range (0x8000_0000, 0xB000_0000) (i.e., the RAM size is greater than
> 2048MB and smaller than 2816MB).
>
> In turn, such a RAM size (justifiedly) triggers
>
>   ASSERT (TopOfLowRam <= PciExBarBase);
>
> in MemMapInitialization(), because we placed the 256MB PCIEXBAR at
> 0x8000_0000 (2GB) exactly, relying on the interface contract. (And, the
> 32-bit PCI hole would follow the PCIEXBAR, covering the [0x9000_0000,
> 0xFC00_0000) range.)
>
> In order to fix this, reorder the 32-bit PCI hole against the PCIEXBAR, as
> follows:
>
> - start the 32-bit PCI hole where it starts on i440fx as well, that is, at
>   2GB or TopOfLowRam, whichever is higher;
>
> - unlike on i440fx, where the 32-bit PCI hole extends up to 0xFC00_0000,
>   stop it at 0xE000_0000 on q35,
>
> - place the PCIEXBAR at 0xE000_0000.
>
> (We cannot place the PCIEXBAR at 0xF000_0000 because the 256MB MMIO area
> that starts there is not entirely free.)
>
> Before this patch, the 32-bit PCI hole used to only *end* at the same spot
> (namely, 0xFC00_0000) between i440fx and q35; now it will only *start* at
> the same spot (namely, 2GB or TopOfLowRam, whichever is higher) between
> both boards.
>
> On q35, the maximal hole shrinks from
>
>   0xFC00_0000 - 0x9000_0000 = 0x6C00_0000 == 1728 MB
>
> to
>
>   0xE000_0000 - 0x8000_0000 == 1536 MB.
>
> We lose 192 MB of the aperture; however, the aperture is now aligned at
> 1GB, rather than 256 MB, and so it could fit a 1GB BAR even.
>
> Regarding the minimal hole (triggered by RAM size 2815MB), its size is
>
>   0xE000_0000 - 0xAFF0_0000 = 769 MB
>
> which is not great, but probably better than a failed ASSERT.
>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1666941
> Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1701710
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>

I take it the 32-bit PCI 'hole' is in fact the 32-bit MMIO BAR window?
That could do with a clarification. I guess it may be an x86-ism to
consider any physical memory region not used by RAM to be a hole, but
it is not terribly helpful.

In any case,

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

> ---
>  OvmfPkg/OvmfPkgIa32.dsc        | 5 +----
>  OvmfPkg/OvmfPkgIa32X64.dsc     | 5 +----
>  OvmfPkg/OvmfPkgX64.dsc         | 5 +----
>  OvmfPkg/PlatformPei/Platform.c | 9 ++++-----
>  4 files changed, 7 insertions(+), 17 deletions(-)
>
> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
> index 36a0f87258dd..bb55b6fa58e1 100644
> --- a/OvmfPkg/OvmfPkgIa32.dsc
> +++ b/OvmfPkg/OvmfPkgIa32.dsc
> @@ -491,10 +491,7 @@ [PcdsFixedAtBuild]
>    # This PCD is used to set the base address of the PCI express hierarchy. It
>    # is only consulted when OVMF runs on Q35. In that case it is programmed into
>    # the PCIEXBAR register.
> -  #
> -  # On Q35 machine types that QEMU intends to support in the long term, QEMU
> -  # never lets the RAM below 4 GB exceed 2 GB.
> -  gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0x80000000
> +  gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0xE0000000
>
>  !ifdef $(SOURCE_DEBUG_ENABLE)
>    gEfiSourceLevelDebugPkgTokenSpaceGuid.PcdDebugLoadImageMethod|0x2
> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
> index 9b341e17d7ff..06c394a6fb1f 100644
> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
> @@ -496,10 +496,7 @@ [PcdsFixedAtBuild]
>    # This PCD is used to set the base address of the PCI express hierarchy. It
>    # is only consulted when OVMF runs on Q35. In that case it is programmed into
>    # the PCIEXBAR register.
> -  #
> -  # On Q35 machine types that QEMU intends to support in the long term, QEMU
> -  # never lets the RAM below 4 GB exceed 2 GB.
> -  gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0x80000000
> +  gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0xE0000000
>
>  !ifdef $(SOURCE_DEBUG_ENABLE)
>    gEfiSourceLevelDebugPkgTokenSpaceGuid.PcdDebugLoadImageMethod|0x2
> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> index a0f87f74dab9..5e0eb043fab9 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -496,10 +496,7 @@ [PcdsFixedAtBuild]
>    # This PCD is used to set the base address of the PCI express hierarchy. It
>    # is only consulted when OVMF runs on Q35. In that case it is programmed into
>    # the PCIEXBAR register.
> -  #
> -  # On Q35 machine types that QEMU intends to support in the long term, QEMU
> -  # never lets the RAM below 4 GB exceed 2 GB.
> -  gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0x80000000
> +  gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0xE0000000
>
>  !ifdef $(SOURCE_DEBUG_ENABLE)
>    gEfiSourceLevelDebugPkgTokenSpaceGuid.PcdDebugLoadImageMethod|0x2
> diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
> index 9c013613a1a0..fd8eccaf3e50 100644
> --- a/OvmfPkg/PlatformPei/Platform.c
> +++ b/OvmfPkg/PlatformPei/Platform.c
> @@ -184,14 +184,13 @@ MemMapInitialization (
>      PciBase = (TopOfLowRam < BASE_2GB) ? BASE_2GB : TopOfLowRam;
>      if (mHostBridgeDevId == INTEL_Q35_MCH_DEVICE_ID) {
>        //
> -      // The MMCONFIG area is expected to fall between the top of low RAM and
> -      // the base of the 32-bit PCI host aperture.
> +      // The 32-bit PCI host aperture is expected to fall between the top of
> +      // low RAM and the base of the MMCONFIG area.
>        //
>        PciExBarBase = FixedPcdGet64 (PcdPciExpressBaseAddress);
> -      ASSERT (TopOfLowRam <= PciExBarBase);
> +      ASSERT (PciBase < PciExBarBase);
>        ASSERT (PciExBarBase <= MAX_UINT32 - SIZE_256MB);
> -      PciBase = (UINT32)(PciExBarBase + SIZE_256MB);
> -      PciSize = 0xFC000000 - PciBase;
> +      PciSize = (UINT32)(PciExBarBase - PciBase);
>      } else {
>        PciSize = 0xFC000000 - PciBase;
>      }
> --
> 2.19.1.3.g30247aa5d201
>
>
>
> ------------
> Groups.io Links: You receive all messages sent to this group.
>
> View/Reply Online (#39968): https://edk2.groups.io/g/devel/message/39968
> Mute This Topic: https://groups.io/mt/31489698/1761188
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub  [ard.biesheuvel@linaro.org]
> ------------
>

Re: [PATCH 4/4] OvmfPkg/PlatformPei: fix MTRR for low-RAM sizes that have many bits clear

[Ard Biesheuvel]

On Sat, 4 May 2019 at 02:07, Laszlo Ersek <lersek@redhat.com> wrote:
>
> Assume that we boot OVMF in a QEMU guest with 1025 MB of RAM. The
> following assertion will fire:
>
> > ASSERT_EFI_ERROR (Status = Out of Resources)
> > ASSERT OvmfPkg/PlatformPei/MemDetect.c(696): !EFI_ERROR (Status)
>
> That's because the range [1025 MB, 4 GB) that we try to mark as
> uncacheable with MTRRs has size 3071 MB:
>
>    0x1_0000_0000
>   -0x0_4010_0000
>   --------------
>    0x0_BFF0_0000
>
> The integer that stands for the uncacheable area size has 11 (eleven) bits
> set to 1. As a result, covering this size requires 11 variable MTRRs (each
> MTRR must cover a naturally aligned, power-of-two sized area). But, if we
> need more variable MTRRs than the CPU can muster (such as 8), then
> MtrrSetMemoryAttribute() fails, and we refuse to continue booting (which
> is justified, in itself).
>
> Unfortunately, this is not difficult to trigger, and the error message is
> well-hidden from end-users, in the OVMF debug log. The following
> mitigation is inspired by SeaBIOS:
>
> Truncate the uncacheable area size to a power-of-two, while keeping the
> end fixed at 4 GB. Such an interval can be covered by just one variable
> MTRR.
>
> This may leave such an MMIO gap, between the end of low-RAM and the start
> of the uncacheable area, that is marked as WB (through the MTRR default).
> Raise the base of the 32-bit PCI MMIO aperture accordingly -- the gap will
> not be used for anything.
>
> On Q35, the minimal 32-bit PCI MMIO aperture (triggered by RAM size 2815
> MB) shrinks from
>
>   0xE000_0000 - 0xAFF0_0000 = 769 MB
>
> to
>
>   0xE000_0000 - 0xC000_0000 = 512 MB
>
> On i440fx, the minimal 32-bit PCI MMIO aperture (triggered by RAM size
> 3583 MB) shrinks from
>
>   0xFC00_0000 - 0xDFF0_0000 = 449 MB
>
> to
>
>   0xFC00_0000 - 0xE000_0000 = 448 MB
>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1666941
> Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1701710
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>

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


> ---
>  OvmfPkg/PlatformPei/Platform.h  |  2 ++
>  OvmfPkg/PlatformPei/MemDetect.c | 23 +++++++++++++++++---
>  OvmfPkg/PlatformPei/Platform.c  |  4 +---
>  3 files changed, 23 insertions(+), 6 deletions(-)
>
> diff --git a/OvmfPkg/PlatformPei/Platform.h b/OvmfPkg/PlatformPei/Platform.h
> index 81af8b71480f..4476ddd871cd 100644
> --- a/OvmfPkg/PlatformPei/Platform.h
> +++ b/OvmfPkg/PlatformPei/Platform.h
> @@ -114,4 +114,6 @@ extern UINT32 mMaxCpuCount;
>
>  extern UINT16 mHostBridgeDevId;
>
> +extern UINT32 mQemuUc32Base;
> +
>  #endif // _PLATFORM_PEI_H_INCLUDED_
> diff --git a/OvmfPkg/PlatformPei/MemDetect.c b/OvmfPkg/PlatformPei/MemDetect.c
> index e890e36408a6..ae73c63d27d5 100644
> --- a/OvmfPkg/PlatformPei/MemDetect.c
> +++ b/OvmfPkg/PlatformPei/MemDetect.c
> @@ -42,6 +42,8 @@ STATIC UINT32 mS3AcpiReservedMemorySize;
>
>  STATIC UINT16 mQ35TsegMbytes;
>
> +UINT32 mQemuUc32Base;
> +
>  VOID
>  Q35TsegMbytesInitialization (
>    VOID
> @@ -663,6 +665,8 @@ QemuInitializeRam (
>    // cover it exactly.
>    //
>    if (IsMtrrSupported ()) {
> +    UINT32 Uc32Size;
> +
>      MtrrGetAllMtrrs (&MtrrSettings);
>
>      //
> @@ -689,11 +693,24 @@ QemuInitializeRam (
>
>      //
>      // Set memory range from the "top of lower RAM" (RAM below 4GB) to 4GB as
> -    // uncacheable
> +    // uncacheable. Make sure one variable MTRR suffices by truncating the size
> +    // to a whole power of two. This will round the base *up*, and a gap (not
> +    // used for either RAM or MMIO) may stay in the middle, marked as
> +    // cacheable-by-default.
>      //
> -    Status = MtrrSetMemoryAttribute (LowerMemorySize,
> -               SIZE_4GB - LowerMemorySize, CacheUncacheable);
> +    Uc32Size = GetPowerOfTwo32 ((UINT32)(SIZE_4GB - LowerMemorySize));
> +    mQemuUc32Base = (UINT32)(SIZE_4GB - Uc32Size);
> +    if (mQemuUc32Base != LowerMemorySize) {
> +      DEBUG ((DEBUG_VERBOSE, "%a: rounded UC32 base from 0x%x up to 0x%x, for "
> +        "an UC32 size of 0x%x\n", __FUNCTION__, (UINT32)LowerMemorySize,
> +        mQemuUc32Base, Uc32Size));
> +    }
> +
> +    Status = MtrrSetMemoryAttribute (mQemuUc32Base, Uc32Size,
> +               CacheUncacheable);
>      ASSERT_EFI_ERROR (Status);
> +  } else {
> +    mQemuUc32Base = (UINT32)LowerMemorySize;
>    }
>  }
>
> diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
> index fd8eccaf3e50..c064b4ed9b8f 100644
> --- a/OvmfPkg/PlatformPei/Platform.c
> +++ b/OvmfPkg/PlatformPei/Platform.c
> @@ -174,14 +174,12 @@ MemMapInitialization (
>    AddIoMemoryRangeHob (0x0A0000, BASE_1MB);
>
>    if (!mXen) {
> -    UINT32  TopOfLowRam;
>      UINT64  PciExBarBase;
>      UINT32  PciBase;
>      UINT32  PciSize;
>
> -    TopOfLowRam = GetSystemMemorySizeBelow4gb ();
>      PciExBarBase = 0;
> -    PciBase = (TopOfLowRam < BASE_2GB) ? BASE_2GB : TopOfLowRam;
> +    PciBase = (mQemuUc32Base < BASE_2GB) ? BASE_2GB : mQemuUc32Base;
>      if (mHostBridgeDevId == INTEL_Q35_MCH_DEVICE_ID) {
>        //
>        // The 32-bit PCI host aperture is expected to fall between the top of
> --
> 2.19.1.3.g30247aa5d201
>

Re: [edk2-devel] [PATCH 3/4] OvmfPkg/PlatformPei: reorder the 32-bit PCI hole vs. the PCIEXBAR on q35

[Laszlo Ersek]

On 05/16/19 10:00, Ard Biesheuvel wrote:
> On Sat, 4 May 2019 at 02:07, Laszlo Ersek <lersek@redhat.com> wrote:
>>
>> Commit 7b8fe63561b4 ("OvmfPkg: PlatformPei: enable PCIEXBAR (aka MMCONFIG
>> / ECAM) on Q35", 2016-03-10) claimed that,
>>
>>   On Q35 machine types that QEMU intends to support in the long term, QEMU
>>   never lets the RAM below 4 GB exceed 2 GB.
>>
>> Alas, this statement came from a misunderstanding that occurred while we
>> worked out the interface contract. In fact QEMU does allow the 32-bit RAM
>> extend up to 0xB000_0000 (exclusive), in case the RAM size falls in the
>> range (0x8000_0000, 0xB000_0000) (i.e., the RAM size is greater than
>> 2048MB and smaller than 2816MB).
>>
>> In turn, such a RAM size (justifiedly) triggers
>>
>>   ASSERT (TopOfLowRam <= PciExBarBase);
>>
>> in MemMapInitialization(), because we placed the 256MB PCIEXBAR at
>> 0x8000_0000 (2GB) exactly, relying on the interface contract. (And, the
>> 32-bit PCI hole would follow the PCIEXBAR, covering the [0x9000_0000,
>> 0xFC00_0000) range.)
>>
>> In order to fix this, reorder the 32-bit PCI hole against the PCIEXBAR, as
>> follows:
>>
>> - start the 32-bit PCI hole where it starts on i440fx as well, that is, at
>>   2GB or TopOfLowRam, whichever is higher;
>>
>> - unlike on i440fx, where the 32-bit PCI hole extends up to 0xFC00_0000,
>>   stop it at 0xE000_0000 on q35,
>>
>> - place the PCIEXBAR at 0xE000_0000.
>>
>> (We cannot place the PCIEXBAR at 0xF000_0000 because the 256MB MMIO area
>> that starts there is not entirely free.)
>>
>> Before this patch, the 32-bit PCI hole used to only *end* at the same spot
>> (namely, 0xFC00_0000) between i440fx and q35; now it will only *start* at
>> the same spot (namely, 2GB or TopOfLowRam, whichever is higher) between
>> both boards.
>>
>> On q35, the maximal hole shrinks from
>>
>>   0xFC00_0000 - 0x9000_0000 = 0x6C00_0000 == 1728 MB
>>
>> to
>>
>>   0xE000_0000 - 0x8000_0000 == 1536 MB.
>>
>> We lose 192 MB of the aperture; however, the aperture is now aligned at
>> 1GB, rather than 256 MB, and so it could fit a 1GB BAR even.
>>
>> Regarding the minimal hole (triggered by RAM size 2815MB), its size is
>>
>>   0xE000_0000 - 0xAFF0_0000 = 769 MB
>>
>> which is not great, but probably better than a failed ASSERT.
>>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Cc: Gerd Hoffmann <kraxel@redhat.com>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1666941
>> Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1701710
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> 
> I take it the 32-bit PCI 'hole' is in fact the 32-bit MMIO BAR window?

Yes, that's correct. We also call it "32-bit PCI MMIO aperture",
sometimes. The interval that PciHostBridgeLib passes to
PciHostBridgeDxe, to allocate BARs out of, when PciBusDxe asks.

> That could do with a clarification. I guess it may be an x86-ism to
> consider any physical memory region not used by RAM to be a hole, but
> it is not terribly helpful.

The "PCI hole" expressions is a QEMU-ism; it is frequently used in
connection with the i440fx and q35 (x86) machine types, and not much
elsewhere:

$ git grep -c pci_hole

hw/i386/acpi-build.c:10
hw/i386/pc.c:1
hw/pci-host/grackle.c:3
hw/pci-host/piix.c:23
hw/pci-host/q35.c:24
hw/pci-host/uninorth.c:4
include/hw/i386/pc.h:1
include/hw/pci-host/q35.h:3
include/hw/pci-host/uninorth.h:1

> In any case,
> 
> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Thank you!
Laszlo

> 
>> ---
>>  OvmfPkg/OvmfPkgIa32.dsc        | 5 +----
>>  OvmfPkg/OvmfPkgIa32X64.dsc     | 5 +----
>>  OvmfPkg/OvmfPkgX64.dsc         | 5 +----
>>  OvmfPkg/PlatformPei/Platform.c | 9 ++++-----
>>  4 files changed, 7 insertions(+), 17 deletions(-)
>>
>> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
>> index 36a0f87258dd..bb55b6fa58e1 100644
>> --- a/OvmfPkg/OvmfPkgIa32.dsc
>> +++ b/OvmfPkg/OvmfPkgIa32.dsc
>> @@ -491,10 +491,7 @@ [PcdsFixedAtBuild]
>>    # This PCD is used to set the base address of the PCI express hierarchy. It
>>    # is only consulted when OVMF runs on Q35. In that case it is programmed into
>>    # the PCIEXBAR register.
>> -  #
>> -  # On Q35 machine types that QEMU intends to support in the long term, QEMU
>> -  # never lets the RAM below 4 GB exceed 2 GB.
>> -  gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0x80000000
>> +  gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0xE0000000
>>
>>  !ifdef $(SOURCE_DEBUG_ENABLE)
>>    gEfiSourceLevelDebugPkgTokenSpaceGuid.PcdDebugLoadImageMethod|0x2
>> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
>> index 9b341e17d7ff..06c394a6fb1f 100644
>> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
>> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
>> @@ -496,10 +496,7 @@ [PcdsFixedAtBuild]
>>    # This PCD is used to set the base address of the PCI express hierarchy. It
>>    # is only consulted when OVMF runs on Q35. In that case it is programmed into
>>    # the PCIEXBAR register.
>> -  #
>> -  # On Q35 machine types that QEMU intends to support in the long term, QEMU
>> -  # never lets the RAM below 4 GB exceed 2 GB.
>> -  gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0x80000000
>> +  gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0xE0000000
>>
>>  !ifdef $(SOURCE_DEBUG_ENABLE)
>>    gEfiSourceLevelDebugPkgTokenSpaceGuid.PcdDebugLoadImageMethod|0x2
>> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
>> index a0f87f74dab9..5e0eb043fab9 100644
>> --- a/OvmfPkg/OvmfPkgX64.dsc
>> +++ b/OvmfPkg/OvmfPkgX64.dsc
>> @@ -496,10 +496,7 @@ [PcdsFixedAtBuild]
>>    # This PCD is used to set the base address of the PCI express hierarchy. It
>>    # is only consulted when OVMF runs on Q35. In that case it is programmed into
>>    # the PCIEXBAR register.
>> -  #
>> -  # On Q35 machine types that QEMU intends to support in the long term, QEMU
>> -  # never lets the RAM below 4 GB exceed 2 GB.
>> -  gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0x80000000
>> +  gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0xE0000000
>>
>>  !ifdef $(SOURCE_DEBUG_ENABLE)
>>    gEfiSourceLevelDebugPkgTokenSpaceGuid.PcdDebugLoadImageMethod|0x2
>> diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
>> index 9c013613a1a0..fd8eccaf3e50 100644
>> --- a/OvmfPkg/PlatformPei/Platform.c
>> +++ b/OvmfPkg/PlatformPei/Platform.c
>> @@ -184,14 +184,13 @@ MemMapInitialization (
>>      PciBase = (TopOfLowRam < BASE_2GB) ? BASE_2GB : TopOfLowRam;
>>      if (mHostBridgeDevId == INTEL_Q35_MCH_DEVICE_ID) {
>>        //
>> -      // The MMCONFIG area is expected to fall between the top of low RAM and
>> -      // the base of the 32-bit PCI host aperture.
>> +      // The 32-bit PCI host aperture is expected to fall between the top of
>> +      // low RAM and the base of the MMCONFIG area.
>>        //
>>        PciExBarBase = FixedPcdGet64 (PcdPciExpressBaseAddress);
>> -      ASSERT (TopOfLowRam <= PciExBarBase);
>> +      ASSERT (PciBase < PciExBarBase);
>>        ASSERT (PciExBarBase <= MAX_UINT32 - SIZE_256MB);
>> -      PciBase = (UINT32)(PciExBarBase + SIZE_256MB);
>> -      PciSize = 0xFC000000 - PciBase;
>> +      PciSize = (UINT32)(PciExBarBase - PciBase);
>>      } else {
>>        PciSize = 0xFC000000 - PciBase;
>>      }
>> --
>> 2.19.1.3.g30247aa5d201
>>
>>
>>
>> ------------
>> Groups.io Links: You receive all messages sent to this group.
>>
>> View/Reply Online (#39968): https://edk2.groups.io/g/devel/message/39968
>> Mute This Topic: https://groups.io/mt/31489698/1761188
>> Group Owner: devel+owner@edk2.groups.io
>> Unsubscribe: https://edk2.groups.io/g/devel/unsub  [ard.biesheuvel@linaro.org]
>> ------------
>>

Re: [edk2-devel] [PATCH 3/4] OvmfPkg/PlatformPei: reorder the 32-bit PCI hole vs. the PCIEXBAR on q35

[Philippe Mathieu-Daudé]

Hi Laszlo,

On 5/16/19 2:18 PM, Laszlo Ersek wrote:
> On 05/16/19 10:00, Ard Biesheuvel wrote:
>> On Sat, 4 May 2019 at 02:07, Laszlo Ersek <lersek@redhat.com> wrote:
>>>
>>> Commit 7b8fe63561b4 ("OvmfPkg: PlatformPei: enable PCIEXBAR (aka MMCONFIG
>>> / ECAM) on Q35", 2016-03-10) claimed that,
>>>
>>>   On Q35 machine types that QEMU intends to support in the long term, QEMU
>>>   never lets the RAM below 4 GB exceed 2 GB.
>>>
>>> Alas, this statement came from a misunderstanding that occurred while we
>>> worked out the interface contract. In fact QEMU does allow the 32-bit RAM
>>> extend up to 0xB000_0000 (exclusive), in case the RAM size falls in the
>>> range (0x8000_0000, 0xB000_0000) (i.e., the RAM size is greater than
>>> 2048MB and smaller than 2816MB).
>>>
>>> In turn, such a RAM size (justifiedly) triggers
>>>
>>>   ASSERT (TopOfLowRam <= PciExBarBase);
>>>
>>> in MemMapInitialization(), because we placed the 256MB PCIEXBAR at
>>> 0x8000_0000 (2GB) exactly, relying on the interface contract. (And, the
>>> 32-bit PCI hole would follow the PCIEXBAR, covering the [0x9000_0000,
>>> 0xFC00_0000) range.)
>>>
>>> In order to fix this, reorder the 32-bit PCI hole against the PCIEXBAR, as
>>> follows:
>>>
>>> - start the 32-bit PCI hole where it starts on i440fx as well, that is, at
>>>   2GB or TopOfLowRam, whichever is higher;
>>>
>>> - unlike on i440fx, where the 32-bit PCI hole extends up to 0xFC00_0000,
>>>   stop it at 0xE000_0000 on q35,
>>>
>>> - place the PCIEXBAR at 0xE000_0000.
>>>
>>> (We cannot place the PCIEXBAR at 0xF000_0000 because the 256MB MMIO area
>>> that starts there is not entirely free.)
>>>
>>> Before this patch, the 32-bit PCI hole used to only *end* at the same spot
>>> (namely, 0xFC00_0000) between i440fx and q35; now it will only *start* at
>>> the same spot (namely, 2GB or TopOfLowRam, whichever is higher) between
>>> both boards.
>>>
>>> On q35, the maximal hole shrinks from
>>>
>>>   0xFC00_0000 - 0x9000_0000 = 0x6C00_0000 == 1728 MB
>>>
>>> to
>>>
>>>   0xE000_0000 - 0x8000_0000 == 1536 MB.
>>>
>>> We lose 192 MB of the aperture; however, the aperture is now aligned at
>>> 1GB, rather than 256 MB, and so it could fit a 1GB BAR even.
>>>
>>> Regarding the minimal hole (triggered by RAM size 2815MB), its size is
>>>
>>>   0xE000_0000 - 0xAFF0_0000 = 769 MB
>>>
>>> which is not great, but probably better than a failed ASSERT.
>>>
>>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> Cc: Gerd Hoffmann <kraxel@redhat.com>
>>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>>> Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1666941
>>> Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1701710
>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>>
>> I take it the 32-bit PCI 'hole' is in fact the 32-bit MMIO BAR window?
> 
> Yes, that's correct. We also call it "32-bit PCI MMIO aperture",
> sometimes. The interval that PciHostBridgeLib passes to
> PciHostBridgeDxe, to allocate BARs out of, when PciBusDxe asks.

Maybe you can amend that comment, ...

> 
>> That could do with a clarification. I guess it may be an x86-ism to
>> consider any physical memory region not used by RAM to be a hole, but
>> it is not terribly helpful.
> 
> The "PCI hole" expressions is a QEMU-ism; it is frequently used in

... and this one in the commit description.

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

> connection with the i440fx and q35 (x86) machine types, and not much
> elsewhere:
> 
> $ git grep -c pci_hole
> 
> hw/i386/acpi-build.c:10
> hw/i386/pc.c:1
> hw/pci-host/grackle.c:3
> hw/pci-host/piix.c:23
> hw/pci-host/q35.c:24
> hw/pci-host/uninorth.c:4
> include/hw/i386/pc.h:1
> include/hw/pci-host/q35.h:3
> include/hw/pci-host/uninorth.h:1
> 
>> In any case,
>>
>> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> 
> Thank you!
> Laszlo
> 
>>
>>> ---
>>>  OvmfPkg/OvmfPkgIa32.dsc        | 5 +----
>>>  OvmfPkg/OvmfPkgIa32X64.dsc     | 5 +----
>>>  OvmfPkg/OvmfPkgX64.dsc         | 5 +----
>>>  OvmfPkg/PlatformPei/Platform.c | 9 ++++-----
>>>  4 files changed, 7 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
>>> index 36a0f87258dd..bb55b6fa58e1 100644
>>> --- a/OvmfPkg/OvmfPkgIa32.dsc
>>> +++ b/OvmfPkg/OvmfPkgIa32.dsc
>>> @@ -491,10 +491,7 @@ [PcdsFixedAtBuild]
>>>    # This PCD is used to set the base address of the PCI express hierarchy. It
>>>    # is only consulted when OVMF runs on Q35. In that case it is programmed into
>>>    # the PCIEXBAR register.
>>> -  #
>>> -  # On Q35 machine types that QEMU intends to support in the long term, QEMU
>>> -  # never lets the RAM below 4 GB exceed 2 GB.
>>> -  gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0x80000000
>>> +  gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0xE0000000
>>>
>>>  !ifdef $(SOURCE_DEBUG_ENABLE)
>>>    gEfiSourceLevelDebugPkgTokenSpaceGuid.PcdDebugLoadImageMethod|0x2
>>> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
>>> index 9b341e17d7ff..06c394a6fb1f 100644
>>> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
>>> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
>>> @@ -496,10 +496,7 @@ [PcdsFixedAtBuild]
>>>    # This PCD is used to set the base address of the PCI express hierarchy. It
>>>    # is only consulted when OVMF runs on Q35. In that case it is programmed into
>>>    # the PCIEXBAR register.
>>> -  #
>>> -  # On Q35 machine types that QEMU intends to support in the long term, QEMU
>>> -  # never lets the RAM below 4 GB exceed 2 GB.
>>> -  gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0x80000000
>>> +  gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0xE0000000
>>>
>>>  !ifdef $(SOURCE_DEBUG_ENABLE)
>>>    gEfiSourceLevelDebugPkgTokenSpaceGuid.PcdDebugLoadImageMethod|0x2
>>> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
>>> index a0f87f74dab9..5e0eb043fab9 100644
>>> --- a/OvmfPkg/OvmfPkgX64.dsc
>>> +++ b/OvmfPkg/OvmfPkgX64.dsc
>>> @@ -496,10 +496,7 @@ [PcdsFixedAtBuild]
>>>    # This PCD is used to set the base address of the PCI express hierarchy. It
>>>    # is only consulted when OVMF runs on Q35. In that case it is programmed into
>>>    # the PCIEXBAR register.
>>> -  #
>>> -  # On Q35 machine types that QEMU intends to support in the long term, QEMU
>>> -  # never lets the RAM below 4 GB exceed 2 GB.
>>> -  gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0x80000000
>>> +  gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0xE0000000
>>>
>>>  !ifdef $(SOURCE_DEBUG_ENABLE)
>>>    gEfiSourceLevelDebugPkgTokenSpaceGuid.PcdDebugLoadImageMethod|0x2
>>> diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
>>> index 9c013613a1a0..fd8eccaf3e50 100644
>>> --- a/OvmfPkg/PlatformPei/Platform.c
>>> +++ b/OvmfPkg/PlatformPei/Platform.c
>>> @@ -184,14 +184,13 @@ MemMapInitialization (
>>>      PciBase = (TopOfLowRam < BASE_2GB) ? BASE_2GB : TopOfLowRam;
>>>      if (mHostBridgeDevId == INTEL_Q35_MCH_DEVICE_ID) {
>>>        //
>>> -      // The MMCONFIG area is expected to fall between the top of low RAM and
>>> -      // the base of the 32-bit PCI host aperture.
>>> +      // The 32-bit PCI host aperture is expected to fall between the top of
>>> +      // low RAM and the base of the MMCONFIG area.
>>>        //
>>>        PciExBarBase = FixedPcdGet64 (PcdPciExpressBaseAddress);
>>> -      ASSERT (TopOfLowRam <= PciExBarBase);
>>> +      ASSERT (PciBase < PciExBarBase);
>>>        ASSERT (PciExBarBase <= MAX_UINT32 - SIZE_256MB);
>>> -      PciBase = (UINT32)(PciExBarBase + SIZE_256MB);
>>> -      PciSize = 0xFC000000 - PciBase;
>>> +      PciSize = (UINT32)(PciExBarBase - PciBase);
>>>      } else {
>>>        PciSize = 0xFC000000 - PciBase;
>>>      }
>>> --
>>> 2.19.1.3.g30247aa5d201
>>>
>>>
>>>
>>> ------------
>>> Groups.io Links: You receive all messages sent to this group.
>>>
>>> View/Reply Online (#39968): https://edk2.groups.io/g/devel/message/39968
>>> Mute This Topic: https://groups.io/mt/31489698/1761188
>>> Group Owner: devel+owner@edk2.groups.io
>>> Unsubscribe: https://edk2.groups.io/g/devel/unsub  [ard.biesheuvel@linaro.org]
>>> ------------
>>>
> 
> 
> 
> 

Re: [edk2-devel] [PATCH 3/4] OvmfPkg/PlatformPei: reorder the 32-bit PCI hole vs. the PCIEXBAR on q35

[Laszlo Ersek]

On 05/16/19 14:24, Philippe Mathieu-Daudé wrote:
> Hi Laszlo,
> 
> On 5/16/19 2:18 PM, Laszlo Ersek wrote:
>> On 05/16/19 10:00, Ard Biesheuvel wrote:
>>> On Sat, 4 May 2019 at 02:07, Laszlo Ersek <lersek@redhat.com> wrote:
>>>>
>>>> Commit 7b8fe63561b4 ("OvmfPkg: PlatformPei: enable PCIEXBAR (aka MMCONFIG
>>>> / ECAM) on Q35", 2016-03-10) claimed that,
>>>>
>>>>   On Q35 machine types that QEMU intends to support in the long term, QEMU
>>>>   never lets the RAM below 4 GB exceed 2 GB.
>>>>
>>>> Alas, this statement came from a misunderstanding that occurred while we
>>>> worked out the interface contract. In fact QEMU does allow the 32-bit RAM
>>>> extend up to 0xB000_0000 (exclusive), in case the RAM size falls in the
>>>> range (0x8000_0000, 0xB000_0000) (i.e., the RAM size is greater than
>>>> 2048MB and smaller than 2816MB).
>>>>
>>>> In turn, such a RAM size (justifiedly) triggers
>>>>
>>>>   ASSERT (TopOfLowRam <= PciExBarBase);
>>>>
>>>> in MemMapInitialization(), because we placed the 256MB PCIEXBAR at
>>>> 0x8000_0000 (2GB) exactly, relying on the interface contract. (And, the
>>>> 32-bit PCI hole would follow the PCIEXBAR, covering the [0x9000_0000,
>>>> 0xFC00_0000) range.)
>>>>
>>>> In order to fix this, reorder the 32-bit PCI hole against the PCIEXBAR, as
>>>> follows:
>>>>
>>>> - start the 32-bit PCI hole where it starts on i440fx as well, that is, at
>>>>   2GB or TopOfLowRam, whichever is higher;
>>>>
>>>> - unlike on i440fx, where the 32-bit PCI hole extends up to 0xFC00_0000,
>>>>   stop it at 0xE000_0000 on q35,
>>>>
>>>> - place the PCIEXBAR at 0xE000_0000.
>>>>
>>>> (We cannot place the PCIEXBAR at 0xF000_0000 because the 256MB MMIO area
>>>> that starts there is not entirely free.)
>>>>
>>>> Before this patch, the 32-bit PCI hole used to only *end* at the same spot
>>>> (namely, 0xFC00_0000) between i440fx and q35; now it will only *start* at
>>>> the same spot (namely, 2GB or TopOfLowRam, whichever is higher) between
>>>> both boards.
>>>>
>>>> On q35, the maximal hole shrinks from
>>>>
>>>>   0xFC00_0000 - 0x9000_0000 = 0x6C00_0000 == 1728 MB
>>>>
>>>> to
>>>>
>>>>   0xE000_0000 - 0x8000_0000 == 1536 MB.
>>>>
>>>> We lose 192 MB of the aperture; however, the aperture is now aligned at
>>>> 1GB, rather than 256 MB, and so it could fit a 1GB BAR even.
>>>>
>>>> Regarding the minimal hole (triggered by RAM size 2815MB), its size is
>>>>
>>>>   0xE000_0000 - 0xAFF0_0000 = 769 MB
>>>>
>>>> which is not great, but probably better than a failed ASSERT.
>>>>
>>>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>>> Cc: Gerd Hoffmann <kraxel@redhat.com>
>>>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>>>> Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1666941
>>>> Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1701710
>>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>>>
>>> I take it the 32-bit PCI 'hole' is in fact the 32-bit MMIO BAR window?
>>
>> Yes, that's correct. We also call it "32-bit PCI MMIO aperture",
>> sometimes. The interval that PciHostBridgeLib passes to
>> PciHostBridgeDxe, to allocate BARs out of, when PciBusDxe asks.
> 
> Maybe you can amend that comment, ...
> 
>>
>>> That could do with a clarification. I guess it may be an x86-ism to
>>> consider any physical memory region not used by RAM to be a hole, but
>>> it is not terribly helpful.
>>
>> The "PCI hole" expressions is a QEMU-ism; it is frequently used in
> 
> ... and this one in the commit description.
> 
> Regardless:
> Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>

I've replaced all occurrences of the word "hole" in this commit message
(including the subject line) with "window". That allowed me to fit into
74 characters even with the modified subject.

Thanks!
Laszlo

>> connection with the i440fx and q35 (x86) machine types, and not much
>> elsewhere:
>>
>> $ git grep -c pci_hole
>>
>> hw/i386/acpi-build.c:10
>> hw/i386/pc.c:1
>> hw/pci-host/grackle.c:3
>> hw/pci-host/piix.c:23
>> hw/pci-host/q35.c:24
>> hw/pci-host/uninorth.c:4
>> include/hw/i386/pc.h:1
>> include/hw/pci-host/q35.h:3
>> include/hw/pci-host/uninorth.h:1
>>
>>> In any case,
>>>
>>> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>
>> Thank you!
>> Laszlo
>>
>>>
>>>> ---
>>>>  OvmfPkg/OvmfPkgIa32.dsc        | 5 +----
>>>>  OvmfPkg/OvmfPkgIa32X64.dsc     | 5 +----
>>>>  OvmfPkg/OvmfPkgX64.dsc         | 5 +----
>>>>  OvmfPkg/PlatformPei/Platform.c | 9 ++++-----
>>>>  4 files changed, 7 insertions(+), 17 deletions(-)
>>>>
>>>> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
>>>> index 36a0f87258dd..bb55b6fa58e1 100644
>>>> --- a/OvmfPkg/OvmfPkgIa32.dsc
>>>> +++ b/OvmfPkg/OvmfPkgIa32.dsc
>>>> @@ -491,10 +491,7 @@ [PcdsFixedAtBuild]
>>>>    # This PCD is used to set the base address of the PCI express hierarchy. It
>>>>    # is only consulted when OVMF runs on Q35. In that case it is programmed into
>>>>    # the PCIEXBAR register.
>>>> -  #
>>>> -  # On Q35 machine types that QEMU intends to support in the long term, QEMU
>>>> -  # never lets the RAM below 4 GB exceed 2 GB.
>>>> -  gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0x80000000
>>>> +  gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0xE0000000
>>>>
>>>>  !ifdef $(SOURCE_DEBUG_ENABLE)
>>>>    gEfiSourceLevelDebugPkgTokenSpaceGuid.PcdDebugLoadImageMethod|0x2
>>>> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
>>>> index 9b341e17d7ff..06c394a6fb1f 100644
>>>> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
>>>> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
>>>> @@ -496,10 +496,7 @@ [PcdsFixedAtBuild]
>>>>    # This PCD is used to set the base address of the PCI express hierarchy. It
>>>>    # is only consulted when OVMF runs on Q35. In that case it is programmed into
>>>>    # the PCIEXBAR register.
>>>> -  #
>>>> -  # On Q35 machine types that QEMU intends to support in the long term, QEMU
>>>> -  # never lets the RAM below 4 GB exceed 2 GB.
>>>> -  gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0x80000000
>>>> +  gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0xE0000000
>>>>
>>>>  !ifdef $(SOURCE_DEBUG_ENABLE)
>>>>    gEfiSourceLevelDebugPkgTokenSpaceGuid.PcdDebugLoadImageMethod|0x2
>>>> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
>>>> index a0f87f74dab9..5e0eb043fab9 100644
>>>> --- a/OvmfPkg/OvmfPkgX64.dsc
>>>> +++ b/OvmfPkg/OvmfPkgX64.dsc
>>>> @@ -496,10 +496,7 @@ [PcdsFixedAtBuild]
>>>>    # This PCD is used to set the base address of the PCI express hierarchy. It
>>>>    # is only consulted when OVMF runs on Q35. In that case it is programmed into
>>>>    # the PCIEXBAR register.
>>>> -  #
>>>> -  # On Q35 machine types that QEMU intends to support in the long term, QEMU
>>>> -  # never lets the RAM below 4 GB exceed 2 GB.
>>>> -  gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0x80000000
>>>> +  gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0xE0000000
>>>>
>>>>  !ifdef $(SOURCE_DEBUG_ENABLE)
>>>>    gEfiSourceLevelDebugPkgTokenSpaceGuid.PcdDebugLoadImageMethod|0x2
>>>> diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
>>>> index 9c013613a1a0..fd8eccaf3e50 100644
>>>> --- a/OvmfPkg/PlatformPei/Platform.c
>>>> +++ b/OvmfPkg/PlatformPei/Platform.c
>>>> @@ -184,14 +184,13 @@ MemMapInitialization (
>>>>      PciBase = (TopOfLowRam < BASE_2GB) ? BASE_2GB : TopOfLowRam;
>>>>      if (mHostBridgeDevId == INTEL_Q35_MCH_DEVICE_ID) {
>>>>        //
>>>> -      // The MMCONFIG area is expected to fall between the top of low RAM and
>>>> -      // the base of the 32-bit PCI host aperture.
>>>> +      // The 32-bit PCI host aperture is expected to fall between the top of
>>>> +      // low RAM and the base of the MMCONFIG area.
>>>>        //
>>>>        PciExBarBase = FixedPcdGet64 (PcdPciExpressBaseAddress);
>>>> -      ASSERT (TopOfLowRam <= PciExBarBase);
>>>> +      ASSERT (PciBase < PciExBarBase);
>>>>        ASSERT (PciExBarBase <= MAX_UINT32 - SIZE_256MB);
>>>> -      PciBase = (UINT32)(PciExBarBase + SIZE_256MB);
>>>> -      PciSize = 0xFC000000 - PciBase;
>>>> +      PciSize = (UINT32)(PciExBarBase - PciBase);
>>>>      } else {
>>>>        PciSize = 0xFC000000 - PciBase;
>>>>      }
>>>> --
>>>> 2.19.1.3.g30247aa5d201
>>>>
>>>>
>>>>
>>>> ------------
>>>> Groups.io Links: You receive all messages sent to this group.
>>>>
>>>> View/Reply Online (#39968): https://edk2.groups.io/g/devel/message/39968
>>>> Mute This Topic: https://groups.io/mt/31489698/1761188
>>>> Group Owner: devel+owner@edk2.groups.io
>>>> Unsubscribe: https://edk2.groups.io/g/devel/unsub  [ard.biesheuvel@linaro.org]
>>>> ------------
>>>>
>>
>>
>>
>>
> 
> 
> 

Re: [edk2-devel] [PATCH 0/4] OvmfPkg/PlatformPei: fix two assertion failures with weird RAM sizes

[Laszlo Ersek]

On 05/04/19 02:07, Laszlo Ersek wrote:
> Repo:   https://github.com/lersek/edk2.git
> Branch: exbar_mtrr_rhbz_1666941
> Ref:    https://bugzilla.redhat.com/show_bug.cgi?id=1666941
> Ref:    https://bugzilla.redhat.com/show_bug.cgi?id=1701710
> 
> When booting OVMF on QEMU with "weird" RAM sizes, QEMU's low-RAM split
> logic can trigger two assertion failures in OVMF:
> 
> - conflict between PCIEXBAR (ECAM) and low-RAM, on q35,
> 
> - running out of variable MTRRs when marking the uncacheable MMIO range
>   in 32-bit address space, on both i440fx and q35.
> 
> This series fixes both issues, by moving around the PCIEXBAR on q35, and
> by truncating the size of the uncacheable 32-bit area to a power of two.
> The latter idea was inspired by SeaBIOS.
> 
> Tested on both machine types, with the following memory sizes (all in
> MB): 1025, 2815, 3583, 5120. On i440fx, the X64 build was used (without
> SMM). On q35, the IA32 and IA32X64 builds were used (with SMM). Testing
> included "/proc/mtrr" verification, 32-bit PCI MMIO aperture
> verification, general dmesg checks, and my usual regression tests too
> (ACPI S3, UEFI variable services, ...).

Thanks to everyone for the feedback!

I've now pushed this series as commit range 3b7a897cd8e3..39b9a5ffe661,
with the following updates (all requested per review feedback):

* added the following tag to every commit message:

  Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1814

* dropped the following tag from every affected commit message (i.e., on
  the first three patches):

  Contributed-under: TianoCore Contribution Agreement 1.1

* picked up Phil's and Ard's R-bs

* replaced "hole" with "window" in the commit message of patch #3, then
  re-folded the text to 74 columns.

Cheers
Laszlo

> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> 
> Thanks
> Laszlo
> 
> Laszlo Ersek (4):
>   OvmfPkg/PlatformPei: assign PciSize on both i440fx/q35 branches
>     explicitly
>   OvmfPkg/PlatformPei: hoist PciBase assignment above the i440fx/q35
>     branching
>   OvmfPkg/PlatformPei: reorder the 32-bit PCI hole vs. the PCIEXBAR on
>     q35
>   OvmfPkg/PlatformPei: fix MTRR for low-RAM sizes that have many bits
>     clear
> 
>  OvmfPkg/OvmfPkgIa32.dsc         |  5 +----
>  OvmfPkg/OvmfPkgIa32X64.dsc      |  5 +----
>  OvmfPkg/OvmfPkgX64.dsc          |  5 +----
>  OvmfPkg/PlatformPei/MemDetect.c | 23 +++++++++++++++++---
>  OvmfPkg/PlatformPei/Platform.c  | 14 +++++-------
>  OvmfPkg/PlatformPei/Platform.h  |  2 ++
>  6 files changed, 31 insertions(+), 23 deletions(-)
>