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


Laszlo Ersek
 

On 06/08/21 23:36, Tom Lendacky wrote:
On 6/8/21 3:49 AM, Laszlo Ersek wrote:
On 06/07/21 15:37, Brijesh Singh wrote:

...

... But maybe I just need to accept that we have to repurpose
SEC_SEV_ES_WORK_AREA, considering it a super-early "HOB list" of sorts.
Same as the PEI phase is considered the "HOB producer phase", outputting
a bunch of disparate bits of info, we could consider the SEV-ES parts of
the Reset Vector such an "early info bits" producer phase. I think this
is a very big conceptual step away from the original purpose of
SEC_SEV_ES_WORK_AREA (note the *name* of the structure: "work area"!
HOBs are not "work areas", they are effectively read-only, once
produced). But perhaps this is what we need (and then with proper
documentation).

NB however that HOBs have types, GUIDed HOBs have GUIDs, the HOB types
are specified in PI, and GUIDs are expressly declared to stand for
various purposes at least in edk2 DEC files. All that helps with
discerning the information flow. So... I'd still prefer keeping
SEC_SEV_ES_WORK_AREA as minimal as possible.

Tom, any comments?
The purpose of the work area was originally two-fold. It is used in the
reset vector code to set the SevEsEnabled bit so that we could keep the
original behavior in SecCoreStartupWithStack() - no initialization of the
exception handlers or early enabling of processor cache. The second use is
for initial AP startup, where we had a known memory address at build time
that could be used to set the initial CS:IP of APs for the first boot.

We expanded the use for the security mitigations, used by the reset vector
code and again in SEC. At the start of PEI, PCDs are then set.

So, yes, if the information can be obtained later, and in this case we're
not talking about CPUID information which would need re-validation, then
there's no need to keep it in the work area and we can keep the size and
information stored in the work area to a minimum.
Thank you very much!
Laszlo


Lendacky, Thomas
 

On 6/8/21 3:49 AM, Laszlo Ersek wrote:
On 06/07/21 15:37, Brijesh Singh wrote:

...

... But maybe I just need to accept that we have to repurpose
SEC_SEV_ES_WORK_AREA, considering it a super-early "HOB list" of sorts.
Same as the PEI phase is considered the "HOB producer phase", outputting
a bunch of disparate bits of info, we could consider the SEV-ES parts of
the Reset Vector such an "early info bits" producer phase. I think this
is a very big conceptual step away from the original purpose of
SEC_SEV_ES_WORK_AREA (note the *name* of the structure: "work area"!
HOBs are not "work areas", they are effectively read-only, once
produced). But perhaps this is what we need (and then with proper
documentation).

NB however that HOBs have types, GUIDed HOBs have GUIDs, the HOB types
are specified in PI, and GUIDs are expressly declared to stand for
various purposes at least in edk2 DEC files. All that helps with
discerning the information flow. So... I'd still prefer keeping
SEC_SEV_ES_WORK_AREA as minimal as possible.

Tom, any comments?
The purpose of the work area was originally two-fold. It is used in the
reset vector code to set the SevEsEnabled bit so that we could keep the
original behavior in SecCoreStartupWithStack() - no initialization of the
exception handlers or early enabling of processor cache. The second use is
for initial AP startup, where we had a known memory address at build time
that could be used to set the initial CS:IP of APs for the first boot.

We expanded the use for the security mitigations, used by the reset vector
code and again in SEC. At the start of PEI, PCDs are then set.

So, yes, if the information can be obtained later, and in this case we're
not talking about CPUID information which would need re-validation, then
there's no need to keep it in the work area and we can keep the size and
information stored in the work area to a minimum.

Thanks,
Tom


Thank you Brijesh for raising great points!
Laszlo


Brijesh Singh
 

On 6/8/21 3:49 AM, Laszlo Ersek wrote:
On 06/07/21 15:37, Brijesh Singh wrote:

Also, I was trying to avoid the cases where the malicious hypervisor
changing the feature value after the GHCB negotiation is completed. 
e.g, during the reset vector they give us one feature value and change
them during SEC or PEI or DXE instances run. They can't break the
security of SNP by doing so, but I thought its good to avoid querying
the features on every phase.
If there is *feasible* security problem (attack), then my "information
flow purity" criteria are irrelevant. Security trumps all.

My understanding is though (per your explanation above) that there is no
real security problem here.

Furthermore, we're not going to query the feature set in every phase.
We're going to set the PCDs in the PEI phase; there shouldn't be
hardware querying in the DXE phase. That's quite standard business in OVMF.


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.
This is one of thing which I noticed last week, we are actually creating
a circular dependency. The MemEncryptSevLib provides the routines which
are used by the VmgExitLib. The MemEncryptSevLib should *not* depend on
the VmgExitLib. If we extend the MemEncryptSevLib to provide a routine
to query the features then we will be required to include the
VmgExitLib. The patch #13 extends the MemEncryptSevLib to provide
functions for the page validation and it requires the VmgExitLib. I am
trying to see what I can do to not create this circular dependency and
still keep code reuse.
As far as I remember, a circular dependency is only a problem if both
library instances in question have constructors. If a library instance
needs no construction (has no constructor), then it does not partake in
the topological sorting (for initialization) performed by BaseTools.

In this case, at the end of your RFCv3 series (minus patch#21), no INF
file in either "OvmfPkg/Library/BaseMemEncryptSevLib" or
"OvmfPkg/Library/VmgExitLib" seems to specify a CONSTRUCTOR, so purely
from the build perspective, there should be no issue.
I have to debug it, but it did appear that this circular dependency
caused problem for the SEV guest when SMM is enabled. If SMM is enabled,
then I get a random #UD as soon as I link the VmgExit to
MemEncryptSevLib. I will see what I find.



But, I have another idea here:

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.
the above basically tells us that this library API would be a single-use
interface. It wouldn't work in SEC, it would do actual work in PEI
(namely, in PlatformPei), and it wouldn't do any "real work" in DXE.

To me, the boundary is not crystal clear, but the above struggles
*suggest* that we might not want this API to be a MemEncryptSevLib
function (or any library function) at all. If abstracting out the API
causes more pain than it does good, then let's just not abstract it.

Meaning, you could open-code the fetching of features (using VmgExitLib)
in PlatormPei, set the PCD, and be done with it. The only place where
the PCD (PcdGhcbHypervisorFeatures) is actually used (as far as I can
see) is patch#21, in UefiCpuPkg. We could reasonably say that the "real
API", namely the declaration of the PCD, *already exists*, namely in the
"UefiCpuPkg/UefiCpuPkg.dec" file, from your commit e6994b1d5226
("UefiCpuPkg: Define the SEV-SNP specific dynamic PCDs", 2021-06-04).

There are other examples where a cross-package PCD is set once and for
all in OvmfPkg/PlatformPei (random example:
"PcdCpuBootLogicalProcessorNumber"). We don't have to turn everything
into a lib class API, especially if it causes more pain than help.
This is much ideal, I would prefer to avoid creating a new library or
add a function into the existing library if this function is only going
to be used once during the PEI to query the feature.



... But maybe I just need to accept that we have to repurpose
SEC_SEV_ES_WORK_AREA, considering it a super-early "HOB list" of sorts.
Same as the PEI phase is considered the "HOB producer phase", outputting
a bunch of disparate bits of info, we could consider the SEV-ES parts of
the Reset Vector such an "early info bits" producer phase. I think this
is a very big conceptual step away from the original purpose of
SEC_SEV_ES_WORK_AREA (note the *name* of the structure: "work area"!
HOBs are not "work areas", they are effectively read-only, once
produced). But perhaps this is what we need (and then with proper
documentation).

NB however that HOBs have types, GUIDed HOBs have GUIDs, the HOB types
are specified in PI, and GUIDs are expressly declared to stand for
various purposes at least in edk2 DEC files. All that helps with
discerning the information flow. So... I'd still prefer keeping
SEC_SEV_ES_WORK_AREA as minimal as possible.

Tom, any comments?

Thank you Brijesh for raising great points!
Laszlo


Laszlo Ersek
 

On 06/07/21 15:37, Brijesh Singh wrote:

Also, I was trying to avoid the cases where the malicious hypervisor
changing the feature value after the GHCB negotiation is completed. 
e.g, during the reset vector they give us one feature value and change
them during SEC or PEI or DXE instances run. They can't break the
security of SNP by doing so, but I thought its good to avoid querying
the features on every phase.
If there is *feasible* security problem (attack), then my "information
flow purity" criteria are irrelevant. Security trumps all.

My understanding is though (per your explanation above) that there is no
real security problem here.

Furthermore, we're not going to query the feature set in every phase.
We're going to set the PCDs in the PEI phase; there shouldn't be
hardware querying in the DXE phase. That's quite standard business in OVMF.


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.
This is one of thing which I noticed last week, we are actually creating
a circular dependency. The MemEncryptSevLib provides the routines which
are used by the VmgExitLib. The MemEncryptSevLib should *not* depend on
the VmgExitLib. If we extend the MemEncryptSevLib to provide a routine
to query the features then we will be required to include the
VmgExitLib. The patch #13 extends the MemEncryptSevLib to provide
functions for the page validation and it requires the VmgExitLib. I am
trying to see what I can do to not create this circular dependency and
still keep code reuse.
As far as I remember, a circular dependency is only a problem if both
library instances in question have constructors. If a library instance
needs no construction (has no constructor), then it does not partake in
the topological sorting (for initialization) performed by BaseTools.

In this case, at the end of your RFCv3 series (minus patch#21), no INF
file in either "OvmfPkg/Library/BaseMemEncryptSevLib" or
"OvmfPkg/Library/VmgExitLib" seems to specify a CONSTRUCTOR, so purely
from the build perspective, there should be no issue.

But, I have another idea here:

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.
the above basically tells us that this library API would be a single-use
interface. It wouldn't work in SEC, it would do actual work in PEI
(namely, in PlatformPei), and it wouldn't do any "real work" in DXE.

To me, the boundary is not crystal clear, but the above struggles
*suggest* that we might not want this API to be a MemEncryptSevLib
function (or any library function) at all. If abstracting out the API
causes more pain than it does good, then let's just not abstract it.

Meaning, you could open-code the fetching of features (using VmgExitLib)
in PlatormPei, set the PCD, and be done with it. The only place where
the PCD (PcdGhcbHypervisorFeatures) is actually used (as far as I can
see) is patch#21, in UefiCpuPkg. We could reasonably say that the "real
API", namely the declaration of the PCD, *already exists*, namely in the
"UefiCpuPkg/UefiCpuPkg.dec" file, from your commit e6994b1d5226
("UefiCpuPkg: Define the SEV-SNP specific dynamic PCDs", 2021-06-04).

There are other examples where a cross-package PCD is set once and for
all in OvmfPkg/PlatformPei (random example:
"PcdCpuBootLogicalProcessorNumber"). We don't have to turn everything
into a lib class API, especially if it causes more pain than help.

... But maybe I just need to accept that we have to repurpose
SEC_SEV_ES_WORK_AREA, considering it a super-early "HOB list" of sorts.
Same as the PEI phase is considered the "HOB producer phase", outputting
a bunch of disparate bits of info, we could consider the SEV-ES parts of
the Reset Vector such an "early info bits" producer phase. I think this
is a very big conceptual step away from the original purpose of
SEC_SEV_ES_WORK_AREA (note the *name* of the structure: "work area"!
HOBs are not "work areas", they are effectively read-only, once
produced). But perhaps this is what we need (and then with proper
documentation).

NB however that HOBs have types, GUIDed HOBs have GUIDs, the HOB types
are specified in PI, and GUIDs are expressly declared to stand for
various purposes at least in edk2 DEC files. All that helps with
discerning the information flow. So... I'd still prefer keeping
SEC_SEV_ES_WORK_AREA as minimal as possible.

Tom, any comments?

Thank you Brijesh for raising great points!
Laszlo


Brijesh Singh
 

Hi Laszlo,


On 6/7/21 6:54 AM, Laszlo Ersek wrote:
Hi Brijesh,

On 05/27/21 01:11, Brijesh Singh wrote:
BZ: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3275&;data=04%7C01%7Cbrijesh.singh%40amd.com%7C107d760e06824b1b6d6d08d929aaffd3%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637586636747299845%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=KQ42j9fOtsJfCh2iziap1v%2BBqC%2FHfR%2BR9ZbO3JHAJaM%3D&reserved=0

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.
Noted.



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.
I will split it, but all the checkpatch and CI stuff passed on my branch
before the submission.




+}
+
/**

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.
Noted, I will match them.



+
+; 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.
Ah good catch. Thanks



; 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".
Noted.



+ ; 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.
Okay, I will try moving all the SEV specific stuff outside this file.



+ 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.
Noted, I will fix it.



+
+ 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".
Sure, while moving SEV bits in a separate file I will try to define a
matching macros for it.




+ 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".
I choose to cache as much as I could during the reset vector time so
that we avoid the need for VmgExit later. But now I understand that you
prefer to query those information when they are needed and not use the
SevEsWorkArea as a cache for other information.

Also, I was trying to avoid the cases where the malicious hypervisor
changing the feature value after the GHCB negotiation is completed. 
e.g, during the reset vector they give us one feature value and change
them during SEC or PEI or DXE instances run. They can't break the
security of SNP by doing so, but I thought its good to avoid querying
the features on every phase.



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.
This is one of thing which I noticed last week, we are actually creating
a circular dependency. The MemEncryptSevLib provides the routines which
are used by the VmgExitLib. The MemEncryptSevLib should *not* depend on
the VmgExitLib. If we extend the MemEncryptSevLib to provide a routine
to query the features then we will be required to include the
VmgExitLib. The patch #13 extends the MemEncryptSevLib to provide
functions for the page validation and it requires the VmgExitLib. I am
trying to see what I can do to not create this circular dependency and
still keep code reuse.

Thanks

Brijesh


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


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


Brijesh Singh
 

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(+)

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));
+}
+
/**

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
+
+; 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

; 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:
+ ; 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
+ 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
+
+ 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
+ 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"
--
2.17.1