|
[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
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);
}
>> 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
--
Yuri Volchkov
Software Specialist
NEC Europe Ltd
Kurfürsten-Anlage 36
D-69115 Heidelberg
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |