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 <ard.biesheu...@linaro.org> >> Cc: Anthony Perard <anthony.per...@citrix.com> >> Cc: Julien Grall <julien.gr...@linaro.org> >> Cc: Justen Jordan L <jordan.l.jus...@intel.com> >> Cc: Laszlo Ersek <ler...@redhat.com> >> Contributed-under: TianoCore Contribution Agreement 1.1 >> Signed-off-by: Brijesh Singh <brijesh.si...@amd.com> >> --- >> .../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 <Library/UefiRuntimeLib.h> >> #include <Protocol/DevicePath.h> >> #include <Protocol/FirmwareVolumeBlock.h> >> +#include <Library/DxeServicesTableLib.h> >> +#include <Library/MemoryAllocationLib.h> > > (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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel