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

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

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