* [PATCH 0/4] OvmfPkg: measure temp stack usage, restore temp RAM to 64KB
@ 2017-11-10 15:49 Laszlo Ersek
2017-11-10 15:49 ` [PATCH 1/4] OvmfPkg/Sec/Ia32: free up EAX for other uses while setting up the stack Laszlo Ersek
` (5 more replies)
0 siblings, 6 replies; 25+ messages in thread
From: Laszlo Ersek @ 2017-11-10 15:49 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Ard Biesheuvel, Jordan Justen, Ruiyu Ni
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
^ permalink raw reply [flat|nested] 25+ messages in thread* [PATCH 1/4] OvmfPkg/Sec/Ia32: free up EAX for other uses while setting up the stack 2017-11-10 15:49 [PATCH 0/4] OvmfPkg: measure temp stack usage, restore temp RAM to 64KB Laszlo Ersek @ 2017-11-10 15:49 ` Laszlo Ersek 2017-11-13 18:08 ` Jordan Justen 2017-11-10 15:49 ` [PATCH 2/4] OvmfPkg/Sec/Ia32: seed the temporary RAM with PcdInitValueInTempStack Laszlo Ersek ` (4 subsequent siblings) 5 siblings, 1 reply; 25+ messages in thread From: Laszlo Ersek @ 2017-11-10 15:49 UTC (permalink / raw) To: edk2-devel-01; +Cc: Ard Biesheuvel, Jordan Justen, Ruiyu Ni 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 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH 1/4] OvmfPkg/Sec/Ia32: free up EAX for other uses while setting up the stack 2017-11-10 15:49 ` [PATCH 1/4] OvmfPkg/Sec/Ia32: free up EAX for other uses while setting up the stack Laszlo Ersek @ 2017-11-13 18:08 ` Jordan Justen 2017-11-13 18:30 ` Laszlo Ersek 0 siblings, 1 reply; 25+ messages in thread From: Jordan Justen @ 2017-11-13 18:08 UTC (permalink / raw) To: Laszlo Ersek, edk2-devel-01; +Cc: Ard Biesheuvel, Ruiyu Ni 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/4] OvmfPkg/Sec/Ia32: free up EAX for other uses while setting up the stack 2017-11-13 18:08 ` Jordan Justen @ 2017-11-13 18:30 ` Laszlo Ersek 0 siblings, 0 replies; 25+ messages in thread From: Laszlo Ersek @ 2017-11-13 18:30 UTC (permalink / raw) To: Jordan Justen, edk2-devel-01; +Cc: Ard Biesheuvel, Ruiyu Ni 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 2/4] OvmfPkg/Sec/Ia32: seed the temporary RAM with PcdInitValueInTempStack 2017-11-10 15:49 [PATCH 0/4] OvmfPkg: measure temp stack usage, restore temp RAM to 64KB Laszlo Ersek 2017-11-10 15:49 ` [PATCH 1/4] OvmfPkg/Sec/Ia32: free up EAX for other uses while setting up the stack Laszlo Ersek @ 2017-11-10 15:49 ` Laszlo Ersek 2017-11-10 15:56 ` Ard Biesheuvel 2017-11-13 18:25 ` Jordan Justen 2017-11-10 15:49 ` [PATCH 3/4] OvmfPkg/Sec/X64: " Laszlo Ersek ` (3 subsequent siblings) 5 siblings, 2 replies; 25+ messages in thread From: Laszlo Ersek @ 2017-11-10 15:49 UTC (permalink / raw) To: edk2-devel-01; +Cc: Ard Biesheuvel, Jordan Justen, Ruiyu Ni 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 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH 2/4] OvmfPkg/Sec/Ia32: seed the temporary RAM with PcdInitValueInTempStack 2017-11-10 15:49 ` [PATCH 2/4] OvmfPkg/Sec/Ia32: seed the temporary RAM with PcdInitValueInTempStack Laszlo Ersek @ 2017-11-10 15:56 ` Ard Biesheuvel 2017-11-10 18:11 ` Laszlo Ersek 2017-11-13 18:25 ` Jordan Justen 1 sibling, 1 reply; 25+ messages in thread From: Ard Biesheuvel @ 2017-11-10 15:56 UTC (permalink / raw) To: Laszlo Ersek; +Cc: edk2-devel-01, Jordan Justen, Ruiyu Ni 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 > > ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 2/4] OvmfPkg/Sec/Ia32: seed the temporary RAM with PcdInitValueInTempStack 2017-11-10 15:56 ` Ard Biesheuvel @ 2017-11-10 18:11 ` Laszlo Ersek 2017-11-10 18:27 ` Laszlo Ersek 2017-11-11 9:10 ` Ard Biesheuvel 0 siblings, 2 replies; 25+ messages in thread From: Laszlo Ersek @ 2017-11-10 18:11 UTC (permalink / raw) To: Ard Biesheuvel; +Cc: edk2-devel-01, Jordan Justen, Ruiyu Ni 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 >> >> ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 2/4] OvmfPkg/Sec/Ia32: seed the temporary RAM with PcdInitValueInTempStack 2017-11-10 18:11 ` Laszlo Ersek @ 2017-11-10 18:27 ` Laszlo Ersek 2017-11-11 9:10 ` Ard Biesheuvel 1 sibling, 0 replies; 25+ messages in thread From: Laszlo Ersek @ 2017-11-10 18:27 UTC (permalink / raw) To: Ard Biesheuvel; +Cc: edk2-devel-01, Jordan Justen, Ruiyu Ni 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 >>> >>> > ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 2/4] OvmfPkg/Sec/Ia32: seed the temporary RAM with PcdInitValueInTempStack 2017-11-10 18:11 ` Laszlo Ersek 2017-11-10 18:27 ` Laszlo Ersek @ 2017-11-11 9:10 ` Ard Biesheuvel 1 sibling, 0 replies; 25+ messages in thread From: Ard Biesheuvel @ 2017-11-11 9:10 UTC (permalink / raw) To: Laszlo Ersek; +Cc: edk2-devel-01, Jordan Justen, Ruiyu Ni 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. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 2/4] OvmfPkg/Sec/Ia32: seed the temporary RAM with PcdInitValueInTempStack 2017-11-10 15:49 ` [PATCH 2/4] OvmfPkg/Sec/Ia32: seed the temporary RAM with PcdInitValueInTempStack Laszlo Ersek 2017-11-10 15:56 ` Ard Biesheuvel @ 2017-11-13 18:25 ` Jordan Justen 2017-11-13 18:36 ` Laszlo Ersek 1 sibling, 1 reply; 25+ messages in thread From: Jordan Justen @ 2017-11-13 18:25 UTC (permalink / raw) To: Laszlo Ersek, edk2-devel-01; +Cc: Ard Biesheuvel, Ruiyu Ni 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 2/4] OvmfPkg/Sec/Ia32: seed the temporary RAM with PcdInitValueInTempStack 2017-11-13 18:25 ` Jordan Justen @ 2017-11-13 18:36 ` Laszlo Ersek 2017-11-13 19:02 ` Jordan Justen 0 siblings, 1 reply; 25+ messages in thread From: Laszlo Ersek @ 2017-11-13 18:36 UTC (permalink / raw) To: Jordan Justen, edk2-devel-01; +Cc: Ard Biesheuvel, Ruiyu Ni 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 2/4] OvmfPkg/Sec/Ia32: seed the temporary RAM with PcdInitValueInTempStack 2017-11-13 18:36 ` Laszlo Ersek @ 2017-11-13 19:02 ` Jordan Justen 2017-11-13 20:58 ` Laszlo Ersek 0 siblings, 1 reply; 25+ messages in thread From: Jordan Justen @ 2017-11-13 19:02 UTC (permalink / raw) To: Laszlo Ersek, edk2-devel-01; +Cc: Ard Biesheuvel, Ruiyu Ni 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 2/4] OvmfPkg/Sec/Ia32: seed the temporary RAM with PcdInitValueInTempStack 2017-11-13 19:02 ` Jordan Justen @ 2017-11-13 20:58 ` Laszlo Ersek 0 siblings, 0 replies; 25+ messages in thread From: Laszlo Ersek @ 2017-11-13 20:58 UTC (permalink / raw) To: Jordan Justen, edk2-devel-01; +Cc: Ard Biesheuvel, Ruiyu Ni 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 3/4] OvmfPkg/Sec/X64: seed the temporary RAM with PcdInitValueInTempStack 2017-11-10 15:49 [PATCH 0/4] OvmfPkg: measure temp stack usage, restore temp RAM to 64KB Laszlo Ersek 2017-11-10 15:49 ` [PATCH 1/4] OvmfPkg/Sec/Ia32: free up EAX for other uses while setting up the stack Laszlo Ersek 2017-11-10 15:49 ` [PATCH 2/4] OvmfPkg/Sec/Ia32: seed the temporary RAM with PcdInitValueInTempStack Laszlo Ersek @ 2017-11-10 15:49 ` Laszlo Ersek 2017-11-10 15:49 ` [PATCH 4/4] OvmfPkg: restore temporary SEC/PEI RAM size to 64KB Laszlo Ersek ` (2 subsequent siblings) 5 siblings, 0 replies; 25+ messages in thread From: Laszlo Ersek @ 2017-11-10 15:49 UTC (permalink / raw) To: edk2-devel-01; +Cc: Ard Biesheuvel, Jordan Justen, Ruiyu Ni 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 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH 4/4] OvmfPkg: restore temporary SEC/PEI RAM size to 64KB 2017-11-10 15:49 [PATCH 0/4] OvmfPkg: measure temp stack usage, restore temp RAM to 64KB Laszlo Ersek ` (2 preceding siblings ...) 2017-11-10 15:49 ` [PATCH 3/4] OvmfPkg/Sec/X64: " Laszlo Ersek @ 2017-11-10 15:49 ` Laszlo Ersek 2017-11-11 9:14 ` [PATCH 0/4] OvmfPkg: measure temp stack usage, restore temp RAM " Ard Biesheuvel 2017-11-11 20:38 ` Jordan Justen 5 siblings, 0 replies; 25+ messages in thread From: Laszlo Ersek @ 2017-11-10 15:49 UTC (permalink / raw) To: edk2-devel-01; +Cc: Ard Biesheuvel, Jordan Justen, Ruiyu Ni (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 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH 0/4] OvmfPkg: measure temp stack usage, restore temp RAM to 64KB 2017-11-10 15:49 [PATCH 0/4] OvmfPkg: measure temp stack usage, restore temp RAM to 64KB Laszlo Ersek ` (3 preceding siblings ...) 2017-11-10 15:49 ` [PATCH 4/4] OvmfPkg: restore temporary SEC/PEI RAM size to 64KB Laszlo Ersek @ 2017-11-11 9:14 ` Ard Biesheuvel 2017-11-11 20:38 ` Jordan Justen 5 siblings, 0 replies; 25+ messages in thread From: Ard Biesheuvel @ 2017-11-11 9:14 UTC (permalink / raw) To: Laszlo Ersek; +Cc: edk2-devel-01, Jordan Justen, Ruiyu Ni 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 > ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 0/4] OvmfPkg: measure temp stack usage, restore temp RAM to 64KB 2017-11-10 15:49 [PATCH 0/4] OvmfPkg: measure temp stack usage, restore temp RAM to 64KB Laszlo Ersek ` (4 preceding siblings ...) 2017-11-11 9:14 ` [PATCH 0/4] OvmfPkg: measure temp stack usage, restore temp RAM " Ard Biesheuvel @ 2017-11-11 20:38 ` Jordan Justen 2017-11-11 22:04 ` Jordan Justen 5 siblings, 1 reply; 25+ messages in thread From: Jordan Justen @ 2017-11-11 20:38 UTC (permalink / raw) To: Laszlo Ersek, edk2-devel-01; +Cc: Ard Biesheuvel, Ruiyu Ni 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 > ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 0/4] OvmfPkg: measure temp stack usage, restore temp RAM to 64KB 2017-11-11 20:38 ` Jordan Justen @ 2017-11-11 22:04 ` Jordan Justen 2017-11-12 10:58 ` Ard Biesheuvel 0 siblings, 1 reply; 25+ messages in thread From: Jordan Justen @ 2017-11-11 22:04 UTC (permalink / raw) To: Laszlo Ersek, edk2-devel-01; +Cc: Ard Biesheuvel, Ruiyu Ni [-- 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 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH 0/4] OvmfPkg: measure temp stack usage, restore temp RAM to 64KB 2017-11-11 22:04 ` Jordan Justen @ 2017-11-12 10:58 ` Ard Biesheuvel 2017-11-13 9:08 ` Jordan Justen 0 siblings, 1 reply; 25+ messages in thread From: Ard Biesheuvel @ 2017-11-12 10:58 UTC (permalink / raw) To: Jordan Justen; +Cc: Laszlo Ersek, edk2-devel-01, Ruiyu Ni 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 >> > ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 0/4] OvmfPkg: measure temp stack usage, restore temp RAM to 64KB 2017-11-12 10:58 ` Ard Biesheuvel @ 2017-11-13 9:08 ` Jordan Justen 2017-11-13 10:09 ` Ard Biesheuvel 0 siblings, 1 reply; 25+ messages in thread From: Jordan Justen @ 2017-11-13 9:08 UTC (permalink / raw) To: Ard Biesheuvel; +Cc: Laszlo Ersek, edk2-devel-01, Ruiyu Ni 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 > >> > ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 0/4] OvmfPkg: measure temp stack usage, restore temp RAM to 64KB 2017-11-13 9:08 ` Jordan Justen @ 2017-11-13 10:09 ` Ard Biesheuvel 2017-11-13 12:34 ` Laszlo Ersek 0 siblings, 1 reply; 25+ messages in thread From: Ard Biesheuvel @ 2017-11-13 10:09 UTC (permalink / raw) To: Jordan Justen; +Cc: Laszlo Ersek, edk2-devel-01, Ruiyu Ni 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. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 0/4] OvmfPkg: measure temp stack usage, restore temp RAM to 64KB 2017-11-13 10:09 ` Ard Biesheuvel @ 2017-11-13 12:34 ` Laszlo Ersek 2017-11-13 13:09 ` Laszlo Ersek 2017-11-13 18:04 ` Jordan Justen 0 siblings, 2 replies; 25+ messages in thread From: Laszlo Ersek @ 2017-11-13 12:34 UTC (permalink / raw) To: Ard Biesheuvel, Jordan Justen; +Cc: edk2-devel-01, Ruiyu Ni 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 0/4] OvmfPkg: measure temp stack usage, restore temp RAM to 64KB 2017-11-13 12:34 ` Laszlo Ersek @ 2017-11-13 13:09 ` Laszlo Ersek 2017-11-13 18:05 ` Jordan Justen 2017-11-13 18:04 ` Jordan Justen 1 sibling, 1 reply; 25+ messages in thread From: Laszlo Ersek @ 2017-11-13 13:09 UTC (permalink / raw) To: Ard Biesheuvel, Jordan Justen; +Cc: Ruiyu Ni, edk2-devel-01 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 0/4] OvmfPkg: measure temp stack usage, restore temp RAM to 64KB 2017-11-13 13:09 ` Laszlo Ersek @ 2017-11-13 18:05 ` Jordan Justen 0 siblings, 0 replies; 25+ messages in thread From: Jordan Justen @ 2017-11-13 18:05 UTC (permalink / raw) To: Ard Biesheuvel, Laszlo Ersek; +Cc: Ruiyu Ni, edk2-devel-01 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 0/4] OvmfPkg: measure temp stack usage, restore temp RAM to 64KB 2017-11-13 12:34 ` Laszlo Ersek 2017-11-13 13:09 ` Laszlo Ersek @ 2017-11-13 18:04 ` Jordan Justen 1 sibling, 0 replies; 25+ messages in thread From: Jordan Justen @ 2017-11-13 18:04 UTC (permalink / raw) To: Ard Biesheuvel, Laszlo Ersek; +Cc: edk2-devel-01, Ruiyu Ni 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
end of thread, other threads:[~2017-11-13 20:54 UTC | newest] Thread overview: 25+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-11-10 15:49 [PATCH 0/4] OvmfPkg: measure temp stack usage, restore temp RAM to 64KB Laszlo Ersek 2017-11-10 15:49 ` [PATCH 1/4] OvmfPkg/Sec/Ia32: free up EAX for other uses while setting up the stack Laszlo Ersek 2017-11-13 18:08 ` Jordan Justen 2017-11-13 18:30 ` Laszlo Ersek 2017-11-10 15:49 ` [PATCH 2/4] OvmfPkg/Sec/Ia32: seed the temporary RAM with PcdInitValueInTempStack Laszlo Ersek 2017-11-10 15:56 ` Ard Biesheuvel 2017-11-10 18:11 ` Laszlo Ersek 2017-11-10 18:27 ` Laszlo Ersek 2017-11-11 9:10 ` Ard Biesheuvel 2017-11-13 18:25 ` Jordan Justen 2017-11-13 18:36 ` Laszlo Ersek 2017-11-13 19:02 ` Jordan Justen 2017-11-13 20:58 ` Laszlo Ersek 2017-11-10 15:49 ` [PATCH 3/4] OvmfPkg/Sec/X64: " Laszlo Ersek 2017-11-10 15:49 ` [PATCH 4/4] OvmfPkg: restore temporary SEC/PEI RAM size to 64KB Laszlo Ersek 2017-11-11 9:14 ` [PATCH 0/4] OvmfPkg: measure temp stack usage, restore temp RAM " Ard Biesheuvel 2017-11-11 20:38 ` Jordan Justen 2017-11-11 22:04 ` Jordan Justen 2017-11-12 10:58 ` Ard Biesheuvel 2017-11-13 9:08 ` Jordan Justen 2017-11-13 10:09 ` Ard Biesheuvel 2017-11-13 12:34 ` Laszlo Ersek 2017-11-13 13:09 ` Laszlo Ersek 2017-11-13 18:05 ` Jordan Justen 2017-11-13 18:04 ` Jordan Justen
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox