[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 06/01/2018 12:48 PM, Simon Kuenzer wrote:
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.
I was referring to the implementation suggested with the enum and
iterating through the enum information.
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.
There would be code duplication because we are going to iterate through
the list enumerator function inside each of the platform libraries.To
avoid this we could have static inline helper function to perform the
iteration of the memory regions and this can be included in the
different platform libraries.
What is the suggested approach? Sorry, I can't follow you. ;-)
Oops!!
I have added some more information above and hope it is more understandable.
* 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.
Oops sorry my bad. I should have looked further into it.
But in the example of heap initialization, the allocator requires
information on the location of heap base and not the entire memory map
information. We could also export the heap information required to the
ukboot library rather than the entire memory region.
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.
No, the platform does not have to be aware of the allocator. I do not
think that would be good choice.
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.
I think that would be better approach as the platform is responsible for
that memory region information and it will have to define APIs to
externally visible information thereby platform library could control
the qualifiers on the information exported.
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
|
