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

[Laszlo Ersek <lersek@redhat.com>]

On 11/13/17 13:34, Laszlo Ersek wrote:

> I guess I could live with BASE_LIBRARY_JUMP_BUFFER.

Actually:

> 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.)

given that I'm opposed to calling any library functions before we reach
the ProcessLibraryConstructorList() call lower down in
SecCoreStartupWithStack(), I cannot agree to calling SetJump() either.

Thanks,
Laszlo