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

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

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