From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mx.groups.io with SMTP id smtpd.web10.34032.1620050148414751964 for ; Mon, 03 May 2021 06:55:48 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=SgBDiUXl; spf=pass (domain: redhat.com, ip: 170.10.133.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1620050147; h=from:from:reply-to:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=FtPXDUoM6I3a3pDF6iDJB203ntP38U6nlepbJAAz4wU=; b=SgBDiUXlTAoljB1RAtYtvpQgmvRpaCbjZNG0LRtLmfnJJ9OaW19p0dCSO2K1IaK/HbLVAm +rHiBr1nUe9rqnvR3BBxdV9I6ekLtAj2NyCAmMA5E8y9v+Yl+2fb0H8XfeuD/5l0IEllEn dPjZuxtTSn1WIvK7bgj0EWakddCTL4M= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-448-BqtZXeF_P_mLmspRpFZhsw-1; Mon, 03 May 2021 09:55:44 -0400 X-MC-Unique: BqtZXeF_P_mLmspRpFZhsw-1 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 7E5DD107ACCA; Mon, 3 May 2021 13:55:43 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-114-8.ams2.redhat.com [10.36.114.8]) by smtp.corp.redhat.com (Postfix) with ESMTP id 5A01619C79; Mon, 3 May 2021 13:55:41 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH RFC v2 03/28] MdePkg: Define the GHCB GPA structure From: "Laszlo Ersek" To: Brijesh Singh , devel@edk2.groups.io Cc: James Bottomley , Min Xu , Jiewen Yao , Tom Lendacky , Jordan Justen , Ard Biesheuvel , Erdem Aktas Reply-To: devel@edk2.groups.io, lersek@redhat.com References: <20210430115148.22267-1-brijesh.singh@amd.com> <20210430115148.22267-4-brijesh.singh@amd.com> <9a880649-bd19-589d-464e-f3cf54bebc3e@redhat.com> Message-ID: Date: Mon, 3 May 2021 15:55:40 +0200 MIME-Version: 1.0 In-Reply-To: <9a880649-bd19-589d-464e-f3cf54bebc3e@redhat.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 >>>>> Cc: Min Xu >>>>> Cc: Jiewen Yao >>>>> Cc: Tom Lendacky >>>>> Cc: Jordan Justen >>>>> Cc: Ard Biesheuvel >>>>> Cc: Laszlo Ersek >>>>> Cc: Erdem Aktas >>>>> Signed-off-by: Brijesh Singh >>>>> --- >>>>> 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 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 >>> >> > > > > > >