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&data=04%7C01%7Cbrijesh.singh%40amd.com%7C9a7e31fbf85043c6ee8508d90e1ba94d%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637556334239842920%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=fSThw3T7P4LcLhcZz9tfy4ZB1Y7Zny0BzwA2jTyWAkY%3D&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 <j...@linux.ibm.com> >> Cc: Min Xu <min.m...@intel.com> >> Cc: Jiewen Yao <jiewen....@intel.com> >> Cc: Tom Lendacky <thomas.lenda...@amd.com> >> Cc: Jordan Justen <jordan.l.jus...@intel.com> >> Cc: Ard Biesheuvel <ardb+tianoc...@kernel.org> >> Cc: Laszlo Ersek <ler...@redhat.com> >> Cc: Erdem Aktas <erdemak...@google.com> >> Signed-off-by: Brijesh Singh <brijesh.si...@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 <ler...@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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-