diff mbox

[edk2] BaseTools: fix a syntax error in 4 KB aligned GNU ld linker script

Message ID CAKv+Gu88OdcCT0QPqFOZ7Ux+vHBegDDwZq39virY1ZxVUd+3wQ@mail.gmail.com
State New
Headers show

Commit Message

Ard Biesheuvel July 2, 2015, 5:05 a.m. UTC
On 2 July 2015 at 02:54, Jordan Justen <jordan.l.justen@intel.com> wrote:
> On 2015-07-01 17:41:11, Gao, Liming wrote:
>> Jordan:
>>   Agree. We will follow this rule, apply the patch and push it
>>   without any modification.
>
> My point is that if there is a mistake, we should not bother to update
> the log message to fix that mistake. Hopefully we can just be more
> careful with the patch message if we stop thinking that we can go back
> and fix it later. :)
>
>>   gcc-4k is added for new UEFI2.5 Properties Table. When enable this
>>   feature, gcc-4k script will be required for RUNTIME driver. It can
>>   be configured in [BuildOptions] of DSC file for RUNTIME driver
>>   only. We could update Ovmf to enable this feature, which can be
>>   turned on/off by build flag. Is it OK to you?
>
> Is there an example of how to enable it for a platform? Or, the
> feature is still is not fully complete?
>

Hello Jordan,

As I reported here:
http://article.gmane.org/gmane.comp.bios.tianocore.devel/16433

I gave this a quick spin by adding this
----------8<------------
----------8<------------

to the OvmfPkg build, and it appears to work fine.

There are a couple of things to be aware of, though.
- It depends on Laszlo's end-of-dxe series
http://thread.gmane.org/gmane.comp.bios.tianocore.devel/16304

- Adding subsequent --script arguments to GNU ld accumulates linker
scripts instead of replacing them. This works correctly in this case
since the linker scripts are identical except for the section
alignments, but it is something to keep in mind.
- It crashes the Linux kernel at boot. The crash seems to be inside
SetVirtualAddressMap (), which suggests that X64 Linux suffers from
the same problem as AARCH64, i.e., that the virtual mapping does not
preserve the offset between adjacent code and data regions. This is
not required by the spec, though, so this feature essentially breaks
backward compatibility.

The latter is a huge concern imo, since the fact that it breaks older
OSes will impede the adoption of an otherwise very useful security
feature.

Regards,
Ard.



>> -----Original Message-----
>> From: Justen, Jordan L
>> Sent: Thursday, July 2, 2015 6:42 AM
>> To: Liu, Yingke D; Ard Biesheuvel
>> Cc: edk2-devel@lists.sourceforge.net; Gao, Liming
>> Subject: RE: [PATCH] BaseTools: fix a syntax error in 4 KB aligned GNU ld linker script
>>
>> On 2015-07-01 01:34:56, Liu, Yingke D wrote:
>> > Hi Ard,
>> >
>> > Thanks for the comments, I have updated the log message.
>>
>> For future reference, I think you should not bother to update the
>> log message. It is a bad practice, and the git archive will not see
>> the updated log message anyhow. It should also be noted that it will
>> not be possible to update the commit message when git is the main
>> upstream repo.
>>
>> I think it is best just to acknowledge the mistake, and remember it
>> for next time.
>>
>> I have a question. Where is the 4K script used? git grep gcc-4K
>> shows no references to it.
>>
>> -Jordan
>>
>> > -----Original Message-----
>> > From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
>> > Sent: Wednesday, July 01, 2015 16:12
>> > To: Liu, Yingke D
>> > Cc: edk2-devel@lists.sourceforge.net; Justen, Jordan L; Gao, Liming
>> > Subject: Re: [PATCH] BaseTools: fix a syntax error in 4 KB aligned GNU
>> > ld linker script
>> >
>> > On 30 June 2015 at 13:02, Gao, Liming <liming.gao@intel.com> wrote:
>> > > Reviewed-by: Liming Gao <liming.gao@intel.com>
>> > >
>> >
>> > Hello Dennis,
>> >
>> > Thanks for merging this.
>> >
>> > Next time, could you please keep the subject line as well?
>> >
>> > > Subject: [PATCH] BaseTools: fix a syntax error in 4 KB aligned GNU
>> > > ld linker script
>> >
>> > You have used this instead:
>> >
>> > > There needs to be a space between the output section name and the
>> > > colon, i.e.,
>> >
>> > as the subject line which looks a little silly imo
>> >
>> > Thanks,
>> > Ard.
>> >
>> >
>> > > -----Original Message-----
>> > > From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
>> > > Sent: Tuesday, June 30, 2015 7:00 PM
>> > > To: Liu, Yingke D; Gao, Liming; edk2-devel@lists.sourceforge.net
>> > > Cc: Ard Biesheuvel
>> > > Subject: [PATCH] BaseTools: fix a syntax error in 4 KB aligned GNU
>> > > ld linker script
>> > >
>> > > There needs to be a space between the output section name and the
>> > > colon, i.e.,
>> > >
>> > >   .text : ALIGN(0x1000)
>> > >        ^
>> > >
>> > > Fix this for all output sections
>> > >
>> > > Contributed-under: TianoCore Contribution Agreement 1.0
>> > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> > > ---
>> > >  BaseTools/Scripts/gcc-4K-align-ld-script | 10 +++++-----
>> > >  1 file changed, 5 insertions(+), 5 deletions(-)
>> > >
>> > > diff --git a/BaseTools/Scripts/gcc-4K-align-ld-script
>> > > b/BaseTools/Scripts/gcc-4K-align-ld-script
>> > > index 1f23079023a6..16cf623a3362 100644
>> > > --- a/BaseTools/Scripts/gcc-4K-align-ld-script
>> > > +++ b/BaseTools/Scripts/gcc-4K-align-ld-script
>> > > @@ -3,12 +3,12 @@ SECTIONS
>> > >  {
>> > >    /* . = 0 + SIZEOF_HEADERS; */
>> > >    . = 0x280;
>> > > -  .text: ALIGN(0x1000)
>> > > +  .text : ALIGN(0x1000)
>> > >    {
>> > >      *(.text .stub .text.* .gnu.linkonce.t.*)
>> > >      . = ALIGN(0x20);
>> > >    }
>> > > -  .data: ALIGN(0x1000)
>> > > +  .data : ALIGN(0x1000)
>> > >    {
>> > >      *(
>> > >        .rodata .rodata.* .gnu.linkonce.r.* @@ -18,16 +18,16 @@ SECTIONS
>> > >      )
>> > >      . = ALIGN(0x20);
>> > >    }
>> > > -  .eh_frame: ALIGN(0x1000)
>> > > +  .eh_frame : ALIGN(0x1000)
>> > >    {
>> > >      KEEP (*(.eh_frame))
>> > >    }
>> > > -  .got: ALIGN(0x1000)
>> > > +  .got : ALIGN(0x1000)
>> > >    {
>> > >      *(.got .got.*)
>> > >      . = ALIGN(0x20);
>> > >    }
>> > > -  .rela: ALIGN(0x1000)
>> > > +  .rela : ALIGN(0x1000)
>> > >    {
>> > >      *(.rela .rela.*)
>> > >    }
>> > > --
>> > > 1.9.1
>> > >

------------------------------------------------------------------------------
Don't Limit Your Business. Reach for the Cloud.
GigeNET's Cloud Solutions provide you with the tools and support that
you need to offload your IT needs and focus on growing your business.
Configured For All Businesses. Start Your Cloud Today.
https://www.gigenetcloud.com/

Comments

Ard Biesheuvel July 2, 2015, 6:10 a.m. UTC | #1
On 2 July 2015 at 08:08, Jordan Justen <jordan.l.justen@intel.com> wrote:
> On 2015-07-01 22:35:12, Gao, Liming wrote:
>> Ard:
>>   Good to know --script option can be appended and work fine.
>
> So, --script will be specified twice to ld. This seems like a very
> fragile implementation of a new UEFI feature.
>
> Is this the long term plan?
>
> Do Visual Studio based builds do something similar?
>

For AARCH64/GCC, I proposed an overlay LD script that just sets the
increased alignment (64 KB in this case)
I.e.,

----8<----
SECTIONS {
 .text : ALIGN(0x10000) { }
  .data : ALIGN(0x10000) { }
}
----8<----

so I suppose the X86 version could be stripped down in a similar way.
Ard Biesheuvel July 2, 2015, 6:28 a.m. UTC | #2
On 2 July 2015 at 08:22, Andrew Fish <afish@apple.com> wrote:
>
>
> Sent from my iPhone
>
>> On Jul 1, 2015, at 11:08 PM, Jordan Justen <jordan.l.justen@intel.com> wrote:
>>
>>> On 2015-07-01 22:35:12, Gao, Liming wrote:
>>> Ard:
>>>  Good to know --script option can be appended and work fine.
>>
>> So, --script will be specified twice to ld. This seems like a very
>> fragile implementation of a new UEFI feature.
>>
>
> I'm not sure about specifics, but in general build tools always honor the last invocation of a flag, as that's required to support local overrides. For example you can turn off optimization in an INF as the 2nd compiler optimization flag has  precedence.
>
> Do the include a file flags work differently?
>

Yes, GNU ld's --script or -T option works differently:

"""
-T scriptfile
--script=scriptfile
    Use scriptfile as the linker script.  This script replaces ld's default
    linker script (rather than adding to it), so commandfile must specify
    everything necessary to describe the output file.    If scriptfile does
    not exist in the current directory, "ld" looks for it in the directories
    specified by any preceding -L options.  Multiple -T options accumulate.
"""


>> Is this the long term plan?
>>
>> Do Visual Studio based builds do something similar?
>>
>> -Jordan
>>
>>> -----Original Message-----
>>> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
>>> Sent: Thursday, July 02, 2015 1:05 PM
>>> To: Justen, Jordan L
>>> Cc: Gao, Liming; Liu, Yingke D; edk2-devel@lists.sourceforge.net
>>> Subject: Re: [PATCH] BaseTools: fix a syntax error in 4 KB aligned GNU ld linker script
>>>
>>>> On 2 July 2015 at 02:54, Jordan Justen <jordan.l.justen@intel.com> wrote:
>>>>> On 2015-07-01 17:41:11, Gao, Liming wrote:
>>>>> Jordan:
>>>>>  Agree. We will follow this rule, apply the patch and push it
>>>>>  without any modification.
>>>>
>>>> My point is that if there is a mistake, we should not bother to update
>>>> the log message to fix that mistake. Hopefully we can just be more
>>>> careful with the patch message if we stop thinking that we can go back
>>>> and fix it later. :)
>>>>
>>>>>  gcc-4k is added for new UEFI2.5 Properties Table. When enable this
>>>>>  feature, gcc-4k script will be required for RUNTIME driver. It can
>>>>>  be configured in [BuildOptions] of DSC file for RUNTIME driver
>>>>>  only. We could update Ovmf to enable this feature, which can be
>>>>>  turned on/off by build flag. Is it OK to you?
>>>>
>>>> Is there an example of how to enable it for a platform? Or, the
>>>> feature is still is not fully complete?
>>>
>>> Hello Jordan,
>>>
>>> As I reported here:
>>> http://article.gmane.org/gmane.comp.bios.tianocore.devel/16433
>>>
>>> I gave this a quick spin by adding this
>>> ----------8<------------
>>> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc index e5fc90d2e610..7f06b51f65cf 100644
>>> --- a/OvmfPkg/OvmfPkgX64.dsc
>>> +++ b/OvmfPkg/OvmfPkgX64.dsc
>>> @@ -48,6 +48,9 @@ [BuildOptions]
>>>   INTEL:*_*_X64_GENFW_FLAGS = --keepexceptiontable  !endif
>>>
>>> +[BuildOptions.X64.EDKII.DXE_RUNTIME_DRIVER]
>>> +  GCC:*_*_*_DLINK_FLAGS =
>>> --script=$(EDK_TOOLS_PATH)/Scripts/gcc-4K-align-ld-script
>>> +
>>> ################################################################################
>>> #
>>> # SKU Identification section - list of all SKU IDs supported by this Platform.
>>> ----------8<------------
>>>
>>> to the OvmfPkg build, and it appears to work fine.
>>>
>>> There are a couple of things to be aware of, though.
>>> - It depends on Laszlo's end-of-dxe series
>>> http://thread.gmane.org/gmane.comp.bios.tianocore.devel/16304
>>>
>>> - Adding subsequent --script arguments to GNU ld accumulates linker scripts instead of replacing them. This works correctly in this case since the linker scripts are identical except for the section alignments, but it is something to keep in mind.
>>> - It crashes the Linux kernel at boot. The crash seems to be inside SetVirtualAddressMap (), which suggests that X64 Linux suffers from the same problem as AARCH64, i.e., that the virtual mapping does not preserve the offset between adjacent code and data regions. This is not required by the spec, though, so this feature essentially breaks backward compatibility.
>>>
>>> The latter is a huge concern imo, since the fact that it breaks older OSes will impede the adoption of an otherwise very useful security feature.
>>>
>>> Regards,
>>> Ard.
>>>
>>>
>>>
>>>>> -----Original Message-----
>>>>> From: Justen, Jordan L
>>>>> Sent: Thursday, July 2, 2015 6:42 AM
>>>>> To: Liu, Yingke D; Ard Biesheuvel
>>>>> Cc: edk2-devel@lists.sourceforge.net; Gao, Liming
>>>>> Subject: RE: [PATCH] BaseTools: fix a syntax error in 4 KB aligned
>>>>> GNU ld linker script
>>>>>
>>>>>> On 2015-07-01 01:34:56, Liu, Yingke D wrote:
>>>>>> Hi Ard,
>>>>>>
>>>>>> Thanks for the comments, I have updated the log message.
>>>>>
>>>>> For future reference, I think you should not bother to update the log
>>>>> message. It is a bad practice, and the git archive will not see the
>>>>> updated log message anyhow. It should also be noted that it will not
>>>>> be possible to update the commit message when git is the main
>>>>> upstream repo.
>>>>>
>>>>> I think it is best just to acknowledge the mistake, and remember it
>>>>> for next time.
>>>>>
>>>>> I have a question. Where is the 4K script used? git grep gcc-4K shows
>>>>> no references to it.
>>>>>
>>>>> -Jordan
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
>>>>>> Sent: Wednesday, July 01, 2015 16:12
>>>>>> To: Liu, Yingke D
>>>>>> Cc: edk2-devel@lists.sourceforge.net; Justen, Jordan L; Gao, Liming
>>>>>> Subject: Re: [PATCH] BaseTools: fix a syntax error in 4 KB aligned
>>>>>> GNU ld linker script
>>>>>>
>>>>>>> On 30 June 2015 at 13:02, Gao, Liming <liming.gao@intel.com> wrote:
>>>>>>> Reviewed-by: Liming Gao <liming.gao@intel.com>
>>>>>>
>>>>>> Hello Dennis,
>>>>>>
>>>>>> Thanks for merging this.
>>>>>>
>>>>>> Next time, could you please keep the subject line as well?
>>>>>>
>>>>>>> Subject: [PATCH] BaseTools: fix a syntax error in 4 KB aligned
>>>>>>> GNU ld linker script
>>>>>>
>>>>>> You have used this instead:
>>>>>>
>>>>>>> There needs to be a space between the output section name and the
>>>>>>> colon, i.e.,
>>>>>>
>>>>>> as the subject line which looks a little silly imo
>>>>>>
>>>>>> Thanks,
>>>>>> Ard.
>>>>>>
>>>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
>>>>>>> Sent: Tuesday, June 30, 2015 7:00 PM
>>>>>>> To: Liu, Yingke D; Gao, Liming; edk2-devel@lists.sourceforge.net
>>>>>>> Cc: Ard Biesheuvel
>>>>>>> Subject: [PATCH] BaseTools: fix a syntax error in 4 KB aligned
>>>>>>> GNU ld linker script
>>>>>>>
>>>>>>> There needs to be a space between the output section name and the
>>>>>>> colon, i.e.,
>>>>>>>
>>>>>>>  .text : ALIGN(0x1000)
>>>>>>>       ^
>>>>>>>
>>>>>>> Fix this for all output sections
>>>>>>>
>>>>>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>>>>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>>>>>> ---
>>>>>>> BaseTools/Scripts/gcc-4K-align-ld-script | 10 +++++-----
>>>>>>> 1 file changed, 5 insertions(+), 5 deletions(-)
>>>>>>>
>>>>>>> diff --git a/BaseTools/Scripts/gcc-4K-align-ld-script
>>>>>>> b/BaseTools/Scripts/gcc-4K-align-ld-script
>>>>>>> index 1f23079023a6..16cf623a3362 100644
>>>>>>> --- a/BaseTools/Scripts/gcc-4K-align-ld-script
>>>>>>> +++ b/BaseTools/Scripts/gcc-4K-align-ld-script
>>>>>>> @@ -3,12 +3,12 @@ SECTIONS
>>>>>>> {
>>>>>>>   /* . = 0 + SIZEOF_HEADERS; */
>>>>>>>   . = 0x280;
>>>>>>> -  .text: ALIGN(0x1000)
>>>>>>> +  .text : ALIGN(0x1000)
>>>>>>>   {
>>>>>>>     *(.text .stub .text.* .gnu.linkonce.t.*)
>>>>>>>     . = ALIGN(0x20);
>>>>>>>   }
>>>>>>> -  .data: ALIGN(0x1000)
>>>>>>> +  .data : ALIGN(0x1000)
>>>>>>>   {
>>>>>>>     *(
>>>>>>>       .rodata .rodata.* .gnu.linkonce.r.* @@ -18,16 +18,16 @@ SECTIONS
>>>>>>>     )
>>>>>>>     . = ALIGN(0x20);
>>>>>>>   }
>>>>>>> -  .eh_frame: ALIGN(0x1000)
>>>>>>> +  .eh_frame : ALIGN(0x1000)
>>>>>>>   {
>>>>>>>     KEEP (*(.eh_frame))
>>>>>>>   }
>>>>>>> -  .got: ALIGN(0x1000)
>>>>>>> +  .got : ALIGN(0x1000)
>>>>>>>   {
>>>>>>>     *(.got .got.*)
>>>>>>>     . = ALIGN(0x20);
>>>>>>>   }
>>>>>>> -  .rela: ALIGN(0x1000)
>>>>>>> +  .rela : ALIGN(0x1000)
>>>>>>>   {
>>>>>>>     *(.rela .rela.*)
>>>>>>>   }
>>>>>>> --
>>>>>>> 1.9.1
>>
>> ------------------------------------------------------------------------------
>> Don't Limit Your Business. Reach for the Cloud.
>> GigeNET's Cloud Solutions provide you with the tools and support that
>> you need to offload your IT needs and focus on growing your business.
>> Configured For All Businesses. Start Your Cloud Today.
>> https://www.gigenetcloud.com/
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.sourceforge.net
>> https://lists.sourceforge.net/lists/listinfo/edk2-devel

------------------------------------------------------------------------------
Don't Limit Your Business. Reach for the Cloud.
GigeNET's Cloud Solutions provide you with the tools and support that
you need to offload your IT needs and focus on growing your business.
Configured For All Businesses. Start Your Cloud Today.
https://www.gigenetcloud.com/
Ard Biesheuvel July 2, 2015, 7:26 a.m. UTC | #3
On 2 July 2015 at 09:21, Gao, Liming <liming.gao@intel.com> wrote:
> Jordan:
>   For now, this feature will break older OSes. The platform requires to add feature flag to enable/disable it. So, DSC BuildOptions extension is added to configure it. In future, if more and more platforms enable it, we could figure out the solution to extent tools_def.txt to configure those settings as the default one.
>

Building DXE_RUNTIME_DRIVER modules with 4 KB section alignment does
not break older OSes, so it makes sense to at least have the plumbing
implemented in BaseTools in an elegant way.

What does break older OSes is actually splitting the PE/COFF image
into separate memory regions. This is not such a concern for AARCH64
since it is still new, so I am pushing for it to be enabled when
possible. (It does break on the current upstream Linux kernel, but I
have some patches on the list that fix that)

Regards,
Ard.




> From: Jordan Justen [mailto:jordan.l.justen@intel.com]
> Sent: Thursday, July 02, 2015 3:04 PM
> To: Andrew Fish; edk2-devel@lists.sourceforge.net
> Subject: Re: [edk2] [PATCH] BaseTools: fix a syntax error in 4 KB aligned GNU ld linker script
>
> On 2015-07-01 23:22:37, Andrew Fish wrote:
>> > On Jul 1, 2015, at 11:08 PM, Jordan Justen <jordan.l.justen@intel.com> wrote:
>> >
>> >> On 2015-07-01 22:35:12, Gao, Liming wrote:
>> >> Ard:
>> >>  Good to know --script option can be appended and work fine.
>> >
>> > So, --script will be specified twice to ld. This seems like a very
>> > fragile implementation of a new UEFI feature.
>> >
>>
>> I'm not sure about specifics, but in general build tools always honor
>> the last invocation of a flag, as that's required to support local
>> overrides. For example you can turn off optimization in an INF as the
>> 2nd compiler optimization flag has precedence.
>>
>> Do the include a file flags work differently?
>
> I don't think we should base the build process for an important UEFI feature on the way that tools work 'in general'. Is this an important feature for EDK II to support?
>
> Assuming it is important, do we really expect all platforms to add linker specific flags to their .dsc to support such a feature? It seems like these tool flags should be specified as needed in the Conf files.
>
> Doesn't it seem more appropriate for the platform to add something like UEFI_PROPERTIES_TABLE = TRUE to the defines of the .dsc? After that all the magic should live in BaseTools.
>
> Is this a feature that platforms need to be able to disable? I think runtime drivers should always be compressed, so it probably won't cost significant flash space.
>
> -Jordan
>
> ------------------------------------------------------------------------------
> Don't Limit Your Business. Reach for the Cloud.
> GigeNET's Cloud Solutions provide you with the tools and support that you need to offload your IT needs and focus on growing your business.
> Configured For All Businesses. Start Your Cloud Today.
> https://www.gigenetcloud.com/
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/edk2-devel
>
> ------------------------------------------------------------------------------
> Don't Limit Your Business. Reach for the Cloud.
> GigeNET's Cloud Solutions provide you with the tools and support that
> you need to offload your IT needs and focus on growing your business.
> Configured For All Businesses. Start Your Cloud Today.
> https://www.gigenetcloud.com/
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/edk2-devel

------------------------------------------------------------------------------
Don't Limit Your Business. Reach for the Cloud.
GigeNET's Cloud Solutions provide you with the tools and support that
you need to offload your IT needs and focus on growing your business.
Configured For All Businesses. Start Your Cloud Today.
https://www.gigenetcloud.com/
diff mbox

Patch

diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index e5fc90d2e610..7f06b51f65cf 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -48,6 +48,9 @@  [BuildOptions]
   INTEL:*_*_X64_GENFW_FLAGS = --keepexceptiontable
 !endif

+[BuildOptions.X64.EDKII.DXE_RUNTIME_DRIVER]
+  GCC:*_*_*_DLINK_FLAGS =
--script=$(EDK_TOOLS_PATH)/Scripts/gcc-4K-align-ld-script
+
 ################################################################################
 #
 # SKU Identification section - list of all SKU IDs supported by this Platform.