On 06/04/21 16:15, Laszlo Ersek wrote:
> 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>
(3) Actually, no.
This patch should be reduced to the following files only:
- OvmfPkg/PlatformPei/AmdSev.c
- OvmfPkg/PlatformPei/PlatformPei.inf
and the following changes should be dropped completely:
- OvmfPkg/Include/Library/MemEncryptSevLib.h
- OvmfPkg/ResetVector/Ia32/PageTables64.asm
- OvmfPkg/ResetVector/ResetVector.nasmb
Specifically, the "SEC_SEV_ES_WORK_AREA.SevSnpEnabled" field should
never be introduced.
The reason is apparent only from patch #10 -- "OvmfPkg/PlatformPei:
register GHCB gpa for the SEV-SNP guest".
The core idea is that in patch#10, in the SEC module, you can implement
SevSnpIsEnabled() by just reading MSR_SEV_STATUS, and checking the SNP
bit. Namely, while the SevSnpIsEnabled() call is made in
SevEsProtocolCheck(), i.e., before exception handling is set up in the
SEC module -- and so you indeed cannot call CPUID --, you don't *have*
to call CPUID at that call site. Where you call SevSnpIsEnabled() in
SevEsProtocolCheck(), you already know that SEV-ES is enabled, so it's
safe to just read the exact same SEV status MSR that the SEV-ES status
comes from in the first place, without any CPUID safety check.
(
General request: please be explicit in the commit messages about the
data flow between modules, and why you are doing what you are doing.
Arriving at the above analysis took me 3+ hours this morning, when --
while reviewing patch#4 -- I took issue with the proliferation of the
new fields in SEC_SEV_ES_WORK_AREA. SEC_SEV_ES_WORK_AREA is *not* a
convenience dump. We should restrict its use as much as possible.
I double-checked how SEC_SEV_ES_WORK_AREA had evolved historically:
* SEC_SEV_ES_WORK_AREA.SevEsEnabled:
1 43c3df78460d OvmfPkg: Reserve a page in memory for the SEV-ES usage
2 0731236fc108 OvmfPkg/PlatformPei: Reserve SEV-ES work area if S3 is
supported
3 8a2732186a53 OvmfPkg/ResetVector: Add support for a 32-bit SEV check
4 13e5492bfdf3 OvmfPkg/Sec: Add #VC exception handling for Sec phase
The "SEC_SEV_ES_WORK_AREA.SevEsEnabled" field is important for the
following reason:
- in an SEV-ES guest, just learning about SEV requires exception
handling; thus, the Reset Vector sets up exception handling
*unconditionally*,
- in SEC, we deal with exception handling regardless of SEV-ES, but
*how* we do that is conditional on SEV-ES.
This means that caching the SEV-ES presence from the Reset Vector to
SEC makes a lot of sense. Given that in the Reset Vector we have
unconditional exception handling, and then in SEC we have a cached
result, we are allowed to only have conditional exception handling in
SEC.
* SEC_SEV_ES_WORK_AREA.RandomData, SEC_SEV_ES_WORK_AREA.EncryptionMask:
1 7cb96c47a94e OvmfPkg/ResetVector: Validate the encryption bit position
for SEV/SEV-ES
2 bd0c1c8e225b OvmfPkg/ResetVector: Perform a simple SEV-ES sanity check
3 3b32be7e7192 OvmfPkg/ResetVector: Save the encryption mask at boot time
4 b97dc4b92ba1 OvmfPkg/MemEncryptSevLib: Add an interface to retrieve the
encryption mask
The "RandomData" and "EncryptionMask" fields in the
SEC_SEV_ES_WORK_AREA structure seem justified because they implement
some serious work (which must be done as early as possible, i.e., in
the Reset Vector), *and* caching the result of that work for the rest
of the firmware saves significant complexity (and the result is
security-related even).
"SEC_SEV_ES_WORK_AREA.SevSnpEnabled" is unlike any of these three
fields.
)
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 (#76122): https://edk2.groups.io/g/devel/message/76122
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]
-=-=-=-=-=-=-=-=-=-=-=-
