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
|
Home