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

[Florian Schmidt <Florian.Schmidt@xxxxxxxxx>]

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.
> 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 : {
--
Dr. Florian Schmidt
フローリアン・シュミット
Research Scientist,
Systems and Machine Learning Group
NEC Laboratories Europe
Kurfürsten-Anlage 36, D-69115 Heidelberg
Tel.     +49 (0)6221 4342-265
Fax:     +49 (0)6221 4342-155
e-mail:  florian.schmidt@xxxxxxxxx
============================================================
Registered at Amtsgericht Mannheim, Germany, HRB728558

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