On 05/27/21 01:10, Brijesh Singh wrote:
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3275
>
> Extend the workarea to include the SEV-SNP enabled fields. This will be set
> when SEV-SNP is active in the guest VM.
>
> Cc: James Bottomley <j...@linux.ibm.com>
> Cc: Min Xu <min.m...@intel.com>
> Cc: Jiewen Yao <jiewen....@intel.com>
> Cc: Tom Lendacky <thomas.lenda...@amd.com>
> Cc: Jordan Justen <jordan.l.jus...@intel.com>
> Cc: Ard Biesheuvel <ardb+tianoc...@kernel.org>
> Cc: Laszlo Ersek <ler...@redhat.com>
> Cc: Erdem Aktas <erdemak...@google.com>
> Signed-off-by: Brijesh Singh <brijesh.si...@amd.com>
> ---
> OvmfPkg/PlatformPei/PlatformPei.inf | 1 +
> OvmfPkg/Include/Library/MemEncryptSevLib.h | 3 ++-
> OvmfPkg/PlatformPei/AmdSev.c | 26 ++++++++++++++++++++++
> OvmfPkg/ResetVector/Ia32/PageTables64.asm | 12 ++++++++++
> OvmfPkg/ResetVector/ResetVector.nasmb | 1 +
> 5 files changed, 42 insertions(+), 1 deletion(-)
(1) Please split this in two patches -- the PlatformPei changes should
be a separate patch. And, I think those should come second, the
ResetVector + header file change should come first.
>
> diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf
> b/OvmfPkg/PlatformPei/PlatformPei.inf
> index 6ef77ba7bb21..bc1dcac48343 100644
> --- a/OvmfPkg/PlatformPei/PlatformPei.inf
> +++ b/OvmfPkg/PlatformPei/PlatformPei.inf
> @@ -110,6 +110,7 @@ [Pcd]
> gUefiCpuPkgTokenSpaceGuid.PcdCpuBootLogicalProcessorNumber
> gUefiCpuPkgTokenSpaceGuid.PcdCpuApStackSize
> gUefiCpuPkgTokenSpaceGuid.PcdSevEsIsEnabled
> + gUefiCpuPkgTokenSpaceGuid.PcdSevSnpIsEnabled
>
> [FixedPcd]
> gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress
> diff --git a/OvmfPkg/Include/Library/MemEncryptSevLib.h
> b/OvmfPkg/Include/Library/MemEncryptSevLib.h
> index 2425d8ba0a36..24507de55c5d 100644
> --- a/OvmfPkg/Include/Library/MemEncryptSevLib.h
> +++ b/OvmfPkg/Include/Library/MemEncryptSevLib.h
> @@ -49,7 +49,8 @@ typedef struct {
> //
> typedef struct _SEC_SEV_ES_WORK_AREA {
> UINT8 SevEsEnabled;
> - UINT8 Reserved1[7];
> + UINT8 SevSnpEnabled;
> + UINT8 Reserved2[6];
>
> UINT64 RandomData;
>
> diff --git a/OvmfPkg/PlatformPei/AmdSev.c b/OvmfPkg/PlatformPei/AmdSev.c
> index a8bf610022ba..67b78fd5fa36 100644
> --- a/OvmfPkg/PlatformPei/AmdSev.c
> +++ b/OvmfPkg/PlatformPei/AmdSev.c
> @@ -22,6 +22,27 @@
>
> #include "Platform.h"
>
> +/**
> +
> + Initialize SEV-SNP support if running as an SEV-SNP guest.
> +
> + **/
> +STATIC
> +VOID
> +AmdSevSnpInitialize (
> + VOID
> + )
> +{
> + RETURN_STATUS PcdStatus;
> +
> + if (!MemEncryptSevSnpIsEnabled ()) {
> + return;
> + }
> +
> + PcdStatus = PcdSetBoolS (PcdSevSnpIsEnabled, TRUE);
> + ASSERT_RETURN_ERROR (PcdStatus);
> +}
> +
> /**
>
> Initialize SEV-ES support if running as an SEV-ES guest.
> @@ -209,4 +230,9 @@ AmdSevInitialize (
> // Check and perform SEV-ES initialization if required.
> //
> AmdSevEsInitialize ();
> +
> + //
> + // Check and perform SEV-SNP initialization if required.
> + //
> + AmdSevSnpInitialize ();
> }
> diff --git a/OvmfPkg/ResetVector/Ia32/PageTables64.asm
> b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
> index 5fae8986d9da..6838cdeec9c3 100644
> --- a/OvmfPkg/ResetVector/Ia32/PageTables64.asm
> +++ b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
> @@ -81,6 +81,11 @@ CheckSevFeatures:
> ; the MSR check below will set the first byte of the workarea to one.
> mov byte[SEV_ES_WORK_AREA], 0
>
> + ; Set the SevSnpEnabled field in workarea to zero to communicate to the
> SEC
> + ; phase that SEV-SNP is not enabled. If SEV-SNP is enabled, this function
> + ; will set it to 1.
> + mov byte[SEV_ES_WORK_AREA_SNP], 0
> +
> ;
> ; Set up exception handlers to check for SEV-ES
> ; Load temporary RAM stack based on PCDs (see SevEsIdtVmmComm for
> @@ -136,6 +141,13 @@ CheckSevFeatures:
> ; phase that SEV-ES is enabled.
> mov byte[SEV_ES_WORK_AREA], 1
>
> + bt eax, 2
> + jnc GetSevEncBit
> +
> + ; Set the second byte of the workarea to one to communicate to the SEC
> + ; phase that the SEV-SNP is enabled
> + mov byte[SEV_ES_WORK_AREA_SNP], 1
> +
> GetSevEncBit:
> ; Get pte bit position to enable memory encryption
> ; CPUID Fn8000_001F[EBX] - Bits 5:0
(2) Please mention in the commit message (of the ResetVector patch),
and/or a comment here in the code, that SEV-SNP is never enabled if
SEV-ES is disabled.
Section "15.34.10 SEV_STATUS MSR" in the APM (doc#24593 v3.37) does not
spell out this dependency.
Furthermore, the mSevStatus / mSevEsStatus / mSevSnpStatus variable
assignments in patch#2 do not form a "dependency cascade" like the one
seen here in the reset vector code.
While "SEV-ES depends on SEV" seems obvious to me (and so the related,
existent jumps in the assembly code are not surprising), the statement
"SEV-SNP depends on SEV-ES" is not *that* obvious to me. Thus a comment
would be welcome.
For *both* patches split out of this one:
Reviewed-by: Laszlo Ersek <ler...@redhat.com>
Thanks
Laszlo
> diff --git a/OvmfPkg/ResetVector/ResetVector.nasmb
> b/OvmfPkg/ResetVector/ResetVector.nasmb
> index 5fbacaed5f9d..1971557b1c00 100644
> --- a/OvmfPkg/ResetVector/ResetVector.nasmb
> +++ b/OvmfPkg/ResetVector/ResetVector.nasmb
> @@ -73,6 +73,7 @@
> %define GHCB_BASE (FixedPcdGet32 (PcdOvmfSecGhcbBase))
> %define GHCB_SIZE (FixedPcdGet32 (PcdOvmfSecGhcbSize))
> %define SEV_ES_WORK_AREA (FixedPcdGet32 (PcdSevEsWorkAreaBase))
> + %define SEV_ES_WORK_AREA_SNP (FixedPcdGet32 (PcdSevEsWorkAreaBase) + 1)
> %define SEV_ES_WORK_AREA_RDRAND (FixedPcdGet32 (PcdSevEsWorkAreaBase) + 8)
> %define SEV_ES_WORK_AREA_ENC_MASK (FixedPcdGet32 (PcdSevEsWorkAreaBase) +
> 16)
> %define SEV_ES_VC_TOP_OF_STACK (FixedPcdGet32 (PcdOvmfSecPeiTempRamBase) +
> FixedPcdGet32 (PcdOvmfSecPeiTempRamSize))
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#76077): https://edk2.groups.io/g/devel/message/76077
Mute This Topic: https://groups.io/mt/83113764/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
