Re: PATCH: Fix EFI support

[Peter Jones <pjones at redhat dot com>]

On 04/15/2009 07:02 PM, H.J. Lu wrote:
> On Wed, Apr 15, 2009 at 2:14 PM, H.J. Lu <hongjiu.lu@intel.com> wrote:
>> On Wed, Apr 15, 2009 at 02:05:25PM -0700, H.J. Lu wrote:
>>> This patch:
>>>
>>> http://sourceware.org/ml/binutils/2008-02/msg00051.html
>>>
>>> breaks EFI. I got
>>>
>>> [hjl@gnu-6 HelloWorldUefiGcc4.4]$ file HelloWorld.dll
>>> HelloWorld.dll: PE32+ executable (EFI boot service driver)
>>> [hjl@gnu-6 HelloWorldUefiGcc4.4]$ objdump -p HelloWorld.dll
>>> objdump: HelloWorld.dll: File format not recognized
>>> [hjl@gnu-6 HelloWorldUefiGcc4.4]$
>>>
>>> The problem is we have 3 EFI targets for each arch. When we scan
>>> all targets on EFI input, we always reach
>>>
>>> if (efi)
>>> {
>>>   bfd_set_error (bfd_error_wrong_format);
>>>   return NULL;
>>> }
>>>
>>> This patch fixes by checking Subsystem.  This patch also adds
>>> bfd_target_efi_p so that we only needs to check one "efi-" instead of
>>> 3 "efi-XXX-".  OK for trunl?
>>>
>> Here is the updated patch to support ia64 EFI. OK for trunk?
>>
>> Thanks.
>>
>>
>> H.J.
>> ---
>> 2009-04-15  H.J. Lu  <hongjiu.lu@intel.com>
>>
>>        * libpei.h (bfd_target_efi_p): New.
>>        (bfd_pe_executable_p): Use it.
>>
>>        * peicode.h (arch_type): Add arch_type_ia64.
>>        (pe_arch): Support arch_type_ia64.
>>        (pe_bfd_object_p): Properly check EFI and PE.
>>
> 
> The more I look at, the more I don't like the current solution.
> If I understand it correctly, we added 2 EFI targets for
> each arch just so that we can do
> 
> # objcopy  -O efi-XXX-x86_64 HelloWorld.elf HelloWorld.efi
> 
> The only difference between 3 EFI targets is different
> IMAGE_SUBSYSTEM_EFI_XXX. Why not just have one EFI
> target, efi-x8_64, defaulting to IMAGE_SUBSYSTEM_EFI_APPLICATION??
> We can add a switch to objcopy with
> 
> --efi=app|bsdrv|rtdrv
> 
> We can provide backward compatibility for efi-XXX-x86_64.
> Any comments?

That seems fine to me if it makes the code simpler or more maintainable.  I
rarely have to use either of bsdrv or rtdrv anyway, but occasionally
they're useful for debugging purposes.

-- 
        Peter