Re: dynamic PCD impact on temporary PEI memory

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

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