Wei Liu
2011-Jun-08 03:19 UTC
[Xen-devel] [PATCH] libxl: enabling upstream qemu as pure pv backend.
commit 02cf9f9cfdf720c636c6ba08f795e49b5eb1f03e Author: Wei Liu <liuw@liuw.name> 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. Signed-off-by: Wei Liu <liuw@liuw.name> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c index 62294b2..4ff3c7d 100644 --- a/tools/libxl/libxl_create.c +++ b/tools/libxl/libxl_create.c @@ -507,7 +507,8 @@ static int do_domain_create(libxl__gc *gc, libxl_domain_config *d_config, libxl_device_console_destroy(&console); if (need_qemu) - libxl__create_xenpv_qemu(gc, domid, d_config->vfbs, &dm_starting); + libxl__create_xenpv_qemu(gc, domid, &d_config->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..0505c84 100644 --- a/tools/libxl/libxl_dm.c +++ b/tools/libxl/libxl_dm.c @@ -702,7 +702,7 @@ retry_transaction: if (ret) goto out_free; } - if (libxl__create_xenpv_qemu(gc, domid, vfb, &dm_starting) < 0) { + if (libxl__create_xenpv_qemu(gc, domid, info, vfb, &dm_starting) < 0) { ret = ERROR_FAIL; goto out_free; } @@ -909,8 +909,8 @@ static int libxl__build_xenpv_qemu_args(libxl__gc *gc, libxl_device_model_info *info) { libxl_ctx *ctx = libxl__gc_owner(gc); - memset(info, 0x00, sizeof(libxl_device_model_info)); + info->vnc = 0; if (vfb != NULL) { info->vnc = vfb->vnc; if (vfb->vnclisten) @@ -927,9 +927,12 @@ static int libxl__build_xenpv_qemu_args(libxl__gc *gc, info->nographic = 1; info->domid = domid; info->dom_name = libxl_domid_to_name(ctx, domid); - info->device_model_version = LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL; - info->device_model = NULL; - info->type = LIBXL_DOMAIN_TYPE_PV; + info->target_ram = 0; + info->videoram = 0; + info->acpi = 0; + info->vcpus = 0; + info->vcpu_avail = 0; + info->xen_platform_pci = 0; return 0; } @@ -985,12 +988,12 @@ out: return ret; } -int libxl__create_xenpv_qemu(libxl__gc *gc, uint32_t domid, libxl_device_vfb *vfb, +int libxl__create_xenpv_qemu(libxl__gc *gc, uint32_t domid, + libxl_device_model_info *dm_info, + libxl_device_vfb *vfb, libxl__device_model_starting **starting_r) { - libxl_device_model_info info; - - libxl__build_xenpv_qemu_args(gc, domid, vfb, &info); - libxl__create_device_model(gc, &info, NULL, 0, NULL, 0, starting_r); + libxl__build_xenpv_qemu_args(gc, domid, vfb, dm_info); + libxl__create_device_model(gc, dm_info, NULL, 0, NULL, 0, starting_r); return 0; } diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h index 64e1d56..55e9f7f 100644 --- a/tools/libxl/libxl_internal.h +++ b/tools/libxl/libxl_internal.h @@ -256,7 +256,9 @@ _hidden int libxl__create_device_model(libxl__gc *gc, libxl_device_disk *disk, int num_disks, libxl_device_nic *vifs, int num_vifs, libxl__device_model_starting **starting_r); -_hidden int libxl__create_xenpv_qemu(libxl__gc *gc, uint32_t domid, libxl_device_vfb *vfb, +_hidden int libxl__create_xenpv_qemu(libxl__gc *gc, uint32_t domid, + libxl_device_model_info *dm_info, + libxl_device_vfb *vfb, libxl__device_model_starting **starting_r); _hidden int libxl__need_xenpv_qemu(libxl__gc *gc, int nr_consoles, libxl_device_console *consoles, diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c index 5415bc5..74a77a7 100644 --- a/tools/libxl/xl_cmdimpl.c +++ b/tools/libxl/xl_cmdimpl.c @@ -626,6 +626,8 @@ static void parse_config_data(const char *configfile_filename_report, int pci_power_mgmt = 0; int pci_msitranslate = 1; int e; + XLU_ConfigList *dmargs; + int nr_dmargs = 0; libxl_domain_create_info *c_info = &d_config->c_info; libxl_domain_build_info *b_info = &d_config->b_info; @@ -1075,14 +1077,10 @@ skip_vfb: break; } - if (c_info->hvm == 1) { - XLU_ConfigList *dmargs; - int nr_dmargs = 0; - - /* init dm from c and b */ - libxl_init_dm_info(dm_info, c_info, b_info); + /* init dm from c and b */ + libxl_init_dm_info(dm_info, c_info, b_info); - /* then process config related to dm */ + if (c_info->hvm == 1) { if (!xlu_cfg_get_string (config, "device_model", &buf)) { fprintf(stderr, "WARNING: ignoring device_model directive.\n" @@ -1147,6 +1145,42 @@ skip_vfb: dm_info->extra[i] = a ? strdup(a) : NULL; } } + } else { + if (!xlu_cfg_get_string (config, "device_model", &buf)) { + fprintf(stderr, + "WARNING: ignoring device_model directive.\n" + "WARNING: Use \"device_model_override\" instead if you really want a non-default device_model\n"); + } + + xlu_cfg_replace_string (config, "device_model_override", + &dm_info->device_model); + if (!xlu_cfg_get_string (config, "device_model_version", &buf)) { + if (!strcmp(buf, "qemu-xen-traditional")) { + dm_info->device_model_version + = LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL; + } else if (!strcmp(buf, "qemu-xen")) { + dm_info->device_model_version + = LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN; + } else { + fprintf(stderr, + "Unknown device_model_version \"%s\" specified\n", buf); + exit(1); + } + } else if (dm_info->device_model) + fprintf(stderr, "WARNING: device model override given without specific DM version\n"); + if (!xlu_cfg_get_long (config, "device_model_stubdomain_override", &l)) + fprintf(stderr, "WARNING: ignoring device_model_stubdomain_override for PV guest\n"); + + if (!xlu_cfg_get_list(config, "device_model_args", &dmargs, &nr_dmargs, 0)) + { + int i; + dm_info->extra = xmalloc(sizeof(char *) * (nr_dmargs + 1)); + dm_info->extra[nr_dmargs] = NULL; + for (i=0; i<nr_dmargs; i++) { + const char *a = xlu_cfg_get_listitem(dmargs, i); + dm_info->extra[i] = a ? strdup(a) : NULL; + } + } } dm_info->type = c_info->hvm ? _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2011-Jun-08 08:55 UTC
Re: [Xen-devel] [PATCH] libxl: enabling upstream qemu as pure pv backend.
On Wed, 2011-06-08 at 04:19 +0100, Wei Liu wrote:> commit 02cf9f9cfdf720c636c6ba08f795e49b5eb1f03e > Author: Wei Liu <liuw@liuw.name> > 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. > > Signed-off-by: Wei Liu <liuw@liuw.name> > > @@ -909,8 +909,8 @@ static int libxl__build_xenpv_qemu_args(libxl__gc *gc, > libxl_device_model_info *info) > { > libxl_ctx *ctx = libxl__gc_owner(gc); > - memset(info, 0x00, sizeof(libxl_device_model_info));Why do you remove this memset?> + info->vnc = 0; > if (vfb != NULL) { > info->vnc = vfb->vnc; > if (vfb->vnclisten) > @@ -927,9 +927,12 @@ static int libxl__build_xenpv_qemu_args(libxl__gc *gc, > info->nographic = 1; > info->domid = domid; > info->dom_name = libxl_domid_to_name(ctx, domid); > - info->device_model_version = LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL; > - info->device_model = NULL; > - info->type = LIBXL_DOMAIN_TYPE_PV; > + info->target_ram = 0; > + info->videoram = 0; > + info->acpi = 0; > + info->vcpus = 0; > + info->vcpu_avail = 0; > + info->xen_platform_pci = 0; > return 0; > } > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c > index 5415bc5..74a77a7 100644 > --- a/tools/libxl/xl_cmdimpl.c > +++ b/tools/libxl/xl_cmdimpl.c > @@ -626,6 +626,8 @@ static void parse_config_data(const char *configfile_filename_report, > int pci_power_mgmt = 0; > int pci_msitranslate = 1; > int e; > + XLU_ConfigList *dmargs; > + int nr_dmargs = 0; > > libxl_domain_create_info *c_info = &d_config->c_info; > libxl_domain_build_info *b_info = &d_config->b_info; > @@ -1075,14 +1077,10 @@ skip_vfb: > break; > } > > - if (c_info->hvm == 1) { > - XLU_ConfigList *dmargs; > - int nr_dmargs = 0; > - > - /* init dm from c and b */ > - libxl_init_dm_info(dm_info, c_info, b_info); > + /* init dm from c and b */ > + libxl_init_dm_info(dm_info, c_info, b_info); > > - /* then process config related to dm */ > + if (c_info->hvm == 1) { > if (!xlu_cfg_get_string (config, "device_model", &buf)) { > fprintf(stderr, > "WARNING: ignoring device_model directive.\n" > @@ -1147,6 +1145,42 @@ skip_vfb: > dm_info->extra[i] = a ? strdup(a) : NULL; > } > } > + } else { > + if (!xlu_cfg_get_string (config, "device_model", &buf)) { > + fprintf(stderr, > + "WARNING: ignoring device_model directive.\n" > + "WARNING: Use \"device_model_override\" instead if you really want a non-default device_model\n"); > + }[...]> + if (!xlu_cfg_get_list(config, "device_model_args", &dmargs, &nr_dmargs, 0)) > + { > + int i; > + dm_info->extra = xmalloc(sizeof(char *) * (nr_dmargs + 1)); > + dm_info->extra[nr_dmargs] = NULL; > + for (i=0; i<nr_dmargs; i++) { > + const char *a = xlu_cfg_get_listitem(dmargs, i); > + dm_info->extra[i] = a ? strdup(a) : NULL; > + } > + }There is no need to duplicate all this code, is there? Just pull the relevant stuff from above out of the c_info->hvm == 1 case. One general concern I have is there are cases where we want 2 DM instances. In particular we can have an FV instance running in a stub domain which uses a PV instance running in dom0 for certain functionality (e.g. emulated VGA in the FV stub domain qemu goes to an xenfb frontend talking to a xenfb backend running in a PV qemu in domain 0). I''m not sure what the best solution here is, we could obviously duplicate up all the options but that seems unpleasant. I guess for the most part we should treat both qemu''s in this case as the same logical entity split into two brains, so most of the options are common to both (and e.g. the versions are matched etc) with libxl taking care of directing the individual options to the right instance (or both). Yeah, that sounds like the answer. Only exception is the device_model_args option where I can see that passing different extra args to each instance would be useful and it is unlikely that libxl will understand them enough to choose where to send them, in fact we probably want 3 varialbe, device_model_args and device_model_args_{pv,fv}. Ian. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Wei Liu
2011-Jun-08 09:53 UTC
Re: [Xen-devel] [PATCH] libxl: enabling upstream qemu as pure pv backend.
On Wed, 2011-06-08 at 09:55 +0100, Ian Campbell wrote:> On Wed, 2011-06-08 at 04:19 +0100, Wei Liu wrote: > > commit 02cf9f9cfdf720c636c6ba08f795e49b5eb1f03e > > Author: Wei Liu <liuw@liuw.name> > > 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. > > > > Signed-off-by: Wei Liu <liuw@liuw.name> > > > > @@ -909,8 +909,8 @@ static int libxl__build_xenpv_qemu_args(libxl__gc *gc, > > libxl_device_model_info *info) > > { > > libxl_ctx *ctx = libxl__gc_owner(gc); > > - memset(info, 0x00, sizeof(libxl_device_model_info)); > > Why do you remove this memset? >Because we are reusing the dm_info passed by ancestor callers, which has already been filled with useful contents.> > + info->vnc = 0; > > if (vfb != NULL) { > > info->vnc = vfb->vnc; > > if (vfb->vnclisten) > > @@ -927,9 +927,12 @@ static int libxl__build_xenpv_qemu_args(libxl__gc *gc, > > info->nographic = 1; > > info->domid = domid; > > info->dom_name = libxl_domid_to_name(ctx, domid); > > - info->device_model_version = LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL; > > - info->device_model = NULL; > > - info->type = LIBXL_DOMAIN_TYPE_PV; > > + info->target_ram = 0; > > + info->videoram = 0; > > + info->acpi = 0; > > + info->vcpus = 0; > > + info->vcpu_avail = 0; > > + info->xen_platform_pci = 0; > > return 0; > > } > > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c > > index 5415bc5..74a77a7 100644 > > --- a/tools/libxl/xl_cmdimpl.c > > +++ b/tools/libxl/xl_cmdimpl.c > > @@ -626,6 +626,8 @@ static void parse_config_data(const char *configfile_filename_report, > > int pci_power_mgmt = 0; > > int pci_msitranslate = 1; > > int e; > > + XLU_ConfigList *dmargs; > > + int nr_dmargs = 0; > > > > libxl_domain_create_info *c_info = &d_config->c_info; > > libxl_domain_build_info *b_info = &d_config->b_info; > > @@ -1075,14 +1077,10 @@ skip_vfb: > > break; > > } > > > > - if (c_info->hvm == 1) { > > - XLU_ConfigList *dmargs; > > - int nr_dmargs = 0; > > - > > - /* init dm from c and b */ > > - libxl_init_dm_info(dm_info, c_info, b_info); > > + /* init dm from c and b */ > > + libxl_init_dm_info(dm_info, c_info, b_info); > > > > - /* then process config related to dm */ > > + if (c_info->hvm == 1) { > > if (!xlu_cfg_get_string (config, "device_model", &buf)) { > > fprintf(stderr, > > "WARNING: ignoring device_model directive.\n" > > @@ -1147,6 +1145,42 @@ skip_vfb: > > dm_info->extra[i] = a ? strdup(a) : NULL; > > } > > } > > + } else { > > + if (!xlu_cfg_get_string (config, "device_model", &buf)) { > > + fprintf(stderr, > > + "WARNING: ignoring device_model directive.\n" > > + "WARNING: Use \"device_model_override\" instead if you really want a non-default device_model\n"); > > + } > [...] > > + if (!xlu_cfg_get_list(config, "device_model_args", &dmargs, &nr_dmargs, 0)) > > + { > > + int i; > > + dm_info->extra = xmalloc(sizeof(char *) * (nr_dmargs + 1)); > > + dm_info->extra[nr_dmargs] = NULL; > > + for (i=0; i<nr_dmargs; i++) { > > + const char *a = xlu_cfg_get_listitem(dmargs, i); > > + dm_info->extra[i] = a ? strdup(a) : NULL; > > + } > > + } > > There is no need to duplicate all this code, is there? > > Just pull the relevant stuff from above out of the c_info->hvm == 1 > case. >Ah, yes. The dmargs parsing can be pulled out. But the WARNING statements parts are different, so I duplicate some code. I will make it cleaner and resend.> One general concern I have is there are cases where we want 2 DM > instances. In particular we can have an FV instance running in a stub > domain which uses a PV instance running in dom0 for certain > functionality (e.g. emulated VGA in the FV stub domain qemu goes to an > xenfb frontend talking to a xenfb backend running in a PV qemu in domain > 0). >Haven''t come across such use case yet. Does libxl supports specifying two DMs for one domain? What''s the syntax?> I''m not sure what the best solution here is, we could obviously > duplicate up all the options but that seems unpleasant. >Agreed.> I guess for the most part we should treat both qemu''s in this case as > the same logical entity split into two brains, so most of the options > are common to both (and e.g. the versions are matched etc) with libxl > taking care of directing the individual options to the right instance > (or both). Yeah, that sounds like the answer. > > Only exception is the device_model_args option where I can see that > passing different extra args to each instance would be useful and it is > unlikely that libxl will understand them enough to choose where to send > them, in fact we probably want 3 varialbe, device_model_args and > device_model_args_{pv,fv}. >Well, we already have device_model_args_{old,new} which handle qemu-xen and upstream qemu respectively. And the main goal of my patch is to enable using upstream qemu as pv backend, so device_model_args_{pv,fv} may not be sufficient -- we have two qemus for pv. The handling logic will be complicated.> Ian. > >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Wei Liu
2011-Jun-08 10:20 UTC
Re: [Xen-devel] [PATCH] libxl: enabling upstream qemu as pure pv backend.
On Wed, 2011-06-08 at 09:55 +0100, Ian Campbell wrote:> One general concern I have is there are cases where we want 2 DM > instances. In particular we can have an FV instance running in a stub > domain which uses a PV instance running in dom0 for certain > functionality (e.g. emulated VGA in the FV stub domain qemu goes to an > xenfb frontend talking to a xenfb backend running in a PV qemu in domain > 0). >Ah, I misunderstood you at first. This use case is a bit complicated. I still wonder if libxl supports this kind of configuration... TBH, I''m not familiar with stubdom. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Stefano Stabellini
2011-Jun-08 11:09 UTC
Re: [Xen-devel] [PATCH] libxl: enabling upstream qemu as pure pv backend.
On Wed, 8 Jun 2011, Ian Campbell wrote:> On Wed, 2011-06-08 at 04:19 +0100, Wei Liu wrote: > > commit 02cf9f9cfdf720c636c6ba08f795e49b5eb1f03e > > Author: Wei Liu <liuw@liuw.name> > > 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. > > > > Signed-off-by: Wei Liu <liuw@liuw.name> > > > > @@ -909,8 +909,8 @@ static int libxl__build_xenpv_qemu_args(libxl__gc *gc, > > libxl_device_model_info *info) > > { > > libxl_ctx *ctx = libxl__gc_owner(gc); > > - memset(info, 0x00, sizeof(libxl_device_model_info)); > > Why do you remove this memset? > > > + info->vnc = 0; > > if (vfb != NULL) { > > info->vnc = vfb->vnc; > > if (vfb->vnclisten) > > @@ -927,9 +927,12 @@ static int libxl__build_xenpv_qemu_args(libxl__gc *gc, > > info->nographic = 1; > > info->domid = domid; > > info->dom_name = libxl_domid_to_name(ctx, domid); > > - info->device_model_version = LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL; > > - info->device_model = NULL; > > - info->type = LIBXL_DOMAIN_TYPE_PV; > > + info->target_ram = 0; > > + info->videoram = 0; > > + info->acpi = 0; > > + info->vcpus = 0; > > + info->vcpu_avail = 0; > > + info->xen_platform_pci = 0; > > return 0; > > } > > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c > > index 5415bc5..74a77a7 100644 > > --- a/tools/libxl/xl_cmdimpl.c > > +++ b/tools/libxl/xl_cmdimpl.c > > @@ -626,6 +626,8 @@ static void parse_config_data(const char *configfile_filename_report, > > int pci_power_mgmt = 0; > > int pci_msitranslate = 1; > > int e; > > + XLU_ConfigList *dmargs; > > + int nr_dmargs = 0; > > > > libxl_domain_create_info *c_info = &d_config->c_info; > > libxl_domain_build_info *b_info = &d_config->b_info; > > @@ -1075,14 +1077,10 @@ skip_vfb: > > break; > > } > > > > - if (c_info->hvm == 1) { > > - XLU_ConfigList *dmargs; > > - int nr_dmargs = 0; > > - > > - /* init dm from c and b */ > > - libxl_init_dm_info(dm_info, c_info, b_info); > > + /* init dm from c and b */ > > + libxl_init_dm_info(dm_info, c_info, b_info); > > > > - /* then process config related to dm */ > > + if (c_info->hvm == 1) { > > if (!xlu_cfg_get_string (config, "device_model", &buf)) { > > fprintf(stderr, > > "WARNING: ignoring device_model directive.\n" > > @@ -1147,6 +1145,42 @@ skip_vfb: > > dm_info->extra[i] = a ? strdup(a) : NULL; > > } > > } > > + } else { > > + if (!xlu_cfg_get_string (config, "device_model", &buf)) { > > + fprintf(stderr, > > + "WARNING: ignoring device_model directive.\n" > > + "WARNING: Use \"device_model_override\" instead if you really want a non-default device_model\n"); > > + } > [...] > > + if (!xlu_cfg_get_list(config, "device_model_args", &dmargs, &nr_dmargs, 0)) > > + { > > + int i; > > + dm_info->extra = xmalloc(sizeof(char *) * (nr_dmargs + 1)); > > + dm_info->extra[nr_dmargs] = NULL; > > + for (i=0; i<nr_dmargs; i++) { > > + const char *a = xlu_cfg_get_listitem(dmargs, i); > > + dm_info->extra[i] = a ? strdup(a) : NULL; > > + } > > + } > > There is no need to duplicate all this code, is there? > > Just pull the relevant stuff from above out of the c_info->hvm == 1 > case. > > One general concern I have is there are cases where we want 2 DM > instances. In particular we can have an FV instance running in a stub > domain which uses a PV instance running in dom0 for certain > functionality (e.g. emulated VGA in the FV stub domain qemu goes to an > xenfb frontend talking to a xenfb backend running in a PV qemu in domain > 0). > > I''m not sure what the best solution here is, we could obviously > duplicate up all the options but that seems unpleasant. > > I guess for the most part we should treat both qemu''s in this case as > the same logical entity split into two brains, so most of the options > are common to both (and e.g. the versions are matched etc) with libxl > taking care of directing the individual options to the right instance > (or both). Yeah, that sounds like the answer.Yes, if the user chooses new qemu for simplicity''s sake libxl should use new qemu for everything.> Only exception is the device_model_args option where I can see that > passing different extra args to each instance would be useful and it is > unlikely that libxl will understand them enough to choose where to send > them, in fact we probably want 3 varialbe, device_model_args and > device_model_args_{pv,fv}.Yes, that would be the best solution. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Stefano Stabellini
2011-Jun-08 11:33 UTC
[Xen-devel] Re: [PATCH] libxl: enabling upstream qemu as pure pv backend.
On Wed, 8 Jun 2011, Wei Liu wrote:> commit 02cf9f9cfdf720c636c6ba08f795e49b5eb1f03e > Author: Wei Liu <liuw@liuw.name> > 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. > > Signed-off-by: Wei Liu <liuw@liuw.name> > > diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c > index 62294b2..4ff3c7d 100644 > --- a/tools/libxl/libxl_create.c > +++ b/tools/libxl/libxl_create.c > @@ -507,7 +507,8 @@ static int do_domain_create(libxl__gc *gc, libxl_domain_config *d_config, > libxl_device_console_destroy(&console); > > if (need_qemu) > - libxl__create_xenpv_qemu(gc, domid, d_config->vfbs, &dm_starting); > + libxl__create_xenpv_qemu(gc, domid, &d_config->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..0505c84 100644 > --- a/tools/libxl/libxl_dm.c > +++ b/tools/libxl/libxl_dm.c > @@ -702,7 +702,7 @@ retry_transaction: > if (ret) > goto out_free; > } > - if (libxl__create_xenpv_qemu(gc, domid, vfb, &dm_starting) < 0) { > + if (libxl__create_xenpv_qemu(gc, domid, info, vfb, &dm_starting) < 0) { > ret = ERROR_FAIL; > goto out_free; > } > @@ -909,8 +909,8 @@ static int libxl__build_xenpv_qemu_args(libxl__gc *gc, > libxl_device_model_info *info) > { > libxl_ctx *ctx = libxl__gc_owner(gc); > - memset(info, 0x00, sizeof(libxl_device_model_info)); > > + info->vnc = 0; > if (vfb != NULL) { > info->vnc = vfb->vnc; > if (vfb->vnclisten) > @@ -927,9 +927,12 @@ static int libxl__build_xenpv_qemu_args(libxl__gc *gc, > info->nographic = 1; > info->domid = domid; > info->dom_name = libxl_domid_to_name(ctx, domid); > - info->device_model_version = LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL; > - info->device_model = NULL; > - info->type = LIBXL_DOMAIN_TYPE_PV; > + info->target_ram = 0; > + info->videoram = 0; > + info->acpi = 0; > + info->vcpus = 0; > + info->vcpu_avail = 0; > + info->xen_platform_pci = 0; > return 0; > }I don''t think is a good idea to reset all these value to 0 here, considering that the info parameter can be passed by the user. It is better to use another libxl_device_model_info local variable and just copy the very few fields we care about. Otherwise in the stubdom case above you''ll have the unwanted side effect of removing useful informations of the stubdom from the structure. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2011-Jun-08 12:23 UTC
Re: [Xen-devel] [PATCH] libxl: enabling upstream qemu as pure pv backend.
On Wed, 2011-06-08 at 10:53 +0100, Wei Liu wrote:> On Wed, 2011-06-08 at 09:55 +0100, Ian Campbell wrote: > > On Wed, 2011-06-08 at 04:19 +0100, Wei Liu wrote: > > > commit 02cf9f9cfdf720c636c6ba08f795e49b5eb1f03e > > > Author: Wei Liu <liuw@liuw.name> > > > 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. > > > > > > Signed-off-by: Wei Liu <liuw@liuw.name> > > > > > > @@ -909,8 +909,8 @@ static int libxl__build_xenpv_qemu_args(libxl__gc *gc, > > > libxl_device_model_info *info) > > > { > > > libxl_ctx *ctx = libxl__gc_owner(gc); > > > - memset(info, 0x00, sizeof(libxl_device_model_info)); > > > > Why do you remove this memset? > > > > Because we are reusing the dm_info passed by ancestor callers, which has > already been filled with useful contents.Oh right, thanks.> > > > + info->vnc = 0; > > > if (vfb != NULL) { > > > info->vnc = vfb->vnc; > > > if (vfb->vnclisten) > > > @@ -927,9 +927,12 @@ static int libxl__build_xenpv_qemu_args(libxl__gc *gc, > > > info->nographic = 1; > > > info->domid = domid; > > > info->dom_name = libxl_domid_to_name(ctx, domid); > > > - info->device_model_version = LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL; > > > - info->device_model = NULL; > > > - info->type = LIBXL_DOMAIN_TYPE_PV; > > > + info->target_ram = 0; > > > + info->videoram = 0; > > > + info->acpi = 0; > > > + info->vcpus = 0; > > > + info->vcpu_avail = 0; > > > + info->xen_platform_pci = 0; > > > return 0; > > > } > > > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c > > > index 5415bc5..74a77a7 100644 > > > --- a/tools/libxl/xl_cmdimpl.c > > > +++ b/tools/libxl/xl_cmdimpl.c > > > @@ -626,6 +626,8 @@ static void parse_config_data(const char *configfile_filename_report, > > > int pci_power_mgmt = 0; > > > int pci_msitranslate = 1; > > > int e; > > > + XLU_ConfigList *dmargs; > > > + int nr_dmargs = 0; > > > > > > libxl_domain_create_info *c_info = &d_config->c_info; > > > libxl_domain_build_info *b_info = &d_config->b_info; > > > @@ -1075,14 +1077,10 @@ skip_vfb: > > > break; > > > } > > > > > > - if (c_info->hvm == 1) { > > > - XLU_ConfigList *dmargs; > > > - int nr_dmargs = 0; > > > - > > > - /* init dm from c and b */ > > > - libxl_init_dm_info(dm_info, c_info, b_info); > > > + /* init dm from c and b */ > > > + libxl_init_dm_info(dm_info, c_info, b_info); > > > > > > - /* then process config related to dm */ > > > + if (c_info->hvm == 1) { > > > if (!xlu_cfg_get_string (config, "device_model", &buf)) { > > > fprintf(stderr, > > > "WARNING: ignoring device_model directive.\n" > > > @@ -1147,6 +1145,42 @@ skip_vfb: > > > dm_info->extra[i] = a ? strdup(a) : NULL; > > > } > > > } > > > + } else { > > > + if (!xlu_cfg_get_string (config, "device_model", &buf)) { > > > + fprintf(stderr, > > > + "WARNING: ignoring device_model directive.\n" > > > + "WARNING: Use \"device_model_override\" instead if you really want a non-default device_model\n"); > > > + } > > [...] > > > + if (!xlu_cfg_get_list(config, "device_model_args", &dmargs, &nr_dmargs, 0)) > > > + { > > > + int i; > > > + dm_info->extra = xmalloc(sizeof(char *) * (nr_dmargs + 1)); > > > + dm_info->extra[nr_dmargs] = NULL; > > > + for (i=0; i<nr_dmargs; i++) { > > > + const char *a = xlu_cfg_get_listitem(dmargs, i); > > > + dm_info->extra[i] = a ? strdup(a) : NULL; > > > + } > > > + } > > > > There is no need to duplicate all this code, is there? > > > > Just pull the relevant stuff from above out of the c_info->hvm == 1 > > case. > > > > Ah, yes. The dmargs parsing can be pulled out.And device_model and device_model_override AFAICT. Don''t we also want vnc*?> But the WARNING statements parts are differentHow so? The only one I can see is the one about stubdom-dm.> > One general concern I have is there are cases where we want 2 DM > > instances. In particular we can have an FV instance running in a stub > > domain which uses a PV instance running in dom0 for certain > > functionality (e.g. emulated VGA in the FV stub domain qemu goes to an > > xenfb frontend talking to a xenfb backend running in a PV qemu in domain > > 0). > > > > Haven''t come across such use case yet. Does libxl supports specifying > two DMs for one domain? What''s the syntax?It doesn''t, my point was how can we support that. [...]> > I guess for the most part we should treat both qemu''s in this case as > > the same logical entity split into two brains, so most of the options > > are common to both (and e.g. the versions are matched etc) with libxl > > taking care of directing the individual options to the right instance > > (or both). Yeah, that sounds like the answer. > > > > Only exception is the device_model_args option where I can see that > > passing different extra args to each instance would be useful and it is > > unlikely that libxl will understand them enough to choose where to send > > them, in fact we probably want 3 varialbe, device_model_args and > > device_model_args_{pv,fv}. > > > > Well, we already have device_model_args_{old,new} which handle qemu-xen > and upstream qemu respectively.Sorry, I wasn''t clear. I was referring to the "device_model_args" configuration variable which basically lets a user add arbitrary extra options to the DM command line. The old/new thing is to handle the differing syntaxes for the qemu-xen vs. upstream qemu trees and not the difference between pv/fv qemu particularly. Although I suppose we will potentially also need libxl__create_xenpv_qemu_old vs _new at some point too. Ian.> And the main goal of my patch is to > enable using upstream qemu as pv backend, so device_model_args_{pv,fv} > may not be sufficient -- we have two qemus for pv. The handling logic > will be complicated. > > > Ian. > > > > > >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2011-Jun-08 12:24 UTC
Re: [Xen-devel] [PATCH] libxl: enabling upstream qemu as pure pv backend.
On Wed, 2011-06-08 at 11:20 +0100, Wei Liu wrote:> On Wed, 2011-06-08 at 09:55 +0100, Ian Campbell wrote: > > One general concern I have is there are cases where we want 2 DM > > instances. In particular we can have an FV instance running in a stub > > domain which uses a PV instance running in dom0 for certain > > functionality (e.g. emulated VGA in the FV stub domain qemu goes to an > > xenfb frontend talking to a xenfb backend running in a PV qemu in domain > > 0). > > > > Ah, I misunderstood you at first. This use case is a bit complicated. I > still wonder if libxl supports this kind of configuration...I think so, and if not then it will need to at some point.> TBH, I''m not familiar with stubdom.stubdom is when you run the HVM device model in its own PV domain instead of as a process in domain 0. Long run this is the model we would like to be moving towards. Ian. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2011-Jun-08 12:27 UTC
Re: [Xen-devel] Re: [PATCH] libxl: enabling upstream qemu as pure pv backend.
On Wed, 2011-06-08 at 12:33 +0100, Stefano Stabellini wrote:> On Wed, 8 Jun 2011, Wei Liu wrote: > > commit 02cf9f9cfdf720c636c6ba08f795e49b5eb1f03e > > Author: Wei Liu <liuw@liuw.name> > > 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. > > > > Signed-off-by: Wei Liu <liuw@liuw.name> > > > > diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c > > index 62294b2..4ff3c7d 100644 > > --- a/tools/libxl/libxl_create.c > > +++ b/tools/libxl/libxl_create.c > > @@ -507,7 +507,8 @@ static int do_domain_create(libxl__gc *gc, libxl_domain_config *d_config, > > libxl_device_console_destroy(&console); > > > > if (need_qemu) > > - libxl__create_xenpv_qemu(gc, domid, d_config->vfbs, &dm_starting); > > + libxl__create_xenpv_qemu(gc, domid, &d_config->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..0505c84 100644 > > --- a/tools/libxl/libxl_dm.c > > +++ b/tools/libxl/libxl_dm.c > > @@ -702,7 +702,7 @@ retry_transaction: > > if (ret) > > goto out_free; > > } > > - if (libxl__create_xenpv_qemu(gc, domid, vfb, &dm_starting) < 0) { > > + if (libxl__create_xenpv_qemu(gc, domid, info, vfb, &dm_starting) < 0) { > > ret = ERROR_FAIL; > > goto out_free; > > } > > @@ -909,8 +909,8 @@ static int libxl__build_xenpv_qemu_args(libxl__gc *gc, > > libxl_device_model_info *info) > > { > > libxl_ctx *ctx = libxl__gc_owner(gc); > > - memset(info, 0x00, sizeof(libxl_device_model_info)); > > > > + info->vnc = 0; > > if (vfb != NULL) { > > info->vnc = vfb->vnc; > > if (vfb->vnclisten) > > @@ -927,9 +927,12 @@ static int libxl__build_xenpv_qemu_args(libxl__gc *gc, > > info->nographic = 1; > > info->domid = domid; > > info->dom_name = libxl_domid_to_name(ctx, domid); > > - info->device_model_version = LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL; > > - info->device_model = NULL; > > - info->type = LIBXL_DOMAIN_TYPE_PV; > > + info->target_ram = 0; > > + info->videoram = 0; > > + info->acpi = 0; > > + info->vcpus = 0; > > + info->vcpu_avail = 0; > > + info->xen_platform_pci = 0; > > return 0; > > } > > I don''t think is a good idea to reset all these value to 0 here, > considering that the info parameter can be passed by the user. > It is better to use another libxl_device_model_info local variable and > just copy the very few fields we care about. > Otherwise in the stubdom case above you''ll have the unwanted side effect > of removing useful informations of the stubdom from the structure.I think ideally the struct would be the same for both the PV and FV qemu and the device model create would only pay attention to the bits which fit the scenario (i.e. the PV case would ignore vcpus != 0, not zero it). This allows us to pass the same instance to both the PV and FV arg constructions routines in the stubdom+PV qemu case. Ian.> > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Stefano Stabellini
2011-Jun-08 13:00 UTC
Re: [Xen-devel] Re: [PATCH] libxl: enabling upstream qemu as pure pv backend.
On Wed, 8 Jun 2011, Ian Campbell wrote:> > I don''t think is a good idea to reset all these value to 0 here, > > considering that the info parameter can be passed by the user. > > It is better to use another libxl_device_model_info local variable and > > just copy the very few fields we care about. > > Otherwise in the stubdom case above you''ll have the unwanted side effect > > of removing useful informations of the stubdom from the structure. > > I think ideally the struct would be the same for both the PV and FV qemu > and the device model create would only pay attention to the bits which > fit the scenario (i.e. the PV case would ignore vcpus != 0, not zero > it). > > This allows us to pass the same instance to both the PV and FV arg > constructions routines in the stubdom+PV qemu case.I wouldn''t be opposed to it. The reason I didn''t suggest to make this change now is that I see a certain argument to keep the vfb settings separate, considering that vfb can be thought as implementation independent protocol. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2011-Jun-08 13:13 UTC
Re: [Xen-devel] Re: [PATCH] libxl: enabling upstream qemu as pure pv backend.
On Wed, 2011-06-08 at 14:00 +0100, Stefano Stabellini wrote:> On Wed, 8 Jun 2011, Ian Campbell wrote: > > > I don''t think is a good idea to reset all these value to 0 here, > > > considering that the info parameter can be passed by the user. > > > It is better to use another libxl_device_model_info local variable and > > > just copy the very few fields we care about. > > > Otherwise in the stubdom case above you''ll have the unwanted side effect > > > of removing useful informations of the stubdom from the structure. > > > > I think ideally the struct would be the same for both the PV and FV qemu > > and the device model create would only pay attention to the bits which > > fit the scenario (i.e. the PV case would ignore vcpus != 0, not zero > > it). > > > > This allows us to pass the same instance to both the PV and FV arg > > constructions routines in the stubdom+PV qemu case. > > I wouldn''t be opposed to it. > The reason I didn''t suggest to make this change now is that I see a > certain argument to keep the vfb settings separate, considering that vfb > can be thought as implementation independent protocol.Hmm, that''s an interesting case, but I think: if qemu-for-PV guest: vfb args -> xenpv qemu (no brainer, it''s the only one) if non-stub qemu-for-FV guest: vfb args -> xenfv qemu (no brainer, it''s the only one) if stub qemu-for-FV guest: vfb args -> xenpv qemu process (what the user connects to) do the right thing internally args -> xenfv in stub domain IOW in the stub case the args for the stub FV qemu are an implementation detail within libxl. Ian. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Stefano Stabellini
2011-Jun-08 13:43 UTC
Re: [Xen-devel] Re: [PATCH] libxl: enabling upstream qemu as pure pv backend.
On Wed, 8 Jun 2011, Ian Campbell wrote:> On Wed, 2011-06-08 at 14:00 +0100, Stefano Stabellini wrote: > > On Wed, 8 Jun 2011, Ian Campbell wrote: > > > > I don''t think is a good idea to reset all these value to 0 here, > > > > considering that the info parameter can be passed by the user. > > > > It is better to use another libxl_device_model_info local variable and > > > > just copy the very few fields we care about. > > > > Otherwise in the stubdom case above you''ll have the unwanted side effect > > > > of removing useful informations of the stubdom from the structure. > > > > > > I think ideally the struct would be the same for both the PV and FV qemu > > > and the device model create would only pay attention to the bits which > > > fit the scenario (i.e. the PV case would ignore vcpus != 0, not zero > > > it). > > > > > > This allows us to pass the same instance to both the PV and FV arg > > > constructions routines in the stubdom+PV qemu case. > > > > I wouldn''t be opposed to it. > > The reason I didn''t suggest to make this change now is that I see a > > certain argument to keep the vfb settings separate, considering that vfb > > can be thought as implementation independent protocol. > > Hmm, that''s an interesting case, but I think: > > if qemu-for-PV guest: > vfb args -> xenpv qemu (no brainer, it''s the only one) > if non-stub qemu-for-FV guest: > vfb args -> xenfv qemu (no brainer, it''s the only one) > if stub qemu-for-FV guest: > vfb args -> xenpv qemu process (what the user connects to) > do the right thing internally args -> xenfv in stub domain > > IOW in the stub case the args for the stub FV qemu are an implementation > detail within libxl.I agree. However I meant that one day we could have a vfb implementation that is not in Qemu. However this reason alone doesn''t prevent us from adding a libxl_device_vfb to libxl_device_model_info. So I guess we could remove the vfb parameter from libxl__create_xenpv_qemu and embed it into libxl_device_model_info. But if we do this we have to be careful because in the stubdom case the libxl_device_model_info that we pass to libxl__create_xenpv_qemu is NOT the same used to build the stubdom. The info parameter passed to libxl__create_stubdom is the libxl_device_model_info that describes the stub qemu-for-FV. While the info parameter that we pass to libxl__create_xenpv_qemu is the libxl_device_model_info that describes the qemu-for-PV in dom0. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2011-Jun-08 13:51 UTC
Re: [Xen-devel] Re: [PATCH] libxl: enabling upstream qemu as pure pv backend.
On Wed, 2011-06-08 at 14:43 +0100, Stefano Stabellini wrote:> On Wed, 8 Jun 2011, Ian Campbell wrote: > > On Wed, 2011-06-08 at 14:00 +0100, Stefano Stabellini wrote: > > > On Wed, 8 Jun 2011, Ian Campbell wrote: > > > > > I don''t think is a good idea to reset all these value to 0 here, > > > > > considering that the info parameter can be passed by the user. > > > > > It is better to use another libxl_device_model_info local variable and > > > > > just copy the very few fields we care about. > > > > > Otherwise in the stubdom case above you''ll have the unwanted side effect > > > > > of removing useful informations of the stubdom from the structure. > > > > > > > > I think ideally the struct would be the same for both the PV and FV qemu > > > > and the device model create would only pay attention to the bits which > > > > fit the scenario (i.e. the PV case would ignore vcpus != 0, not zero > > > > it). > > > > > > > > This allows us to pass the same instance to both the PV and FV arg > > > > constructions routines in the stubdom+PV qemu case. > > > > > > I wouldn''t be opposed to it. > > > The reason I didn''t suggest to make this change now is that I see a > > > certain argument to keep the vfb settings separate, considering that vfb > > > can be thought as implementation independent protocol. > > > > Hmm, that''s an interesting case, but I think: > > > > if qemu-for-PV guest: > > vfb args -> xenpv qemu (no brainer, it''s the only one) > > if non-stub qemu-for-FV guest: > > vfb args -> xenfv qemu (no brainer, it''s the only one) > > if stub qemu-for-FV guest: > > vfb args -> xenpv qemu process (what the user connects to) > > do the right thing internally args -> xenfv in stub domain > > > > IOW in the stub case the args for the stub FV qemu are an implementation > > detail within libxl. > > I agree. > However I meant that one day we could have a vfb implementation that is > not in Qemu. However this reason alone doesn''t prevent us from adding a > libxl_device_vfb to libxl_device_model_info. > So I guess we could remove the vfb parameter from > libxl__create_xenpv_qemu and embed it into libxl_device_model_info.Shouldn''t the vfb stuff come from the libxl_domain_info (is it c_ or b_ I forget) not the dm_info info? It''s really a property of the guest not the device model.> But if we do this we have to be careful because in the stubdom case the > libxl_device_model_info that we pass to libxl__create_xenpv_qemu is NOT > the same used to build the stubdom. > The info parameter passed to libxl__create_stubdom is the > libxl_device_model_info that describes the stub qemu-for-FV. > While the info parameter that we pass to libxl__create_xenpv_qemu is the > libxl_device_model_info that describes the qemu-for-PV in dom0.Right, my suggestion was that they potentially _could_ be the same, with the two functions doinging the right thing internally. This would avoid the need to figure out which params need to be copied from the user supplied struct to the xenpv one -- they would simply be the same. Ian. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Konrad Rzeszutek Wilk
2011-Jun-08 14:07 UTC
Re: [Xen-devel] [PATCH] libxl: enabling upstream qemu as pure pv backend.
On Wed, Jun 08, 2011 at 09:55:18AM +0100, Ian Campbell wrote:> On Wed, 2011-06-08 at 04:19 +0100, Wei Liu wrote: > > commit 02cf9f9cfdf720c636c6ba08f795e49b5eb1f03e > > Author: Wei Liu <liuw@liuw.name> > > 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. > > > > Signed-off-by: Wei Liu <liuw@liuw.name> > > > > @@ -909,8 +909,8 @@ static int libxl__build_xenpv_qemu_args(libxl__gc *gc, > > libxl_device_model_info *info) > > { > > libxl_ctx *ctx = libxl__gc_owner(gc); > > - memset(info, 0x00, sizeof(libxl_device_model_info)); > > Why do you remove this memset? > > > + info->vnc = 0; > > if (vfb != NULL) { > > info->vnc = vfb->vnc; > > if (vfb->vnclisten) > > @@ -927,9 +927,12 @@ static int libxl__build_xenpv_qemu_args(libxl__gc *gc, > > info->nographic = 1; > > info->domid = domid; > > info->dom_name = libxl_domid_to_name(ctx, domid); > > - info->device_model_version = LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL; > > - info->device_model = NULL; > > - info->type = LIBXL_DOMAIN_TYPE_PV; > > + info->target_ram = 0; > > + info->videoram = 0; > > + info->acpi = 0; > > + info->vcpus = 0; > > + info->vcpu_avail = 0; > > + info->xen_platform_pci = 0; > > return 0; > > } > > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c > > index 5415bc5..74a77a7 100644 > > --- a/tools/libxl/xl_cmdimpl.c > > +++ b/tools/libxl/xl_cmdimpl.c > > @@ -626,6 +626,8 @@ static void parse_config_data(const char *configfile_filename_report, > > int pci_power_mgmt = 0; > > int pci_msitranslate = 1; > > int e; > > + XLU_ConfigList *dmargs; > > + int nr_dmargs = 0; > > > > libxl_domain_create_info *c_info = &d_config->c_info; > > libxl_domain_build_info *b_info = &d_config->b_info; > > @@ -1075,14 +1077,10 @@ skip_vfb: > > break; > > } > > > > - if (c_info->hvm == 1) { > > - XLU_ConfigList *dmargs; > > - int nr_dmargs = 0; > > - > > - /* init dm from c and b */ > > - libxl_init_dm_info(dm_info, c_info, b_info); > > + /* init dm from c and b */ > > + libxl_init_dm_info(dm_info, c_info, b_info);Looks like a space issue. You might want to make sure that you have a uniform amount of spaces in the patches. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Stefano Stabellini
2011-Jun-08 15:51 UTC
Re: [Xen-devel] Re: [PATCH] libxl: enabling upstream qemu as pure pv backend.
On Wed, 8 Jun 2011, Ian Campbell wrote:> Shouldn''t the vfb stuff come from the libxl_domain_info (is it c_ or b_ > I forget) not the dm_info info? It''s really a property of the guest not > the device model.yeah, good point> > But if we do this we have to be careful because in the stubdom case the > > libxl_device_model_info that we pass to libxl__create_xenpv_qemu is NOT > > the same used to build the stubdom. > > The info parameter passed to libxl__create_stubdom is the > > libxl_device_model_info that describes the stub qemu-for-FV. > > While the info parameter that we pass to libxl__create_xenpv_qemu is the > > libxl_device_model_info that describes the qemu-for-PV in dom0. > > Right, my suggestion was that they potentially _could_ be the same, with > the two functions doinging the right thing internally. This would avoid > the need to figure out which params need to be copied from the user > supplied struct to the xenpv one -- they would simply be the same. >Ideally the vnc and sdl proprieties of a vfb should be moved into the corresponding fields of device_model_info, because they are not specific to the vfb protocol, rather a configuration of the backend (qemu). This way we wouldn''t need libxl__build_xenpv_qemu_args anymore. I wouldn''t use the same device_model_info instance for both qemu-FV and qemu-PV in the stubdom case, I would rather keep them separate. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2011-Jun-08 15:53 UTC
Re: [Xen-devel] Re: [PATCH] libxl: enabling upstream qemu as pure pv backend.
On Wed, 2011-06-08 at 16:51 +0100, Stefano Stabellini wrote:> On Wed, 8 Jun 2011, Ian Campbell wrote: > > Shouldn''t the vfb stuff come from the libxl_domain_info (is it c_ or b_ > > I forget) not the dm_info info? It''s really a property of the guest not > > the device model. > > yeah, good point > > > > > But if we do this we have to be careful because in the stubdom case the > > > libxl_device_model_info that we pass to libxl__create_xenpv_qemu is NOT > > > the same used to build the stubdom. > > > The info parameter passed to libxl__create_stubdom is the > > > libxl_device_model_info that describes the stub qemu-for-FV. > > > While the info parameter that we pass to libxl__create_xenpv_qemu is the > > > libxl_device_model_info that describes the qemu-for-PV in dom0. > > > > Right, my suggestion was that they potentially _could_ be the same, with > > the two functions doinging the right thing internally. This would avoid > > the need to figure out which params need to be copied from the user > > supplied struct to the xenpv one -- they would simply be the same. > > > > Ideally the vnc and sdl proprieties of a vfb should be moved into > the corresponding fields of device_model_info, because they are not > specific to the vfb protocol, rather a configuration of the backend > (qemu). > This way we wouldn''t need libxl__build_xenpv_qemu_args anymore. > > I wouldn''t use the same device_model_info instance for both qemu-FV and > qemu-PV in the stubdom case, I would rather keep them separate.OK, then we either need a way to configure both (dupping the options) or one needs to be derived from the other in some useful way... Ian. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Stefano Stabellini
2011-Jun-08 16:09 UTC
Re: [Xen-devel] Re: [PATCH] libxl: enabling upstream qemu as pure pv backend.
On Wed, 8 Jun 2011, Ian Campbell wrote:> > > > But if we do this we have to be careful because in the stubdom case the > > > > libxl_device_model_info that we pass to libxl__create_xenpv_qemu is NOT > > > > the same used to build the stubdom. > > > > The info parameter passed to libxl__create_stubdom is the > > > > libxl_device_model_info that describes the stub qemu-for-FV. > > > > While the info parameter that we pass to libxl__create_xenpv_qemu is the > > > > libxl_device_model_info that describes the qemu-for-PV in dom0. > > > > > > Right, my suggestion was that they potentially _could_ be the same, with > > > the two functions doinging the right thing internally. This would avoid > > > the need to figure out which params need to be copied from the user > > > supplied struct to the xenpv one -- they would simply be the same. > > > > > > > Ideally the vnc and sdl proprieties of a vfb should be moved into > > the corresponding fields of device_model_info, because they are not > > specific to the vfb protocol, rather a configuration of the backend > > (qemu). > > This way we wouldn''t need libxl__build_xenpv_qemu_args anymore. > > > > I wouldn''t use the same device_model_info instance for both qemu-FV and > > qemu-PV in the stubdom case, I would rather keep them separate. > > OK, then we either need a way to configure both (dupping the options) or > one needs to be derived from the other in some useful way...First the vfb configuration is derived from the xenfv configuration by libxl__vfb_and_vkb_from_device_model_info. Then from the vfb we build xenpv in libxl__create_xenpv_qemu. device_model_into->vfb->device_model_info If we move the sdl and vnc options away from vfb we could derive both the xenpv configuration and the vfb configuration in libxl__vfb_and_vkb_from_device_model_info, that would need to be renamed. libxl__create_stubdom would just add the vfb to xenstore and call libxl__create_xenpv_qemu that doesn''t need to do any "translation" anymore. So I guess it would simplify the code a bit. device_model_info-->device_model_info | -->vfb _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Wei Liu
2011-Jun-09 07:07 UTC
[Xen-devel] Re: [PATCH] libxl: enabling upstream qemu as pure pv backend.
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. * 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? * 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@liuw.name> 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@liuw.name> 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; } @@ -909,7 +927,6 @@ static int libxl__build_xenpv_qemu_args(libxl__gc *gc, libxl_device_model_info *info) { libxl_ctx *ctx = libxl__gc_owner(gc); - memset(info, 0x00, sizeof(libxl_device_model_info)); if (vfb != NULL) { info->vnc = vfb->vnc; @@ -927,9 +944,6 @@ static int libxl__build_xenpv_qemu_args(libxl__gc *gc, info->nographic = 1; info->domid = domid; info->dom_name = libxl_domid_to_name(ctx, domid); - info->device_model_version = LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL; - info->device_model = NULL; - info->type = LIBXL_DOMAIN_TYPE_PV; return 0; } @@ -985,12 +999,12 @@ out: return ret; } -int libxl__create_xenpv_qemu(libxl__gc *gc, uint32_t domid, libxl_device_vfb *vfb, +int libxl__create_xenpv_qemu(libxl__gc *gc, uint32_t domid, + libxl_device_model_info *info, + libxl_device_vfb *vfb, libxl__device_model_starting **starting_r) { - libxl_device_model_info info; - - libxl__build_xenpv_qemu_args(gc, domid, vfb, &info); - libxl__create_device_model(gc, &info, NULL, 0, NULL, 0, starting_r); + libxl__build_xenpv_qemu_args(gc, domid, vfb, info); + libxl__create_device_model(gc, info, NULL, 0, NULL, 0, starting_r); return 0; } diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h index 64e1d56..55e9f7f 100644 --- a/tools/libxl/libxl_internal.h +++ b/tools/libxl/libxl_internal.h @@ -256,7 +256,9 @@ _hidden int libxl__create_device_model(libxl__gc *gc, libxl_device_disk *disk, int num_disks, libxl_device_nic *vifs, int num_vifs, libxl__device_model_starting **starting_r); -_hidden int libxl__create_xenpv_qemu(libxl__gc *gc, uint32_t domid, libxl_device_vfb *vfb, +_hidden int libxl__create_xenpv_qemu(libxl__gc *gc, uint32_t domid, + libxl_device_model_info *dm_info, + libxl_device_vfb *vfb, libxl__device_model_starting **starting_r); _hidden int libxl__need_xenpv_qemu(libxl__gc *gc, int nr_consoles, libxl_device_console *consoles, diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c index 5415bc5..ced6dfb 100644 --- a/tools/libxl/xl_cmdimpl.c +++ b/tools/libxl/xl_cmdimpl.c @@ -627,6 +627,13 @@ static void parse_config_data(const char *configfile_filename_report, int pci_msitranslate = 1; int e; + XLU_ConfigList *dmargs; + int nr_dmargs = 0; + XLU_ConfigList *dmargs_fv; + int nr_dmargs_fv = 0; + XLU_ConfigList *dmargs_pv; + int nr_dmargs_pv = 0; + libxl_domain_create_info *c_info = &d_config->c_info; libxl_domain_build_info *b_info = &d_config->b_info; @@ -1075,40 +1082,76 @@ skip_vfb: break; } - if (c_info->hvm == 1) { - XLU_ConfigList *dmargs; - int nr_dmargs = 0; - - /* init dm from c and b */ - libxl_init_dm_info(dm_info, c_info, b_info); + /* init dm from c and b */ + libxl_init_dm_info(dm_info, c_info, b_info); + /* parse device model arguments, this works for pv, fv and stubdom */ + if (!xlu_cfg_get_string (config, "device_model", &buf)) { + fprintf(stderr, + "WARNING: ignoring device_model directive.\n" + "WARNING: Use \"device_model_override\" instead if you really want a non-default device_model\n"); + if (strstr(buf, "stubdom-dm")) { + if (c_info->hvm == 1) { + fprintf(stderr, "WARNING: Or use \"device_model_stubdomain_override\" if you want to enable stubdomains\n"); + } else { + fprintf(stderr, "WARNING: ignoring \"device_model_stubdomain_override\" directive for pv guest\n"); + } + } + } - /* then process config related to dm */ - if (!xlu_cfg_get_string (config, "device_model", &buf)) { + xlu_cfg_replace_string (config, "device_model_override", + &dm_info->device_model); + if (!xlu_cfg_get_string (config, "device_model_version", &buf)) { + if (!strcmp(buf, "qemu-xen-traditional")) { + dm_info->device_model_version + = LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL; + } else if (!strcmp(buf, "qemu-xen")) { + dm_info->device_model_version + = LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN; + } else { fprintf(stderr, - "WARNING: ignoring device_model directive.\n" - "WARNING: Use \"device_model_override\" instead if you really want a non-default device_model\n"); - if (strstr(buf, "stubdom-dm")) - fprintf(stderr, "WARNING: Or use \"device_model_stubdomain_override\" if you want to enable stubdomains\n"); + "Unknown device_model_version \"%s\" specified\n", buf); + exit(1); } + } else if (dm_info->device_model) + fprintf(stderr, "WARNING: device model override given without specific DM version\n"); + if (!xlu_cfg_get_long (config, "device_model_stubdomain_override", &l)) + dm_info->device_model_stubdomain = l; - xlu_cfg_replace_string (config, "device_model_override", - &dm_info->device_model); - if (!xlu_cfg_get_string (config, "device_model_version", &buf)) { - if (!strcmp(buf, "qemu-xen-traditional")) { - dm_info->device_model_version - = LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL; - } else if (!strcmp(buf, "qemu-xen")) { - dm_info->device_model_version - = LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN; - } else { - fprintf(stderr, - "Unknown device_model_version \"%s\" specified\n", buf); - exit(1); - } - } else if (dm_info->device_model) - fprintf(stderr, "WARNING: device model override given without specific DM version\n"); - if (!xlu_cfg_get_long (config, "device_model_stubdomain_override", &l)) - dm_info->device_model_stubdomain = l; + /* parse extra args for qemu, common to both pv, fv */ + if (!xlu_cfg_get_list(config, "device_model_args", &dmargs, &nr_dmargs, 0)) + { + int i; + dm_info->extra = xmalloc(sizeof(char *) * (nr_dmargs + 1)); + dm_info->extra[nr_dmargs] = NULL; + for (i=0; i<nr_dmargs; i++) { + const char *a = xlu_cfg_get_listitem(dmargs, i); + dm_info->extra[i] = a ? strdup(a) : NULL; + } + } + /* parse extra args dedicated to pv */ + if (!xlu_cfg_get_list(config, "device_model_args_pv", &dmargs_pv, &nr_dmargs_pv, 0)) + { + int i; + dm_info->extra_pv = xmalloc(sizeof(char *) * (nr_dmargs_pv + 1)); + dm_info->extra_pv[nr_dmargs_pv] = NULL; + for (i = 0; i<nr_dmargs_pv; i++) { + const char *a = xlu_cfg_get_listitem(dmargs_pv, i); + dm_info->extra_pv[i] = a ? strdup(a) : NULL; + } + } + /* parse extra args dedicated to fv */ + if (!xlu_cfg_get_list(config, "device_model_args_fv", &dmargs_fv, &nr_dmargs_fv, 0)) + { + int i; + dm_info->extra_fv = xmalloc(sizeof(char *) * (nr_dmargs_fv + 1)); + dm_info->extra_fv[nr_dmargs_fv] = NULL; + for (i = 0; i<nr_dmargs_fv; i++) { + const char *a = xlu_cfg_get_listitem(dmargs_fv, i); + dm_info->extra_fv[i] = a ? strdup(a) : NULL; + } + } + + if (c_info->hvm == 1) { if (!xlu_cfg_get_long (config, "stdvga", &l)) dm_info->stdvga = l; if (!xlu_cfg_get_long (config, "vnc", &l)) @@ -1136,17 +1179,6 @@ skip_vfb: xlu_cfg_replace_string (config, "soundhw", &dm_info->soundhw); if (!xlu_cfg_get_long (config, "xen_platform_pci", &l)) dm_info->xen_platform_pci = l; - - if (!xlu_cfg_get_list(config, "device_model_args", &dmargs, &nr_dmargs, 0)) - { - int i; - dm_info->extra = xmalloc(sizeof(char *) * (nr_dmargs + 1)); - dm_info->extra[nr_dmargs] = NULL; - for (i=0; i<nr_dmargs; i++) { - const char *a = xlu_cfg_get_listitem(dmargs, i); - dm_info->extra[i] = a ? strdup(a) : NULL; - } - } } dm_info->type = c_info->hvm ? _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2011-Jun-09 08:52 UTC
[Xen-devel] Re: [PATCH] libxl: enabling upstream qemu as pure pv backend.
On Thu, 2011-06-09 at 08:07 +0100, 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.Agreed although you will also need _old and _new variants, making four functions. It''s not clear how much in common they will have but please consider making pv vs fv a parameter to the _old/_new functions i.e. try and keep it down to just 2 functions (of course if they have nothing in common 4 functions will be fine).> * > 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.I''m wondering if all these functions shouldn''t take a libxl_domain_config (which contains libxl_dm_info), after all there is no fundamental reason that creating the DM should''t be at liberty to base it''s behaviour on any aspect of the domains cfg.> > * > vfb is derive from d_config (libxl_domain_config), which is a domain > property.Aha, an example of what I meant above, convenient ;-)> 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''m not entirely sure. In a world with multiple VFBs (note: we don''t currently support this)you could, I suppose have one on SDL and one on VNC. I suppose you might even want a single VFB exposed over both, that doesn''t seem unreasonable (maybe this works today?)> * > 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?There will always be a need for at least two DMs, that is in the stubdom case (one DM in a stubdom, the other in dom0), however they should both be the same version of the DM, i.e. both upstream or both traditional. In the future it''s also possible that we would want to have the option of multiple qemu''s, e.g. one per qdisk backend, for isolation and robustness.> * > 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?The device_model_args are basically a trap door to allow users to do things which libxl didn''t anticipate (i.e. as a stopgap until libxl can be updated with that feature). As such extra args could be needed for either DM. The distinction only really matters in the stubdom case.> 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<------------------[...]> @@ -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) {This is what I was thinking of when I said you might be able to just pass the same dm_info to both -- since you only ever copy fields verbatim, and libxl__create_xenpv_qemu (presumably) only looks at that subset of fields why not just pass the whole lot through. The rest looked ok, although I didn''t review in detail yet. Ian. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Wei Liu
2011-Jun-09 09:49 UTC
[Xen-devel] Re: [PATCH] libxl: enabling upstream qemu as pure pv backend.
On Thu, 2011-06-09 at 09:52 +0100, Ian Campbell wrote:> On Thu, 2011-06-09 at 08:07 +0100, 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. > > Agreed although you will also need _old and _new variants, making four > functions. It''s not clear how much in common they will have but please > consider making pv vs fv a parameter to the _old/_new functions i.e. try > and keep it down to just 2 functions (of course if they have nothing in > common 4 functions will be fine). >Well, I''m referring to configuration variables for config file but not functions in libxl... These config variables become parameters of libxl__build_device_model_args_{old,new}. (see patch) However, pv/fv configuration variables are related to guest machine type rather than qemu type. It is possible to add two more variables device_model_args_{old,new} and get them related to qemu type, if necessary. However, so many configuration variables, five in total -- pv/fv/old/new plus the original one, may confuse users. Too bad.> > * > > 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. > > I''m wondering if all these functions shouldn''t take a > libxl_domain_config (which contains libxl_dm_info), after all there is > no fundamental reason that creating the DM should''t be at liberty to > base it''s behaviour on any aspect of the domains cfg. >I don''t think so. DM creation has nothing to do with domain_config, that''s what I see in xl_cmdimpl.c:parse_config_data.> > > > * > > vfb is derive from d_config (libxl_domain_config), which is a domain > > property. > > Aha, an example of what I meant above, convenient ;-) > > > 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''m not entirely sure. In a world with multiple VFBs (note: we don''t > currently support this)you could, I suppose have one on SDL and one on > VNC. I suppose you might even want a single VFB exposed over both, that > doesn''t seem unreasonable (maybe this works today?) >Currently I have no idea how to do this.> > * > > 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? > > There will always be a need for at least two DMs, that is in the stubdom > case (one DM in a stubdom, the other in dom0), however they should both > be the same version of the DM, i.e. both upstream or both traditional. >I understand. DM in stubdom is statically packed in ioemu-stubdom.gz. Upstream qemu is not supported by now (AFAIK). In this case, we are not configuring two DMs, just one.> In the future it''s also possible that we would want to have the option > of multiple qemu''s, e.g. one per qdisk backend, for isolation and > robustness.> > * > > 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? > > The device_model_args are basically a trap door to allow users to do > things which libxl didn''t anticipate (i.e. as a stopgap until libxl can > be updated with that feature). As such extra args could be needed for > either DM. The distinction only really matters in the stubdom case. >Can you tell me how to pass parameters to the qemu running inside stubdom? What I see in the code is that libxl passes args to the xenpv qemu running in dom0 and leave qemu running inside stubdom untouched.> > 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<------------------ > [...] > > @@ -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) { > > This is what I was thinking of when I said you might be able to just > pass the same dm_info to both -- since you only ever copy fields > verbatim, and libxl__create_xenpv_qemu (presumably) only looks at that > subset of fields why not just pass the whole lot through. >I''m afraid this kind of verbatim copying is necessary. Luckily, this kind of operation is hidden from the user. In the original code, libxl__create_xenpv_qemu allocates its own dm_info, which is zero-out with libxl__build_xenpv_qemu_args before using. Because libxl__create_xenpv_qemu eventually calls libxl__create_device_model. If there are rubbish contents in dm_info, the creation is likely to fail.> The rest looked ok, although I didn''t review in detail yet. > > Ian. >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2011-Jun-09 10:15 UTC
[Xen-devel] Re: [PATCH] libxl: enabling upstream qemu as pure pv backend.
On Thu, 2011-06-09 at 10:49 +0100, Wei Liu wrote:> On Thu, 2011-06-09 at 09:52 +0100, Ian Campbell wrote: > > On Thu, 2011-06-09 at 08:07 +0100, 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. > > > > Agreed although you will also need _old and _new variants, making four > > functions. It''s not clear how much in common they will have but please > > consider making pv vs fv a parameter to the _old/_new functions i.e. try > > and keep it down to just 2 functions (of course if they have nothing in > > common 4 functions will be fine). > > > > Well, I''m referring to configuration variables for config file but not > functions in libxl...Oh right, it''s all rather confusingly named isn''t it ;-)> These config variables become parameters of > libxl__build_device_model_args_{old,new}. (see patch) > > However, pv/fv configuration variables are related to guest machine type > rather than qemu type. It is possible to add two more variables > device_model_args_{old,new} and get them related to qemu type, if > necessary.I don''t think this will be necessary. If a user has selected new qemu then they should expect to put device_model_args in the new syntax. if they intend to switch back and forth then they can always keep the other one commented out...> However, so many configuration variables, five in total -- pv/fv/old/new > plus the original one, may confuse users. Too bad.Yeah, just the 3 is bad enough, but necessary I think.> > > * > > > 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. > > > > I''m wondering if all these functions shouldn''t take a > > libxl_domain_config (which contains libxl_dm_info), after all there is > > no fundamental reason that creating the DM should''t be at liberty to > > base it''s behaviour on any aspect of the domains cfg. > > > > I don''t think so. DM creation has nothing to do with domain_config, > that''s what I see in xl_cmdimpl.c:parse_config_data.Well, the configuration of the DM you are creating has everything to do with the configuration of the domains it serves.> > > > > > > * > > > vfb is derive from d_config (libxl_domain_config), which is a domain > > > property. > > > > Aha, an example of what I meant above, convenient ;-) > > > > > 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''m not entirely sure. In a world with multiple VFBs (note: we don''t > > currently support this)you could, I suppose have one on SDL and one on > > VNC. I suppose you might even want a single VFB exposed over both, that > > doesn''t seem unreasonable (maybe this works today?) > > > > Currently I have no idea how to do this.Ignore it for now IMHO.> > > * > > > 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? > > > > There will always be a need for at least two DMs, that is in the stubdom > > case (one DM in a stubdom, the other in dom0), however they should both > > be the same version of the DM, i.e. both upstream or both traditional. > > > > I understand. DM in stubdom is statically packed in ioemu-stubdom.gz. > Upstream qemu is not supported by now (AFAIK).It will be (hopefully) in the future though.> In this case, we are not configuring two DMs, just one.The DM in a stubdom still has another qmeu process in dom0. I guess logically you can think of them as two parts of the same DM, which I guess is what you are doing?> In the future it''s also possible that we would want to have the option > > of multiple qemu''s, e.g. one per qdisk backend, for isolation and > > robustness. > > > > * > > > 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? > > > > The device_model_args are basically a trap door to allow users to do > > things which libxl didn''t anticipate (i.e. as a stopgap until libxl can > > be updated with that feature). As such extra args could be needed for > > either DM. The distinction only really matters in the stubdom case. > > > > Can you tell me how to pass parameters to the qemu running inside > stubdom? What I see in the code is that libxl passes args to the xenpv > qemu running in dom0 and leave qemu running inside stubdom untouched.It looks as if libxl__write_dmargs derives the stubdom parameters from the dom0 process parameters (by filtering out uninteresting options) and writes them to xenstore. Apart from, apparently, the "-domid <domid>" paramter which it puts on the stubdom kernel command line.> > > > 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<------------------ > > [...] > > > @@ -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) { > > > > This is what I was thinking of when I said you might be able to just > > pass the same dm_info to both -- since you only ever copy fields > > verbatim, and libxl__create_xenpv_qemu (presumably) only looks at that > > subset of fields why not just pass the whole lot through. > > > > I''m afraid this kind of verbatim copying is necessary. Luckily, this > kind of operation is hidden from the user. > > In the original code, libxl__create_xenpv_qemu allocates its own > dm_info, which is zero-out with libxl__build_xenpv_qemu_args before > using. Because libxl__create_xenpv_qemu eventually calls > libxl__create_device_model. If there are rubbish contents in dm_info, > the creation is likely to fail.OK. I think we can keep the copying for now and maybe someone will decide to untangle it a bit more in the future.> > The rest looked ok, although I didn''t review in detail yet. > > > > Ian. > > > >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Wei Liu
2011-Jun-09 10:47 UTC
[Xen-devel] Re: [PATCH] libxl: enabling upstream qemu as pure pv backend.
On Thu, 2011-06-09 at 11:15 +0100, Ian Campbell wrote:> Well, the configuration of the DM you are creating has everything to do > with the configuration of the domains it serves. >Ah, what I mean is that in current design, everything needed to create a DM is stored in dm_info, so we don''t have to tangle with domain_config. I would rather leave those interfaces unchanged by now.> The DM in a stubdom still has another qmeu process in dom0. I guess > logically you can think of them as two parts of the same DM, which I > guess is what you are doing? >Yes, I understand. That''s why I''m talking about one DM, not two.> It looks as if libxl__write_dmargs derives the stubdom parameters from > the dom0 process parameters (by filtering out uninteresting options) and > writes them to xenstore. >Hmm... That''s it. The xenpv qemu and xenfv qemu in stubdom are sharing the same configuration variable. Wei. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Stefano Stabellini
2011-Jun-09 15:20 UTC
[Xen-devel] Re: [PATCH] libxl: enabling upstream qemu as pure pv backend.
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).> * > 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@liuw.name> > 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@liuw.name> > > 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. Apart from Ian''s comments, the rest is fine. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Wei Liu
2011-Jun-09 15:49 UTC
[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@eu.citrix.com> 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@liuw.name> >> 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@liuw.name> >> >> 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@lists.xensource.com http://lists.xensource.com/xen-devel
Stefano Stabellini
2011-Jun-09 16:00 UTC
[Xen-devel] Re: [PATCH] libxl: enabling upstream qemu as pure pv backend.
On Thu, 9 Jun 2011, Wei Liu wrote:> What should be improved? This thread is so long, can you be more specific? >I had the impression that Ian had some specific comments to be addresses but reading back the thread I may be wrong. Ian, are you OK with this patch? _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Wei Liu
2011-Jun-10 05:47 UTC
[Xen-devel] Re: [PATCH] libxl: enabling upstream qemu as pure pv backend.
On Thu, 2011-06-09 at 16:20 +0100, Stefano Stabellini 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). > > > > * > > 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@liuw.name> > > 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@liuw.name> > > > > 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. >I re-check this part and find that the xenpv_dm_info.type is necessary to be hardcoded. Things are a bit different in stubdom creation compared to pv creation. In pv creation, copying is ok, because there is only one qemu we need to pay attention to. In fv with stubdom case. There are two qemu''s. xenpv qemu <--> stubdom (pv) with xenfv qemu <--> hvm domU That''s the relationship between them. To my understanding, the `type` field in dm_info represents which type of domain the qemu is supporting. The original device model info (which is the `info` passed in libxl__create_stubdom) is filled with user config. It represents user''s domU DM config, which has type LIBXL_DOMAIN_TYPE_FV (supports a fv domain), while the xenpv supporting stubdom is expecting type LIBXL_DOMAIN_TYPE_PV (stubdom is pv). If I directly copy this field from info, libxl__create_xenpv_qemu (which calls general creating function libxl__create_device_model) will create a FV stubdom. And the qemu running in stubdom is hardcoded with ''-M xenfv'' option in libxl_dm.c:libxl__write_dmargs. So I think hardcode `type` is acceptable. To summarize: these two qemu''s are by design of two different types. copying device_model_version and device_model fields is ok.> Apart from Ian''s comments, the rest is fine._______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Stefano Stabellini
2011-Jun-10 11:05 UTC
[Xen-devel] Re: [PATCH] libxl: enabling upstream qemu as pure pv backend.
On Fri, 10 Jun 2011, Wei Liu wrote:> > > @@ -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. > > > > I re-check this part and find that the xenpv_dm_info.type is necessary > to be hardcoded. > > Things are a bit different in stubdom creation compared to pv creation. > In pv creation, copying is ok, because there is only one qemu we need to > pay attention to. > > In fv with stubdom case. There are two qemu''s. > > xenpv qemu <--> stubdom (pv) with xenfv qemu <--> hvm domU > > That''s the relationship between them. > > To my understanding, the `type` field in dm_info represents which type > of domain the qemu is supporting. > > The original device model info (which is the `info` passed in > libxl__create_stubdom) is filled with user config. It represents user''s > domU DM config, which has type LIBXL_DOMAIN_TYPE_FV (supports a fv > domain), while the xenpv supporting stubdom is expecting type > LIBXL_DOMAIN_TYPE_PV (stubdom is pv). > > If I directly copy this field from info, libxl__create_xenpv_qemu (which > calls general creating function libxl__create_device_model) will create > a FV stubdom. > > And the qemu running in stubdom is hardcoded with ''-M xenfv'' option in > libxl_dm.c:libxl__write_dmargs. So I think hardcode `type` is > acceptable. > > To summarize: > these two qemu''s are by design of two different types. > copying device_model_version and device_model fields is ok.Sorry my mistake. You are absolutely right; just copy device_model_version and device_model. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel