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

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

Some feedback:

1) 0002-MdePkg-GHCB-APIC-ID-retrieval-support-definitions

MdePkg only contains the definition in the standard.

Question: Is EFI_APIC_IDS_GUID definition in some AMD/SVSM specification?

2) 0012-UefiCpuPkg-CcSvsmLib-Create-the-CcSvsmLib-library-to-support-an-SVSM

I am not sure the position of SVSM.
If the SVSM interface is AMD specific, the it should be AmdSvsmLib.
If the SVSM interface is generic, then we should define everything in a generic way.

It is very confusing to mix a generic CcSvsm lib with AMD specific <Register/Amd/Ghcb.h>.


Thank you
Yao, Jiewen

> -----Original Message-----
> From: Tom Lendacky <thomas.lendacky@amd.com>
> Sent: Friday, February 23, 2024 1:30 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 v2 00/23] 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.
> 
> When running under an SVSM, OVMF must know the APIC IDs of the vCPUs that
> it will be starting. As a result, the GHCB APIC ID retrieval action must
> be performed. Since this service can also work with SEV-SNP running at
> VMPL0, the patches to make use of this feature are near the beginning of
> the series.
> 
> How OVMF interacts with and uses the SVSM is documented in the SVSM
> specification [1] and the GHCB specification [2].
> 
> This support creates a new CcSvsmLib library that is used by MpInitLib.
> This requires an update to the edk2-platform DSC files to add the new
> library. The edk2-platform change would be needed after patch 12, but
> before patch 15.
> 
> This series introduces support to run OVMF under an SVSM. It consists
> of:
>   - Retrieving the list of vCPU APIC IDs and starting up all APs without
>     performing a broadcast SIPI
>   - Reorganizing 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
>   - Detecting and allowing OVMF to run in a VMPL other than 0 when an
>     SVSM is present
> 
> The series is based off of commit:
> 
>   2ca8d5597443 ("UefiCpuPkg/PiSmmCpuDxeSmm: Check BspIndex first before
> lock cmpxchg")
> 
> [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
> 
> ---
> 
> Changes in v2:
> - Move the APIC IDs retrieval support to the beginning of the patch series
>     - Use a GUIDed HOB to hold the APIC ID list instead of a PCD
> - Split up Page State Change reorganization into multiple patches
> - Created CcSvsmLib library instead of extending CcExitLib
>     - This will require a corresponding update to edk2-platform DSC files
>     - Removed Ray Ni's Acked-by since it is not a minor change
> - Variable name changes and other misc changes
> 
> Tom Lendacky (23):
>   OvmfPkg/BaseMemEncryptLib: Fix error check from AsmRmpAdjust()
>   MdePkg: GHCB APIC ID retrieval support definitions
>   OvmfPkg/PlatformPei: Retrieve APIC IDs from the hypervisor
>   UefiCpuPkg/MpInitLib: Always use AP Create if PcdSevSnpApicIds is set
>   OvmfPkg/BaseMemEncryptSevLib: Fix uncrustify errors
>   OvmfPkg/BaseMemEncryptSevLib: Calculate memory size for Page State
>     Change
>   MdePkg: Avoid hardcoded value for number of Page State Change entries
>   OvmfPkg/BaseMemEncryptSevLib: Re-organize page state change support
>   OvmfPkg/BaseMemEncryptSevLib: Maximize Page State Change efficiency
>   MdePkg/Register/Amd: Define the SVSM related information
>   MdePkg/BaseLib: Add a new VMGEXIT instruction invocation for SVSM
>   UefiCpuPkg/CcSvsmLib: Create the CcSvsmLib library to support an SVSM
>   UefiPayloadPkg: Prepare UefiPayloadPkg to use the CcSvsmLib library
>   Ovmfpkg/CcSvsmLib: Create CcSvsmLib to handle SVSM related services
>   UefiCpuPkg/MpInitLib: Use CcSvsmSnpVmsaRmpAdjust() to set/clear VMSA
>   OvmfPkg/BaseMemEncryptSevLib: Use CcSvsmSnpPvalidate() to validate
>     pages
>   OvmfPkg: Create a calling area used to communicate with the SVSM
>   OvmfPkg/CcSvsmLib: Add support for the SVSM_CORE_PVALIDATE call
>   OvmfPkg/BaseMemEncryptSevLib: Maximize Page State Change efficiency
>   OvmfPkg/CcSvsmLib: Add support for the SVSM create/delete vCPU calls
>   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
> 
>  MdePkg/MdePkg.dec                                                     |   5 +-
>  OvmfPkg/OvmfPkg.dec                                                   |   4 +
>  UefiCpuPkg/UefiCpuPkg.dec                                             |   5 +-
>  OvmfPkg/AmdSev/AmdSevX64.dsc                                          |   1 +
>  OvmfPkg/Bhyve/BhyveX64.dsc                                            |   1 +
>  OvmfPkg/CloudHv/CloudHvX64.dsc                                        |   1 +
>  OvmfPkg/IntelTdx/IntelTdxX64.dsc                                      |   1 +
>  OvmfPkg/Microvm/MicrovmX64.dsc                                        |   1 +
>  OvmfPkg/OvmfPkgIa32.dsc                                               |   1 +
>  OvmfPkg/OvmfPkgIa32X64.dsc                                            |   3 +-
>  OvmfPkg/OvmfPkgX64.dsc                                                |   1 +
>  OvmfPkg/OvmfXen.dsc                                                   |   1 +
>  UefiCpuPkg/UefiCpuPkg.dsc                                             |   4 +-
>  UefiPayloadPkg/UefiPayloadPkg.dsc                                     |   1 +
>  OvmfPkg/AmdSev/AmdSevX64.fdf                                          |   9 +-
>  OvmfPkg/OvmfPkgX64.fdf                                                |   3 +
>  MdePkg/Library/BaseLib/BaseLib.inf                                    |   2 +
>  OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLib.inf          |   3
> +-
>  OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf          |   3 +-
>  OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncryptSevLib.inf          |   3
> +-
>  OvmfPkg/Library/CcExitLib/CcExitLib.inf                               |   3 +-
>  OvmfPkg/Library/CcExitLib/SecCcExitLib.inf                            |   3 +-
>  OvmfPkg/Library/CcSvsmLib/CcSvsmLib.inf                               |  38 ++
>  OvmfPkg/PlatformPei/PlatformPei.inf                                   |   3 +
>  OvmfPkg/ResetVector/ResetVector.inf                                   |   2 +
>  UefiCpuPkg/Library/CcSvsmLibNull/CcSvsmLibNull.inf                    |  27 ++
>  UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf                         |   2 +
>  UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf                         |   2 +
>  MdePkg/Include/Library/BaseLib.h                                      |  39 ++
>  MdePkg/Include/Register/Amd/Fam17Msr.h                                |  19 +-
>  MdePkg/Include/Register/Amd/Ghcb.h                                    |  23 +-
>  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                                            |   9 +-
>  OvmfPkg/Library/BaseMemEncryptSevLib/X64/SnpPageStateChange.h         |   6
> +-
>  UefiCpuPkg/Include/Library/CcSvsmLib.h                                | 101 ++++
>  UefiCpuPkg/Library/MpInitLib/MpLib.h                                  |  29 +-
>  OvmfPkg/Library/BaseMemEncryptSevLib/X64/DxeSnpSystemRamValidate.c    |
> 11 +-
>  OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c        |  27
> +-
>  OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiSnpSystemRamValidate.c    |
> 22 +-
>  OvmfPkg/Library/BaseMemEncryptSevLib/X64/SecSnpSystemRamValidate.c    |
> 31 +-
>  OvmfPkg/Library/BaseMemEncryptSevLib/X64/SnpPageStateChangeInternal.c |
> 206 ++++----
>  OvmfPkg/Library/CcExitLib/CcExitVcHandler.c                           |  29 +-
>  OvmfPkg/Library/CcSvsmLib/CcSvsmLib.c                                 | 500
> ++++++++++++++++++++
>  OvmfPkg/PlatformPei/AmdSev.c                                          | 102 +++-
>  UefiCpuPkg/Library/CcSvsmLibNull/CcSvsmLibNull.c                      | 108 +++++
>  UefiCpuPkg/Library/MpInitLib/Ia32/AmdSev.c                            |  21 +-
>  UefiCpuPkg/Library/MpInitLib/MpLib.c                                  |   9 +-
>  UefiCpuPkg/Library/MpInitLib/X64/AmdSev.c                             | 134 ++++--
>  MdePkg/Library/BaseLib/Ia32/VmgExitSvsm.nasm                          |  39 ++
>  MdePkg/Library/BaseLib/X64/VmgExitSvsm.nasm                           |  94 ++++
>  OvmfPkg/ResetVector/ResetVector.nasmb                                 |   6 +-
>  OvmfPkg/ResetVector/X64/OvmfSevMetadata.asm                           |  11 +-
>  UefiCpuPkg/Library/CcSvsmLibNull/CcSvsmLibNull.uni                    |  13 +
>  55 files changed, 1628 insertions(+), 233 deletions(-)
>  create mode 100644 OvmfPkg/Library/CcSvsmLib/CcSvsmLib.inf
>  create mode 100644 UefiCpuPkg/Library/CcSvsmLibNull/CcSvsmLibNull.inf
>  create mode 100644 MdePkg/Include/Register/Amd/Svsm.h
>  create mode 100644 MdePkg/Include/Register/Amd/SvsmMsr.h
>  create mode 100644 UefiCpuPkg/Include/Library/CcSvsmLib.h
>  create mode 100644 OvmfPkg/Library/CcSvsmLib/CcSvsmLib.c
>  create mode 100644 UefiCpuPkg/Library/CcSvsmLibNull/CcSvsmLibNull.c
>  create mode 100644 MdePkg/Library/BaseLib/Ia32/VmgExitSvsm.nasm
>  create mode 100644 MdePkg/Library/BaseLib/X64/VmgExitSvsm.nasm
>  create mode 100644 UefiCpuPkg/Library/CcSvsmLibNull/CcSvsmLibNull.uni
> 
> --
> 2.42.0



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