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 (#80076): https://edk2.groups.io/g/devel/message/80076
Mute This Topic: https://groups.io/mt/85306653/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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.yao@intel.com>

0002-OvmfPkg-reserve-CPUID-page-for-SEV-SNP
Reviewed-by: Jiewen Yao <Jiewen.yao@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.yao@intel.com>

0004-OvmfPkg-ResetVector-invalidate-the-GHCB-page
Acked-by: Jiewen Yao <Jiewen.yao@intel.com>

0005-OvmfPkg-ResetVector-check-the-vmpl-level
Acked-by: Jiewen Yao <Jiewen.yao@intel.com>

0006-OvmfPkg-ResetVector-pre-validate-the-data-pages-used-in-SEC-phase
Acked-by: Jiewen Yao <Jiewen.yao@intel.com>

0007-OvmfPkg-ResetVector-use-SEV-SNP-validated-CPUID-values
Acked-by: Jiewen Yao <Jiewen.yao@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.yao@intel.com>

0010-OvmfPkg-SecMain-move-SEV-specific-routines-in-AmdSev.c
Reviewed-by: Jiewen Yao <Jiewen.yao@intel.com>

0011-OvmfPkg-SecMain-register-GHCB-gpa-for-the-SEV-SNP-guest
Acked-by: Jiewen Yao <Jiewen.yao@intel.com>

0012-OvmfPkg-VmgExitLib-use-SEV-SNP-validated-CPUID-values
Acked-by: Jiewen Yao <Jiewen.yao@intel.com>

0013-OvmfPkg-PlatformPei-register-GHCB-gpa-for-the-SEV-SNP-guest
Acked-by: Jiewen Yao <Jiewen.yao@intel.com>

0014-OvmfPkg-AmdSevDxe-do-not-use-extended-PCI-config-space
Acked-by: Jiewen Yao <Jiewen.yao@intel.com>

0015-OvmfPkg-MemEncryptSevLib-add-support-to-validate-system-RAM
Acked-by: Jiewen Yao <Jiewen.yao@intel.com>

0016-OvmfPkg-BaseMemEncryptSevLib-skip-the-pre-validated-system-RAM
Acked-by: Jiewen Yao <Jiewen.yao@intel.com>

0017-OvmfPkg-MemEncryptSevLib-add-support-to-validate-4GB-memory-in-PEI-phase
Acked-by: Jiewen Yao <Jiewen.yao@intel.com>

0018-OvmfPkg-SecMain-pre-validate-the-memory-used-for-decompressing-Fv
Acked-by: Jiewen Yao <Jiewen.yao@intel.com>

0019-OvmfPkg-PlatformPei-validate-the-system-RAM-when-SNP-is-active
Acked-by: Jiewen Yao <Jiewen.yao@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.yao@intel.com>

0022-MdePkg-GHCB-increase-the-GHCB-protocol-max-version
Acked-by: Jiewen Yao <Jiewen.yao@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.yao@intel.com>

0026-OvmfPkg-MemEncryptSevLib-skip-page-state-change-for-Mmio-address
Acked-by: Jiewen Yao <Jiewen.yao@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.singh@amd.com>
> Sent: Thursday, September 2, 2021 12:16 AM
> To: devel@edk2.groups.io
> Cc: James Bottomley <jejb@linux.ibm.com>; Xu, Min M <min.m.xu@intel.com>;
> Yao, Jiewen <jiewen.yao@intel.com>; Tom Lendacky
> <thomas.lendacky@amd.com>; Justen, Jordan L <jordan.l.justen@intel.com>;
> Ard Biesheuvel <ardb+tianocore@kernel.org>; Erdem Aktas
> <erdemaktas@google.com>; Michael Roth <Michael.Roth@amd.com>; Gerd
> Hoffmann <kraxel@redhat.com>; Brijesh Singh <brijesh.singh@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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On September 7, 2021 10:37 AM, Jiewen Yao wrote:
> 
> 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, ......
> 
We have CONFIDENTIAL_COMPUTING_WORK_AREA_HEADER definition in OvmfPkg\Include\WorkArea.h like below:
typedef struct _CONFIDENTIAL_COMPUTING_WORK_AREA_HEADER {
  UINT8                   GuestType;   // 0 - legacy guest, 1 - SEV guest, 2 - tdx guest
  UINT8                   Reserved1[3];
} CONFIDENTIAL_COMPUTING_WORK_AREA_HEADER;

Can we define the PcdConfidentialComputingCategory like below:
  ## This dynamic PCD indicates the Confidential Computing Category
  #  [7:0]     Confidential Computing Category  (0 - Non-Cc, 1 - AmdSev, 2 - IntelTdx)
  #  [15:8]   Sub-Category (defined by each vendor, SEV-ES, SEV-SNP, or TDX-1.0, TDX-2.0, etc)
  #  [31:16] Reserved
  # @Prompt Confidential Computing Category
  gUefiCpuPkgTokenSpaceGuid.PcdConfidentialComputingCategory|0|UINT32|0x60000018

Thanks!
Min


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#80318): https://edk2.groups.io/g/devel/message/80318
Mute This Topic: https://groups.io/mt/85306653/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Yes, that is good idea.

> -----Original Message-----
> From: Xu, Min M <min.m.xu@intel.com>
> Sent: Wednesday, September 8, 2021 10:30 AM
> To: Yao, Jiewen <jiewen.yao@intel.com>; Brijesh Singh
> <brijesh.singh@amd.com>; devel@edk2.groups.io
> Cc: James Bottomley <jejb@linux.ibm.com>; Tom Lendacky
> <thomas.lendacky@amd.com>; Justen, Jordan L <jordan.l.justen@intel.com>;
> Ard Biesheuvel <ardb+tianocore@kernel.org>; Erdem Aktas
> <erdemaktas@google.com>; Michael Roth <Michael.Roth@amd.com>; Gerd
> Hoffmann <kraxel@redhat.com>
> Subject: RE: [PATCH v6 00/29] Add AMD Secure Nested Paging (SEV-SNP)
> support
> 
> On September 7, 2021 10:37 AM, Jiewen Yao wrote:
> >
> > 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, ......
> >
> We have CONFIDENTIAL_COMPUTING_WORK_AREA_HEADER definition in
> OvmfPkg\Include\WorkArea.h like below:
> typedef struct _CONFIDENTIAL_COMPUTING_WORK_AREA_HEADER {
>   UINT8                   GuestType;   // 0 - legacy guest, 1 - SEV guest, 2 - tdx guest
>   UINT8                   Reserved1[3];
> } CONFIDENTIAL_COMPUTING_WORK_AREA_HEADER;
> 
> Can we define the PcdConfidentialComputingCategory like below:
>   ## This dynamic PCD indicates the Confidential Computing Category
>   #  [7:0]     Confidential Computing Category  (0 - Non-Cc, 1 - AmdSev, 2 -
> IntelTdx)
>   #  [15:8]   Sub-Category (defined by each vendor, SEV-ES, SEV-SNP, or TDX-1.0,
> TDX-2.0, etc)
>   #  [31:16] Reserved
>   # @Prompt Confidential Computing Category
> 
> gUefiCpuPkgTokenSpaceGuid.PcdConfidentialComputingCategory|0|UINT32|0x
> 60000018
> 
> Thanks!
> Min


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#80320): https://edk2.groups.io/g/devel/message/80320
Mute This Topic: https://groups.io/mt/85306653/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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.yao@intel.com>
> 
> 0002-OvmfPkg-reserve-CPUID-page-for-SEV-SNP
> Reviewed-by: Jiewen Yao <Jiewen.yao@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.yao@intel.com>
> 
> 0004-OvmfPkg-ResetVector-invalidate-the-GHCB-page
> Acked-by: Jiewen Yao <Jiewen.yao@intel.com>
> 
> 0005-OvmfPkg-ResetVector-check-the-vmpl-level
> Acked-by: Jiewen Yao <Jiewen.yao@intel.com>
> 
> 0006-OvmfPkg-ResetVector-pre-validate-the-data-pages-used-in-SEC-phase
> Acked-by: Jiewen Yao <Jiewen.yao@intel.com>
> 
> 0007-OvmfPkg-ResetVector-use-SEV-SNP-validated-CPUID-values
> Acked-by: Jiewen Yao <Jiewen.yao@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.yao@intel.com>
> 
> 0010-OvmfPkg-SecMain-move-SEV-specific-routines-in-AmdSev.c
> Reviewed-by: Jiewen Yao <Jiewen.yao@intel.com>
> 
> 0011-OvmfPkg-SecMain-register-GHCB-gpa-for-the-SEV-SNP-guest
> Acked-by: Jiewen Yao <Jiewen.yao@intel.com>
> 
> 0012-OvmfPkg-VmgExitLib-use-SEV-SNP-validated-CPUID-values
> Acked-by: Jiewen Yao <Jiewen.yao@intel.com>
> 
> 0013-OvmfPkg-PlatformPei-register-GHCB-gpa-for-the-SEV-SNP-guest
> Acked-by: Jiewen Yao <Jiewen.yao@intel.com>
> 
> 0014-OvmfPkg-AmdSevDxe-do-not-use-extended-PCI-config-space
> Acked-by: Jiewen Yao <Jiewen.yao@intel.com>
> 
> 0015-OvmfPkg-MemEncryptSevLib-add-support-to-validate-system-RAM
> Acked-by: Jiewen Yao <Jiewen.yao@intel.com>
> 
> 0016-OvmfPkg-BaseMemEncryptSevLib-skip-the-pre-validated-system-RAM
> Acked-by: Jiewen Yao <Jiewen.yao@intel.com>
> 
> 0017-OvmfPkg-MemEncryptSevLib-add-support-to-validate-4GB-memory-in-PEI-phase
> Acked-by: Jiewen Yao <Jiewen.yao@intel.com>
> 
> 0018-OvmfPkg-SecMain-pre-validate-the-memory-used-for-decompressing-Fv
> Acked-by: Jiewen Yao <Jiewen.yao@intel.com>
> 
> 0019-OvmfPkg-PlatformPei-validate-the-system-RAM-when-SNP-is-active
> Acked-by: Jiewen Yao <Jiewen.yao@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.yao@intel.com>
> 
> 0022-MdePkg-GHCB-increase-the-GHCB-protocol-max-version
> Acked-by: Jiewen Yao <Jiewen.yao@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.yao@intel.com>
> 
> 0026-OvmfPkg-MemEncryptSevLib-skip-page-state-change-for-Mmio-address
> Acked-by: Jiewen Yao <Jiewen.yao@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.singh@amd.com>
>> Sent: Thursday, September 2, 2021 12:16 AM
>> To: devel@edk2.groups.io
>> Cc: James Bottomley <jejb@linux.ibm.com>; Xu, Min M <min.m.xu@intel.com>;
>> Yao, Jiewen <jiewen.yao@intel.com>; Tom Lendacky
>> <thomas.lendacky@amd.com>; Justen, Jordan L <jordan.l.justen@intel.com>;
>> Ard Biesheuvel <ardb+tianocore@kernel.org>; Erdem Aktas
>> <erdemaktas@google.com>; Michael Roth <Michael.Roth@amd.com>; Gerd
>> Hoffmann <kraxel@redhat.com>; Brijesh Singh <brijesh.singh@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&amp;data=04%7C01%7Cbrijesh.singh%40amd.com%7C33df27781053475362e208d971a85cee%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637665791405981353%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=8vfBxVawRoEeDCR0DHJhfhTgPr66704twMGZ8%2BY%2BLGI%3D&amp;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-&amp;data=04%7C01%7Cbrijesh.singh%40amd.com%7C33df27781053475362e208d971a85cee%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637665791405981353%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=tobk2zHk1ziA6nZ9bvwNrohRuIN7bTEh5ZXFNzwTTX0%3D&amp;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&amp;data=04%7C01%7Cbrijesh.singh%40amd.com%7C33df27781053475362e208d971a85cee%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637665791405981353%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=%2FDTzbh8F6CtvvC263r7xJGX6WAQ8yCAuKLkPM7GwBvQ%3D&amp;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&amp;data=04%7C01%7Cbrijesh.singh%40amd.com%7C33df27781053475362e208d971a85cee%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637665791405981353%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=6lvmuqOQbvNoXG50qK5QGYG6XEdojJ%2BHlkKrODZRAHY%3D&amp;reserved=0
>>
>> GHCB spec:
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdeveloper.amd.com%2Fwp-content%2Fresources%2F56421.pdf&amp;data=04%7C01%7Cbrijesh.singh%40amd.com%7C33df27781053475362e208d971a85cee%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637665791405981353%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=Q1oa5gB3CthKPkentzFJE3B3LfBpZq%2B4y8EzPTlPzl8%3D&amp;reserved=0
>>
>> SEV-SNP firmware specification:
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.amd.com%2Fsystem%2Ffiles%2FTechDocs%2F56860.pdf&amp;data=04%7C01%7Cbrijesh.singh%40amd.com%7C33df27781053475362e208d971a85cee%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637665791405981353%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=7cLoMR52WAvMe%2Fr4rKGGYx2wadopvXKnSGi%2FghyEdJA%3D&amp;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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On September 9, 2021 3:46 AM, Brijesh Singh wrote:
> 
> 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).
> 
Hi, Brijesh
if you are considering to introduce a new PcdConfidentialComputingCategory
as Jiewen suggested below:
> >
> > 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, ......
> >
I also have some suggestions.

As we have below definition in WorkArea.h
  typedef struct _CONFIDENTIAL_COMPUTING_WORK_AREA_HEADER {
    UINT8                   GuestType;
    UINT8                   Reserved1[3];
  } CONFIDENTIAL_COMPUTING_WORK_AREA_HEADER;

Can we update above CONFIDENTIAL_COMPUTING_WORK_AREA_HEADER to below:
  typedef struct _CONFIDENTIAL_COMPUTING_WORK_AREA_HEADER {
    UINT8                   GuestType;
    UINT8                   SubType;               // subtype which indicates SEV-ES, SEV-NP, or TDX 1.0, TDX 2.0 etc.
    UINT8                   Reserved1[2];
  } CONFIDENTIAL_COMPUTING_WORK_AREA_HEADER;

The PcdConfidentialComputingCategory can be defined as UINT32, like below:
  ## This dynamic PCD indicates the Confidential Computing Category
  #  [7:0]   Confidential Computing Category  (0 - Non-Cc, 1 - AmdSev, 2 - IntelTdx)
  #  [15:8]  Sub-Category (defined by each vendor, SEV-ES, SEV-SNP, or TDX-1.0, TDX-2.0, etc)
  #  [31:16] Reserved
  # @Prompt Confidential Computing Category
  gUefiCpuPkgTokenSpaceGuid.PcdConfidentialComputingCategory|0|UINT32|0x60000018

So that we simply copy the CONFIDENTIAL_COMPUTING_WORK_AREA_HEADER to PcdConfidentialComputingCategory.
What's your thought?

Thanks!
Min


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#80390): https://edk2.groups.io/g/devel/message/80390
Mute This Topic: https://groups.io/mt/85306653/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Hi Min,

On 9/8/21 7:31 PM, Xu, Min M wrote:
> On September 9, 2021 3:46 AM, Brijesh Singh wrote:
>> 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).
>>
> Hi, Brijesh
> if you are considering to introduce a new PcdConfidentialComputingCategory
> as Jiewen suggested below:
>>> 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, ......
> I also have some suggestions.
>
> As we have below definition in WorkArea.h
>   typedef struct _CONFIDENTIAL_COMPUTING_WORK_AREA_HEADER {
>     UINT8                   GuestType;
>     UINT8                   Reserved1[3];
>   } CONFIDENTIAL_COMPUTING_WORK_AREA_HEADER;
>
> Can we update above CONFIDENTIAL_COMPUTING_WORK_AREA_HEADER to below:
>   typedef struct _CONFIDENTIAL_COMPUTING_WORK_AREA_HEADER {
>     UINT8                   GuestType;
>     UINT8                   SubType;               // subtype which indicates SEV-ES, SEV-NP, or TDX 1.0, TDX 2.0 etc.
>     UINT8                   Reserved1[2];
>   } CONFIDENTIAL_COMPUTING_WORK_AREA_HEADER;
>
> The PcdConfidentialComputingCategory can be defined as UINT32, like below:
>   ## This dynamic PCD indicates the Confidential Computing Category
>   #  [7:0]   Confidential Computing Category  (0 - Non-Cc, 1 - AmdSev, 2 - IntelTdx)
>   #  [15:8]  Sub-Category (defined by each vendor, SEV-ES, SEV-SNP, or TDX-1.0, TDX-2.0, etc)
>   #  [31:16] Reserved
>   # @Prompt Confidential Computing Category
>   gUefiCpuPkgTokenSpaceGuid.PcdConfidentialComputingCategory|0|UINT32|0x60000018
>
> So that we simply copy the CONFIDENTIAL_COMPUTING_WORK_AREA_HEADER to PcdConfidentialComputingCategory.
> What's your thought?

I am not sure if its a good idea to pack a header like above in a 32-bit
PCD. The caller need to unpack the 32-bit number and perform a bitshit
etc. Additionally we also need to check for reserved bits being set to
zero etc. I am more inclined toward something like this:

enum {

   /* The guest is running with memory encryption disabled. */

    CC_ATTR_UNDEF,

  /* The guest is running with active AMD SEV memory encryption. */

   CC_ATTR_AMD_SEV,

  /* The guest is running with active AMD SEV-ES memory encryption. */

  CC_ATTR_AMD_SEV_ES,

  /* The guest is running with active AMD SEV-SNP memory encryption. */

  CC_ATTR_AMD_SEV_SNP,

 /* The guest is running with active Intel TDX memory encryption. */

 CC_ATTR_INTEL_TDX,

 /* The guest is running with active Intel SGX memory encryption. */

 CC_ATTR_INTEL_SGX,

} ConfidentialComputingAttr;

The PcdConfidentialComputingAttr will be set to zero. The OVMF will set
this dynamic PCD in PEI phase. The UefiCpuPkg will provide a new
function that can be used by anyone to check the CC guest type.

BOOLEAN

CcPlatformHas(enum ConfidentialComputingAttr attr)

{

    UINT32 Val;

    Val = PcdGet32 (PcdConfidentialComputingAttr);

    return val == attr;

}


> Thanks!
> Min


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#80426): https://edk2.groups.io/g/devel/message/80426
Mute This Topic: https://groups.io/mt/85306653/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

I do not see any conflict here.

You can use below definition
 enum {
   CC_ATTR_AMD_SEV = 0x0001,
   CC_ATTR_AMD_SEV_ES =  0x0101,
   CC_ATTR_AMD_SEV_SNP =  0x0201,
   CC_ATTR_INTEL_TDX =  0x0002,
 } ConfidentialComputingAttr;

BTW: Please remove SGX, we don’t need it here.

Thank you
Yao Jiewen

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Brijesh
> Singh via groups.io
> Sent: Thursday, September 9, 2021 6:51 PM
> To: Xu, Min M <min.m.xu@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>;
> devel@edk2.groups.io
> Cc: James Bottomley <jejb@linux.ibm.com>; Tom Lendacky
> <thomas.lendacky@amd.com>; Justen, Jordan L <jordan.l.justen@intel.com>;
> Ard Biesheuvel <ardb+tianocore@kernel.org>; Erdem Aktas
> <erdemaktas@google.com>; Michael Roth <Michael.Roth@amd.com>; Gerd
> Hoffmann <kraxel@redhat.com>
> Subject: Re: [edk2-devel] [PATCH v6 00/29] Add AMD Secure Nested Paging
> (SEV-SNP) support
> 
> Hi Min,
> 
> On 9/8/21 7:31 PM, Xu, Min M wrote:
> > On September 9, 2021 3:46 AM, Brijesh Singh wrote:
> >> 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).
> >>
> > Hi, Brijesh
> > if you are considering to introduce a new PcdConfidentialComputingCategory
> > as Jiewen suggested below:
> >>> 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, ......
> > I also have some suggestions.
> >
> > As we have below definition in WorkArea.h
> >   typedef struct _CONFIDENTIAL_COMPUTING_WORK_AREA_HEADER {
> >     UINT8                   GuestType;
> >     UINT8                   Reserved1[3];
> >   } CONFIDENTIAL_COMPUTING_WORK_AREA_HEADER;
> >
> > Can we update above CONFIDENTIAL_COMPUTING_WORK_AREA_HEADER to
> below:
> >   typedef struct _CONFIDENTIAL_COMPUTING_WORK_AREA_HEADER {
> >     UINT8                   GuestType;
> >     UINT8                   SubType;               // subtype which indicates SEV-ES, SEV-NP,
> or TDX 1.0, TDX 2.0 etc.
> >     UINT8                   Reserved1[2];
> >   } CONFIDENTIAL_COMPUTING_WORK_AREA_HEADER;
> >
> > The PcdConfidentialComputingCategory can be defined as UINT32, like below:
> >   ## This dynamic PCD indicates the Confidential Computing Category
> >   #  [7:0]   Confidential Computing Category  (0 - Non-Cc, 1 - AmdSev, 2 -
> IntelTdx)
> >   #  [15:8]  Sub-Category (defined by each vendor, SEV-ES, SEV-SNP, or TDX-1.0,
> TDX-2.0, etc)
> >   #  [31:16] Reserved
> >   # @Prompt Confidential Computing Category
> >
> gUefiCpuPkgTokenSpaceGuid.PcdConfidentialComputingCategory|0|UINT32|0x
> 60000018
> >
> > So that we simply copy the
> CONFIDENTIAL_COMPUTING_WORK_AREA_HEADER to
> PcdConfidentialComputingCategory.
> > What's your thought?
> 
> I am not sure if its a good idea to pack a header like above in a 32-bit
> PCD. The caller need to unpack the 32-bit number and perform a bitshit
> etc. Additionally we also need to check for reserved bits being set to
> zero etc. I am more inclined toward something like this:
> 
> enum {
> 
>    /* The guest is running with memory encryption disabled. */
> 
>     CC_ATTR_UNDEF,
> 
>   /* The guest is running with active AMD SEV memory encryption. */
> 
>    CC_ATTR_AMD_SEV,
> 
>   /* The guest is running with active AMD SEV-ES memory encryption. */
> 
>   CC_ATTR_AMD_SEV_ES,
> 
>   /* The guest is running with active AMD SEV-SNP memory encryption. */
> 
>   CC_ATTR_AMD_SEV_SNP,
> 
>  /* The guest is running with active Intel TDX memory encryption. */
> 
>  CC_ATTR_INTEL_TDX,
> 
>  /* The guest is running with active Intel SGX memory encryption. */
> 
>  CC_ATTR_INTEL_SGX,
> 
> } ConfidentialComputingAttr;
> 
> The PcdConfidentialComputingAttr will be set to zero. The OVMF will set
> this dynamic PCD in PEI phase. The UefiCpuPkg will provide a new
> function that can be used by anyone to check the CC guest type.
> 
> BOOLEAN
> 
> CcPlatformHas(enum ConfidentialComputingAttr attr)
> 
> {
> 
>     UINT32 Val;
> 
>     Val = PcdGet32 (PcdConfidentialComputingAttr);
> 
>     return val == attr;
> 
> }
> 
> 
> > Thanks!
> > Min
> 
> 
> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#80433): https://edk2.groups.io/g/devel/message/80433
Mute This Topic: https://groups.io/mt/85306653/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

  Hi,

> I am not sure if its a good idea to pack a header like above in a 32-bit
> PCD. The caller need to unpack the 32-bit number and perform a bitshit
> etc. Additionally we also need to check for reserved bits being set to
> zero etc. I am more inclined toward something like this:
> 
> enum {

Well, various places probably just need to know whenever they should
call into the sev or the tdx library, so grouping stuff makes sense to
me.  We don't need bitfields for that though, could also be done this
way:

enum {
   NOT_ENCRYPTED = 0,
   AMD_SEV = 0x100,
   AMD_SEV_ES,
   [ ... ]
   INTEL_TDX = 0x200,
   [ ... ]
}

So if you need the exact mode you can compare values as-is, if you want
figure which vendor library should be called you'll just mask out the
least significant 8 bits.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#80429): https://edk2.groups.io/g/devel/message/80429
Mute This Topic: https://groups.io/mt/85306653/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On 9/9/21 6:22 AM, Gerd Hoffmann wrote:
>   Hi,
>
>> I am not sure if its a good idea to pack a header like above in a 32-bit
>> PCD. The caller need to unpack the 32-bit number and perform a bitshit
>> etc. Additionally we also need to check for reserved bits being set to
>> zero etc. I am more inclined toward something like this:
>>
>> enum {
> Well, various places probably just need to know whenever they should
> call into the sev or the tdx library, so grouping stuff makes sense to
> me.  We don't need bitfields for that though, could also be done this
> way:
>
> enum {
>    NOT_ENCRYPTED = 0,
>    AMD_SEV = 0x100,
>    AMD_SEV_ES,
>    [ ... ]
>    INTEL_TDX = 0x200,
>    [ ... ]
> }
>
> So if you need the exact mode you can compare values as-is, if you want
> figure which vendor library should be called you'll just mask out the
> least significant 8 bits.

Yes, this also works fine.

thanks




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#80431): https://edk2.groups.io/g/devel/message/80431
Mute This Topic: https://groups.io/mt/85306653/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On September 9, 2021 7:40 PM, Brijesh Singh wrote:
> On 9/9/21 6:22 AM, Gerd Hoffmann wrote:
> >   Hi,
> >
> >> I am not sure if its a good idea to pack a header like above in a
> >> 32-bit PCD. The caller need to unpack the 32-bit number and perform a
> >> bitshit etc. Additionally we also need to check for reserved bits
> >> being set to zero etc. I am more inclined toward something like this:
> >>
> >> enum {
> > Well, various places probably just need to know whenever they should
> > call into the sev or the tdx library, so grouping stuff makes sense to
> > me.  We don't need bitfields for that though, could also be done this
> > way:
> >
> > enum {
> >    NOT_ENCRYPTED = 0,
> >    AMD_SEV = 0x100,
> >    AMD_SEV_ES,
> >    [ ... ]
> >    INTEL_TDX = 0x200,
> >    [ ... ]
> > }
> >
> > So if you need the exact mode you can compare values as-is, if you
> > want figure which vendor library should be called you'll just mask out
> > the least significant 8 bits.
> 
> Yes, this also works fine.
Agree. So let's follow this way.

Thanks!
Min


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#80432): https://edk2.groups.io/g/devel/message/80432
Mute This Topic: https://groups.io/mt/85306653/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Hi Yao,

I am going through implementing your feedback. I have covered most of
it. But your comment on moving some of the changes from MpFunc.nasm to
Sev.nasm may make code harder to read. It is mainly because the GPA
registration and Topo check are not self-contained routines. They depend
on some previous register states and need to jump to different paths
based on the condition. All those jump are in generic AP init routines.

As you can see, the changes in MpFunc.nasm are more petite, and if it's
a big issue, we can revisit it later and devise a proposal to move the
big chunk of generic code in a separate file then have TDX and SEV call
into it. At that time, we can add OneTimeCall or OneTimeReturn macros
etc., to jump between different files to make it more readable. Would
you please let me know if that is acceptable?

-Brijesh

On 9/6/21 9:36 PM, Yao, Jiewen wrote:
>
> 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
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#80550): https://edk2.groups.io/g/devel/message/80550
Mute This Topic: https://groups.io/mt/85306653/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Hi Brijesh
I think it is OK to leave MpFunc.nasm in this series.
We can revisit later.

Thank you
Yao Jiewen

> -----Original Message-----
> From: Brijesh Singh <brijesh.singh@amd.com>
> Sent: Monday, September 13, 2021 6:56 AM
> To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io
> Cc: James Bottomley <jejb@linux.ibm.com>; Xu, Min M <min.m.xu@intel.com>;
> Tom Lendacky <thomas.lendacky@amd.com>; Justen, Jordan L
> <jordan.l.justen@intel.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>;
> Erdem Aktas <erdemaktas@google.com>; Michael Roth
> <Michael.Roth@amd.com>; Gerd Hoffmann <kraxel@redhat.com>
> Subject: Re: [PATCH v6 00/29] Add AMD Secure Nested Paging (SEV-SNP)
> support
> 
> Hi Yao,
> 
> I am going through implementing your feedback. I have covered most of
> it. But your comment on moving some of the changes from MpFunc.nasm to
> Sev.nasm may make code harder to read. It is mainly because the GPA
> registration and Topo check are not self-contained routines. They depend
> on some previous register states and need to jump to different paths
> based on the condition. All those jump are in generic AP init routines.
> 
> As you can see, the changes in MpFunc.nasm are more petite, and if it's
> a big issue, we can revisit it later and devise a proposal to move the
> big chunk of generic code in a separate file then have TDX and SEV call
> into it. At that time, we can add OneTimeCall or OneTimeReturn macros
> etc., to jump between different files to make it more readable. Would
> you please let me know if that is acceptable?
> 
> -Brijesh
> 
> On 9/6/21 9:36 PM, Yao, Jiewen wrote:
> >
> > 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
> >


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#80551): https://edk2.groups.io/g/devel/message/80551
Mute This Topic: https://groups.io/mt/85306653/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-