Re: [Minios-devel] [UNIKRAFT PATCHv4 21/43] plat/kvm: Add Arm64 basic entry code

[Julien Grall <julien.grall@xxxxxxx>]

Hi,

On 07/06/2018 10:03 AM, Wei Chen wrote:
> QEMU/KVM can boot an Arm64 elf image without multiboot. In this case,
> we can plage _libkvmplat_entry to entry64.S directly as the vCPU
> reset entry. In this basic entry code, we just initialize the boot
> stack and prepare jumping to _libkvmplat_start.
Can you clarify why you are using the ELF format and not Image? My main 
concern is the former does not seem to have a clear description of the 
state of the VM at boot.
For instance, it is not clear what is the state of the cache, SCTLR... 
You also assume the MMU is turned on. Do you have a pointer on what is 
the expected state at boot? This would be quite useful to review the 
boot code.
> Signed-off-by: Wei Chen <Wei.Chen@xxxxxxx>
> ---
>   plat/kvm/arm/entry64.S | 36 ++++++++++++++++++++++++++++++
>   plat/kvm/arm/setup.c   | 50 ++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 86 insertions(+)
>   create mode 100644 plat/kvm/arm/entry64.S
>   create mode 100644 plat/kvm/arm/setup.c
>
> diff --git a/plat/kvm/arm/entry64.S b/plat/kvm/arm/entry64.S
> new file mode 100644
> index 0000000..8a8a2e0
> --- /dev/null
> +++ b/plat/kvm/arm/entry64.S
> @@ -0,0 +1,36 @@
> +#include <uk/arch/limits.h>
> +#include <arm/cpu_defs.h>
> +
> +.data
> +.globl _dtb
> +
> +#define BOOT_STACK_SIZE PAGE_SIZE
> +
> +/*
> + * The registers used by _libkvmplat_start:
> + * x0 - FDT pointer
> + */
> +
> +.text
> +ENTRY(_libkvmplat_entry)
> +       /* Boot stack is placed after pagetable area temporarily */
> +       ldr x26, =_end
> +       add x26, x26, #PAGE_TABLE_SIZE
> +       add x27, x26, #BOOT_STACK_SIZE
> +
> +       /* Clean the boot stack */
> +1:
> +       stp xzr, xzr, [x26], #16
> +       stp xzr, xzr, [x26], #16
> +       stp xzr, xzr, [x26], #16
> +       stp xzr, xzr, [x26], #16
I guess you expect the stack to be 64-byte aligned? If so, It would be 
nice to write it down in a comment.
> +       cmp x26, x27
> +       b.lo 1b
> +
> +       mov sp, x27
> +
> +
> +       /* Load dtb address to x0 as a parameter */
> +       ldr x0, =_dtb
> +       b _libkvmplat_start
> +END(_libkvmplat_entry)
> diff --git a/plat/kvm/arm/setup.c b/plat/kvm/arm/setup.c
> new file mode 100644
> index 0000000..a5581b7
> --- /dev/null
> +++ b/plat/kvm/arm/setup.c
> @@ -0,0 +1,50 @@
> +/* SPDX-License-Identifier: ISC */
> +/*
> + * Authors: Dan Williams
> + *               Martin Lucina
> + *               Ricardo Koller
> + *               Felipe Huici <felipe.huici@xxxxxxxxx>
> + *               Florian Schmidt <florian.schmidt@xxxxxxxxx>
> + *               Simon Kuenzer <simon.kuenzer@xxxxxxxxx>
> + *               Wei Chen <Wei.Chen@xxxxxxx>
> + *
> + * Copyright (c) 2015-2017 IBM
> + * Copyright (c) 2016-2017 Docker, Inc.
> + * Copyright (c) 2017 NEC Europe Ltd., NEC Corporation
> + * Copyright (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.
> + */
> +
> +#include <string.h>
> +#include <libfdt.h>
> +#include <kvm/console.h>
> +#include <uk/arch/limits.h>
> +#include <uk/plat/console.h>
> +#include <uk/assert.h>
> +#include <uk/essentials.h>
> +
> +static void _init_cpufeatures(void)
> +{
> +       /* TODO */
> +}
How about adding this function in the patch filling the body?

> +
> +void _libkvmplat_start(void *dtb_pointer)
> +{
> +       _init_cpufeatures();
> +       _libkvmplat_init_console();
> +
> +       uk_printd(DLVL_INFO, "Entering from KVM (arm64)...\n");
> +}
Cheers,

--
Julien Grall

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