Re: [Minios-devel] [UNIKRAFT PATCHv4 19/43] plat/kvm: Add link script for Arm64

[Julien Grall <julien.grall@xxxxxxx>]

Hi,

On 07/06/2018 10:03 AM, Wei Chen wrote:
> This link script is based on x86 version, except following
> differences:
> 1. Arm64 needs DTB to parse devices, and QEMU/KVM will revserve
s/revserve/reserved/

>     the first 64KB of RAM as DTB area. In this case, we add a DTB
>     section to this link script.
Do you mind giving a pointer to the QEMU/KVM layout? But then, do we 
really want to tie the linker script to QEMU layout?
> 2. We will use mmu to control each section's attribute, so the
>     boundaries of sections with different memory attributes must
>     be 4KiB alignment. For instance, the dtb section is readonly,
>     but the text section  is readonly+exec. So the _text must start
>     at a 4KiB alignment  address.
>
> Signed-off-by: Wei Chen <Wei.Chen@xxxxxxx>
> ---
>   plat/kvm/arm/link64.ld | 111 +++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 111 insertions(+)
>   create mode 100644 plat/kvm/arm/link64.ld
>
> diff --git a/plat/kvm/arm/link64.ld b/plat/kvm/arm/link64.ld
> new file mode 100644
> index 0000000..8798a5b
> --- /dev/null
> +++ b/plat/kvm/arm/link64.ld
> @@ -0,0 +1,111 @@
> +/* SPDX-License-Identifier: ISC */
See previous e-mail on SPDX.

> +/*
> + * Author(s): Dan Williams <djwillia@xxxxxxxxxx>
> + *            Martin Lucina <martin.lucina@xxxxxxxxxx>
> + *            Simon Kuenzer <simon.kuenzer@xxxxxxxxx>
> + *            Wei Chen <wei.chen@xxxxxxx>
> + *
> + * Copyright (c) 2016, IBM
> + *           (c) 2016-2017 Docker, Inc.
> + *           (c) 2017, NEC Europe Ltd.
> + *           (c) 2018, Arm Ltd.
> + *
> + * Permission to use, copy, modify, and/or distribute this software
> + * for any purpose with or without fee is hereby granted, provided
> + * that the above copyright notice and this permission notice appear
> + * in all copies.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL
> + * WARRANTIES WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED
> + * WARRANTIES OF MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE
> + * AUTHOR BE LIABLE FOR ANY SPECIAL, DIRECT, INDIRECT, OR
> + * CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM LOSS
> + * OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT,
> + * NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN
> + * CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
> + */
> +
> +OUTPUT_FORMAT("elf64-littleaarch64")
> +OUTPUT_ARCH(aarch64)
> +ENTRY(_libkvmplat_entry)
> +
> +/*
> + * We use mmu to control each section's attribute, so the boundaries
> + * of sections with different memory attributes must be 4KiB alignment.
> + * For instance, the dtb section is readonly, but the text section
> + * is readonly+exec. So the _text must start at a 4KiB alignment
> + * address.
> + */
> +SECTIONS {
> +       /* QEMU-AArch64 virt platform's ram base address */
> +       . = 0x40000000;
> +
> +       /* Reserve first 64 KBytes for DTB */
> +       _dtb = .;
> +       . = . + 0x10000;
Please use a macro here to define 0x10000. This will be easier to see 
what is done.
> +
> +       /* Code */
> +       _text = .;
> +       .text :
> +       {
> +               *(.text)
> +               *(.text.*)
> +       }
> +
> +       . = ALIGN(0x1000);
> +       _etext = .;
> +
> +       /* Read-only data */
> +       _rodata = .;
> +       .rodata :
> +       {
> +               *(.rodata)
> +               *(.rodata.*)
> +       }
> +       .eh_frame :
> +       {
> +               *(.eh_frame)
> +       }
> +       _erodata = .;
> +
> +       /* Constructor tables (read-only) */
> +       _ctors = .;
> +       .preinit_array : {
> +               . = ALIGN(0x8);
> +               PROVIDE_HIDDEN (__preinit_array_start = .);
> +               KEEP (*(.preinit_array))
> +               PROVIDE_HIDDEN (__preinit_array_end = .);
> +       }
> +
> +       .init_array : {
> +               . = ALIGN(0x8);
> +               PROVIDE_HIDDEN (__init_array_start = .);
> +               KEEP (*(SORT_BY_INIT_PRIORITY(.init_array.*) 
> SORT_BY_INIT_PRIORITY(.ctors.*)))
> +               KEEP (*(.init_array .ctors))
> +               PROVIDE_HIDDEN (__init_array_end = .);
> +       }
> +       . = ALIGN(0x1000);
I guess, you want this to be page-aligned. If so, please use PAGE_SIZE.

> +       _ectors = .;
I would move this before the . = ALIGN(...) to make clear the alignment 
is required for data.
> +
> +       /* Read-write data (initialized) */
> +       _data = .;
> +       .data :
> +       {
> +               *(.data)
> +               *(.data.*)
> +       }
> +       _edata = .;
> +
> +       . = ALIGN(0x1000);
Same here.

> +       __bss_start = .;
> +       /* Read-write data (uninitialized) */
The word "uninitialized" is a bit misleading here. In C, this section 
should be Zeroed. So there are not really "unitialized".
This raise the question of who is going to initialize that region?

> +       .bss :
> +       {
> +               *(.bss)
> +               *(.bss.*)
> +               *(COMMON)
> +               . = ALIGN(0x1000);
> +       }
> +
> +       _end = .;
> +}
Cheers,

--
Julien Grall

--
Julien Grall

_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel