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

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

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&data=04%7C01%7Cbrijesh.singh%40amd.com%7C9eac9a93753d403dcc4d08d90e2dbcb5%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637556411874265560%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=eNafEGfhCMOkOboQOJnxq8Rw%2BOTuvAUGIziDuELV8%2Bk%3D&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
>>>
>>
> 
> 
> 
> 
> 
>