Thank you so much Yao for reviewing the patches. Based on some comments
from Gerd I may update code around the reset vector area (mainly use the
metadata format etc). For your comments regarding the introducing a new
PcdConfidentialComputingCategory I will look to see what I can come up
with and in UefiCpuPkg I will try to move all the SEV specific functions
in new files (where applicable).
thanks
Brijesh
On 9/6/21 9:36 PM, Yao, Jiewen wrote:
Thank you Brijesh
It took me a while to review this series. Here is my feedback.
I am not sure what you prefer, to put all comment together? Or reply 29 email
separately?
Let me put them together in this version. If you prefer a different way, please
let me know.
My strategy is same as previous. I will focus on common part and review as
detail as possible.
For SEV specific thing, I will ACK and let AMD people make decision unless I
have big concern on the design.
You can add my A-B and R-B in next version.
0001-OvmfPkg-reserve-SNP-secrets-page
Reviewed-by: Jiewen Yao <jiewen....@intel.com>
0002-OvmfPkg-reserve-CPUID-page-for-SEV-SNP
Reviewed-by: Jiewen Yao <jiewen....@intel.com>
0003-OvmfPkg-ResetVector-introduce-SEV-SNP-boot-block-GUID
I am still thinking if it is possible to move all SEV define GUID blob to a
standalone file, and TDX define GUID blob to another file.
Anyway, that can be done later.
Reviewed-by: Jiewen Yao <jiewen....@intel.com>
0004-OvmfPkg-ResetVector-invalidate-the-GHCB-page
Acked-by: Jiewen Yao <jiewen....@intel.com>
0005-OvmfPkg-ResetVector-check-the-vmpl-level
Acked-by: Jiewen Yao <jiewen....@intel.com>
0006-OvmfPkg-ResetVector-pre-validate-the-data-pages-used-in-SEC-phase
Acked-by: Jiewen Yao <jiewen....@intel.com>
0007-OvmfPkg-ResetVector-use-SEV-SNP-validated-CPUID-values
Acked-by: Jiewen Yao <jiewen....@intel.com>
0008-UefiCpuPkg-Define-the-SEV-SNP-specific-dynamic-PCDs
I really don't like the idea to use BOOL PcdSevEsIsEnabled and
PcdSevSnpIsEnabled.
Can we define *one* PCD - such as PcdConfidentialComputingCategory?
We can assign range 0x0000~0xFFFF to AMD SEV, 0x10000~0x1FFFF to Intel TDX.
Then SEV=0x0000, SEV-ES=0x0001, SEV-SNP=0x0002, and TDX=0x10000 later.
I really don't want to keep adding PCD endlessly in the future, like
PcdSevXXXIsEnabled, PcdSevYYYIsEnabled, PcdTdxIsEnabled, PcdTdx20Enabled,
PcdTdx30Enabled, ......
0009-OvmfPkg-MemEncryptSevLib-add-MemEncryptSevSnpEnabled()
I am not sure since we have PCD in 0008, why we need to expose the function -
MemEncryptSevSnpIsEnabled() and MemEncryptSevEsIsEnabled()?
Should we always use PCD anywhere else?
Anyway, Acked-by: Jiewen Yao <jiewen....@intel.com>
0010-OvmfPkg-SecMain-move-SEV-specific-routines-in-AmdSev.c
Reviewed-by: Jiewen Yao <jiewen....@intel.com>
0011-OvmfPkg-SecMain-register-GHCB-gpa-for-the-SEV-SNP-guest
Acked-by: Jiewen Yao <jiewen....@intel.com>
0012-OvmfPkg-VmgExitLib-use-SEV-SNP-validated-CPUID-values
Acked-by: Jiewen Yao <jiewen....@intel.com>
0013-OvmfPkg-PlatformPei-register-GHCB-gpa-for-the-SEV-SNP-guest
Acked-by: Jiewen Yao <jiewen....@intel.com>
0014-OvmfPkg-AmdSevDxe-do-not-use-extended-PCI-config-space
Acked-by: Jiewen Yao <jiewen....@intel.com>
0015-OvmfPkg-MemEncryptSevLib-add-support-to-validate-system-RAM
Acked-by: Jiewen Yao <jiewen....@intel.com>
0016-OvmfPkg-BaseMemEncryptSevLib-skip-the-pre-validated-system-RAM
Acked-by: Jiewen Yao <jiewen....@intel.com>
0017-OvmfPkg-MemEncryptSevLib-add-support-to-validate-4GB-memory-in-PEI-phase
Acked-by: Jiewen Yao <jiewen....@intel.com>
0018-OvmfPkg-SecMain-pre-validate-the-memory-used-for-decompressing-Fv
Acked-by: Jiewen Yao <jiewen....@intel.com>
0019-OvmfPkg-PlatformPei-validate-the-system-RAM-when-SNP-is-active
Acked-by: Jiewen Yao <jiewen....@intel.com>
0020-OvmfPkg-PlatformPei-set-the-SEV-SNP-enabled-PCD
See 0008
0021-OvmfPkg-PlatformPei-set-the-Hypervisor-Features-PCD
Acked-by: Jiewen Yao <jiewen....@intel.com>
0022-MdePkg-GHCB-increase-the-GHCB-protocol-max-version
Acked-by: Jiewen Yao <jiewen....@intel.com>
0023-UefiCpuPkg-MpLib-add-support-to-register-GHCB-GPA-when-SEV-SNP-is-enabled
1) See 0008.
2) For MpFuncs.nasm, I recommend to move AmdSev specific initialization to a
standalone file, such as Sev.nasm
0024-UefiCpuPkg-MpInitLib-use-BSP-to-do-extended-topology-check
See 0023
0025-OvmfPkg-MemEncryptSevLib-change-the-page-state-in-the-RMP-table
Acked-by: Jiewen Yao <jiewen....@intel.com>
0026-OvmfPkg-MemEncryptSevLib-skip-page-state-change-for-Mmio-address
Acked-by: Jiewen Yao <jiewen....@intel.com>
0027-OvmfPkg-PlatformPei-mark-cpuid-and-secrets-memory-reserved-in-EFI-map
Would you please move SEV specific init to another Sev.c?
Also I found MemEncryptSevEsIsEnabled() and MemEncryptSevSnpIsEnabled() are
there.
I suggest just use one API
MemEncryptSevEsIsEnabled() {
DoSevInitializeRamRegions()
}
Then you can check more in DoSevInitializeRamRegions().
DoSevInitializeRamRegions() {
MemEncryptSevSnpIsEnabled() {
}
}
0028-OvmfPkg-AmdSev-expose-the-SNP-reserved-pages-through-configuration-table
I am not convinced to include SEV specific data structure in a generic
structure in ConfidentialComputingSecret.h.
I recommend moving it to SEV specific file.
0029-UefiCpuPkg-MpInitLib-Use-SEV-SNP-AP-Creation-NAE-event-to-launch-APs
See 0008, 0023.
I recommend to move SevSnpCreateSaveArea() to Sev.c.
Thank you
Yao Jiewen
-----Original Message-----
From: Brijesh Singh <brijesh.si...@amd.com>
Sent: Thursday, September 2, 2021 12:16 AM
To: devel@edk2.groups.io
Cc: James Bottomley <j...@linux.ibm.com>; Xu, Min M <min.m...@intel.com>;
Yao, Jiewen <jiewen....@intel.com>; Tom Lendacky
<thomas.lenda...@amd.com>; Justen, Jordan L <jordan.l.jus...@intel.com>;
Ard Biesheuvel <ardb+tianoc...@kernel.org>; Erdem Aktas
<erdemak...@google.com>; Michael Roth <michael.r...@amd.com>; Gerd
Hoffmann <kra...@redhat.com>; Brijesh Singh <brijesh.si...@amd.com>
Subject: [PATCH v6 00/29] Add AMD Secure Nested Paging (SEV-SNP) support
BZ:
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3275&data=04%7C01%7Cbrijesh.singh%40amd.com%7C33df27781053475362e208d971a85cee%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637665791405981353%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=8vfBxVawRoEeDCR0DHJhfhTgPr66704twMGZ8%2BY%2BLGI%3D&reserved=0
SEV-SNP builds upon existing SEV and SEV-ES functionality while adding
new hardware-based memory protections. SEV-SNP adds strong memory
integrity
protection to help prevent malicious hypervisor-based attacks like data
replay, memory re-mapping and more in order to create an isolated memory
encryption environment.
This series provides the basic building blocks to support booting the SEV-SNP
VMs, it does not cover all the security enhancement introduced by the SEV-SNP
such as interrupt protection.
Many of the integrity guarantees of SEV-SNP are enforced through a new
structure called the Reverse Map Table (RMP). Adding a new page to SEV-SNP
VM requires a 2-step process. First, the hypervisor assigns a page to the
guest using the new RMPUPDATE instruction. This transitions the page to
guest-invalid. Second, the guest validates the page using the new PVALIDATE
instruction. The SEV-SNP VMs can use the new "Page State Change Request
NAE"
defined in the GHCB specification to ask hypervisor to add or remove page
from the RMP table.
Each page assigned to the SEV-SNP VM can either be validated or unvalidated,
as indicated by the Validated flag in the page's RMP entry. There are two
approaches that can be taken for the page validation: Pre-validation and
Lazy Validation.
Under pre-validation, the pages are validated prior to first use. And under
lazy validation, pages are validated when first accessed. An access to a
unvalidated page results in a #VC exception, at which time the exception
handler may validate the page. Lazy validation requires careful tracking of
the validated pages to avoid validating the same GPA more than once. The
recently introduced "Unaccepted" memory type can be used to communicate
the
unvalidated memory ranges to the Guest OS.
At this time we only support the pre-validation. OVMF detects all the available
system RAM in the PEI phase. When SEV-SNP is enabled, the memory is validated
before it is made available to the EDK2 core.
Now that series contains all the basic support required to launch SEV-SNP
guest. We are still missing the Interrupt security feature provided by the
SNP. The feature will be added after the base support is accepted.
Additional resources
---------------------
SEV-SNP whitepaper
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.amd.com%2Fsystem%2Ffiles%2FTechDocs%2FSEV-SNP-strengthening-vm-&data=04%7C01%7Cbrijesh.singh%40amd.com%7C33df27781053475362e208d971a85cee%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637665791405981353%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=tobk2zHk1ziA6nZ9bvwNrohRuIN7bTEh5ZXFNzwTTX0%3D&reserved=0
isolation-with-integrity-protection-and-more.pdf
APM 2:
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.amd.com%2Fsystem%2Ffiles%2FTechDocs%2F24593.pdf&data=04%7C01%7Cbrijesh.singh%40amd.com%7C33df27781053475362e208d971a85cee%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637665791405981353%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=%2FDTzbh8F6CtvvC263r7xJGX6WAQ8yCAuKLkPM7GwBvQ%3D&reserved=0
(section 15.36)
The complete source is available at
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FAMDESE%2Fovmf%2Ftree%2Fsev-snp-rfc-5&data=04%7C01%7Cbrijesh.singh%40amd.com%7C33df27781053475362e208d971a85cee%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637665791405981353%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=6lvmuqOQbvNoXG50qK5QGYG6XEdojJ%2BHlkKrODZRAHY%3D&reserved=0
GHCB spec:
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdeveloper.amd.com%2Fwp-content%2Fresources%2F56421.pdf&data=04%7C01%7Cbrijesh.singh%40amd.com%7C33df27781053475362e208d971a85cee%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637665791405981353%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=Q1oa5gB3CthKPkentzFJE3B3LfBpZq%2B4y8EzPTlPzl8%3D&reserved=0
SEV-SNP firmware specification:
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.amd.com%2Fsystem%2Ffiles%2FTechDocs%2F56860.pdf&data=04%7C01%7Cbrijesh.singh%40amd.com%7C33df27781053475362e208d971a85cee%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637665791405981353%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=7cLoMR52WAvMe%2Fr4rKGGYx2wadopvXKnSGi%2FghyEdJA%3D&reserved=0
Change since v5:
* When possible use the CPUID value from CPUID page
* Move the SEV specific functions from SecMain.c in AmdSev.c
* Rebase to the latest code
* Add the review feedback from Yao.
Change since v4:
* Use the correct MSR for the SEV_STATUS
* Add VMPL-0 check
Change since v3:
* ResetVector: move all SEV specific code in AmdSev.asm and add macros to
keep
the code readable.
* Drop extending the EsWorkArea to contain SNP specific state.
* Drop the GhcbGpa library and call the VmgExit directly to register GHCB GPA.
* Install the CC blob config table from AmdSevDxe instead of extending the
AmdSev/SecretsDxe for it.
* Add the separate PCDs for the SNP Secrets.
Changes since v2:
* Add support for the AP creation.
* Use the module-scoping override to make AmdSevDxe use the IO port for PCI
reads.
* Use the reserved memory type for CPUID and Secrets page.
*
Changes since v1:
* Drop the interval tree support to detect the pre-validated overlap region.
* Use an array to keep track of pre-validated regions.
* Add support to query the Hypervisor feature and verify that SNP feature is
supported.
* Introduce MemEncryptSevClearMmioPageEncMask() to clear the C-bit from
MMIO ranges.
* Pull the SevSecretDxe and SevSecretPei into OVMF package build.
* Extend the SevSecretDxe to expose confidential computing blob location
through
EFI configuration table.
Brijesh Singh (25):
OvmfPkg: reserve SNP secrets page
OvmfPkg: reserve CPUID page for SEV-SNP
OvmfPkg/ResetVector: introduce SEV-SNP boot block GUID
OvmfPkg/ResetVector: invalidate the GHCB page
OvmfPkg/ResetVector: check the vmpl level
OvmfPkg/ResetVector: pre-validate the data pages used in SEC phase
UefiCpuPkg: Define the SEV-SNP specific dynamic PCDs
OvmfPkg/MemEncryptSevLib: add MemEncryptSevSnpEnabled()
OvmfPkg/SecMain: move SEV specific routines in AmdSev.c
OvmfPkg/SecMain: register GHCB gpa for the SEV-SNP guest
OvmfPkg/PlatformPei: register GHCB gpa for the SEV-SNP guest
OvmfPkg/AmdSevDxe: do not use extended PCI config space
OvmfPkg/MemEncryptSevLib: add support to validate system RAM
OvmfPkg/BaseMemEncryptSevLib: skip the pre-validated system RAM
OvmfPkg/MemEncryptSevLib: add support to validate > 4GB memory in PEI
phase
OvmfPkg/SecMain: pre-validate the memory used for decompressing Fv
OvmfPkg/PlatformPei: validate the system RAM when SNP is active
OvmfPkg/PlatformPei: set the SEV-SNP enabled PCD
OvmfPkg/PlatformPei: set the Hypervisor Features PCD
MdePkg/GHCB: increase the GHCB protocol max version
UefiCpuPkg/MpLib: add support to register GHCB GPA when SEV-SNP is
enabled
OvmfPkg/MemEncryptSevLib: change the page state in the RMP table
OvmfPkg/MemEncryptSevLib: skip page state change for Mmio address
OvmfPkg/PlatformPei: mark cpuid and secrets memory reserved in EFI map
OvmfPkg/AmdSev: expose the SNP reserved pages through configuration
table
Michael Roth (3):
OvmfPkg/ResetVector: use SEV-SNP-validated CPUID values
OvmfPkg/VmgExitLib: use SEV-SNP-validated CPUID values
UefiCpuPkg/MpInitLib: use BSP to do extended topology check
Tom Lendacky (1):
UefiCpuPkg/MpInitLib: Use SEV-SNP AP Creation NAE event to launch APs
OvmfPkg/OvmfPkg.dec | 23 +
UefiCpuPkg/UefiCpuPkg.dec | 11 +
OvmfPkg/AmdSev/AmdSevX64.dsc | 5 +-
OvmfPkg/Bhyve/BhyveX64.dsc | 5 +-
OvmfPkg/OvmfPkgIa32.dsc | 1 +
OvmfPkg/OvmfPkgIa32X64.dsc | 6 +-
OvmfPkg/OvmfPkgX64.dsc | 5 +-
OvmfPkg/OvmfXen.dsc | 5 +-
OvmfPkg/OvmfPkgX64.fdf | 12 +-
OvmfPkg/AmdSevDxe/AmdSevDxe.inf | 7 +
.../DxeMemEncryptSevLib.inf | 3 +
.../PeiMemEncryptSevLib.inf | 7 +
.../SecMemEncryptSevLib.inf | 3 +
OvmfPkg/Library/VmgExitLib/SecVmgExitLib.inf | 2 +
OvmfPkg/Library/VmgExitLib/VmgExitLib.inf | 3 +
OvmfPkg/PlatformPei/PlatformPei.inf | 10 +
OvmfPkg/ResetVector/ResetVector.inf | 6 +
OvmfPkg/Sec/SecMain.inf | 4 +
UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf | 4 +
UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf | 4 +
MdePkg/Include/Register/Amd/Ghcb.h | 2 +-
.../Guid/ConfidentialComputingSecret.h | 18 +
OvmfPkg/Include/Library/MemEncryptSevLib.h | 26 +
.../X64/SnpPageStateChange.h | 31 ++
.../BaseMemEncryptSevLib/X64/VirtualMemory.h | 19 +
OvmfPkg/Sec/AmdSev.h | 95 ++++
UefiCpuPkg/Library/MpInitLib/MpLib.h | 20 +
OvmfPkg/AmdSevDxe/AmdSevDxe.c | 23 +
.../DxeMemEncryptSevLibInternal.c | 27 ++
.../Ia32/MemEncryptSevLib.c | 17 +
.../PeiMemEncryptSevLibInternal.c | 27 ++
.../SecMemEncryptSevLibInternal.c | 19 +
.../X64/DxeSnpSystemRamValidate.c | 40 ++
.../X64/PeiDxeVirtualMemory.c | 167 ++++++-
.../X64/PeiSnpSystemRamValidate.c | 126 +++++
.../X64/SecSnpSystemRamValidate.c | 36 ++
.../X64/SnpPageStateChangeInternal.c | 295 ++++++++++++
OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c | 444 ++++++++++++++++--
OvmfPkg/PlatformPei/AmdSev.c | 192 ++++++++
OvmfPkg/PlatformPei/MemDetect.c | 21 +
OvmfPkg/Sec/AmdSev.c | 267 +++++++++++
OvmfPkg/Sec/SecMain.c | 160 +------
UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 11 +-
.../MpInitLib/Ia32/SevSnpRmpAdjustInternal.c | 31 ++
UefiCpuPkg/Library/MpInitLib/MpLib.c | 286 ++++++++++-
.../MpInitLib/X64/SevSnpRmpAdjustInternal.c | 44 ++
OvmfPkg/FvmainCompactScratchEnd.fdf.inc | 5 +
OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm | 28 ++
OvmfPkg/ResetVector/Ia32/AmdSev.asm | 307 +++++++++++-
OvmfPkg/ResetVector/ResetVector.nasmb | 6 +
UefiCpuPkg/Library/MpInitLib/MpEqu.inc | 2 +
UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm | 78 +++
52 files changed, 2771 insertions(+), 225 deletions(-)
create mode 100644
OvmfPkg/Library/BaseMemEncryptSevLib/X64/SnpPageStateChange.h
create mode 100644 OvmfPkg/Sec/AmdSev.h
create mode 100644
OvmfPkg/Library/BaseMemEncryptSevLib/X64/DxeSnpSystemRamValidate.c
create mode 100644
OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiSnpSystemRamValidate.c
create mode 100644
OvmfPkg/Library/BaseMemEncryptSevLib/X64/SecSnpSystemRamValidate.c
create mode 100644
OvmfPkg/Library/BaseMemEncryptSevLib/X64/SnpPageStateChangeInternal.c
create mode 100644 OvmfPkg/Sec/AmdSev.c
create mode 100644
UefiCpuPkg/Library/MpInitLib/Ia32/SevSnpRmpAdjustInternal.c
create mode 100644
UefiCpuPkg/Library/MpInitLib/X64/SevSnpRmpAdjustInternal.c
--
2.17.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#80378): https://edk2.groups.io/g/devel/message/80378
Mute This Topic: https://groups.io/mt/85306653/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
