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

["Brijesh Singh" <brijesh.singh@amd.com>]

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.


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