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

[Laszlo Ersek <lersek@redhat.com>]

On 11/13/17 11:09, Ard Biesheuvel wrote:
> 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.
>

I chose assembly because it seemed much cleaner and simpler to seed the
stack before C code actually started using the stack.

GCC sometimes plays dirty tricks with laying out local variables on the
stack, so addresses of local variables cannot / should not be used for
this purpose. (For example, see commit f98f5ec304ec, "UefiCpuPkg:
S3Resume2Pei: align return stacks explicitly", 2013-12-13.)

BASE_LIBRARY_JUMP_BUFFER didn't occur to me (I've always considered jump
buffers opaque to client code).

AsmReadSp() is IPF only (the BaseLib class header says so, and the tree
agrees).

AsmReadEsp() and AsmReadRsp() do not exist in BaseLib, and they would
only be implementable for x86. I think platform-dependent library
*interfaces* don't belong into BaseLib (in other words, AsmReadSp() is
already a mistake in my opinion; we had better not make it worse).

I guess I could live with BASE_LIBRARY_JUMP_BUFFER.

More specific comments:

> diff --git a/OvmfPkg/Sec/SecMain.c b/OvmfPkg/Sec/SecMain.c
> index f7fec3d8c0..077f7d6563 100644
> --- a/OvmfPkg/Sec/SecMain.c
> +++ b/OvmfPkg/Sec/SecMain.c
> @@ -1,7 +1,7 @@
>  /** @file
>    Main SEC phase code.  Transitions to PEI.
>
> -  Copyright (c) 2008 - 2015, Intel Corporation. All rights reserved.<BR>
> +  Copyright (c) 2008 - 2017, Intel Corporation. All rights reserved.<BR>
>    (C) Copyright 2016 Hewlett Packard Enterprise Development LP<BR>
>
>    This program and the accompanying materials
> @@ -731,6 +731,25 @@ SecCoreStartupWithStack (
>    UINT32                      Index;
>    volatile UINT8              *Table;
>
> +  //
> +  // Fill most of temporary RAM with PcdInitValueInTempStack. We stop
> +  // filling at the current stack pointer - 512 bytes.
> +  //
> +  DEBUG_CODE_BEGIN ();
> +  BASE_LIBRARY_JUMP_BUFFER  JumpBuffer;
> +  UINTN                     StackUsed;
> +
> +  SetJump (&JumpBuffer);
> +#if defined (MDE_CPU_IA32)
> +  StackUsed = (UINTN)TopOfCurrentStack - JumpBuffer.Esp;
> +#elif defined (MDE_CPU_X64)
> +  StackUsed = (UINTN)TopOfCurrentStack - JumpBuffer.Rsp;
> +#endif
> +  SetMem32 ((VOID*)(UINTN)PcdGet32 (PcdOvmfSecPeiTempRamBase),
> +            PcdGet32 (PcdOvmfSecPeiTempRamSize) - StackUsed - 512,
> +            FixedPcdGet32 (PcdInitValueInTempStack));

(1) SetMem32() is likely problematic in itself; please refer to the
following comment -- partly visible in the context of Jordan's patch --,
from commit 320b4f084a25 ("OvmfPkg: Sec: force reinit of
BaseExtractGuidedSectionLib handler table", 2015-11-30):

  //
  // To ensure SMM can't be compromised on S3 resume, we must force re-init of
  // the BaseExtractGuidedSectionLib. Since this is before library contructors
  // are called, we must use a loop rather than SetMem.
  //

Thus, we should use a loop and a pointer-to-volatile. (It would likely
be slower than the REP STOSD / REP STOSQ.)


(2) The indentation of arguments is off.

(It doesn't matter if we replace SetMem32() anyway, due to (1).)


(3) If we replace SetMem32() with a loop, and (consequently) there's no
risk for SetMem32() to bust its own stack / return address, then how
should the constant 512 change? Does it become zero?

What does GCC guarantee about the value of ESP / RSP at that point,
versus the addresses of SecCoreStartupWithStack()'s own local variables?

> +  DEBUG_CODE_END ();
> +
>    //
>    // To ensure SMM can't be compromised on S3 resume, we must force re-init of
>    // the BaseExtractGuidedSectionLib. Since this is before library contructors

Thanks
Laszlo