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 C51EB211F8883 for ; Fri, 6 Jul 2018 04:35:10 -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 D8529818A6BF; Fri, 6 Jul 2018 11:35:09 +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 6B4872026D68; Fri, 6 Jul 2018 11:35:07 +0000 (UTC) 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> From: Laszlo Ersek Message-ID: Date: Fri, 6 Jul 2018 13:35:06 +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: <1530817945-8030-3-git-send-email-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.8]); Fri, 06 Jul 2018 11:35:09 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.8]); Fri, 06 Jul 2018 11:35:09 +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:35:11 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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. Thanks Laszlo