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

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

[Laszlo Ersek]

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
  OvmfPkg: restore temporary SEC/PEI RAM size to 64KB

 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

[PATCH 1/4] OvmfPkg/Sec/Ia32: free up EAX for other uses while setting up the stack

[Laszlo Ersek]

Currently EAX is used as an intermediary in setting ESP to
SEC_TOP_OF_STACK, and in passing SEC_TOP_OF_STACK to
SecCoreStartupWithStack() as the "TopOfCurrentStack" argument.

In a later patch we'll use EAX differently, so replace it with EBX under
the current use.

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/Sec/Ia32/SecEntry.nasm | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/OvmfPkg/Sec/Ia32/SecEntry.nasm b/OvmfPkg/Sec/Ia32/SecEntry.nasm
index 7fee1c2b2e4f..54d074e621f6 100644
--- a/OvmfPkg/Sec/Ia32/SecEntry.nasm
+++ b/OvmfPkg/Sec/Ia32/SecEntry.nasm
@@ -40,8 +40,8 @@ ASM_PFX(_ModuleEntryPoint):
     ;
     %define SEC_TOP_OF_STACK (FixedPcdGet32 (PcdOvmfSecPeiTempRamBase) + \
                           FixedPcdGet32 (PcdOvmfSecPeiTempRamSize))
-    mov     eax, SEC_TOP_OF_STACK
-    mov     esp, eax
+    mov     ebx, SEC_TOP_OF_STACK
+    mov     esp, ebx
     nop
 
     ;
@@ -50,7 +50,7 @@ ASM_PFX(_ModuleEntryPoint):
     ;   [esp+4] BootFirmwareVolumePtr
     ;   [esp+8] TopOfCurrentStack
     ;
-    push    eax
+    push    ebx
     push    ebp
     call    ASM_PFX(SecCoreStartupWithStack)
 
-- 
2.14.1.3.gb7cf6e02401b

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

[Laszlo Ersek]

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
 ;
 ; @return     None  This routine does not return
 ;
@@ -44,6 +45,18 @@ ASM_PFX(_ModuleEntryPoint):
     mov     esp, ebx
     nop
 
+    ;
+    ; Fill the temporary RAM with the initial stack value.
+    ; The loop below will seed the heap as well, but that's harmless.
+    ;
+    mov     eax, FixedPcdGet32 (PcdInitValueInTempStack)  ; dword to store
+    mov     edi, FixedPcdGet32 (PcdOvmfSecPeiTempRamBase) ; base address,
+                                                          ;   relative to ES
+    mov     ecx, FixedPcdGet32 (PcdOvmfSecPeiTempRamSize) ; byte count
+    shr     ecx, 2                                        ; dword count
+    cld                                                   ; store from base up
+    rep stosd
+
     ;
     ; Setup parameters and call SecCoreStartupWithStack
     ;   [esp]   return address for call
-- 
2.14.1.3.gb7cf6e02401b

[PATCH 3/4] OvmfPkg/Sec/X64: seed the temporary RAM with PcdInitValueInTempStack

[Laszlo Ersek]

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:       5080 bytes. <----
>   temporary memory heap used for HobList: 8080 bytes.
>   temporary memory heap occupied by memory pages: 0 bytes.

* S3 resume (no SMM / PEI decompression)

> Temp Stack : BaseAddress=0x814000 Length=0x4000
> Temp Heap  : BaseAddress=0x810000 Length=0x4000
> Total temporary memory:    32768 bytes.
>   temporary memory stack ever used:       5048 bytes. <----
>   temporary memory heap used for HobList: 7112 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 stosq", 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/X64/SecEntry.nasm | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/OvmfPkg/Sec/X64/SecEntry.nasm b/OvmfPkg/Sec/X64/SecEntry.nasm
index f40427aa8e04..686008f41e15 100644
--- a/OvmfPkg/Sec/X64/SecEntry.nasm
+++ b/OvmfPkg/Sec/X64/SecEntry.nasm
@@ -30,6 +30,7 @@ extern ASM_PFX(SecCoreStartupWithStack)
 ; @param[in]  RAX   Initial value of the EAX register (BIST: Built-in Self Test)
 ; @param[in]  DI    'BP': boot-strap processor, or 'AP': application processor
 ; @param[in]  RBP   Pointer to the start of the Boot Firmware Volume
+; @param[in]  ES    Set to LINEAR_SEL in TransitionFromReal16To32BitFlat
 ;
 ; @return     None  This routine does not return
 ;
@@ -44,6 +45,20 @@ ASM_PFX(_ModuleEntryPoint):
     mov     rsp, SEC_TOP_OF_STACK
     nop
 
+    ;
+    ; Fill the temporary RAM with the initial stack value.
+    ; The loop below will seed the heap as well, but that's harmless.
+    ;
+    mov     rax, FixedPcdGet32 (PcdInitValueInTempStack)  ; dword to store
+    shl     rax, 32
+    or      rax, FixedPcdGet32 (PcdInitValueInTempStack)  ; qword to store
+    mov     rdi, FixedPcdGet32 (PcdOvmfSecPeiTempRamBase) ; base address,
+                                                          ;   relative to ES
+    mov     rcx, FixedPcdGet32 (PcdOvmfSecPeiTempRamSize) ; byte count
+    shr     rcx, 3                                        ; qword count
+    cld                                                   ; store from base up
+    rep stosq
+
     ;
     ; Setup parameters and call SecCoreStartupWithStack
     ;   rcx: BootFirmwareVolumePtr
-- 
2.14.1.3.gb7cf6e02401b

[PATCH 4/4] OvmfPkg: restore temporary SEC/PEI RAM size to 64KB

[Laszlo Ersek]

(1) In the PEI phase, the PCD database is maintained in a GUID HOB. In
    OVMF, we load the PCD PEIM before any other PEIMs (using APRIORI PEI),
    so that all other PEIMs can use dynamic PCDs. Consequently,

    - the PCD GUID HOB is initially allocated from the temporary SEC/PEI
      heap,

    - whenever we introduce a dynamic PCD to a PEIM built into OVMF such
      that the PCD is new to OVMF's whole PEI phase, the PCD GUID HOB (and
      its temporary heap footprint) grow.

    I've noticed that, if we add just one more dynamic PCD to the PEI
    phase, then in the X64 build,

    - we get very close to the half of the temporary heap (i.e., 8192
      bytes),

    - obscure PEI phase hangs or DXE core initialization failures
      (ASSERTs) occur. The symptoms vary between the FD_SIZE_2MB and
      FD_SIZE_4MB builds of X64 OVMF.

(2) I've found that commit

      2bbd7e2fbd4b ("UefiCpuPkg/MtrrLib: Update algorithm to calculate
                    optimal settings", 2017-09-27)

    introduced a large (16KB) stack allocation:

> The patch changes existing MtrrSetMemoryAttributeInMtrrSettings() and
> MtrrSetMemoryAttribute() to use the 4-page stack buffer for calculation.
> ...
> +#define SCRATCH_BUFFER_SIZE           (4 * SIZE_4KB)
> ...
> @@ -2207,17 +2462,66 @@ MtrrSetMemoryAttributeInMtrrSettings (
> ...
> +  UINT8                      Scratch[SCRATCH_BUFFER_SIZE];

(3) OVMF's temp SEC/PEI RAM size has been 32KB ever since commit

      7cb6b0e06809 ("OvmfPkg: Move SEC/PEI Temporary RAM from 0x70000 to
                    0x810000", 2014-01-21)

    Of that, the upper 16KB half is stack (growing down), and the lower
    16KB half is heap.

    Thus, OvmfPkg/PlatformPei's calls to "UefiCpuPkg/Library/MtrrLib", in
    QemuInitializeRam(), cause the Scratch array to overflow the entire
    stack (heading towards lower addresses), and corrupt the heap below
    the stack. It turns out that the total stack demand is about 24KB, so
    the overflow is able to corrupt the upper 8KB of the heap. If that
    part of the heap is actually used (for example because we grow the PCD
    GUID HOB sufficiently), mayhem ensues.

(4) Right after commit 7cb6b0e06809 (see above), there would be no room
    left above the 32KB temp SEC/PEI RAM. However, given more recent
    commits

      45d870815156 ("OvmfPkg/PlatformPei: rebase and resize the permanent
                    PEI memory for S3", 2016-07-13)

      6b04cca4d697 ("OvmfPkg: remove PcdS3AcpiReservedMemoryBase,
                    PcdS3AcpiReservedMemorySize", 2016-07-12)

    we can now restore the temp SEC/PEI RAM size to the original
    (pre-7cb6b0e06809) 64KB. This will allow for a 32KB temp SEC/PEI
    stack, which accommodates the ~24KB demand mentioned in (3).

    (Prior patches in this series will let us monitor the stack usage in
    the future.)

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
Ref: http://mid.mail-archive.com/a49cc089-12ae-a887-a4d6-4dc509233a74@redhat.com
Ref: http://mid.mail-archive.com/03e369bb-77c4-0134-258f-bdae62cbc8c5@redhat.com
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/OvmfPkgIa32.fdf    | 2 +-
 OvmfPkg/OvmfPkgIa32X64.fdf | 2 +-
 OvmfPkg/OvmfPkgX64.fdf     | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/OvmfPkg/OvmfPkgIa32.fdf b/OvmfPkg/OvmfPkgIa32.fdf
index 751522411857..06a439f8cba5 100644
--- a/OvmfPkg/OvmfPkgIa32.fdf
+++ b/OvmfPkg/OvmfPkgIa32.fdf
@@ -82,7 +82,7 @@ [FD.MEMFD]
 0x007000|0x001000
 gEfiMdePkgTokenSpaceGuid.PcdGuidedExtractHandlerTableAddress|gUefiOvmfPkgTokenSpaceGuid.PcdGuidedExtractHandlerTableSize
 
-0x010000|0x008000
+0x010000|0x010000
 gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize
 
 0x020000|0x0E0000
diff --git a/OvmfPkg/OvmfPkgIa32X64.fdf b/OvmfPkg/OvmfPkgIa32X64.fdf
index f1a2044fb716..ced4c5639f39 100644
--- a/OvmfPkg/OvmfPkgIa32X64.fdf
+++ b/OvmfPkg/OvmfPkgIa32X64.fdf
@@ -82,7 +82,7 @@ [FD.MEMFD]
 0x007000|0x001000
 gEfiMdePkgTokenSpaceGuid.PcdGuidedExtractHandlerTableAddress|gUefiOvmfPkgTokenSpaceGuid.PcdGuidedExtractHandlerTableSize
 
-0x010000|0x008000
+0x010000|0x010000
 gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize
 
 0x020000|0x0E0000
diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf
index 32000a3b934c..62dd58f6e47a 100644
--- a/OvmfPkg/OvmfPkgX64.fdf
+++ b/OvmfPkg/OvmfPkgX64.fdf
@@ -82,7 +82,7 @@ [FD.MEMFD]
 0x007000|0x001000
 gEfiMdePkgTokenSpaceGuid.PcdGuidedExtractHandlerTableAddress|gUefiOvmfPkgTokenSpaceGuid.PcdGuidedExtractHandlerTableSize
 
-0x010000|0x008000
+0x010000|0x010000
 gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize
 
 0x020000|0x0E0000
-- 
2.14.1.3.gb7cf6e02401b

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

[Ard Biesheuvel]

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


>  ;
>  ; @return     None  This routine does not return
>  ;
> @@ -44,6 +45,18 @@ ASM_PFX(_ModuleEntryPoint):
>      mov     esp, ebx
>      nop
>
> +    ;
> +    ; Fill the temporary RAM with the initial stack value.
> +    ; The loop below will seed the heap as well, but that's harmless.
> +    ;
> +    mov     eax, FixedPcdGet32 (PcdInitValueInTempStack)  ; dword to store
> +    mov     edi, FixedPcdGet32 (PcdOvmfSecPeiTempRamBase) ; base address,
> +                                                          ;   relative to ES
> +    mov     ecx, FixedPcdGet32 (PcdOvmfSecPeiTempRamSize) ; byte count
> +    shr     ecx, 2                                        ; dword count
> +    cld                                                   ; store from base up
> +    rep stosd
> +
>      ;
>      ; Setup parameters and call SecCoreStartupWithStack
>      ;   [esp]   return address for call
> --
> 2.14.1.3.gb7cf6e02401b
>
>

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

[Laszlo Ersek]

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.

Thanks,
Laszlo


>>  ;
>>  ; @return     None  This routine does not return
>>  ;
>> @@ -44,6 +45,18 @@ ASM_PFX(_ModuleEntryPoint):
>>      mov     esp, ebx
>>      nop
>>
>> +    ;
>> +    ; Fill the temporary RAM with the initial stack value.
>> +    ; The loop below will seed the heap as well, but that's harmless.
>> +    ;
>> +    mov     eax, FixedPcdGet32 (PcdInitValueInTempStack)  ; dword to store
>> +    mov     edi, FixedPcdGet32 (PcdOvmfSecPeiTempRamBase) ; base address,
>> +                                                          ;   relative to ES
>> +    mov     ecx, FixedPcdGet32 (PcdOvmfSecPeiTempRamSize) ; byte count
>> +    shr     ecx, 2                                        ; dword count
>> +    cld                                                   ; store from base up
>> +    rep stosd
>> +
>>      ;
>>      ; Setup parameters and call SecCoreStartupWithStack
>>      ;   [esp]   return address for call
>> --
>> 2.14.1.3.gb7cf6e02401b
>>
>>

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

[Laszlo Ersek]

On 11/10/17 19:11, Laszlo Ersek 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.

I should add that the X64 version is a bit different, as far as the
"environment" goes. In the X64 version, paging *is* enabled (we build
the page tables, and enter long (64-bit) mode, *between* the code quoted
above, and the code added in the X64 patch). However, the segment
described by ES is the same (it remains limited to 4GB). Furthermore,
the (identity-mapping) page tables are only built for the first 4GB of RAM.

Thus the address translation is more involved in the X64 variant, but
ultimately RDI can be used in the exact same sense ("plain, zero-based
physical address").

Thanks
Laszlo

>>>  ;
>>>  ; @return     None  This routine does not return
>>>  ;
>>> @@ -44,6 +45,18 @@ ASM_PFX(_ModuleEntryPoint):
>>>      mov     esp, ebx
>>>      nop
>>>
>>> +    ;
>>> +    ; Fill the temporary RAM with the initial stack value.
>>> +    ; The loop below will seed the heap as well, but that's harmless.
>>> +    ;
>>> +    mov     eax, FixedPcdGet32 (PcdInitValueInTempStack)  ; dword to store
>>> +    mov     edi, FixedPcdGet32 (PcdOvmfSecPeiTempRamBase) ; base address,
>>> +                                                          ;   relative to ES
>>> +    mov     ecx, FixedPcdGet32 (PcdOvmfSecPeiTempRamSize) ; byte count
>>> +    shr     ecx, 2                                        ; dword count
>>> +    cld                                                   ; store from base up
>>> +    rep stosd
>>> +
>>>      ;
>>>      ; Setup parameters and call SecCoreStartupWithStack
>>>      ;   [esp]   return address for call
>>> --
>>> 2.14.1.3.gb7cf6e02401b
>>>
>>>
> 

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

[Ard Biesheuvel]

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.

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

[Ard Biesheuvel]

On 10 November 2017 at 15:49, Laszlo Ersek <lersek@redhat.com> 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
>   OvmfPkg: restore temporary SEC/PEI RAM size to 64KB
>

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>


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

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

[Jordan Justen]

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?

-Jordan

>   OvmfPkg: restore temporary SEC/PEI RAM size to 64KB
> 
>  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
> 

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

[Jordan Justen]

[-- Attachment #1: Type: text/plain, Size: 2384 bytes --]

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.

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

[-- Attachment #2: 0001-OvmfPkg-Sec-Fill-most-of-temp-RAM-with-PcdInitValueI.patch --]
[-- Type: text/x-diff, Size: 2952 bytes --]

>From 0acb3277e5f7b6ac195c6dbf4fc3ce544e65d9a7 Mon Sep 17 00:00:00 2001
From: Jordan Justen <jordan.l.justen@intel.com>
Date: Sat, 11 Nov 2017 13:25:35 -0800
Subject: [PATCH] OvmfPkg/Sec: Fill most of temp RAM with
 PcdInitValueInTempStack

During a DEBUG build this allows the PEI dispatcher to report the
amount of stack used by SEC/PEI.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jordan Justen <jordan.l.justen@intel.com>
---
 OvmfPkg/Sec/SecMain.c   | 21 ++++++++++++++++++++-
 OvmfPkg/Sec/SecMain.inf |  3 ++-
 2 files changed, 22 insertions(+), 2 deletions(-)

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));
+  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
diff --git a/OvmfPkg/Sec/SecMain.inf b/OvmfPkg/Sec/SecMain.inf
index 711b595309..c63215393b 100644
--- a/OvmfPkg/Sec/SecMain.inf
+++ b/OvmfPkg/Sec/SecMain.inf
@@ -1,7 +1,7 @@
 ## @file
 #  SEC Driver
 #
-#  Copyright (c) 2008 - 2015, Intel Corporation. All rights reserved.<BR>
+#  Copyright (c) 2008 - 2017, Intel Corporation. All rights reserved.<BR>
 #
 #  This program and the accompanying materials
 #  are licensed and made available under the terms and conditions of the BSD License
@@ -71,6 +71,7 @@
   gEfiMdePkgTokenSpaceGuid.PcdGuidedExtractHandlerTableAddress
   gUefiOvmfPkgTokenSpaceGuid.PcdGuidedExtractHandlerTableSize
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfDecompressionScratchEnd
+  gEfiMdeModulePkgTokenSpaceGuid.PcdInitValueInTempStack
 
 [FeaturePcd]
   gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire
-- 
2.15.0

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

[Ard Biesheuvel]

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 () and having to leave an
arbitrary 512 byte window both seem pretty good reasons to stick with
assembly.

Is your concern that the stack gets cleared in RELEASE builds as well?
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
>> >

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

[Jordan Justen]

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

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

[Ard Biesheuvel]

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.

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

[Laszlo Ersek]

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

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

[Laszlo Ersek]

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

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

[Jordan Justen]

On 2017-11-13 04:34:05, Laszlo Ersek wrote:
> 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).

There are plenty of arch specific register accessor functions in
BaseLib.

-Jordan

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

[Jordan Justen]

On 2017-11-13 05:09:03, Laszlo Ersek wrote:
> 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.

Good point.

-Jordan

Re: [PATCH 1/4] OvmfPkg/Sec/Ia32: free up EAX for other uses while setting up the stack

[Jordan Justen]

On 2017-11-10 07:49:05, Laszlo Ersek wrote:
> Currently EAX is used as an intermediary in setting ESP to
> SEC_TOP_OF_STACK, and in passing SEC_TOP_OF_STACK to
> SecCoreStartupWithStack() as the "TopOfCurrentStack" argument.
> 
> In a later patch we'll use EAX differently, so replace it with EBX under
> the current use.
> 
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  OvmfPkg/Sec/Ia32/SecEntry.nasm | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/OvmfPkg/Sec/Ia32/SecEntry.nasm b/OvmfPkg/Sec/Ia32/SecEntry.nasm
> index 7fee1c2b2e4f..54d074e621f6 100644
> --- a/OvmfPkg/Sec/Ia32/SecEntry.nasm
> +++ b/OvmfPkg/Sec/Ia32/SecEntry.nasm
> @@ -40,8 +40,8 @@ ASM_PFX(_ModuleEntryPoint):
>      ;
>      %define SEC_TOP_OF_STACK (FixedPcdGet32 (PcdOvmfSecPeiTempRamBase) + \
>                            FixedPcdGet32 (PcdOvmfSecPeiTempRamSize))
> -    mov     eax, SEC_TOP_OF_STACK
> -    mov     esp, eax
> +    mov     ebx, SEC_TOP_OF_STACK
> +    mov     esp, ebx

Can't you initialize the temp RAM before this code?

-Jordan

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

[Jordan Justen]

On 2017-11-10 07:49:06, Laszlo Ersek wrote:
> 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

Can you document all the segment registers, and also document them in
UefiCpuPkg/ResetVector/Vtf0/Main.asm?

>  ;
>  ; @return     None  This routine does not return
>  ;
> @@ -44,6 +45,18 @@ ASM_PFX(_ModuleEntryPoint):
>      mov     esp, ebx
>      nop
>  
> +    ;
> +    ; Fill the temporary RAM with the initial stack value.
> +    ; The loop below will seed the heap as well, but that's harmless.
> +    ;
> +    mov     eax, FixedPcdGet32 (PcdInitValueInTempStack)  ; dword to store
> +    mov     edi, FixedPcdGet32 (PcdOvmfSecPeiTempRamBase) ; base address,
> +                                                          ;   relative to ES
> +    mov     ecx, FixedPcdGet32 (PcdOvmfSecPeiTempRamSize) ; byte count
> +    shr     ecx, 2                                        ; dword count

I'm not sure, but I think NASM might let you do something like:

    mov     ecx, FixedPcdGet32 (PcdOvmfSecPeiTempRamSize) / 4

> +    cld                                                   ; store from base up
> +    rep stosd

I think if you move this above the code in patch 1, then patch 1 is
not needed. I also think it would be reasonable to merge 2 & 3, but
separate is fine too.

-Jordan

Re: [PATCH 1/4] OvmfPkg/Sec/Ia32: free up EAX for other uses while setting up the stack

[Laszlo Ersek]

On 11/13/17 19:08, Jordan Justen wrote:
> On 2017-11-10 07:49:05, Laszlo Ersek wrote:
>> Currently EAX is used as an intermediary in setting ESP to
>> SEC_TOP_OF_STACK, and in passing SEC_TOP_OF_STACK to
>> SecCoreStartupWithStack() as the "TopOfCurrentStack" argument.
>>
>> In a later patch we'll use EAX differently, so replace it with EBX under
>> the current use.
>>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>>  OvmfPkg/Sec/Ia32/SecEntry.nasm | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/OvmfPkg/Sec/Ia32/SecEntry.nasm b/OvmfPkg/Sec/Ia32/SecEntry.nasm
>> index 7fee1c2b2e4f..54d074e621f6 100644
>> --- a/OvmfPkg/Sec/Ia32/SecEntry.nasm
>> +++ b/OvmfPkg/Sec/Ia32/SecEntry.nasm
>> @@ -40,8 +40,8 @@ ASM_PFX(_ModuleEntryPoint):
>>      ;
>>      %define SEC_TOP_OF_STACK (FixedPcdGet32 (PcdOvmfSecPeiTempRamBase) + \
>>                            FixedPcdGet32 (PcdOvmfSecPeiTempRamSize))
>> -    mov     eax, SEC_TOP_OF_STACK
>> -    mov     esp, eax
>> +    mov     ebx, SEC_TOP_OF_STACK
>> +    mov     esp, ebx
> 
> Can't you initialize the temp RAM before this code?

Should be possible, yes.

Thanks
Laszlo

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

[Laszlo Ersek]

On 11/13/17 19:25, Jordan Justen wrote:
> On 2017-11-10 07:49:06, Laszlo Ersek wrote:
>> 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
> 
> Can you document all the segment registers, and also document them in
> UefiCpuPkg/ResetVector/Vtf0/Main.asm?

Do you mean the above format (i.e., @param[in]...), just repeated for
the other segment registers too?

Regarding "UefiCpuPkg/ResetVector/Vtf0/Main.asm", what format do you
suggest? The @param[in]... format wouldn't be right, because the segment
registers are set up in TransitionFromReal16To32BitFlat. Should I write
a free-form comment / list above

    OneTimeCall TransitionFromReal16To32BitFlat

?

> 
>>  ;
>>  ; @return     None  This routine does not return
>>  ;
>> @@ -44,6 +45,18 @@ ASM_PFX(_ModuleEntryPoint):
>>      mov     esp, ebx
>>      nop
>>  
>> +    ;
>> +    ; Fill the temporary RAM with the initial stack value.
>> +    ; The loop below will seed the heap as well, but that's harmless.
>> +    ;
>> +    mov     eax, FixedPcdGet32 (PcdInitValueInTempStack)  ; dword to store
>> +    mov     edi, FixedPcdGet32 (PcdOvmfSecPeiTempRamBase) ; base address,
>> +                                                          ;   relative to ES
>> +    mov     ecx, FixedPcdGet32 (PcdOvmfSecPeiTempRamSize) ; byte count
>> +    shr     ecx, 2                                        ; dword count
> 
> I'm not sure, but I think NASM might let you do something like:
> 
>     mov     ecx, FixedPcdGet32 (PcdOvmfSecPeiTempRamSize) / 4
OK, I can try that. I was worried about NASM arithmetic in general, but
I see the "info" page describes "Multiplication and Division", so the
semantics should be well-defined.

> 
>> +    cld                                                   ; store from base up
>> +    rep stosd
> 
> I think if you move this above the code in patch 1, then patch 1 is
> not needed.

I think I agree (to be seen in practice :) )

> I also think it would be reasonable to merge 2 & 3, but
> separate is fine too.

If I remove the "shr" from both, then I might feel tempted to merge
them; I'm not sure yet. For testing at least I prefer to keep them separate.

Thanks
Laszlo

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

[Jordan Justen]

On 2017-11-13 10:36:45, Laszlo Ersek wrote:
> On 11/13/17 19:25, Jordan Justen wrote:
> > On 2017-11-10 07:49:06, Laszlo Ersek wrote:
> >> 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
> > 
> > Can you document all the segment registers, and also document them in
> > UefiCpuPkg/ResetVector/Vtf0/Main.asm?
> 
> Do you mean the above format (i.e., @param[in]...), just repeated for
> the other segment registers too?
> 
> Regarding "UefiCpuPkg/ResetVector/Vtf0/Main.asm", what format do you
> suggest? The @param[in]... format wouldn't be right, because the segment
> registers are set up in TransitionFromReal16To32BitFlat. Should I write
> a free-form comment / list above
> 
>     OneTimeCall TransitionFromReal16To32BitFlat

How does something like this sound?

; @param[out] DS    Selector allowing flat access to all addresses

It seems to cover 32/64 bit and get the point across.

-Jordan

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

[Laszlo Ersek]

On 11/13/17 20:02, Jordan Justen wrote:
> On 2017-11-13 10:36:45, Laszlo Ersek wrote:
>> On 11/13/17 19:25, Jordan Justen wrote:
>>> On 2017-11-10 07:49:06, Laszlo Ersek wrote:
>>>> 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
>>>
>>> Can you document all the segment registers, and also document them in
>>> UefiCpuPkg/ResetVector/Vtf0/Main.asm?
>>
>> Do you mean the above format (i.e., @param[in]...), just repeated for
>> the other segment registers too?
>>
>> Regarding "UefiCpuPkg/ResetVector/Vtf0/Main.asm", what format do you
>> suggest? The @param[in]... format wouldn't be right, because the segment
>> registers are set up in TransitionFromReal16To32BitFlat. Should I write
>> a free-form comment / list above
>>
>>     OneTimeCall TransitionFromReal16To32BitFlat
> 
> How does something like this sound?
> 
> ; @param[out] DS    Selector allowing flat access to all addresses
> 
> It seems to cover 32/64 bit and get the point across.

Sounds good, thanks!
Laszlo