[Xen-devel] Re: [PATCH] libxl: enabling upstream qemu as pure pv backend.

[Ian Campbell <Ian.Campbell@xxxxxxxxxxxxx>]

On Thu, 2011-06-09 at 08:07 +0100, Wei Liu wrote:
> The dm creating logic is as followed:
> 
> if hvm
>   libxl__create_device_model
>     if stubdom
>       libxl__create_stubdom -> libxl__create_xenpv_qemu
>                                 |
>                                 --> libxl__create_device_model
>     else
>       spawn and exec qemu
> else /* pv */
>   if need_qemu
>     libxl__create_xenpv_qemu
>      |
>      --> libxl__create_device_model
> 
> *
> I think adding device_model_args_{pv,fv} is a good idea.

Agreed although you will also need _old and _new variants, making four
functions. It's not clear how much in common they will have but please
consider making pv vs fv a parameter to the _old/_new functions i.e. try
and keep it down to just 2 functions (of course if they have nothing in
common 4 functions will be fine).

> *
> Since libxl__create_stubdom receives a dm_info structure, I think it is
> ok for libxl__create_xenpv_qemu to receive one, too. This dm_info is key
> structure to indicate xenpv qemu's type (traditional or upstream). But
> once we enter libxl__create_xenpv_qemu, we lost knowledge of whether we
> are creating a stubdom/qemu-xen or upstream qemu. So the caller should
> be responsible for filling in a new dm_info for
> libxl__create_xenpv_qemu.

I'm wondering if all these functions shouldn't take a
libxl_domain_config (which contains libxl_dm_info), after all there is
no fundamental reason that creating the DM should't be at liberty to
base it's behaviour on any aspect of the domains cfg.

> 
> *
> vfb is derive from d_config (libxl_domain_config), which is a domain
> property.

Aha, an example of what I meant above, convenient ;-)

>  Currently, the existing code either use
> libxl__vfb_and_vkb_from_device_model_info (if we are using stubdom) or
> direct parsing (if we are using xenpv_qemu). Though the code is
> redundent, the parsing is just the same essentially. What's the point of
> moving vnc and sdl out of vfb?

I'm not entirely sure. In a world with multiple VFBs (note: we don't
currently support this)you could, I suppose have one on SDL and one on
VNC. I suppose you might even want a single VFB exposed over both, that
doesn't seem unreasonable (maybe this works today?)

> *
> Configure two DMs for one domain? Haven't thought about that. I doubt
> that if it really necessary if we are moving towards a unified DM --
> upstream qemu -- wouldn't that be sufficient in the long run?

There will always be a need for at least two DMs, that is in the stubdom
case (one DM in a stubdom, the other in dom0), however they should both
be the same version of the DM, i.e. both upstream or both traditional.

In the future it's also possible that we would want to have the option
of multiple qemu's, e.g. one per qdisk backend, for isolation and
robustness.

> *
> To my understanding, stubdom is minios+qemu-xen. If I (the user) am
> using stubdom and specify device model args, these args should go to
> xenpv qemu, not xenfv in stubdom, right?

The device_model_args are basically a trap door to allow users to do
things which libxl didn't anticipate (i.e. as a stopgap until libxl can
be updated with that feature). As such extra args could be needed for
either DM. The distinction only really matters in the stubdom case.

>  What I see in the code is that
> we only need a few args (e.g. disks, vifs) to start stubdom. The
> internal setup to connect to domU is done within stubdom.
> 
> To summarize, I give a second prototype of my patch. Please review.
> 
> libxl__build_xenpv_qemu_args handles common options to both xenpv qemu
> and upstream qemu, while libxl__build_device_model_args distinguish
> between old and new qemu's and build args respectively.
> 
> libxl__create_xenpv_qemu is not allocating a struct
> libxl_device_model_info anymore, because at this point, it doesn't know
> if it is building a stubdom/qemu-xen (traditional type) or upstream
> qemu. The allocating and filling becomes caller's responsibility.
> 
> This patch has been tested with pv guest creating, fv guest creating and
> fv-stubdom guest creating.
> 
> -----------8<------------------
[...]
> @@ -506,8 +507,15 @@ static int do_domain_create(libxl__gc *gc, 
> libxl_domain_config *d_config,
>          libxl__device_console_add(gc, domid, &console, &state);
>          libxl_device_console_destroy(&console);
> 
> +        /* only copy those useful configs */
> +        memset((void*)&xenpv_dm_info, 0x00, sizeof(libxl_device_model_info));
> +        xenpv_dm_info.device_model_version =
> +            d_config->dm_info.device_model_version;
> +        xenpv_dm_info.type = d_config->dm_info.type;
> +        xenpv_dm_info.device_model = d_config->dm_info.device_model;
>          if (need_qemu)
> -            libxl__create_xenpv_qemu(gc, domid, d_config->vfbs, 
> &dm_starting);
> +            libxl__create_xenpv_qemu(gc, domid, &xenpv_dm_info,
> +                                     d_config->vfbs, &dm_starting);
>      }
> 
>      if (dm_starting) {

This is what I was thinking of when I said you might be able to just
pass the same dm_info to both -- since you only ever copy fields
verbatim, and libxl__create_xenpv_qemu (presumably) only looks at that
subset of fields why not just pass the whole lot through.

The rest looked ok, although I didn't review in detail yet.

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel