[PATCH] libxl: enabling upstream qemu as pure pv backend.

[PATCH] libxl: enabling upstream qemu as pure pv backend.

[Wei Liu]

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 ?

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

[Ian Campbell]

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.

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

[Wei Liu]

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

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

[Wei Liu]

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.

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

[Stefano Stabellini]

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.

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

[Stefano Stabellini]

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.

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

[Ian Campbell]

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 different

How 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.
> > 
> > 
> 
> 

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

[Ian Campbell]

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.

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

[Ian Campbell]

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

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

[Stefano Stabellini]

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.

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

[Ian Campbell]

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.

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

[Stefano Stabellini]

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.

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

[Ian Campbell]

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.

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

[Konrad Rzeszutek Wilk]

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.

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

[Stefano Stabellini]

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.

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

[Ian Campbell]

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.

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

[Stefano Stabellini]

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

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

[Wei Liu]

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 ?

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

[Ian Campbell]

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.

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

[Wei Liu]

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

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

[Ian Campbell]

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

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

[Wei Liu]

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.

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

[Stefano Stabellini]

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.

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

[Wei Liu]

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?

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

[Stefano Stabellini]

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?

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

[Wei Liu]

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.

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

[Stefano Stabellini]

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.

[PATCH] libxl: enabling upstream qemu as pure pv backend.

[Wei Liu]

This patch was dropped. Resending.

This version contains minor bug fix. Please don't use previous version.

-----------------------------------------------------------

commit a0f5de6a0cf3e033efbc2297817bcddcdce6f0fd
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 adds 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.idl b/tools/libxl/libxl.idl
index f7249b1..183d2cd 100644
--- a/tools/libxl/libxl.idl
+++ b/tools/libxl/libxl.idl
@@ -240,6 +240,8 @@ The password never expires"""),
     ("vcpu_avail",       integer,           False, "vcpus actually available"),
     ("xen_platform_pci", bool,              False, "enable/disable the xen platform pci device"),
     ("extra",            libxl_string_list, False, "extra parameters pass directly to qemu, NULL terminated"),
+    ("extra_pv",         libxl_string_list, False, "extra parameters pass directly to qemu for PV guest, NULL terminated"),
+    ("extra_fv",         libxl_string_list, False, "extra parameters pass directly to qemu for FV guest, NULL terminated"),
     ],
     comment=
 """Device Model information.
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index b74b66f..0dc0d11 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -495,6 +495,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]);
@@ -515,8 +516,18 @@ 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;
+        xenpv_dm_info.extra = d_config->dm_info.extra;
+        xenpv_dm_info.extra_pv = d_config->dm_info.extra_pv;
+        xenpv_dm_info.extra_fv = d_config->dm_info.extra_fv;
         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 b9bf4b0..e8a7664 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -206,9 +206,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);
@@ -403,9 +407,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;
     }
 
@@ -614,6 +622,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;
@@ -647,6 +656,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;
@@ -741,7 +751,14 @@ 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 = info->device_model_version;
+    xenpv_dm_info.type = LIBXL_DOMAIN_TYPE_PV;
+    xenpv_dm_info.device_model = info->device_model;
+    if (libxl__create_xenpv_qemu(gc, domid,
+                                 &xenpv_dm_info,
+                                 vfb, &dm_starting) < 0) {
         ret = ERROR_FAIL;
         goto out_free;
     }
@@ -950,7 +967,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;
@@ -968,9 +984,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;
 }
 
@@ -1011,12 +1024,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 579188e..3175368 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -264,7 +264,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 dafd741..be58871 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -523,6 +523,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;
 
@@ -994,40 +1001,74 @@ 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 (c_info->hvm == 1 &&
+        !xlu_cfg_get_long (config, "device_model_stubdomain_override", &l))
+        dm_info->device_model_stubdomain = l;
+
+#define parse_extra_args(type)                                          \
+    if (!xlu_cfg_get_list(config, "device_model_args"#type,             \
+                          &dmargs##type, &nr_dmargs##type, 0))          \
+    {                                                                   \
+        int i;                                                          \
+        dm_info->extra##type =                                          \
+            xmalloc(sizeof(char*)*(nr_dmargs##type + 1));               \
+        dm_info->extra##type[nr_dmargs##type] = NULL;                   \
+        for (i=0; i<nr_dmargs##type; i++) {                             \
+            const char *a = xlu_cfg_get_listitem(dmargs##type, i);      \
+            dm_info->extra##type[i] = a ? strdup(a) : NULL;             \
+        }                                                               \
+    }                                                                   \
 
-        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 */
+    parse_extra_args();
+
+    /* parse extra args dedicated to pv */
+    parse_extra_args(_pv);
+
+    /* parse extra args dedicated to fv */
+    parse_extra_args(_fv);
+
+#undef parse_extra_args
+
+    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))
@@ -1069,17 +1110,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 ?