| Message ID | 1489943988-15378-1-git-send-email-ard.biesheuvel@linaro.org |
|---|---|
| State | Accepted |
| Commit | 09da11081915dea25d2d1bba87cdf460102e52bb |
| Headers | show |
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/BootGraphicsResourceTableDxe.c | 41 ++------------------ 1 file changed, 3 insertions(+), 38 deletions(-) diff --git a/MdeModulePkg/Universal/Acpi/BootGraphicsResourceTableDxe/BootGraphicsResourceTableDxe.c b/MdeModulePkg/Universal/Acpi/BootGraphicsResourceTableDxe/BootGraphicsResourceTableDxe.c index 804ffa5a6bb6..6a7165a95489 100644 --- a/MdeModulePkg/Universal/Acpi/BootGraphicsResourceTableDxe/BootGraphicsResourceTableDxe.c +++ b/MdeModulePkg/Universal/Acpi/BootGraphicsResourceTableDxe/BootGraph +++ 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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 21 March 2017 at 02:37, Gao, Liming <liming.gao@intel.com> wrote: > Yes. Spec has no such limitation. I agree this change. > > Reviewed-by: Liming Gao <liming.gao@intel.com> > Pushed as 09da11081915, thanks. > 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 > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
diff --git a/MdeModulePkg/Universal/Acpi/BootGraphicsResourceTableDxe/BootGraphicsResourceTableDxe.c b/MdeModulePkg/Universal/Acpi/BootGraphicsResourceTableDxe/BootGraphicsResourceTableDxe.c index 804ffa5a6bb6..6a7165a95489 100644 --- a/MdeModulePkg/Universal/Acpi/BootGraphicsResourceTableDxe/BootGraphicsResourceTableDxe.c +++ b/MdeModulePkg/Universal/Acpi/BootGraphicsResourceTableDxe/BootGraphicsResourceTableDxe.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;
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/BootGraphicsResourceTableDxe.c | 41 ++------------------ 1 file changed, 3 insertions(+), 38 deletions(-) -- 2.7.4 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel