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

Re: [Minios-devel] [UNIKRAFT PATCH 4/4] arch: boilerplate Arm support for thread-local storage



Hi Costin,

On 4/16/19 3:22 PM, Costin Lupu wrote:
Hi Florian,

I'm not sure what is the expected outcome of this patch, however I
couldn't build on ARM64 because the following files are not included in
the Makefile:

The whole point of this patch was simply to make sure unikraft still compiled on Arm, even if there was no usable functionality in some parts. Apparently, I failed with that task. ;-)

However:

+LIBKVMPLAT_SRCS-$(CONFIG_ARCH_ARM_64) +=
$(UK_PLAT_COMMON_BASE)/thread.c|common
+LIBKVMPLAT_SRCS-$(CONFIG_ARCH_ARM_64) +=
$(UK_PLAT_COMMON_BASE)/sw_ctx.c|common

That doesn't seem right to me either, for two reasons: There is no proper scheduling implemented yet for Arm, if I remember correctly; and sw_ctx.c can't properly work on Arm because it includes x86-specific code at this point in time. I think any build that enables uksched at this point in time fails to build. Maybe that can be fixed by including those two files, but I would expect everything to fail spectacularly as soon as you run it, then.

So yeah, the files in this patch are just there to allow compiling and linking on Arm with the current feature set (scheduling disabled). Plus some very preliminary untested implementation of tls_copy() for Arm64, since I figured I'd do it now that I had read all the documentation fresh in my mind.



Now, I lost track of the ARM changes and I'm not sure if this is WIP or
it should be fixed by these TLS series.

Please see my other comments inline.

On 4/15/19 1:03 PM, Florian Schmidt wrote:
Arm32 is ure boilerplate. Arm64 has some rough guesses about how the

A typo here: ure?

Ah, yes. It should have been "pure boilerplate".


layout should look like, but is untested.

Signed-off-by: Florian Schmidt <florian.schmidt@xxxxxxxxx>
---
  arch/arm/arm/include/uk/asm/thread.h   | 60 ++++++++++++++++++++++
  arch/arm/arm64/include/uk/asm/thread.h | 70 ++++++++++++++++++++++++++
  plat/kvm/arm/link64.lds.S              | 18 +++++++
  plat/xen/arm/link32.lds                | 18 +++++++
  4 files changed, 166 insertions(+)
  create mode 100644 arch/arm/arm/include/uk/asm/thread.h
  create mode 100644 arch/arm/arm64/include/uk/asm/thread.h

diff --git a/arch/arm/arm/include/uk/asm/thread.h 
b/arch/arm/arm/include/uk/asm/thread.h
new file mode 100644
index 00000000..7225209e
--- /dev/null
+++ b/arch/arm/arm/include/uk/asm/thread.h
@@ -0,0 +1,60 @@
+/* SPDX-License-Identifier: BSD-3-Clause */
+/*
+ * Authors: Florian Schmidt <florian.schmidt@xxxxxxxxx>
+ *
+ * Copyright (c) 2019, NEC Europe Ltd., NEC Corporation. All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ *
+ * 1. Redistributions of source code must retain the above copyright
+ *    notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ *    notice, this list of conditions and the following disclaimer in the
+ *    documentation and/or other materials provided with the distribution.
+ * 3. Neither the name of the copyright holder nor the names of its
+ *    contributors may be used to endorse or promote products derived from
+ *    this software without specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
+ * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+ * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE
+ * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
+ * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
+ * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
+ * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
+ * POSSIBILITY OF SUCH DAMAGE.
+ *
+ * THIS HEADER MAY NOT BE EXTRACTED OR MODIFIED IN ANY WAY.
+ */
+
+#ifndef __UKARCH_THREAD_H__
+#error Do not include this header directly
+#endif
+
+#error Thread-local storage not implemented for arm32!
+
+extern char _tls_start[], _etdata[], _tls_end[];
+
+static inline unsigned long tls_area_size(void)
+{
+       return 0;
+}
+
+static inline unsigned int tls_area_align(void)
+{
+       return 1;
+}
+
+static inline void tls_copy(void *tls_area)
+{
+}
+
+static inline void *tls_pointer(void *tls_area)
+{
+       return NULL;
+}
diff --git a/arch/arm/arm64/include/uk/asm/thread.h 
b/arch/arm/arm64/include/uk/asm/thread.h
new file mode 100644
index 00000000..2f5d9b98
--- /dev/null
+++ b/arch/arm/arm64/include/uk/asm/thread.h

Lets follow the same suggestions for this files as the ones mentioned
for the previous patches.

@@ -0,0 +1,70 @@
+/* SPDX-License-Identifier: BSD-3-Clause */
+/*
+ * Authors: Florian Schmidt <florian.schmidt@xxxxxxxxx>
+ *
+ * Copyright (c) 2019, NEC Europe Ltd., NEC Corporation. All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ *
+ * 1. Redistributions of source code must retain the above copyright
+ *    notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ *    notice, this list of conditions and the following disclaimer in the
+ *    documentation and/or other materials provided with the distribution.
+ * 3. Neither the name of the copyright holder nor the names of its
+ *    contributors may be used to endorse or promote products derived from
+ *    this software without specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
+ * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+ * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE
+ * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
+ * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
+ * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
+ * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
+ * POSSIBILITY OF SUCH DAMAGE.
+ *
+ * THIS HEADER MAY NOT BE EXTRACTED OR MODIFIED IN ANY WAY.
+ */
+
+#ifndef __UKARCH_THREAD_H__
+#error Do not include this header directly
+#endif
+
+#warning Thread-local storage has not been tested on aarch64!
+
+extern char _tls_start[], _etdata[], _tls_end[];
+
+static inline unsigned long tls_area_size(void)
+{
+       /* aarch64 ABI adds 16 bytes of TCB at the beginning of the TLS area,
+        * followed by the actual TLS data.
+        */
+       return _tls_end - _tls_start + 16;
+}
+
+static inline unsigned int tls_area_align(void)
+{
+       return 8;
+}
+
+static inline void tls_copy(void *tls_area)
+{
+       unsigned int tls_len = _tls_end - _tls_start;
+       unsigned int tls_data_len = _etdata - _tls_start;
+       unsigned int tls_bss_len = _tls_end - _etdata;
+
+       memset(tls_area, 0, 16);
+       memcpy(tls_area+16, _tls_start, tls_data_len);
+       memset(tls_area+tls_data_len+16, 0, tls_bss_len);

Again, lets stick to Unikraft coding style for the spacing of these 2
lines of code in order to avoid propagating such examples.

Agreed.


+}
+
+static inline void *tls_pointer(void *tls_area)
+{
+       return tls_area;
+}
diff --git a/plat/kvm/arm/link64.lds.S b/plat/kvm/arm/link64.lds.S
index 82328633..1ce22781 100644
--- a/plat/kvm/arm/link64.lds.S
+++ b/plat/kvm/arm/link64.lds.S
@@ -118,6 +118,24 @@ SECTIONS {
        _ectors = .;
        . = ALIGN(__PAGE_SIZE);
+ . = ALIGN(0x8);
+       _tls_start = .;
+       .tdata :
+       {
+               *(.tdata)
+               *(.tdata.*)
+               *(.gnu.linkonce.td.*)
+       }
+       _etdata = .;
+       .tbss :
+       {
+               *(.tbss)
+               *(.tbss.*)
+               *(.gnu.linkonce.tb.*)
+               . = ALIGN(0x8);
+       }
+       _tls_end = . + SIZEOF(.tbss);
+

Again these linker script changes may be refactored into a single linker
file which would be used by all architectures and platforms.

        /* Read-write data that is initialized explicitly in code */
        _data = .;
        .data :
diff --git a/plat/xen/arm/link32.lds b/plat/xen/arm/link32.lds
index 2b8e42dd..e679ba6f 100644
--- a/plat/xen/arm/link32.lds
+++ b/plat/xen/arm/link32.lds
@@ -96,6 +96,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®.