[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [Xen-devel] Re: [PATCH] libxl: enabling upstream qemu as pure pv backend.
On Thu, Jun 9, 2011 at 11:20 PM, Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx> wrote: > 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). > > Well, leave it out this patch. >> * >> 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. > OK. > Apart from Ian's comments, the rest is fine. > What should be improved? This thread is so long, can you be more specific? _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxxxxxxxx http://lists.xensource.com/xen-devel
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |