dynamic PCD impact on temporary PEI memory

[Laszlo Ersek <lersek@redhat.com>]

Hi,

I've encountered an interesting phenomenon, and I'd like to discuss it
before I submit an OvmfPkg patch.

* Background #1:

OVMF uses 32KB total memory for temporary SEC/PEI heap and stack,
combined. The fixed PCDs for the base address of this (combined) area,
and for its size, are set in the OVMF FDF files, uniformly between IA32
/ X64 / IA32X64:

- PcdOvmfSecPeiTempRamBase = 0x00810000 (8MB + 64KB)
- PcdOvmfSecPeiTempRamSize = 0x00008000 (32KB)

See:

> [FD.MEMFD]
> BaseAddress   = $(MEMFD_BASE_ADDRESS)
> [...]
> 0x007000|0x001000
> gEfiMdePkgTokenSpaceGuid.PcdGuidedExtractHandlerTableAddress|gUefiOvmfPkgTokenSpaceGuid.PcdGuidedExtractHandlerTableSize
>
> 0x010000|0x008000
> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize
>
> 0x020000|0x0E0000
> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfPeiMemFvBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfPeiMemFvSize
> [...]

and "MEMFD_BASE_ADDRESS" comes from "OvmfPkg.fdf.inc":

> DEFINE MEMFD_BASE_ADDRESS = 0x800000

(There is a reason why I also quoted
"PcdGuidedExtractHandlerTableAddress" and "PcdOvmfPeiMemFvBase" above;
but more on that later.)

Half of this memory (16KB) is used for stack, the other half is used for
heap.


* Background #2:

"MdeModulePkg/Universal/PCD/Pei/Pcd.inf" writes (rewrapping it here for
better readability):

> #    3.1 PcdPeim and PcdDxe
> #      PEI PCD database is maintained by PcdPeim driver run from
> #      flash. PcdPeim driver build guid hob in temporary memory and
> #      copy the binary data base from flash to temporary memory for
> #      PEI PCD database.
> #      DXE PCD database is maintained by PcdDxe driver.At entry point
> #      of PcdDxe driver, a new PCD database is allocated in boot-time
> #      memory which including all PEI PCD and DXE PCD entry.

This means that the more dynamic PCDs we have in the PEI phase
(cumulatively across all PEIMs), the larger the PCD HOB will be, and the
more temporary PEI RAM (heap) will be needed.


* Background #3:

Recently we added yet another dynamic PCD to
"OvmfPkg/PlatformPei/PlatformPei.inf" -- and with that, to OVMF's PEI
phase at all --, namely

  gEfiSecurityPkgTokenSpaceGuid.PcdOptionRomImageVerificationPolicy

which has type UINT32.

Unfortunately, I messed up the patch review a bit, so I cannot point to
direct neighbor commits, from which the first doesn't have the PCD in
PlatformPei just yet, and the other commit (right after) does. There's a
build error between these two points in the git history, so the commits
that can be tested like described above are not adjacent.

Anyway, the nearest points in history that *can* be used for comparison
are:

- the commit just before adding the PCD to PlatformPei; namely
  071f1d19ddbc ("SecurityPkg: make PcdOptionRomImageVerificationPolicy
  dynamic", 2017-10-05);

- the commit when OVMF can be built again (without -D
  SECURE_BOOT_ENABLE); namely 1958124a6cb0 ("OvmfPkg: fix dynamic
  default for oprom verification policy PCD without SB", 2017-10-17).

Below I'm going to refer to these commits as "before" and "after".


* Symptom #1:

I built OVMF for IA32, IA32X64 and X64, both "before" and "after". Then
I compared the log files, to see the impact of the addition of exactly
one UINT32 dynamic PCD to the PCD HOB, on temporary PEI memory usage.

- Diff between "before" and "after", for IA32:

>  Temp Stack : BaseAddress=0x814000 Length=0x4000
>  Temp Heap  : BaseAddress=0x810000 Length=0x4000
>  Total temporary memory:    32768 bytes.
>    temporary memory stack ever used:       16384 bytes.
> -  temporary memory heap used for HobList: 5616 bytes.
> +  temporary memory heap used for HobList: 5664 bytes.
>    temporary memory heap occupied by memory pages: 0 bytes.

- Diff between "before" and "after", for IA32X64:

>  Temp Stack : BaseAddress=0x814000 Length=0x4000
>  Temp Heap  : BaseAddress=0x810000 Length=0x4000
>  Total temporary memory:    32768 bytes.
>    temporary memory stack ever used:       16384 bytes.
> -  temporary memory heap used for HobList: 5616 bytes.
> +  temporary memory heap used for HobList: 5664 bytes.
>    temporary memory heap occupied by memory pages: 0 bytes.

- Diff between "before" and "after", for IA32X64:

>  Temp Stack : BaseAddress=0x814000 Length=0x4000
>  Temp Heap  : BaseAddress=0x810000 Length=0x4000
>  Total temporary memory:    32768 bytes.
>    temporary memory stack ever used:       16384 bytes.
> -  temporary memory heap used for HobList: 7952 bytes.
> +  temporary memory heap used for HobList: 8032 bytes.
>    temporary memory heap occupied by memory pages: 0 bytes.

Observations:

- The IA32 and IA32X64 builds are identical in this regard (not
  surprisingly; the PEI phase is 32-bit in both).

- When PEI is 32-bit, the increase is 48 bytes.

- When PEI is 64-bit, the increase is 80 bytes.

- The *basis* (i.e., "before") heap consumption in 64-bit PEI is
  significantly higher than in 32-bit PEI (7952 bytes vs. 5616 bytes).
  And, the addition of the new dynamic PCD only widens the gap.


* Symptom #2:

(This is actually the symptom that causes me to write this email :) ,
I'm just laying things out in reverse order of my "discoveries".)

In my local tree, I have *another* dynamic PCD (of type BOOLEAN) in
PlatformPei (and thereby in the PEI phase). Today I've found that the
addition of

  gEfiSecurityPkgTokenSpaceGuid.PcdOptionRomImageVerificationPolicy

in *combination* with that dynamic PCD that I have privately, prevents
X64 OVMF from booting -- and only X64; IA32 and IA32X64 are fine.

For X64, the exact failure symptoms appear to vary between FD_SIZE_2MB
and FD_SIZE_4MB builds. In one case the PEI phase gets stuck with 100%
CPU consumption, in the other case the DXE core fails to initialize with
the following message:

> ASSERT MdeModulePkg/Core/Dxe/Gcd/Gcd.c(2208): Found

Anyway, let me quote the same comparisons:

- Diff between "after" and "(not really) broken", IA32:

>  Temp Stack : BaseAddress=0x814000 Length=0x4000
>  Temp Heap  : BaseAddress=0x810000 Length=0x4000
>  Total temporary memory:    32768 bytes.
>    temporary memory stack ever used:       16384 bytes.
> -  temporary memory heap used for HobList: 5664 bytes.
> +  temporary memory heap used for HobList: 5696 bytes.
>    temporary memory heap occupied by memory pages: 0 bytes.

- Diff between "after" and "(not really) broken", IA32X64:

>  Temp Stack : BaseAddress=0x814000 Length=0x4000
>  Temp Heap  : BaseAddress=0x810000 Length=0x4000
>  Total temporary memory:    32768 bytes.
>    temporary memory stack ever used:       16384 bytes.
> -  temporary memory heap used for HobList: 5664 bytes.
> +  temporary memory heap used for HobList: 5696 bytes.
>    temporary memory heap occupied by memory pages: 0 bytes.

- Diff between "after" and "broken", X64:

>  Temp Stack : BaseAddress=0x814000 Length=0x4000
>  Temp Heap  : BaseAddress=0x810000 Length=0x4000
>  Total temporary memory:    32768 bytes.
>    temporary memory stack ever used:       16384 bytes.
> -  temporary memory heap used for HobList: 8032 bytes.
> +  temporary memory heap used for HobList: 8096 bytes.
>    temporary memory heap occupied by memory pages: 0 bytes.

Observations:

- The IA32 and IA32X64 builds again behave identically.

- The increase for the IA32 and IA32X64 builds each is 32 bytes.

- The IA32 and IA32X64 builds boot fine.

- The increase for the X64 build is 64 bytes.

- With a temp heap usage of 8096 bytes (for HobList), X64 no longer
  boots.

  This value looks suspiciously close to the half of the temp heap (8192
  bytes), but maybe that *proportion* doesn't matter -- we might just
  run out of heap due to other (non-HobList) temp heap needs. And,
  perhaps, corrupt the temp stack above the temp heap.


* What to do:

At this point we are one dynamic PCD apart from breaking the PEI phase
of upstream OVMF (X64); we should prevent that.

If you scroll back to where I quoted [FD.MEMFD], you'll see that we have
32KB un-occupied both below and above the temp RAM.

- Regarding the gap below the temp RAM, it has never been used. The last
  time we took some memory from there was in 2014. See the following
  commit:

  ad43bc6b2e35 OvmfPkg: PlatformPei: protect SEC's GUIDed section
               handler table thru S3

- Regarding the gap above the temp RAM, it used to be occupied, but we
  freed it up in the following commits (parts of a larger series):

  45d870815156 OvmfPkg/PlatformPei: rebase and resize the permanent PEI
               memory for S3

  6b04cca4d697 OvmfPkg: remove PcdS3AcpiReservedMemoryBase,
               PcdS3AcpiReservedMemorySize

I suggest assigning 8KB to the temp PEI RAM from the gap *above*. Adding
8KB would increase both the temp stack and the temp heap by 1 page (4KB)
each.

What do you guys think?

Thanks,
Laszlo