[PATCH 0/3] OvmfPkg: gigabyte page tweaks

[PATCH 0/3] OvmfPkg: gigabyte page tweaks

[Gerd Hoffmann]

Gerd Hoffmann (3):
  OvmfPkg/PlatformInitLib: check PcdUse1GPageTable
  OvmfPkg/OvmfPkgIa32X64: enable 1G pages
  OvmfPkg/MicrovmX64: enable 1G pages

 OvmfPkg/Microvm/MicrovmX64.dsc                      | 3 +++
 OvmfPkg/OvmfPkgIa32X64.dsc                          | 3 +++
 OvmfPkg/Library/PlatformInitLib/PlatformInitLib.inf | 1 +
 OvmfPkg/Library/PlatformInitLib/MemDetect.c         | 5 +++++
 4 files changed, 12 insertions(+)

-- 
2.40.1

[PATCH 1/3] OvmfPkg/PlatformInitLib: check PcdUse1GPageTable

[Gerd Hoffmann]

If PcdUse1GPageTable is not enabled restrict the physical address space
used to 1TB, to limit the amount of memory needed for identity mapping
page tables.

The same already happens in case the processor has no support for
gigabyte pages.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 OvmfPkg/Library/PlatformInitLib/PlatformInitLib.inf | 1 +
 OvmfPkg/Library/PlatformInitLib/MemDetect.c         | 5 +++++
 2 files changed, 6 insertions(+)

diff --git a/OvmfPkg/Library/PlatformInitLib/PlatformInitLib.inf b/OvmfPkg/Library/PlatformInitLib/PlatformInitLib.inf
index 86a82ad3e084..5a79d95b689c 100644
--- a/OvmfPkg/Library/PlatformInitLib/PlatformInitLib.inf
+++ b/OvmfPkg/Library/PlatformInitLib/PlatformInitLib.inf
@@ -58,6 +58,7 @@ [LibraryClasses.X64]
 
 [Pcd]
   gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress
+  gEfiMdeModulePkgTokenSpaceGuid.PcdUse1GPageTable
 
 [FixedPcd]
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfWorkAreaBase
diff --git a/OvmfPkg/Library/PlatformInitLib/MemDetect.c b/OvmfPkg/Library/PlatformInitLib/MemDetect.c
index acf90b4e93fd..1102b00ecbf0 100644
--- a/OvmfPkg/Library/PlatformInitLib/MemDetect.c
+++ b/OvmfPkg/Library/PlatformInitLib/MemDetect.c
@@ -663,6 +663,11 @@ PlatformAddressWidthFromCpuid (
       PhysBits = 40;
     }
 
+    if (!FixedPcdGetBool (PcdUse1GPageTable) && (PhysBits > 40)) {
+      DEBUG ((DEBUG_INFO, "%a: limit PhysBits to 40 (PcdUse1GPageTable is false)\n", __func__));
+      PhysBits = 40;
+    }
+
     PlatformInfoHob->PhysMemAddressWidth = PhysBits;
     PlatformInfoHob->FirstNonAddress     = LShiftU64 (1, PlatformInfoHob->PhysMemAddressWidth);
   }
-- 
2.40.1

Re: [PATCH 1/3] OvmfPkg/PlatformInitLib: check PcdUse1GPageTable

[Ard Biesheuvel]

On Wed, 17 May 2023 at 12:24, Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> If PcdUse1GPageTable is not enabled restrict the physical address space
> used to 1TB, to limit the amount of memory needed for identity mapping
> page tables.
>
> The same already happens in case the processor has no support for
> gigabyte pages.
>

Apologies for the noob question, but does this mean EDK2 maps the
entire address space 1:1, not just the region that as any DRAM in it?


> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  OvmfPkg/Library/PlatformInitLib/PlatformInitLib.inf | 1 +
>  OvmfPkg/Library/PlatformInitLib/MemDetect.c         | 5 +++++
>  2 files changed, 6 insertions(+)
>
> diff --git a/OvmfPkg/Library/PlatformInitLib/PlatformInitLib.inf b/OvmfPkg/Library/PlatformInitLib/PlatformInitLib.inf
> index 86a82ad3e084..5a79d95b689c 100644
> --- a/OvmfPkg/Library/PlatformInitLib/PlatformInitLib.inf
> +++ b/OvmfPkg/Library/PlatformInitLib/PlatformInitLib.inf
> @@ -58,6 +58,7 @@ [LibraryClasses.X64]
>
>  [Pcd]
>    gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdUse1GPageTable
>
>  [FixedPcd]
>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfWorkAreaBase
> diff --git a/OvmfPkg/Library/PlatformInitLib/MemDetect.c b/OvmfPkg/Library/PlatformInitLib/MemDetect.c
> index acf90b4e93fd..1102b00ecbf0 100644
> --- a/OvmfPkg/Library/PlatformInitLib/MemDetect.c
> +++ b/OvmfPkg/Library/PlatformInitLib/MemDetect.c
> @@ -663,6 +663,11 @@ PlatformAddressWidthFromCpuid (
>        PhysBits = 40;
>      }
>
> +    if (!FixedPcdGetBool (PcdUse1GPageTable) && (PhysBits > 40)) {
> +      DEBUG ((DEBUG_INFO, "%a: limit PhysBits to 40 (PcdUse1GPageTable is false)\n", __func__));
> +      PhysBits = 40;
> +    }
> +
>      PlatformInfoHob->PhysMemAddressWidth = PhysBits;
>      PlatformInfoHob->FirstNonAddress     = LShiftU64 (1, PlatformInfoHob->PhysMemAddressWidth);
>    }
> --
> 2.40.1
>

Re: [PATCH 1/3] OvmfPkg/PlatformInitLib: check PcdUse1GPageTable

[Gerd Hoffmann]

On Mon, May 22, 2023 at 12:37:51PM +0200, Ard Biesheuvel wrote:
> On Wed, 17 May 2023 at 12:24, Gerd Hoffmann <kraxel@redhat.com> wrote:
> >
> > If PcdUse1GPageTable is not enabled restrict the physical address space
> > used to 1TB, to limit the amount of memory needed for identity mapping
> > page tables.
> >
> > The same already happens in case the processor has no support for
> > gigabyte pages.
> >
> 
> Apologies for the noob question, but does this mean EDK2 maps the
> entire address space 1:1, not just the region that as any DRAM in it?

I just saw memory usage and boot time go up.  Didn't check whenever that
was just the 64-bit mmio area (which recent ovmf scales up with the
available address space) or all address space.

take care,
  Gerd

[PATCH 2/3] OvmfPkg/OvmfPkgIa32X64: enable 1G pages

[Gerd Hoffmann]

Reduces the memory footprint and speeds up booting.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 OvmfPkg/OvmfPkgIa32X64.dsc | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index 25974230a27e..c89257a9977e 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -557,6 +557,9 @@ [PcdsFixedAtBuild]
   # never lets the RAM below 4 GB exceed 2816 MB.
   gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0xE0000000
 
+  # use 1G pages
+  gEfiMdeModulePkgTokenSpaceGuid.PcdUse1GPageTable|TRUE
+
 !if $(SOURCE_DEBUG_ENABLE) == TRUE
   gEfiSourceLevelDebugPkgTokenSpaceGuid.PcdDebugLoadImageMethod|0x2
 !endif
-- 
2.40.1

[PATCH 3/3] OvmfPkg/MicrovmX64: enable 1G pages

[Gerd Hoffmann]

Reduces the memory footprint and speeds up booting.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 OvmfPkg/Microvm/MicrovmX64.dsc | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/OvmfPkg/Microvm/MicrovmX64.dsc b/OvmfPkg/Microvm/MicrovmX64.dsc
index 5f671bc3840d..46f951ae058f 100644
--- a/OvmfPkg/Microvm/MicrovmX64.dsc
+++ b/OvmfPkg/Microvm/MicrovmX64.dsc
@@ -544,6 +544,9 @@ [PcdsFixedAtBuild]
   gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiRuntimeServicesCode|0x100
   gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiRuntimeServicesData|0x100
 
+  # use 1G pages
+  gEfiMdeModulePkgTokenSpaceGuid.PcdUse1GPageTable|TRUE
+
   #
   # Network Pcds
   #
-- 
2.40.1

Re: [edk2-devel] [PATCH 0/3] OvmfPkg: gigabyte page tweaks

[Pedro Falcato]

On Wed, May 17, 2023 at 11:24 AM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
>
>
> Gerd Hoffmann (3):
>   OvmfPkg/PlatformInitLib: check PcdUse1GPageTable
>   OvmfPkg/OvmfPkgIa32X64: enable 1G pages
>   OvmfPkg/MicrovmX64: enable 1G pages
>
>  OvmfPkg/Microvm/MicrovmX64.dsc                      | 3 +++
>  OvmfPkg/OvmfPkgIa32X64.dsc                          | 3 +++
>  OvmfPkg/Library/PlatformInitLib/PlatformInitLib.inf | 1 +
>  OvmfPkg/Library/PlatformInitLib/MemDetect.c         | 5 +++++
>  4 files changed, 12 insertions(+)

Just a passing comment: note that this change can have possibly bad
side effects on reliability/performance.
I noticed this a few days ago when snooping around info mem/tlb for
various kernels and firmware.

The Intel SDM and AMD manuals both mention possible bad side effects
for having large pages (2MB/4MB/1GB) spanning multiple MTRRs.
Quoting from the AMD manual (just for a change, the Intel SDM has
similar passings plus a "we hacked it up because everyone was breaking
it" clause for the lower ~2MB region of physical space):
    "When paging is enabled, software can use large page sizes (2
Mbytes and 4 Mbytes) in addition to the more typical 4-Kbyte page
size. When large page sizes are used, it is possible for multiple
MTRRs to span the memory range within a single large page. Each MTRR
can characterize the regions within the page with different memory
types. If this occurs, the effective memory-type used by the processor
within the large page is undefined."

As far as I know, EDK2 (or at least my local OVMF builds) do not
handle this. In theory, you should break these particular large pages
down. In practice, nothing bad seems to have come up yet (AFAIK),
except poor-ish/poor-er performance.
So this problem is still standing (I'm meaning to take a stab at this
in the near future), but using 1GB pages may worsen the situation by
increasing the nasty overlaps a heck of a lot more.

While I don't expect my comment to be a blocker (particularly as other
OVMF platforms are already using them), I think it's probably a good
idea to let you know.

-- 
Pedro

Re: [edk2-devel] [PATCH 0/3] OvmfPkg: gigabyte page tweaks

[Gerd Hoffmann]

On Thu, May 18, 2023 at 12:16:21AM +0100, Pedro Falcato wrote:
> On Wed, May 17, 2023 at 11:24 AM Gerd Hoffmann <kraxel@redhat.com> wrote:

> The Intel SDM and AMD manuals both mention possible bad side effects
> for having large pages (2MB/4MB/1GB) spanning multiple MTRRs.

qemu takes care to use gigabyte-alignment for guest RAM for performance
reasons.  Unless you are explicitly creating something unusual the
memory layout below 4G is 2G RAM + 2G MMIO (q35 machine type) or 3G RAM
+ 1G MMIO (pc machine type).

edk2 creates mtrr uncachable entries for the mmio regions (both 32bit
and 64bit window).  On q35 it looks like this:

  reg00: base=0x080000000 ( 2048MB), size= 2048MB, count=1: uncachable
  reg01: base=0x7000000000 (458752MB), size=65536MB, count=1: uncachable

Both are gigabyte-aligned and are a multiple of gigabytes in size, so
you simply can't create gigabyte page table entries crossing mtrr entry
borders.

That's relatively recent change though, q35 used to be different, see
commit e4b3fd905a17 ("OvmfPkg/PlatformInitLib: simplify mtrr setup")

> While I don't expect my comment to be a blocker (particularly as other
> OVMF platforms are already using them), I think it's probably a good
> idea to let you know.

Yes, thanks, sure good to know.  But I don't expect that being a
problem.

take care,
  Gerd

Re: [PATCH 0/3] OvmfPkg: gigabyte page tweaks

[Ard Biesheuvel]

On Wed, 17 May 2023 at 12:24, Gerd Hoffmann <kraxel@redhat.com> wrote:
>
>
>
> Gerd Hoffmann (3):
>   OvmfPkg/PlatformInitLib: check PcdUse1GPageTable
>   OvmfPkg/OvmfPkgIa32X64: enable 1G pages
>   OvmfPkg/MicrovmX64: enable 1G pages
>

Acked-by: Ard Biesheuvel <ardb@kernel.org>

>  OvmfPkg/Microvm/MicrovmX64.dsc                      | 3 +++
>  OvmfPkg/OvmfPkgIa32X64.dsc                          | 3 +++
>  OvmfPkg/Library/PlatformInitLib/PlatformInitLib.inf | 1 +
>  OvmfPkg/Library/PlatformInitLib/MemDetect.c         | 5 +++++
>  4 files changed, 12 insertions(+)
>
> --
> 2.40.1
>

Re: [PATCH 0/3] OvmfPkg: gigabyte page tweaks

[Ard Biesheuvel]

On Thu, 25 May 2023 at 17:48, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Wed, 17 May 2023 at 12:24, Gerd Hoffmann <kraxel@redhat.com> wrote:
> >
> >
> >
> > Gerd Hoffmann (3):
> >   OvmfPkg/PlatformInitLib: check PcdUse1GPageTable
> >   OvmfPkg/OvmfPkgIa32X64: enable 1G pages
> >   OvmfPkg/MicrovmX64: enable 1G pages
> >
>
> Acked-by: Ard Biesheuvel <ardb@kernel.org>
>


Merged as #4441