Re: [Minios-devel] [UNIKRAFT] Xen PVH platform

[Simon Kuenzer <simon.kuenzer@xxxxxxxxx>]

Hi Marek,

On 18.03.19, 14:14, "Marek Marczykowski-Górecki" 
<marmarek@xxxxxxxxxxxxxxxxxxxxxx> wrote:

    On Mon, Mar 18, 2019 at 08:57:31AM +0000, Simon Kuenzer wrote:
    > Hey Marek,
    > 
    > On 12.03.19, 17:03, "Minios-devel on behalf of Marek 
Marczykowski-Górecki" <minios-devel-bounces@xxxxxxxxxxxxxxxxxxxx on behalf of 
marmarek@xxxxxxxxxxxxxxxxxxxxxx> wrote:
    > 
    >     On Tue, Mar 12, 2019 at 08:52:11AM +0000, Simon Kuenzer wrote:
    >     > Hey Marek,
    >     > 
    >     > yes, you are right. We are supporting only PV on x86 for now. The 
pieces of PVH/HVM that we have left are left-overs from taking some of the code 
from Mini-OS. In general, we would be happy to have also PVH support.
    >     > So far, our focus was on increasing functionality with libraries 
and drivers in order to make the project more useful for most people. However, 
if you have time, we are happy to receive patches to enable PVH ;-) . Let us 
know if you are interested.
    >     
    >     Yes! Actually, I already have something that boots on PVH. It's quite
    >     specific, as it's plugged into MirageOS, which use unikraft as just 
one
    >     of its packages right now. It's very much work in progress state.
    > 
    > This sounds great! We are happy to receive patches. ;-)
    
    This work in progress state is here:
    https://github.com/marmarek/unikraft/commits/xen-on-freestanding-pvh
    
    Some of the commits are directly applicable for vanilla Unikraft, but
    some are made with MirageOS in mind only.

Wow, this has a long commit history but I am glad to see that you guys got it 
working!
    
    >     > The most natural way for Unikraft would be to build two Xen 
binaries: One for PV and another one for PVH. The idea is that you get the most 
optimized image for your execution environment. This way you would avoid 
impacts because of the two implementations. You also would not require a 
detection at runtime.
    >     
    >     I see, that makes sense. On the other hand, passing CONFIG_PARAVIRT
    >     through multiple layers may be problematic, so I think it may be worth
    >     having some universal binary. But I'll leave it for some undefined
    >     future.
    > 
    > Which layers do you mean? Unikraft? Or the whole Unikernel that you build 
and that includes MirageOS?
    
    The later one.

Okay, got it. Is this also the actual reason why you prefer having just one 
image to cover PV and PVH?
    
    >     Anyway, even without having PV+PVH binary, I have one important 
question:
    >     how start_info_t and HYPERVISOR_start_info should looks like? Right 
now
    >     I've made it an union (with "pv" under CONFIG_PARAVIRT and "hvm" under
    >     CONFIG_XEN_HVMLITE). This require all the places using this symbol to 
be
    >     changed. Some alternative would be to have a separate symbol like
    >     HYPERVISOR_start_info_hvm for the other start info structure and keep
    >     HYPERVISOR_start_info unchanged (or under CONFIG_PARAVIRT). While
    >     changing everything using HYPERVISOR_start_info is some pain, IMO it's
    >     better than forgetting something and erroneously using (uninitialized)
    >     PV version on PVH...
    >     
    >     What do you prefer?
    >     
    > Hum... good question. I would actually check how much differences you see 
in the PV and PVH code, as well as these structs. How many fields are similar 
and how much of the existing code could be re-used? 
    
    The structs are very different. I think the only common part is command
    line and modules (which are ignored by Unikraft anyway). PVH start info
    is much smaller, a lot of things you get from start info on PV have
    different discovery mechanisms on PVH (hypercall page use cpuid + msr,
    memory map, shared info page, xenstore page and console rings use
    hypercalls).
    As for the code, besides different initialization of things that are in
    PV start info struct, the code is quite similar. I think the most
    notable difference is memory management - besides different way of
    obtaining memory map, page tables are constructed slightly differently:
     - there is no need to translate pfn->mfn
     - there is no need for mmu_update hypercall (you can update page tables
       directly, and also write to CR3 directly)
     - page tables are constructed using 2MB pages (this is inherited from
       Mini-OS, but I think it's good idea for unikernels)

Hum, depends. Yes, many unikernels could be already happy with two pages. I am 
not sure what 2MB pages mean for Xen devices and grants. Or would you still 
manage those regions with 4K pages?
    
    > Since I assume building different binaries for PVH and PV, you are able 
to do the build with a different set of sources. 
    
    Yes. I think the most notable difference I use #ifdef-like solution,
    that would require significantly more duplication with runtime detection
    is pfn<->mfn translation. If using different builds, I simply define
    pfn_to_mfn/mfn_to_pfn as 1:1.

I am sorry that I still do not see an advantage of an image that auto-detects 
PV/PVH ;-) . I see it a bit contrary to the Unikraft philosophy where we want 
to specialize before runtime: towards the application and towards the execution 
platform.
I think this is fine because a user or toolstack anyways needs to specify the 
VM mode with a domain configuration. Why couldn’t there the fitting image be 
specified?
And because the build system can output you multiple images with a single build 
the additional costs are minimal.

Now, if we assume that we compile multiple images, the #ifdefs is probably good 
enough. You can compile a source files multiple times with different compiler 
flags: e.g., one build for each target platform (PV, PVH).

    > For instance, entry64.S for PVH can be a complete different one than the 
one for PV. 
    
    Actually, entry64.S is very similar. On PVH I only need to add a little
    prologue to entry point.

Sounds like another #ifdef ;-)
    
    > Some other files will probably have minor cases. I would use an approach 
that looks minimal while keeping readability.
    > Maybe you provide alternative struct definitions depending on the 
configuration?
    
    I was thinking about this too, but it would definitely not work with
    runtime detection (which I would still leave as a possibility, even if
    not implemented right now).
    There is also another issue to consider here - MirageOS currently access
    start info structure too, so having different definitions here means
    MirageOS must be compiled differently for PV and PVH.
    Mindy, can you list things that MirageOS really needs from the start
    info structure? Maybe some abstract interface could be added here, that
    would hide this difference?

Hum... The ideal case would be if there is not such a dependency between mirage 
and a specific platform. This would make mirage run on KVM and possibly other 
platforms out-of-the box. But I understand that you want to run the drivers in 
ocaml and need to know something about the actual platform you run on. The 
solution would be to introduce a kind of generic platform interfaces 
(`ukplat_*()`) that mirage could use to detect if it runs on KVM, Xen, etc. I 
am not sure yet how this would look like. What does Mirage require from the 
platform lib when run on Xen? Did you ever test also a different platform, like 
KVM, to understand what you need there? 
    
    > Btw, CONFIG_PARAVIRT and CONFIG_HVMLITE are left-overs from our Mini-OS 
port. I think it makes sense to clean this up and replace the definitions.
    
    CONFIG_XEN_PV, CONFIG_XEN_PVH ?

Yes.
    
    Is unikraft targeting also full Xen HVM?

Not for now. I am also not sure if we there is a gain for Unikraft HVM on Xen, 
assuming we would have PVH.
    
    I think it's still tempting to allow runtime detection - for example
    building with both CONFIG_XEN_PV and CONFIG_XEN_PVH enabled would make
    it choose those few functions at runtime. This would mean probably
    duplicating most of mm.c, maybe even as mm_pv.c and mm_hvm.c (instead of
    using #ifdef inside functions). But on the other hand, that would
    improve readability...


Thanks a lot,

Simon
    
    -- 
    Best Regards,
    Marek Marczykowski-Górecki
    Invisible Things Lab
    A: Because it messes up the order in which people normally read text.
    Q: Why is top-posting such a bad thing?
    

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