Re: [edk2-devel] [PATCH 00/16] Provide SEV-SNP support for running under an SVSM

["Yao, Jiewen" <jiewen.yao@intel.com>]

Some initial feedback:

Patch 1 - OvmfPkg/BaseMemEncryptSevLib: Re-organize page state change support
Please split MdePkg update, since it requires different reviewer.

Patch 4 - UefiCpuPkg/CcExitLib: Extend the CcExitLib library to support an SVSM
I am not sure why we need to expose SVSM API in CcExitLib. Why the Exception handle need to aware of SVSM?
If other library need SVSM API, then why not create a SvsmLib?

Patch 11 - UefiCpuPkg: Create APIC ID list PCD
Why use PCD? Why not use HOB?

Thank you
Yao, Jiewen

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Yao, Jiewen
> Sent: Sunday, January 28, 2024 12:11 PM
> To: Tom Lendacky <thomas.lendacky@amd.com>; devel@edk2.groups.io
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>; Aktas, Erdem
> <erdemaktas@google.com>; Gerd Hoffmann <kraxel@redhat.com>; Laszlo Ersek
> <lersek@redhat.com>; Liming Gao <gaoliming@byosoft.com.cn>; Kinney, Michael
> D <michael.d.kinney@intel.com>; Xu, Min M <min.m.xu@intel.com>; Liu,
> Zhiguang <zhiguang.liu@intel.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>;
> Ni, Ray <ray.ni@intel.com>; Michael Roth <michael.roth@amd.com>
> Subject: Re: [edk2-devel] [PATCH 00/16] Provide SEV-SNP support for running
> under an SVSM
> 
> Thanks Tom. Below is exactly what I am looking for:
> "the decision to use the SVSM API will be based on the VMPL level at which
> OVMF is running."
> 
> OVMF needs to detect SEV-SNP, then make next level decision on VMPL.
> Makes sense to me.
> 
> Thank you
> Yao, Jiewen
> 
> > -----Original Message-----
> > From: Tom Lendacky <thomas.lendacky@amd.com>
> > Sent: Sunday, January 28, 2024 1:49 AM
> > To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io
> > Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>; Aktas, Erdem
> > <erdemaktas@google.com>; Gerd Hoffmann <kraxel@redhat.com>; Laszlo
> Ersek
> > <lersek@redhat.com>; Liming Gao <gaoliming@byosoft.com.cn>; Kinney,
> Michael
> > D <michael.d.kinney@intel.com>; Xu, Min M <min.m.xu@intel.com>; Liu,
> > Zhiguang <zhiguang.liu@intel.com>; Kumar, Rahul R
> <rahul.r.kumar@intel.com>;
> > Ni, Ray <ray.ni@intel.com>; Michael Roth <michael.roth@amd.com>
> > Subject: Re: [PATCH 00/16] Provide SEV-SNP support for running under an SVSM
> >
> > On 1/26/24 22:04, Yao, Jiewen wrote:
> > > Thanks Tom.
> > > Please give me some time to digest this patch set before I can give some
> > feedback.
> > >
> > > One quick question to you:
> > > With this patch, we need to support multiple SEV modes:
> > > 1. SEV guest firmware
> > > 2. SEV-ES guest firmware
> > > 3. SEV-SNP guest firmware
> > > 4. SEV-SNP SVSM guest firmware
> >
> > This last mode is still an SNP guest, it just requires invoking an API to
> > perform operations that require VMPL0 permissions. I'm not sure what you
> > mean by having firmware at the end of each mode. The same firmware is used
> > for all SEV guest modes as well as non-SEV guests.
> >
> > > And all these mode requires runtime detection. Am I right?
> >
> > Yes
> >
> > > If so, where is the flag to set those mode?
> >
> > There are function calls available to detect the SEV mode. See the
> > implementation of MemEncryptSevIsEnabled(), MemEncryptSevEsIsEnabled()
> and
> > MemEncryptSevSnpIsEnabled().
> >
> > OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncryptSevLibInternal.c
> > OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLibInternal.c
> > OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLibInternal.c
> >
> > (OvmfPkg/Sec/AmdSev.c also has some early detection support)
> >
> > Note:
> >    - An SEV-SNP guest is also considered an SEV-ES and SEV guest.
> >    - An SEV-ES guest is also considered an SEV guest.
> >
> > Within the CcExitLib library, the decision to use the SVSM API will be
> > based on the VMPL level at which OVMF is running.
> >
> > Thanks,
> > Tom
> >
> > >
> > > Please correct me if my understanding is wrong.
> > >
> > > Thank you
> > > Yao, Jiewen
> > >
> > >> -----Original Message-----
> > >> From: Tom Lendacky <thomas.lendacky@amd.com>
> > >> Sent: Saturday, January 27, 2024 6:13 AM
> > >> To: devel@edk2.groups.io
> > >> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>; Aktas, Erdem
> > >> <erdemaktas@google.com>; Gerd Hoffmann <kraxel@redhat.com>; Yao,
> > Jiewen
> > >> <jiewen.yao@intel.com>; Laszlo Ersek <lersek@redhat.com>; Liming Gao
> > >> <gaoliming@byosoft.com.cn>; Kinney, Michael D
> > <michael.d.kinney@intel.com>;
> > >> Xu, Min M <min.m.xu@intel.com>; Liu, Zhiguang <zhiguang.liu@intel.com>;
> > >> Kumar, Rahul R <rahul.r.kumar@intel.com>; Ni, Ray <ray.ni@intel.com>;
> > Michael
> > >> Roth <michael.roth@amd.com>
> > >> Subject: [PATCH 00/16] Provide SEV-SNP support for running under an SVSM
> > >>
> > >>
> > >> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4654
> > >>
> > >> This series adds SEV-SNP support for running OVMF under an Secure VM
> > >> Service Module (SVSM) at a less privileged VM Privilege Level (VMPL).
> > >> By running at a less priviledged VMPL, the SVSM can be used to provide
> > >> services, e.g. a virtual TPM, for the guest OS within the SEV-SNP
> > >> confidential VM (CVM) rather than trust such services from the hypervisor.
> > >>
> > >> Currently, OVMF expects to run at the highest VMPL, VMPL0, and there are
> > >> certain SNP related operations that require that VMPL level. Specifically,
> > >> the PVALIDATE instruction and the RMPADJUST instruction when setting the
> > >> the VMSA attribute of a page (used when starting APs).
> > >>
> > >> If OVMF is to run at a less privileged VMPL, e.g. VMPL2, then it must
> > >> use an SVSM (which is running at VMPL0) to perform the operations that
> > >> it is no longer able to perform.
> > >>
> > >> How OVMF interacts with and uses the SVSM is documented in the SVSM
> > >> specification [1] and the GHCB specification [2].
> > >>
> > >> This series introduces support to run OVMF under an SVSM. It consists
> > >> of:
> > >>    - Reorganize the page state change support to not directly use the
> > >>      GHCB buffer since an SVSM will use the calling area buffer, instead
> > >>    - Detecting the presence of an SVSM
> > >>    - When not running at VMPL0, invoking the SVSM for page validation and
> > >>      VMSA page creation/deletion
> > >>    - Retrieving the list of vCPU APIC IDs and starting up all APs without
> > >>      performing a broadcast SIPI
> > >>    - Detecting and allowing OVMF to run in a VMPL other than 0 when an
> > >>      SVSM is present
> > >>
> > >> The series is based off of commit:
> > >>
> > >>    7d7decfa3dc8 ("UefiPayloadPkg/Crypto: Support external Crypto drivers.")
> > >>
> > >> [1] https://www.amd.com/content/dam/amd/en/documents/epyc-
> technical-
> > >> docs/specifications/58019.pdf
> > >> [2] https://www.amd.com/content/dam/amd/en/documents/epyc-
> technical-
> > >> docs/specifications/56421.pdf
> > >>
> > >> ---
> > >>
> > >> Tom Lendacky (16):
> > >>    OvmfPkg/BaseMemEncryptSevLib: Re-organize page state change support
> > >>    MdePkg/Register/Amd: Define the SVSM related information
> > >>    MdePkg/BaseLib: Add a new VMGEXIT instruction invocation for SVSM
> > >>    UefiCpuPkg/CcExitLib: Extend the CcExitLib library to support an SVSM
> > >>    Ovmfpkg/CcExitLib: Extend CcExitLib to handle SVSM related services
> > >>    OvmfPkg: Create a calling area used to communicate with the SVSM
> > >>    OvmfPkg/CcExitLib: Add support for the SVSM_CORE_PVALIDATE call
> > >>    OvmfPkg/CcExitLib: Add support for the SVSM create/delete vCPU calls
> > >>    UefiCpuPkg/MpInitLib: Use CcExitSnpVmsaRmpAdjust() to set/clear VMSA
> > >>    MdePkg: GHCB APIC ID retrieval support definitions
> > >>    UefiCpuPkg: Create APIC ID list PCD
> > >>    OvmfPkg/PlatformPei: Retrieve APIC IDs from the hypervisor
> > >>    UefiCpuPkg/MpInitLib: Always use AP Create if PcdSevSnpApicIds is set
> > >>    UefiCpuPkg/MpInitLib: AP creation support under an SVSM
> > >>    Ovmfpkg/CcExitLib: Provide SVSM discovery support
> > >>    OvmfPkg/BaseMemEncryptLib: Check for presence of an SVSM when not
> at
> > >>      VMPL0
> > >>
> > >>   OvmfPkg/OvmfPkg.dec                                                   |   4 +
> > >>   UefiCpuPkg/UefiCpuPkg.dec                                             |   7 +-
> > >>   OvmfPkg/AmdSev/AmdSevX64.fdf                                          |   9 +-
> > >>   OvmfPkg/OvmfPkgX64.fdf                                                |   3 +
> > >>   MdePkg/Library/BaseLib/BaseLib.inf                                    |   2 +
> > >>   OvmfPkg/Library/CcExitLib/CcExitLib.inf                               |   5 +-
> > >>   OvmfPkg/Library/CcExitLib/SecCcExitLib.inf                            |   5 +-
> > >>   OvmfPkg/PlatformPei/PlatformPei.inf                                   |   3 +
> > >>   OvmfPkg/ResetVector/ResetVector.inf                                   |   2 +
> > >>   UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf                         |   1 +
> > >>   UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf                         |   3 +-
> > >>   MdePkg/Include/Library/BaseLib.h                                      |  39 ++
> > >>   MdePkg/Include/Register/Amd/Fam17Msr.h                                |  19 +-
> > >>   MdePkg/Include/Register/Amd/Ghcb.h                                    |  19 +-
> > >>   MdePkg/Include/Register/Amd/Msr.h                                     |   3 +-
> > >>   MdePkg/Include/Register/Amd/Svsm.h                                    | 101 ++++
> > >>   MdePkg/Include/Register/Amd/SvsmMsr.h                                 |  35 ++
> > >>   OvmfPkg/Include/WorkArea.h                                            |   7 +
> > >>   OvmfPkg/Library/BaseMemEncryptSevLib/X64/SnpPageStateChange.h
> |
> > 4
> > >> +-
> > >>   OvmfPkg/Library/CcExitLib/CcExitSvsm.h                                |  29 ++
> > >>   UefiCpuPkg/Include/Library/CcExitLib.h                                |  71 ++-
> > >>   UefiCpuPkg/Library/MpInitLib/MpLib.h                                  |  27 +-
> > >>
> OvmfPkg/Library/BaseMemEncryptSevLib/X64/DxeSnpSystemRamValidate.c
> > |
> > >> 16 +-
> > >>   OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c
> |
> > 25
> > >> +-
> > >>
> OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiSnpSystemRamValidate.c
> > |
> > >> 20 +-
> > >>
> OvmfPkg/Library/BaseMemEncryptSevLib/X64/SecSnpSystemRamValidate.c
> > |
> > >> 25 +-
> > >>
> > OvmfPkg/Library/BaseMemEncryptSevLib/X64/SnpPageStateChangeInternal.c
> |
> > >> 203 ++++----
> > >>   OvmfPkg/Library/CcExitLib/CcExitSvsm.c                                | 532
> > >> ++++++++++++++++++++
> > >>   OvmfPkg/Library/CcExitLib/CcExitVcHandler.c                           |  29 +-
> > >>   OvmfPkg/PlatformPei/AmdSev.c                                          | 100 +++-
> > >>   UefiCpuPkg/Library/CcExitLibNull/CcExitLibNull.c                      |  82 ++-
> > >>   UefiCpuPkg/Library/MpInitLib/Ia32/AmdSev.c                            |  19 +-
> > >>   UefiCpuPkg/Library/MpInitLib/MpLib.c                                  |   7 +-
> > >>   UefiCpuPkg/Library/MpInitLib/X64/AmdSev.c                             | 127 +++--
> > >>   MdePkg/Library/BaseLib/Ia32/VmgExitSvsm.nasm                          |  39 ++
> > >>   MdePkg/Library/BaseLib/X64/VmgExitSvsm.nasm                           |  94 ++++
> > >>   OvmfPkg/ResetVector/ResetVector.nasmb                                 |   6 +-
> > >>   OvmfPkg/ResetVector/X64/OvmfSevMetadata.asm                           |   9 +
> > >>   UefiCpuPkg/UefiCpuPkg.uni                                             |   3 +
> > >>   39 files changed, 1524 insertions(+), 210 deletions(-)
> > >>   create mode 100644 MdePkg/Include/Register/Amd/Svsm.h
> > >>   create mode 100644 MdePkg/Include/Register/Amd/SvsmMsr.h
> > >>   create mode 100644 OvmfPkg/Library/CcExitLib/CcExitSvsm.h
> > >>   create mode 100644 OvmfPkg/Library/CcExitLib/CcExitSvsm.c
> > >>   create mode 100644 MdePkg/Library/BaseLib/Ia32/VmgExitSvsm.nasm
> > >>   create mode 100644 MdePkg/Library/BaseLib/X64/VmgExitSvsm.nasm
> > >>
> > >> --
> > >> 2.42.0
> > >
> 
> 
> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115289): https://edk2.groups.io/g/devel/message/115289
Mute This Topic: https://groups.io/mt/103986434/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-