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

Brijesh Singh posted 28 patches 1 month, 2 weeks ago

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

Posted by Brijesh Singh 1 month, 2 weeks ago
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3275

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
+#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
-- 
2.17.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#74628): https://edk2.groups.io/g/devel/message/74628
Mute This Topic: https://groups.io/mt/82479048/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-


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

Posted by Laszlo Ersek 1 month, 2 weeks ago
Hi Brijesh, Tom,

On 04/30/21 13:51, Brijesh Singh wrote:
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3275
> 
> 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.


> +#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.

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


(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



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#74695): https://edk2.groups.io/g/devel/message/74695
Mute This Topic: https://groups.io/mt/82479048/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-


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

Posted by Brijesh Singh 1 month, 2 weeks ago
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&amp;data=04%7C01%7Cbrijesh.singh%40amd.com%7C9a7e31fbf85043c6ee8508d90e1ba94d%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637556334239842920%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=fSThw3T7P4LcLhcZz9tfy4ZB1Y7Zny0BzwA2jTyWAkY%3D&amp;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
>


-=-=-=-=-=-=-=-=-=-=-=-
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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-


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

Posted by Laszlo Ersek 1 month, 2 weeks ago
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&amp;data=04%7C01%7Cbrijesh.singh%40amd.com%7C9a7e31fbf85043c6ee8508d90e1ba94d%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637556334239842920%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=fSThw3T7P4LcLhcZz9tfy4ZB1Y7Zny0BzwA2jTyWAkY%3D&amp;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
>>
> 



-=-=-=-=-=-=-=-=-=-=-=-
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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-