From mboxrd@z Thu Jan 1 00:00:00 1970 From: Wei Liu Subject: Re: [PATCH] libxl: enabling upstream qemu as pure pv backend. Date: Thu, 9 Jun 2011 23:49:42 +0800 Message-ID: References: <1307503152.31359.2.camel@limbo> <1307536033.775.732.camel@zakaz.uk.xensource.com> <1307538807.775.737.camel@zakaz.uk.xensource.com> <1307541074.775.739.camel@zakaz.uk.xensource.com> <1307548380.775.744.camel@zakaz.uk.xensource.com> <1307603224.31235.8.camel@limbo> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Stefano Stabellini Cc: Ian Campbell , "xen-devel@lists.xensource.com" List-Id: xen-devel@lists.xenproject.org On Thu, Jun 9, 2011 at 11:20 PM, Stefano Stabellini wrote: > On Thu, 9 Jun 2011, Wei Liu wrote: >> The dm creating logic is as followed: >> >> if hvm >> =C2=A0 libxl__create_device_model >> =C2=A0 =C2=A0 if stubdom >> =C2=A0 =C2=A0 =C2=A0 libxl__create_stubdom -> libxl__create_xenpv_qemu >> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 | >> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 --> libxl__create_device_model >> =C2=A0 =C2=A0 else >> =C2=A0 =C2=A0 =C2=A0 spawn and exec qemu >> else /* pv */ >> =C2=A0 if need_qemu >> =C2=A0 =C2=A0 libxl__create_xenpv_qemu >> =C2=A0 =C2=A0 =C2=A0| >> =C2=A0 =C2=A0 =C2=A0--> 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 >> Date: =C2=A0 Wed Jun 8 11:13:25 2011 +0800 >> >> =C2=A0 =C2=A0 libxl: enabling upstream qemu as pure pv backend. >> >> =C2=A0 =C2=A0 This patch makes device_model_{version,override} work for = pure pv >> =C2=A0 =C2=A0 guest, so that users can specify upstream qemu as pure pv = backend >> =C2=A0 =C2=A0 other than traditional qemu-xen. >> >> =C2=A0 =C2=A0 This patch also add device_model_args_{pv,fv} options for = pv and >> =C2=A0 =C2=A0 fv guest respectively. >> >> =C2=A0 =C2=A0 Internally, original libxl__create_xenpv_qemu allocates a = new empty >> =C2=A0 =C2=A0 dm_info (struct libxl_device_model_info) for every xenpv q= emu created. >> =C2=A0 =C2=A0 Now the caller of libxl__create_xenpv_qemu is responsible = for >> =C2=A0 =C2=A0 allocating this dm_info. >> >> =C2=A0 =C2=A0 Signed-off-by: Wei Liu >> >> 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_dom= ain_config *d_config, >> =C2=A0 =C2=A0 =C2=A0} else { >> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0int need_qemu =3D 0; >> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0libxl_device_console console; >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0libxl_device_model_info xenpv_dm_info; >> >> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0for (i =3D 0; i < d_config->num_vfbs; = i++) { >> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0libxl_device_vfb_add(ctx= , domid, &d_config->vfbs[i]); >> @@ -506,8 +507,15 @@ static int do_domain_create(libxl__gc *gc, libxl_do= main_config *d_config, >> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0libxl__device_console_add(gc, domid, &= console, &state); >> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0libxl_device_console_destroy(&console)= ; >> >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0/* only copy those useful configs */ >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0memset((void*)&xenpv_dm_info, 0x00, sizeof(= libxl_device_model_info)); >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0xenpv_dm_info.device_model_version =3D >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0d_config->dm_info.device_mode= l_version; >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0xenpv_dm_info.type =3D d_config->dm_info.ty= pe; >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0xenpv_dm_info.device_model =3D d_config->dm= _info.device_model; >> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (need_qemu) >> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0libxl__create_xenpv_qemu(gc, = domid, d_config->vfbs, &dm_starting); >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0libxl__create_xenpv_qemu(gc, = domid, &xenpv_dm_info, >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 d_config->vfbs, &dm= _starting); >> =C2=A0 =C2=A0 =C2=A0} >> >> =C2=A0 =C2=A0 =C2=A0if (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(l= ibxl__gc *gc, >> =C2=A0 =C2=A0 =C2=A0switch (info->type) { >> =C2=A0 =C2=A0 =C2=A0case LIBXL_DOMAIN_TYPE_PV: >> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0flexarray_append(dm_args, "xenpv"); >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0for (i =3D 0; info->extra_pv && info->extra= _pv[i] !=3D NULL; i++) >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0flexarray_append(dm_args, inf= o->extra_pv[i]); >> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0break; >> =C2=A0 =C2=A0 =C2=A0case LIBXL_DOMAIN_TYPE_FV: >> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0flexarray_append(dm_args, "xenfv"); >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0for (i =3D 0; info->extra_fv && info->extra= _fv[i] !=3D NULL; i++) >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0flexarray_append(dm_args, inf= o->extra_fv[i]); >> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0break; >> =C2=A0 =C2=A0 =C2=A0} >> =C2=A0 =C2=A0 =C2=A0flexarray_append(dm_args, NULL); >> @@ -364,9 +368,13 @@ static char ** libxl__build_device_model_args_new(l= ibxl__gc *gc, >> =C2=A0 =C2=A0 =C2=A0switch (info->type) { >> =C2=A0 =C2=A0 =C2=A0case LIBXL_DOMAIN_TYPE_PV: >> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0flexarray_append(dm_args, "xenpv"); >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0for (i =3D 0; info->extra_pv && info->extra= _pv[i] !=3D NULL; i++) >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0flexarray_append(dm_args, inf= o->extra_pv[i]); >> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0break; >> =C2=A0 =C2=A0 =C2=A0case LIBXL_DOMAIN_TYPE_FV: >> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0flexarray_append(dm_args, "xenfv"); >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0for (i =3D 0; info->extra_fv && info->extra= _fv[i] !=3D NULL; i++) >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0flexarray_append(dm_args, inf= o->extra_fv[i]); >> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0break; >> =C2=A0 =C2=A0 =C2=A0} >> >> @@ -575,6 +583,7 @@ static int libxl__create_stubdom(libxl__gc *gc, >> =C2=A0 =C2=A0 =C2=A0struct xs_permissions perm[2]; >> =C2=A0 =C2=A0 =C2=A0xs_transaction_t t; >> =C2=A0 =C2=A0 =C2=A0libxl__device_model_starting *dm_starting =3D 0; >> + =C2=A0 =C2=A0libxl_device_model_info xenpv_dm_info; >> >> =C2=A0 =C2=A0 =C2=A0if (info->device_model_version !=3D LIBXL_DEVICE_MOD= EL_VERSION_QEMU_XEN_TRADITIONAL) { >> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0ret =3D ERROR_INVAL; >> @@ -608,6 +617,7 @@ static int libxl__create_stubdom(libxl__gc *gc, >> >> =C2=A0 =C2=A0 =C2=A0/* fixme: this function can leak the stubdom if it f= ails */ >> >> + =C2=A0 =C2=A0domid =3D 0; >> =C2=A0 =C2=A0 =C2=A0ret =3D libxl__domain_make(gc, &c_info, &domid); >> =C2=A0 =C2=A0 =C2=A0if (ret) >> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0goto out_free; >> @@ -702,7 +712,15 @@ retry_transaction: >> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (ret) >> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0goto out_free; >> =C2=A0 =C2=A0 =C2=A0} >> - =C2=A0 =C2=A0if (libxl__create_xenpv_qemu(gc, domid, vfb, &dm_starting= ) < 0) { >> + >> + =C2=A0 =C2=A0memset((void*)&xenpv_dm_info, 0x00, sizeof(libxl_device_m= odel_info)); >> + =C2=A0 =C2=A0xenpv_dm_info.device_model_version =3D >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITI= ONAL; >> + =C2=A0 =C2=A0xenpv_dm_info.type =3D LIBXL_DOMAIN_TYPE_PV; >> + =C2=A0 =C2=A0xenpv_dm_info.device_model =3D NULL; >> + =C2=A0 =C2=A0if (libxl__create_xenpv_qemu(gc, domid, >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 &xenpv_dm_info, >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 vfb, &dm_starting) < 0) { >> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0ret =3D ERROR_FAIL; >> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0goto out_free; >> =C2=A0 =C2=A0 =C2=A0} > > 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?