On Thu, 2011-06-09 at 10:49 +0100, Wei Liu wrote:
> On Thu, 2011-06-09 at 09:52 +0100, Ian Campbell wrote:
> > 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).
> >
>
> Well, I'm referring to configuration variables for config file but not
> functions in libxl...
Oh right, it's all rather confusingly named isn't it ;-)
> These config variables become parameters of
> libxl__build_device_model_args_{old,new}. (see patch)
>
> However, pv/fv configuration variables are related to guest machine type
> rather than qemu type. It is possible to add two more variables
> device_model_args_{old,new} and get them related to qemu type, if
> necessary.
I don't think this will be necessary. If a user has selected new qemu
then they should expect to put device_model_args in the new syntax. if
they intend to switch back and forth then they can always keep the other
one commented out...
> However, so many configuration variables, five in total -- pv/fv/old/new
> plus the original one, may confuse users. Too bad.
Yeah, just the 3 is bad enough, but necessary I think.
> > > *
> > > 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.
> >
>
> I don't think so. DM creation has nothing to do with domain_config,
> that's what I see in xl_cmdimpl.c:parse_config_data.
Well, the configuration of the DM you are creating has everything to do
with the configuration of the domains it serves.
>
> > >
> > > *
> > > 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?)
> >
>
> Currently I have no idea how to do this.
Ignore it for now IMHO.
> > > *
> > > 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.
> >
>
> I understand. DM in stubdom is statically packed in ioemu-stubdom.gz.
> Upstream qemu is not supported by now (AFAIK).
It will be (hopefully) in the future though.
> In this case, we are not configuring two DMs, just one.
The DM in a stubdom still has another qmeu process in dom0. I guess
logically you can think of them as two parts of the same DM, which I
guess is what you are doing?
> 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.
> >
>
> Can you tell me how to pass parameters to the qemu running inside
> stubdom? What I see in the code is that libxl passes args to the xenpv
> qemu running in dom0 and leave qemu running inside stubdom untouched.
It looks as if libxl__write_dmargs derives the stubdom parameters from
the dom0 process parameters (by filtering out uninteresting options) and
writes them to xenstore.
Apart from, apparently, the "-domid <domid>" paramter which it puts on
the stubdom kernel command line.
>
> > > 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.
> >
>
> I'm afraid this kind of verbatim copying is necessary. Luckily, this
> kind of operation is hidden from the user.
>
> In the original code, libxl__create_xenpv_qemu allocates its own
> dm_info, which is zero-out with libxl__build_xenpv_qemu_args before
> using. Because libxl__create_xenpv_qemu eventually calls
> libxl__create_device_model. If there are rubbish contents in dm_info,
> the creation is likely to fail.
OK. I think we can keep the copying for now and maybe someone will
decide to untangle it a bit more in the future.
> > 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
|
Home