Re: [PATCH RFC v3 04/22] OvmfPkg/MemEncryptSevLib: extend Es Workarea to include hv features


Laszlo Ersek
 

Hi Brijesh,

On 05/27/21 01:11, Brijesh Singh wrote:
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3275

The GHCB Version 2 introduces advertisement of features that are supported
by the hypervisor. The features value is saved in the SevEs workarea. Save
the value in the PCD for the later use.

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 | 2 +
OvmfPkg/PlatformPei/AmdSev.c | 26 +++++
OvmfPkg/ResetVector/Ia32/PageTables64.asm | 122 +++++++++++++++++++++
OvmfPkg/ResetVector/ResetVector.nasmb | 1 +
5 files changed, 152 insertions(+)
(1) Please split this patch: the PlatformPei changes should be in the second patch.


diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf b/OvmfPkg/PlatformPei/PlatformPei.inf
index bc1dcac48343..3256ccfe88d8 100644
--- a/OvmfPkg/PlatformPei/PlatformPei.inf
+++ b/OvmfPkg/PlatformPei/PlatformPei.inf
@@ -111,6 +111,7 @@ [Pcd]
gUefiCpuPkgTokenSpaceGuid.PcdCpuApStackSize
gUefiCpuPkgTokenSpaceGuid.PcdSevEsIsEnabled
gUefiCpuPkgTokenSpaceGuid.PcdSevSnpIsEnabled
+ gUefiCpuPkgTokenSpaceGuid.PcdGhcbHypervisorFeatures

[FixedPcd]
gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress
diff --git a/OvmfPkg/Include/Library/MemEncryptSevLib.h b/OvmfPkg/Include/Library/MemEncryptSevLib.h
index 24507de55c5d..dd1c97d4a9a3 100644
--- a/OvmfPkg/Include/Library/MemEncryptSevLib.h
+++ b/OvmfPkg/Include/Library/MemEncryptSevLib.h
@@ -55,6 +55,8 @@ typedef struct _SEC_SEV_ES_WORK_AREA {
UINT64 RandomData;

UINT64 EncryptionMask;
+
+ UINT64 HypervisorFeatures;
} SEC_SEV_ES_WORK_AREA;

//
diff --git a/OvmfPkg/PlatformPei/AmdSev.c b/OvmfPkg/PlatformPei/AmdSev.c
index 67b78fd5fa36..81e40e0889aa 100644
--- a/OvmfPkg/PlatformPei/AmdSev.c
+++ b/OvmfPkg/PlatformPei/AmdSev.c
@@ -43,6 +43,27 @@ AmdSevSnpInitialize (
ASSERT_RETURN_ERROR (PcdStatus);
}

+/**
+
+ Function to set the PcdHypervisorFeatures.
+**/
+STATIC
+VOID
+AmdSevHypervisorFeatures (
+ VOID
+ )
+{
+ SEC_SEV_ES_WORK_AREA *SevEsWorkArea;
+ RETURN_STATUS PcdStatus;
+
+ SevEsWorkArea = (SEC_SEV_ES_WORK_AREA *) FixedPcdGet32 (PcdSevEsWorkAreaBase);
+
+ PcdStatus = PcdSet64S (PcdGhcbHypervisorFeatures, SevEsWorkArea->HypervisorFeatures);
+ ASSERT_RETURN_ERROR (PcdStatus);
+
+ DEBUG ((DEBUG_INFO, "GHCB Hypervisor Features=0x%Lx\n", SevEsWorkArea->HypervisorFeatures));
(2) Overlong line.

Please avoid basic mistakes like this, even in an RFC series.


+}
+
/**

Initialize SEV-ES support if running as an SEV-ES guest.
@@ -73,6 +94,11 @@ AmdSevEsInitialize (
PcdStatus = PcdSetBoolS (PcdSevEsIsEnabled, TRUE);
ASSERT_RETURN_ERROR (PcdStatus);

+ //
+ // Set the hypervisor features PCD.
+ //
+ AmdSevHypervisorFeatures ();
+
//
// Allocate GHCB and per-CPU variable pages.
// Since the pages must survive across the UEFI to OS transition
diff --git a/OvmfPkg/ResetVector/Ia32/PageTables64.asm b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
index 6838cdeec9c3..75e63d2a0561 100644
--- a/OvmfPkg/ResetVector/Ia32/PageTables64.asm
+++ b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
@@ -62,6 +62,16 @@ BITS 32
%define GHCB_CPUID_REGISTER_SHIFT 30
%define CPUID_INSN_LEN 2

+; GHCB SEV Information MSR protocol
+%define GHCB_SEV_INFORMATION_REQUEST 2
+%define GHCB_SEV_INFORMATION_RESPONSE 1
(3) These macro names do not match the ones in "MdePkg/Include/Register/Amd/Fam17Msr.h" (GHCB_INFO_SEV_INFO_GET, GHCB_INFO_SEV_INFO, respectively).

They don't *have* to match, technically speaking, but one goal of using macros for magic numbers is so we can grep the source for them. The macros just below (for values 128 and 129) do match the header file.


+
+; GHCB Hypervisor features MSR protocol
+%define GHCB_HYPERVISOR_FEATURES_REQUEST 128
+%define GHCB_HYPERVISOR_FEATURES_RESPONSE 129
+
+; GHCB request to terminate protocol values
+%define GHCB_GENERAL_TERMINATE_REQUEST 255
(4) Not only does this macro name not match the one in the header file (which is GHCB_INFO_TERMINATE_REQUEST), even the value is wrong. The header file has

#define GHCB_INFO_TERMINATE_REQUEST 256

and I checked the GHCBv2 spec too; there is no operation defined with opcode 255.



; Check if Secure Encrypted Virtualization (SEV) features are enabled.
;
@@ -86,6 +96,13 @@ CheckSevFeatures:
; will set it to 1.
mov byte[SEV_ES_WORK_AREA_SNP], 0

+ ; Set the Hypervisor features field in the workarea to zero to communicate
+ ; to the hypervisor features to the SEC phase. The hypervisor feature is
+ ; filled during the call to CheckHypervisorFeatures.
+ mov eax, 0
+ mov dword[SEV_ES_WORK_AREA_HYPERVISOR_FEATURES], eax
+ mov dword[SEV_ES_WORK_AREA_HYPERVISOR_FEATURES + 4], eax
+
;
; Set up exception handlers to check for SEV-ES
; Load temporary RAM stack based on PCDs (see SevEsIdtVmmComm for
@@ -225,6 +242,106 @@ IsSevEsEnabled:
SevEsDisabled:
OneTimeCallRet IsSevEsEnabled

+; The version 2 of GHCB specification added the support to query the hypervisor features.
+; If the GHCB version is >=2 then read the hypervisor features.
+;
+; Modified: EAX, EBX, ECX, EDX
+;
+CheckHypervisorFeatures:
(5) Arguably this label name should contain "Sev".


+ ; Get the SEV Information
+ ; Setup GHCB MSR
+ ; GHCB_MSR[11:0] = SEV information request
+ ;
+ mov edx, 0
+ mov eax, GHCB_SEV_INFORMATION_REQUEST
+ mov ecx, 0xc0010130
+ wrmsr
+
+ ;
+ ; Issue VMGEXIT - NASM doesn't support the vmmcall instruction in 32-bit
+ ; mode, so work around this by temporarily switching to 64-bit mode.
+ ;
+BITS 64
+ rep vmmcall
+BITS 32
+
+ ;
+ ; SEV Information Response GHCB MSR
+ ; GHCB_MSR[63:48] = Maximum protocol version
+ ; GHCB_MSR[47:32] = Minimum protocol version
+ ; GHCB_MSR[11:0] = SEV information response
+ ;
+ mov ecx, 0xc0010130
+ rdmsr
+ and eax, 0xfff
+ cmp eax, GHCB_SEV_INFORMATION_RESPONSE
+ jnz TerminateSevGuestLaunch
(6) Before modifying the ResetVector module like this, please insert a refactoring patch as follows:

- A new SEV-specific assembly include file should be introduced. The majority of the "OvmfPkg/ResetVector/Ia32/PageTables64.asm" file now deals with SEV aspects, but the file-top comment still says "Sets the CR3 register for 64-bit paging". It's high time that we move SEV stuff to a file with a name that references SEV.

- We now have five (5) invocations of the GHCB MSR protocol in this file, and every one of them open-codes the same setup, the same 0xc0010130 MSR constant, the same retval check logic with possible guest termination, the same "rep vmmcall" workaround for the 32-bit limitation of NASM, and so on. this file is now borderline unreadable. At the minimum, please introduce a function-like NASM macro with two arguments (= the request & response opcodes), and extract as much as possible.



+ shr edx, 16
+ cmp edx, 2
+ jl CheckHypervisorFeaturesDone
+
+ ; Get the hypervisor features
+ ; Setup GHCB MSR
+ ; GHCB_MSR[11:0] = Hypervisor features request
+ ;
+ mov edx, 0
+ mov eax, GHCB_HYPERVISOR_FEATURES_REQUEST
+ mov ecx, 0xc0010130
+ wrmsr
+
+ ;
+ ; Issue VMGEXIT - NASM doesn't support the vmmcall instruction in 32-bit
+ ; mode, so work around this by temporarily switching to 64-bit mode.
+ ;
+BITS 64
+ rep vmmcall
+BITS 32
+
+ ;
+ ; Hypervisor features reponse
+ ; GHCB_MSR[63:12] = Features bitmap
+ ; GHCB_MSR[11:0] = Hypervisor features response
+ ;
+ mov ecx, 0xc0010130
+ rdmsr
+ mov ebx, eax
+ and eax, 0xfff
+ cmp eax, GHCB_HYPERVISOR_FEATURES_RESPONSE
+ jnz TerminateSevGuestLaunch
+
+ shr ebx, 12
+ mov dword[SEV_ES_WORK_AREA_HYPERVISOR_FEATURES], ebx
+ mov dword[SEV_ES_WORK_AREA_HYPERVISOR_FEATURES + 4], edx
(7) According to the spec, the FEATURES bitmap is a contiguous bitmap of 52 bits. The way the EDX:EAX qword is shifted right by 12 bits above is incorrect. The EAX half is shifted OK (through EBX), but the EDX half is not shifted down by 12 bits at all, it is simply stored to the most significant dword of the "HypervisorFeatures" field. This basically inserts a 12 bit wide gap in the FEATURES bitmap.


+
+ jmp CheckHypervisorFeaturesDone
+TerminateSevGuestLaunch:
+ ;
+ ; Setup GHCB MSR
+ ; GHCB_MSR[23:16] = 0
+ ; GHCB_MSR[15:12] = 0
+ ; GHCB_MSR[11:0] = Terminate Request
+ ;
+ mov edx, 0
+ mov eax, GHCB_GENERAL_TERMINATE_REQUEST
(8) The "MdePkg/Include/Register/Amd/Fam17Msr.h" header file introduces GHCB_TERMINATE_GHCB, GHCB_TERMINATE_GHCB_GENERAL, GHCB_TERMINATE_GHCB_PROTOCOL. Can we use some of those here (with a separate, but matching, NASM macro of course)? See SevEsProtocolFailure() in "OvmfPkg/Sec/SecMain.c".


+ mov ecx, 0xc0010130
+ wrmsr
+
+ ;
+ ; Issue VMGEXIT - NASM doesn't support the vmmcall instruction in 32-bit
+ ; mode, so work around this by temporarily switching to 64-bit mode.
+ ;
+BITS 64
+ rep vmmcall
+BITS 32
+
+TerminateSevGuestLaunchHlt:
+ cli
+ hlt
+ jmp TerminateSevGuestLaunchHlt
+
+CheckHypervisorFeaturesDone:
+ OneTimeCallRet CheckHypervisorFeatures
+
;
; Modified: EAX, EBX, ECX, EDX
;
@@ -328,6 +445,11 @@ clearGhcbMemoryLoop:
mov dword[ecx * 4 + GHCB_BASE - 4], eax
loop clearGhcbMemoryLoop

+ ;
+ ; It is SEV-ES guest, query the Hypervisor features
+ ;
+ OneTimeCall CheckHypervisorFeatures
+
SetCr3:
;
; Set CR3 now that the paging structures are available
diff --git a/OvmfPkg/ResetVector/ResetVector.nasmb b/OvmfPkg/ResetVector/ResetVector.nasmb
index 1971557b1c00..5beba3ecb290 100644
--- a/OvmfPkg/ResetVector/ResetVector.nasmb
+++ b/OvmfPkg/ResetVector/ResetVector.nasmb
@@ -76,6 +76,7 @@
%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_WORK_AREA_HYPERVISOR_FEATURES (FixedPcdGet32 (PcdSevEsWorkAreaBase) + 24)
%define SEV_ES_VC_TOP_OF_STACK (FixedPcdGet32 (PcdOvmfSecPeiTempRamBase) + FixedPcdGet32 (PcdOvmfSecPeiTempRamSize))
%include "Ia32/Flat32ToFlat64.asm"
%include "Ia32/PageTables64.asm"
(9) And, I'm arriving at the following assertion only now unfortunately, after spending about 4 hours on reviewing this patch, and the history of SEC_SEV_ES_WORK_AREA.

I assert that the "SEC_SEV_ES_WORK_AREA.HypervisorFeatures" field should not exist. The only read site is here, in "OvmfPkg/PlatformPei".

Instead, we should have a new MemEncryptSevLib function that outputs the FEATURES bitmask. It should be similar to MemEncryptSevGetEncryptionMask(), but it should return a RETURN_STATUS, and produce the FEATURES bitmask through an output parameter.

The SEC instance of the function should return RETURN_UNSUPPORTED.

The PEI instance should use the GHCB MSR protocol, with the help of the AsmCpuId(), AsmWriteMsr64(), AsmReadMsr64() and AsmVmgExit() BaseLib functions.

The DXE instance should read back the PCD.

And so the OvmfPkg/ResetVector hunks should be dropped from this patch.

Thanks,
Laszlo

Join devel@edk2.groups.io to automatically receive all group messages.