diff mbox

[edk2,2/3] BaseTools: Update GenFw to support 4K alignment.

Message ID CAKv+Gu8tnp4NBPMSZ0Qk6nit7Ag4wt=AENBNYXyi+QUig9NBTA@mail.gmail.com
State New
Headers show

Commit Message

Ard Biesheuvel June 24, 2015, 10:12 a.m. UTC 
On 24 June 2015 at 10:26, Gao, Liming <liming.gao@intel.com> wrote:
> Got your point. I agree with this change. We will include it in the patch and add you in Signed-off-by list.
>

OK, thanks.

There is one additional issue though: when using alignment of > 4 KB,
the GenFw tool may crash, since it tries to pad out the .reloc section
up to section alignment, which may cover unmapped host memory. So
something like below is needed, and MAX_COFF_ALIGNMENT needs to be set
to some sensible but sufficiently high value. Since PE/COFF specifies
64 KB as the maximum file alignment, and GenFw uses mCoffAlignment for
both SectionAlignment and FileAlignment, I suppose 64 KB would be
appropriate here.

"""
"""

Comments

Ard Biesheuvel June 25, 2015, 6:41 a.m. UTC | #1 
On 25 June 2015 at 05:33, Liu, Yingke D <yingke.d.liu@intel.com> wrote:
> Ard,
>
> Following is the full GenFw patch, please confirm it:
>
> BaseTools: Update GenFw to support 4K alignment.
>
> Get maximum section alignment from ELF sections
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  BaseTools/Source/C/GenFw/Elf32Convert.c | 16 +++++++++++++++-
>  BaseTools/Source/C/GenFw/Elf64Convert.c | 16 +++++++++++++++-
>  BaseTools/Source/C/GenFw/ElfConvert.c   |  4 ++--
>  BaseTools/Source/C/GenFw/ElfConvert.h   |  1 +
>  4 files changed, 33 insertions(+), 4 deletions(-)
>

Hello Dennis,

The patch looks fine to me.

Thanks,
Ard.

> diff --git a/BaseTools/Source/C/GenFw/Elf32Convert.c b/BaseTools/Source/C/GenFw/Elf32Convert.c
> index 5c7b689..10d9892 100644
> --- a/BaseTools/Source/C/GenFw/Elf32Convert.c
> +++ b/BaseTools/Source/C/GenFw/Elf32Convert.c
> @@ -96,7 +96,7 @@ STATIC Elf_Phdr *mPhdrBase;
>  //
>  // Coff information
>  //
> -STATIC const UINT32 mCoffAlignment = 0x20;
> +STATIC UINT32 mCoffAlignment = 0x20;
>
>  //
>  // PE section alignment.
> @@ -292,6 +292,20 @@ ScanSections32 (
>    mCoffOffset += mCoffNbrSections * sizeof(EFI_IMAGE_SECTION_HEADER);
>
>    //
> +  // Set mCoffAlignment to the maximum alignment of the input sections
> +  // we care about
> +  //
> +  for (i = 0; i < mEhdr->e_shnum; i++) {
> +    Elf_Shdr *shdr = GetShdrByIndex(i);
> +    if (shdr->sh_addralign <= mCoffAlignment) {
> +      continue;
> +    }
> +    if (IsTextShdr(shdr) || IsDataShdr(shdr) || IsHiiRsrcShdr(shdr)) {
> +      mCoffAlignment = (UINT32)shdr->sh_addralign;
> +    }
> +  }
> +
> +  //
>    // First text sections.
>    //
>    mCoffOffset = CoffAlign(mCoffOffset);
> diff --git a/BaseTools/Source/C/GenFw/Elf64Convert.c b/BaseTools/Source/C/GenFw/Elf64Convert.c
> index 25b90e2..d2becf1 100644
> --- a/BaseTools/Source/C/GenFw/Elf64Convert.c
> +++ b/BaseTools/Source/C/GenFw/Elf64Convert.c
> @@ -97,7 +97,7 @@ STATIC Elf_Phdr *mPhdrBase;
>  //
>  // Coff information
>  //
> -STATIC const UINT32 mCoffAlignment = 0x20;
> +STATIC UINT32 mCoffAlignment = 0x20;
>
>  //
>  // PE section alignment.
> @@ -286,6 +286,20 @@ ScanSections64 (
>    mCoffOffset += mCoffNbrSections * sizeof(EFI_IMAGE_SECTION_HEADER);
>
>    //
> +  // Set mCoffAlignment to the maximum alignment of the input sections
> +  // we care about
> +  //
> +  for (i = 0; i < mEhdr->e_shnum; i++) {
> +    Elf_Shdr *shdr = GetShdrByIndex(i);
> +    if (shdr->sh_addralign <= mCoffAlignment) {
> +      continue;
> +    }
> +    if (IsTextShdr(shdr) || IsDataShdr(shdr) || IsHiiRsrcShdr(shdr)) {
> +      mCoffAlignment = (UINT32)shdr->sh_addralign;
> +    }
> +  }
> +
> +  //
>    // First text sections.
>    //
>    mCoffOffset = CoffAlign(mCoffOffset);
> diff --git a/BaseTools/Source/C/GenFw/ElfConvert.c b/BaseTools/Source/C/GenFw/ElfConvert.c
> index 1a84d3c..6211389 100644
> --- a/BaseTools/Source/C/GenFw/ElfConvert.c
> +++ b/BaseTools/Source/C/GenFw/ElfConvert.c
> @@ -96,11 +96,11 @@ CoffAddFixup(
>
>      mCoffFile = realloc (
>        mCoffFile,
> -      mCoffOffset + sizeof(EFI_IMAGE_BASE_RELOCATION) + 2*0x1000
> +      mCoffOffset + sizeof(EFI_IMAGE_BASE_RELOCATION) + 2 * MAX_COFF_ALIGNMENT
>        );
>      memset (
>        mCoffFile + mCoffOffset, 0,
> -      sizeof(EFI_IMAGE_BASE_RELOCATION) + 2*0x1000
> +      sizeof(EFI_IMAGE_BASE_RELOCATION) + 2 * MAX_COFF_ALIGNMENT
>        );
>
>      mCoffBaseRel = (EFI_IMAGE_BASE_RELOCATION*)(mCoffFile + mCoffOffset);
> diff --git a/BaseTools/Source/C/GenFw/ElfConvert.h b/BaseTools/Source/C/GenFw/ElfConvert.h
> index b27a2f9..56f165e 100644
> --- a/BaseTools/Source/C/GenFw/ElfConvert.h
> +++ b/BaseTools/Source/C/GenFw/ElfConvert.h
> @@ -34,6 +34,7 @@ extern UINT32 mOutImageType;
>  // Common EFI specific data.
>  //
>  #define ELF_HII_SECTION_NAME ".hii"
> +#define MAX_COFF_ALIGNMENT 0x10000
>
>  //
>  // Filter Types
> --
>
> Dennis
>
> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: Wednesday, June 24, 2015 18:13
> To: edk2-devel@lists.sourceforge.net
> Subject: Re: [edk2] [Patch 2/3] BaseTools: Update GenFw to support 4K alignment.
>
> On 24 June 2015 at 10:26, Gao, Liming <liming.gao@intel.com> wrote:
>> Got your point. I agree with this change. We will include it in the patch and add you in Signed-off-by list.
>>
>
> OK, thanks.
>
> There is one additional issue though: when using alignment of > 4 KB, the GenFw tool may crash, since it tries to pad out the .reloc section up to section alignment, which may cover unmapped host memory. So something like below is needed, and MAX_COFF_ALIGNMENT needs to be set to some sensible but sufficiently high value. Since PE/COFF specifies
> 64 KB as the maximum file alignment, and GenFw uses mCoffAlignment for both SectionAlignment and FileAlignment, I suppose 64 KB would be appropriate here.
>
> """
> diff --git a/BaseTools/Source/C/GenFw/ElfConvert.c
> b/BaseTools/Source/C/GenFw/ElfConvert.c
> index 1a84d3c28794..e315d913378d 100644
> --- a/BaseTools/Source/C/GenFw/ElfConvert.c
> +++ b/BaseTools/Source/C/GenFw/ElfConvert.c
> @@ -96,11 +101,11 @@ CoffAddFixup(
>
>      mCoffFile = realloc (
>        mCoffFile,
> -      mCoffOffset + sizeof(EFI_IMAGE_BASE_RELOCATION) + 2*0x1000
> +      mCoffOffset + sizeof(EFI_IMAGE_BASE_RELOCATION) + 2 *
> + MAX_COFF_ALIGNMENT
>        );
>      memset (
>        mCoffFile + mCoffOffset, 0,
> -      sizeof(EFI_IMAGE_BASE_RELOCATION) + 2*0x1000
> +      sizeof(EFI_IMAGE_BASE_RELOCATION) + 2 * MAX_COFF_ALIGNMENT
>        );
>
>      mCoffBaseRel = (EFI_IMAGE_BASE_RELOCATION*)(mCoffFile + mCoffOffset); """
>
> --
> Ard.
>
>
>> -----Original Message-----
>> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
>> Sent: Wednesday, June 24, 2015 4:19 PM
>> To: edk2-devel@lists.sourceforge.net
>> Subject: Re: [edk2] [Patch 2/3] BaseTools: Update GenFw to support 4K alignment.
>>
>> On 24 June 2015 at 10:15, Gao, Liming <liming.gao@intel.com> wrote:
>>> Ard:
>>>   Good suggestion. How about go through every Shdr and choose the max Shdr->sh_addralign?  The alignment should be power of 2.
>>>
>>
>> Indeed. Something like this seems to work fine:
>>
>> """
>> diff --git a/BaseTools/Source/C/GenFw/Elf64Convert.c
>> b/BaseTools/Source/C/GenFw/Elf64Convert.c
>> index 2266e487cec7..4025191e868e 100644
>> --- a/BaseTools/Source/C/GenFw/Elf64Convert.c
>> +++ b/BaseTools/Source/C/GenFw/Elf64Convert.c
>> @@ -97,7 +97,7 @@ STATIC Elf_Phdr *mPhdrBase;  //  // Coff information  // -STATIC const UINT32 mCoffAlignment = 0x20;
>> +STATIC UINT32 mCoffAlignment = 0x20;
>>
>>  //
>>  // PE section alignment.
>> @@ -286,6 +286,20 @@ ScanSections64 (
>>    mCoffOffset += mCoffNbrSections * sizeof(EFI_IMAGE_SECTION_HEADER);
>>
>>    //
>> +  // Set mCoffAlignment to the maximum alignment of the input sections
>> + // we care about  //  for (i = 0; i < mEhdr->e_shnum; i++) {
>> +    Elf_Shdr *shdr = GetShdrByIndex(i);
>> +    if (shdr->sh_addralign <= mCoffAlignment) {
>> +      continue;
>> +    }
>> +    if (IsTextShdr(shdr) || IsDataShdr(shdr) || IsHiiRsrcShdr(shdr)) {
>> +      mCoffAlignment = shdr->sh_addralign;
>> +    }
>> +  }
>> +
>> +  //
>>    // First text sections.
>>    //
>>    mCoffOffset = CoffAlign(mCoffOffset); """
>>
>> Note that we may want to use 64 KB instead of 4 KB on AArch64, since the OS may use 64 KB pages. So we should avoid hardcoding 4 KB values for section alignment.
>>
>> --
>> Ard.
>>
>>
>>> Thanks
>>> Liming
>>> -----Original Message-----
>>> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
>>> Sent: Tuesday, June 23, 2015 5:14 PM
>>> To: edk2-devel@lists.sourceforge.net
>>> Subject: Re: [edk2] [Patch 2/3] BaseTools: Update GenFw to support 4K alignment.
>>>
>>> On 23 June 2015 at 10:19, Yingke Liu <yingke.d.liu@intel.com> wrote:
>>>> If current ELF file is 4K aligned, the converted PE should also be 4K aligned.
>>>>
>>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>>> Signed-off-by: Yingke Liu <yingke.d.liu@intel.com>
>>>> ---
>>>>  BaseTools/Source/C/GenFw/Elf32Convert.c | 8 ++++++--
>>>> BaseTools/Source/C/GenFw/Elf64Convert.c | 6 +++++-
>>>>  2 files changed, 11 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/BaseTools/Source/C/GenFw/Elf32Convert.c
>>>> b/BaseTools/Source/C/GenFw/Elf32Convert.c
>>>> index 5c7b689..9245851 100644
>>>> --- a/BaseTools/Source/C/GenFw/Elf32Convert.c
>>>> +++ b/BaseTools/Source/C/GenFw/Elf32Convert.c
>>>> @@ -96,7 +96,7 @@ STATIC Elf_Phdr *mPhdrBase;  //  // Coff information
>>>> // -STATIC const UINT32 mCoffAlignment = 0x20;
>>>> +STATIC UINT32 mCoffAlignment = 0x20;
>>>>
>>>>  //
>>>>  // PE section alignment.
>>>> @@ -154,7 +154,11 @@ InitializeElf32 (
>>>>      Error (NULL, 0, 3000, "Unsupported", "ELF e_version (%u) not EV_CURRENT (%d)", (unsigned) mEhdr->e_version, EV_CURRENT);
>>>>      return FALSE;
>>>>    }
>>>> -
>>>> +
>>>> +  if ((mEhdr->e_entry & 0xFFF) == 0) {
>>>> +    mCoffAlignment = 0x1000;
>>>> +  }
>>>> +
>>>
>>> Can you explain why you are using the alignment of the entry point here? Wouldn't it be much better if mCoffAlignment is simply the max() of the alignments of all the sections?
>>>
>>>>    //
>>>>    // Update section header pointers
>>>>    //
>>>> diff --git a/BaseTools/Source/C/GenFw/Elf64Convert.c
>>>> b/BaseTools/Source/C/GenFw/Elf64Convert.c
>>>> index 25b90e2..8737e30 100644
>>>> --- a/BaseTools/Source/C/GenFw/Elf64Convert.c
>>>> +++ b/BaseTools/Source/C/GenFw/Elf64Convert.c
>>>> @@ -97,7 +97,7 @@ STATIC Elf_Phdr *mPhdrBase;  //  // Coff information
>>>> // -STATIC const UINT32 mCoffAlignment = 0x20;
>>>> +STATIC UINT32 mCoffAlignment = 0x20;
>>>>
>>>>  //
>>>>  // PE section alignment.
>>>> @@ -158,6 +158,10 @@ InitializeElf64 (
>>>>      return FALSE;
>>>>    }
>>>>
>>>> +  if ((mEhdr->e_entry & 0xFFF) == 0) {
>>>> +    mCoffAlignment = 0x1000;
>>>> +  }
>>>> +
>>>>    //
>>>>    // Update section header pointers
>>>>    //
>>>> --
>>>> 1.9.5.msysgit.0
>>>>
>>>>
>>>> ----------------------------------------------------------------------
>>>> -------- Monitor 25 network devices or servers for free with
>>>> OpManager!
>>>> OpManager is web-based network management software that monitors
>>>> network devices and physical & virtual servers, alerts via email & sms
>>>> for fault. Monitor 25 devices for free with no restriction. Download
>>>> now http://ad.doubleclick.net/ddm/clk/292181274;119417398;o
>>>> _______________________________________________
>>>> edk2-devel mailing list
>>>> edk2-devel@lists.sourceforge.net
>>>> https://lists.sourceforge.net/lists/listinfo/edk2-devel
>>>
>>> ------------------------------------------------------------------------------
>>> Monitor 25 network devices or servers for free with OpManager!
>>> OpManager is web-based network management software that monitors network devices and physical & virtual servers, alerts via email & sms for fault. Monitor 25 devices for free with no restriction. Download now http://ad.doubleclick.net/ddm/clk/292181274;119417398;o
>>> _______________________________________________
>>> edk2-devel mailing list
>>> edk2-devel@lists.sourceforge.net
>>> https://lists.sourceforge.net/lists/listinfo/edk2-devel
>>>
>>> ------------------------------------------------------------------------------
>>> Monitor 25 network devices or servers for free with OpManager!
>>> OpManager is web-based network management software that monitors
>>> network devices and physical & virtual servers, alerts via email & sms
>>> for fault. Monitor 25 devices for free with no restriction. Download now
>>> http://ad.doubleclick.net/ddm/clk/292181274;119417398;o
>>> _______________________________________________
>>> edk2-devel mailing list
>>> edk2-devel@lists.sourceforge.net
>>> https://lists.sourceforge.net/lists/listinfo/edk2-devel
>>
>> ------------------------------------------------------------------------------
>> Monitor 25 network devices or servers for free with OpManager!
>> OpManager is web-based network management software that monitors
>> network devices and physical & virtual servers, alerts via email & sms
>> for fault. Monitor 25 devices for free with no restriction. Download now
>> http://ad.doubleclick.net/ddm/clk/292181274;119417398;o
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.sourceforge.net
>> https://lists.sourceforge.net/lists/listinfo/edk2-devel
>>
>> ------------------------------------------------------------------------------
>> Monitor 25 network devices or servers for free with OpManager!
>> OpManager is web-based network management software that monitors
>> network devices and physical & virtual servers, alerts via email & sms
>> for fault. Monitor 25 devices for free with no restriction. Download now
>> http://ad.doubleclick.net/ddm/clk/292181274;119417398;o
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.sourceforge.net
>> https://lists.sourceforge.net/lists/listinfo/edk2-devel
>
> ------------------------------------------------------------------------------
> Monitor 25 network devices or servers for free with OpManager!
> OpManager is web-based network management software that monitors
> network devices and physical & virtual servers, alerts via email & sms
> for fault. Monitor 25 devices for free with no restriction. Download now
> http://ad.doubleclick.net/ddm/clk/292181274;119417398;o
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/edk2-devel

------------------------------------------------------------------------------
Monitor 25 network devices or servers for free with OpManager!
OpManager is web-based network management software that monitors 
network devices and physical & virtual servers, alerts via email & sms 
for fault. Monitor 25 devices for free with no restriction. Download now
http://ad.doubleclick.net/ddm/clk/292181274;119417398;o
diff mbox

Patch

diff --git a/BaseTools/Source/C/GenFw/ElfConvert.c
b/BaseTools/Source/C/GenFw/ElfConvert.c
index 1a84d3c28794..e315d913378d 100644
--- a/BaseTools/Source/C/GenFw/ElfConvert.c
+++ b/BaseTools/Source/C/GenFw/ElfConvert.c
@@ -96,11 +101,11 @@  CoffAddFixup(

     mCoffFile = realloc (
       mCoffFile,
-      mCoffOffset + sizeof(EFI_IMAGE_BASE_RELOCATION) + 2*0x1000
+      mCoffOffset + sizeof(EFI_IMAGE_BASE_RELOCATION) + 2 * MAX_COFF_ALIGNMENT
       );
     memset (
       mCoffFile + mCoffOffset, 0,
-      sizeof(EFI_IMAGE_BASE_RELOCATION) + 2*0x1000
+      sizeof(EFI_IMAGE_BASE_RELOCATION) + 2 * MAX_COFF_ALIGNMENT
       );

     mCoffBaseRel = (EFI_IMAGE_BASE_RELOCATION*)(mCoffFile + mCoffOffset);