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

[Wei Chen <Wei.Chen@xxxxxxx>]

Hi Julien,

> -----Original Message-----
> From: Julien Grall <julien.grall@xxxxxxx>
> Sent: 2018年7月8日 6:24
> To: Wei Chen <Wei.Chen@xxxxxxx>; minios-devel@xxxxxxxxxxxxxxxxxxxx;
> simon.kuenzer@xxxxxxxxx
> Cc: Kaly Xin <Kaly.Xin@xxxxxxx>; nd <nd@xxxxxxx>
> Subject: Re: [Minios-devel] [UNIKRAFT PATCHv4 21/43] plat/kvm: Add Arm64 basic
> entry code
> 
> 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.
> 

It's little hard for me to answer your question. This is why I reply this
Comment at the last. Actually, when I was selecting the elf image I didn’t
think so much. And most Unikernel projects that I have involved (ukvm, mini-os)
are using the elf image, both for arm and x86.

So I don't know and understand your concern. Could please give me a
detail of "clear description of the state of the VM at boot" ?

> For instance, it is not clear what is the state of the cache, SCTLR...

If we use other format image can we get above information? How does it do this?

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

I don't have the pointer, I just refer to FreeBSD's steps.
 
> >
> > 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.
> 

Why did you have such feeling? I think my stack is 16-bytes alignment.

> > +   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?

Ok.

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