[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.