Re: [edk2-devel] [PATCH v6 00/42] SEV-ES guest support

["Lendacky, Thomas" <thomas.lendacky@amd.com>]

On 3/30/20 7:47 PM, Dong, Eric wrote:
> Hi Tom,
> 
> Sorry for late response. It’s a huge patch, please give me two more weeks 
> to detail review them.
> 
> I have rough go through these patches and have some basic comments for 
> them now:
> 
> 1.It’s better to spit patch if changes files not in same package. Like 
> patch 1/42.

Ok, will do.

> 
> 2.All functions need to have comments for them. Miss comments in patch 
> 10/42 and others.

Just external functions or both external and internal (STATIC) functions, too?

Thanks,
Tom

> 
> Please update patches to fix above basic checks first.
> 
> Thanks,
> 
> Eric
> 
> *From:*devel@edk2.groups.io [mailto:devel@edk2.groups.io] *On Behalf Of 
> *Lendacky, Thomas
> *Sent:* Tuesday, March 31, 2020 12:54 AM
> *To:* devel@edk2.groups.io
> *Cc:* Justen, Jordan L <jordan.l.justen@intel.com>; Laszlo Ersek 
> <lersek@redhat.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>; Kinney, 
> Michael D <michael.d.kinney@intel.com>; Gao, Liming 
> <liming.gao@intel.com>; Dong, Eric <eric.dong@intel.com>; Ni, Ray 
> <ray.ni@intel.com>; Brijesh Singh <brijesh.singh@amd.com>; You, Benjamin 
> <benjamin.you@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Dong, Guo 
> <guo.dong@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Wang, Jian J 
> <jian.j.wang@intel.com>; Ma, Maurice <maurice.ma@intel.com>
> *Subject:* Re: [edk2-devel] [PATCH v6 00/42] SEV-ES guest support
> 
> I've gotten some nice feedback from Laszlo, especially on the OvmfPkg side
> of this patchset, but haven't seen much response from the other
> maintainers. Is there any feedback on the MdePkg, MdeModulePkg and
> UefiCpuPkg changes that needs to be addressed in order to merge this?
> 
> I do have some minor changes on ensuring the per-CPU variable page stays
> encrypted, but not much beyond that. Those changes can be submitted
> afterwards or as a new version before inclusion.
> 
> Thanks,
> Tom
> 
> On 3/24/20 12:40 PM, Tom Lendacky wrote:
>> This patch series provides support for running EDK2/OVMF under SEV-ES.
>> 
>> Secure Encrypted Virtualization - Encrypted State (SEV-ES) expands on the
>> SEV support to protect the guest register state from the hypervisor. See
>> "AMD64 Architecture Programmer's Manual Volume 2: System Programming",
>> section "15.35 Encrypted State (SEV-ES)" [1].
>> 
>> In order to allow a hypervisor to perform functions on behalf of a guest,
>> there is architectural support for notifying a guest's operating system
>> when certain types of VMEXITs are about to occur. This allows the guest to
>> selectively share information with the hypervisor to satisfy the requested
>> function. The notification is performed using a new exception, the VMM
>> Communication exception (#VC). The information is shared through the
>> Guest-Hypervisor Communication Block (GHCB) using the VMGEXIT instruction.
>> The GHCB format and the protocol for using it is documented in "SEV-ES
>> Guest-Hypervisor Communication Block Standardization" [2].
>> 
>> The main areas of the EDK2 code that are updated to support SEV-ES are
>> around the exception handling support and the AP boot support.
>> 
>> Exception support is required starting in Sec, continuing through Pei
>> and into Dxe in order to handle #VC exceptions that are generated.  Each
>> AP requires it's own GHCB page as well as a page to hold values specific
>> to that AP.
>> 
>> AP booting poses some interesting challenges. The INIT-SIPI-SIPI sequence
>> is typically used to boot the APs. However, the hypervisor is not allowed
>> to update the guest registers. The GHCB document [2] talks about how SMP
>> booting under SEV-ES is performed.
>> 
>> Since the GHCB page must be a shared (unencrypted) page, the processor
>> must be running in long mode in order for the guest and hypervisor to
>> communicate with each other. As a result, SEV-ES is only supported under
>> the X64 architecture.
>> 
>> [1] https://www.amd.com/system/files/TechDocs/24593.pdf 
> <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.amd.com%2Fsystem%2Ffiles%2FTechDocs%2F24593.pdf&data=02%7C01%7Cthomas.lendacky%40amd.com%7C2ee33c1d932a4906558f08d7d50d1ca2%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637212125835211690&sdata=Q%2BIjeq%2FRDgiovKtPeA4TGDVorCK07jQVNZ7N9kvD%2BuE%3D&reserved=0>
>> [2] https://developer.amd.com/wp-content/resources/56421.pdf 
> <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdeveloper.amd.com%2Fwp-content%2Fresources%2F56421.pdf&data=02%7C01%7Cthomas.lendacky%40amd.com%7C2ee33c1d932a4906558f08d7d50d1ca2%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637212125835221679&sdata=bos02T0YR3i5xji9rhjPl7jpS5uJPKt1Q0hhdy%2FoMR0%3D&reserved=0>
>> 
>> ---
>> 
>> These patches are based on commit:
>> 2f524a745e23 ("BaseTools:Fix build tools print traceback info issue")
>> 
>> Proper execution of SEV-ES relies on Bugzilla 2340 being fixed.
>> 
>> A version of the tree (with an extra patch to workaround Bugzilla 2340) can
>> be found at:
>> https://github.com/AMDESE/ovmf/tree/sev-es-v13 
> <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FAMDESE%2Fovmf%2Ftree%2Fsev-es-v13&data=02%7C01%7Cthomas.lendacky%40amd.com%7C2ee33c1d932a4906558f08d7d50d1ca2%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637212125835221679&sdata=fmIyS5QBB7YGDSqTFiBIe%2BBdH1zatcEplUdNC2wi%2Fhc%3D&reserved=0>
>> 
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org <mailto:ard.biesheuvel@linaro.org>>
>> Cc: Benjamin You <benjamin.you@intel.com <mailto:benjamin.you@intel.com>>
>> Cc: Dandan Bi <dandan.bi@intel.com <mailto:dandan.bi@intel.com>>
>> Cc: Eric Dong <eric.dong@intel.com <mailto:eric.dong@intel.com>>
>> Cc: Guo Dong <guo.dong@intel.com <mailto:guo.dong@intel.com>>
>> Cc: Hao A Wu <hao.a.wu@intel.com <mailto:hao.a.wu@intel.com>>
>> Cc: Jian J Wang <jian.j.wang@intel.com <mailto:jian.j.wang@intel.com>>
>> Cc: Jordan Justen <jordan.l.justen@intel.com <mailto:jordan.l.justen@intel.com>>
>> Cc: Laszlo Ersek <lersek@redhat.com <mailto:lersek@redhat.com>>
>> Cc: Liming Gao <liming.gao@intel.com <mailto:liming.gao@intel.com>>
>> Cc: Maurice Ma <maurice.ma@intel.com <mailto:maurice.ma@intel.com>>
>> Cc: Michael D Kinney <michael.d.kinney@intel.com <mailto:michael.d.kinney@intel.com>>
>> Cc: Ray Ni <ray.ni@intel.com <mailto:ray.ni@intel.com>>
>> 
>> Changes since v5:
>> - Remove extraneous VmgExitLib usage
>> - Miscellaneous changes to address feedback (coding style, etc.)
>> 
>> Changes since v4:
>> - Move the SEV-ES protocol negotiation out of the SEC exception handler
>>    and into the SecMain.c file. As a result:
>>    - Move the SecGhcb related PCDs out of UefiCpuPkg and into OvmfPkg
>>    - Combine SecAMDSevVcHandler.c and PeiDxeAMDSevVcHandler.c into a
>>      single AMDSevVcHandler.c
>> - Consolidate VmgExitLib usage into common LibraryClasses sections
>> - Add documentation comments to the VmgExitLib functions
>> 
>> Changes since v3:
>> - Remove the need for the MP library finalization routine. The AP
>>    jump table address will be held by the hypervisor rather than
>>    communicated via the GHCB MSR. This removes some fragility around
>>    the UEFI to OS transition.
>> - Rename the SEV-ES RIP reset area to SEV-ES workarea and use it to
>>    communicate the SEV-ES status, so that SEC CPU exception handling is
>>    only established for an SEV-ES guest.
>> - Fix SMM build breakageAdd around QemuFlashPtrWrite().
>> - Fix SMM build breakage by adding VC exception support the SMM CPU
>>    exception handling.
>> - Add memory fencing around the invocation of AsmVmgExit().
>> - Clarify comments around the SEV-ES AP reset RIP values and usage.
>> - Move some PCD definitions from MdeModulePkg to UefiCpuPkg.
>> - Remove the 16-bit code selector definition from MdeModulePkg
>> 
>> Changes since v2:
>> - Added a way to locate the SEV-ES fixed AP RIP address for starting
>>    AP's to avoid updating the actual flash image (build time location
>>    that is identified with a GUID value).
>> - Create a VmgExit library to replace static inline functions.
>> - Move some PCDs to the appropriate packages
>> - Add support for writing to QEMU flash under SEV-ES
>> - Add additional MMIO opcode support
>> - Cleaned up the GHCB MSR CPUID protocol support
>> 
>> Changes since v1:
>> - Patches reworked to be more specific to the component/area being updated
>>    and order of definition/usage
>> - Created a library for VMGEXIT-related functions to replace use of inline
>>    functions
>> - Allocation method for GDT changed from AllocatePool to AllocatePages
>> - Early caching only enabled for SEV-ES guests
>> - Ensure AP loop mode set to halt loop mode for SEV-ES guests
>> - Reserved SEC GHCB-related memory areas when S3 is enabled
>> 
>> Tom Lendacky (42):
>>    MdePkg: Create PCDs to be used in support of SEV-ES
>>    MdePkg: Add the MSR definition for the GHCB register
>>    MdePkg: Add a structure definition for the GHCB
>>    MdeModulePkg/DxeIplPeim: Support GHCB pages when creating page tables
>>    MdePkg/BaseLib: Add support for the XGETBV instruction
>>    MdePkg/BaseLib: Add support for the VMGEXIT instruction
>>    UefiCpuPkg: Implement library support for VMGEXIT
>>    OvmfPkg: Prepare OvmfPkg to use the VmgExitLib library
>>    UefiPayloadPkg: Prepare UefiPayloadPkg to use the VmgExitLib library
>>    UefiCpuPkg/CpuExceptionHandler: Add base support for the #VC exception
>>    UefiCpuPkg/CpuExceptionHandler: Add support for IOIO_PROT NAE events
>>    UefiCpuPkg/CpuExceptionHandler: Support string IO for IOIO_PROT NAE
>>      events
>>    UefiCpuPkg/CpuExceptionHandler: Add support for CPUID NAE events
>>    UefiCpuPkg/CpuExceptionHandler: Add support for MSR_PROT NAE events
>>    UefiCpuPkg/CpuExceptionHandler: Add support for NPF NAE events (MMIO)
>>    UefiCpuPkg/CpuExceptionHandler: Add support for WBINVD NAE events
>>    UefiCpuPkg/CpuExceptionHandler: Add support for RDTSC NAE events
>>    UefiCpuPkg/CpuExceptionHandler: Add support for RDPMC NAE events
>>    UefiCpuPkg/CpuExceptionHandler: Add support for INVD NAE events
>>    UefiCpuPkg/CpuExceptionHandler: Add support for VMMCALL NAE events
>>    UefiCpuPkg/CpuExceptionHandler: Add support for RDTSCP NAE events
>>    UefiCpuPkg/CpuExceptionHandler: Add support for MONITOR/MONITORX NAE
>>      events
>>    UefiCpuPkg/CpuExceptionHandler: Add support for MWAIT/MWAITX NAE
>>      events
>>    UefiCpuPkg/CpuExceptionHandler: Add support for DR7 Read/Write NAE
>>      events
>>    OvmfPkg/MemEncryptSevLib: Add an SEV-ES guest indicator function
>>    OvmfPkg: Add support to perform SEV-ES initialization
>>    OvmfPkg: Create a GHCB page for use during Sec phase
>>    OvmfPkg/PlatformPei: Reserve GHCB-related areas if S3 is supported
>>    OvmfPkg: Create GHCB pages for use during Pei and Dxe phase
>>    OvmfPkg/PlatformPei: Move early GDT into ram when SEV-ES is enabled
>>    UefiCpuPkg: Create an SEV-ES workarea PCD
>>    OvmfPkg: Reserve a page in memory for the SEV-ES usage
>>    OvmfPkg/ResetVector: Add support for a 32-bit SEV check
>>    OvmfPkg/Sec: Add #VC exception handling for Sec phase
>>    OvmfPkg/Sec: Enable cache early to speed up booting
>>    OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Bypass flash detection with
>>      SEV-ES is enabled
>>    UefiCpuPkg: Add a 16-bit protected mode code segment descriptor
>>    UefiCpuPkg/MpInitLib: Add CPU MP data flag to indicate if SEV-ES is
>>      enabled
>>    UefiCpuPkg: Allow AP booting under SEV-ES
>>    OvmfPkg: Use the SEV-ES work area for the SEV-ES AP reset vector
>>    OvmfPkg: Move the GHCB allocations into reserved memory
>>    UefiCpuPkg/MpInitLib: Prepare SEV-ES guest APs for OS use
>> 
>>   MdeModulePkg/MdeModulePkg.dec                 |    9 +
>>   OvmfPkg/OvmfPkg.dec                           |    9 +
>>   UefiCpuPkg/UefiCpuPkg.dec                     |   17 +
>>   OvmfPkg/OvmfPkgIa32.dsc                       |    6 +
>>   OvmfPkg/OvmfPkgIa32X64.dsc                    |    6 +
>>   OvmfPkg/OvmfPkgX64.dsc                        |    6 +
>>   OvmfPkg/OvmfXen.dsc                           |    1 +
>>   UefiCpuPkg/UefiCpuPkg.dsc                     |    2 +
>>   UefiPayloadPkg/UefiPayloadPkgIa32.dsc         |    2 +
>>   UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc      |    2 +
>>   OvmfPkg/OvmfPkgX64.fdf                        |    9 +
>>   MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf       |    2 +
>>   MdePkg/Library/BaseLib/BaseLib.inf            |    4 +
>>   OvmfPkg/PlatformPei/PlatformPei.inf           |    7 +
>>   .../FvbServicesRuntimeDxe.inf                 |    2 +
>>   OvmfPkg/ResetVector/ResetVector.inf           |    8 +
>>   OvmfPkg/Sec/SecMain.inf                       |    4 +
>>   .../DxeCpuExceptionHandlerLib.inf             |    5 +
>>   .../PeiCpuExceptionHandlerLib.inf             |    5 +
>>   .../SecPeiCpuExceptionHandlerLib.inf          |    5 +
>>   .../SmmCpuExceptionHandlerLib.inf             |    5 +
>>   UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf |    4 +
>>   UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf |    4 +
>>   UefiCpuPkg/Library/VmgExitLib/VmgExitLib.inf  |   33 +
>>   .../Core/DxeIplPeim/X64/VirtualMemory.h       |   12 +-
>>   MdePkg/Include/Library/BaseLib.h              |   31 +
>>   MdePkg/Include/Register/Amd/Fam17Msr.h        |   42 +
>>   MdePkg/Include/Register/Amd/Ghcb.h            |  136 ++
>>   OvmfPkg/Include/Library/MemEncryptSevLib.h    |   12 +
>>   .../QemuFlash.h                               |    6 +
>>   UefiCpuPkg/CpuDxe/CpuGdt.h                    |    4 +-
>>   UefiCpuPkg/Include/Library/VmgExitLib.h       |  111 ++
>>   .../CpuExceptionHandlerLib/AMDSevVcCommon.h   |   26 +
>>   .../CpuExceptionCommon.h                      |    2 +
>>   UefiCpuPkg/Library/MpInitLib/MpLib.h          |   68 +-
>>   .../Core/DxeIplPeim/Ia32/DxeLoadFunc.c        |    4 +-
>>   .../Core/DxeIplPeim/X64/DxeLoadFunc.c         |   11 +-
>>   .../Core/DxeIplPeim/X64/VirtualMemory.c       |   49 +-
>>   MdePkg/Library/BaseLib/Ia32/GccInline.c       |   45 +
>>   MdePkg/Library/BaseLib/X64/GccInline.c        |   47 +
>>   .../MemEncryptSevLibInternal.c                |   75 +-
>>   OvmfPkg/PlatformPei/AmdSev.c                  |   82 ++
>>   OvmfPkg/PlatformPei/MemDetect.c               |   23 +
>>   .../QemuFlash.c                               |   23 +-
>>   .../QemuFlashDxe.c                            |   15 +
>>   .../QemuFlashSmm.c                            |    9 +
>>   OvmfPkg/Sec/SecMain.c                         |  160 ++-
>>   UefiCpuPkg/CpuDxe/CpuGdt.c                    |    8 +-
>>   .../CpuExceptionHandlerLib/AMDSevVcHandler.c  |   29 +
>>   .../CpuExceptionCommon.c                      |    2 +-
>>   .../Ia32/ArchAMDSevVcHandler.c                |   24 +
>>   .../PeiDxeSmmCpuException.c                   |   16 +
>>   .../SecPeiCpuException.c                      |   16 +
>>   .../X64/ArchAMDSevVcHandler.c                 | 1237 +++++++++++++++++
>>   UefiCpuPkg/Library/MpInitLib/DxeMpLib.c       |  114 +-
>>   UefiCpuPkg/Library/MpInitLib/MpLib.c          |  257 +++-
>>   UefiCpuPkg/Library/MpInitLib/PeiMpLib.c       |   19 +
>>   UefiCpuPkg/Library/VmgExitLib/VmgExitLib.c    |  249 ++++
>>   UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c  |    2 +-
>>   MdePkg/Library/BaseLib/Ia32/VmgExit.nasm      |   37 +
>>   MdePkg/Library/BaseLib/Ia32/XGetBv.nasm       |   31 +
>>   MdePkg/Library/BaseLib/X64/VmgExit.nasm       |   32 +
>>   MdePkg/Library/BaseLib/X64/XGetBv.nasm        |   34 +
>>   OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm  |  100 ++
>>   OvmfPkg/ResetVector/Ia32/PageTables64.asm     |  351 ++++-
>>   OvmfPkg/ResetVector/ResetVector.nasmb         |   20 +
>>   .../X64/ExceptionHandlerAsm.nasm              |   17 +
>>   UefiCpuPkg/Library/MpInitLib/Ia32/MpEqu.inc   |    2 +-
>>   .../Library/MpInitLib/Ia32/MpFuncs.nasm       |   15 +
>>   UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc    |    4 +-
>>   UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm |  370 ++++-
>>   UefiCpuPkg/Library/VmgExitLib/VmgExitLib.uni  |   15 +
>>   .../ResetVector/Vtf0/Ia16/Real16ToFlat32.asm  |    9 +
>>   73 files changed, 4061 insertions(+), 99 deletions(-)
>>   create mode 100644 UefiCpuPkg/Library/VmgExitLib/VmgExitLib.inf
>>   create mode 100644 MdePkg/Include/Register/Amd/Ghcb.h
>>   create mode 100644 UefiCpuPkg/Include/Library/VmgExitLib.h
>>   create mode 100644 UefiCpuPkg/Library/CpuExceptionHandlerLib/AMDSevVcCommon.h
>>   create mode 100644 UefiCpuPkg/Library/CpuExceptionHandlerLib/AMDSevVcHandler.c
>>   create mode 100644 UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ArchAMDSevVcHandler.c
>>   create mode 100644 UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchAMDSevVcHandler.c
>>   create mode 100644 UefiCpuPkg/Library/VmgExitLib/VmgExitLib.c
>>   create mode 100644 MdePkg/Library/BaseLib/Ia32/VmgExit.nasm
>>   create mode 100644 MdePkg/Library/BaseLib/Ia32/XGetBv.nasm
>>   create mode 100644 MdePkg/Library/BaseLib/X64/VmgExit.nasm
>>   create mode 100644 MdePkg/Library/BaseLib/X64/XGetBv.nasm
>>   create mode 100644 OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
>>   create mode 100644 UefiCpuPkg/Library/VmgExitLib/VmgExitLib.uni
>> 
> 
> 
>