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://bugzilla.tianocore.org/show_bug.cgi?id=3275
>
> 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://www.amd.com/system/files/TechDocs/SEV-SNP-strengthening-vm-
> isolation-with-integrity-protection-and-more.pdf
>
> APM 2: https://www.amd.com/system/files/TechDocs/24593.pdf (section 15.36)
>
> The complete source is available at
> https://github.com/AMDESE/ovmf/tree/sev-snp-rfc-5
>
> GHCB spec:
> https://developer.amd.com/wp-content/resources/56421.pdf
>
> SEV-SNP firmware specification:
> https://www.amd.com/system/files/TechDocs/56860.pdf
>
> 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 (#80281): https://edk2.groups.io/g/devel/message/80281
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]
-=-=-=-=-=-=-=-=-=-=-=-
