[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
On 01.06.2018 13:14, Yuri Volchkov wrote:
I would suggest that we proceed with the current version of the patch,
and I and/or Sharan can express suggestions in a form of follow-up
patches. Sometimes code speaks better :)
Reviewed-by: Yuri Volchkov <yuri.volchkov@xxxxxxxxx>
This sounds good, thanks a lot!
Simon
Simon Kuenzer <simon.kuenzer@xxxxxxxxx> writes:
On 01.06.2018 11:02, Sharan Santhanam wrote:
Hello Simon,
On 05/30/2018 04:04 PM, Simon Kuenzer wrote:
On 23.05.2018 13:53, Yuri Volchkov wrote:
Sharan Santhanam <sharan.santhanam@xxxxxxxxx> writes:
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.
I support this too. Also agree that this probably does not belong to
this patch series.
In my opinion a cool way to do this would be like:
enum {
MEMREG_TEXT = 0,
MEMREG_DATA,
MEMREG_BSS,
/* . . . */
MEMREG_COUNT
}
Then, in the ukplat_entry():
for (i = 0; i < MEMREG_COUNT; ++i) {
ukplat_memregion_get(i, &md);
}
Hum, yeah, we could define it within memory.c. This would be done by
each platform individually but we won't export the enum on the
platform API.
I agree the platform enum should not be exported outside the library.
The reason is that we have a restriction coming from the platform
API/ABI and the current building procedure: Each library is built just
once but linked multiple times against different sets of platform
libraries. To keep this working, there should never be code part of a
non-platform library binary which actually belongs to a single or a
subset of platforms only.
This way, we save building time, and we keep libraries independent of
platforms. LTO should safe us for the cases where we normally would
have used `static inline` or macros on the platform API.
* I do not see the difference between the current approach and the
suggested approach in terms of the build time. There could be code
duplication of memory region loading but I think we could also avoid
with platform internal library of function, thereby sharing the plat
object file across the different platform libraries. We may argue that
is what the ukboot library is currently doing this but in my opinion
putting it part of the platform library provides a better distinction
between platform specific information and boot lib.
What is the suggested approach? Sorry, I can't follow you. ;-)
* We could also abstract the information exposed to the boot library
further and call it to initialize the "ukplat_memory_initialize" and let
each platform decide on how to initialize it memory regions. As
currently we exposing the number of memory regions also to the ukboot
library.
The task of the current interface is actually not the initialization of
a memory region (e.g., setting up page table entries). This is still
happening as part of the platform library, but the boot strapping needs
information where to find the address range for initializing the heap
management.
I see the following risk with your suggestion: We would need to make the
platform aware of a particular memory allocator implementation (like
ukalloc) so that such functions are called. However, we also want to
support completely customized cases or cases where the Unikernel builder
wants to implement something by its own for the heap, so I want to have
the platform as self-contained as much as possible.
Alternatively, the platform could define a callback-based API for
handing over memory to allocators. But honestly, I think this makes it
more complicated as the interface we have now: Having a way to query the
platform to get know where which region is available.
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!
Thanks & Regards
Sharan Santhanam
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel
Thanks & Regards
Sharan Santhanam
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel
|
