Re: [PATCH 2/4] OvmfPkg/Sec/Ia32: seed the temporary RAM with PcdInitValueInTempStack

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

On 10 November 2017 at 18:11, Laszlo Ersek <lersek@redhat.com> wrote:
> On 11/10/17 16:56, Ard Biesheuvel wrote:
>> On 10 November 2017 at 15:49, Laszlo Ersek <lersek@redhat.com> wrote:
>>> This allows the PEI core to report the maximum temporary SEC/PEI stack
>>> usage on the DEBUG_INFO level, in the PeiCheckAndSwitchStack() function
>>> [MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c]:
>>>
>>> * Normal boot:
>>>
>>>> Temp Stack : BaseAddress=0x814000 Length=0x4000
>>>> Temp Heap  : BaseAddress=0x810000 Length=0x4000
>>>> Total temporary memory:    32768 bytes.
>>>>   temporary memory stack ever used:       3664 bytes. <----
>>>>   temporary memory heap used for HobList: 5904 bytes.
>>>>   temporary memory heap occupied by memory pages: 0 bytes.
>>>
>>> * S3 resume (with PEI decompression / SMM):
>>>
>>>> Temp Stack : BaseAddress=0x814000 Length=0x4000
>>>> Temp Heap  : BaseAddress=0x810000 Length=0x4000
>>>> Total temporary memory:    32768 bytes.
>>>>   temporary memory stack ever used:       3428 bytes. <----
>>>>   temporary memory heap used for HobList: 4816 bytes.
>>>>   temporary memory heap occupied by memory pages: 0 bytes.
>>>
>>> I unit-tested this change by transitorily adding an infinite loop right
>>> after the "rep stosd", and dumping the guest's temp SEC/PEI RAM (32KB
>>> currently) while the guest was stuck in the loop. The dump includes one
>>> dword from before and after the temp SEC/PEI RAM:
>>>
>>>> $ virsh qemu-monitor-command GUEST_NAME --hmp 'xp /8194wx 0x80FFFC'
>>>>
>>>> 000000000080fffc: 0x00000000 0x5aa55aa5 0x5aa55aa5 0x5aa55aa5
>>>> 000000000081000c: 0x5aa55aa5 0x5aa55aa5 0x5aa55aa5 0x5aa55aa5
>>>> ...
>>>> 0000000000817fec: 0x5aa55aa5 0x5aa55aa5 0x5aa55aa5 0x5aa55aa5
>>>> 0000000000817ffc: 0x5aa55aa5 0x00000000
>>>
>>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>>> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
>>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=747
>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>>> ---
>>>  OvmfPkg/Sec/SecMain.inf        |  1 +
>>>  OvmfPkg/Sec/Ia32/SecEntry.nasm | 13 +++++++++++++
>>>  2 files changed, 14 insertions(+)
>>>
>>> diff --git a/OvmfPkg/Sec/SecMain.inf b/OvmfPkg/Sec/SecMain.inf
>>> index 711b59530907..6051cb3c6c4c 100644
>>> --- a/OvmfPkg/Sec/SecMain.inf
>>> +++ b/OvmfPkg/Sec/SecMain.inf
>>> @@ -71,6 +71,7 @@ [Pcd]
>>>    gEfiMdePkgTokenSpaceGuid.PcdGuidedExtractHandlerTableAddress
>>>    gUefiOvmfPkgTokenSpaceGuid.PcdGuidedExtractHandlerTableSize
>>>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfDecompressionScratchEnd
>>> +  gEfiMdeModulePkgTokenSpaceGuid.PcdInitValueInTempStack
>>>
>>>  [FeaturePcd]
>>>    gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire
>>> diff --git a/OvmfPkg/Sec/Ia32/SecEntry.nasm b/OvmfPkg/Sec/Ia32/SecEntry.nasm
>>> index 54d074e621f6..1d426fafa888 100644
>>> --- a/OvmfPkg/Sec/Ia32/SecEntry.nasm
>>> +++ b/OvmfPkg/Sec/Ia32/SecEntry.nasm
>>> @@ -29,6 +29,7 @@ extern ASM_PFX(SecCoreStartupWithStack)
>>>  ; @param[in]  EAX   Initial value of the EAX register (BIST: Built-in Self Test)
>>>  ; @param[in]  DI    'BP': boot-strap processor, or 'AP': application processor
>>>  ; @param[in]  EBP   Pointer to the start of the Boot Firmware Volume
>>> +; @param[in]  ES    Set to LINEAR_SEL in TransitionFromReal16To32BitFlat
>>
>> What does this mean? Does it belong in this patch? (Knowing you, and
>> noticing that the next patch adds it to the x86 version of this code
>> as well, I am sure it probably does, but I just need you to explain it
>> to me :-))
>
> See the STOSD instruction in the Intel SDM, or just on the web:
>
> http://x86.renejeschke.de/html/file_module_x86_id_306.html
>
>> Stores a byte, word, or doubleword from the AL, AX, or EAX register,
>> respectively, into the destination operand. The destination operand is
>> a memory location, the address of which is read from either the ES:EDI
>> or the ES:DI registers (depending on the address-size attribute of the
>> instruction, 32 or 16, respectively). The ES segment cannot be
>> overridden with a segment override prefix.
>
> It means that whatever we put in EDI, it will be relative to the segment
> "identified by" ES. (See the code comment near "mov edi": "base address,
> relative to ES".)
>
> Above I put "identified by" in quotes, because the definition of
> "identified by" depends on the mode the processor is in.
>
> (Instead of a botched attempt at writing up x86 segmentation (plus,
> optionally, paging) generally :) , I'll just leave these references
> here:
>
> * Intel SDM: 2.2 MODES OF OPERATION; Figure 2-3. Transitions Among the
>   Processor's Operating Modes
>
> * https://en.wikipedia.org/wiki/Unreal_mode
>
> * https://en.wikipedia.org/wiki/X86_memory_segmentation#80286_protected_mode
>
> * https://en.wikipedia.org/wiki/X86_memory_segmentation#80386_protected_mode
> )
>
> So using STOSD and ES:EDI as an example, here's a hugely inaccurate list
> of possible address calculations, dependent on processor mode:
>
> * real mode:
>
>   physical_address = ES * 0x10 + DI
>
>   E.g., A000:1234 means physical address A1234.
>
> * big real mode (segmentation enabled, paging disabled):
>
>   physical_address = GDT[ES].base_addr + EDI;
>
>   Basically, the value in ES is a "selector"; an offset into the GDT
>   (Global Descriptor Table). The base address of the GDT has been loaded
>   into the GDTR with the LGDT instruction. Then, at the offset
>   pointed-to by ES into the GDT, we find a segment descriptor entry,
>   which provides the base address of the segment (and some other
>   characteristics). The physical address is relative to that base.
>
> * 80386 protected mode (segmentation and paging):
>
>   virtual_address = GDT[ES].base_addr + EDI;
>   physical_address = paging(virtual_address);
>
>   Where in paging(), the CPU can use a cached translation from the TLB,
>   or perform a page table walk.
>
> In the code at hand, we are in "big real mode" (also called unreal mode,
> 32-bit flat mode). Segmentation is enabled, paging is not. The segment
> selector in ES is an "implicit" parameter for the code being added,
> because the STOSD instruction relies on ES.
>
> We move from real mode to big real mode earlier in
>
>   UefiCpuPkg/ResetVector/Vtf0/Ia16/Real16ToFlat32.asm
>
>> %define SEC_DEFAULT_CR0  0x40000023                               <---- bit 31, "Paging Enabled", is clear
>>                                                                         bit  0, "Protection Enable", is set, it will enable segmentation
>> %define SEC_DEFAULT_CR4  0x640
>>
>> BITS    16
>>
>> ;
>> ; Modified:  EAX, EBX
>> ;
>> TransitionFromReal16To32BitFlat:
>>
>>     debugShowPostCode POSTCODE_16BIT_MODE
>>
>>     cli
>>
>>     mov     bx, 0xf000
>>     mov     ds, bx
>>
>>     mov     bx, ADDR16_OF(gdtr)
>>
>> o32 lgdt    [cs:bx]                                               <---- loads the GDT's address into GDTR
>>
>>     mov     eax, SEC_DEFAULT_CR0
>>     mov     cr0, eax
>>
>>     jmp     LINEAR_CODE_SEL:dword ADDR_OF(jumpTo32BitAndLandHere) <--- actual mode switch
>> BITS    32
>> jumpTo32BitAndLandHere:
>>
>>     mov     eax, SEC_DEFAULT_CR4
>>     mov     cr4, eax
>>
>>     debugShowPostCode POSTCODE_32BIT_MODE
>>
>>     mov     ax, LINEAR_SEL
>>     mov     ds, ax
>>     mov     es, ax                                                <---- this is where ES gets the LINEAR_SEL offset into the GDT
>>     mov     fs, ax
>>     mov     gs, ax
>>     mov     ss, ax
>>
>>     OneTimeCallRet TransitionFromReal16To32BitFlat
>
> The LINEAR_SEL offset is defined lower down in the same file, where the
> actual contents of that segment descriptor are defined as well:
>
>> ;
>> ; The Global Descriptor Table (GDT)
>> ;
>>
>> GDT_BASE:
>> ; null descriptor
>> NULL_SEL            equ $-GDT_BASE
>>     DW      0            ; limit 15:0
>>     DW      0            ; base 15:0
>>     DB      0            ; base 23:16
>>     DB      0            ; sys flag, dpl, type
>>     DB      0            ; limit 19:16, flags
>>     DB      0            ; base 31:24
>>
>> ; linear data segment descriptor
>> LINEAR_SEL          equ $-GDT_BASE
>>     DW      0xffff       ; limit 15:0
>>     DW      0            ; base 15:0
>>     DB      0            ; base 23:16
>>     DB      PRESENT_FLAG(1)|DPL(0)|SYSTEM_FLAG(1)|DESC_TYPE(DATA32_TYPE)
>>     DB      GRANULARITY_FLAG(1)|DEFAULT_SIZE32(1)|CODE64_FLAG(0)|UPPER_LIMIT(0xf)
>>     DB      0            ; base 31:24
>>
>> [...]
>
> The point is that this descriptor defines a segment that is based at
> address 0, and limited at address 4GB. (Because "limit", including
> UPPER_LIMIT(0xf), gives 0xf_ffff, and GRANULARITY_FLAG(1) means the
> limit is expressed in 4KB pages).
>
> So, I added the "@param[in]  ES" comment to show that I was aware of:
>
> - being in unreal mode (in fact the file being patched itself states
>   "Processor is in flat protected mode"),
>
> - STOSD using ES,
>
> - ES having the selector value LINEAR_SEL,
>
> - the segment descriptor pointed-to by LINEAR_SEL defining a 0-based
>   32-bit segment, meant for "read/write data".
>
> In other words, that I was allowed to use EDI as a plain, zero-based
> physical address.
>

Excellent clarification, thanks.