Re: [edk2-devel] [PATCH RFC v3 03/22] OvmfPkg/MemEncryptSevLib: extend the workarea to include SNP enabled field

["Laszlo Ersek" <lersek@redhat.com>]

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 <jejb@linux.ibm.com>
>> Cc: Min Xu <min.m.xu@intel.com>
>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>> Cc: Tom Lendacky <thomas.lendacky@amd.com>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Erdem Aktas <erdemaktas@google.com>
>> Signed-off-by: Brijesh Singh <brijesh.singh@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 <lersek@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))
>>
>
>
>
> 
>
>