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

[Jordan Justen <jordan.l.justen@intel.com>]

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.

-Jordan

> Can't we put an #ifndef MDEPKG_NDEBUG around the code instead?
> 
> >> >   OvmfPkg: restore temporary SEC/PEI RAM size to 64KB
> >
> > This patch is Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>
> >
> >> >
> >> >  OvmfPkg/OvmfPkgIa32.fdf        |  2 +-
> >> >  OvmfPkg/OvmfPkgIa32X64.fdf     |  2 +-
> >> >  OvmfPkg/OvmfPkgX64.fdf         |  2 +-
> >> >  OvmfPkg/Sec/SecMain.inf        |  1 +
> >> >  OvmfPkg/Sec/Ia32/SecEntry.nasm | 19 ++++++++++++++++---
> >> >  OvmfPkg/Sec/X64/SecEntry.nasm  | 15 +++++++++++++++
> >> >  6 files changed, 35 insertions(+), 6 deletions(-)
> >> >
> >> > --
> >> > 2.14.1.3.gb7cf6e02401b
> >> >