[edk2,4/4] EdkCompatibilityPkg: remove PE/COFF header workaround for ELILO on IPF

Message ID 20180906134523.2036-5-ard.biesheuvel@linaro.org
State New
Headers show
Series
  • remove all 11 occurrences of ELILO on IPF PE/COFF header hack
Related show

Commit Message

Ard Biesheuvel Sept. 6, 2018, 1:45 p.m.
Now that Itanium support has been dropped, we can remove the various
occurrences of the ELILO on Itanium PE/COFF header workaround.

Link: https://bugzilla.tianocore.org/show_bug.cgi?id=816
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

---
 EdkCompatibilityPkg/Foundation/Library/EdkIIGlueLib/Library/BasePeCoffLib/BasePeCoff.c | 60 +++-----------------
 1 file changed, 8 insertions(+), 52 deletions(-)

-- 
2.18.0

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Comments

Laszlo Ersek Sept. 6, 2018, 4:53 p.m. | #1
On 09/06/18 15:45, Ard Biesheuvel wrote:
> Now that Itanium support has been dropped, we can remove the various

> occurrences of the ELILO on Itanium PE/COFF header workaround.

>

> Link: https://bugzilla.tianocore.org/show_bug.cgi?id=816

> Contributed-under: TianoCore Contribution Agreement 1.1

> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> ---

>  EdkCompatibilityPkg/Foundation/Library/EdkIIGlueLib/Library/BasePeCoffLib/BasePeCoff.c | 60 +++-----------------

>  1 file changed, 8 insertions(+), 52 deletions(-)


Should we care about EdkCompatibilityPkg at all? Because:

* IPF removal seems not to have occurred to EdkCompatibilityPkg:

  $ git grep -w IPF -- 'EdkCompatibilityPkg/*inf'
  [bunch of hits]

* In <https://bugzilla.tianocore.org/show_bug.cgi?id=816#c7>, you wrote:

> [...] there is a big difference between IPF drivers that are never

> referenced by modern platforms, and workarounds in generic code that

> are present in every modern build for every platform, and are only

> intended for a specific build of ELILO.


> The former is essentially dead code. The latter gets executed many

> times on every boot of every modern UEFI platform in existence.


Under that distinction, I would classify EdkCompatibilityPkg as the
first category, i.e., essentially dead code.

(I'm pointing this out in the hope that it'll save me the review of this
patch! :) )

Thanks!
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel Sept. 6, 2018, 5:27 p.m. | #2
On 6 September 2018 at 18:53, Laszlo Ersek <lersek@redhat.com> wrote:
> On 09/06/18 15:45, Ard Biesheuvel wrote:

>> Now that Itanium support has been dropped, we can remove the various

>> occurrences of the ELILO on Itanium PE/COFF header workaround.

>>

>> Link: https://bugzilla.tianocore.org/show_bug.cgi?id=816

>> Contributed-under: TianoCore Contribution Agreement 1.1

>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

>> ---

>>  EdkCompatibilityPkg/Foundation/Library/EdkIIGlueLib/Library/BasePeCoffLib/BasePeCoff.c | 60 +++-----------------

>>  1 file changed, 8 insertions(+), 52 deletions(-)

>

> Should we care about EdkCompatibilityPkg at all? Because:

>

> * IPF removal seems not to have occurred to EdkCompatibilityPkg:

>

>   $ git grep -w IPF -- 'EdkCompatibilityPkg/*inf'

>   [bunch of hits]

>

> * In <https://bugzilla.tianocore.org/show_bug.cgi?id=816#c7>, you wrote:

>

>> [...] there is a big difference between IPF drivers that are never

>> referenced by modern platforms, and workarounds in generic code that

>> are present in every modern build for every platform, and are only

>> intended for a specific build of ELILO.

>

>> The former is essentially dead code. The latter gets executed many

>> times on every boot of every modern UEFI platform in existence.

>

> Under that distinction, I would classify EdkCompatibilityPkg as the

> first category, i.e., essentially dead code.

>


OK, fair enough. I don't care about EdkCompatibilityPkg at all, I just
wanted to be thorough, but if others don't care either, I'll drop this
from v2.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Carsey, Jaben Sept. 6, 2018, 6:01 p.m. | #3
We have a BZ to remove the compatibility package... I’d call updating it redundant and not worth your time.

Jaben

> On Sep 6, 2018, at 10:27 AM, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:

> 

>> On 6 September 2018 at 18:53, Laszlo Ersek <lersek@redhat.com> wrote:

>>> On 09/06/18 15:45, Ard Biesheuvel wrote:

>>> Now that Itanium support has been dropped, we can remove the various

>>> occurrences of the ELILO on Itanium PE/COFF header workaround.

>>> 

>>> Link: https://bugzilla.tianocore.org/show_bug.cgi?id=816

>>> Contributed-under: TianoCore Contribution Agreement 1.1

>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

>>> ---

>>> EdkCompatibilityPkg/Foundation/Library/EdkIIGlueLib/Library/BasePeCoffLib/BasePeCoff.c | 60 +++-----------------

>>> 1 file changed, 8 insertions(+), 52 deletions(-)

>> 

>> Should we care about EdkCompatibilityPkg at all? Because:

>> 

>> * IPF removal seems not to have occurred to EdkCompatibilityPkg:

>> 

>>  $ git grep -w IPF -- 'EdkCompatibilityPkg/*inf'

>>  [bunch of hits]

>> 

>> * In <https://bugzilla.tianocore.org/show_bug.cgi?id=816#c7>, you wrote:

>> 

>>> [...] there is a big difference between IPF drivers that are never

>>> referenced by modern platforms, and workarounds in generic code that

>>> are present in every modern build for every platform, and are only

>>> intended for a specific build of ELILO.

>> 

>>> The former is essentially dead code. The latter gets executed many

>>> times on every boot of every modern UEFI platform in existence.

>> 

>> Under that distinction, I would classify EdkCompatibilityPkg as the

>> first category, i.e., essentially dead code.

>> 

> 

> OK, fair enough. I don't care about EdkCompatibilityPkg at all, I just

> wanted to be thorough, but if others don't care either, I'll drop this

> from v2.

> _______________________________________________

> edk2-devel mailing list

> edk2-devel@lists.01.org

> https://lists.01.org/mailman/listinfo/edk2-devel

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Laszlo Ersek Sept. 6, 2018, 6:15 p.m. | #4
On 09/06/18 20:01, Carsey, Jaben wrote:
> We have a BZ to remove the compatibility package... I’d call updating it redundant and not worth your time.


Ah, very good point!

https://bugzilla.tianocore.org/show_bug.cgi?id=1103

Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Patch

diff --git a/EdkCompatibilityPkg/Foundation/Library/EdkIIGlueLib/Library/BasePeCoffLib/BasePeCoff.c b/EdkCompatibilityPkg/Foundation/Library/EdkIIGlueLib/Library/BasePeCoffLib/BasePeCoff.c
index f99c23e5ee4c..28d39b342afd 100644
--- a/EdkCompatibilityPkg/Foundation/Library/EdkIIGlueLib/Library/BasePeCoffLib/BasePeCoff.c
+++ b/EdkCompatibilityPkg/Foundation/Library/EdkIIGlueLib/Library/BasePeCoffLib/BasePeCoff.c
@@ -22,36 +22,6 @@  Abstract:
 
 #include "BasePeCoffLibInternals.h"
 
-/**
-  Retrieves the magic value from the PE/COFF header.
-
-  @param  Hdr             The buffer in which to return the PE32, PE32+, or TE header.
-
-  @return EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC - Image is PE32
-  @return EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC - Image is PE32+
-
-**/
-UINT16
-PeCoffLoaderGetPeHeaderMagicValue (
-  IN  EFI_IMAGE_OPTIONAL_HEADER_PTR_UNION  Hdr
-  )
-{
-  //
-  // NOTE: Some versions of Linux ELILO for Itanium have an incorrect magic value 
-  //       in the PE/COFF Header.  If the MachineType is Itanium(IA64) and the 
-  //       Magic value in the OptionalHeader is  EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC
-  //       then override the returned value to EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC
-  //
-  if (Hdr.Pe32->FileHeader.Machine == EFI_IMAGE_MACHINE_IA64 && Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
-    return EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC;
-  }
-  //
-  // Return the magic value from the PC/COFF Optional Header
-  //
-  return Hdr.Pe32->OptionalHeader.Magic;
-}
-
-
 /**
   Retrieves the PE or TE Header from a PE/COFF or TE image.
 
@@ -71,7 +41,6 @@  GluePeCoffLoaderGetPeHeader (
   RETURN_STATUS         Status;
   EFI_IMAGE_DOS_HEADER  DosHdr;
   UINTN                 Size;
-  UINT16                Magic;
 
   //
   // Read the DOS image header to check for it's existance
@@ -130,9 +99,7 @@  GluePeCoffLoaderGetPeHeader (
     ImageContext->IsTeImage = FALSE;
     ImageContext->Machine = Hdr.Pe32->FileHeader.Machine;
 
-    Magic = PeCoffLoaderGetPeHeaderMagicValue (Hdr);
-
-    if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
+    if (Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
       //
       // Use PE32 offset
       //
@@ -141,7 +108,7 @@  GluePeCoffLoaderGetPeHeader (
       ImageContext->SectionAlignment  = Hdr.Pe32->OptionalHeader.SectionAlignment;
       ImageContext->SizeOfHeaders     = Hdr.Pe32->OptionalHeader.SizeOfHeaders;
 
-    } else if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC) {
+    } else if (Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC) {
       //
       // Use PE32+ offset
       //
@@ -209,7 +176,6 @@  GluePeCoffLoaderGetImageInfo (
   EFI_IMAGE_SECTION_HEADER              SectionHeader;
   EFI_IMAGE_DEBUG_DIRECTORY_ENTRY       DebugEntry;
   UINT32                                NumberOfRvaAndSizes;
-  UINT16                                Magic;
 
   if (NULL == ImageContext) {
     return RETURN_INVALID_PARAMETER;
@@ -225,13 +191,11 @@  GluePeCoffLoaderGetImageInfo (
     return Status;
   }
 
-  Magic = PeCoffLoaderGetPeHeaderMagicValue (Hdr);
-
   //
   // Retrieve the base address of the image
   //
   if (!(ImageContext->IsTeImage)) {
-    if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
+    if (Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
       //
       // Use PE32 offset
       //
@@ -276,7 +240,7 @@  GluePeCoffLoaderGetImageInfo (
   }
 
   if (!(ImageContext->IsTeImage)) {
-    if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
+    if (Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
       //
       // Use PE32 offset
       //
@@ -520,7 +484,6 @@  GluePeCoffLoaderRelocateImage (
   CHAR8                                 *FixupData;
   PHYSICAL_ADDRESS                      BaseAddress;
   UINT32                                NumberOfRvaAndSizes;
-  UINT16                                Magic;
 
   ASSERT (ImageContext != NULL);
 
@@ -552,9 +515,7 @@  GluePeCoffLoaderRelocateImage (
   if (!(ImageContext->IsTeImage)) {
     Hdr.Pe32 = (EFI_IMAGE_NT_HEADERS32 *)((UINTN)ImageContext->ImageAddress + ImageContext->PeCoffHeaderOffset);
 
-    Magic = PeCoffLoaderGetPeHeaderMagicValue (Hdr);
-
-    if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
+    if (Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
       //
       // Use PE32 offset
       //
@@ -777,7 +738,6 @@  GluePeCoffLoaderLoadImage (
   UINTN                                 Size;
   UINT32                                TempDebugEntryRva;
   UINT32                                NumberOfRvaAndSizes;
-  UINT16                                Magic;
 
   ASSERT (ImageContext != NULL);
 
@@ -965,12 +925,11 @@  GluePeCoffLoaderLoadImage (
   //
   // Get image's entry point
   //
-  Magic = PeCoffLoaderGetPeHeaderMagicValue (Hdr);
   if (!(ImageContext->IsTeImage)) {
     //
     // Sizes of AddressOfEntryPoint are different so we need to do this safely
     //
-    if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
+    if (Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
       //
       // Use PE32 offset
       //
@@ -1004,7 +963,7 @@  GluePeCoffLoaderLoadImage (
   // the optional header to verify a desired directory entry is there.
   //
   if (!(ImageContext->IsTeImage)) {
-    if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
+    if (Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
       //
       // Use PE32 offset
       //
@@ -1172,7 +1131,6 @@  PeCoffLoaderRelocateImageForRuntime (
   CHAR8                               *FixupData;
   UINTN                               Adjust;
   RETURN_STATUS                       Status;
-  UINT16                              Magic;
 
   OldBase = (CHAR8 *)((UINTN)ImageBase);
   NewBase = (CHAR8 *)((UINTN)VirtImageBase);
@@ -1201,9 +1159,7 @@  PeCoffLoaderRelocateImageForRuntime (
     return ;
   }
 
-  Magic = PeCoffLoaderGetPeHeaderMagicValue (Hdr);
-
-  if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
+  if (Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
     //
     // Use PE32 offset
     //