[PATCH v2 0/4] UefiCpuPkg, OvmfPkg: add reset vector docs, tweak temp stack / RAM

[PATCH v2 0/4] UefiCpuPkg, OvmfPkg: add reset vector docs, tweak temp stack / RAM

[Laszlo Ersek]

Repo:   https://github.com/lersek/edk2.git
Branch: temp_ram_tweaks_v2

This is a series for

  https://bugzilla.tianocore.org/show_bug.cgi?id=747

specifically an update on the v1 series at

  https://lists.01.org/pipermail/edk2-devel/2017-November/017239.html

Each patch has its notes section on the changes relative to v1.

I re-ran the unit tests described in some of the patches; in addition to
the functional tests and my usual regression tests.

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>

Thanks
Laszlo

Laszlo Ersek (4):
  UefiCpuPkg/ResetVector/Vtf0: document segment register setup
  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/Ia32/SecEntry.nasm                      | 18 +++++++++++++++++
 OvmfPkg/Sec/SecMain.inf                             |  1 +
 OvmfPkg/Sec/X64/SecEntry.nasm                       | 21 ++++++++++++++++++++
 UefiCpuPkg/ResetVector/Vtf0/Ia16/Real16ToFlat32.asm |  6 ++++++
 UefiCpuPkg/ResetVector/Vtf0/Main.asm                |  5 +++++
 8 files changed, 54 insertions(+), 3 deletions(-)

-- 
2.14.1.3.gb7cf6e02401b

[PATCH v2 1/4] UefiCpuPkg/ResetVector/Vtf0: document segment register setup

[Laszlo Ersek]

"Main.asm" calls TransitionFromReal16To32BitFlat (and does some other
things) before it jumps to the platform's SEC entry point.

TransitionFromReal16To32BitFlat enters big real mode, and sets the DS, ES,
FS, GS, and SS registers to offset ("selector") LINEAR_SEL in the GDT
(defined in "UefiCpuPkg/ResetVector/Vtf0/Ia16/Real16ToFlat32.asm"). The
GDT entry ("segment descriptor") at LINEAR_SEL defines a segment covering
the full 32-bit address space, meant for "read/write data".

Document this fact for all the affected segment registers, as output
parameters for TransitionFromReal16To32BitFlat, saying "Selector allowing
flat access to all addresses".

For 64-bit SEC, "Main.asm" calls Transition32FlatTo64Flat in addition,
between calling TransitionFromReal16To32BitFlat and jumping to the SEC
entry point. Transition32FlatTo64Flat enters long mode. In long mode,
segmentation is largely ignored:

- all segments are considered flat (covering the whole 64-bit address
  space),

- with the (possible) exception of FS and GS, whose bases can still be
  changed, albeit with new methods, not through the GDT. (Through the
  IA32_FS_BASE and IA32_GS_BASE Model Specific Registers, and/or the
  WRFSBASE, WRGSBASE and SWAPGS instructions.)

Thus, document the segment registers with the same "Selector allowing flat
access to all addresses" language on the "Main.asm" level too, since that
is valid for both 32-bit and 64-bit modes.

(Technically, "Main.asm" does not return, but RBP/EBP, passed similarly to
the SEC entry point, is already documented as an output parameter.)

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Suggested-by: Jordan Justen <jordan.l.justen@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---

Notes:
    v2:
    - new patch [Jordan]

 UefiCpuPkg/ResetVector/Vtf0/Ia16/Real16ToFlat32.asm | 6 ++++++
 UefiCpuPkg/ResetVector/Vtf0/Main.asm                | 5 +++++
 2 files changed, 11 insertions(+)

diff --git a/UefiCpuPkg/ResetVector/Vtf0/Ia16/Real16ToFlat32.asm b/UefiCpuPkg/ResetVector/Vtf0/Ia16/Real16ToFlat32.asm
index 146df600a63b..bc68c8dd749a 100644
--- a/UefiCpuPkg/ResetVector/Vtf0/Ia16/Real16ToFlat32.asm
+++ b/UefiCpuPkg/ResetVector/Vtf0/Ia16/Real16ToFlat32.asm
@@ -21,6 +21,12 @@ BITS    16
 ;
 ; Modified:  EAX, EBX
 ;
+; @param[out]     DS       Selector allowing flat access to all addresses
+; @param[out]     ES       Selector allowing flat access to all addresses
+; @param[out]     FS       Selector allowing flat access to all addresses
+; @param[out]     GS       Selector allowing flat access to all addresses
+; @param[out]     SS       Selector allowing flat access to all addresses
+;
 TransitionFromReal16To32BitFlat:
 
     debugShowPostCode POSTCODE_16BIT_MODE
diff --git a/UefiCpuPkg/ResetVector/Vtf0/Main.asm b/UefiCpuPkg/ResetVector/Vtf0/Main.asm
index ebfb9015d49c..57f080688b6f 100644
--- a/UefiCpuPkg/ResetVector/Vtf0/Main.asm
+++ b/UefiCpuPkg/ResetVector/Vtf0/Main.asm
@@ -24,6 +24,11 @@ BITS    16
 ; @param[in,out]  DI       'BP': boot-strap processor, or
 ;                          'AP': application processor
 ; @param[out]     RBP/EBP  Address of Boot Firmware Volume (BFV)
+; @param[out]     DS       Selector allowing flat access to all addresses
+; @param[out]     ES       Selector allowing flat access to all addresses
+; @param[out]     FS       Selector allowing flat access to all addresses
+; @param[out]     GS       Selector allowing flat access to all addresses
+; @param[out]     SS       Selector allowing flat access to all addresses
 ;
 ; @return         None  This routine jumps to SEC and does not return
 ;
-- 
2.14.1.3.gb7cf6e02401b

[PATCH v2 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>
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>
---

Notes:
    v2:
    - update comment on ES [Jordan]
    - document the other segment registers too [Jordan]
    - move seeding to top of routine [Jordan]
    - hence, drop earlier EAX -> EBX renaming [Jordan]
    - replace runtime right shift with compile-time division [Jordan]
    - drop Ard's Reviewed-by

 OvmfPkg/Sec/SecMain.inf        |  1 +
 OvmfPkg/Sec/Ia32/SecEntry.nasm | 18 ++++++++++++++++++
 2 files changed, 19 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 7fee1c2b2e4f..03501969ebce 100644
--- a/OvmfPkg/Sec/Ia32/SecEntry.nasm
+++ b/OvmfPkg/Sec/Ia32/SecEntry.nasm
@@ -29,12 +29,30 @@ 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]  DS    Selector allowing flat access to all addresses
+; @param[in]  ES    Selector allowing flat access to all addresses
+; @param[in]  FS    Selector allowing flat access to all addresses
+; @param[in]  GS    Selector allowing flat access to all addresses
+; @param[in]  SS    Selector allowing flat access to all addresses
 ;
 ; @return     None  This routine does not return
 ;
 global ASM_PFX(_ModuleEntryPoint)
 ASM_PFX(_ModuleEntryPoint):
 
+    ;
+    ; 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) / 4 ; dword count
+    cld                                                       ; store from base
+                                                              ;   up
+    rep stosd
+
     ;
     ; Load temporary RAM stack based on PCDs
     ;
-- 
2.14.1.3.gb7cf6e02401b

[PATCH v2 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>
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>
---

Notes:
    v2:
    - update comment on ES [Jordan]
    - document the other segment registers too [Jordan]
    - move seeding to top of routine, for consistency with the IA32 version
    - replace runtime right shift with compile-time division [Jordan]
    - compose qword-to-write at compile-time, not at runtime
    - drop Ard's Reviewed-by

 OvmfPkg/Sec/X64/SecEntry.nasm | 21 ++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/OvmfPkg/Sec/X64/SecEntry.nasm b/OvmfPkg/Sec/X64/SecEntry.nasm
index f40427aa8e04..7c55032ac962 100644
--- a/OvmfPkg/Sec/X64/SecEntry.nasm
+++ b/OvmfPkg/Sec/X64/SecEntry.nasm
@@ -30,12 +30,33 @@ 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]  DS    Selector allowing flat access to all addresses
+; @param[in]  ES    Selector allowing flat access to all addresses
+; @param[in]  FS    Selector allowing flat access to all addresses
+; @param[in]  GS    Selector allowing flat access to all addresses
+; @param[in]  SS    Selector allowing flat access to all addresses
 ;
 ; @return     None  This routine does not return
 ;
 global ASM_PFX(_ModuleEntryPoint)
 ASM_PFX(_ModuleEntryPoint):
 
+    ;
+    ; 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              \
+                    ) << 32) |                           \
+                 FixedPcdGet32 (PcdInitValueInTempStack)      ; qword to store
+    mov     rdi, FixedPcdGet32 (PcdOvmfSecPeiTempRamBase)     ; base address,
+                                                              ;   relative to
+                                                              ;   ES
+    mov     rcx, FixedPcdGet32 (PcdOvmfSecPeiTempRamSize) / 8 ; qword count
+    cld                                                       ; store from base
+                                                              ;   up
+    rep stosq
+
     ;
     ; Load temporary RAM stack based on PCDs
     ;
-- 
2.14.1.3.gb7cf6e02401b

[PATCH v2 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>
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>
Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>
---

Notes:
    v2:
    - pick up review tags from Ard and Jordan

 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 v2 0/4] UefiCpuPkg, OvmfPkg: add reset vector docs, tweak temp stack / RAM

[Jordan Justen]

Series Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>

On 2017-11-15 13:56:59, Laszlo Ersek wrote:
> Repo:   https://github.com/lersek/edk2.git
> Branch: temp_ram_tweaks_v2
> 
> This is a series for
> 
>   https://bugzilla.tianocore.org/show_bug.cgi?id=747
> 
> specifically an update on the v1 series at
> 
>   https://lists.01.org/pipermail/edk2-devel/2017-November/017239.html
> 
> Each patch has its notes section on the changes relative to v1.
> 
> I re-ran the unit tests described in some of the patches; in addition to
> the functional tests and my usual regression tests.
> 
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> 
> Thanks
> Laszlo
> 
> Laszlo Ersek (4):
>   UefiCpuPkg/ResetVector/Vtf0: document segment register setup
>   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/Ia32/SecEntry.nasm                      | 18 +++++++++++++++++
>  OvmfPkg/Sec/SecMain.inf                             |  1 +
>  OvmfPkg/Sec/X64/SecEntry.nasm                       | 21 ++++++++++++++++++++
>  UefiCpuPkg/ResetVector/Vtf0/Ia16/Real16ToFlat32.asm |  6 ++++++
>  UefiCpuPkg/ResetVector/Vtf0/Main.asm                |  5 +++++
>  8 files changed, 54 insertions(+), 3 deletions(-)
> 
> -- 
> 2.14.1.3.gb7cf6e02401b
> 

Re: [PATCH v2 1/4] UefiCpuPkg/ResetVector/Vtf0: document segment register setup

[Ard Biesheuvel]

On 15 November 2017 at 21:57, Laszlo Ersek <lersek@redhat.com> wrote:
> "Main.asm" calls TransitionFromReal16To32BitFlat (and does some other
> things) before it jumps to the platform's SEC entry point.
>
> TransitionFromReal16To32BitFlat enters big real mode, and sets the DS, ES,
> FS, GS, and SS registers to offset ("selector") LINEAR_SEL in the GDT
> (defined in "UefiCpuPkg/ResetVector/Vtf0/Ia16/Real16ToFlat32.asm"). The
> GDT entry ("segment descriptor") at LINEAR_SEL defines a segment covering
> the full 32-bit address space, meant for "read/write data".
>
> Document this fact for all the affected segment registers, as output
> parameters for TransitionFromReal16To32BitFlat, saying "Selector allowing
> flat access to all addresses".
>
> For 64-bit SEC, "Main.asm" calls Transition32FlatTo64Flat in addition,
> between calling TransitionFromReal16To32BitFlat and jumping to the SEC
> entry point. Transition32FlatTo64Flat enters long mode. In long mode,
> segmentation is largely ignored:
>
> - all segments are considered flat (covering the whole 64-bit address
>   space),
>
> - with the (possible) exception of FS and GS, whose bases can still be
>   changed, albeit with new methods, not through the GDT. (Through the
>   IA32_FS_BASE and IA32_GS_BASE Model Specific Registers, and/or the
>   WRFSBASE, WRGSBASE and SWAPGS instructions.)
>
> Thus, document the segment registers with the same "Selector allowing flat
> access to all addresses" language on the "Main.asm" level too, since that
> is valid for both 32-bit and 64-bit modes.
>
> (Technically, "Main.asm" does not return, but RBP/EBP, passed similarly to
> the SEC entry point, is already documented as an output parameter.)
>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Suggested-by: Jordan Justen <jordan.l.justen@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>

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

> ---
>
> Notes:
>     v2:
>     - new patch [Jordan]
>
>  UefiCpuPkg/ResetVector/Vtf0/Ia16/Real16ToFlat32.asm | 6 ++++++
>  UefiCpuPkg/ResetVector/Vtf0/Main.asm                | 5 +++++
>  2 files changed, 11 insertions(+)
>
> diff --git a/UefiCpuPkg/ResetVector/Vtf0/Ia16/Real16ToFlat32.asm b/UefiCpuPkg/ResetVector/Vtf0/Ia16/Real16ToFlat32.asm
> index 146df600a63b..bc68c8dd749a 100644
> --- a/UefiCpuPkg/ResetVector/Vtf0/Ia16/Real16ToFlat32.asm
> +++ b/UefiCpuPkg/ResetVector/Vtf0/Ia16/Real16ToFlat32.asm
> @@ -21,6 +21,12 @@ BITS    16
>  ;
>  ; Modified:  EAX, EBX
>  ;
> +; @param[out]     DS       Selector allowing flat access to all addresses
> +; @param[out]     ES       Selector allowing flat access to all addresses
> +; @param[out]     FS       Selector allowing flat access to all addresses
> +; @param[out]     GS       Selector allowing flat access to all addresses
> +; @param[out]     SS       Selector allowing flat access to all addresses
> +;
>  TransitionFromReal16To32BitFlat:
>
>      debugShowPostCode POSTCODE_16BIT_MODE
> diff --git a/UefiCpuPkg/ResetVector/Vtf0/Main.asm b/UefiCpuPkg/ResetVector/Vtf0/Main.asm
> index ebfb9015d49c..57f080688b6f 100644
> --- a/UefiCpuPkg/ResetVector/Vtf0/Main.asm
> +++ b/UefiCpuPkg/ResetVector/Vtf0/Main.asm
> @@ -24,6 +24,11 @@ BITS    16
>  ; @param[in,out]  DI       'BP': boot-strap processor, or
>  ;                          'AP': application processor
>  ; @param[out]     RBP/EBP  Address of Boot Firmware Volume (BFV)
> +; @param[out]     DS       Selector allowing flat access to all addresses
> +; @param[out]     ES       Selector allowing flat access to all addresses
> +; @param[out]     FS       Selector allowing flat access to all addresses
> +; @param[out]     GS       Selector allowing flat access to all addresses
> +; @param[out]     SS       Selector allowing flat access to all addresses
>  ;
>  ; @return         None  This routine jumps to SEC and does not return
>  ;
> --
> 2.14.1.3.gb7cf6e02401b
>
>

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

[Ard Biesheuvel]

On 15 November 2017 at 21:57, 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>
> 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>

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

> ---
>
> Notes:
>     v2:
>     - update comment on ES [Jordan]
>     - document the other segment registers too [Jordan]
>     - move seeding to top of routine [Jordan]
>     - hence, drop earlier EAX -> EBX renaming [Jordan]
>     - replace runtime right shift with compile-time division [Jordan]
>     - drop Ard's Reviewed-by
>
>  OvmfPkg/Sec/SecMain.inf        |  1 +
>  OvmfPkg/Sec/Ia32/SecEntry.nasm | 18 ++++++++++++++++++
>  2 files changed, 19 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 7fee1c2b2e4f..03501969ebce 100644
> --- a/OvmfPkg/Sec/Ia32/SecEntry.nasm
> +++ b/OvmfPkg/Sec/Ia32/SecEntry.nasm
> @@ -29,12 +29,30 @@ 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]  DS    Selector allowing flat access to all addresses
> +; @param[in]  ES    Selector allowing flat access to all addresses
> +; @param[in]  FS    Selector allowing flat access to all addresses
> +; @param[in]  GS    Selector allowing flat access to all addresses
> +; @param[in]  SS    Selector allowing flat access to all addresses
>  ;
>  ; @return     None  This routine does not return
>  ;
>  global ASM_PFX(_ModuleEntryPoint)
>  ASM_PFX(_ModuleEntryPoint):
>
> +    ;
> +    ; 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) / 4 ; dword count
> +    cld                                                       ; store from base
> +                                                              ;   up
> +    rep stosd
> +
>      ;
>      ; Load temporary RAM stack based on PCDs
>      ;
> --
> 2.14.1.3.gb7cf6e02401b
>
>

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

[Ard Biesheuvel]

On 15 November 2017 at 21:57, 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:       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>
> 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>

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

> ---
>
> Notes:
>     v2:
>     - update comment on ES [Jordan]
>     - document the other segment registers too [Jordan]
>     - move seeding to top of routine, for consistency with the IA32 version
>     - replace runtime right shift with compile-time division [Jordan]
>     - compose qword-to-write at compile-time, not at runtime
>     - drop Ard's Reviewed-by
>
>  OvmfPkg/Sec/X64/SecEntry.nasm | 21 ++++++++++++++++++++
>  1 file changed, 21 insertions(+)
>
> diff --git a/OvmfPkg/Sec/X64/SecEntry.nasm b/OvmfPkg/Sec/X64/SecEntry.nasm
> index f40427aa8e04..7c55032ac962 100644
> --- a/OvmfPkg/Sec/X64/SecEntry.nasm
> +++ b/OvmfPkg/Sec/X64/SecEntry.nasm
> @@ -30,12 +30,33 @@ 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]  DS    Selector allowing flat access to all addresses
> +; @param[in]  ES    Selector allowing flat access to all addresses
> +; @param[in]  FS    Selector allowing flat access to all addresses
> +; @param[in]  GS    Selector allowing flat access to all addresses
> +; @param[in]  SS    Selector allowing flat access to all addresses
>  ;
>  ; @return     None  This routine does not return
>  ;
>  global ASM_PFX(_ModuleEntryPoint)
>  ASM_PFX(_ModuleEntryPoint):
>
> +    ;
> +    ; 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              \
> +                    ) << 32) |                           \
> +                 FixedPcdGet32 (PcdInitValueInTempStack)      ; qword to store
> +    mov     rdi, FixedPcdGet32 (PcdOvmfSecPeiTempRamBase)     ; base address,
> +                                                              ;   relative to
> +                                                              ;   ES
> +    mov     rcx, FixedPcdGet32 (PcdOvmfSecPeiTempRamSize) / 8 ; qword count
> +    cld                                                       ; store from base
> +                                                              ;   up
> +    rep stosq
> +
>      ;
>      ; Load temporary RAM stack based on PCDs
>      ;
> --
> 2.14.1.3.gb7cf6e02401b
>
>

Re: [PATCH v2 0/4] UefiCpuPkg, OvmfPkg: add reset vector docs, tweak temp stack / RAM

[Laszlo Ersek]

On 11/15/17 22:56, Laszlo Ersek wrote:
> Repo:   https://github.com/lersek/edk2.git
> Branch: temp_ram_tweaks_v2
> 
> This is a series for
> 
>   https://bugzilla.tianocore.org/show_bug.cgi?id=747
> 
> specifically an update on the v1 series at
> 
>   https://lists.01.org/pipermail/edk2-devel/2017-November/017239.html
> 
> Each patch has its notes section on the changes relative to v1.
> 
> I re-ran the unit tests described in some of the patches; in addition to
> the functional tests and my usual regression tests.
> 
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> 
> Thanks
> Laszlo
> 
> Laszlo Ersek (4):
>   UefiCpuPkg/ResetVector/Vtf0: document segment register setup
>   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/Ia32/SecEntry.nasm                      | 18 +++++++++++++++++
>  OvmfPkg/Sec/SecMain.inf                             |  1 +
>  OvmfPkg/Sec/X64/SecEntry.nasm                       | 21 ++++++++++++++++++++
>  UefiCpuPkg/ResetVector/Vtf0/Ia16/Real16ToFlat32.asm |  6 ++++++
>  UefiCpuPkg/ResetVector/Vtf0/Main.asm                |  5 +++++
>  8 files changed, 54 insertions(+), 3 deletions(-)
> 

Pushed as commit range 1195b8578764..d41fd8e839a3.

(I didn't get a maintainer review from Eric for patch #1, but it adds
only comments (no regressions possible) and Ard and Jordan reviewed that
too. My queue is overflowing. :/)

Thanks!
Laszlo

Re: [PATCH v2 0/4] UefiCpuPkg, OvmfPkg: add reset vector docs, tweak temp stack / RAM

[Dong, Eric]

Hi Laszlo,

Reviewed-by: Eric Dong <eric.dong@intel.com>

I'm ok with this change, Sorry for later response.

Thanks,
Eric
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Saturday, November 18, 2017 1:15 AM
> To: edk2-devel-01 <edk2-devel@lists.01.org>
> Cc: Justen, Jordan L <jordan.l.justen@intel.com>; Dong, Eric
> <eric.dong@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Subject: Re: [edk2] [PATCH v2 0/4] UefiCpuPkg, OvmfPkg: add reset vector
> docs, tweak temp stack / RAM
> 
> On 11/15/17 22:56, Laszlo Ersek wrote:
> > Repo:   https://github.com/lersek/edk2.git
> > Branch: temp_ram_tweaks_v2
> >
> > This is a series for
> >
> >   https://bugzilla.tianocore.org/show_bug.cgi?id=747
> >
> > specifically an update on the v1 series at
> >
> >   https://lists.01.org/pipermail/edk2-devel/2017-November/017239.html
> >
> > Each patch has its notes section on the changes relative to v1.
> >
> > I re-ran the unit tests described in some of the patches; in addition
> > to the functional tests and my usual regression tests.
> >
> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > Cc: Eric Dong <eric.dong@intel.com>
> > Cc: Jordan Justen <jordan.l.justen@intel.com>
> >
> > Thanks
> > Laszlo
> >
> > Laszlo Ersek (4):
> >   UefiCpuPkg/ResetVector/Vtf0: document segment register setup
> >   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/Ia32/SecEntry.nasm                      | 18 +++++++++++++++++
> >  OvmfPkg/Sec/SecMain.inf                             |  1 +
> >  OvmfPkg/Sec/X64/SecEntry.nasm                       | 21
> ++++++++++++++++++++
> >  UefiCpuPkg/ResetVector/Vtf0/Ia16/Real16ToFlat32.asm |  6 ++++++
> >  UefiCpuPkg/ResetVector/Vtf0/Main.asm                |  5 +++++
> >  8 files changed, 54 insertions(+), 3 deletions(-)
> >
> 
> Pushed as commit range 1195b8578764..d41fd8e839a3.
> 
> (I didn't get a maintainer review from Eric for patch #1, but it adds only
> comments (no regressions possible) and Ard and Jordan reviewed that too.
> My queue is overflowing. :/)
> 
> Thanks!
> Laszlo