Xen devel - Jun 2013 - [PATCH] Add a device_model_machine option for HVM domains to libxl.

The device-model machine type used for HVM domains is currently hardcoded to
''xenfv''. This patch adds a device_model_machine option, which
defaults to
''xenfv'' for comatibility, but allows the specification of an
alternative
machine type ''pc''.
Note that use of the ''pc'' machine type relies on patches that
I have
submitted to qemu-devel.

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
---
 docs/man/xl.cfg.pod.5       |   27 +++++++++++++++++++++++++++
 tools/libxl/libxl_create.c  |    3 +++
 tools/libxl/libxl_dm.c      |   27 ++++++++++++++++++++++-----
 tools/libxl/libxl_types.idl |    7 +++++++
 tools/libxl/xl_cmdimpl.c    |    9 +++++++++
 5 files changed, 68 insertions(+), 5 deletions(-)

diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
index b7d64a6..e066778 100644
--- a/docs/man/xl.cfg.pod.5
+++ b/docs/man/xl.cfg.pod.5
@@ -966,6 +966,9 @@ improves performance and is strongly recommended when
available. PV
 drivers are available for various Operating Systems including HVM
 Linux L<http://wiki.xen.org/wiki/XenLinuxPVonHVMdrivers> and Microsoft
 Windows L<http://wiki.xen.org/wiki/XenWindowsGplPv>.
+This option only has effect when using the qemu-xen-traditional
+device-model, or when a device_model_machine value of something other
+than xenfv is specified when using the upstream qemu-xen device-model.
 
 =item B<viridian=BOOLEAN>
 
@@ -1197,6 +1200,30 @@ you have existing guests then, depending on the nature of
the guest
 Operating System, you may wish to force them to use the device
 model which they were installed with.
 
+=item B<device_model_machine="MACHINE-TYPE">
+
+Selects which machine type should be specified to the device-model.
+This option only takes effect when using the upstream qemu-xen
+device-model.
+Valid values are:
+
+=over 4
+
+=item B<xenfv>
+
+This is the default and implies creation of the Xen platform PCI
+device.
+
+=item B<pc>
+
+Creation of the Xen platform PCI device is controlled by the
+xen_platform_pci option.
+
+=back
+
+Use of the pc machine type may not work with versions of
+the qemu-xen device-model prior to 1.6.
+
 =item B<device_model_override="PATH">
 
 Override the path to the binary to be used as the device-model. The
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 0c32d0b..2b84e9e 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -246,6 +246,9 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
         libxl_defbool_setdefault(&b_info->u.hvm.usb,               
false);
         libxl_defbool_setdefault(&b_info->u.hvm.xen_platform_pci,  
true);
 
+        if (b_info->u.hvm.machine == LIBXL_DEVICE_MODEL_HVM_MACHINE_DEFAULT)
+            b_info->u.hvm.machine = LIBXL_DEVICE_MODEL_HVM_MACHINE_XENFV;
+
         if (!b_info->u.hvm.boot) {
             b_info->u.hvm.boot = strdup("cda");
             if (!b_info->u.hvm.boot) return ERROR_NOMEM;
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index ac1f90e..eb2ca46 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -570,18 +570,31 @@ static char **
libxl__build_device_model_args_new(libxl__gc *gc,
     }
     for (i = 0; b_info->extra && b_info->extra[i] != NULL; i++)
         flexarray_append(dm_args, b_info->extra[i]);
-    flexarray_append(dm_args, "-M");
+
+    flexarray_append(dm_args, "-machine");
     switch (b_info->type) {
     case LIBXL_DOMAIN_TYPE_PV:
         flexarray_append(dm_args, "xenpv");
         for (i = 0; b_info->extra_pv && b_info->extra_pv[i] !=
NULL; i++)
             flexarray_append(dm_args, b_info->extra_pv[i]);
         break;
-    case LIBXL_DOMAIN_TYPE_HVM:
-        flexarray_append(dm_args, "xenfv");
+    case LIBXL_DOMAIN_TYPE_HVM: {
+        char *machine = (char
*)libxl_device_model_hvm_machine_to_string(b_info->u.hvm.machine);
+
+        if (b_info->u.hvm.machine != LIBXL_DEVICE_MODEL_HVM_MACHINE_XENFV) {
+            flexarray_append(dm_args, libxl__sprintf(gc,
"type=%s,accel=xen", machine));
+            if (libxl_defbool_val(b_info->u.hvm.xen_platform_pci)) {
+                flexarray_append(dm_args, "-device");
+                flexarray_append(dm_args, "xen-platform");
+            }
+        } else {
+            flexarray_append(dm_args, libxl__sprintf(gc, "type=%s",
machine));
+        }
+
         for (i = 0; b_info->extra_hvm && b_info->extra_hvm[i] !=
NULL; i++)
             flexarray_append(dm_args, b_info->extra_hvm[i]);
         break;
+    }
     default:
         abort();
     }
@@ -748,11 +761,15 @@ static int libxl__write_stub_dmargs(libxl__gc *gc,
     i = 1;
     dmargs[0] = ''\0'';
     while (args[i] != NULL) {
-        if (strcmp(args[i], "-sdl") && strcmp(args[i],
"-M") && strcmp(args[i], "xenfv")) {
+        if (!strcmp(args[i], "-sdl"))
+            i++;
+        else if (!strcmp(args[i], "-M"))
+            i += 2;
+        else {
             strcat(dmargs, " ");
             strcat(dmargs, args[i]);
+            i++;
         }
-        i++;
     }
     path = libxl__sprintf(gc, "%s/image/dmargs", vm_path);
 
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index d218a2d..bf31249 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -39,6 +39,12 @@ libxl_device_model_version =
Enumeration("device_model_version", [
     (2, "QEMU_XEN"),             # Upstream based qemu-xen device
model
     ])
 
+libxl_device_model_hvm_machine =
Enumeration("device_model_hvm_machine", [
+    (0, "DEFAULT"),
+    (1, "XENFV"),
+    (2, "PC"),
+    ])
+
 libxl_console_type = Enumeration("console_type", [
     (1, "SERIAL"),
     (2, "PV"),
@@ -331,6 +337,7 @@ libxl_domain_build_info =
Struct("domain_build_info",[
                                        ("usbdevice",        string),
                                        ("soundhw",          string),
                                        ("xen_platform_pci",
libxl_defbool),
+                                       ("machine",         
libxl_device_model_hvm_machine),
                                        ("usbdevice_list",  
libxl_string_list),
                                        ])),
                  ("pv", Struct(None, [("kernel", string),
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 8a478ba..2fe5532 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1526,6 +1526,15 @@ skip_vfb:
             exit (1);
 
         }
+
+        b_info->u.hvm.machine = LIBXL_DEVICE_MODEL_HVM_MACHINE_DEFAULT;
+        if (!xlu_cfg_get_string (config, "device_model_machine",
&buf, 0)) {
+            if (libxl_device_model_hvm_machine_from_string(buf,
&b_info->u.hvm.machine) < 0) {
+                fprintf(stderr,
+                    "Unknown device_model_machine type \"%s\"
specified\n", buf);
+                exit (1);
+            }
+        }
     }
 
     xlu_cfg_destroy(config);
-- 
1.7.10.4

On Mon, 17 Jun 2013, Paul Durrant wrote:
> The device-model machine type used for HVM domains is currently hardcoded
to
> ''xenfv''. This patch adds a device_model_machine option,
which defaults to
> ''xenfv'' for comatibility, but allows the specification of
an alternative
> machine type ''pc''.
> Note that use of the ''pc'' machine type relies on patches
that I have
> submitted to qemu-devel.
> 
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
NACK

I don''t think that this kind of option should exposed to the user
except
maybe for debugging purposed.
We need a way to automatically select the right one.

>  docs/man/xl.cfg.pod.5       |   27 +++++++++++++++++++++++++++
>  tools/libxl/libxl_create.c  |    3 +++
>  tools/libxl/libxl_dm.c      |   27 ++++++++++++++++++++++-----
>  tools/libxl/libxl_types.idl |    7 +++++++
>  tools/libxl/xl_cmdimpl.c    |    9 +++++++++
>  5 files changed, 68 insertions(+), 5 deletions(-)
> 
> diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
> index b7d64a6..e066778 100644
> --- a/docs/man/xl.cfg.pod.5
> +++ b/docs/man/xl.cfg.pod.5
> @@ -966,6 +966,9 @@ improves performance and is strongly recommended when
available. PV
>  drivers are available for various Operating Systems including HVM
>  Linux L<http://wiki.xen.org/wiki/XenLinuxPVonHVMdrivers> and
Microsoft
>  Windows L<http://wiki.xen.org/wiki/XenWindowsGplPv>.
> +This option only has effect when using the qemu-xen-traditional
> +device-model, or when a device_model_machine value of something other
> +than xenfv is specified when using the upstream qemu-xen device-model.
>  
>  =item B<viridian=BOOLEAN>
>  
> @@ -1197,6 +1200,30 @@ you have existing guests then, depending on the
nature of the guest
>  Operating System, you may wish to force them to use the device
>  model which they were installed with.
>  
> +=item B<device_model_machine="MACHINE-TYPE">
> +
> +Selects which machine type should be specified to the device-model.
> +This option only takes effect when using the upstream qemu-xen
> +device-model.
> +Valid values are:
> +
> +=over 4
> +
> +=item B<xenfv>
> +
> +This is the default and implies creation of the Xen platform PCI
> +device.
> +
> +=item B<pc>
> +
> +Creation of the Xen platform PCI device is controlled by the
> +xen_platform_pci option.
> +
> +=back
> +
> +Use of the pc machine type may not work with versions of
> +the qemu-xen device-model prior to 1.6.
> +
>  =item B<device_model_override="PATH">
>  
>  Override the path to the binary to be used as the device-model. The
> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index 0c32d0b..2b84e9e 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -246,6 +246,9 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
>          libxl_defbool_setdefault(&b_info->u.hvm.usb,               
false);
>          libxl_defbool_setdefault(&b_info->u.hvm.xen_platform_pci,  
true);
>  
> +        if (b_info->u.hvm.machine ==
LIBXL_DEVICE_MODEL_HVM_MACHINE_DEFAULT)
> +            b_info->u.hvm.machine =
LIBXL_DEVICE_MODEL_HVM_MACHINE_XENFV;
> +
>          if (!b_info->u.hvm.boot) {
>              b_info->u.hvm.boot = strdup("cda");
>              if (!b_info->u.hvm.boot) return ERROR_NOMEM;
> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index ac1f90e..eb2ca46 100644
> --- a/tools/libxl/libxl_dm.c
> +++ b/tools/libxl/libxl_dm.c
> @@ -570,18 +570,31 @@ static char **
libxl__build_device_model_args_new(libxl__gc *gc,
>      }
>      for (i = 0; b_info->extra && b_info->extra[i] != NULL;
i++)
>          flexarray_append(dm_args, b_info->extra[i]);
> -    flexarray_append(dm_args, "-M");
> +
> +    flexarray_append(dm_args, "-machine");
>      switch (b_info->type) {
>      case LIBXL_DOMAIN_TYPE_PV:
>          flexarray_append(dm_args, "xenpv");
>          for (i = 0; b_info->extra_pv && b_info->extra_pv[i]
!= NULL; i++)
>              flexarray_append(dm_args, b_info->extra_pv[i]);
>          break;
> -    case LIBXL_DOMAIN_TYPE_HVM:
> -        flexarray_append(dm_args, "xenfv");
> +    case LIBXL_DOMAIN_TYPE_HVM: {
> +        char *machine = (char
*)libxl_device_model_hvm_machine_to_string(b_info->u.hvm.machine);
> +
> +        if (b_info->u.hvm.machine !=
LIBXL_DEVICE_MODEL_HVM_MACHINE_XENFV) {
> +            flexarray_append(dm_args, libxl__sprintf(gc,
"type=%s,accel=xen", machine));
> +            if (libxl_defbool_val(b_info->u.hvm.xen_platform_pci)) {
> +                flexarray_append(dm_args, "-device");
> +                flexarray_append(dm_args, "xen-platform");
> +            }
> +        } else {
> +            flexarray_append(dm_args, libxl__sprintf(gc,
"type=%s", machine));
> +        }
> +
>          for (i = 0; b_info->extra_hvm &&
b_info->extra_hvm[i] != NULL; i++)
>              flexarray_append(dm_args, b_info->extra_hvm[i]);
>          break;
> +    }
>      default:
>          abort();
>      }
> @@ -748,11 +761,15 @@ static int libxl__write_stub_dmargs(libxl__gc *gc,
>      i = 1;
>      dmargs[0] = ''\0'';
>      while (args[i] != NULL) {
> -        if (strcmp(args[i], "-sdl") && strcmp(args[i],
"-M") && strcmp(args[i], "xenfv")) {
> +        if (!strcmp(args[i], "-sdl"))
> +            i++;
> +        else if (!strcmp(args[i], "-M"))
> +            i += 2;
> +        else {
>              strcat(dmargs, " ");
>              strcat(dmargs, args[i]);
> +            i++;
>          }
> -        i++;
>      }
>      path = libxl__sprintf(gc, "%s/image/dmargs", vm_path);
>  
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index d218a2d..bf31249 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -39,6 +39,12 @@ libxl_device_model_version =
Enumeration("device_model_version", [
>      (2, "QEMU_XEN"),             # Upstream based qemu-xen
device model
>      ])
>  
> +libxl_device_model_hvm_machine =
Enumeration("device_model_hvm_machine", [
> +    (0, "DEFAULT"),
> +    (1, "XENFV"),
> +    (2, "PC"),
> +    ])
> +
>  libxl_console_type = Enumeration("console_type", [
>      (1, "SERIAL"),
>      (2, "PV"),
> @@ -331,6 +337,7 @@ libxl_domain_build_info =
Struct("domain_build_info",[
>                                         ("usbdevice",       
string),
>                                         ("soundhw",         
string),
>                                         ("xen_platform_pci",
libxl_defbool),
> +                                       ("machine",         
libxl_device_model_hvm_machine),
>                                         ("usbdevice_list",  
libxl_string_list),
>                                         ])),
>                   ("pv", Struct(None, [("kernel",
string),
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 8a478ba..2fe5532 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -1526,6 +1526,15 @@ skip_vfb:
>              exit (1);
>  
>          }
> +
> +        b_info->u.hvm.machine = LIBXL_DEVICE_MODEL_HVM_MACHINE_DEFAULT;
> +        if (!xlu_cfg_get_string (config, "device_model_machine",
&buf, 0)) {
> +            if (libxl_device_model_hvm_machine_from_string(buf,
&b_info->u.hvm.machine) < 0) {
> +                fprintf(stderr,
> +                    "Unknown device_model_machine type
\"%s\" specified\n", buf);
> +                exit (1);
> +            }
> +        }
>      }
>  
>      xlu_cfg_destroy(config);
> -- 
> 1.7.10.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>

On Tue, 2013-06-18 at 20:06 +0100, Stefano Stabellini
wrote:
> On Mon, 17 Jun 2013, Paul Durrant wrote:
> > The device-model machine type used for HVM domains is currently
hardcoded to
> > ''xenfv''. This patch adds a device_model_machine
option, which defaults to
> > ''xenfv'' for comatibility, but allows the
specification of an alternative
> > machine type ''pc''.
> > Note that use of the ''pc'' machine type relies on
patches that I have
> > submitted to qemu-devel.
> > 
> > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> 
> NACK
> 
> I don''t think that this kind of option should exposed to the user
except
> maybe for debugging purposed.
> We need a way to automatically select the right one.
I think ultimately we will need some sort of option like this to support
evolving the virtualised platform, i.e. when we want to introduce for
more modern systems (e.g. Q35 based with PCIe support).

I didn''t review in details since obviously there is a higher level
discussion to have first, but I did spot:
> +        b_info->u.hvm.machine = LIBXL_DEVICE_MODEL_HVM_MACHINE_DEFAULT;
This isn''t needed, libxl_domain_build_info_init() will DTRT here.

Ian,

> -----Original Message-----
> From: Ian Campbell
> Sent: 19 June 2013 09:10
> To: Stefano Stabellini
> Cc: Paul Durrant; xen-devel@lists.xen.org
> Subject: Re: [Xen-devel] [PATCH] Add a device_model_machine option for
> HVM domains to libxl.
> 
> On Tue, 2013-06-18 at 20:06 +0100, Stefano Stabellini wrote:
> > On Mon, 17 Jun 2013, Paul Durrant wrote:
> > > The device-model machine type used for HVM domains is currently
> hardcoded to
> > > ''xenfv''. This patch adds a device_model_machine
option, which defaults
> to
> > > ''xenfv'' for comatibility, but allows the
specification of an alternative
> > > machine type ''pc''.
> > > Note that use of the ''pc'' machine type relies
on patches that I have
> > > submitted to qemu-devel.
> > >
> > > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> >
> > NACK
> >
> > I don''t think that this kind of option should exposed to the
user except
> > maybe for debugging purposed.
> > We need a way to automatically select the right one.
> 
> I think ultimately we will need some sort of option like this to support
> evolving the virtualised platform, i.e. when we want to introduce for
> more modern systems (e.g. Q35 based with PCIe support).
> 
> I didn''t review in details since obviously there is a higher level
> discussion to have first, but I did spot:
> 
> > +        b_info->u.hvm.machine >
LIBXL_DEVICE_MODEL_HVM_MACHINE_DEFAULT;
> 
> This isn''t needed, libxl_domain_build_info_init() will DTRT here.
> 
That''s what I thought. But without that line I seem to get some random
value.

  Paul

> -----Original Message-----
> From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com]
> Sent: 18 June 2013 20:06
> To: Paul Durrant
> Cc: xen-devel@lists.xen.org
> Subject: Re: [Xen-devel] [PATCH] Add a device_model_machine option for
> HVM domains to libxl.
> 
> On Mon, 17 Jun 2013, Paul Durrant wrote:
> > The device-model machine type used for HVM domains is currently
> hardcoded to
> > ''xenfv''. This patch adds a device_model_machine
option, which defaults to
> > ''xenfv'' for comatibility, but allows the
specification of an alternative
> > machine type ''pc''.
> > Note that use of the ''pc'' machine type relies on
patches that I have
> > submitted to qemu-devel.
> >
> > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> 
> NACK
> 
> I don''t think that this kind of option should exposed to the user
except
> maybe for debugging purposed.
> We need a way to automatically select the right one.
> 
Ok, but what is the *right* one? Would you accept this patch without the manpage
tweak? Xenfv''s hardcoded platform device creation is completely
stalling my attempts to get Citrix Windows PV drivers running on upstream.
Having this option and defaulting it to xenfv seems like a fairly pragmatic
approach to me.

  Paul

On Wed, 2013-06-19 at 09:29 +0100, Paul Durrant wrote:
> > > +        b_info->u.hvm.machine > >
LIBXL_DEVICE_MODEL_HVM_MACHINE_DEFAULT;
> > 
> > This isn''t needed, libxl_domain_build_info_init() will DTRT
here.
> > 
> 
> That''s what I thought. But without that line I seem to get some
random value.
Weird. The autogenerated function "libxl_domain_build_info_init_type"
should have coped with this, can you post its content?

Ian.

> -----Original Message-----
> From: Ian Campbell
> Sent: 19 June 2013 09:48
> To: Paul Durrant
> Cc: Stefano Stabellini; xen-devel@lists.xen.org
> Subject: Re: [Xen-devel] [PATCH] Add a device_model_machine option for
> HVM domains to libxl.
> 
> On Wed, 2013-06-19 at 09:29 +0100, Paul Durrant wrote:
> > > > +        b_info->u.hvm.machine > > >
LIBXL_DEVICE_MODEL_HVM_MACHINE_DEFAULT;
> > >
> > > This isn''t needed, libxl_domain_build_info_init() will
DTRT here.
> > >
> >
> > That''s what I thought. But without that line I seem to get
some random
> value.
> 
> Weird. The autogenerated function
"libxl_domain_build_info_init_type"
> should have coped with this, can you post its content?
> 
No sign of initialization here... Perhaps I needed to delete the fail to cause
re-generation, or maybe running ''make tools'' is not enough?

void libxl_domain_build_info_init_type(libxl_domain_build_info *p,
libxl_domain_type type)
{
    assert(p->type == LIBXL_DOMAIN_TYPE_INVALID);
    p->type = type;
    switch (p->type) {
    case LIBXL_DOMAIN_TYPE_HVM:
        p->u.hvm.timer_mode = LIBXL_TIMER_MODE_DEFAULT;
        libxl_vga_interface_info_init(&p->u.hvm.vga);
        libxl_vnc_info_init(&p->u.hvm.vnc);
        libxl_sdl_info_init(&p->u.hvm.sdl);
        libxl_spice_info_init(&p->u.hvm.spice);
        break;
    case LIBXL_DOMAIN_TYPE_PV:
        p->u.pv.slack_memkb = LIBXL_MEMKB_DEFAULT;
        break;
    case LIBXL_DOMAIN_TYPE_INVALID:
        break;
    }
}

On Wed, 2013-06-19 at 10:00 +0100, Paul Durrant wrote:
> > -----Original Message-----
> > From: Ian Campbell
> > Sent: 19 June 2013 09:48
> > To: Paul Durrant
> > Cc: Stefano Stabellini; xen-devel@lists.xen.org
> > Subject: Re: [Xen-devel] [PATCH] Add a device_model_machine option for
> > HVM domains to libxl.
> > 
> > On Wed, 2013-06-19 at 09:29 +0100, Paul Durrant wrote:
> > > > > +        b_info->u.hvm.machine > > > >
LIBXL_DEVICE_MODEL_HVM_MACHINE_DEFAULT;
> > > >
> > > > This isn''t needed, libxl_domain_build_info_init()
will DTRT here.
> > > >
> > >
> > > That''s what I thought. But without that line I seem to
get some random
> > value.
> > 
> > Weird. The autogenerated function
"libxl_domain_build_info_init_type"
> > should have coped with this, can you post its content?
> > 
> 
> No sign of initialization here... 
Ah, I think what is supposed to happen is:
libxl_domain_build_info_init: memsets the whole thing to zero and then
explicitly initialises any non-zero defaults

libxl_domain_build_info_init_type: initialises any additional non-zero
defaults based on having determined the type for the union.

Since your enum defaults to zero I think it is therefore expected that
it isn''t explicitly initialised, although it doesn''t explain
why the
field isn''t caught by the memset.
> Perhaps I needed to delete the fail to cause re-generation, or maybe
running ''make tools'' is not enough?
Good idea to be safe.

If that doesn''t help then my guess would be that something is
overwriting the field (e.g. via an erroneous use of u.pv outside a
switch/if). In that case could you try bisecting with printf between the
call to libxl_domain_build_info_init and the point where you needed to
initialise.
> 
> void libxl_domain_build_info_init_type(libxl_domain_build_info *p,
libxl_domain_type type)
> {
>     assert(p->type == LIBXL_DOMAIN_TYPE_INVALID);
>     p->type = type;
>     switch (p->type) {
>     case LIBXL_DOMAIN_TYPE_HVM:
>         p->u.hvm.timer_mode = LIBXL_TIMER_MODE_DEFAULT;
>         libxl_vga_interface_info_init(&p->u.hvm.vga);
>         libxl_vnc_info_init(&p->u.hvm.vnc);
>         libxl_sdl_info_init(&p->u.hvm.sdl);
>         libxl_spice_info_init(&p->u.hvm.spice);
>         break;
>     case LIBXL_DOMAIN_TYPE_PV:
>         p->u.pv.slack_memkb = LIBXL_MEMKB_DEFAULT;
>         break;
>     case LIBXL_DOMAIN_TYPE_INVALID:
>         break;
>     }
> }
>

On Wed, 19 Jun 2013, Paul Durrant wrote:
> > -----Original Message-----
> > From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com]
> > Sent: 18 June 2013 20:06
> > To: Paul Durrant
> > Cc: xen-devel@lists.xen.org
> > Subject: Re: [Xen-devel] [PATCH] Add a device_model_machine option for
> > HVM domains to libxl.
> > 
> > On Mon, 17 Jun 2013, Paul Durrant wrote:
> > > The device-model machine type used for HVM domains is currently
> > hardcoded to
> > > ''xenfv''. This patch adds a device_model_machine
option, which defaults to
> > > ''xenfv'' for comatibility, but allows the
specification of an alternative
> > > machine type ''pc''.
> > > Note that use of the ''pc'' machine type relies
on patches that I have
> > > submitted to qemu-devel.
> > >
> > > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> > 
> > NACK
> > 
> > I don''t think that this kind of option should exposed to the
user except
> > maybe for debugging purposed.
> > We need a way to automatically select the right one.
> > 
> 
> Ok, but what is the *right* one? Would you accept this patch without the
manpage tweak? Xenfv''s hardcoded platform device creation is completely
stalling my attempts to get Citrix Windows PV drivers running on upstream.
Having this option and defaulting it to xenfv seems like a fairly pragmatic
approach to me.
Sorry, I realize that my messages have been a bit confusing.

I am not opposed to having this option as a companion of
device_model_override, I am just opposed to using this option to solve
the compatibility problem with QEMU.

I would like to see an additional patch that queries the running QEMU
for its version and uses it to determine the default QEMU machine
version.  Then we can also allow users to override it with something
like this patch.

> -----Original Message-----
> From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com]
> Sent: 19 June 2013 18:26
> To: Paul Durrant
> Cc: Stefano Stabellini; xen-devel@lists.xen.org
> Subject: RE: [Xen-devel] [PATCH] Add a device_model_machine option for
> HVM domains to libxl.
> 
> On Wed, 19 Jun 2013, Paul Durrant wrote:
> > > -----Original Message-----
> > > From: Stefano Stabellini
[mailto:stefano.stabellini@eu.citrix.com]
> > > Sent: 18 June 2013 20:06
> > > To: Paul Durrant
> > > Cc: xen-devel@lists.xen.org
> > > Subject: Re: [Xen-devel] [PATCH] Add a device_model_machine
option
> for
> > > HVM domains to libxl.
> > >
> > > On Mon, 17 Jun 2013, Paul Durrant wrote:
> > > > The device-model machine type used for HVM domains is
currently
> > > hardcoded to
> > > > ''xenfv''. This patch adds a
device_model_machine option, which
> defaults to
> > > > ''xenfv'' for comatibility, but allows the
specification of an alternative
> > > > machine type ''pc''.
> > > > Note that use of the ''pc'' machine type
relies on patches that I have
> > > > submitted to qemu-devel.
> > > >
> > > > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> > >
> > > NACK
> > >
> > > I don''t think that this kind of option should exposed to
the user except
> > > maybe for debugging purposed.
> > > We need a way to automatically select the right one.
> > >
> >
> > Ok, but what is the *right* one? Would you accept this patch without
the
> manpage tweak? Xenfv''s hardcoded platform device creation is
completely
> stalling my attempts to get Citrix Windows PV drivers running on upstream.
> Having this option and defaulting it to xenfv seems like a fairly pragmatic
> approach to me.
> 
> Sorry, I realize that my messages have been a bit confusing.
> 
> I am not opposed to having this option as a companion of
> device_model_override, I am just opposed to using this option to solve
> the compatibility problem with QEMU.
> 
> I would like to see an additional patch that queries the running QEMU
> for its version and uses it to determine the default QEMU machine
> version.  Then we can also allow users to override it with something
> like this patch.
Yes, that''s quite reasonable and such an additional patch series is my
intention, but like so many things I don''t have time to work on that
now. So, if you think the new option has implications beyond my intention as an
override then I''d be happy to modify it.
Here''s a plan:

- I make it freeform, rather than an enumeration (meaning folks have to add the
,accel=xen option themselves, but giving flexibility)
- I make it more clear in the manpage (and possibly in the option name itself)
that it is an override

Does that sound ok?

  Paul

On Thu, 20 Jun 2013, Paul Durrant wrote:
> > -----Original Message-----
> > From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com]
> > Sent: 19 June 2013 18:26
> > To: Paul Durrant
> > Cc: Stefano Stabellini; xen-devel@lists.xen.org
> > Subject: RE: [Xen-devel] [PATCH] Add a device_model_machine option for
> > HVM domains to libxl.
> > 
> > On Wed, 19 Jun 2013, Paul Durrant wrote:
> > > > -----Original Message-----
> > > > From: Stefano Stabellini
[mailto:stefano.stabellini@eu.citrix.com]
> > > > Sent: 18 June 2013 20:06
> > > > To: Paul Durrant
> > > > Cc: xen-devel@lists.xen.org
> > > > Subject: Re: [Xen-devel] [PATCH] Add a device_model_machine
option
> > for
> > > > HVM domains to libxl.
> > > >
> > > > On Mon, 17 Jun 2013, Paul Durrant wrote:
> > > > > The device-model machine type used for HVM domains is
currently
> > > > hardcoded to
> > > > > ''xenfv''. This patch adds a
device_model_machine option, which
> > defaults to
> > > > > ''xenfv'' for comatibility, but allows
the specification of an alternative
> > > > > machine type ''pc''.
> > > > > Note that use of the ''pc'' machine
type relies on patches that I have
> > > > > submitted to qemu-devel.
> > > > >
> > > > > Signed-off-by: Paul Durrant
<paul.durrant@citrix.com>
> > > >
> > > > NACK
> > > >
> > > > I don''t think that this kind of option should
exposed to the user except
> > > > maybe for debugging purposed.
> > > > We need a way to automatically select the right one.
> > > >
> > >
> > > Ok, but what is the *right* one? Would you accept this patch
without the
> > manpage tweak? Xenfv''s hardcoded platform device creation is
completely
> > stalling my attempts to get Citrix Windows PV drivers running on
upstream.
> > Having this option and defaulting it to xenfv seems like a fairly
pragmatic
> > approach to me.
> > 
> > Sorry, I realize that my messages have been a bit confusing.
> > 
> > I am not opposed to having this option as a companion of
> > device_model_override, I am just opposed to using this option to solve
> > the compatibility problem with QEMU.
> > 
> > I would like to see an additional patch that queries the running QEMU
> > for its version and uses it to determine the default QEMU machine
> > version.  Then we can also allow users to override it with something
> > like this patch.
> 
> Yes, that''s quite reasonable and such an additional patch series
is my intention, but like so many things I don''t have time to work on
that now.
You just need to use the already existing QMP command query-version and
send it to the QEMU instance instantiated from the init script. It
should be a pretty small patch.

> So, if you think the new option has implications beyond my intention as an
override then I''d be happy to modify it.
> Here''s a plan:
> 
> - I make it freeform, rather than an enumeration (meaning folks have to add
the ,accel=xen option themselves, but giving flexibility)
> - I make it more clear in the manpage (and possibly in the option name
itself) that it is an override
> 
> Does that sound ok?
They look like reasonable improvements over this patch.

On Thu, 2013-06-20 at 11:04 +0100, Stefano Stabellini
wrote:
> On Thu, 20 Jun 2013, Paul Durrant wrote:
> > > -----Original Message-----
> > > From: Stefano Stabellini
[mailto:stefano.stabellini@eu.citrix.com]
> > > Sent: 19 June 2013 18:26
> > > To: Paul Durrant
> > > Cc: Stefano Stabellini; xen-devel@lists.xen.org
> > > Subject: RE: [Xen-devel] [PATCH] Add a device_model_machine
option for
> > > HVM domains to libxl.
> > > 
> > > On Wed, 19 Jun 2013, Paul Durrant wrote:
> > > > > -----Original Message-----
> > > > > From: Stefano Stabellini
[mailto:stefano.stabellini@eu.citrix.com]
> > > > > Sent: 18 June 2013 20:06
> > > > > To: Paul Durrant
> > > > > Cc: xen-devel@lists.xen.org
> > > > > Subject: Re: [Xen-devel] [PATCH] Add a
device_model_machine option
> > > for
> > > > > HVM domains to libxl.
> > > > >
> > > > > On Mon, 17 Jun 2013, Paul Durrant wrote:
> > > > > > The device-model machine type used for HVM domains
is currently
> > > > > hardcoded to
> > > > > > ''xenfv''. This patch adds a
device_model_machine option, which
> > > defaults to
> > > > > > ''xenfv'' for comatibility, but
allows the specification of an alternative
> > > > > > machine type ''pc''.
> > > > > > Note that use of the ''pc''
machine type relies on patches that I have
> > > > > > submitted to qemu-devel.
> > > > > >
> > > > > > Signed-off-by: Paul Durrant
<paul.durrant@citrix.com>
> > > > >
> > > > > NACK
> > > > >
> > > > > I don''t think that this kind of option should
exposed to the user except
> > > > > maybe for debugging purposed.
> > > > > We need a way to automatically select the right one.
> > > > >
> > > >
> > > > Ok, but what is the *right* one? Would you accept this patch
without the
> > > manpage tweak? Xenfv''s hardcoded platform device
creation is completely
> > > stalling my attempts to get Citrix Windows PV drivers running on
upstream.
> > > Having this option and defaulting it to xenfv seems like a fairly
pragmatic
> > > approach to me.
> > > 
> > > Sorry, I realize that my messages have been a bit confusing.
> > > 
> > > I am not opposed to having this option as a companion of
> > > device_model_override, I am just opposed to using this option to
solve
> > > the compatibility problem with QEMU.
> > > 
> > > I would like to see an additional patch that queries the running
QEMU
> > > for its version and uses it to determine the default QEMU machine
> > > version.  Then we can also allow users to override it with
something
> > > like this patch.
> > 
> > Yes, that''s quite reasonable and such an additional patch
series is my intention, but like so many things I don''t have time to
work on that now.
> 
> You just need to use the already existing QMP command query-version and
> send it to the QEMU instance instantiated from the init script. It
> should be a pretty small patch.
Don''t forget that it also needs to detect the case where the user has
used an override for the qemu binary, such that it doesn''t match the
dom0 instance. Or are you planning to just ignore this case and require
users of device_model_override to also use the machine type override? 
> > So, if you think the new option has implications beyond my intention
as an override then I''d be happy to modify it.
> > Here''s a plan:
> > 
> > - I make it freeform, rather than an enumeration (meaning folks have
to add the ,accel=xen option themselves, but giving flexibility)
> > - I make it more clear in the manpage (and possibly in the option name
itself) that it is an override
> > 
> > Does that sound ok?
> 
> They look like reasonable improvements over this patch.
I''m not sure that requiring a free form field rather than an
enumeration
is the best interface. An enumeration allows us to note which machines
we have support for i.e. the things we might reasonably expect people to
use in order to remain compatible with the virtual platform from a
previous release. It also naturally provides us a list of versions which
we can compare against qemu''s machine-list command.

A free form list does give more of an escape hatch though.

Ian.

On Wed, 26 Jun 2013, Ian Campbell wrote:
> On Thu, 2013-06-20 at 11:04 +0100, Stefano Stabellini wrote:
> > On Thu, 20 Jun 2013, Paul Durrant wrote:
> > > > -----Original Message-----
> > > > From: Stefano Stabellini
[mailto:stefano.stabellini@eu.citrix.com]
> > > > Sent: 19 June 2013 18:26
> > > > To: Paul Durrant
> > > > Cc: Stefano Stabellini; xen-devel@lists.xen.org
> > > > Subject: RE: [Xen-devel] [PATCH] Add a device_model_machine
option for
> > > > HVM domains to libxl.
> > > > 
> > > > On Wed, 19 Jun 2013, Paul Durrant wrote:
> > > > > > -----Original Message-----
> > > > > > From: Stefano Stabellini
[mailto:stefano.stabellini@eu.citrix.com]
> > > > > > Sent: 18 June 2013 20:06
> > > > > > To: Paul Durrant
> > > > > > Cc: xen-devel@lists.xen.org
> > > > > > Subject: Re: [Xen-devel] [PATCH] Add a
device_model_machine option
> > > > for
> > > > > > HVM domains to libxl.
> > > > > >
> > > > > > On Mon, 17 Jun 2013, Paul Durrant wrote:
> > > > > > > The device-model machine type used for HVM
domains is currently
> > > > > > hardcoded to
> > > > > > > ''xenfv''. This patch adds a
device_model_machine option, which
> > > > defaults to
> > > > > > > ''xenfv'' for comatibility,
but allows the specification of an alternative
> > > > > > > machine type ''pc''.
> > > > > > > Note that use of the ''pc''
machine type relies on patches that I have
> > > > > > > submitted to qemu-devel.
> > > > > > >
> > > > > > > Signed-off-by: Paul Durrant
<paul.durrant@citrix.com>
> > > > > >
> > > > > > NACK
> > > > > >
> > > > > > I don''t think that this kind of option
should exposed to the user except
> > > > > > maybe for debugging purposed.
> > > > > > We need a way to automatically select the right
one.
> > > > > >
> > > > >
> > > > > Ok, but what is the *right* one? Would you accept this
patch without the
> > > > manpage tweak? Xenfv''s hardcoded platform device
creation is completely
> > > > stalling my attempts to get Citrix Windows PV drivers
running on upstream.
> > > > Having this option and defaulting it to xenfv seems like a
fairly pragmatic
> > > > approach to me.
> > > > 
> > > > Sorry, I realize that my messages have been a bit confusing.
> > > > 
> > > > I am not opposed to having this option as a companion of
> > > > device_model_override, I am just opposed to using this
option to solve
> > > > the compatibility problem with QEMU.
> > > > 
> > > > I would like to see an additional patch that queries the
running QEMU
> > > > for its version and uses it to determine the default QEMU
machine
> > > > version.  Then we can also allow users to override it with
something
> > > > like this patch.
> > > 
> > > Yes, that''s quite reasonable and such an additional
patch series is my intention, but like so many things I don''t have time
to work on that now.
> > 
> > You just need to use the already existing QMP command query-version
and
> > send it to the QEMU instance instantiated from the init script. It
> > should be a pretty small patch.
> 
> Don''t forget that it also needs to detect the case where the user
has
> used an override for the qemu binary, such that it doesn''t match
the
> dom0 instance. Or are you planning to just ignore this case and require
> users of device_model_override to also use the machine type override? 
I think that ignoring the case, although not ideal, would be acceptable.