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 <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 +++++++ >> 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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
