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

[Costin Lupu <costin.lup@xxxxxxxxx>]

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.

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