Re: [PATCH 0/4] OvmfPkg: measure temp stack usage, restore temp RAM to 64KB

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

On 13 November 2017 at 09:08, Jordan Justen <jordan.l.justen@intel.com> wrote:
> On 2017-11-12 02:58:37, Ard Biesheuvel wrote:
>> On 11 November 2017 at 22:04, Jordan Justen <jordan.l.justen@intel.com> wrote:
>> > On 2017-11-11 12:38:21, Jordan Justen wrote:
>> >> On 2017-11-10 07:49:04, Laszlo Ersek wrote:
>> >> > The first three patches enable the PEI_CORE to report OVMF's temp
>> >> > SEC/PEI stack and heap usage.
>> >> >
>> >> >   - This depends on the new fixed PCD "PcdInitValueInTempStack",
>> >> >     recently added for
>> >> >     <https://bugzilla.tianocore.org/show_bug.cgi?id=740>
>> >> >     ("INIT_CAR_VALUE should be defined in a central location").
>> >> >
>> >> >   - Ard recently implemented the same in ArmPlatformPkg, for
>> >> >     <https://bugzilla.tianocore.org/show_bug.cgi?id=748> ("measure temp
>> >> >     SEC/PEI stack usage").
>> >> >
>> >> > The last (fourth) patch adapts OVMF to the larger MtrrLib stack demand
>> >> > that originates from commit 2bbd7e2fbd4b ("UefiCpuPkg/MtrrLib: Update
>> >> > algorithm to calculate optimal settings", 2017-09-27). OVMF's temp RAM
>> >> > size is restored to the historical / original 64KB.
>> >> >
>> >> > This work is tracked in
>> >> > <https://bugzilla.tianocore.org/show_bug.cgi?id=747> ("measure temp
>> >> > SEC/PEI stack usage; grow temp SEC/PEI RAM"), with links to related
>> >> > mailing list discussions.
>> >> >
>> >> > Repo:   https://github.com/lersek/edk2.git
>> >> > Branch: temp_ram_tweaks
>> >> >
>> >> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> >> > Cc: Jordan Justen <jordan.l.justen@intel.com>
>> >> > Cc: Ruiyu Ni <ruiyu.ni@intel.com>
>> >> >
>> >> > Thanks
>> >> > Laszlo
>> >> >
>> >> > Laszlo Ersek (4):
>> >> >   OvmfPkg/Sec/Ia32: free up EAX for other uses while setting up the
>> >> >     stack
>> >> >   OvmfPkg/Sec/Ia32: seed the temporary RAM with PcdInitValueInTempStack
>> >> >   OvmfPkg/Sec/X64: seed the temporary RAM with PcdInitValueInTempStack
>> >>
>> >> I'd like to try a different option for these 3. Can you hold off a bit
>> >> before pushing this series?
>> >
>> > I think we should use a C based approach instead, like in the attached
>> > patch.
>> >
>>
>> I'm not sure: having to abuse SetJump ()
>
> True, that was annoying. It seems like we could have AsmReadEsp and
> AsmReadRsp in BaseLib since we have AsmReadSp for IPF.
>
>> and having to leave an
>> arbitrary 512 byte window both seem pretty good reasons to stick with
>> assembly.
>
> Also true. I chose 512 because it seemed like more than SetMem32 could
> reasonably need, but also much below the minimum I would expect PEI to
> use. (It seemed that around 4k ended up being used.)
>
>> Is your concern that the stack gets cleared in RELEASE builds as well?
>
> No. I just prefer if we can use C rather than assembly whenever it is
> reasonable.
>

No discussion there. But in my opinion, anything involving the
absolute value of the stack pointer does not belong in C.