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 C916B203369CB for ; Fri, 6 Jul 2018 04:48:35 -0700 (PDT) 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 CABB381663EC; Fri, 6 Jul 2018 11:48:34 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-154.rdu2.redhat.com [10.10.120.154]) by smtp.corp.redhat.com (Postfix) with ESMTP id 721B42026D68; Fri, 6 Jul 2018 11:48:33 +0000 (UTC) From: Laszlo Ersek To: Brijesh Singh , edk2-devel@lists.01.org Cc: Lendacky Thomas , Ard Biesheuvel , Anthony Perard , Julien Grall , Justen Jordan L References: <1530817945-8030-1-git-send-email-brijesh.singh@amd.com> <1530817945-8030-3-git-send-email-brijesh.singh@amd.com> Message-ID: Date: Fri, 6 Jul 2018 13:48:32 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: 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.8]); Fri, 06 Jul 2018 11:48:34 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.8]); Fri, 06 Jul 2018 11:48:34 +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 v3 2/3] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Do not expose MMIO in SMM build X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.27 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 06 Jul 2018 11:48:36 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 07/06/18 13:35, Laszlo Ersek wrote: > On 07/05/18 21:12, Brijesh Singh wrote: >> In the SMM build, only an SMM driver is using the address range hence we >> do not need to expose the flash MMIO range in EFI runtime mapping. >> >> Cc: Ard Biesheuvel >> Cc: Anthony Perard >> Cc: Julien Grall >> Cc: Justen Jordan L >> Cc: Laszlo Ersek >> Contributed-under: TianoCore Contribution Agreement 1.1 >> Signed-off-by: Brijesh Singh >> --- >> .../FwBlockService.c | 50 --------------------- >> .../FwBlockService.h | 7 +++ >> .../FwBlockServiceDxe.c | 51 ++++++++++++++++++++++ >> .../FwBlockServiceSmm.c | 13 ++++++ >> 4 files changed, 71 insertions(+), 50 deletions(-) >> >> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c >> index 28499991a43c..eec8b1b1ae9d 100644 >> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c >> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c >> @@ -831,56 +831,6 @@ ValidateFvHeader ( >> >> STATIC >> EFI_STATUS >> -MarkIoMemoryRangeForRuntimeAccess ( >> - EFI_PHYSICAL_ADDRESS BaseAddress, >> - UINTN Length >> - ) >> -{ >> - EFI_STATUS Status; >> - EFI_GCD_MEMORY_SPACE_DESCRIPTOR GcdDescriptor; >> - >> - // >> - // Mark flash region as runtime memory >> - // >> - Status = gDS->RemoveMemorySpace ( >> - BaseAddress, >> - Length >> - ); >> - >> - Status = gDS->AddMemorySpace ( >> - EfiGcdMemoryTypeMemoryMappedIo, >> - BaseAddress, >> - Length, >> - EFI_MEMORY_UC | EFI_MEMORY_RUNTIME >> - ); >> - ASSERT_EFI_ERROR (Status); >> - >> - Status = gDS->AllocateMemorySpace ( >> - AllocateAddress, >> - EfiGcdMemoryTypeMemoryMappedIo, >> - 0, >> - Length, >> - &BaseAddress, >> - gImageHandle, >> - NULL >> - ); >> - ASSERT_EFI_ERROR (Status); >> - >> - Status = gDS->GetMemorySpaceDescriptor (BaseAddress, &GcdDescriptor); >> - ASSERT_EFI_ERROR (Status); >> - >> - Status = gDS->SetMemorySpaceAttributes ( >> - BaseAddress, >> - Length, >> - GcdDescriptor.Attributes | EFI_MEMORY_RUNTIME >> - ); >> - ASSERT_EFI_ERROR (Status); >> - >> - return Status; >> -} >> - >> -STATIC >> -EFI_STATUS >> InitializeVariableFvHeader ( >> VOID >> ) >> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.h b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.h >> index 1f9287b08769..178f578d49f0 100644 >> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.h >> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.h >> @@ -189,4 +189,11 @@ VOID >> InstallVirtualAddressChangeHandler ( >> VOID >> ); >> + >> +EFI_STATUS >> +MarkIoMemoryRangeForRuntimeAccess ( >> + IN EFI_PHYSICAL_ADDRESS BaseAddress, >> + IN UINTN Length >> + ); >> + >> #endif >> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c >> index 63b308658e36..646427bf4e2c 100644 >> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c >> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c >> @@ -22,6 +22,8 @@ >> #include >> #include >> #include >> +#include >> +#include > > (1) Can you drop "MemoryAllocationLib.h"? I don't think it is needed. > > (2) Also, can you add the "DxeServicesTableLib.h" include directive so > that the Library #include list remains sorted? > >> >> #include "FwBlockService.h" >> #include "QemuFlash.h" >> @@ -155,3 +157,52 @@ InstallVirtualAddressChangeHandler ( >> ); >> ASSERT_EFI_ERROR (Status); >> } >> + >> +EFI_STATUS >> +MarkIoMemoryRangeForRuntimeAccess ( >> + EFI_PHYSICAL_ADDRESS BaseAddress, >> + UINTN Length >> + ) >> +{ >> + EFI_STATUS Status; >> + EFI_GCD_MEMORY_SPACE_DESCRIPTOR GcdDescriptor; >> + >> + // >> + // Mark flash region as runtime memory >> + // >> + Status = gDS->RemoveMemorySpace ( >> + BaseAddress, >> + Length >> + ); >> + >> + Status = gDS->AddMemorySpace ( >> + EfiGcdMemoryTypeMemoryMappedIo, >> + BaseAddress, >> + Length, >> + EFI_MEMORY_UC | EFI_MEMORY_RUNTIME >> + ); >> + ASSERT_EFI_ERROR (Status); >> + >> + Status = gDS->AllocateMemorySpace ( >> + AllocateAddress, >> + EfiGcdMemoryTypeMemoryMappedIo, >> + 0, >> + Length, >> + &BaseAddress, >> + gImageHandle, >> + NULL >> + ); >> + ASSERT_EFI_ERROR (Status); >> + >> + Status = gDS->GetMemorySpaceDescriptor (BaseAddress, &GcdDescriptor); >> + ASSERT_EFI_ERROR (Status); >> + >> + Status = gDS->SetMemorySpaceAttributes ( >> + BaseAddress, >> + Length, >> + GcdDescriptor.Attributes | EFI_MEMORY_RUNTIME >> + ); >> + ASSERT_EFI_ERROR (Status); >> + >> + return Status; >> +} > > (3) Please don't forget to update this function (post-move) as I > requested for patch #1. > >> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceSmm.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceSmm.c >> index e0617f2503a2..cdb073348158 100644 >> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceSmm.c >> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceSmm.c >> @@ -67,3 +67,16 @@ InstallVirtualAddressChangeHandler ( >> // Nothing. >> // >> } >> + >> +EFI_STATUS >> +MarkIoMemoryRangeForRuntimeAccess ( >> + EFI_PHYSICAL_ADDRESS BaseAddress, >> + UINTN Length >> + ) >> +{ >> + // >> + // Nothing >> + // >> + >> + return EFI_SUCCESS; >> +} >> > > Looks OK to me otherwise. Sorry, got another remark: (4) In "FwBlockService.h", you declare the function prototype -- very correctly -- with the "IN" parameter decorators. Can you please add the same "IN" decorators to both *definitions* of the function as well? Thank you very much! Laszlo