From 9f75aacc7ab5410345b4811901a78a375851aa8b Mon Sep 17 00:00:00 2001 From: Laszlo Ersek Date: Wed, 15 Nov 2017 19:00:17 +0100 Subject: [PATCH 1/9] UefiCpuPkg/ResetVector/Vtf0: document segment register setup "Main.asm" calls TransitionFromReal16To32BitFlat (and does some other things) before it jumps to the platform's SEC entry point. TransitionFromReal16To32BitFlat enters big real mode, and sets the DS, ES, FS, GS, and SS registers to offset ("selector") LINEAR_SEL in the GDT (defined in "UefiCpuPkg/ResetVector/Vtf0/Ia16/Real16ToFlat32.asm"). The GDT entry ("segment descriptor") at LINEAR_SEL defines a segment covering the full 32-bit address space, meant for "read/write data". Document this fact for all the affected segment registers, as output parameters for TransitionFromReal16To32BitFlat, saying "Selector allowing flat access to all addresses". For 64-bit SEC, "Main.asm" calls Transition32FlatTo64Flat in addition, between calling TransitionFromReal16To32BitFlat and jumping to the SEC entry point. Transition32FlatTo64Flat enters long mode. In long mode, segmentation is largely ignored: - all segments are considered flat (covering the whole 64-bit address space), - with the (possible) exception of FS and GS, whose bases can still be changed, albeit with new methods, not through the GDT. (Through the IA32_FS_BASE and IA32_GS_BASE Model Specific Registers, and/or the WRFSBASE, WRGSBASE and SWAPGS instructions.) Thus, document the segment registers with the same "Selector allowing flat access to all addresses" language on the "Main.asm" level too, since that is valid for both 32-bit and 64-bit modes. (Technically, "Main.asm" does not return, but RBP/EBP, passed similarly to the SEC entry point, is already documented as an output parameter.) Cc: Ard Biesheuvel Cc: Eric Dong Cc: Jordan Justen Suggested-by: Jordan Justen Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Laszlo Ersek Acked-by: Ard Biesheuvel Reviewed-by: Jordan Justen --- UefiCpuPkg/ResetVector/Vtf0/Ia16/Real16ToFlat32.asm | 6 ++++++ UefiCpuPkg/ResetVector/Vtf0/Main.asm | 5 +++++ 2 files changed, 11 insertions(+) diff --git a/UefiCpuPkg/ResetVector/Vtf0/Ia16/Real16ToFlat32.asm b/UefiCpuPkg/ResetVector/Vtf0/Ia16/Real16ToFlat32.asm index 146df600a63b..bc68c8dd749a 100644 --- a/UefiCpuPkg/ResetVector/Vtf0/Ia16/Real16ToFlat32.asm +++ b/UefiCpuPkg/ResetVector/Vtf0/Ia16/Real16ToFlat32.asm @@ -21,6 +21,12 @@ BITS 16 ; ; Modified: EAX, EBX ; +; @param[out] DS Selector allowing flat access to all addresses +; @param[out] ES Selector allowing flat access to all addresses +; @param[out] FS Selector allowing flat access to all addresses +; @param[out] GS Selector allowing flat access to all addresses +; @param[out] SS Selector allowing flat access to all addresses +; TransitionFromReal16To32BitFlat: debugShowPostCode POSTCODE_16BIT_MODE diff --git a/UefiCpuPkg/ResetVector/Vtf0/Main.asm b/UefiCpuPkg/ResetVector/Vtf0/Main.asm index ebfb9015d49c..57f080688b6f 100644 --- a/UefiCpuPkg/ResetVector/Vtf0/Main.asm +++ b/UefiCpuPkg/ResetVector/Vtf0/Main.asm @@ -24,6 +24,11 @@ BITS 16 ; @param[in,out] DI 'BP': boot-strap processor, or ; 'AP': application processor ; @param[out] RBP/EBP Address of Boot Firmware Volume (BFV) +; @param[out] DS Selector allowing flat access to all addresses +; @param[out] ES Selector allowing flat access to all addresses +; @param[out] FS Selector allowing flat access to all addresses +; @param[out] GS Selector allowing flat access to all addresses +; @param[out] SS Selector allowing flat access to all addresses ; ; @return None This routine jumps to SEC and does not return ; From 9d9350a579d3f3d52ea1012111fad300c4e19fa2 Mon Sep 17 00:00:00 2001 From: Laszlo Ersek Date: Wed, 8 Nov 2017 19:43:21 +0100 Subject: [PATCH 2/9] OvmfPkg/Sec/Ia32: seed the temporary RAM with PcdInitValueInTempStack 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 Cc: Jordan Justen Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=747 Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Laszlo Ersek Reviewed-by: Ard Biesheuvel Reviewed-by: Jordan Justen --- OvmfPkg/Sec/Ia32/SecEntry.nasm | 18 ++++++++++++++++++ OvmfPkg/Sec/SecMain.inf | 1 + 2 files changed, 19 insertions(+) diff --git a/OvmfPkg/Sec/Ia32/SecEntry.nasm b/OvmfPkg/Sec/Ia32/SecEntry.nasm index 7fee1c2b2e4f..03501969ebce 100644 --- a/OvmfPkg/Sec/Ia32/SecEntry.nasm +++ b/OvmfPkg/Sec/Ia32/SecEntry.nasm @@ -29,12 +29,30 @@ extern ASM_PFX(SecCoreStartupWithStack) ; @param[in] EAX Initial value of the EAX register (BIST: Built-in Self Test) ; @param[in] DI 'BP': boot-strap processor, or 'AP': application processor ; @param[in] EBP Pointer to the start of the Boot Firmware Volume +; @param[in] DS Selector allowing flat access to all addresses +; @param[in] ES Selector allowing flat access to all addresses +; @param[in] FS Selector allowing flat access to all addresses +; @param[in] GS Selector allowing flat access to all addresses +; @param[in] SS Selector allowing flat access to all addresses ; ; @return None This routine does not return ; global ASM_PFX(_ModuleEntryPoint) ASM_PFX(_ModuleEntryPoint): + ; + ; Fill the temporary RAM with the initial stack value. + ; The loop below will seed the heap as well, but that's harmless. + ; + mov eax, FixedPcdGet32 (PcdInitValueInTempStack) ; dword to store + mov edi, FixedPcdGet32 (PcdOvmfSecPeiTempRamBase) ; base address, + ; relative to + ; ES + mov ecx, FixedPcdGet32 (PcdOvmfSecPeiTempRamSize) / 4 ; dword count + cld ; store from base + ; up + rep stosd + ; ; Load temporary RAM stack based on PCDs ; 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 @@ gEfiMdePkgTokenSpaceGuid.PcdGuidedExtractHandlerTableAddress gUefiOvmfPkgTokenSpaceGuid.PcdGuidedExtractHandlerTableSize gUefiOvmfPkgTokenSpaceGuid.PcdOvmfDecompressionScratchEnd + gEfiMdeModulePkgTokenSpaceGuid.PcdInitValueInTempStack [FeaturePcd] gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire From 2278b8a82ea945e9e93b0695b78896ba8db414d9 Mon Sep 17 00:00:00 2001 From: Laszlo Ersek Date: Wed, 8 Nov 2017 20:32:30 +0100 Subject: [PATCH 3/9] OvmfPkg/Sec/X64: seed the temporary RAM with PcdInitValueInTempStack 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 Cc: Jordan Justen Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=747 Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Laszlo Ersek Reviewed-by: Ard Biesheuvel Reviewed-by: Jordan Justen --- OvmfPkg/Sec/X64/SecEntry.nasm | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/OvmfPkg/Sec/X64/SecEntry.nasm b/OvmfPkg/Sec/X64/SecEntry.nasm index f40427aa8e04..7c55032ac962 100644 --- a/OvmfPkg/Sec/X64/SecEntry.nasm +++ b/OvmfPkg/Sec/X64/SecEntry.nasm @@ -30,12 +30,33 @@ extern ASM_PFX(SecCoreStartupWithStack) ; @param[in] RAX Initial value of the EAX register (BIST: Built-in Self Test) ; @param[in] DI 'BP': boot-strap processor, or 'AP': application processor ; @param[in] RBP Pointer to the start of the Boot Firmware Volume +; @param[in] DS Selector allowing flat access to all addresses +; @param[in] ES Selector allowing flat access to all addresses +; @param[in] FS Selector allowing flat access to all addresses +; @param[in] GS Selector allowing flat access to all addresses +; @param[in] SS Selector allowing flat access to all addresses ; ; @return None This routine does not return ; global ASM_PFX(_ModuleEntryPoint) ASM_PFX(_ModuleEntryPoint): + ; + ; Fill the temporary RAM with the initial stack value. + ; The loop below will seed the heap as well, but that's harmless. + ; + mov rax, (FixedPcdGet32 ( \ + PcdInitValueInTempStack \ + ) << 32) | \ + FixedPcdGet32 (PcdInitValueInTempStack) ; qword to store + mov rdi, FixedPcdGet32 (PcdOvmfSecPeiTempRamBase) ; base address, + ; relative to + ; ES + mov rcx, FixedPcdGet32 (PcdOvmfSecPeiTempRamSize) / 8 ; qword count + cld ; store from base + ; up + rep stosq + ; ; Load temporary RAM stack based on PCDs ; From d41fd8e839a31e6374cf37c8516311db4e4a3dbc Mon Sep 17 00:00:00 2001 From: Laszlo Ersek Date: Tue, 24 Oct 2017 17:21:25 +0200 Subject: [PATCH 4/9] OvmfPkg: restore temporary SEC/PEI RAM size to 64KB (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 Cc: Jordan Justen 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 Reviewed-by: Ard Biesheuvel Reviewed-by: Jordan Justen --- 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 @@ gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageBase|gUefiOvmfPkgTokenSpaceGuid. 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 @@ gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageBase|gUefiOvmfPkgTokenSpaceGuid. 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 @@ gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageBase|gUefiOvmfPkgTokenSpaceGuid. 0x007000|0x001000 gEfiMdePkgTokenSpaceGuid.PcdGuidedExtractHandlerTableAddress|gUefiOvmfPkgTokenSpaceGuid.PcdGuidedExtractHandlerTableSize -0x010000|0x008000 +0x010000|0x010000 gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize 0x020000|0x0E0000 From 6dead8d5af5d1bb1624bd9c1a39383704d8c31f8 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Thu, 16 Nov 2017 21:30:58 +0100 Subject: [PATCH 5/9] OvmfPkg: make PlatformDebugLibIoPort a proper BASE library Remove Uefi.h, which includes UefiSpec.h, and change the return value to match the RETURN_STATUS type. Contributed-under: TianoCore Contribution Agreement 1.1 Cc: Laszlo Ersek Cc: Ard Biesheuvel Cc: Jordan Justen (Intel address) Signed-off-by: Paolo Bonzini Tested-by: Laszlo Ersek Reviewed-by: Laszlo Ersek --- OvmfPkg/Library/PlatformDebugLibIoPort/DebugLib.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLib.c b/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLib.c index 5435767c1c69..74f4d9c2d631 100644 --- a/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLib.c +++ b/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLib.c @@ -15,7 +15,6 @@ **/ #include -#include #include #include #include @@ -32,7 +31,7 @@ /** This constructor function does not have to do anything. - @retval EFI_SUCCESS The constructor always returns RETURN_SUCCESS. + @retval RETURN_SUCCESS The constructor always returns RETURN_SUCCESS. **/ RETURN_STATUS @@ -41,7 +40,7 @@ PlatformDebugLibIoPortConstructor ( VOID ) { - return EFI_SUCCESS; + return RETURN_SUCCESS; } /** From c9eb56e5fddc77a6a68c57c5685f64adbb5a05b9 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Thu, 16 Nov 2017 21:30:59 +0100 Subject: [PATCH 6/9] OvmfPkg: create a separate PlatformDebugLibIoPort instance for SEC The next patch will want to add a global variable to PlatformDebugLibIoPort, but this is not suitable for the SEC phase, because SEC runs from read-only flash. The solution is to have two library instances, one for SEC and another for all other firmware phases. This patch adds the "plumbing" for the SEC library instance, separating the INF files and moving the constructor to a separate C source file. Contributed-under: TianoCore Contribution Agreement 1.1 Cc: Laszlo Ersek Cc: Ard Biesheuvel Cc: Jordan Justen (Intel address) Signed-off-by: Paolo Bonzini Tested-by: Laszlo Ersek Reviewed-by: Laszlo Ersek --- .../Library/PlatformDebugLibIoPort/DebugLib.c | 15 ------ .../PlatformDebugLibIoPort/DebugLibDetect.c | 31 +++++++++++ .../DebugLibDetectRom.c | 31 +++++++++++ .../PlatformDebugLibIoPort.inf | 3 +- .../PlatformRomDebugLibIoPort.inf | 52 +++++++++++++++++++ OvmfPkg/OvmfPkgIa32.dsc | 2 +- OvmfPkg/OvmfPkgIa32X64.dsc | 2 +- OvmfPkg/OvmfPkgX64.dsc | 2 +- 8 files changed, 119 insertions(+), 19 deletions(-) create mode 100644 OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetect.c create mode 100644 OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetectRom.c create mode 100644 OvmfPkg/Library/PlatformDebugLibIoPort/PlatformRomDebugLibIoPort.inf diff --git a/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLib.c b/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLib.c index 74f4d9c2d631..5a1c86f2c3fd 100644 --- a/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLib.c +++ b/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLib.c @@ -28,21 +28,6 @@ // #define MAX_DEBUG_MESSAGE_LENGTH 0x100 -/** - This constructor function does not have to do anything. - - @retval RETURN_SUCCESS The constructor always returns RETURN_SUCCESS. - -**/ -RETURN_STATUS -EFIAPI -PlatformDebugLibIoPortConstructor ( - VOID - ) -{ - return RETURN_SUCCESS; -} - /** Prints a debug message to the debug output device if the specified error level is enabled. diff --git a/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetect.c b/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetect.c new file mode 100644 index 000000000000..bad054f28679 --- /dev/null +++ b/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetect.c @@ -0,0 +1,31 @@ +/** @file + Constructor code for QEMU debug port library. + Non-SEC instance. + + Copyright (c) 2017, Red Hat, Inc.
+ This program and the accompanying materials + are licensed and made available under the terms and conditions of the BSD License + which accompanies this distribution. The full text of the license may be found at + http://opensource.org/licenses/bsd-license.php. + + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, + WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. + +**/ + +#include + +/** + This constructor function does not have anything to do. + + @retval RETURN_SUCCESS The constructor always returns RETURN_SUCCESS. + +**/ +RETURN_STATUS +EFIAPI +PlatformDebugLibIoPortConstructor ( + VOID + ) +{ + return RETURN_SUCCESS; +} diff --git a/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetectRom.c b/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetectRom.c new file mode 100644 index 000000000000..83a118a0f78f --- /dev/null +++ b/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetectRom.c @@ -0,0 +1,31 @@ +/** @file + Constructor code for QEMU debug port library. + SEC instance. + + Copyright (c) 2017, Red Hat, Inc.
+ This program and the accompanying materials + are licensed and made available under the terms and conditions of the BSD License + which accompanies this distribution. The full text of the license may be found at + http://opensource.org/licenses/bsd-license.php. + + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, + WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. + +**/ + +#include + +/** + This constructor function does not have anything to do. + + @retval RETURN_SUCCESS The constructor always returns RETURN_SUCCESS. + +**/ +RETURN_STATUS +EFIAPI +PlatformRomDebugLibIoPortConstructor ( + VOID + ) +{ + return RETURN_SUCCESS; +} diff --git a/OvmfPkg/Library/PlatformDebugLibIoPort/PlatformDebugLibIoPort.inf b/OvmfPkg/Library/PlatformDebugLibIoPort/PlatformDebugLibIoPort.inf index 0e74fe94cbc0..de3c2f542bfe 100644 --- a/OvmfPkg/Library/PlatformDebugLibIoPort/PlatformDebugLibIoPort.inf +++ b/OvmfPkg/Library/PlatformDebugLibIoPort/PlatformDebugLibIoPort.inf @@ -21,7 +21,7 @@ FILE_GUID = DF934DA3-CD31-49FE-AF50-B3C87C79325F MODULE_TYPE = BASE VERSION_STRING = 1.0 - LIBRARY_CLASS = DebugLib + LIBRARY_CLASS = DebugLib|PEI_CORE PEIM DXE_CORE DXE_DRIVER DXE_RUNTIME_DRIVER SMM_CORE DXE_SMM_DRIVER UEFI_DRIVER UEFI_APPLICATION CONSTRUCTOR = PlatformDebugLibIoPortConstructor # @@ -30,6 +30,7 @@ [Sources] DebugLib.c + DebugLibDetect.c [Packages] MdePkg/MdePkg.dec diff --git a/OvmfPkg/Library/PlatformDebugLibIoPort/PlatformRomDebugLibIoPort.inf b/OvmfPkg/Library/PlatformDebugLibIoPort/PlatformRomDebugLibIoPort.inf new file mode 100644 index 000000000000..491c0318de56 --- /dev/null +++ b/OvmfPkg/Library/PlatformDebugLibIoPort/PlatformRomDebugLibIoPort.inf @@ -0,0 +1,52 @@ +## @file +# Instance of Debug Library for the QEMU debug console port. +# It uses Print Library to produce formatted output strings. +# +# Copyright (c) 2006 - 2015, Intel Corporation. All rights reserved.
+# Copyright (c) 2017, Red Hat, Inc.
+# +# This program and the accompanying materials +# are licensed and made available under the terms and conditions of the BSD License +# which accompanies this distribution. The full text of the license may be found at +# http://opensource.org/licenses/bsd-license.php. +# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, +# WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. +# +# +## + +[Defines] + INF_VERSION = 0x00010005 + BASE_NAME = PlatformRomDebugLibIoPort + FILE_GUID = CEB0D9D3-328F-4C24-8C02-28FA1986AE1B + MODULE_TYPE = BASE + VERSION_STRING = 1.0 + LIBRARY_CLASS = DebugLib|SEC + CONSTRUCTOR = PlatformRomDebugLibIoPortConstructor + +# +# VALID_ARCHITECTURES = IA32 X64 IPF EBC +# + +[Sources] + DebugLib.c + DebugLibDetectRom.c + +[Packages] + MdePkg/MdePkg.dec + OvmfPkg/OvmfPkg.dec + +[LibraryClasses] + BaseMemoryLib + IoLib + PcdLib + PrintLib + BaseLib + DebugPrintErrorLevelLib + +[Pcd] + gUefiOvmfPkgTokenSpaceGuid.PcdDebugIoPort ## CONSUMES + gEfiMdePkgTokenSpaceGuid.PcdDebugClearMemoryValue ## CONSUMES + gEfiMdePkgTokenSpaceGuid.PcdDebugPropertyMask ## CONSUMES + gEfiMdePkgTokenSpaceGuid.PcdFixedDebugPrintErrorLevel ## CONSUMES + diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc index c2f534fdbf3b..7ccb61147f27 100644 --- a/OvmfPkg/OvmfPkgIa32.dsc +++ b/OvmfPkg/OvmfPkgIa32.dsc @@ -207,7 +207,7 @@ !ifdef $(DEBUG_ON_SERIAL_PORT) DebugLib|MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf !else - DebugLib|OvmfPkg/Library/PlatformDebugLibIoPort/PlatformDebugLibIoPort.inf + DebugLib|OvmfPkg/Library/PlatformDebugLibIoPort/PlatformRomDebugLibIoPort.inf !endif ReportStatusCodeLib|MdeModulePkg/Library/PeiReportStatusCodeLib/PeiReportStatusCodeLib.inf ExtractGuidedSectionLib|MdePkg/Library/BaseExtractGuidedSectionLib/BaseExtractGuidedSectionLib.inf diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc index 9f300a2e6f32..237ec71b5ec0 100644 --- a/OvmfPkg/OvmfPkgIa32X64.dsc +++ b/OvmfPkg/OvmfPkgIa32X64.dsc @@ -212,7 +212,7 @@ !ifdef $(DEBUG_ON_SERIAL_PORT) DebugLib|MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf !else - DebugLib|OvmfPkg/Library/PlatformDebugLibIoPort/PlatformDebugLibIoPort.inf + DebugLib|OvmfPkg/Library/PlatformDebugLibIoPort/PlatformRomDebugLibIoPort.inf !endif ReportStatusCodeLib|MdeModulePkg/Library/PeiReportStatusCodeLib/PeiReportStatusCodeLib.inf ExtractGuidedSectionLib|MdePkg/Library/BaseExtractGuidedSectionLib/BaseExtractGuidedSectionLib.inf diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc index 1ffcf37f8b92..a5047fa38e61 100644 --- a/OvmfPkg/OvmfPkgX64.dsc +++ b/OvmfPkg/OvmfPkgX64.dsc @@ -212,7 +212,7 @@ !ifdef $(DEBUG_ON_SERIAL_PORT) DebugLib|MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf !else - DebugLib|OvmfPkg/Library/PlatformDebugLibIoPort/PlatformDebugLibIoPort.inf + DebugLib|OvmfPkg/Library/PlatformDebugLibIoPort/PlatformRomDebugLibIoPort.inf !endif ReportStatusCodeLib|MdeModulePkg/Library/PeiReportStatusCodeLib/PeiReportStatusCodeLib.inf ExtractGuidedSectionLib|MdePkg/Library/BaseExtractGuidedSectionLib/BaseExtractGuidedSectionLib.inf From c09d9571300a089c35f5df2773b70edc25050d0d Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Thu, 16 Nov 2017 21:31:00 +0100 Subject: [PATCH 7/9] OvmfPkg: save on I/O port accesses when the debug port is not in use When SEV is enabled, every debug message printed by OVMF to the QEMU debug port traps from the guest to QEMU character by character because "REP OUTSB" cannot be used by IoWriteFifo8. Furthermore, when OVMF is built with the DEBUG_VERBOSE bit (value 0x00400000) enabled in "gEfiMdePkgTokenSpaceGuid.PcdDebugPrintErrorLevel", then the OvmfPkg/IoMmuDxe driver, and the OvmfPkg/Library/BaseMemEncryptSevLib library instance that is built into it, produce a huge amount of log messages. Therefore, in SEV guests, the boot time impact is huge (about 45 seconds _additional_ time spent writing to the debug port). While these messages are very useful for analyzing guest behavior, most of the time the user won't be capturing the OVMF debug log. In fact libvirt does not provide a method for configuring log capture; users that wish to do this (or are instructed to do this) have to resort to . The debug console device provides a handy detection mechanism; when read, it returns 0xE9 (which is very much unlike the 0xFF that is returned by an unused port). Use it to skip the possibly expensive OUT instructions when the debug I/O port isn't plugged anywhere. For SEC, the debug port has to be read before each full message. However: - if the debug port is available, then reading one byte before writing a full message isn't tragic, especially because SEC doesn't print many messages - if the debug port is not available, then reading one byte instead of writing a full message is still a win. Contributed-under: TianoCore Contribution Agreement 1.0 Cc: Laszlo Ersek Cc: Ard Biesheuvel Cc: Jordan Justen (Intel address) Signed-off-by: Paolo Bonzini Tested-by: Laszlo Ersek Reviewed-by: Laszlo Ersek --- .../Library/PlatformDebugLibIoPort/DebugLib.c | 28 +++++++-- .../PlatformDebugLibIoPort/DebugLibDetect.c | 30 +++++++++- .../PlatformDebugLibIoPort/DebugLibDetect.h | 57 +++++++++++++++++++ .../DebugLibDetectRom.c | 21 ++++++- 4 files changed, 127 insertions(+), 9 deletions(-) create mode 100644 OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetect.h diff --git a/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLib.c b/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLib.c index 5a1c86f2c3fd..36cde54976d8 100644 --- a/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLib.c +++ b/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLib.c @@ -22,6 +22,7 @@ #include #include #include +#include "DebugLibDetect.h" // // Define the maximum debug and assert message length that this library supports @@ -61,9 +62,10 @@ DebugPrint ( ASSERT (Format != NULL); // - // Check driver debug mask value and global mask + // Check if the global mask disables this message or the device is inactive // - if ((ErrorLevel & GetDebugPrintErrorLevel ()) == 0) { + if ((ErrorLevel & GetDebugPrintErrorLevel ()) == 0 || + !PlatformDebugLibIoPortFound ()) { return; } @@ -120,9 +122,11 @@ DebugAssert ( FileName, (UINT64)LineNumber, Description); // - // Send the print string to the debug I/O port + // Send the print string to the debug I/O port, if present // - IoWriteFifo8 (PcdGet16 (PcdDebugIoPort), Length, Buffer); + if (PlatformDebugLibIoPortFound ()) { + IoWriteFifo8 (PcdGet16 (PcdDebugIoPort), Length, Buffer); + } // // Generate a Breakpoint, DeadLoop, or NOP based on PCD settings @@ -265,3 +269,19 @@ DebugPrintLevelEnabled ( { return (BOOLEAN) ((ErrorLevel & PcdGet32(PcdFixedDebugPrintErrorLevel)) != 0); } + +/** + Return the result of detecting the debug I/O port device. + + @retval TRUE if the debug I/O port device was detected. + @retval FALSE otherwise + +**/ +BOOLEAN +EFIAPI +PlatformDebugLibIoPortDetect ( + VOID + ) +{ + return IoRead8 (PcdGet16 (PcdDebugIoPort)) == BOCHS_DEBUG_PORT_MAGIC; +} diff --git a/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetect.c b/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetect.c index bad054f28679..81c44eece95f 100644 --- a/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetect.c +++ b/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetect.c @@ -1,6 +1,6 @@ /** @file - Constructor code for QEMU debug port library. - Non-SEC instance. + Detection code for QEMU debug port. + Non-SEC instance, caches the result of detection. Copyright (c) 2017, Red Hat, Inc.
This program and the accompanying materials @@ -14,9 +14,16 @@ **/ #include +#include "DebugLibDetect.h" + +// +// Set to TRUE if the debug I/O port is enabled +// +STATIC BOOLEAN mDebugIoPortFound = FALSE; /** - This constructor function does not have anything to do. + This constructor function checks if the debug I/O port device is present, + caching the result for later use. @retval RETURN_SUCCESS The constructor always returns RETURN_SUCCESS. @@ -27,5 +34,22 @@ PlatformDebugLibIoPortConstructor ( VOID ) { + mDebugIoPortFound = PlatformDebugLibIoPortDetect(); return RETURN_SUCCESS; } + +/** + Return the cached result of detecting the debug I/O port device. + + @retval TRUE if the debug I/O port device was detected. + @retval FALSE otherwise + +**/ +BOOLEAN +EFIAPI +PlatformDebugLibIoPortFound ( + VOID + ) +{ + return mDebugIoPortFound; +} diff --git a/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetect.h b/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetect.h new file mode 100644 index 000000000000..1f739b55d845 --- /dev/null +++ b/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetect.h @@ -0,0 +1,57 @@ +/** @file + Base Debug library instance for QEMU debug port. + It uses PrintLib to send debug messages to a fixed I/O port. + + Copyright (c) 2017, Red Hat, Inc.
+ This program and the accompanying materials + are licensed and made available under the terms and conditions of the BSD License + which accompanies this distribution. The full text of the license may be found at + http://opensource.org/licenses/bsd-license.php. + + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, + WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. + +**/ + +#ifndef __DEBUG_IO_PORT_DETECT_H__ +#define __DEBUG_IO_PORT_DETECT_H__ + +#include + +// +// The constant value that is read from the debug I/O port +// +#define BOCHS_DEBUG_PORT_MAGIC 0xE9 + + +/** + Helper function to return whether the virtual machine has a debug I/O port. + PlatformDebugLibIoPortFound can call this function directly or cache the + result. + + @retval TRUE if the debug I/O port device was detected. + @retval FALSE otherwise + +**/ +BOOLEAN +EFIAPI +PlatformDebugLibIoPortDetect ( + VOID + ); + +/** + Return whether the virtual machine has a debug I/O port. DebugLib.c + calls this function instead of PlatformDebugLibIoPortDetect, to allow + caching if possible. + + @retval TRUE if the debug I/O port device was detected. + @retval FALSE otherwise + +**/ +BOOLEAN +EFIAPI +PlatformDebugLibIoPortFound ( + VOID + ); + +#endif diff --git a/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetectRom.c b/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetectRom.c index 83a118a0f78f..b950919675d3 100644 --- a/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetectRom.c +++ b/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetectRom.c @@ -1,6 +1,6 @@ /** @file - Constructor code for QEMU debug port library. - SEC instance. + Detection code for QEMU debug port. + SEC instance, cannot cache the result of detection. Copyright (c) 2017, Red Hat, Inc.
This program and the accompanying materials @@ -14,6 +14,7 @@ **/ #include +#include "DebugLibDetect.h" /** This constructor function does not have anything to do. @@ -29,3 +30,19 @@ PlatformRomDebugLibIoPortConstructor ( { return RETURN_SUCCESS; } + +/** + Return the result of detecting the debug I/O port device. + + @retval TRUE if the debug I/O port device was detected. + @retval FALSE otherwise + +**/ +BOOLEAN +EFIAPI +PlatformDebugLibIoPortFound ( + VOID + ) +{ + return PlatformDebugLibIoPortDetect (); +} From d8030c2ae06b771c78d0438c869cd882e82d0c8f Mon Sep 17 00:00:00 2001 From: Michael D Kinney Date: Tue, 14 Nov 2017 15:56:30 -0800 Subject: [PATCH 8/9] MdeModulePkg/UsbMassStorageDxe: Enhance Request Sense Handling https://bugzilla.tianocore.org/show_bug.cgi?id=782 Update the Request Sense check for the Request Sense Key of USB_BOOT_SENSE_UNIT_ATTENTION. For this Sense Key, the Additional Sense Key to EFI_STATUS mappings are: USB_BOOT_ASC_MEDIA_CHANGE -> EFI_MEDIA_CHANGE USB_BOOT_ASC_NOT_READY -> EFI_NOT_READY USB_BOOT_ASC_NO_MEDIA -> EFI_NOT_READY All others -> EFI_DEVICE_ERROR A USB flash drive is returning Request Sense Key of USB_BOOT_SENSE_UNIT_ATTENTION and an Additional Sense Key of USB_BOOT_ASC_NO_MEDIA for a few seconds before returning an Additional Sense Key of USB_BOOT_ASC_MEDIA_CHANGE. The current logic treats this initial Request Sense info as an error and reties the command 5 times before failing completely. With this change the USB Flash Drive works correctly. Cc: Star Zeng Cc: Eric Dong Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Michael D Kinney Reviewed-by: Star Zeng --- MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c b/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c index 2eb30f0c5f50..a8b6a1c5f13c 100644 --- a/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c +++ b/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c @@ -120,6 +120,10 @@ UsbBootRequestSense ( Status = EFI_MEDIA_CHANGED; Media->ReadOnly = FALSE; Media->MediaId++; + } else if (SenseData.Asc == USB_BOOT_ASC_NOT_READY) { + Status = EFI_NOT_READY; + } else if (SenseData.Asc == USB_BOOT_ASC_NO_MEDIA) { + Status = EFI_NOT_READY; } break; From b2662641d56a7ebfe46f1c8ebd77611d3037cf65 Mon Sep 17 00:00:00 2001 From: Ard Biesheuvel Date: Fri, 17 Nov 2017 15:40:38 +0000 Subject: [PATCH 9/9] ArmPlatformPkg/ArmPlatformLibNull: remove bogus PCD dependencies Remove dependencies on gArmTokenSpaceGuid.PcdSystemMemoryBase and gArmTokenSpaceGuid.PcdSystemMemorySize, the presence of which in a [FixedPcd] section makes this module unusable for ArmVirtQemu. Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Ard Biesheuvel Reviewed-by: Leif Lindholm --- .../Library/ArmPlatformLibNull/ArmPlatformLibNull.inf | 3 --- 1 file changed, 3 deletions(-) diff --git a/ArmPlatformPkg/Library/ArmPlatformLibNull/ArmPlatformLibNull.inf b/ArmPlatformPkg/Library/ArmPlatformLibNull/ArmPlatformLibNull.inf index 97b2a4b6e495..e59aef611a3c 100644 --- a/ArmPlatformPkg/Library/ArmPlatformLibNull/ArmPlatformLibNull.inf +++ b/ArmPlatformPkg/Library/ArmPlatformLibNull/ArmPlatformLibNull.inf @@ -41,9 +41,6 @@ AArch64/ArmPlatformHelper.S [FixedPcd] - gArmTokenSpaceGuid.PcdSystemMemoryBase - gArmTokenSpaceGuid.PcdSystemMemorySize - gArmTokenSpaceGuid.PcdArmPrimaryCoreMask gArmTokenSpaceGuid.PcdArmPrimaryCore