| 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 | expand |
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
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
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
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
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 //
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