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 D30742034D8C0 for ; Thu, 22 Feb 2018 04:02:08 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.rdu2.redhat.com [10.11.54.4]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id F3F65857A1; Thu, 22 Feb 2018 12:08:07 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-138.rdu2.redhat.com [10.10.120.138]) by smtp.corp.redhat.com (Postfix) with ESMTP id F01472026E0E; Thu, 22 Feb 2018 12:08:04 +0000 (UTC) To: Brijesh Singh , edk2-devel@lists.01.org Cc: Tom Lendacky , Jordan Justen , Ard Biesheuvel , Michael Kinney , Paolo Bonzini References: <20180221165212.6643-1-brijesh.singh@amd.com> <20180221165212.6643-3-brijesh.singh@amd.com> From: Laszlo Ersek Message-ID: <6a0cd77f-13d8-b8dd-8ad2-931347e72a7c@redhat.com> Date: Thu, 22 Feb 2018 13:08:04 +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: <20180221165212.6643-3-brijesh.singh@amd.com> X-Scanned-By: MIMEDefang 2.78 on 10.11.54.4 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.2]); Thu, 22 Feb 2018 12:08:08 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.2]); Thu, 22 Feb 2018 12:08:08 +0000 (UTC) for IP:'10.11.54.4' DOMAIN:'int-mx04.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'lersek@redhat.com' RCPT:'' Subject: Re: [PATCH 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: Thu, 22 Feb 2018 12:02:09 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 02/21/18 17:52, Brijesh Singh wrote: > Commit:24e4ad7 (OvmfPkg: Add AmdSevDxe driver) added a driver which runs > early in PEI phase and clears the C-bit from all MMIO regions (including s/PEI/DXE/ > Qemu Flash). 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. The problem statement is good (including the comment in the code). However, I would prefer if we could reflect the full AmdSevDxe logic to the SMM page tables. In other words, when -- or shortly after -- the SMM page tables are built, we should clear the C-bit in all those PTEs that cover known MMIO and as-yet NonExistent memory ranges. We already have a bunch of "mAddressEncMask" usage in PiSmmCpuDxeSmm. Can we investigate this a bit? If it turns out to be impossible, I guess I might be OK with this patch. I have more comments: > > 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/FwBlockService.h | 5 +++ > OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c | 5 +++ > OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c | 10 ++++++ > OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceSmm.c | 35 ++++++++++++++++++++ > 5 files changed, 56 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/FwBlockService.h b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.h > index 1f9287b08769..704ed477ba14 100644 > --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.h > +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.h > @@ -189,4 +189,9 @@ VOID > InstallVirtualAddressChangeHandler ( > VOID > ); > + > +VOID > +FvbBeforeFlashProbe ( > + VOID > + ); > #endif Please drop the "Fvb" prefix; this function is not an FVB protocol member. > diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c > index 558b395dff4a..b7b9bf1fb8d9 100644 > --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c > +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c > @@ -967,6 +967,11 @@ FvbInitialize ( > UINTN NumOfBlocks; > RETURN_STATUS PcdStatus; > > + // > + // execute platform specific hooks before probing the flash > + // > + FvbBeforeFlashProbe (); > + > if (EFI_ERROR (QemuFlashInitialize ())) { > // > // Return an error so image will be unloaded > diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c > index 63b308658e36..7d274c08ad12 100644 > --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c > +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c > @@ -155,3 +155,13 @@ InstallVirtualAddressChangeHandler ( > ); > ASSERT_EFI_ERROR (Status); > } > + > +VOID > +FvbBeforeFlashProbe ( > + VOID > + ) > +{ > + // > + // Do nothing > + // > +} > diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceSmm.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceSmm.c > index e0617f2503a2..d97b13f47bf7 100644 > --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceSmm.c > +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceSmm.c > @@ -17,6 +17,7 @@ > #include > #include > #include > +#include > #include > #include > > @@ -67,3 +68,37 @@ InstallVirtualAddressChangeHandler ( > // Nothing. > // > } > + > +VOID > +FvbBeforeFlashProbe ( > + VOID > + ) > +{ > + > + ASSERT (FeaturePcdGet (PcdSmmSmramRequire)); > + > + // > + // When SEV is enabled, AmdSevDxe runs early in PEI phase and clears the C-bit s/PEI/DXE/ > + // from the MMIO space (including flash ranges) but the driver runs in non SMM > + // 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. > + // > + if (MemEncryptSevIsEnabled ()) { > + EFI_STATUS Status; > + EFI_PHYSICAL_ADDRESS BaseAddress; > + UINTN FdBlockSize, FdBlockCount; > + > + BaseAddress = (EFI_PHYSICAL_ADDRESS) PcdGet32 (PcdOvmfFdBaseAddress); > + FdBlockSize = PcdGet32 (PcdOvmfFirmwareBlockSize); > + FdBlockCount = PcdGet32 (PcdOvmfFirmwareFdSize) / FdBlockSize; > + > + Status = MemEncryptSevClearPageEncMask ( > + 0, > + BaseAddress, > + EFI_SIZE_TO_PAGES (FdBlockSize * FdBlockCount), > + FALSE > + ); > + ASSERT_EFI_ERROR (Status); > + } > +} > I think it would be better to hook this logic into QemuFlashInitialize(). That function already computes mFlashBase, mFdBlockSize and mFdBlockCount. Right before the call to QemuFlashDetected(), we could call BeforeFlashProbe(). The latter could take the base address, the block size and count as parameters, or just use the global variables. But, again, my preference would be to mirror the AmdSevDxe logic into (or right after) the SMM page table setup. Perhaps that can be done in SmmCpuFeaturesInitializeProcessor(), when IsMonarch is TRUE -- this function is called from SmmInitHandler(), and at that point, the SMM page tables are already in use. (See above the SmmInitHandler() call site in "UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmInit.nasm".) Thanks, Laszlo