Re: [edk2-devel] [PATCH RFC v2 02/28] MdePkg: Define the GHCB Hypervisor features

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

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@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>
>>> ---
>>>  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@redhat.com>
>>
>>
>> Thanks
>> Laszlo
>>
>