[edk2-devel] [PATCH RFC v2 02/28] MdePkg: Define the GHCB Hypervisor features
Brijesh Singh
brijesh.singh at amd.com
Mon May 3 12:20:28 UTC 2021
On 5/3/21 5:10 AM, Laszlo Ersek wrote:
> Hi Brijesh, Tom,
>
> On 04/30/21 13:51, 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%7C9a7e31fbf85043c6ee8508d90e1ba94d%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637556334239842920%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=fSThw3T7P4LcLhcZz9tfy4ZB1Y7Zny0BzwA2jTyWAkY%3D&reserved=0
>>
>> Version 2 of GHCB introduces advertisement of features that are supported
>> by the hypervisor. See the GHCB spec section 2.2 for an additional details.
>>
>> Cc: James Bottomley <jejb at linux.ibm.com>
>> Cc: Min Xu <min.m.xu at intel.com>
>> Cc: Jiewen Yao <jiewen.yao at intel.com>
>> Cc: Tom Lendacky <thomas.lendacky at amd.com>
>> Cc: Jordan Justen <jordan.l.justen at intel.com>
>> Cc: Ard Biesheuvel <ardb+tianocore at kernel.org>
>> Cc: Laszlo Ersek <lersek at redhat.com>
>> Cc: Erdem Aktas <erdemaktas at google.com>
>> Signed-off-by: Brijesh Singh <brijesh.singh at amd.com>
>> ---
>> MdePkg/Include/Register/Amd/Fam17Msr.h | 7 +++++++
>> MdePkg/Include/Register/Amd/Ghcb.h | 6 ++++++
>> 2 files changed, 13 insertions(+)
>>
>> diff --git a/MdePkg/Include/Register/Amd/Fam17Msr.h b/MdePkg/Include/Register/Amd/Fam17Msr.h
>> index 4d33bef220..a65d51ab12 100644
>> --- a/MdePkg/Include/Register/Amd/Fam17Msr.h
>> +++ b/MdePkg/Include/Register/Amd/Fam17Msr.h
>> @@ -48,6 +48,11 @@ typedef union {
>> UINT32 Reserved2:32;
>> } GhcbTerminate;
>>
>> + struct {
>> + UINT64 Function:12;
>> + UINT64 Features:52;
>> + } GhcbHypervisorFeatures;
>> +
>> VOID *Ghcb;
>>
>> UINT64 GhcbPhysicalAddress;
>> @@ -57,6 +62,8 @@ typedef union {
>> #define GHCB_INFO_SEV_INFO_GET 2
>> #define GHCB_INFO_CPUID_REQUEST 4
>> #define GHCB_INFO_CPUID_RESPONSE 5
>> +#define GHCB_HYPERVISOR_FEATURES_REQUEST 128
>> +#define GHCB_HYPERVISOR_FEATURES_RESPONSE 129
>> #define GHCB_INFO_TERMINATE_REQUEST 256
>>
>> #define GHCB_TERMINATE_GHCB 0
>> diff --git a/MdePkg/Include/Register/Amd/Ghcb.h b/MdePkg/Include/Register/Amd/Ghcb.h
>> index ccdb662af7..2d64a4c28f 100644
>> --- a/MdePkg/Include/Register/Amd/Ghcb.h
>> +++ b/MdePkg/Include/Register/Amd/Ghcb.h
>> @@ -54,6 +54,7 @@
>> #define SVM_EXIT_NMI_COMPLETE 0x80000003ULL
>> #define SVM_EXIT_AP_RESET_HOLD 0x80000004ULL
>> #define SVM_EXIT_AP_JUMP_TABLE 0x80000005ULL
>> +#define SVM_EXIT_HYPERVISOR_FEATURES 0x8000FFFDULL
>> #define SVM_EXIT_UNSUPPORTED 0x8000FFFFULL
>>
>> //
>> @@ -154,4 +155,9 @@ typedef union {
>> #define GHCB_EVENT_INJECTION_TYPE_EXCEPTION 3
>> #define GHCB_EVENT_INJECTION_TYPE_SOFT_INT 4
>>
>> +// Hypervisor features
> (1) Comment style -- leading and trailing // lines missing.
Noted.
>
>
>> +#define GHCB_HV_FEATURES_SNP BIT0
>> +#define GHCB_HV_FEATURES_SNP_AP_CREATE (GHCB_HV_FEATURES_SNP | BIT1)
>> +#define GHCB_HV_FEATURES_SNP_RESTRICTED_INJECTION (GHCB_HV_FEATURES_SNP_AP_CREATE | BIT2)
>> +#define GHCB_HV_FEATURES_SNP_RESTRICTED_INJECTION_TIMER (GHCB_HV_FEATURES_SNP_RESTRICTED_INJECTION | BIT3)
>> #endif
>>
> I'm going to take this series slow, because I need to rebuild whatever
> understanding I've ever had of SEV-ES from the bottom up.
>
> The patch looks good to me (I checked the GHCB spec 2.0, and the values
> seem to match).
>
> But I need some confirmation. The GHCB spec defines the "GHCB MSR"
> protocol, where MSR_SEV_ES_GHCB can be used for a direct
> request/response protocol when the least significant 12 bits are nonzero
> (i.e., they stand for a "function"). The sequence in this case (from the
> guest side is): wrmsr, vmgexit, rdmsr.
>
> On the host side, upon vmgexit, the MSR's twelve least significant bits
> are checked, and if they are nonzero, the function is handled, and the
> response is provided in the high-order bits of the MSR. Otherwise, if
> the "function" is zero, the MSR's contents are taken as a GPA, and then
> the pointed-to page (the GHCB) is consulted for the actual request.
>
> This means that some functions are possible for the guest to call in two
> ways -- with and without a (decrypted) GHCB existing. (The spec writes
> in 2.3.1, "The GHCB MSR protocol is valid at any time but is most useful
> when the GHCB page cannot be written by the guest in an unencrypted
> fashion").
>
> One of the new things the GHCB 2.0 spec introduces is the "hypervisor
> feature advertisement", which is (apparently) one of those functions
> that are available to the guest via both the GHCB *MSR protocol*
> (function = GHCB_HYPERVISOR_FEATURES_REQUEST) and the GHCB *page*
> (SwExitCode = SVM_EXIT_HYPERVISOR_FEATURES, response in SwExitInfo2).
>
> My question is: when is it useful to fetch the hv features through the
> GHCB *page* (i.e., not through the MSR protocol)? At the end of the
> series, I don't see any use for SVM_EXIT_HYPERVISOR_FEATURES.
In my OVMF and Linux-guest patches I am using the MSR protocol based
HV_FEATUERS because I query the features during the negotiation and
cache it. The value is saved in Es workarea and platformPei saves in a PCD.
In a different implementation, a guest can call the HV_FEATURES every
time they need to consult the feature values. I think spec wanted to
keep the flexibility that feature can be queried through the non-MSR
based vmgexit so that the guest does not need save/restore the GHCB
address after the GHCB is established. If I was not caching the feature
value in patch #16 then I would have used the non-MSR based vmgexit to
query the value in PlatformPei to build the PCD.
> A similarly unused macro (from before this series) is
> SVM_EXIT_NMI_COMPLETE. So I guess the approach in the edk2 SEV* work has
> been to incorporate all spec-defined constants in MdePkg. That's a valid
> approach per se; what I'd like to understand is what use case for
> SVM_EXIT_HYPERVISOR_FEATURES the GHCB *spec* foresees.
> (2) Does the spec define SVM_EXIT_HYPERVISOR_FEATURES for completeness'
> sake -- so that no function be restricted to the MSR protocol? (IOW,
> should the MSR protocol be a subset, by principle, of the functions
> available through the GHCB *page*?)
I think non-MSR based vmgexit is done for completeness sake. It maybe
used by other HV or Guests (e.g Windows, Unix etc etc). At this time I
am not using it in OVMF or Linux guest.
>
> I prefer to define only such macros in edk2 that are actually used --
> but I admit that may be different from the general MdePkg rules. So I
> don't mind SVM_EXIT_HYPERVISOR_FEATURES, it's just a bit more difficult
> to review / understand without actual use.
Good point, I have no issue removing the unused macro. If we see a need
for it then it can be added in the future.
>
> (3) I suggest the following subject:
>
> MdePkg/Register/Amd: define GHCB macros for hypervisor feature detection
>
> (72 chars)
>
> With (1) and (3) fixed:
>
> Reviewed-by: Laszlo Ersek <lersek at redhat.com>
>
>
> Thanks
> Laszlo
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#74700): https://edk2.groups.io/g/devel/message/74700
Mute This Topic: https://groups.io/mt/82479048/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list