Re: [Minios-devel] [UNIKRAFT PATCHv4 00/43] Add arm64/kvm support for Unikraft

[Julien Grall <julien.grall@xxxxxxx>]

On 09/07/18 08:20, Wei Chen wrote:
> Hi Julien,
Hi Wei,

> > -----Original Message-----
> > From: Julien Grall <julien.grall@xxxxxxx>
> > Sent: 2018年7月8日 5:39
> > 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 00/43] Add arm64/kvm support for
> > Unikraft
> >
> > Hi Wei,
> >
> > The title says v4 but I don't seem to find the previous versions on the
> > ML. Did I miss anything?
> Sorry, I sent two versions in mailing list using the name
> "Prepare build scripts to support ARM64". And sent v3 in github including
> Arm64 code for demo. Because at that time, Simon was very busy, and he
> Wasn't ready to review my code. The v4 was generated by my local script,
> I think it should be v3 in this ML.
Please mention such renaming in the cover letter in the future.

> > On 07/06/2018 10:03 AM, Wei Chen wrote:
> > > This patch series enable Unikraft on arm64/kvm. As we
> > > haven't implemented GIC libraries and full timer support,
> > > this patch series can ONLY work without uksched.
> > >
> > > What we have done in this patch series:
> > > 1. Modified the build scripts and restructured
> > >      the folders to improve the  multi-arch and multi-plat
> > >      support,
> > > 2. Added boot code for Arm64 QEMU-KVM platform,
> > So you are only targeting KVM with QEMU? kvmtools (quite useful for
> > lighter development) or any other will not work?
> As I had discussed with Simon before, we planned to enable Arm64/KVM on
> QEMU first. Because QEMU-KVM is the de-facto implementation of KVM.
> After features on QEMU-KVM become mature, we will add other hypervisors
> like kvmtools and ukvm later.
kvmtools and ukvm are not different hypervisors. They are different 
tools/monitor to create your VM.
So your Unikraft binary would be tailored to a given monitor? Or you do 
you plan to support all of them in one?
> > > 3. Enabled MMU and setup a 1:1 mapping page table for
> > >      physical memory and virtual memory,
> > > 4. Added an exception table to handle SYNC, IRQ and other
> > >      exceptions (Just dumping registers in this stage),
> > > 5. Supported device tree,
> > I am a bit confused with the reason of adding DT here. I would have
> > thought it was for getting the code as generic as possible, but a lot of
> > this series makes the assumption on the memory layout.
> >
> > I would rather avoid a mix of both world (DT vs Hardcoded). This is
> > making the code more difficult to read and maintain.
> Sorry, I don't understand your comment here clear. What did "assumption on
> the memory layout" mean here? My platform is QEMU-KVM virt, I think the
> memory layout is clear and fixed.
>
> Except the PL011 UART for early debug, I parsed memory, command line,
> PSCI and PL011 for console from device tree. I will remove the PL011 early
> Debug later, if we will not need it anymore.
The main goal of Device-Tree is to abstract the platform and therefore 
avoiding hardcoded and make your kernel image more generic.
If we take the example of the PL011, you are mapping the region at a 
fixed address when creating the page-tables but get the base-address 
from the Device-Tree. What is the benefits to have some time use DT and 
sometimes hardcode the value?
My point is, if you know your platform layout, then you have no need to 
use Device-Tree to get the base address for PL011 or whether PSCI will 
use HVC/SMC.
If you plan to tailor your kernel image to a given platform then you 
should not use DT for other bits than memory size/command line.
If you plan to have generic image, then you should use Device-Tree 
everywhere.
Both of them have pros/cons, I would prefer the latter but also 
understand the reason of using the latter. What I would like to avoid is 
a mix of both solutions, this is the worst of all.
> And I hardcored the offsets of DTB, page table and stack. But I think
> these Offsets couldn't be parsed from device tree.
I wasn't referring to that :).

> > > 6. A simple PSCI library for CPU suspend, reset and system
> > >      shutdown
> > > 7. PL011 UART for console and STDIO
> > > 8. A simple virtual timer library for debug timestamp.
> > > Wei Chen (43):
> > To help the review it would be nice if you could split in smaller series.
> At the very beginning, Simon and I decided to send the first series
> Without any arm64/KVM support code, just including the folder layout
> and scripts modification. But after that, Unikraft added lots of code.
> The multi-arch modification inevitably affected the plat/ folders.
> We moved some code to plat/common. So I combined two series into one.
> I think this would give reviewers an direct overview.
TBH, I don't see any benefits to have the arm64 code for the reworking. 
The main issue with big series is the length of reviewing and can also 
be discouraging for the contributors as it requires to be spotless on 
changes.
Cheers,

--
Julien Grall

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