[edk2-devel] [PATCH RFC v2 03/28] MdePkg: Define the GHCB GPA structure

Brijesh Singh posted 28 patches 1 month, 2 weeks ago

[edk2-devel] [PATCH RFC v2 03/28] MdePkg: Define the GHCB GPA structure

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

An SEV-SNP guest is required to perform the GHCB GPA registration. See
the GHCB specification for further 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 +++++++
 1 file changed, 7 insertions(+)

diff --git a/MdePkg/Include/Register/Amd/Fam17Msr.h b/MdePkg/Include/Register/Amd/Fam17Msr.h
index a65d51ab12..e19bd04b6c 100644
--- a/MdePkg/Include/Register/Amd/Fam17Msr.h
+++ b/MdePkg/Include/Register/Amd/Fam17Msr.h
@@ -53,6 +53,11 @@ typedef union {
     UINT64  Features:52;
   } GhcbHypervisorFeatures;
 
+  struct {
+    UINT64  Function:12;
+    UINT64  GuestFrameNumber:52;
+  } GhcbGpaRegister;
+
   VOID    *Ghcb;
 
   UINT64  GhcbPhysicalAddress;
@@ -62,6 +67,8 @@ typedef union {
 #define GHCB_INFO_SEV_INFO_GET             2
 #define GHCB_INFO_CPUID_REQUEST            4
 #define GHCB_INFO_CPUID_RESPONSE           5
+#define GHCB_INFO_GHCB_GPA_REGISTER_REQUEST   18
+#define GHCB_INFO_GHCB_GPA_REGISTER_RESPONSE  19
 #define GHCB_HYPERVISOR_FEATURES_REQUEST   128
 #define GHCB_HYPERVISOR_FEATURES_RESPONSE  129
 #define GHCB_INFO_TERMINATE_REQUEST        256
-- 
2.17.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#74630): https://edk2.groups.io/g/devel/message/74630
Mute This Topic: https://groups.io/mt/82479050/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 03/28] MdePkg: Define the GHCB GPA structure

Posted by Laszlo Ersek 1 month, 2 weeks ago
On 04/30/21 13:51, Brijesh Singh wrote:
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3275
> 
> An SEV-SNP guest is required to perform the GHCB GPA registration. See
> the GHCB specification for further 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 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/MdePkg/Include/Register/Amd/Fam17Msr.h b/MdePkg/Include/Register/Amd/Fam17Msr.h
> index a65d51ab12..e19bd04b6c 100644
> --- a/MdePkg/Include/Register/Amd/Fam17Msr.h
> +++ b/MdePkg/Include/Register/Amd/Fam17Msr.h
> @@ -53,6 +53,11 @@ typedef union {
>      UINT64  Features:52;
>    } GhcbHypervisorFeatures;
>  
> +  struct {
> +    UINT64  Function:12;
> +    UINT64  GuestFrameNumber:52;
> +  } GhcbGpaRegister;
> +
>    VOID    *Ghcb;
>  
>    UINT64  GhcbPhysicalAddress;
> @@ -62,6 +67,8 @@ typedef union {
>  #define GHCB_INFO_SEV_INFO_GET             2
>  #define GHCB_INFO_CPUID_REQUEST            4
>  #define GHCB_INFO_CPUID_RESPONSE           5
> +#define GHCB_INFO_GHCB_GPA_REGISTER_REQUEST   18
> +#define GHCB_INFO_GHCB_GPA_REGISTER_RESPONSE  19
>  #define GHCB_HYPERVISOR_FEATURES_REQUEST   128
>  #define GHCB_HYPERVISOR_FEATURES_RESPONSE  129
>  #define GHCB_INFO_TERMINATE_REQUEST        256
> 

The number match the spec (2.0), but I have some remarks / questions.

(1) Patch #2 (SVM_EXIT_HYPERVISOR_FEATURES) and this patch
(GHCB_INFO_GHCB_GPA_REGISTER_REQUEST) break the nice alignments of the
macro values (replacement texts) in both header files. Can you prepend a
whitespace-only patch that simply moves the affected "columns" to the
right far enough?

(2) I've checked section 2.3.2 "GHCB GPA Registration" in the spec
(2.0). What is the specific risk of allowing a guest to switch from one
GHCB address to another?

(3) It seems strange to expect that a guest stick with a particular GHCB
address for its entire lifetime (including firmware and OS) -- in fact
OVMF already uses multiple GHCB addresses. The spec does not explain how
the guest can "unlock" (de-register) a registered GHCB address.
Furthermore, if a guest can do that *at all* (which I think it must --
we're already using different GHCB addresses between SEC and DXE, for
example), then what protection does the *temporary* locking of the GHCB
address provide?

I'll stop reviewing here, because I think I need to understand your
answers. I'd like to have a rudimentary mental basis for reviewing the rest.

Thanks
Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#74696): https://edk2.groups.io/g/devel/message/74696
Mute This Topic: https://groups.io/mt/82479050/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 03/28] MdePkg: Define the GHCB GPA structure

Posted by Laszlo Ersek 1 month, 2 weeks ago
On 05/03/21 12:24, Laszlo Ersek wrote:
> On 04/30/21 13:51, Brijesh Singh wrote:
>> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3275
>>
>> An SEV-SNP guest is required to perform the GHCB GPA registration. See
>> the GHCB specification for further 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 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/MdePkg/Include/Register/Amd/Fam17Msr.h b/MdePkg/Include/Register/Amd/Fam17Msr.h
>> index a65d51ab12..e19bd04b6c 100644
>> --- a/MdePkg/Include/Register/Amd/Fam17Msr.h
>> +++ b/MdePkg/Include/Register/Amd/Fam17Msr.h
>> @@ -53,6 +53,11 @@ typedef union {
>>      UINT64  Features:52;
>>    } GhcbHypervisorFeatures;
>>  
>> +  struct {
>> +    UINT64  Function:12;
>> +    UINT64  GuestFrameNumber:52;
>> +  } GhcbGpaRegister;
>> +
>>    VOID    *Ghcb;
>>  
>>    UINT64  GhcbPhysicalAddress;
>> @@ -62,6 +67,8 @@ typedef union {
>>  #define GHCB_INFO_SEV_INFO_GET             2
>>  #define GHCB_INFO_CPUID_REQUEST            4
>>  #define GHCB_INFO_CPUID_RESPONSE           5
>> +#define GHCB_INFO_GHCB_GPA_REGISTER_REQUEST   18
>> +#define GHCB_INFO_GHCB_GPA_REGISTER_RESPONSE  19
>>  #define GHCB_HYPERVISOR_FEATURES_REQUEST   128
>>  #define GHCB_HYPERVISOR_FEATURES_RESPONSE  129
>>  #define GHCB_INFO_TERMINATE_REQUEST        256
>>
> 
> The number match the spec (2.0), but I have some remarks / questions.
> 
> (1) Patch #2 (SVM_EXIT_HYPERVISOR_FEATURES) and this patch
> (GHCB_INFO_GHCB_GPA_REGISTER_REQUEST) break the nice alignments of the
> macro values (replacement texts) in both header files. Can you prepend a
> whitespace-only patch that simply moves the affected "columns" to the
> right far enough?
> 
> (2) I've checked section 2.3.2 "GHCB GPA Registration" in the spec
> (2.0). What is the specific risk of allowing a guest to switch from one
> GHCB address to another?
> 
> (3) It seems strange to expect that a guest stick with a particular GHCB
> address for its entire lifetime (including firmware and OS) -- in fact
> OVMF already uses multiple GHCB addresses. The spec does not explain how
> the guest can "unlock" (de-register) a registered GHCB address.
> Furthermore, if a guest can do that *at all* (which I think it must --
> we're already using different GHCB addresses between SEC and DXE, for
> example), then what protection does the *temporary* locking of the GHCB
> address provide?
> 
> I'll stop reviewing here, because I think I need to understand your
> answers. I'd like to have a rudimentary mental basis for reviewing the rest.

... interestingly, with reference to my question (2) under patch "RFC v2
02/28", the GHCB GPA registration function is one that can *only* be
performed with the GHCB MSR protocol, and not through the GHCB page.

So that shows that the MSR protocol's functions cannot be considered a
pure subset of the GHCB page's functions. If
SVM_EXIT_HYPERVISOR_FEATURES didn't exist (and the same function would
only be accessible via GHCB_HYPERVISOR_FEATURES_REQUEST), then no
"larger principle" would be damaged.

Thanks
Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#74699): https://edk2.groups.io/g/devel/message/74699
Mute This Topic: https://groups.io/mt/82479050/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 03/28] MdePkg: Define the GHCB GPA structure

Posted by Brijesh Singh 1 month, 2 weeks ago
On 5/3/21 7:19 AM, Laszlo Ersek wrote:
> On 05/03/21 12:24, Laszlo Ersek wrote:
>> 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%7C9eac9a93753d403dcc4d08d90e2dbcb5%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637556411874265560%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=eNafEGfhCMOkOboQOJnxq8Rw%2BOTuvAUGIziDuELV8%2Bk%3D&amp;reserved=0
>>>
>>> An SEV-SNP guest is required to perform the GHCB GPA registration. See
>>> the GHCB specification for further 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 +++++++
>>>  1 file changed, 7 insertions(+)
>>>
>>> diff --git a/MdePkg/Include/Register/Amd/Fam17Msr.h b/MdePkg/Include/Register/Amd/Fam17Msr.h
>>> index a65d51ab12..e19bd04b6c 100644
>>> --- a/MdePkg/Include/Register/Amd/Fam17Msr.h
>>> +++ b/MdePkg/Include/Register/Amd/Fam17Msr.h
>>> @@ -53,6 +53,11 @@ typedef union {
>>>      UINT64  Features:52;
>>>    } GhcbHypervisorFeatures;
>>>  
>>> +  struct {
>>> +    UINT64  Function:12;
>>> +    UINT64  GuestFrameNumber:52;
>>> +  } GhcbGpaRegister;
>>> +
>>>    VOID    *Ghcb;
>>>  
>>>    UINT64  GhcbPhysicalAddress;
>>> @@ -62,6 +67,8 @@ typedef union {
>>>  #define GHCB_INFO_SEV_INFO_GET             2
>>>  #define GHCB_INFO_CPUID_REQUEST            4
>>>  #define GHCB_INFO_CPUID_RESPONSE           5
>>> +#define GHCB_INFO_GHCB_GPA_REGISTER_REQUEST   18
>>> +#define GHCB_INFO_GHCB_GPA_REGISTER_RESPONSE  19
>>>  #define GHCB_HYPERVISOR_FEATURES_REQUEST   128
>>>  #define GHCB_HYPERVISOR_FEATURES_RESPONSE  129
>>>  #define GHCB_INFO_TERMINATE_REQUEST        256
>>>
>> The number match the spec (2.0), but I have some remarks / questions.
>>
>> (1) Patch #2 (SVM_EXIT_HYPERVISOR_FEATURES) and this patch
>> (GHCB_INFO_GHCB_GPA_REGISTER_REQUEST) break the nice alignments of the
>> macro values (replacement texts) in both header files. Can you prepend a
>> whitespace-only patch that simply moves the affected "columns" to the
>> right far enough?

Sure, do you want me to the post after all the new VMGEXIT's are defined ?


>>
>> (2) I've checked section 2.3.2 "GHCB GPA Registration" in the spec
>> (2.0). What is the specific risk of allowing a guest to switch from one
>> GHCB address to another?

The GHCB is a shared page, there is no risk to switch from one page to
another. This feature is designed to simplify some of the hypervisor
implementation. Since the GHCB is accessed on every vmgexit, a
hypervisor may prefer to create a map during the registration and refer
the map instead of creating a new mapping on every vmgexit.


>>
>> (3) It seems strange to expect that a guest stick with a particular GHCB
>> address for its entire lifetime (including firmware and OS) -- in fact
>> OVMF already uses multiple GHCB addresses. The spec does not explain how
>> the guest can "unlock" (de-register) a registered GHCB address.
>> Furthermore, if a guest can do that *at all* (which I think it must --
>> we're already using different GHCB addresses between SEC and DXE, for
>> example), then what protection does the *temporary* locking of the GHCB
>> address provide?

The spec does not force that GHCB should *never* change once registered.
It says that before switching to new GHCB page, the guest must register
the page. As you rightly said that OVMF uses multiple GHCBs from SEC to
DXE. There is no unregister, registering a new GHCB is a hint to
hypervisor that it should drop the old GHCB mapping. The GHCB
registration is not a PSP function, and are not designed to mitigate a
security exploits. It is purely a hypevisor virtualized feature.


>> I'll stop reviewing here, because I think I need to understand your
>> answers. I'd like to have a rudimentary mental basis for reviewing the rest.
> ... interestingly, with reference to my question (2) under patch "RFC v2
> 02/28", the GHCB GPA registration function is one that can *only* be
> performed with the GHCB MSR protocol, and not through the GHCB page.
>
> So that shows that the MSR protocol's functions cannot be considered a
> pure subset of the GHCB page's functions. If
> SVM_EXIT_HYPERVISOR_FEATURES didn't exist (and the same function would
> only be accessible via GHCB_HYPERVISOR_FEATURES_REQUEST), then no
> "larger principle" would be damaged.

That is correct, not every exit have both MSR and non MSR protocol based
vmgexit. It seems that during the spec review no other HV vendor saw the
need for non-MSR based exit. Certainly, I don't see a need for it in KVM
and can't comment on other HV ;)


>
> Thanks
> Laszlo
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#74701): https://edk2.groups.io/g/devel/message/74701
Mute This Topic: https://groups.io/mt/82479050/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 03/28] MdePkg: Define the GHCB GPA structure

Posted by Laszlo Ersek 1 month, 2 weeks ago
On 05/03/21 14:55, Brijesh Singh wrote:
> 
> On 5/3/21 7:19 AM, Laszlo Ersek wrote:
>> On 05/03/21 12:24, Laszlo Ersek wrote:
>>> 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%7C9eac9a93753d403dcc4d08d90e2dbcb5%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637556411874265560%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=eNafEGfhCMOkOboQOJnxq8Rw%2BOTuvAUGIziDuELV8%2Bk%3D&amp;reserved=0
>>>>
>>>> An SEV-SNP guest is required to perform the GHCB GPA registration. See
>>>> the GHCB specification for further 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 +++++++
>>>>  1 file changed, 7 insertions(+)
>>>>
>>>> diff --git a/MdePkg/Include/Register/Amd/Fam17Msr.h b/MdePkg/Include/Register/Amd/Fam17Msr.h
>>>> index a65d51ab12..e19bd04b6c 100644
>>>> --- a/MdePkg/Include/Register/Amd/Fam17Msr.h
>>>> +++ b/MdePkg/Include/Register/Amd/Fam17Msr.h
>>>> @@ -53,6 +53,11 @@ typedef union {
>>>>      UINT64  Features:52;
>>>>    } GhcbHypervisorFeatures;
>>>>  
>>>> +  struct {
>>>> +    UINT64  Function:12;
>>>> +    UINT64  GuestFrameNumber:52;
>>>> +  } GhcbGpaRegister;
>>>> +
>>>>    VOID    *Ghcb;
>>>>  
>>>>    UINT64  GhcbPhysicalAddress;
>>>> @@ -62,6 +67,8 @@ typedef union {
>>>>  #define GHCB_INFO_SEV_INFO_GET             2
>>>>  #define GHCB_INFO_CPUID_REQUEST            4
>>>>  #define GHCB_INFO_CPUID_RESPONSE           5
>>>> +#define GHCB_INFO_GHCB_GPA_REGISTER_REQUEST   18
>>>> +#define GHCB_INFO_GHCB_GPA_REGISTER_RESPONSE  19
>>>>  #define GHCB_HYPERVISOR_FEATURES_REQUEST   128
>>>>  #define GHCB_HYPERVISOR_FEATURES_RESPONSE  129
>>>>  #define GHCB_INFO_TERMINATE_REQUEST        256
>>>>
>>> The number match the spec (2.0), but I have some remarks / questions.
>>>
>>> (1) Patch #2 (SVM_EXIT_HYPERVISOR_FEATURES) and this patch
>>> (GHCB_INFO_GHCB_GPA_REGISTER_REQUEST) break the nice alignments of the
>>> macro values (replacement texts) in both header files. Can you prepend a
>>> whitespace-only patch that simply moves the affected "columns" to the
>>> right far enough?
> 
> Sure, do you want me to the post after all the new VMGEXIT's are defined ?

Optimally, you should please look at the header file at the end of the
series, and determine the new starting *character* column for the macro
replacement texts. Then, at the very beginning of the series, pad
everything to that column. This way, you only need to adjust the
whitespace once (every macro addition will then fit nicely in place
later on), and whenever you add a new macro, it will already have the
final amount of whitespace needed.

> 
> 
>>>
>>> (2) I've checked section 2.3.2 "GHCB GPA Registration" in the spec
>>> (2.0). What is the specific risk of allowing a guest to switch from one
>>> GHCB address to another?
> 
> The GHCB is a shared page, there is no risk to switch from one page to
> another. This feature is designed to simplify some of the hypervisor
> implementation. Since the GHCB is accessed on every vmgexit, a
> hypervisor may prefer to create a map during the registration and refer
> the map instead of creating a new mapping on every vmgexit.

OK. So my comment in return is not for the patch set, but the spec: I
think this motivation should be highlighted in the spec. "Some
hypervisors may prefer" is vague. Prefer that for what? "Simplicity of
implementation" is a good answer (eliminate new mappings on every exit),
but it should be explained (perhaps in informative / non-normative text).

> 
> 
>>>
>>> (3) It seems strange to expect that a guest stick with a particular GHCB
>>> address for its entire lifetime (including firmware and OS) -- in fact
>>> OVMF already uses multiple GHCB addresses. The spec does not explain how
>>> the guest can "unlock" (de-register) a registered GHCB address.
>>> Furthermore, if a guest can do that *at all* (which I think it must --
>>> we're already using different GHCB addresses between SEC and DXE, for
>>> example), then what protection does the *temporary* locking of the GHCB
>>> address provide?
> 
> The spec does not force that GHCB should *never* change once registered.
> It says that before switching to new GHCB page, the guest must register
> the page. As you rightly said that OVMF uses multiple GHCBs from SEC to
> DXE. There is no unregister, registering a new GHCB is a hint to
> hypervisor that it should drop the old GHCB mapping. The GHCB
> registration is not a PSP function, and are not designed to mitigate a
> security exploits. It is purely a hypevisor virtualized feature.

Yes, very reasonable; it's a new "paravirt op" basically, where the
guest provides additional info to the hypervisor, for performance
optimization (or simplicity of code / implementation). That motivation
should be clarified.

> 
> 
>>> I'll stop reviewing here, because I think I need to understand your
>>> answers. I'd like to have a rudimentary mental basis for reviewing the rest.
>> ... interestingly, with reference to my question (2) under patch "RFC v2
>> 02/28", the GHCB GPA registration function is one that can *only* be
>> performed with the GHCB MSR protocol, and not through the GHCB page.
>>
>> So that shows that the MSR protocol's functions cannot be considered a
>> pure subset of the GHCB page's functions. If
>> SVM_EXIT_HYPERVISOR_FEATURES didn't exist (and the same function would
>> only be accessible via GHCB_HYPERVISOR_FEATURES_REQUEST), then no
>> "larger principle" would be damaged.
> 
> That is correct, not every exit have both MSR and non MSR protocol based
> vmgexit. It seems that during the spec review no other HV vendor saw the
> need for non-MSR based exit. Certainly, I don't see a need for it in KVM
> and can't comment on other HV ;)

I think a general comment that there's no intent to make either access
method a subset of the other could be helpful. Personally I don't mind
if an interface spec grows organically (i.e. if something is not
specified because people have never needed it). I just didn't know what
to expect.

Also, I'm sorry that I'm looking at the new version(s) of the spec only
now. I can usually deal with abstract interfaces only when there's code
and actual use cases.

Thanks
Laszlo



> 
> 
>>
>> Thanks
>> Laszlo
>>
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#74704): https://edk2.groups.io/g/devel/message/74704
Mute This Topic: https://groups.io/mt/82479050/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 03/28] MdePkg: Define the GHCB GPA structure

Posted by Laszlo Ersek 1 month, 2 weeks ago
On 05/03/21 15:50, Laszlo Ersek wrote:
> On 05/03/21 14:55, Brijesh Singh wrote:
>>
>> On 5/3/21 7:19 AM, Laszlo Ersek wrote:
>>> On 05/03/21 12:24, Laszlo Ersek wrote:
>>>> 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%7C9eac9a93753d403dcc4d08d90e2dbcb5%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637556411874265560%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=eNafEGfhCMOkOboQOJnxq8Rw%2BOTuvAUGIziDuELV8%2Bk%3D&amp;reserved=0
>>>>>
>>>>> An SEV-SNP guest is required to perform the GHCB GPA registration. See
>>>>> the GHCB specification for further 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 +++++++
>>>>>  1 file changed, 7 insertions(+)
>>>>>
>>>>> diff --git a/MdePkg/Include/Register/Amd/Fam17Msr.h b/MdePkg/Include/Register/Amd/Fam17Msr.h
>>>>> index a65d51ab12..e19bd04b6c 100644
>>>>> --- a/MdePkg/Include/Register/Amd/Fam17Msr.h
>>>>> +++ b/MdePkg/Include/Register/Amd/Fam17Msr.h
>>>>> @@ -53,6 +53,11 @@ typedef union {
>>>>>      UINT64  Features:52;
>>>>>    } GhcbHypervisorFeatures;
>>>>>  
>>>>> +  struct {
>>>>> +    UINT64  Function:12;
>>>>> +    UINT64  GuestFrameNumber:52;
>>>>> +  } GhcbGpaRegister;
>>>>> +
>>>>>    VOID    *Ghcb;
>>>>>  
>>>>>    UINT64  GhcbPhysicalAddress;
>>>>> @@ -62,6 +67,8 @@ typedef union {
>>>>>  #define GHCB_INFO_SEV_INFO_GET             2
>>>>>  #define GHCB_INFO_CPUID_REQUEST            4
>>>>>  #define GHCB_INFO_CPUID_RESPONSE           5
>>>>> +#define GHCB_INFO_GHCB_GPA_REGISTER_REQUEST   18
>>>>> +#define GHCB_INFO_GHCB_GPA_REGISTER_RESPONSE  19
>>>>>  #define GHCB_HYPERVISOR_FEATURES_REQUEST   128
>>>>>  #define GHCB_HYPERVISOR_FEATURES_RESPONSE  129
>>>>>  #define GHCB_INFO_TERMINATE_REQUEST        256
>>>>>
>>>> The number match the spec (2.0), but I have some remarks / questions.
>>>>
>>>> (1) Patch #2 (SVM_EXIT_HYPERVISOR_FEATURES) and this patch
>>>> (GHCB_INFO_GHCB_GPA_REGISTER_REQUEST) break the nice alignments of the
>>>> macro values (replacement texts) in both header files. Can you prepend a
>>>> whitespace-only patch that simply moves the affected "columns" to the
>>>> right far enough?
>>
>> Sure, do you want me to the post after all the new VMGEXIT's are defined ?
> 
> Optimally, you should please look at the header file at the end of the
> series, and determine the new starting *character* column for the macro
> replacement texts. Then, at the very beginning of the series, pad
> everything to that column. This way, you only need to adjust the
> whitespace once (every macro addition will then fit nicely in place
> later on), and whenever you add a new macro, it will already have the
> final amount of whitespace needed.

Ultimately, with the whitespace fixed (1):

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo

> 
>>
>>
>>>>
>>>> (2) I've checked section 2.3.2 "GHCB GPA Registration" in the spec
>>>> (2.0). What is the specific risk of allowing a guest to switch from one
>>>> GHCB address to another?
>>
>> The GHCB is a shared page, there is no risk to switch from one page to
>> another. This feature is designed to simplify some of the hypervisor
>> implementation. Since the GHCB is accessed on every vmgexit, a
>> hypervisor may prefer to create a map during the registration and refer
>> the map instead of creating a new mapping on every vmgexit.
> 
> OK. So my comment in return is not for the patch set, but the spec: I
> think this motivation should be highlighted in the spec. "Some
> hypervisors may prefer" is vague. Prefer that for what? "Simplicity of
> implementation" is a good answer (eliminate new mappings on every exit),
> but it should be explained (perhaps in informative / non-normative text).
> 
>>
>>
>>>>
>>>> (3) It seems strange to expect that a guest stick with a particular GHCB
>>>> address for its entire lifetime (including firmware and OS) -- in fact
>>>> OVMF already uses multiple GHCB addresses. The spec does not explain how
>>>> the guest can "unlock" (de-register) a registered GHCB address.
>>>> Furthermore, if a guest can do that *at all* (which I think it must --
>>>> we're already using different GHCB addresses between SEC and DXE, for
>>>> example), then what protection does the *temporary* locking of the GHCB
>>>> address provide?
>>
>> The spec does not force that GHCB should *never* change once registered.
>> It says that before switching to new GHCB page, the guest must register
>> the page. As you rightly said that OVMF uses multiple GHCBs from SEC to
>> DXE. There is no unregister, registering a new GHCB is a hint to
>> hypervisor that it should drop the old GHCB mapping. The GHCB
>> registration is not a PSP function, and are not designed to mitigate a
>> security exploits. It is purely a hypevisor virtualized feature.
> 
> Yes, very reasonable; it's a new "paravirt op" basically, where the
> guest provides additional info to the hypervisor, for performance
> optimization (or simplicity of code / implementation). That motivation
> should be clarified.
> 
>>
>>
>>>> I'll stop reviewing here, because I think I need to understand your
>>>> answers. I'd like to have a rudimentary mental basis for reviewing the rest.
>>> ... interestingly, with reference to my question (2) under patch "RFC v2
>>> 02/28", the GHCB GPA registration function is one that can *only* be
>>> performed with the GHCB MSR protocol, and not through the GHCB page.
>>>
>>> So that shows that the MSR protocol's functions cannot be considered a
>>> pure subset of the GHCB page's functions. If
>>> SVM_EXIT_HYPERVISOR_FEATURES didn't exist (and the same function would
>>> only be accessible via GHCB_HYPERVISOR_FEATURES_REQUEST), then no
>>> "larger principle" would be damaged.
>>
>> That is correct, not every exit have both MSR and non MSR protocol based
>> vmgexit. It seems that during the spec review no other HV vendor saw the
>> need for non-MSR based exit. Certainly, I don't see a need for it in KVM
>> and can't comment on other HV ;)
> 
> I think a general comment that there's no intent to make either access
> method a subset of the other could be helpful. Personally I don't mind
> if an interface spec grows organically (i.e. if something is not
> specified because people have never needed it). I just didn't know what
> to expect.
> 
> Also, I'm sorry that I'm looking at the new version(s) of the spec only
> now. I can usually deal with abstract interfaces only when there's code
> and actual use cases.
> 
> Thanks
> Laszlo
> 
> 
> 
>>
>>
>>>
>>> Thanks
>>> Laszlo
>>>
>>
> 
> 
> 
> 
> 
> 



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