Re: [PATCH] MdeModulePkg/BootGraphicsResourceTableDxe: don't allocate below 4 GB

["Gao, Liming" <liming.gao@intel.com>]

Yes. Spec has no such limitation. I agree this change. 

Reviewed-by: Liming Gao <liming.gao@intel.com>

Thanks
Liming
>-----Original Message-----
>From: Zeng, Star
>Sent: Monday, March 20, 2017 9:10 AM
>To: Ard Biesheuvel <ard.biesheuvel@linaro.org>; edk2-devel@lists.01.org;
>Tian, Feng <feng.tian@intel.com>
>Cc: Yao, Jiewen <jiewen.yao@intel.com>; Gao, Liming
><liming.gao@intel.com>; Zeng, Star <star.zeng@intel.com>
>Subject: RE: [PATCH] MdeModulePkg/BootGraphicsResourceTableDxe: don't
>allocate below 4 GB
>
>I am ok with this patch.
>
>Jiewen and Liming, do you have any comments?
>
>Thanks,
>Star
>-----Original Message-----
>From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
>Sent: Monday, March 20, 2017 1:20 AM
>To: edk2-devel@lists.01.org; Zeng, Star <star.zeng@intel.com>; Tian, Feng
><feng.tian@intel.com>
>Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>Subject: [PATCH] MdeModulePkg/BootGraphicsResourceTableDxe: don't
>allocate below 4 GB
>
>The BGRT table has an 8 byte field for the memory address of the image data,
>and yet the driver explicitly allocates below 4 GB. This results in an ASSERT()
>on systems that do not have any memory below 4 GB to begin with.
>
>Since neither the PI, the UEFI or the ACPI spec contain any mention of why
>this data should reside below 4 GB, replace the allocation call with an ordinary
>AllocatePages() call.
>
>Contributed-under: TianoCore Contribution Agreement 1.0
>Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>---
>
>MdeModulePkg/Universal/Acpi/BootGraphicsResourceTableDxe/BootGraphi
>csResourceTableDxe.c | 41 ++------------------
> 1 file changed, 3 insertions(+), 38 deletions(-)
>
>diff --git
>a/MdeModulePkg/Universal/Acpi/BootGraphicsResourceTableDxe/BootGrap
>hicsResourceTableDxe.c
>b/MdeModulePkg/Universal/Acpi/BootGraphicsResourceTableDxe/BootGrap
>hicsResourceTableDxe.c
>index 804ffa5a6bb6..6a7165a95489 100644
>---
>a/MdeModulePkg/Universal/Acpi/BootGraphicsResourceTableDxe/BootGrap
>hicsResourceTableDxe.c
>+++
>b/MdeModulePkg/Universal/Acpi/BootGraphicsResourceTableDxe/BootGrap
>h
>+++ icsResourceTableDxe.c
>@@ -227,43 +227,6 @@ BgrtAcpiTableChecksum (  }
>
> /**
>-  Allocate EfiBootServicesData below 4G memory address.
>-
>-  This function allocates EfiBootServicesData below 4G memory address.
>-
>-  @param[in]  Size   Size of memory to allocate.
>-
>-  @return Allocated address for output.
>-
>-**/
>-VOID *
>-BgrtAllocateBsDataMemoryBelow4G (
>-  IN UINTN       Size
>-  )
>-{
>-  UINTN                 Pages;
>-  EFI_PHYSICAL_ADDRESS  Address;
>-  EFI_STATUS            Status;
>-  VOID                  *Buffer;
>-
>-  Pages   = EFI_SIZE_TO_PAGES (Size);
>-  Address = 0xffffffff;
>-
>-  Status = gBS->AllocatePages (
>-                  AllocateMaxAddress,
>-                  EfiBootServicesData,
>-                  Pages,
>-                  &Address
>-                  );
>-  ASSERT_EFI_ERROR (Status);
>-
>-  Buffer = (VOID *) (UINTN) Address;
>-  ZeroMem (Buffer, Size);
>-
>-  return Buffer;
>-}
>-
>-/**
>   Install Boot Graphics Resource Table to ACPI table.
>
>   @return Status code.
>@@ -358,11 +321,13 @@ InstallBootGraphicsResourceTable (
>     // The image should be stored in EfiBootServicesData, allowing the system
>to reclaim the memory
>     //
>     BmpSize = (mLogoWidth * 3 + PaddingSize) * mLogoHeight + sizeof
>(BMP_IMAGE_HEADER);
>-    ImageBuffer = BgrtAllocateBsDataMemoryBelow4G (BmpSize);
>+    ImageBuffer = AllocatePages (EFI_SIZE_TO_PAGES (BmpSize));
>     if (ImageBuffer == NULL) {
>       return EFI_OUT_OF_RESOURCES;
>     }
>
>+    ZeroMem (ImageBuffer, BmpSize);
>+
>     mBmpImageHeaderTemplate.Size = (UINT32) BmpSize;
>     mBmpImageHeaderTemplate.ImageSize = (UINT32) BmpSize - sizeof
>(BMP_IMAGE_HEADER);
>     mBmpImageHeaderTemplate.PixelWidth = (UINT32) mLogoWidth;
>--
>2.7.4