|
[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
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>
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
--
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 |