[Xen-devel] Re: [PATCH] libxl: enabling upstream qemu as pure pv backend.

[Wei Liu <liuw@xxxxxxxxx>]

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