Re: dynamic PCD impact on temporary PEI memory

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 19 October 2017 at 23:08, Laszlo Ersek <lersek@redhat.com> wrote:
> 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.

The code that performs this check looks broken to me btw: it looks for
INIT_CAR_VALUE on the stack, but it is not clear to me where the stack
is initialised with this value, and that fact that we always seem to
use exactly the entire stack looks suspicious as well.

So perhaps you could reuse some of that space for the heap as well?

#define INIT_CAR_VALUE 0x5AA55AA5


>> -  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