Re: [Minios-devel] [UNIKRAFT PATCH 1/4] plat: prepare linker script for thread-local storage

[Costin Lupu <costin.lup@xxxxxxxxx>]

Hi Florian,

On 4/24/19 9:42 AM, Florian Schmidt wrote:
> Hi Costin,
> 
> On 4/16/19 3:20 PM, Costin Lupu wrote:
>> Hi Florian,
>>
>> Thanks for this series. As expected, I have some comments.
>>
>> I noticed that the changes for the linker scripts are the same for all
>> platforms and architectures. Now, I believe it would be better to use a
>> single linker script which would contain the required directives and
>> then this script would be included by the platform scripts. It's just
>> the same approach start by the 'plat: Add the eh_frame and eh_frame_hdr
>> sections' patch. You can:
>>
>> 1) follow the approach in 'plat/*: Introduce unikraft internal
>> constructors', meaning that you would extend the
>> 'plat/common/x86/link64.lds' script, or
>>
>> 2) (and this is my preferred solution) introduce a new script, say
>> tls.lds, which would be included by all other scripts.
> 
> That is a good point. I forgot that we have a common linker script. I
> have to think about this a bit more, but my first feeling is that this
> code could go there instead of providing yet another linker script that
> needs to be included. An important consideration here is probably the
> linuxu target, which always works slightly differently from the others.
> For example, we do not provide a replacement for the default linker
> script for linuxu.
> 
> I'm now wondering: plat/common/x86/link64.lds at the moment only
> contains the exception header sections. Is there a specific reason these
> are necessary for xen and kvm, but not for linuxu?
> How does the exception code work with linuxu, if it's missing the
> symbols for __eh_frame_start and friends? Is the whole point for these
> symbols to be used in the memory regions function (which is only used in
> xen and kvm), and the default linker script provides exception header
> sections that are sufficient for linuxu without including the additional
> linker script?
> 
> My current feeling is: if this is a bug, and the linuxu platform should
> indeed include plat/common/x86/link64.lds, then I would add the tls
> stuff there. Otherwise, we could think about an additional linker
> script. However, in that case I would consider renaming the
> generic-sounding plat/common/x86/link64.lds to something a bit more
> expressive that conveys it's really only for eh_frame, and embracing the
> concept of "mix-in" linker scripts fully.
> 

AFAIK, there were some issues for linuxu and the decision was to
postpone pushing the changes until the issues would be fixed. Vlad and
Felipe might have more details about that.

>>
>> On 4/15/19 1:03 PM, Florian Schmidt wrote:
>>> Aggregate all .tdata and .tbss variables in the correct sections, and
>>> define global variables that point at section beginning and end.
>>>
>>> Signed-off-by: Florian Schmidt <florian.schmidt@xxxxxxxxx>
>>> ---
>>>   plat/common/include/sections.h |  5 +++++
>>>   plat/kvm/x86/link64.lds        | 18 ++++++++++++++++++
>>>   plat/linuxu/Linker.uk          |  1 +
>>>   plat/linuxu/link.ld            | 22 ++++++++++++++++++++++
>>>   plat/xen/x86/link64.lds        | 18 ++++++++++++++++++
>>>   5 files changed, 64 insertions(+)
>>>   create mode 100644 plat/linuxu/link.ld
>>>
>>> diff --git a/plat/common/include/sections.h
>>> b/plat/common/include/sections.h
>>> index 1052cfc7..a2677ce8 100644
>>> --- a/plat/common/include/sections.h
>>> +++ b/plat/common/include/sections.h
>>> @@ -63,6 +63,11 @@ extern char _data[], _edata[];
>>>   /* [_ctors, _ectors]: contains constructor tables (read-only) */
>>>   extern char _ctors[], _ectors[];
>>>   +/* [_tls_start, _tls_end]: contains .tdata.* and .tbss.* sections */
>>> +extern char _tls_start[], _tls_end[];
>>> +/* _etdata: denotes end of .tdata (and start of .tbss */
>>> +extern char _etdata[];
>>> +
>>>   /* __bss_start: start of BSS sections */
>>>   extern char __bss_start[];
>>>   diff --git a/plat/kvm/x86/link64.lds b/plat/kvm/x86/link64.lds
>>> index c96f7501..4e52da49 100644
>>> --- a/plat/kvm/x86/link64.lds
>>> +++ b/plat/kvm/x86/link64.lds
>>> @@ -71,6 +71,24 @@ SECTIONS
>>>       }
>>>       _ectors = .;
>>>   +    . = ALIGN(0x8);
>>> +    _tls_start = .;
>>> +    .tdata :
>>> +    {
>>> +        *(.tdata)
>>> +        *(.tdata.*)
>>> +        *(.gnu.linkonce.td.*)
>>> +    }
>>> +    _etdata = .;
>>> +    .tbss :
>>> +    {
>>> +        *(.tbss)
>>> +        *(.tbss.*)
>>> +        *(.gnu.linkonce.tb.*)
>>> +        . = ALIGN(0x8);
>>> +    }
>>> +    _tls_end = . + SIZEOF(.tbss);
>>
>> This is something I don't understand. _tls_end is already at the end of
>> .tbss. So why is SIZEOF(.tbss) added? Doesn't this mean that .tbss would
>> be twice the size?
>> Is this expected? If so, would you mind adding a comment in the commit
>> message clearifying this?
>>
>>> +
>>>       /* Read-write data (initialized) */
>>>       . = ALIGN(0x1000);
>>>       _data = .;
>>> diff --git a/plat/linuxu/Linker.uk b/plat/linuxu/Linker.uk
>>> index ef4b201c..e0cd1743 100644
>>> --- a/plat/linuxu/Linker.uk
>>> +++ b/plat/linuxu/Linker.uk
>>> @@ -1,4 +1,5 @@
>>>   LINUXU_LDFLAGS-y += -Wl,-e,_liblinuxuplat_start
>>> +EXTRA_LD_SCRIPT-y  += $(CONFIG_UK_BASE)/plat/linuxu/link.ld
>>>     ##
>>>   ## Link image
>>> diff --git a/plat/linuxu/link.ld b/plat/linuxu/link.ld
>>> new file mode 100644
>>> index 00000000..3f5cd682
>>> --- /dev/null
>>> +++ b/plat/linuxu/link.ld
>>> @@ -0,0 +1,22 @@
>>> +SECTIONS
>>> +{
>>> +    . = ALIGN(0x8);
>>> +    _tls_start = .;
>>> +    .tdata :
>>> +    {
>>> +        *(.tdata)
>>> +        *(.tdata.*)
>>> +        *(.gnu.linkonce.td.*)
>>> +    }
>>> +    _etdata = .;
>>> +    .tbss :
>>> +    {
>>> +        *(.tbss)
>>> +        *(.tbss.*)
>>> +        *(.gnu.linkonce.tb.*)
>>> +        . = ALIGN(0x8);
>>> +    }
>>> +    _tls_end = . + SIZEOF(.tbss);
>>> +}
>>> +
>>> +INSERT BEFORE .data
>>> diff --git a/plat/xen/x86/link64.lds b/plat/xen/x86/link64.lds
>>> index 82ab653a..c8af67c0 100644
>>> --- a/plat/xen/x86/link64.lds
>>> +++ b/plat/xen/x86/link64.lds
>>> @@ -67,6 +67,24 @@ SECTIONS
>>>       . = ALIGN(4096);
>>>       _ectors = .;
>>>   +    . = ALIGN(0x8);
>>> +    _tls_start = .;
>>> +    .tdata :
>>> +    {
>>> +        *(.tdata)
>>> +        *(.tdata.*)
>>> +        *(.gnu.linkonce.td.*)
>>> +    }
>>> +    _etdata = .;
>>> +    .tbss :
>>> +    {
>>> +        *(.tbss)
>>> +        *(.tbss.*)
>>> +        *(.gnu.linkonce.tb.*)
>>> +        . = ALIGN(0x8);
>>> +    }
>>> +    _tls_end = . + SIZEOF(.tbss);
>>> +
>>>       /* Data */
>>>       _data = .;
>>>       .data : {
>>>
> 

_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel