[edk2-devel] [PATCH RFC v2 02/28] MdePkg: Define the GHCB Hypervisor features
Laszlo Ersek
lersek at redhat.com
Mon May 3 13:40:49 UTC 2021
On 05/03/21 14:20, Brijesh Singh wrote:
>
> 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.
Thanks for the explanation.
I'm not necessarily requesting that SVM_EXIT_HYPERVISOR_FEATURES be
removed from the patch. We already have the (similarly unused)
SVM_EXIT_NMI_COMPLETE macro in MdePkg. We should aim for consistency --
we should decide whether MdePkg headers need to stick with the spec(s)
as closely and comprehensively as possible, or whether they should only
incorporate what's actually used by code. Given that edk2 is a "kit",
and MdePkg is the most central part of it, I think being comprehensive
in MdePkg is not a bad choice. Given that SVM_EXIT_NMI_COMPLETE already
exemplifies this approach, I think we can and *should* bring in
SVM_EXIT_HYPERVISOR_FEATURES as well, given that the spec provides it.
Thanks
Laszlo
>
>
>>
>> (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 (#74703): https://edk2.groups.io/g/devel/message/74703
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