From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=66.187.233.73; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 07E9422402DF1 for ; Wed, 28 Feb 2018 11:00:27 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 4D50817CEF5; Wed, 28 Feb 2018 19:06:34 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-11.rdu2.redhat.com [10.10.120.11]) by smtp.corp.redhat.com (Postfix) with ESMTP id C2064AFD5A; Wed, 28 Feb 2018 19:06:32 +0000 (UTC) To: Brijesh Singh , edk2-devel@lists.01.org Cc: Tom Lendacky , Paolo Bonzini , Michael Kinney , Jordan Justen , Ard Biesheuvel References: <20180228161415.28723-1-brijesh.singh@amd.com> <20180228161415.28723-2-brijesh.singh@amd.com> From: Laszlo Ersek Message-ID: <7bfa8782-d85d-e2bb-e515-ae6dd1b37275@redhat.com> Date: Wed, 28 Feb 2018 20:06:31 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 MIME-Version: 1.0 In-Reply-To: <20180228161415.28723-2-brijesh.singh@amd.com> X-Scanned-By: MIMEDefang 2.79 on 10.11.54.5 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.2]); Wed, 28 Feb 2018 19:06:34 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.2]); Wed, 28 Feb 2018 19:06:34 +0000 (UTC) for IP:'10.11.54.5' DOMAIN:'int-mx05.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'lersek@redhat.com' RCPT:'' Subject: Re: [PATCH v2 1/2] OvmfPkg/AmdSevDxe: Clear the C-bit from SMM Saved State X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 28 Feb 2018 19:00:28 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Hi Brijesh, On 02/28/18 17:14, Brijesh Singh wrote: > When OVMF is built with SMM, SMMSaved State area (SMM_DEFAULT_SMBASE + > SMRAM_SAVE_STATE_MAP_OFFSET) contains data which need to be accessed by > both guest and hypervisor. Since the data need to be accessed by both > hence we must map the SMMSaved State area as unencrypted (i.e C-bit > cleared). > > This patch clears the SavedStateArea address before SMBASE relocation. > Currently, we do not clear the SavedStateArea address after SMBASE is > relocated due to the following reasons: > > 1) Guest BIOS never access the relocated SavedStateArea. > > 2) The C-bit works on page-aligned address, but the SavedStateArea > address is not a page-aligned. Theoretically, we could roundup the address > and clear the C-bit of aligned address but looking carefully we found > that some portion of the page contains code -- which will causes a bigger > issue for the SEV guest. When SEV is enabled, all the code must be > encrypted otherwise hardware will cause trap. > > Cc: Jordan Justen > Cc: Laszlo Ersek > Cc: Ard Biesheuvel > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Brijesh Singh > --- > OvmfPkg/AmdSevDxe/AmdSevDxe.inf | 4 +++ > OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf | 1 + > OvmfPkg/AmdSevDxe/AmdSevDxe.c | 35 ++++++++++++++++++++ > OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c | 21 ++++++++++++ > 4 files changed, 61 insertions(+) I've been staring at this patch for ~2 hours now (I've also read your other email). I like this approach (and the comments / commit message), but IMO an important detail is missing. I started writing up my notes, but the list got very long. Is it OK with you if I send my ideas as a patch set (replacing just this patch)? I think I'd like to turn this patch into 3-4 patches. Thanks! Laszlo > diff --git a/OvmfPkg/AmdSevDxe/AmdSevDxe.inf b/OvmfPkg/AmdSevDxe/AmdSevDxe.inf > index 41635a57a454..162ed98a2fbe 100644 > --- a/OvmfPkg/AmdSevDxe/AmdSevDxe.inf > +++ b/OvmfPkg/AmdSevDxe/AmdSevDxe.inf > @@ -29,6 +29,7 @@ [Packages] > MdePkg/MdePkg.dec > MdeModulePkg/MdeModulePkg.dec > OvmfPkg/OvmfPkg.dec > + UefiCpuPkg/UefiCpuPkg.dec > > [LibraryClasses] > BaseLib > @@ -41,3 +42,6 @@ [LibraryClasses] > > [Depex] > TRUE > + > +[FeaturePcd] > + gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire > diff --git a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf > index 31edf3a9c1fd..ba564abb787b 100644 > --- a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf > +++ b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf > @@ -36,3 +36,4 @@ [LibraryClasses] > PcdLib > DebugLib > SmmServicesTableLib > + MemEncryptSevLib > diff --git a/OvmfPkg/AmdSevDxe/AmdSevDxe.c b/OvmfPkg/AmdSevDxe/AmdSevDxe.c > index e472096320ea..5803e8655049 100644 > --- a/OvmfPkg/AmdSevDxe/AmdSevDxe.c > +++ b/OvmfPkg/AmdSevDxe/AmdSevDxe.c > @@ -25,6 +25,8 @@ > #include > #include > #include > +#include > +#include > > EFI_STATUS > EFIAPI > @@ -71,5 +73,38 @@ AmdSevDxeEntryPoint ( > FreePool (AllDescMap); > } > > + // > + // When SMM is enabled, clear the C-bit from SMM Saved State Area > + // > + // NOTES: The SavedStateArea address cleared here is before SMBASE > + // relocation. Currently, we do not clear the SavedStateArea address after > + // SMBASE is relocated due to the following reasons: > + // > + // 1) Guest BIOS never access the relocated SavedStateArea. > + // > + // 2) The C-bit works on page-aligned address, but the SavedStateArea > + // address is not a page-aligned. Theoretically, we could roundup the address > + // and clear the C-bit of aligned address but looking carefully we found > + // that some portion of the page contains code -- which will causes a bigger > + // issues for SEV guest. When SEV is enabled, all the code must be encrypted > + // otherwise hardware will cause trap. > + // > + // We restore the C-bit for this SMM Saved State Area after SMBASE relocation > + // is completed (See OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c). > + // > + if (FeaturePcdGet (PcdSmmSmramRequire)) { > + EFI_PHYSICAL_ADDRESS SmmSavedStateAreaAddress; > + > + SmmSavedStateAreaAddress = SMM_DEFAULT_SMBASE + SMRAM_SAVE_STATE_MAP_OFFSET; > + > + Status = MemEncryptSevClearPageEncMask ( > + 0, > + SmmSavedStateAreaAddress, > + EFI_SIZE_TO_PAGES (sizeof(QEMU_SMRAM_SAVE_STATE_MAP)), > + FALSE > + ); > + ASSERT_EFI_ERROR (Status); > + } > + > return EFI_SUCCESS; > } > diff --git a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c > index a307f64c9c61..946294701c62 100644 > --- a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c > +++ b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c > @@ -20,6 +20,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > #include > #include > #include > +#include > #include > > // > @@ -183,6 +184,26 @@ SmmCpuFeaturesSmmRelocationComplete ( > VOID > ) > { > + EFI_STATUS Status; > + EFI_PHYSICAL_ADDRESS SmmSavedStateAreaAddress; > + > + // > + // When SEV is enabled, the SMM SavedState is mapped with C=0 > + // (See OvmfPkg/AmdSevDxe/AmdSevDxe.c). Now the SMBASE is relocated hence we > + // remap the address with C=1. > + // > + if (!MemEncryptSevIsEnabled ()) { > + return; > + } > + > + SmmSavedStateAreaAddress = SMM_DEFAULT_SMBASE + SMRAM_SAVE_STATE_MAP_OFFSET; > + Status = MemEncryptSevSetPageEncMask ( > + 0, > + SmmSavedStateAreaAddress, > + EFI_SIZE_TO_PAGES (sizeof(QEMU_SMRAM_SAVE_STATE_MAP)), > + FALSE > + ); > + ASSERT_EFI_ERROR (Status); > } > > /** >