On Thu, 9 Jun 2011, 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.
>
> *
> 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.
Agreed.
> *
> vfb is derive from d_config (libxl_domain_config), which is a domain
> property. 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 don't feel strongly about it so you can leave it out this patch.
However if we removed the sdl and vnc settings from vfb and used the
same fields in device_model_info instead, we wouldn't need to "convert"
the vfb settings into device_model settings anymore
(libxl__build_xenpv_qemu_args would go away).
> *
> 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?
>
> *
> 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? 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<------------------
>
> commit e7ca429c34bd1f306f0819d447456dbe48e53e6e
> Author: Wei Liu <liuw@xxxxxxxxx>
> Date: Wed Jun 8 11:13:25 2011 +0800
>
> libxl: enabling upstream qemu as pure pv backend.
>
> This patch makes device_model_{version,override} work for pure pv
> guest, so that users can specify upstream qemu as pure pv backend
> other than traditional qemu-xen.
>
> This patch also add device_model_args_{pv,fv} options for pv and
> fv guest respectively.
>
> Internally, original libxl__create_xenpv_qemu allocates a new empty
> dm_info (struct libxl_device_model_info) for every xenpv qemu created.
> Now the caller of libxl__create_xenpv_qemu is responsible for
> allocating this dm_info.
>
> Signed-off-by: Wei Liu <liuw@xxxxxxxxx>
>
> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index 62294b2..92550bb 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -486,6 +486,7 @@ static int do_domain_create(libxl__gc *gc,
> libxl_domain_config *d_config,
> } else {
> int need_qemu = 0;
> libxl_device_console console;
> + libxl_device_model_info xenpv_dm_info;
>
> for (i = 0; i < d_config->num_vfbs; i++) {
> libxl_device_vfb_add(ctx, domid, &d_config->vfbs[i]);
> @@ -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) {
> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index 47a51c8..473e3e4 100644
> --- a/tools/libxl/libxl_dm.c
> +++ b/tools/libxl/libxl_dm.c
> @@ -207,9 +207,13 @@ static char **
> libxl__build_device_model_args_old(libxl__gc *gc,
> switch (info->type) {
> case LIBXL_DOMAIN_TYPE_PV:
> flexarray_append(dm_args, "xenpv");
> + for (i = 0; info->extra_pv && info->extra_pv[i] != NULL; i++)
> + flexarray_append(dm_args, info->extra_pv[i]);
> break;
> case LIBXL_DOMAIN_TYPE_FV:
> flexarray_append(dm_args, "xenfv");
> + for (i = 0; info->extra_fv && info->extra_fv[i] != NULL; i++)
> + flexarray_append(dm_args, info->extra_fv[i]);
> break;
> }
> flexarray_append(dm_args, NULL);
> @@ -364,9 +368,13 @@ static char **
> libxl__build_device_model_args_new(libxl__gc *gc,
> switch (info->type) {
> case LIBXL_DOMAIN_TYPE_PV:
> flexarray_append(dm_args, "xenpv");
> + for (i = 0; info->extra_pv && info->extra_pv[i] != NULL; i++)
> + flexarray_append(dm_args, info->extra_pv[i]);
> break;
> case LIBXL_DOMAIN_TYPE_FV:
> flexarray_append(dm_args, "xenfv");
> + for (i = 0; info->extra_fv && info->extra_fv[i] != NULL; i++)
> + flexarray_append(dm_args, info->extra_fv[i]);
> break;
> }
>
> @@ -575,6 +583,7 @@ static int libxl__create_stubdom(libxl__gc *gc,
> struct xs_permissions perm[2];
> xs_transaction_t t;
> libxl__device_model_starting *dm_starting = 0;
> + libxl_device_model_info xenpv_dm_info;
>
> if (info->device_model_version !=
> LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL) {
> ret = ERROR_INVAL;
> @@ -608,6 +617,7 @@ static int libxl__create_stubdom(libxl__gc *gc,
>
> /* fixme: this function can leak the stubdom if it fails */
>
> + domid = 0;
> ret = libxl__domain_make(gc, &c_info, &domid);
> if (ret)
> goto out_free;
> @@ -702,7 +712,15 @@ retry_transaction:
> if (ret)
> goto out_free;
> }
> - if (libxl__create_xenpv_qemu(gc, domid, vfb, &dm_starting) < 0) {
> +
> + memset((void*)&xenpv_dm_info, 0x00, sizeof(libxl_device_model_info));
> + xenpv_dm_info.device_model_version =
> + LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL;
> + xenpv_dm_info.type = LIBXL_DOMAIN_TYPE_PV;
> + xenpv_dm_info.device_model = NULL;
> + if (libxl__create_xenpv_qemu(gc, domid,
> + &xenpv_dm_info,
> + vfb, &dm_starting) < 0) {
> ret = ERROR_FAIL;
> goto out_free;
> }
You should set device_model_version, type and device_model from the same
fields in info, so that the device model version running in the stubdom
is the same as the one running in dom0.
We don't want to mismatch the two of them.
Apart from Ian's comments, the rest is fine.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home