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

[Julien Grall <julien.grall@xxxxxxx>]

On 12/07/18 10:43, Wei Chen wrote:
> Hi Julien,
Hi Wei,

> > -----Original Message-----
> > From: Julien Grall <julien.grall@xxxxxxx>
> > Sent: 2018年7月11日 23:52
> > 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
> >
> >
> >
> > On 11/07/18 10:50, Wei Chen wrote:
> > > Hi Julien,
> > Hi Wei,
> >
> > > > -----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.
> > Mini-OS ARM is using the zImage format, not ELF. For UKVM, IIRC, you
> Yes, arm32 is zImage, but x86_64 is using OUTPUT_FORMAT("elf64-x86-64").
> And about the ukvm, Yes, I wrote it by myself, because the ukvm requires elf
> format, it only support elf loader. We want to make our Unikernel application
> like a normal elf application that can run on virtual machine directly. We don't
> want to be compatible with the Linux image boot protocol, it's too complex for 
> us.
> And we don't have some many information need to pass.
Why is it too complex? The Image format is basically a couple fields to 
slap at the top of your binary and a few guidelines for the tools how to 
deal with the VM state. Most of the boot code will be the same.
Also, most likely if you are going to support kvmtools, you would need 
to use the Image format. For Xen, support for ELF will require a lot of 
reworks and to be honest it is not worth the effort (I tried it before).
> > wrote it yourself. So I guess you based your understanding of the state
> > of the VM from somewhere?
> Each SoC, include QEMU virtual machine, they will give most system registers
> a default value. I am using cortex-a53 VCPU, the QEMU gives it a reset value
> to cpu->reset_sctlr = 0x00c50838; MMU, I/D cache are disabled.
> But yes, you're right, I ignore the MMU has been disabled already, and disable
> It again in entry code.
This looks very fragile to rely on for a specific processor. What if we 
decide to use another Cortex-A*? Or even another Arm CPU?
We need a clear definition of the VM state. For instance, you say the 
cache is disabled. Has the kernel image/DT been cleaned to PoC by the 
tools? You will also have to be careful when writing the page-table as 
"Cache disabled" does not rely mean "disabled". It is more a by-pass of 
the cache.
> > > 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" ?
> > My concern is you don't know what is the state of the memory, caches,
> > CPU mode... For the Image protocol, this is clearly identified in [1].
> >
> > If you tell me ELF has the same description somewhere, then fine. I will
> > be happy to look at it and compare with what you did.
> >
> > > > 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?
> >
> > Yes, this is described in the format documentation [1].
> >
> > > > 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.
> > AFAIK, FreeBSD has its own bootloader based on UEFI. This is different
> > from booting without firmware.
> Yes, you're right, I will adjust the steps according to reset_sctlr
See my answer above.

> > > > > 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.
> > Do you agree that each stp instruction will clear 16-byte? So if you
> > execute 4 stp consecutively without boundary check, you impose the stack
> > to be 64-byte aligned.
> >
> > Did I miss anything?
> Sorry, I still can't understand. Why I clear 16-byte at one instruction need
> 64-byte alignment? Do you have any article for me to get the pointer?
> And why I execute 4 stp consecutively need boundary check?
Every stp instruction will clear 16-byte at a time, there are no 64-byte 
alignment.
However, if we take you assumption that the stack is 16-byte aligned, it 
would be possible to have a stack with only 32-byte.
        1) First stp, will clear byte 0 - 15
        2) Second stp will clear by 16-31
        3) The third stp will clear whatever is after.
        4) The fourth stp will do the same.
        5) The size is compared and notice we are after, so bail out.

3) and 4) will overwrite whatever is after, you don't know what it is. 
You may hit a device, a non-existing region...
So you are assuming the stack size will always 64-byte aligned. That's 
assumption should be written in a comment probably on top of the 
definition of BOOT_STACK_SIZE.
Cheers,

--
Julien Grall

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