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

Re: [UNIKRAFT PATCH v2] include: Move sections.h to include/uk



Hi all,

@Michalis
I think, a problem that we still architecturally have is that we compile general purpose libs (like ukboot) once and link them multiple times for each selected platform. This implies that exposing platform-specific symbols to general purpose code is tricky. In general purpose code, you can only rely on symbols that every platform is providing. This is why I think that such specific symbols (like _dtb) should stay within the platform and should be (indirectly) exported (if at all needed) through a driver~ish or query API, something similar we have with the memory regions, uknetdev, and ukblkdev. The original idea was to make sure that general purpose applications stay portable across different platforms. Of course, this consideration does not include the case where you want to develop something just for a single platform, like a driver domain for Xen which would anyways not be portable to, lets say KVM. For now you would be forced to put such code as platform library which isn't nice, I agree. I think this needs a solution in general. Maybe the build system could behave differently and unlock additional defintions when we select only a single platform.

@Stefan
Regarding moving sections.h to the generic `include/` folder, I still did not really understand the reason and it is unclear to me what should be the defined behavior in case of ASLR. In comparison, what does happen with PIE executables on Linux? To my understanding, the linker will resolve the symbols with the addresses in the image at compile time. Do you know if PIE executables make use of the dynamic linker for resolving? In such a case, this is a less problematic point.

Alternatively, would the memory region query interface fit, too? The idea of this would to overcome the problem that some things may change at runtime (e.g., early platform boot code) compared to compile time.

I think, we have two options:

(1) In the case that the dynamic linker is normally taking care of solving such symbols for position-independent executables, I think it should be fine to place the header to `include/uk/plat/sections.h`. The idea would be to revisit it later when we introduce ASLR. In general we should make sure that we define only the symbols in there which all platforms linker scripts provide (like the ones for text, bss, etc). Platform-specific symbols should stay in the platform, like `_dtb`.

(2) Do not expose section.h to general purpose code and use the memory region interface and maybe extend it with what you need.

What do you think?

Thanks,

Simon

On 28.10.20 15:26, Michalis Pappas wrote:
Before exposing sections.h to applications, I would like to take the opportunity to discuss some architectural matters.

Currently linker scripts and their associated symbol definitions are owned by platforms. At the same time, sections.h is under plat/common/, and contains some common symbol names that are more or less declared by all platforms. Platforms wishing to declare additional, and perhaps less common symbol names, have the option to either (1) pollute sections.h, or (2) add these symbols to a platform-specific header. In case (1) sections.h will eventually contain a superset of all section symbols declared by all platforms, which is dirty. In case (2) platforms lose the ability to export their additional symbols to applications, which is also a problem.

One possible approach to work around this would be to make symbols.h platform-specific, and then allow platforms to export platform-specific headers under platform subdirectories in include/uk/plat/ (eg include/uk/plat/kvm/sections.h), or alternatively export the current sections.h through include/uk/ and export additional platform-specific headers through platform subdirectories in include/uk/plat/, as above.

Thanks,

Michalis

On 28/10/2020 14:45, Stefan Teodorescu wrote:
Hi everyone,

Sorry for the confusion, this is my fault that I didn't clarify enough
in the commit message and that I didn't update it in the meantime.

I am using the sections.h header in the mem_layout.h header (which is
also in include/uk) that I am writing now to describe the virtual
memory areas (kernel, stack, heap etc.). At the moment, I think I was
using the sections.h header in some arch code but this is not the case
anymore. However, as Costin mentioned, we thought at the time that we
could move this anyway, since it's not actually platform dependent.

I'll send a v2 with an updated commit message.

Thanks,
Stefan



On Wed, Oct 28, 2020 at 3:19 PM Costin Lupu <costin.lupu@xxxxxxxxx> wrote:
Hi Simon,

Regardless whether these changes are needed or not by arch code (I'll
leave that for Ștefan to explain), the definitions in sections.h should
stay under include/uk in order to be accessible from everywhere.
Moreover, the definitions in it are related to neither arch or platform.

Cheers,
Costin

On 10/28/20 3:14 PM, Simon Kuenzer wrote:
Hey Stefan, hey Costin,

what is the reason that arch code needs sections.h? I can't read it from
the commit message. Architecture code should only be the parts that are
totally independent on the platform details. They are mostly helpers for
arithmetics and CPU register definitions. I am concerned that
architecture code may break when doing assumptions on symbol locations;
imagine ASLR where locations are moved at runtime.

The NX bit patch is upstream.

Thanks,

Simon

On 26.10.20 17:14, stefanl.teodorescu@xxxxxxxxx wrote:
From: Stefan Teodorescu <stefanl.teodorescu@xxxxxxxxx>

Move plat/common/include/uk/plat/common/sections.h to
include/uk/sections.h to be able to include it in arch code. This file
is platform independent, so it does not affect anything.

Signed-off-by: Stefan Teodorescu <stefanl.teodorescu@xxxxxxxxx>
---
   {plat/common/include/uk/plat/common => include/uk}/sections.h | 0
   plat/kvm/arm/entry64.S                                        | 2 +-    plat/kvm/arm/setup.c                                          | 2 +-    plat/kvm/memory.c                                             | 2 +-    plat/kvm/x86/setup.c                                          | 2 +-    plat/xen/arm/setup.c                                          | 2 +-    plat/xen/include/xen-arm/mm.h                                 | 2 +-    plat/xen/include/xen-x86/mm.h                                 | 2 +-    plat/xen/memory.c                                             | 2 +-    plat/xen/x86/mm.c                                             | 2 +-
   10 files changed, 9 insertions(+), 9 deletions(-)
   rename {plat/common/include/uk/plat/common => include/uk}/sections.h
(100%)

diff --git a/plat/common/include/uk/plat/common/sections.h
b/include/uk/sections.h
similarity index 100%
rename from plat/common/include/uk/plat/common/sections.h
rename to include/uk/sections.h
diff --git a/plat/kvm/arm/entry64.S b/plat/kvm/arm/entry64.S
index c4de334c..8be3d59f 100644
--- a/plat/kvm/arm/entry64.S
+++ b/plat/kvm/arm/entry64.S
@@ -35,7 +35,7 @@
   #include <uk/asm.h>
   #include <kvm-arm/mm.h>
   #include <arm/cpu_defs.h>
-#include <uk/plat/common/sections.h>
+#include <uk/sections.h>
   #include <uk/config.h>

   .global page_table_size
diff --git a/plat/kvm/arm/setup.c b/plat/kvm/arm/setup.c
index 41e63755..8513b48c 100644
--- a/plat/kvm/arm/setup.c
+++ b/plat/kvm/arm/setup.c
@@ -20,7 +20,7 @@
    */
   #include <uk/config.h>
   #include <libfdt.h>
-#include <uk/plat/common/sections.h>
+#include <uk/sections.h>
   #include <kvm/console.h>
   #include <kvm/config.h>
   #include <uk/assert.h>
diff --git a/plat/kvm/memory.c b/plat/kvm/memory.c
index 1d9269ec..61d3e6ca 100644
--- a/plat/kvm/memory.c
+++ b/plat/kvm/memory.c
@@ -19,7 +19,7 @@
    * CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
    */

-#include <uk/plat/common/sections.h>
+#include <uk/sections.h>
   #include <sys/types.h>
   #include <uk/plat/memory.h>
   #include <uk/assert.h>
diff --git a/plat/kvm/x86/setup.c b/plat/kvm/x86/setup.c
index 9c7a93a8..1cab0a63 100644
--- a/plat/kvm/x86/setup.c
+++ b/plat/kvm/x86/setup.c
@@ -27,7 +27,7 @@
    */

   #include <string.h>
-#include <uk/plat/common/sections.h>
+#include <uk/sections.h>
   #include <x86/cpu.h>
   #include <x86/traps.h>
   #include <kvm/config.h>
diff --git a/plat/xen/arm/setup.c b/plat/xen/arm/setup.c
index 2df3b46c..bcbc939e 100644
--- a/plat/xen/arm/setup.c
+++ b/plat/xen/arm/setup.c
@@ -25,7 +25,7 @@
   /* Ported from Mini-OS */

   #include <string.h>
-#include <uk/plat/common/sections.h>
+#include <uk/sections.h>
   #include <xen-arm/os.h>
   #include <xen-arm/mm.h>
   #include <xen/xen.h>
diff --git a/plat/xen/include/xen-arm/mm.h
b/plat/xen/include/xen-arm/mm.h
index 659de843..bbd31ddd 100644
--- a/plat/xen/include/xen-arm/mm.h
+++ b/plat/xen/include/xen-arm/mm.h
@@ -28,7 +28,7 @@
   #define _ARCH_MM_H_

   #include <stdint.h>
-#include <uk/plat/common/sections.h>
+#include <uk/sections.h>
   #include <uk/arch/limits.h>

   typedef uint64_t paddr_t;
diff --git a/plat/xen/include/xen-x86/mm.h
b/plat/xen/include/xen-x86/mm.h
index ffbedb09..99ba6d05 100644
--- a/plat/xen/include/xen-x86/mm.h
+++ b/plat/xen/include/xen-x86/mm.h
@@ -25,7 +25,7 @@
   #ifndef _ARCH_MM_H_
   #define _ARCH_MM_H_

-#include <uk/plat/common/sections.h>
+#include <uk/sections.h>
   #ifndef __ASSEMBLY__
   #include <xen/xen.h>
   #if defined(__i386__)
diff --git a/plat/xen/memory.c b/plat/xen/memory.c
index 8e7a7dda..27a317c4 100644
--- a/plat/xen/memory.c
+++ b/plat/xen/memory.c
@@ -34,7 +34,7 @@
    */

   #include <string.h>
-#include <uk/plat/common/sections.h>
+#include <uk/sections.h>

   #include <common/gnttab.h>
   #if (defined __X86_32__) || (defined __X86_64__)
diff --git a/plat/xen/x86/mm.c b/plat/xen/x86/mm.c
index e006ab7f..9e352eed 100644
--- a/plat/xen/x86/mm.c
+++ b/plat/xen/x86/mm.c
@@ -36,7 +36,7 @@
    */

   #include <string.h>
-#include <uk/plat/common/sections.h>
+#include <uk/sections.h>
   #include <errno.h>
   #include <uk/alloc.h>
   #include <uk/plat/config.h>
--
2.29.0






 


Rackspace

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