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 <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? >> >> 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 <ler...@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 >>> >> > > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#74705): https://edk2.groups.io/g/devel/message/74705 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] -=-=-=-=-=-=-=-=-=-=-=-
