dynamic PCD impact on temporary PEI memory

dynamic PCD impact on temporary PEI memory

[Laszlo Ersek]

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

Re: dynamic PCD impact on temporary PEI memory

[Ard Biesheuvel]

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

Re: dynamic PCD impact on temporary PEI memory

[Laszlo Ersek]

On 10/20/17 09:57, Ard Biesheuvel wrote:
> On 19 October 2017 at 23:08, Laszlo Ersek <lersek@redhat.com> wrote:

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

Possibly. Two observations hold me back:

- I did notice that "temporary memory stack ever used" was always the
full size. It seemed wrong (as you say), but without the actual usage
being reported, I wouldn't know how much to reassign from stack to heap.

- OVMF SEC currently halves the temp RAM between stack and heap. It
doesn't look hard to update. But, I'm not attracted to tracking down
other occurrences (if any) of same assumption in other (non-OvmfPkg)
parts of edk2.

Thanks!
Laszlo

Re: dynamic PCD impact on temporary PEI memory

[Ard Biesheuvel]

On 20 October 2017 at 10:43, Laszlo Ersek <lersek@redhat.com> wrote:
> On 10/20/17 09:57, Ard Biesheuvel wrote:
>> On 19 October 2017 at 23:08, Laszlo Ersek <lersek@redhat.com> wrote:
>
>>> * 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
>
> Possibly. Two observations hold me back:
>
> - I did notice that "temporary memory stack ever used" was always the
> full size. It seemed wrong (as you say), but without the actual usage
> being reported, I wouldn't know how much to reassign from stack to heap.
>

This change

--- a/ArmPlatformPkg/PrePeiCore/AArch64/PrePeiCoreEntryPoint.S
+++ b/ArmPlatformPkg/PrePeiCore/AArch64/PrePeiCoreEntryPoint.S
@@ -84,4 +84,11 @@ _PrepareArguments:

 _SetupPrimaryCoreStack:
   mov   sp, x1
-  b     _PrepareArguments
+
+  MOV64 (x8, FixedPcdGet32(PcdCPUCorePrimaryStackSize))
+  MOV64 (x9, 0x5aa55aa55aa55aa5)
+0:sub   x10, sp, x8
+  subs  x8, x8, #16
+  stp   x9, x9, [x10]
+  b.mi  _PrepareArguments
+  b     0b

gets me from

Total temporary memory:    16352 bytes.
  temporary memory stack ever used:       8176 bytes.
  temporary memory heap used for HobList: 4720 bytes.

to

Total temporary memory:    16352 bytes.
  temporary memory stack ever used:       4820 bytes.
  temporary memory heap used for HobList: 4720 bytes.

so it should simply be a matter of seeding the stack with the correct
magic value in the startup code. Regardless of whether this is of any
use to you in this particular case, I intend to add this to the ARM
startup code. So thanks for spotting this!

> - OVMF SEC currently halves the temp RAM between stack and heap. It
> doesn't look hard to update. But, I'm not attracted to tracking down
> other occurrences (if any) of same assumption in other (non-OvmfPkg)
> parts of edk2.
>

I would not expect there to be many platform wide assumptions
regarding how SEC deals with this, but of course, I am not coming from
the x86 world :-)

Re: dynamic PCD impact on temporary PEI memory

[Laszlo Ersek]

On 10/20/17 12:22, Ard Biesheuvel wrote:
> On 20 October 2017 at 10:43, Laszlo Ersek <lersek@redhat.com> wrote:
>> On 10/20/17 09:57, Ard Biesheuvel wrote:
>>> On 19 October 2017 at 23:08, Laszlo Ersek <lersek@redhat.com> wrote:
>>
>>>> * 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
>>
>> Possibly. Two observations hold me back:
>>
>> - I did notice that "temporary memory stack ever used" was always the
>> full size. It seemed wrong (as you say), but without the actual usage
>> being reported, I wouldn't know how much to reassign from stack to heap.
>>
> 
> This change
> 
> --- a/ArmPlatformPkg/PrePeiCore/AArch64/PrePeiCoreEntryPoint.S
> +++ b/ArmPlatformPkg/PrePeiCore/AArch64/PrePeiCoreEntryPoint.S
> @@ -84,4 +84,11 @@ _PrepareArguments:
> 
>  _SetupPrimaryCoreStack:
>    mov   sp, x1
> -  b     _PrepareArguments
> +
> +  MOV64 (x8, FixedPcdGet32(PcdCPUCorePrimaryStackSize))
> +  MOV64 (x9, 0x5aa55aa55aa55aa5)
> +0:sub   x10, sp, x8
> +  subs  x8, x8, #16
> +  stp   x9, x9, [x10]
> +  b.mi  _PrepareArguments
> +  b     0b
> 
> gets me from
> 
> Total temporary memory:    16352 bytes.
>   temporary memory stack ever used:       8176 bytes.
>   temporary memory heap used for HobList: 4720 bytes.
> 
> to
> 
> Total temporary memory:    16352 bytes.
>   temporary memory stack ever used:       4820 bytes.
>   temporary memory heap used for HobList: 4720 bytes.
> 
> so it should simply be a matter of seeding the stack with the correct
> magic value in the startup code. Regardless of whether this is of any
> use to you in this particular case, I intend to add this to the ARM
> startup code. So thanks for spotting this!

Right, we should do the same in OVMF SEC then.

I didn't realize this was a platform requirement.

Given that the platform starts using the stack immediately, after
setting it up, in retrospect it looks quite obvious that only the
platform can seed the stack.

I guess I should prepend a patch like this to the other one I planned to
post.

> 
>> - OVMF SEC currently halves the temp RAM between stack and heap. It
>> doesn't look hard to update. But, I'm not attracted to tracking down
>> other occurrences (if any) of same assumption in other (non-OvmfPkg)
>> parts of edk2.
>>
> 
> I would not expect there to be many platform wide assumptions
> regarding how SEC deals with this, but of course, I am not coming from
> the x86 world :-)

Haha :)

But, you are making a good point; I realize the PCD we use for temp RAM
size (combining stack and heap) is from the OvmfPkg.dec file -- its full
name is:

  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize

I'll look more into this as time allows.

Thanks!
Laszlo

Re: dynamic PCD impact on temporary PEI memory

[Ard Biesheuvel]

On 20 October 2017 at 11:33, Laszlo Ersek <lersek@redhat.com> wrote:
> On 10/20/17 12:22, Ard Biesheuvel wrote:
>> On 20 October 2017 at 10:43, Laszlo Ersek <lersek@redhat.com> wrote:
>>> On 10/20/17 09:57, Ard Biesheuvel wrote:
>>>> On 19 October 2017 at 23:08, Laszlo Ersek <lersek@redhat.com> wrote:
>>>
>>>>> * 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
>>>
>>> Possibly. Two observations hold me back:
>>>
>>> - I did notice that "temporary memory stack ever used" was always the
>>> full size. It seemed wrong (as you say), but without the actual usage
>>> being reported, I wouldn't know how much to reassign from stack to heap.
>>>
>>
>> This change
>>
>> --- a/ArmPlatformPkg/PrePeiCore/AArch64/PrePeiCoreEntryPoint.S
>> +++ b/ArmPlatformPkg/PrePeiCore/AArch64/PrePeiCoreEntryPoint.S
>> @@ -84,4 +84,11 @@ _PrepareArguments:
>>
>>  _SetupPrimaryCoreStack:
>>    mov   sp, x1
>> -  b     _PrepareArguments
>> +
>> +  MOV64 (x8, FixedPcdGet32(PcdCPUCorePrimaryStackSize))
>> +  MOV64 (x9, 0x5aa55aa55aa55aa5)
>> +0:sub   x10, sp, x8
>> +  subs  x8, x8, #16
>> +  stp   x9, x9, [x10]
>> +  b.mi  _PrepareArguments
>> +  b     0b
>>
>> gets me from
>>
>> Total temporary memory:    16352 bytes.
>>   temporary memory stack ever used:       8176 bytes.
>>   temporary memory heap used for HobList: 4720 bytes.
>>
>> to
>>
>> Total temporary memory:    16352 bytes.
>>   temporary memory stack ever used:       4820 bytes.
>>   temporary memory heap used for HobList: 4720 bytes.
>>
>> so it should simply be a matter of seeding the stack with the correct
>> magic value in the startup code. Regardless of whether this is of any
>> use to you in this particular case, I intend to add this to the ARM
>> startup code. So thanks for spotting this!
>
> Right, we should do the same in OVMF SEC then.
>

Yes. I'll gladly defer to the experts in this case, even though I
co-own OVMF these days ...

> I didn't realize this was a platform requirement.
>
> Given that the platform starts using the stack immediately, after
> setting it up, in retrospect it looks quite obvious that only the
> platform can seed the stack.
>

Yes. But even though PEI core seems to assume it, only Nt32Pkg appears
to implement it atm.

> I guess I should prepend a patch like this to the other one I planned to
> post.
>
>>
>>> - OVMF SEC currently halves the temp RAM between stack and heap. It
>>> doesn't look hard to update. But, I'm not attracted to tracking down
>>> other occurrences (if any) of same assumption in other (non-OvmfPkg)
>>> parts of edk2.
>>>
>>
>> I would not expect there to be many platform wide assumptions
>> regarding how SEC deals with this, but of course, I am not coming from
>> the x86 world :-)
>
> Haha :)
>
> But, you are making a good point; I realize the PCD we use for temp RAM
> size (combining stack and heap) is from the OvmfPkg.dec file -- its full
> name is:
>
>   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize
>
> I'll look more into this as time allows.
>
> Thanks!
> Laszlo