[PATCH for-edk2-stable201905 0/6] work around a QEMU issue triggered by the original TianoCore#1814 fix

[PATCH for-edk2-stable201905 0/6] work around a QEMU issue triggered by the original TianoCore#1814 fix

[Laszlo Ersek]

Bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=1859
Repo:     https://github.com/lersek/edk2.git
Branch:   exbar_mtrr_bz_1859

The fix (commit range 3b7a897cd8e3..39b9a5ffe661) for
<https://bugzilla.tianocore.org/show_bug.cgi?id=1814> is technically
correct, but it tickles an (arguably unjustified) assumption in QEMU the
wrong way. For end users, this makes the original fix for TianoCore#1814
a regression, under certain circumstances.

In theory, the assumption should be eliminated in QEMU, but in practice,
that could be quite intrusive and/or take long. It seems possible to
work around the problem in OVMF, satisfying the assumption again; for
that, OVMF needs a different fix (and a different trade-off) for the
original problem described in TianoCore#1814.

Please see the detailed problem statement and the workaround's idea in
TianoCore#1859.

If possible, I'd like to get this into edk2-stable201905 (which we're
postponing by two weeks anyway, for the sake of the OpenSSL 1.1.1b
upgrade).

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 (6):
  Revert "OvmfPkg/PlatformPei: fix MTRR for low-RAM sizes that have many
    bits clear"
  Revert "OvmfPkg/PlatformPei: reorder the 32-bit PCI window vs. the
    PCIEXBAR on q35"
  Revert "OvmfPkg/PlatformPei: hoist PciBase assignment above the
    i440fx/q35 branching"
  Revert "OvmfPkg/PlatformPei: assign PciSize on both i440fx/q35
    branches explicitly"
  OvmfPkg: raise the PCIEXBAR base to 2816 MB on Q35
  OvmfPkg/PlatformPei: set 32-bit UC area at PciBase / PciExBarBase
    (pc/q35)

 OvmfPkg/OvmfPkgIa32.dsc         |  5 +-
 OvmfPkg/OvmfPkgIa32X64.dsc      |  5 +-
 OvmfPkg/OvmfPkgX64.dsc          |  5 +-
 OvmfPkg/PlatformPei/Platform.h  |  5 ++
 OvmfPkg/PlatformPei/MemDetect.c | 70 +++++++++++++++-----
 OvmfPkg/PlatformPei/Platform.c  | 17 +++--
 6 files changed, 80 insertions(+), 27 deletions(-)

-- 
2.19.1.3.g30247aa5d201

[PATCH for-edk2-stable201905 1/6] Revert "OvmfPkg/PlatformPei: fix MTRR for low-RAM sizes that have many bits clear"

[Laszlo Ersek]

This reverts commit 39b9a5ffe6618b7870be2a54fe7725000249c33a.

The original fix for <https://bugzilla.tianocore.org/show_bug.cgi?id=1814>
triggered a bug / incorrect assumption in QEMU.

QEMU assumes that the PCIEXBAR is below the 32-bit PCI window, not above
it. When the firmware doesn't satisfy this assumption, QEMU generates an
\_SB.PCI0._CRS object in the ACPI DSDT that does not reflect the
firmware's 32-bit MMIO BAR assignments. This causes OSes to re-assign
32-bit MMIO BARs.

Working around the problem in the firmware looks less problematic than
fixing QEMU. Revert the original changes first, before implementing an
alternative fix.

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1859
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, 6 insertions(+), 23 deletions(-)

diff --git a/OvmfPkg/PlatformPei/Platform.h b/OvmfPkg/PlatformPei/Platform.h
index 4476ddd871cd..81af8b71480f 100644
--- a/OvmfPkg/PlatformPei/Platform.h
+++ b/OvmfPkg/PlatformPei/Platform.h
@@ -114,6 +114,4 @@ 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 ae73c63d27d5..e890e36408a6 100644
--- a/OvmfPkg/PlatformPei/MemDetect.c
+++ b/OvmfPkg/PlatformPei/MemDetect.c
@@ -42,8 +42,6 @@ STATIC UINT32 mS3AcpiReservedMemorySize;
 
 STATIC UINT16 mQ35TsegMbytes;
 
-UINT32 mQemuUc32Base;
-
 VOID
 Q35TsegMbytesInitialization (
   VOID
@@ -665,8 +663,6 @@ QemuInitializeRam (
   // cover it exactly.
   //
   if (IsMtrrSupported ()) {
-    UINT32 Uc32Size;
-
     MtrrGetAllMtrrs (&MtrrSettings);
 
     //
@@ -693,24 +689,11 @@ QemuInitializeRam (
 
     //
     // Set memory range from the "top of lower RAM" (RAM below 4GB) to 4GB as
-    // 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.
+    // uncacheable
     //
-    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);
+    Status = MtrrSetMemoryAttribute (LowerMemorySize,
+               SIZE_4GB - LowerMemorySize, CacheUncacheable);
     ASSERT_EFI_ERROR (Status);
-  } else {
-    mQemuUc32Base = (UINT32)LowerMemorySize;
   }
 }
 
diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
index c064b4ed9b8f..fd8eccaf3e50 100644
--- a/OvmfPkg/PlatformPei/Platform.c
+++ b/OvmfPkg/PlatformPei/Platform.c
@@ -174,12 +174,14 @@ MemMapInitialization (
   AddIoMemoryRangeHob (0x0A0000, BASE_1MB);
 
   if (!mXen) {
+    UINT32  TopOfLowRam;
     UINT64  PciExBarBase;
     UINT32  PciBase;
     UINT32  PciSize;
 
+    TopOfLowRam = GetSystemMemorySizeBelow4gb ();
     PciExBarBase = 0;
-    PciBase = (mQemuUc32Base < BASE_2GB) ? BASE_2GB : mQemuUc32Base;
+    PciBase = (TopOfLowRam < BASE_2GB) ? BASE_2GB : TopOfLowRam;
     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 for-edk2-stable201905 1/6] Revert "OvmfPkg/PlatformPei: fix MTRR for low-RAM sizes that have many bits clear"

[Philippe Mathieu-Daudé]

On 5/29/19 5:12 PM, Laszlo Ersek wrote:
> This reverts commit 39b9a5ffe6618b7870be2a54fe7725000249c33a.
> 
> The original fix for <https://bugzilla.tianocore.org/show_bug.cgi?id=1814>
> triggered a bug / incorrect assumption in QEMU.
> 
> QEMU assumes that the PCIEXBAR is below the 32-bit PCI window, not above
> it. When the firmware doesn't satisfy this assumption, QEMU generates an
> \_SB.PCI0._CRS object in the ACPI DSDT that does not reflect the
> firmware's 32-bit MMIO BAR assignments. This causes OSes to re-assign
> 32-bit MMIO BARs.
> 
> Working around the problem in the firmware looks less problematic than
> fixing QEMU. Revert the original changes first, before implementing an
> alternative fix.
> 
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1859
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>

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

> ---
>  OvmfPkg/PlatformPei/Platform.h  |  2 --
>  OvmfPkg/PlatformPei/MemDetect.c | 23 +++-----------------
>  OvmfPkg/PlatformPei/Platform.c  |  4 +++-
>  3 files changed, 6 insertions(+), 23 deletions(-)
> 
> diff --git a/OvmfPkg/PlatformPei/Platform.h b/OvmfPkg/PlatformPei/Platform.h
> index 4476ddd871cd..81af8b71480f 100644
> --- a/OvmfPkg/PlatformPei/Platform.h
> +++ b/OvmfPkg/PlatformPei/Platform.h
> @@ -114,6 +114,4 @@ 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 ae73c63d27d5..e890e36408a6 100644
> --- a/OvmfPkg/PlatformPei/MemDetect.c
> +++ b/OvmfPkg/PlatformPei/MemDetect.c
> @@ -42,8 +42,6 @@ STATIC UINT32 mS3AcpiReservedMemorySize;
>  
>  STATIC UINT16 mQ35TsegMbytes;
>  
> -UINT32 mQemuUc32Base;
> -
>  VOID
>  Q35TsegMbytesInitialization (
>    VOID
> @@ -665,8 +663,6 @@ QemuInitializeRam (
>    // cover it exactly.
>    //
>    if (IsMtrrSupported ()) {
> -    UINT32 Uc32Size;
> -
>      MtrrGetAllMtrrs (&MtrrSettings);
>  
>      //
> @@ -693,24 +689,11 @@ QemuInitializeRam (
>  
>      //
>      // Set memory range from the "top of lower RAM" (RAM below 4GB) to 4GB as
> -    // 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.
> +    // uncacheable
>      //
> -    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);
> +    Status = MtrrSetMemoryAttribute (LowerMemorySize,
> +               SIZE_4GB - LowerMemorySize, CacheUncacheable);
>      ASSERT_EFI_ERROR (Status);
> -  } else {
> -    mQemuUc32Base = (UINT32)LowerMemorySize;
>    }
>  }
>  
> diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
> index c064b4ed9b8f..fd8eccaf3e50 100644
> --- a/OvmfPkg/PlatformPei/Platform.c
> +++ b/OvmfPkg/PlatformPei/Platform.c
> @@ -174,12 +174,14 @@ MemMapInitialization (
>    AddIoMemoryRangeHob (0x0A0000, BASE_1MB);
>  
>    if (!mXen) {
> +    UINT32  TopOfLowRam;
>      UINT64  PciExBarBase;
>      UINT32  PciBase;
>      UINT32  PciSize;
>  
> +    TopOfLowRam = GetSystemMemorySizeBelow4gb ();
>      PciExBarBase = 0;
> -    PciBase = (mQemuUc32Base < BASE_2GB) ? BASE_2GB : mQemuUc32Base;
> +    PciBase = (TopOfLowRam < BASE_2GB) ? BASE_2GB : TopOfLowRam;
>      if (mHostBridgeDevId == INTEL_Q35_MCH_DEVICE_ID) {
>        //
>        // The 32-bit PCI host aperture is expected to fall between the top of
> 

[PATCH for-edk2-stable201905 2/6] Revert "OvmfPkg/PlatformPei: reorder the 32-bit PCI window vs. the PCIEXBAR on q35"

[Laszlo Ersek]

This reverts commit 75136b29541b0e093a51d2e2c2af8d19855c2b60.

The original fix for <https://bugzilla.tianocore.org/show_bug.cgi?id=1814>
triggered a bug / incorrect assumption in QEMU.

QEMU assumes that the PCIEXBAR is below the 32-bit PCI window, not above
it. When the firmware doesn't satisfy this assumption, QEMU generates an
\_SB.PCI0._CRS object in the ACPI DSDT that does not reflect the
firmware's 32-bit MMIO BAR assignments. This causes OSes to re-assign
32-bit MMIO BARs.

Working around the problem in the firmware looks less problematic than
fixing QEMU. Revert the original changes first, before implementing an
alternative fix.

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1859
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, 17 insertions(+), 7 deletions(-)

diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
index b3446ece311a..578fc6c98ec8 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -490,7 +490,10 @@ [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.
-  gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0xE0000000
+  #
+  # 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
 
 !ifdef $(SOURCE_DEBUG_ENABLE)
   gEfiSourceLevelDebugPkgTokenSpaceGuid.PcdDebugLoadImageMethod|0x2
diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index 679d4eb8dd36..eade8f62d3de 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -495,7 +495,10 @@ [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.
-  gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0xE0000000
+  #
+  # 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
 
 !ifdef $(SOURCE_DEBUG_ENABLE)
   gEfiSourceLevelDebugPkgTokenSpaceGuid.PcdDebugLoadImageMethod|0x2
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index 56a9560262aa..733a4c9d8a43 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -495,7 +495,10 @@ [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.
-  gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0xE0000000
+  #
+  # 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
 
 !ifdef $(SOURCE_DEBUG_ENABLE)
   gEfiSourceLevelDebugPkgTokenSpaceGuid.PcdDebugLoadImageMethod|0x2
diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
index fd8eccaf3e50..9c013613a1a0 100644
--- a/OvmfPkg/PlatformPei/Platform.c
+++ b/OvmfPkg/PlatformPei/Platform.c
@@ -184,13 +184,14 @@ MemMapInitialization (
     PciBase = (TopOfLowRam < BASE_2GB) ? BASE_2GB : TopOfLowRam;
     if (mHostBridgeDevId == INTEL_Q35_MCH_DEVICE_ID) {
       //
-      // The 32-bit PCI host aperture is expected to fall between the top of
-      // low RAM and the base of the MMCONFIG area.
+      // The MMCONFIG area is expected to fall between the top of low RAM and
+      // the base of the 32-bit PCI host aperture.
       //
       PciExBarBase = FixedPcdGet64 (PcdPciExpressBaseAddress);
-      ASSERT (PciBase < PciExBarBase);
+      ASSERT (TopOfLowRam <= PciExBarBase);
       ASSERT (PciExBarBase <= MAX_UINT32 - SIZE_256MB);
-      PciSize = (UINT32)(PciExBarBase - PciBase);
+      PciBase = (UINT32)(PciExBarBase + SIZE_256MB);
+      PciSize = 0xFC000000 - PciBase;
     } else {
       PciSize = 0xFC000000 - PciBase;
     }
-- 
2.19.1.3.g30247aa5d201

Re: [edk2-devel] [PATCH for-edk2-stable201905 2/6] Revert "OvmfPkg/PlatformPei: reorder the 32-bit PCI window vs. the PCIEXBAR on q35"

[Philippe Mathieu-Daudé]

On 5/29/19 5:12 PM, Laszlo Ersek wrote:
> This reverts commit 75136b29541b0e093a51d2e2c2af8d19855c2b60.
> 
> The original fix for <https://bugzilla.tianocore.org/show_bug.cgi?id=1814>
> triggered a bug / incorrect assumption in QEMU.
> 
> QEMU assumes that the PCIEXBAR is below the 32-bit PCI window, not above
> it. When the firmware doesn't satisfy this assumption, QEMU generates an
> \_SB.PCI0._CRS object in the ACPI DSDT that does not reflect the
> firmware's 32-bit MMIO BAR assignments. This causes OSes to re-assign
> 32-bit MMIO BARs.
> 
> Working around the problem in the firmware looks less problematic than
> fixing QEMU. Revert the original changes first, before implementing an
> alternative fix.
> 
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1859
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>

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

> ---
>  OvmfPkg/OvmfPkgIa32.dsc        | 5 ++++-
>  OvmfPkg/OvmfPkgIa32X64.dsc     | 5 ++++-
>  OvmfPkg/OvmfPkgX64.dsc         | 5 ++++-
>  OvmfPkg/PlatformPei/Platform.c | 9 +++++----
>  4 files changed, 17 insertions(+), 7 deletions(-)
> 
> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
> index b3446ece311a..578fc6c98ec8 100644
> --- a/OvmfPkg/OvmfPkgIa32.dsc
> +++ b/OvmfPkg/OvmfPkgIa32.dsc
> @@ -490,7 +490,10 @@ [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.
> -  gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0xE0000000
> +  #
> +  # 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
>  
>  !ifdef $(SOURCE_DEBUG_ENABLE)
>    gEfiSourceLevelDebugPkgTokenSpaceGuid.PcdDebugLoadImageMethod|0x2
> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
> index 679d4eb8dd36..eade8f62d3de 100644
> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
> @@ -495,7 +495,10 @@ [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.
> -  gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0xE0000000
> +  #
> +  # 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
>  
>  !ifdef $(SOURCE_DEBUG_ENABLE)
>    gEfiSourceLevelDebugPkgTokenSpaceGuid.PcdDebugLoadImageMethod|0x2
> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> index 56a9560262aa..733a4c9d8a43 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -495,7 +495,10 @@ [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.
> -  gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0xE0000000
> +  #
> +  # 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
>  
>  !ifdef $(SOURCE_DEBUG_ENABLE)
>    gEfiSourceLevelDebugPkgTokenSpaceGuid.PcdDebugLoadImageMethod|0x2
> diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
> index fd8eccaf3e50..9c013613a1a0 100644
> --- a/OvmfPkg/PlatformPei/Platform.c
> +++ b/OvmfPkg/PlatformPei/Platform.c
> @@ -184,13 +184,14 @@ MemMapInitialization (
>      PciBase = (TopOfLowRam < BASE_2GB) ? BASE_2GB : TopOfLowRam;
>      if (mHostBridgeDevId == INTEL_Q35_MCH_DEVICE_ID) {
>        //
> -      // The 32-bit PCI host aperture is expected to fall between the top of
> -      // low RAM and the base of the MMCONFIG area.
> +      // The MMCONFIG area is expected to fall between the top of low RAM and
> +      // the base of the 32-bit PCI host aperture.
>        //
>        PciExBarBase = FixedPcdGet64 (PcdPciExpressBaseAddress);
> -      ASSERT (PciBase < PciExBarBase);
> +      ASSERT (TopOfLowRam <= PciExBarBase);
>        ASSERT (PciExBarBase <= MAX_UINT32 - SIZE_256MB);
> -      PciSize = (UINT32)(PciExBarBase - PciBase);
> +      PciBase = (UINT32)(PciExBarBase + SIZE_256MB);
> +      PciSize = 0xFC000000 - PciBase;
>      } else {
>        PciSize = 0xFC000000 - PciBase;
>      }
> 

[PATCH for-edk2-stable201905 3/6] Revert "OvmfPkg/PlatformPei: hoist PciBase assignment above the i440fx/q35 branching"

[Laszlo Ersek]

This reverts commit 9a2e8d7c65ef7f39c6754d27e52954b616bc6628.

The original fix for <https://bugzilla.tianocore.org/show_bug.cgi?id=1814>
triggered a bug / incorrect assumption in QEMU.

QEMU assumes that the PCIEXBAR is below the 32-bit PCI window, not above
it. When the firmware doesn't satisfy this assumption, QEMU generates an
\_SB.PCI0._CRS object in the ACPI DSDT that does not reflect the
firmware's 32-bit MMIO BAR assignments. This causes OSes to re-assign
32-bit MMIO BARs.

Working around the problem in the firmware looks less problematic than
fixing QEMU. Revert the original changes first, before implementing an
alternative fix.

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1859
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 9c013613a1a0..5e0a15484230 100644
--- a/OvmfPkg/PlatformPei/Platform.c
+++ b/OvmfPkg/PlatformPei/Platform.c
@@ -181,7 +181,6 @@ 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
@@ -193,6 +192,7 @@ 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 for-edk2-stable201905 3/6] Revert "OvmfPkg/PlatformPei: hoist PciBase assignment above the i440fx/q35 branching"

[Philippe Mathieu-Daudé]

On 5/29/19 5:12 PM, Laszlo Ersek wrote:
> This reverts commit 9a2e8d7c65ef7f39c6754d27e52954b616bc6628.
> 
> The original fix for <https://bugzilla.tianocore.org/show_bug.cgi?id=1814>
> triggered a bug / incorrect assumption in QEMU.
> 
> QEMU assumes that the PCIEXBAR is below the 32-bit PCI window, not above
> it. When the firmware doesn't satisfy this assumption, QEMU generates an
> \_SB.PCI0._CRS object in the ACPI DSDT that does not reflect the
> firmware's 32-bit MMIO BAR assignments. This causes OSes to re-assign
> 32-bit MMIO BARs.
> 
> Working around the problem in the firmware looks less problematic than
> fixing QEMU. Revert the original changes first, before implementing an
> alternative fix.
> 
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1859
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>

Reviewed-by: Philippe Mathieu-Daude <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 9c013613a1a0..5e0a15484230 100644
> --- a/OvmfPkg/PlatformPei/Platform.c
> +++ b/OvmfPkg/PlatformPei/Platform.c
> @@ -181,7 +181,6 @@ 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
> @@ -193,6 +192,7 @@ MemMapInitialization (
>        PciBase = (UINT32)(PciExBarBase + SIZE_256MB);
>        PciSize = 0xFC000000 - PciBase;
>      } else {
> +      PciBase = (TopOfLowRam < BASE_2GB) ? BASE_2GB : TopOfLowRam;
>        PciSize = 0xFC000000 - PciBase;
>      }
>  
> 

[PATCH for-edk2-stable201905 4/6] Revert "OvmfPkg/PlatformPei: assign PciSize on both i440fx/q35 branches explicitly"

[Laszlo Ersek]

This reverts commit 60e95bf5094fbb9b728729ccfaf32184b3662317.

The original fix for <https://bugzilla.tianocore.org/show_bug.cgi?id=1814>
triggered a bug / incorrect assumption in QEMU.

QEMU assumes that the PCIEXBAR is below the 32-bit PCI window, not above
it. When the firmware doesn't satisfy this assumption, QEMU generates an
\_SB.PCI0._CRS object in the ACPI DSDT that does not reflect the
firmware's 32-bit MMIO BAR assignments. This causes OSes to re-assign
32-bit MMIO BARs.

Working around the problem in the firmware looks less problematic than
fixing QEMU. Revert the original changes first, before implementing an
alternative fix.

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

diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
index 5e0a15484230..0876316eefbc 100644
--- a/OvmfPkg/PlatformPei/Platform.c
+++ b/OvmfPkg/PlatformPei/Platform.c
@@ -190,10 +190,8 @@ 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;
     }
 
     //
@@ -209,6 +207,7 @@ 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: [edk2-devel] [PATCH for-edk2-stable201905 4/6] Revert "OvmfPkg/PlatformPei: assign PciSize on both i440fx/q35 branches explicitly"

[Philippe Mathieu-Daudé]

On 5/29/19 5:12 PM, Laszlo Ersek wrote:
> This reverts commit 60e95bf5094fbb9b728729ccfaf32184b3662317.
> 
> The original fix for <https://bugzilla.tianocore.org/show_bug.cgi?id=1814>
> triggered a bug / incorrect assumption in QEMU.
> 
> QEMU assumes that the PCIEXBAR is below the 32-bit PCI window, not above
> it. When the firmware doesn't satisfy this assumption, QEMU generates an
> \_SB.PCI0._CRS object in the ACPI DSDT that does not reflect the
> firmware's 32-bit MMIO BAR assignments. This causes OSes to re-assign
> 32-bit MMIO BARs.
> 
> Working around the problem in the firmware looks less problematic than
> fixing QEMU. Revert the original changes first, before implementing an
> alternative fix.
> 
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1859
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>

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

> ---
>  OvmfPkg/PlatformPei/Platform.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
> index 5e0a15484230..0876316eefbc 100644
> --- a/OvmfPkg/PlatformPei/Platform.c
> +++ b/OvmfPkg/PlatformPei/Platform.c
> @@ -190,10 +190,8 @@ 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;
>      }
>  
>      //
> @@ -209,6 +207,7 @@ MemMapInitialization (
>      // 0xFED20000    gap                          896 KB
>      // 0xFEE00000    LAPIC                          1 MB
>      //
> +    PciSize = 0xFC000000 - PciBase;
>      AddIoMemoryBaseSizeHob (PciBase, PciSize);
>      PcdStatus = PcdSet64S (PcdPciMmio32Base, PciBase);
>      ASSERT_RETURN_ERROR (PcdStatus);
> 

[PATCH for-edk2-stable201905 5/6] OvmfPkg: raise the PCIEXBAR base to 2816 MB on Q35

[Laszlo Ersek]

(This is a replacement for commit 75136b29541b, "OvmfPkg/PlatformPei:
reorder the 32-bit PCI window vs. the PCIEXBAR on q35", 2019-05-16).

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 window would follow the PCIEXBAR, covering the [0x9000_0000,
0xFC00_0000) range.)

In order to fix this, place the PCIEXBAR at 2816MB (0xB000_0000), and
start the 32-bit PCI window at 3 GB (0xC000_0000). This shrinks the 32-bit
PCI window to

  0xFC00_0000 - 0xC000_0000 = 0x3C00_0000 = 960 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.tianocore.org/show_bug.cgi?id=1859
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/OvmfPkgIa32.dsc    | 4 ++--
 OvmfPkg/OvmfPkgIa32X64.dsc | 4 ++--
 OvmfPkg/OvmfPkgX64.dsc     | 4 ++--
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
index 578fc6c98ec8..e74a9d5a5149 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -492,8 +492,8 @@ [PcdsFixedAtBuild]
   # 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
+  # never lets the RAM below 4 GB exceed 2816 MB.
+  gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0xB0000000
 
 !ifdef $(SOURCE_DEBUG_ENABLE)
   gEfiSourceLevelDebugPkgTokenSpaceGuid.PcdDebugLoadImageMethod|0x2
diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index eade8f62d3de..67ac015991fd 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -497,8 +497,8 @@ [PcdsFixedAtBuild]
   # 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
+  # never lets the RAM below 4 GB exceed 2816 MB.
+  gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0xB0000000
 
 !ifdef $(SOURCE_DEBUG_ENABLE)
   gEfiSourceLevelDebugPkgTokenSpaceGuid.PcdDebugLoadImageMethod|0x2
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index 733a4c9d8a43..68073ef55b4d 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -497,8 +497,8 @@ [PcdsFixedAtBuild]
   # 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
+  # never lets the RAM below 4 GB exceed 2816 MB.
+  gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0xB0000000
 
 !ifdef $(SOURCE_DEBUG_ENABLE)
   gEfiSourceLevelDebugPkgTokenSpaceGuid.PcdDebugLoadImageMethod|0x2
-- 
2.19.1.3.g30247aa5d201

Re: [edk2-devel] [PATCH for-edk2-stable201905 5/6] OvmfPkg: raise the PCIEXBAR base to 2816 MB on Q35

[Philippe Mathieu-Daudé]

On 5/29/19 5:12 PM, Laszlo Ersek wrote:
> (This is a replacement for commit 75136b29541b, "OvmfPkg/PlatformPei:
> reorder the 32-bit PCI window vs. the PCIEXBAR on q35", 2019-05-16).
> 
> 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 window would follow the PCIEXBAR, covering the [0x9000_0000,
> 0xFC00_0000) range.)
> 
> In order to fix this, place the PCIEXBAR at 2816MB (0xB000_0000), and
> start the 32-bit PCI window at 3 GB (0xC000_0000). This shrinks the 32-bit
> PCI window to
> 
>   0xFC00_0000 - 0xC000_0000 = 0x3C00_0000 = 960 MB.

This fix is simpler.

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

> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1859
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  OvmfPkg/OvmfPkgIa32.dsc    | 4 ++--
>  OvmfPkg/OvmfPkgIa32X64.dsc | 4 ++--
>  OvmfPkg/OvmfPkgX64.dsc     | 4 ++--
>  3 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
> index 578fc6c98ec8..e74a9d5a5149 100644
> --- a/OvmfPkg/OvmfPkgIa32.dsc
> +++ b/OvmfPkg/OvmfPkgIa32.dsc
> @@ -492,8 +492,8 @@ [PcdsFixedAtBuild]
>    # 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
> +  # never lets the RAM below 4 GB exceed 2816 MB.
> +  gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0xB0000000
>  
>  !ifdef $(SOURCE_DEBUG_ENABLE)
>    gEfiSourceLevelDebugPkgTokenSpaceGuid.PcdDebugLoadImageMethod|0x2
> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
> index eade8f62d3de..67ac015991fd 100644
> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
> @@ -497,8 +497,8 @@ [PcdsFixedAtBuild]
>    # 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
> +  # never lets the RAM below 4 GB exceed 2816 MB.
> +  gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0xB0000000
>  
>  !ifdef $(SOURCE_DEBUG_ENABLE)
>    gEfiSourceLevelDebugPkgTokenSpaceGuid.PcdDebugLoadImageMethod|0x2
> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> index 733a4c9d8a43..68073ef55b4d 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -497,8 +497,8 @@ [PcdsFixedAtBuild]
>    # 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
> +  # never lets the RAM below 4 GB exceed 2816 MB.
> +  gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0xB0000000
>  
>  !ifdef $(SOURCE_DEBUG_ENABLE)
>    gEfiSourceLevelDebugPkgTokenSpaceGuid.PcdDebugLoadImageMethod|0x2
> 

[PATCH for-edk2-stable201905 6/6] OvmfPkg/PlatformPei: set 32-bit UC area at PciBase / PciExBarBase (pc/q35)

[Laszlo Ersek]

(This is a replacement for commit 39b9a5ffe661 ("OvmfPkg/PlatformPei: fix
MTRR for low-RAM sizes that have many bits clear", 2019-05-16).)

Reintroduce the same logic as seen in commit 39b9a5ffe661 for the pc
(i440fx) board type.

For q35, the same approach doesn't work any longer, given that (a) we'd
like to keep the PCIEXBAR in the platform DSC a fixed-at-build PCD, and
(b) QEMU expects the PCIEXBAR to reside at a lower address than the 32-bit
PCI MMIO aperture.

Therefore, introduce a helper function for determining the 32-bit
"uncacheable" (MMIO) area base address:

- On q35, this function behaves statically. Furthermore, the MTRR setup
  exploits that the range [0xB000_0000, 0xFFFF_FFFF] can be marked UC with
  just two variable MTRRs (one at 0xB000_0000 (size 256MB), another at
  0xC000_0000 (size 1GB)).

- On pc (i440fx), the function behaves dynamically, implementing the same
  logic as commit 39b9a5ffe661 did. The PciBase value is adjusted to the
  value calculated, similarly to commit 39b9a5ffe661. A further
  simplification is that we show that the UC32 area size truncation to a
  whole power of two automatically guarantees a >=2GB base address.

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

diff --git a/OvmfPkg/PlatformPei/Platform.h b/OvmfPkg/PlatformPei/Platform.h
index 81af8b71480f..2f3cebcd3a6a 100644
--- a/OvmfPkg/PlatformPei/Platform.h
+++ b/OvmfPkg/PlatformPei/Platform.h
@@ -62,6 +62,11 @@ GetSystemMemorySizeBelow4gb (
   VOID
   );
 
+VOID
+QemuUc32BaseInitialization (
+  VOID
+  );
+
 VOID
 InitializeRamRegions (
   VOID
@@ -114,4 +119,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..d451989f31c9 100644
--- a/OvmfPkg/PlatformPei/MemDetect.c
+++ b/OvmfPkg/PlatformPei/MemDetect.c
@@ -14,6 +14,7 @@ Module Name:
 // The package level header files this module uses
 //
 #include <IndustryStandard/E820.h>
+#include <IndustryStandard/I440FxPiix4.h>
 #include <IndustryStandard/Q35MchIch9.h>
 #include <PiPei.h>
 
@@ -42,6 +43,8 @@ STATIC UINT32 mS3AcpiReservedMemorySize;
 
 STATIC UINT16 mQ35TsegMbytes;
 
+UINT32 mQemuUc32Base;
+
 VOID
 Q35TsegMbytesInitialization (
   VOID
@@ -98,6 +101,54 @@ Q35TsegMbytesInitialization (
 }
 
 
+VOID
+QemuUc32BaseInitialization (
+  VOID
+  )
+{
+  UINT32 LowerMemorySize;
+  UINT32 Uc32Size;
+
+  if (mXen) {
+    return;
+  }
+
+  if (mHostBridgeDevId == INTEL_Q35_MCH_DEVICE_ID) {
+    //
+    // On q35, the 32-bit area that we'll mark as UC, through variable MTRRs,
+    // starts at PcdPciExpressBaseAddress. The platform DSC is responsible for
+    // setting PcdPciExpressBaseAddress such that describing the
+    // [PcdPciExpressBaseAddress, 4GB) range require a very small number of
+    // variable MTRRs (preferably 1 or 2).
+    //
+    ASSERT (FixedPcdGet64 (PcdPciExpressBaseAddress) <= MAX_UINT32);
+    mQemuUc32Base = (UINT32)FixedPcdGet64 (PcdPciExpressBaseAddress);
+    return;
+  }
+
+  ASSERT (mHostBridgeDevId == INTEL_82441_DEVICE_ID);
+  //
+  // On i440fx, start with the [LowerMemorySize, 4GB) range. Make sure one
+  // variable MTRR suffices by truncating the size to a whole power of two,
+  // while keeping the end affixed to 4GB. This will round the base up.
+  //
+  LowerMemorySize = GetSystemMemorySizeBelow4gb ();
+  Uc32Size = GetPowerOfTwo32 ((UINT32)(SIZE_4GB - LowerMemorySize));
+  mQemuUc32Base = (UINT32)(SIZE_4GB - Uc32Size);
+  //
+  // Assuming that LowerMemorySize is at least 1 byte, Uc32Size is at most 2GB.
+  // Therefore mQemuUc32Base is at least 2GB.
+  //
+  ASSERT (mQemuUc32Base >= BASE_2GB);
+
+  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__, LowerMemorySize, mQemuUc32Base,
+      Uc32Size));
+  }
+}
+
+
 /**
   Iterate over the RAM entries in QEMU's fw_cfg E820 RAM map that start outside
   of the 32-bit address range.
@@ -688,11 +739,11 @@ QemuInitializeRam (
     ASSERT_EFI_ERROR (Status);
 
     //
-    // Set memory range from the "top of lower RAM" (RAM below 4GB) to 4GB as
-    // uncacheable
+    // Set the memory range from the start of the 32-bit MMIO area (32-bit PCI
+    // MMIO aperture on i440fx, PCIEXBAR on q35) to 4GB as uncacheable.
     //
-    Status = MtrrSetMemoryAttribute (LowerMemorySize,
-               SIZE_4GB - LowerMemorySize, CacheUncacheable);
+    Status = MtrrSetMemoryAttribute (mQemuUc32Base, SIZE_4GB - mQemuUc32Base,
+               CacheUncacheable);
     ASSERT_EFI_ERROR (Status);
   }
 }
diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
index 0876316eefbc..3ba2459872d9 100644
--- a/OvmfPkg/PlatformPei/Platform.c
+++ b/OvmfPkg/PlatformPei/Platform.c
@@ -191,7 +191,8 @@ MemMapInitialization (
       ASSERT (PciExBarBase <= MAX_UINT32 - SIZE_256MB);
       PciBase = (UINT32)(PciExBarBase + SIZE_256MB);
     } else {
-      PciBase = (TopOfLowRam < BASE_2GB) ? BASE_2GB : TopOfLowRam;
+      ASSERT (TopOfLowRam <= mQemuUc32Base);
+      PciBase = mQemuUc32Base;
     }
 
     //
@@ -650,6 +651,8 @@ InitializePlatform (
 
   PublishPeiMemory ();
 
+  QemuUc32BaseInitialization ();
+
   InitializeRamRegions ();
 
   if (mXen) {
-- 
2.19.1.3.g30247aa5d201

Re: [edk2-devel] [PATCH for-edk2-stable201905 6/6] OvmfPkg/PlatformPei: set 32-bit UC area at PciBase / PciExBarBase (pc/q35)

[Philippe Mathieu-Daudé]

On 5/29/19 5:12 PM, Laszlo Ersek wrote:
> (This is a replacement for commit 39b9a5ffe661 ("OvmfPkg/PlatformPei: fix
> MTRR for low-RAM sizes that have many bits clear", 2019-05-16).)
> 
> Reintroduce the same logic as seen in commit 39b9a5ffe661 for the pc
> (i440fx) board type.
> 
> For q35, the same approach doesn't work any longer, given that (a) we'd
> like to keep the PCIEXBAR in the platform DSC a fixed-at-build PCD, and
> (b) QEMU expects the PCIEXBAR to reside at a lower address than the 32-bit
> PCI MMIO aperture.
> 
> Therefore, introduce a helper function for determining the 32-bit
> "uncacheable" (MMIO) area base address:
> 
> - On q35, this function behaves statically. Furthermore, the MTRR setup
>   exploits that the range [0xB000_0000, 0xFFFF_FFFF] can be marked UC with
>   just two variable MTRRs (one at 0xB000_0000 (size 256MB), another at
>   0xC000_0000 (size 1GB)).
> 
> - On pc (i440fx), the function behaves dynamically, implementing the same
>   logic as commit 39b9a5ffe661 did. The PciBase value is adjusted to the
>   value calculated, similarly to commit 39b9a5ffe661. A further
>   simplification is that we show that the UC32 area size truncation to a
>   whole power of two automatically guarantees a >=2GB base address.
> 
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1859
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>

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

> ---
>  OvmfPkg/PlatformPei/Platform.h  |  7 +++
>  OvmfPkg/PlatformPei/MemDetect.c | 59 ++++++++++++++++++--
>  OvmfPkg/PlatformPei/Platform.c  |  5 +-
>  3 files changed, 66 insertions(+), 5 deletions(-)
> 
> diff --git a/OvmfPkg/PlatformPei/Platform.h b/OvmfPkg/PlatformPei/Platform.h
> index 81af8b71480f..2f3cebcd3a6a 100644
> --- a/OvmfPkg/PlatformPei/Platform.h
> +++ b/OvmfPkg/PlatformPei/Platform.h
> @@ -62,6 +62,11 @@ GetSystemMemorySizeBelow4gb (
>    VOID
>    );
>  
> +VOID
> +QemuUc32BaseInitialization (
> +  VOID
> +  );
> +
>  VOID
>  InitializeRamRegions (
>    VOID
> @@ -114,4 +119,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..d451989f31c9 100644
> --- a/OvmfPkg/PlatformPei/MemDetect.c
> +++ b/OvmfPkg/PlatformPei/MemDetect.c
> @@ -14,6 +14,7 @@ Module Name:
>  // The package level header files this module uses
>  //
>  #include <IndustryStandard/E820.h>
> +#include <IndustryStandard/I440FxPiix4.h>
>  #include <IndustryStandard/Q35MchIch9.h>
>  #include <PiPei.h>
>  
> @@ -42,6 +43,8 @@ STATIC UINT32 mS3AcpiReservedMemorySize;
>  
>  STATIC UINT16 mQ35TsegMbytes;
>  
> +UINT32 mQemuUc32Base;
> +
>  VOID
>  Q35TsegMbytesInitialization (
>    VOID
> @@ -98,6 +101,54 @@ Q35TsegMbytesInitialization (
>  }
>  
>  
> +VOID
> +QemuUc32BaseInitialization (
> +  VOID
> +  )
> +{
> +  UINT32 LowerMemorySize;
> +  UINT32 Uc32Size;
> +
> +  if (mXen) {
> +    return;
> +  }
> +
> +  if (mHostBridgeDevId == INTEL_Q35_MCH_DEVICE_ID) {
> +    //
> +    // On q35, the 32-bit area that we'll mark as UC, through variable MTRRs,
> +    // starts at PcdPciExpressBaseAddress. The platform DSC is responsible for
> +    // setting PcdPciExpressBaseAddress such that describing the
> +    // [PcdPciExpressBaseAddress, 4GB) range require a very small number of
> +    // variable MTRRs (preferably 1 or 2).
> +    //
> +    ASSERT (FixedPcdGet64 (PcdPciExpressBaseAddress) <= MAX_UINT32);
> +    mQemuUc32Base = (UINT32)FixedPcdGet64 (PcdPciExpressBaseAddress);
> +    return;
> +  }
> +
> +  ASSERT (mHostBridgeDevId == INTEL_82441_DEVICE_ID);
> +  //
> +  // On i440fx, start with the [LowerMemorySize, 4GB) range. Make sure one
> +  // variable MTRR suffices by truncating the size to a whole power of two,
> +  // while keeping the end affixed to 4GB. This will round the base up.
> +  //
> +  LowerMemorySize = GetSystemMemorySizeBelow4gb ();
> +  Uc32Size = GetPowerOfTwo32 ((UINT32)(SIZE_4GB - LowerMemorySize));
> +  mQemuUc32Base = (UINT32)(SIZE_4GB - Uc32Size);
> +  //
> +  // Assuming that LowerMemorySize is at least 1 byte, Uc32Size is at most 2GB.
> +  // Therefore mQemuUc32Base is at least 2GB.
> +  //
> +  ASSERT (mQemuUc32Base >= BASE_2GB);
> +
> +  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__, LowerMemorySize, mQemuUc32Base,
> +      Uc32Size));
> +  }
> +}
> +
> +
>  /**
>    Iterate over the RAM entries in QEMU's fw_cfg E820 RAM map that start outside
>    of the 32-bit address range.
> @@ -688,11 +739,11 @@ QemuInitializeRam (
>      ASSERT_EFI_ERROR (Status);
>  
>      //
> -    // Set memory range from the "top of lower RAM" (RAM below 4GB) to 4GB as
> -    // uncacheable
> +    // Set the memory range from the start of the 32-bit MMIO area (32-bit PCI
> +    // MMIO aperture on i440fx, PCIEXBAR on q35) to 4GB as uncacheable.
>      //
> -    Status = MtrrSetMemoryAttribute (LowerMemorySize,
> -               SIZE_4GB - LowerMemorySize, CacheUncacheable);
> +    Status = MtrrSetMemoryAttribute (mQemuUc32Base, SIZE_4GB - mQemuUc32Base,
> +               CacheUncacheable);
>      ASSERT_EFI_ERROR (Status);
>    }
>  }
> diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
> index 0876316eefbc..3ba2459872d9 100644
> --- a/OvmfPkg/PlatformPei/Platform.c
> +++ b/OvmfPkg/PlatformPei/Platform.c
> @@ -191,7 +191,8 @@ MemMapInitialization (
>        ASSERT (PciExBarBase <= MAX_UINT32 - SIZE_256MB);
>        PciBase = (UINT32)(PciExBarBase + SIZE_256MB);
>      } else {
> -      PciBase = (TopOfLowRam < BASE_2GB) ? BASE_2GB : TopOfLowRam;
> +      ASSERT (TopOfLowRam <= mQemuUc32Base);
> +      PciBase = mQemuUc32Base;
>      }
>  
>      //
> @@ -650,6 +651,8 @@ InitializePlatform (
>  
>    PublishPeiMemory ();
>  
> +  QemuUc32BaseInitialization ();
> +
>    InitializeRamRegions ();
>  
>    if (mXen) {
> 

Re: [PATCH for-edk2-stable201905 0/6] work around a QEMU issue triggered by the original TianoCore#1814 fix

[Ard Biesheuvel]

On Wed, 29 May 2019 at 17:12, Laszlo Ersek <lersek@redhat.com> wrote:
>
> Bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=1859
> Repo:     https://github.com/lersek/edk2.git
> Branch:   exbar_mtrr_bz_1859
>
> The fix (commit range 3b7a897cd8e3..39b9a5ffe661) for
> <https://bugzilla.tianocore.org/show_bug.cgi?id=1814> is technically
> correct, but it tickles an (arguably unjustified) assumption in QEMU the
> wrong way. For end users, this makes the original fix for TianoCore#1814
> a regression, under certain circumstances.
>
> In theory, the assumption should be eliminated in QEMU, but in practice,
> that could be quite intrusive and/or take long. It seems possible to
> work around the problem in OVMF, satisfying the assumption again; for
> that, OVMF needs a different fix (and a different trade-off) for the
> original problem described in TianoCore#1814.
>
> Please see the detailed problem statement and the workaround's idea in
> TianoCore#1859.
>
> If possible, I'd like to get this into edk2-stable201905 (which we're
> postponing by two weeks anyway, for the sake of the OpenSSL 1.1.1b
> upgrade).
>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
>

For the series,

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

I think the reverts obviously belong in the stable tag, and I don't
see why we shouldn't include the other two patches as well.



> Laszlo Ersek (6):
>   Revert "OvmfPkg/PlatformPei: fix MTRR for low-RAM sizes that have many
>     bits clear"
>   Revert "OvmfPkg/PlatformPei: reorder the 32-bit PCI window vs. the
>     PCIEXBAR on q35"
>   Revert "OvmfPkg/PlatformPei: hoist PciBase assignment above the
>     i440fx/q35 branching"
>   Revert "OvmfPkg/PlatformPei: assign PciSize on both i440fx/q35
>     branches explicitly"
>   OvmfPkg: raise the PCIEXBAR base to 2816 MB on Q35
>   OvmfPkg/PlatformPei: set 32-bit UC area at PciBase / PciExBarBase
>     (pc/q35)
>
>  OvmfPkg/OvmfPkgIa32.dsc         |  5 +-
>  OvmfPkg/OvmfPkgIa32X64.dsc      |  5 +-
>  OvmfPkg/OvmfPkgX64.dsc          |  5 +-
>  OvmfPkg/PlatformPei/Platform.h  |  5 ++
>  OvmfPkg/PlatformPei/MemDetect.c | 70 +++++++++++++++-----
>  OvmfPkg/PlatformPei/Platform.c  | 17 +++--
>  6 files changed, 80 insertions(+), 27 deletions(-)
>
> --
> 2.19.1.3.g30247aa5d201
>

Re: [edk2-devel] [PATCH for-edk2-stable201905 0/6] work around a QEMU issue triggered by the original TianoCore#1814 fix

[Laszlo Ersek]

On 05/29/19 17:12, Laszlo Ersek wrote:
> Bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=1859
> Repo:     https://github.com/lersek/edk2.git
> Branch:   exbar_mtrr_bz_1859
> 
> The fix (commit range 3b7a897cd8e3..39b9a5ffe661) for
> <https://bugzilla.tianocore.org/show_bug.cgi?id=1814> is technically
> correct, but it tickles an (arguably unjustified) assumption in QEMU the
> wrong way. For end users, this makes the original fix for TianoCore#1814
> a regression, under certain circumstances.
> 
> In theory, the assumption should be eliminated in QEMU, but in practice,
> that could be quite intrusive and/or take long. It seems possible to
> work around the problem in OVMF, satisfying the assumption again; for
> that, OVMF needs a different fix (and a different trade-off) for the
> original problem described in TianoCore#1814.
> 
> Please see the detailed problem statement and the workaround's idea in
> TianoCore#1859.
> 
> If possible, I'd like to get this into edk2-stable201905 (which we're
> postponing by two weeks anyway, for the sake of the OpenSSL 1.1.1b
> upgrade).
> 
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>

Thank you everyone for the feedback; I've pushed the series as commit
range f03859ea6c8f..49edde15230a.

Laszlo

> Laszlo Ersek (6):
>   Revert "OvmfPkg/PlatformPei: fix MTRR for low-RAM sizes that have many
>     bits clear"
>   Revert "OvmfPkg/PlatformPei: reorder the 32-bit PCI window vs. the
>     PCIEXBAR on q35"
>   Revert "OvmfPkg/PlatformPei: hoist PciBase assignment above the
>     i440fx/q35 branching"
>   Revert "OvmfPkg/PlatformPei: assign PciSize on both i440fx/q35
>     branches explicitly"
>   OvmfPkg: raise the PCIEXBAR base to 2816 MB on Q35
>   OvmfPkg/PlatformPei: set 32-bit UC area at PciBase / PciExBarBase
>     (pc/q35)
> 
>  OvmfPkg/OvmfPkgIa32.dsc         |  5 +-
>  OvmfPkg/OvmfPkgIa32X64.dsc      |  5 +-
>  OvmfPkg/OvmfPkgX64.dsc          |  5 +-
>  OvmfPkg/PlatformPei/Platform.h  |  5 ++
>  OvmfPkg/PlatformPei/MemDetect.c | 70 +++++++++++++++-----
>  OvmfPkg/PlatformPei/Platform.c  | 17 +++--
>  6 files changed, 80 insertions(+), 27 deletions(-)
>