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 671EB22402DEB for ; Wed, 28 Feb 2018 11:35:25 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.rdu2.redhat.com [10.11.54.3]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id F0C7FEAEA0; Wed, 28 Feb 2018 19:41:31 +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 7C07210AF9CA; Wed, 28 Feb 2018 19:41:30 +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-3-brijesh.singh@amd.com> From: Laszlo Ersek Message-ID: Date: Wed, 28 Feb 2018 20:41:29 +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-3-brijesh.singh@amd.com> X-Scanned-By: MIMEDefang 2.78 on 10.11.54.3 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.1]); Wed, 28 Feb 2018 19:41:32 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.1]); Wed, 28 Feb 2018 19:41:32 +0000 (UTC) for IP:'10.11.54.3' DOMAIN:'int-mx03.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'lersek@redhat.com' RCPT:'' Subject: Re: [PATCH v2 2/2] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Clear C-bit when SEV is active 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:35:25 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 02/28/18 17:14, Brijesh Singh wrote: > Commit:24e4ad7 (OvmfPkg: Add AmdSevDxe driver) added a driver which runs > early in DXE phase and clears the C-bit from all MMIO regions (including > Qemu Flash). (1) This appears incorrect / inexact; AmdSevDxe is dispatched from APRIORI DXE before the flash driver is dispatched, and the MMIO GCD entry is only added by the flash driver. So in this case, AmdSevDxe clears the C-bit on a NonExistent entry that will later be split and accommodate the flash MMIO range. > When SMM is enabled, we build two sets of page tables; first > page table is used when executing code in non SMM mode (SMM-less-pgtable) > and second page table is used when we are executing code in SMM mode > (SMM-pgtable). > > During boot time, AmdSevDxe driver clears the C-bit from the > SMM-less-pgtable. But when SMM is enabled, Qemu Flash services are used > from SMM mode. > > In this patch we explicitly clear the C-bit from Qemu flash MMIO range > before we probe the flash. When OVMF is built with SMM_REQUIRE then > call to initialize the flash services happen after the SMM-pgtable is > created and processor is serving the first SMI. At this time we will > have access to the SMM-pgtable. (2) Please replace "is serving" with "has served". > > Cc: Jordan Justen > Cc: Laszlo Ersek > Cc: Ard Biesheuvel > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Brijesh Singh > --- > OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesSmm.inf | 1 + > OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.h | 7 +++++ > OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c | 12 +++++++ > OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceSmm.c | 33 ++++++++++++++++++++ > OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c | 6 ++++ > 5 files changed, 59 insertions(+) > > diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesSmm.inf b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesSmm.inf > index ba2d3679a46d..d365e27cbe59 100644 > --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesSmm.inf > +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesSmm.inf > @@ -53,6 +53,7 @@ [LibraryClasses] > DevicePathLib > DxeServicesTableLib > MemoryAllocationLib > + MemEncryptSevLib > PcdLib > SmmServicesTableLib > UefiBootServicesTableLib > diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.h b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.h > index 8d83dca7a52c..6c4099c140e8 100644 > --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.h > +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.h > @@ -88,5 +88,12 @@ QemuFlashConvertPointers ( > VOID > ); > > +VOID > +BeforeFlashProbe ( > + EFI_PHYSICAL_ADDRESS BaseAddress, > + UINTN FdBlockSize, > + UINTN FdBlockCount > + ); > + > #endif (3) Sorry that I'm again requesting a name change for this function. Can we call it QemuFlashBeforeProbe()? To be consistent with the other function names in this header file. (4) Please add "IN" decorators (also to the function definitions). > > diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c > index 63b308658e36..a4614de3c901 100644 > --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c > +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c > @@ -155,3 +155,15 @@ InstallVirtualAddressChangeHandler ( > ); > ASSERT_EFI_ERROR (Status); > } > + > +VOID > +BeforeFlashProbe ( > + EFI_PHYSICAL_ADDRESS BaseAddress, > + UINTN FdBlockSize, > + UINTN FdBlockCount > + ) > +{ > + // > + // Do nothing > + // > +} (5) This function definition should go into the existent file "QemuFlashDxe.c". > diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceSmm.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceSmm.c > index e0617f2503a2..a6cad5af223b 100644 > --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceSmm.c > +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceSmm.c > @@ -17,6 +17,7 @@ > #include > #include > #include > +#include > #include > #include > > @@ -67,3 +68,35 @@ InstallVirtualAddressChangeHandler ( > // Nothing. > // > } > + > +VOID > +BeforeFlashProbe ( > + EFI_PHYSICAL_ADDRESS BaseAddress, > + UINTN FdBlockSize, > + UINTN FdBlockCount > + ) > +{ > + EFI_STATUS Status; > + > + ASSERT (FeaturePcdGet (PcdSmmSmramRequire)); > + > + if (!MemEncryptSevIsEnabled()) { > + return; > + } > + > + // > + // When SEV is enabled, AmdSevDxe runs early in DXE phase and clears the C-bit > + // from the MMIO space (including flash ranges) but the driver runs in non SMM (6) Please update the comment according to (1). > + // context hence it cleared the flash ranges from non SMM page table. > + // When SMM is enabled, the flash services are accessed from the SMM mode > + // hence we explicitly clear the C-bit on flash ranges from SMM page table. > + // > + > + Status = MemEncryptSevClearPageEncMask ( > + 0, > + BaseAddress, > + EFI_SIZE_TO_PAGES (FdBlockSize * FdBlockCount), > + FALSE > + ); (7) The closing paren is not indented correctly, it should be aligned with the arguments. > + ASSERT_EFI_ERROR (Status); > +} (8) This function definition should go into a new file called "QemuFlashSmm.c" -- please make sure you add a license block at the top, and use CRLF line endings --, and the new file should be added to "FvbServicesSmm.inf". > diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c > index 5677b5ee119c..f63e11723415 100644 > --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c > +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c > @@ -244,6 +244,12 @@ QemuFlashInitialize ( > ASSERT(PcdGet32 (PcdOvmfFirmwareFdSize) % mFdBlockSize == 0); > mFdBlockCount = PcdGet32 (PcdOvmfFirmwareFdSize) / mFdBlockSize; > > + // > + // execute platform specific hooks before probing the flash > + // (9) Please replace "platform" with "module type". > + BeforeFlashProbe ((EFI_PHYSICAL_ADDRESS)(UINTN) mFlashBase, > + mFdBlockSize, mFdBlockCount); > + (10) The indentation is not idiomatic. > if (!QemuFlashDetected ()) { > ASSERT (!FeaturePcdGet (PcdSmmSmramRequire)); > return EFI_WRITE_PROTECTED; > I think this patch is good, just a few warts left to clean up. Thanks! Laszlo