Re: dynamic PCD impact on temporary PEI memory

[Laszlo Ersek <lersek@redhat.com>]

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