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

["Lendacky, Thomas via groups.io" <thomas.lendacky=amd.com@groups.io>]

On 2/9/24 02:11, Yao, Jiewen wrote:
> Some initial feedback:
> 
> Patch 1 - OvmfPkg/BaseMemEncryptSevLib: Re-organize page state change support
> Please split MdePkg update, since it requires different reviewer.

Yes, I had noticed this also and have split this out separately.

> 
> 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?

I chose the CcExitLib library because of the issuance of GHCB requests / 
VMGEXIT and the guarding of the GHCB from that library today. I can 
certainly look at creating a separate library if that is truly 
required/preferred, but to me it made sense to put that function in that 
library. Please let me know your thoughts.

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

Yes, Ray had the same request and it will be converted to a HOB in the 
next version.

Thanks,
Tom

> 
> 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 (#115308): https://edk2.groups.io/g/devel/message/115308
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]
-=-=-=-=-=-=-=-=-=-=-=-