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

Re: [Minios-devel] [UNIKRAFT PATCH v3 2/7] plat/kvm: Align linker script with Xen platform ones



Hello Simon,

Please find the comments in line


On 05/22/2018 02:20 PM, Simon Kuenzer wrote:
The linker scripts of Xen and KVM diverged too much. This patch
is aligning KVMs with the ones from the Xen platform:

- Unify symbols provided by linker script that mark start and end
   of sections
- Remove currently unused and unsupported eh_frame section
   It may be added again when we officially introduce support
   together with the other platforms
- Use tabs for identation
- Keep multiboot header just once

Signed-off-by: Simon Kuenzer<simon.kuenzer@xxxxxxxxx>
---
  plat/kvm/memory.c      | 32 +++++++++++------
  plat/kvm/x86/entry64.S |  2 +-
  plat/kvm/x86/link64.ld | 97 ++++++++++++++++++++++----------------------------
  plat/kvm/x86/setup.c   |  2 +-
  4 files changed, 67 insertions(+), 66 deletions(-)

diff --git a/plat/kvm/memory.c b/plat/kvm/memory.c
index cfb15a6..705c6df 100644
--- a/plat/kvm/memory.c
+++ b/plat/kvm/memory.c
@@ -32,20 +32,21 @@ extern void *_libkvmplat_mem_end;
int ukplat_memregion_count(void)
  {
-       return 5;
+       return 6;
  }
int ukplat_memregion_get(int i, struct ukplat_memregion_desc *m)
  {
-       extern char _stext[], _etext[], _erodata[], _end[];
+       extern char _text, _etext, _data, _edata, _rodata, _erodata,
+                   __bss_start, _end;
        int ret;
UK_ASSERT(m); switch (i) {
        case 0: /* text */
-               m->base  = &_stext;
-               m->len   = (size_t) &_etext - (size_t) &_stext;
+               m->base  = &_text;
+               m->len   = (size_t) &_etext - (size_t) &_text;
                m->flags = (UKPLAT_MEMRF_RESERVED
                            | UKPLAT_MEMRF_READABLE);
  #if UKPLAT_MEMRNAME
@@ -54,8 +55,8 @@ int ukplat_memregion_get(int i, struct ukplat_memregion_desc 
*m)
                ret = 0;
                break;
        case 1: /* rodata */
-               m->base  = &_etext;
-               m->len   = (size_t) &_erodata - (size_t) &_etext;
+               m->base  = &_rodata;
+               m->len   = (size_t) &_erodata - (size_t) &_rodata;
                m->flags = (UKPLAT_MEMRF_RESERVED
                            | UKPLAT_MEMRF_READABLE);
  #if UKPLAT_MEMRNAME
@@ -64,8 +65,8 @@ int ukplat_memregion_get(int i, struct ukplat_memregion_desc 
*m)
                ret = 0;
                break;
        case 2: /* data */
-               m->base  = &_erodata;
-               m->len   = (size_t) &_end - (size_t) &_erodata;
+               m->base  = &_data;
+               m->len   = (size_t) &_edata - (size_t) &_data;
                m->flags = (UKPLAT_MEMRF_RESERVED
                            | UKPLAT_MEMRF_READABLE
                            | UKPLAT_MEMRF_WRITABLE);
@@ -74,7 +75,18 @@ int ukplat_memregion_get(int i, struct ukplat_memregion_desc 
*m)
  #endif
                ret = 0;
                break;
-       case 3: /* heap */
+       case 3: /* bss */
+               m->base  = &__bss_start;
+               m->len   = (size_t) &_end - (size_t) &__bss_start;
+               m->flags = (UKPLAT_MEMRF_RESERVED
+                           | UKPLAT_MEMRF_READABLE
+                           | UKPLAT_MEMRF_WRITABLE);
+#if UKPLAT_MEMRNAME
+               m->name  = "bss";
+#endif
+               ret = 0;
+               break;
+       case 4: /* heap */
                m->base  = _libkvmplat_heap_start;
                m->len   = (size_t) _libkvmplat_stack_top
                           - (size_t) _libkvmplat_heap_start;
@@ -84,7 +96,7 @@ int ukplat_memregion_get(int i, struct ukplat_memregion_desc 
*m)
  #endif
                ret = 0;
                break;
-       case 4: /* stack */
+       case 5: /* stack */
                m->base  = _libkvmplat_stack_top;
                m->len   = (size_t) _libkvmplat_mem_end
                           - (size_t) _libkvmplat_stack_top;
Would it not be useful to use platform specific private data structure to determine the number of section and the index of each section? In the current implementation we are using magic number. This suggested change does not affect the purpose of this patch. As a result we can take in this patch perform the modification in another patch set.
diff --git a/plat/kvm/x86/entry64.S b/plat/kvm/x86/entry64.S
index 2d14386..47980ad 100644
--- a/plat/kvm/x86/entry64.S
+++ b/plat/kvm/x86/entry64.S
@@ -46,7 +46,7 @@ _multiboot_header:
  .long _multiboot_header
  .long 0x100000
  .long _edata
-.long _ebss
+.long _end
  .long _libkvmplat_start32
.section .bss
diff --git a/plat/kvm/x86/link64.ld b/plat/kvm/x86/link64.ld
index 85ea058..a9f3ac3 100644
--- a/plat/kvm/x86/link64.ld
+++ b/plat/kvm/x86/link64.ld
@@ -6,7 +6,7 @@
   *
   * Copyright (c) 2016, IBM
   *           (c) 2016-2017 Docker, Inc.
- *           (c) 2017, NEC Europe Ltd.
+ *           (c) 2017-2018, NEC Europe Ltd.
   *
   * Permission to use, copy, modify, and/or distribute this software
   * for any purpose with or without fee is hereby granted, provided
@@ -24,63 +24,52 @@
   */
ENTRY(_libkvmplat_entry)
+SECTIONS
+{
+       . = 0x100000;
-SECTIONS {
-    . = 0x100000;
+       /* Code */
+       _text = .;
+       .text :
+       {
+               /* prevent linker gc from removing multiboot header */
+               KEEP (*(.data.multiboot))
- /* Code */
-    _stext = .;
+               *(.text)
+               *(.text.*)
+       }
+       _etext = .;
- .text :
-    {
-        *(.data.multiboot)
-        /* prevent linker gc from removing multiboot header */
-        KEEP(*(.data.multiboot))
-        *(.text)
-        *(.text.*)
-    }
+       /* Read-only data */
+       . = ALIGN(0x1000);
+       _rodata = .;
+       .rodata :
+       {
+               *(.rodata)
+               *(.rodata.*)
+       }
+       _erodata = .;
- _etext = .;
+       /* Read-write data (initialized) */
+       . = ALIGN(0x1000);
+       _data = .;
+       .data :
+       {
+               *(.data)
+               *(.data.*)
+       }
+       _edata = .;
- . = ALIGN(0x1000);
-    /* Read-only data */
-    .rodata :
-    {
-        *(.rodata)
-        *(.rodata.*)
-    }
-    .eh_frame :
-    {
-        *(.eh_frame)
-    }
+       /* Read-write data (uninitialized) */
+       . = ALIGN(0x1000);
+       __bss_start = .;
+       .bss :
+       {
+               *(.bss)
+               *(.bss.*)
+               *(COMMON)
+               . = ALIGN(0x1000);
+       }
- _erodata = .;
-
-    . = ALIGN(0x1000);
-    /* Read-write data (initialized) */
-    .got :
-    {
-        *(.got.plt)
-        *(.got)
-    }
-    .data :
-    {
-        *(.data)
-        *(.data.*)
-    }
-
-    _edata = .;
-
-    . = ALIGN(0x1000);
-    /* Read-write data (uninitialized) */
-    .bss :
-    {
-        *(.bss)
-        *(.bss.*)
-        *(COMMON)
-    }
-
-    . = ALIGN(0x1000);
-    _ebss = .;
-    _end = .;
+       _end = .;
  }
diff --git a/plat/kvm/x86/setup.c b/plat/kvm/x86/setup.c
index 6895f29..332d10e 100644
--- a/plat/kvm/x86/setup.c
+++ b/plat/kvm/x86/setup.c
@@ -78,7 +78,7 @@ static inline void _mb_get_cmdline(struct multiboot_info *mi, 
char *cmdline,
static inline void _mb_init_mem(struct multiboot_info *mi)
  {
-       extern char _end[];
+       extern char _end;
        multiboot_memory_map_t *m;
        size_t offset, max_addr;
Reviewed-by: Sharan Snathanam <sharan.santhanam@xxxxxxxxx>


Thanks & Regards
Sharan Santhanam

_______________________________________________
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®.