|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [XEN][RFC PATCH V2 15/17] xl: support spawn/destroy on multiple device model
On Wed, 2012-08-22 at 13:32 +0100, Julien Grall wrote:
> Old configuration file is still working with qemu disaggregation.
> Before to spawn any QEMU, the toolstack will fill correctly, if needed,
> configuration structure.
>
> For the moment, the toolstack spawns device models one by one.
>
> Signed-off-by: Julien Grall <julien.grall@xxxxxxxxxx>
> ---
> tools/libxl/libxl.c | 16 ++-
> tools/libxl/libxl_create.c | 150 +++++++++++++-----
> tools/libxl/libxl_device.c | 7 +-
> tools/libxl/libxl_dm.c | 369
> ++++++++++++++++++++++++++++++------------
> tools/libxl/libxl_dom.c | 4 +-
> tools/libxl/libxl_internal.h | 36 +++--
> 6 files changed, 421 insertions(+), 161 deletions(-)
>
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 8ea3478..60718b6 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -1330,7 +1330,8 @@ static void stubdom_destroy_callback(libxl__egc *egc,
> }
>
> dds->stubdom_finished = 1;
> - savefile = libxl__device_model_savefile(gc, dis->domid);
> + /* FIXME: get dmid */
> + savefile = libxl__device_model_savefile(gc, dis->domid, 0);
> rc = libxl__remove_file(gc, savefile);
> /*
> * On suspend libxl__domain_save_device_model will have already
> @@ -1423,10 +1424,8 @@ void libxl__destroy_domid(libxl__egc *egc,
> libxl__destroy_domid_state *dis)
> LIBXL__LOG_ERRNOVAL(ctx, LIBXL__LOG_ERROR, rc, "xc_domain_pause
> failed for %d", domid);
> }
> if (dm_present) {
> - if (libxl__destroy_device_model(gc, domid) < 0)
> - LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "libxl__destroy_device_model
> failed for %d", domid);
> -
> - libxl__qmp_cleanup(gc, domid);
> + if (libxl__destroy_device_models(gc, domid) < 0)
> + LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "libxl__destroy_device_models
> failed for %d", domid);
> }
> dis->drs.ao = ao;
> dis->drs.domid = domid;
> @@ -1725,6 +1724,13 @@ out:
>
>
> /******************************************************************************/
>
> +int libxl__dm_setdefault(libxl__gc *gc, libxl_dm *dm)
> +{
> + return 0;
> +}
> +
> +/******************************************************************************/
> +
> int libxl__device_disk_setdefault(libxl__gc *gc, libxl_device_disk *disk)
> {
> int rc;
> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index 5f0d26f..7160c78 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -35,6 +35,10 @@ void libxl_domain_config_dispose(libxl_domain_config
> *d_config)
> {
> int i;
>
> + for (i=0; i<d_config->num_dms; i++)
> + libxl_dm_dispose(&d_config->dms[i]);
> + free(d_config->dms);
We are adding libxl_FOO_list_free functions for new ones of these as we
introduce new ones), can you do that for the dm type please.
> +
> for (i=0; i<d_config->num_disks; i++)
> libxl_device_disk_dispose(&d_config->disks[i]);
> free(d_config->disks);
> @@ -59,6 +63,50 @@ void libxl_domain_config_dispose(libxl_domain_config
> *d_config)
> libxl_domain_build_info_dispose(&d_config->b_info);
> }
>
> +static int libxl__domain_config_setdefault(libxl__gc *gc,
> + libxl_domain_config *d_config)
> +{
> + libxl_domain_build_info *b_info = &d_config->b_info;
> + uint64_t cap = 0;
> + int i = 0;
> + int ret = 0;
> + libxl_dm *default_dm = NULL;
> +
> + if (b_info->device_model_version ==
> LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL
> + && (d_config->num_dms > 1))
> + return ERROR_INVAL;
> +
> + if (!d_config->num_dms) {
> + d_config->dms = malloc(sizeof (*d_config->dms));
You should use libxl__zalloc or libxl__calloc or something here with the
NO_GC argument to get the expected error handling.
> @@ -991,12 +1057,11 @@ static void domcreate_launch_dm(libxl__egc *egc,
> libxl__multidev *multidev,
> libxl__device_console_dispose(&console);
>
> if (need_qemu) {
> - dcs->dmss.dm.guest_domid = domid;
> - libxl__spawn_local_dm(egc, &dcs->dmss.dm);
> + assert(dcs->dmss);
> + domcreate_spawn_devmodel(egc, dcs, dcs->current_dmid);
> return;
> } else {
> - assert(!dcs->dmss.dm.guest_domid);
> - domcreate_devmodel_started(egc, &dcs->dmss.dm, 0);
> + assert(!dcs->dmss);
Doesn't this stop progress in this case meaning we'll never get to the
end of the async op?
> return;
> }
> }
[..]
> @@ -1044,7 +1044,8 @@ int libxl__wait_for_device_model(libxl__gc *gc,
> void *check_callback_userdata)
> {
> char *path;
> - path = libxl__sprintf(gc, "/local/domain/0/device-model/%d/state",
> domid);
> + path = libxl__sprintf(gc, "/local/domain/0/dms/%u/%u/state",
> + domid, dmid);
Isn't this control path shared with qemu? I'm not sure we can just
change it like that? We need to at least retain compatibility with
pre-disag qemus.
> return libxl__wait_for_offspring(gc, domid,
> LIBXL_DEVICE_MODEL_START_TIMEOUT,
> "Device Model", path, state, spawning,
> const char *libxl__domain_device_model(libxl__gc *gc,
> - const libxl_domain_build_info *info)
> + uint32_t dmid,
> + const libxl_domain_build_info *b_info)
> {
> libxl_ctx *ctx = libxl__gc_owner(gc);
> const char *dm;
> + libxl_domain_config *guest_config = CONTAINER_OF(b_info, *guest_config,
> + b_info);
>
> - if (libxl_defbool_val(info->device_model_stubdomain))
> + if (libxl_defbool_val(guest_config->b_info.device_model_stubdomain))
You just extracted guest_config from b_info but you still have the
b_info point to hand. Why not use it? Likewise a few more times below.
> {
> + /**
The ** implies some sort of automagic comments->doc parsing process
which we don't have here.
> + * PCI device number. Before 3, we have IDE, ISA, SouthBridge and
> + * XEN PCI. Theses devices will be emulate in each QEMU, but only
> + * one QEMU (the one which emulates default device) will register
> + * these devices through Xen PCI hypercall.
> + */
> + static unsigned int bdf = 3;
Do you mean const rather than static?
Isn't this baking in some implementation detail from the current qemu
version? What happens if it changes?
> +
> libxl_ctx *ctx = libxl__gc_owner(gc);
> const libxl_domain_create_info *c_info = &guest_config->c_info;
> const libxl_domain_build_info *b_info = &guest_config->b_info;
> + const libxl_dm *dm_config = &guest_config->dms[dmid];
> const libxl_device_disk *disks = guest_config->disks;
> const libxl_device_nic *nics = guest_config->nics;
> const int num_disks = guest_config->num_disks;
> const int num_nics = guest_config->num_nics;
> - const libxl_vnc_info *vnc = libxl__dm_vnc(guest_config);
> + const libxl_vnc_info *vnc = libxl__dm_vnc(dmid, guest_config);
> const libxl_sdl_info *sdl = dm_sdl(guest_config);
> const char *keymap = dm_keymap(guest_config);
> flexarray_t *dm_args;
> int i;
> uint64_t ram_size;
> + uint32_t cap_ui = dm_config->capabilities & LIBXL_DM_CAP_UI;
> + uint32_t cap_ide = dm_config->capabilities & LIBXL_DM_CAP_IDE;
> + uint32_t cap_serial = dm_config->capabilities & LIBXL_DM_CAP_SERIAL;
> + uint32_t cap_audio = dm_config->capabilities & LIBXL_DM_CAP_AUDIO;
->capabilities is defined as 64 bits, but you use 32 here, which happens
to work if you know what the actual values of the enum are but whoever
adds the 33rd capability will probably get it wrong.
bool cap_foo = !! (dm_....capabiltieis & LIBXL_DM_CAP_FOO)
would probably work?
>
> dm_args = flexarray_make(16, 1);
> if (!dm_args)
> @@ -348,11 +389,12 @@ static char **
> libxl__build_device_model_args_new(libxl__gc *gc,
> "-xen-domid",
> libxl__sprintf(gc, "%d", guest_domid), NULL);
>
> + flexarray_append(dm_args, "-nodefaults");
Does this not cause a change in behaviour other than what you've
accounted for here?
> @@ -528,65 +583,69 @@ static char **
> libxl__build_device_model_args_new(libxl__gc *gc,
> abort();
> }
>
> - ram_size = libxl__sizekb_to_mb(b_info->max_memkb - b_info->video_memkb);
> + // Allocate ram space of 32Mo per previous device model to store rom
What is this about?
(also that Mo looks a bit odd in among all these mb's)
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |