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

[Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>]

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.

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

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

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

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

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

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

Is unikraft targeting also full Xen HVM?

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

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

Attachment: signature.asc
Description: PGP signature

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