[PATCH] Add device_model_pvdevice parameter for HVM guests

[PATCH] Add device_model_pvdevice parameter for HVM guests

[Paul Durrant]

The parameter determines which, if any, xen-pvdevice is specified on the
QEMU command line. The default value is 'none' which means no argument will
be passed. A value of 'xenserver' specifies a xen-pvdevice with device-id
0xc000 (the initial value in the xenserver namespace - see
docs/misc/pci-device-reservations.txt).

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@citrix.com>
---
 docs/man/xl.cfg.pod.5       |   22 ++++++++++++++++++++++
 tools/libxl/libxl_dm.c      |    9 +++++++++
 tools/libxl/libxl_types.idl |    5 +++++
 tools/libxl/xl_cmdimpl.c    |   14 ++++++++++++++
 4 files changed, 50 insertions(+)

diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
index b7d64a6..2220bb6 100644
--- a/docs/man/xl.cfg.pod.5
+++ b/docs/man/xl.cfg.pod.5
@@ -1214,6 +1214,28 @@ you have selected.
 
 Assign an XSM security label to the device-model stubdomain.
 
+=item B<device_model_pvdevice="PVDEVICE">
+
+Selects which variant of the xen-pvdevice should be used for this
+guest. Valid values are:
+
+=over 4
+
+=item B<none>
+
+The xen-pvdevice should be omitted. This is the default.
+
+=item B<xenserver>
+
+The xenserver variant of the xen-pvdevice will be specified, enabling
+the use of XenServer PV drivers in the guest.
+
+=back
+
+This parameter only takes effect when device_model_version=qemu-xen.
+
+=back
+
 =item B<device_model_args=[ "ARG", "ARG", ...]>
 
 Pass additional arbitrary options on the device-model command
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index ac1f90e..2924298 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -647,6 +647,15 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc,
             flexarray_append(dm_args, "-drive");
             flexarray_append(dm_args, drive);
         }
+
+        switch (b_info->u.hvm.device_model_pvdevice) {
+        case LIBXL_DEVICE_MODEL_PVDEVICE_XENSERVER:
+            flexarray_append(dm_args, "-device");
+            flexarray_append(dm_args, "xen-pvdevice,device-id=0xc000");
+            break;
+        default:
+            break;
+        }
     }
     flexarray_append(dm_args, NULL);
     return (char **) flexarray_contents(dm_args);
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index d218a2d..7165139 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -132,6 +132,10 @@ libxl_vga_interface_type = Enumeration("vga_interface_type", [
     (2, "STD"),
     ], init_val = 0)
 
+libxl_device_model_pvdevice = Enumeration("device_model_pvdevice", [
+    (0, "NONE"),
+    (1, "XENSERVER"),
+    ])
 #
 # Complex libxl types
 #
@@ -332,6 +336,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
                                        ("soundhw",          string),
                                        ("xen_platform_pci", libxl_defbool),
                                        ("usbdevice_list",   libxl_string_list),
+                                       ("device_model_pvdevice", libxl_device_model_pvdevice),
                                        ])),
                  ("pv", Struct(None, [("kernel", string),
                                       ("slack_memkb", MemKB),
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 8a478ba..cfaa54e 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1526,6 +1526,20 @@ skip_vfb:
             exit (1);
 
         }
+
+        if (!xlu_cfg_get_string (config, "device_model_pvdevice", &buf, 0)) {
+            libxl_device_model_pvdevice d;
+
+            e = libxl_device_model_pvdevice_from_string(buf, &d);
+            if (e) {
+                fprintf(stderr,
+                        "xl: unknown device_model_pvdevice '%s'\n",
+                        buf);
+                exit(-ERROR_FAIL);
+            }
+                
+            b_info->u.hvm.device_model_pvdevice = d;
+        }
     }
 
     xlu_cfg_destroy(config);
-- 
1.7.10.4

Re: [PATCH] Add device_model_pvdevice parameter for HVM guests

[Paul Durrant]

> -----Original Message-----
> From: Paul Durrant [mailto:paul.durrant@citrix.com]
> Sent: 04 July 2013 11:50
> To: xen-devel@lists.xen.org
> Cc: Paul Durrant; Stefano Stabellini
> Subject: [PATCH] Add device_model_pvdevice parameter for HVM guests
> 
> The parameter determines which, if any, xen-pvdevice is specified on the
> QEMU command line. The default value is 'none' which means no argument
> will
> be passed. A value of 'xenserver' specifies a xen-pvdevice with device-id
> 0xc000 (the initial value in the xenserver namespace - see
> docs/misc/pci-device-reservations.txt).
> 
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> Cc: Stefano Stabellini <stefano.stabellini@citrix.com>

Ping?

> ---
>  docs/man/xl.cfg.pod.5       |   22 ++++++++++++++++++++++
>  tools/libxl/libxl_dm.c      |    9 +++++++++
>  tools/libxl/libxl_types.idl |    5 +++++
>  tools/libxl/xl_cmdimpl.c    |   14 ++++++++++++++
>  4 files changed, 50 insertions(+)
> 
> diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
> index b7d64a6..2220bb6 100644
> --- a/docs/man/xl.cfg.pod.5
> +++ b/docs/man/xl.cfg.pod.5
> @@ -1214,6 +1214,28 @@ you have selected.
> 
>  Assign an XSM security label to the device-model stubdomain.
> 
> +=item B<device_model_pvdevice="PVDEVICE">
> +
> +Selects which variant of the xen-pvdevice should be used for this
> +guest. Valid values are:
> +
> +=over 4
> +
> +=item B<none>
> +
> +The xen-pvdevice should be omitted. This is the default.
> +
> +=item B<xenserver>
> +
> +The xenserver variant of the xen-pvdevice will be specified, enabling
> +the use of XenServer PV drivers in the guest.
> +
> +=back
> +
> +This parameter only takes effect when device_model_version=qemu-xen.
> +
> +=back
> +
>  =item B<device_model_args=[ "ARG", "ARG", ...]>
> 
>  Pass additional arbitrary options on the device-model command
> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index ac1f90e..2924298 100644
> --- a/tools/libxl/libxl_dm.c
> +++ b/tools/libxl/libxl_dm.c
> @@ -647,6 +647,15 @@ static char **
> libxl__build_device_model_args_new(libxl__gc *gc,
>              flexarray_append(dm_args, "-drive");
>              flexarray_append(dm_args, drive);
>          }
> +
> +        switch (b_info->u.hvm.device_model_pvdevice) {
> +        case LIBXL_DEVICE_MODEL_PVDEVICE_XENSERVER:
> +            flexarray_append(dm_args, "-device");
> +            flexarray_append(dm_args, "xen-pvdevice,device-id=0xc000");
> +            break;
> +        default:
> +            break;
> +        }
>      }
>      flexarray_append(dm_args, NULL);
>      return (char **) flexarray_contents(dm_args);
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index d218a2d..7165139 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -132,6 +132,10 @@ libxl_vga_interface_type =
> Enumeration("vga_interface_type", [
>      (2, "STD"),
>      ], init_val = 0)
> 
> +libxl_device_model_pvdevice = Enumeration("device_model_pvdevice", [
> +    (0, "NONE"),
> +    (1, "XENSERVER"),
> +    ])
>  #
>  # Complex libxl types
>  #
> @@ -332,6 +336,7 @@ libxl_domain_build_info =
> Struct("domain_build_info",[
>                                         ("soundhw",          string),
>                                         ("xen_platform_pci", libxl_defbool),
>                                         ("usbdevice_list",   libxl_string_list),
> +                                       ("device_model_pvdevice",
> libxl_device_model_pvdevice),
>                                         ])),
>                   ("pv", Struct(None, [("kernel", string),
>                                        ("slack_memkb", MemKB),
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 8a478ba..cfaa54e 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -1526,6 +1526,20 @@ skip_vfb:
>              exit (1);
> 
>          }
> +
> +        if (!xlu_cfg_get_string (config, "device_model_pvdevice", &buf, 0)) {
> +            libxl_device_model_pvdevice d;
> +
> +            e = libxl_device_model_pvdevice_from_string(buf, &d);
> +            if (e) {
> +                fprintf(stderr,
> +                        "xl: unknown device_model_pvdevice '%s'\n",
> +                        buf);
> +                exit(-ERROR_FAIL);
> +            }
> +
> +            b_info->u.hvm.device_model_pvdevice = d;
> +        }
>      }
> 
>      xlu_cfg_destroy(config);
> --
> 1.7.10.4

Re: [PATCH] Add device_model_pvdevice parameter for HVM guests

[Stefano Stabellini]

On Thu, 11 Jul 2013, Paul Durrant wrote:
> > -----Original Message-----
> > From: Paul Durrant [mailto:paul.durrant@citrix.com]
> > Sent: 04 July 2013 11:50
> > To: xen-devel@lists.xen.org
> > Cc: Paul Durrant; Stefano Stabellini
> > Subject: [PATCH] Add device_model_pvdevice parameter for HVM guests
> > 
> > The parameter determines which, if any, xen-pvdevice is specified on the
> > QEMU command line. The default value is 'none' which means no argument
> > will
> > be passed. A value of 'xenserver' specifies a xen-pvdevice with device-id
> > 0xc000 (the initial value in the xenserver namespace - see
> > docs/misc/pci-device-reservations.txt).
> > 
> > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> > Cc: Stefano Stabellini <stefano.stabellini@citrix.com>
> 
> Ping?

The patch looks OK to me.



> > ---
> >  docs/man/xl.cfg.pod.5       |   22 ++++++++++++++++++++++
> >  tools/libxl/libxl_dm.c      |    9 +++++++++
> >  tools/libxl/libxl_types.idl |    5 +++++
> >  tools/libxl/xl_cmdimpl.c    |   14 ++++++++++++++
> >  4 files changed, 50 insertions(+)
> > 
> > diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
> > index b7d64a6..2220bb6 100644
> > --- a/docs/man/xl.cfg.pod.5
> > +++ b/docs/man/xl.cfg.pod.5
> > @@ -1214,6 +1214,28 @@ you have selected.
> > 
> >  Assign an XSM security label to the device-model stubdomain.
> > 
> > +=item B<device_model_pvdevice="PVDEVICE">
> > +
> > +Selects which variant of the xen-pvdevice should be used for this
> > +guest. Valid values are:
> > +
> > +=over 4
> > +
> > +=item B<none>
> > +
> > +The xen-pvdevice should be omitted. This is the default.
> > +
> > +=item B<xenserver>
> > +
> > +The xenserver variant of the xen-pvdevice will be specified, enabling
> > +the use of XenServer PV drivers in the guest.
> > +
> > +=back
> > +
> > +This parameter only takes effect when device_model_version=qemu-xen.
> > +
> > +=back
> > +
> >  =item B<device_model_args=[ "ARG", "ARG", ...]>
> > 
> >  Pass additional arbitrary options on the device-model command
> > diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> > index ac1f90e..2924298 100644
> > --- a/tools/libxl/libxl_dm.c
> > +++ b/tools/libxl/libxl_dm.c
> > @@ -647,6 +647,15 @@ static char **
> > libxl__build_device_model_args_new(libxl__gc *gc,
> >              flexarray_append(dm_args, "-drive");
> >              flexarray_append(dm_args, drive);
> >          }
> > +
> > +        switch (b_info->u.hvm.device_model_pvdevice) {
> > +        case LIBXL_DEVICE_MODEL_PVDEVICE_XENSERVER:
> > +            flexarray_append(dm_args, "-device");
> > +            flexarray_append(dm_args, "xen-pvdevice,device-id=0xc000");
> > +            break;
> > +        default:
> > +            break;
> > +        }
> >      }
> >      flexarray_append(dm_args, NULL);
> >      return (char **) flexarray_contents(dm_args);
> > diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> > index d218a2d..7165139 100644
> > --- a/tools/libxl/libxl_types.idl
> > +++ b/tools/libxl/libxl_types.idl
> > @@ -132,6 +132,10 @@ libxl_vga_interface_type =
> > Enumeration("vga_interface_type", [
> >      (2, "STD"),
> >      ], init_val = 0)
> > 
> > +libxl_device_model_pvdevice = Enumeration("device_model_pvdevice", [
> > +    (0, "NONE"),
> > +    (1, "XENSERVER"),
> > +    ])
> >  #
> >  # Complex libxl types
> >  #
> > @@ -332,6 +336,7 @@ libxl_domain_build_info =
> > Struct("domain_build_info",[
> >                                         ("soundhw",          string),
> >                                         ("xen_platform_pci", libxl_defbool),
> >                                         ("usbdevice_list",   libxl_string_list),
> > +                                       ("device_model_pvdevice",
> > libxl_device_model_pvdevice),
> >                                         ])),
> >                   ("pv", Struct(None, [("kernel", string),
> >                                        ("slack_memkb", MemKB),
> > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> > index 8a478ba..cfaa54e 100644
> > --- a/tools/libxl/xl_cmdimpl.c
> > +++ b/tools/libxl/xl_cmdimpl.c
> > @@ -1526,6 +1526,20 @@ skip_vfb:
> >              exit (1);
> > 
> >          }
> > +
> > +        if (!xlu_cfg_get_string (config, "device_model_pvdevice", &buf, 0)) {
> > +            libxl_device_model_pvdevice d;
> > +
> > +            e = libxl_device_model_pvdevice_from_string(buf, &d);
> > +            if (e) {
> > +                fprintf(stderr,
> > +                        "xl: unknown device_model_pvdevice '%s'\n",
> > +                        buf);
> > +                exit(-ERROR_FAIL);
> > +            }
> > +
> > +            b_info->u.hvm.device_model_pvdevice = d;
> > +        }
> >      }
> > 
> >      xlu_cfg_destroy(config);
> > --
> > 1.7.10.4
> 

Re: [PATCH] Add device_model_pvdevice parameter for HVM guests

[Matt Wilson]

On Thu, Jul 04, 2013 at 11:50:06AM +0100, Paul Durrant wrote:
> The parameter determines which, if any, xen-pvdevice is specified on the
> QEMU command line. The default value is 'none' which means no argument will
> be passed. A value of 'xenserver' specifies a xen-pvdevice with device-id
> 0xc000 (the initial value in the xenserver namespace - see
> docs/misc/pci-device-reservations.txt).

Why bake this into the toolset API? I don't like the idea of a toolset
API change every time a new device ID is introduced by a PV driver.

Can this be a free form text parameter instead?

--msw

> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> Cc: Stefano Stabellini <stefano.stabellini@citrix.com>
> ---
>  docs/man/xl.cfg.pod.5       |   22 ++++++++++++++++++++++
>  tools/libxl/libxl_dm.c      |    9 +++++++++
>  tools/libxl/libxl_types.idl |    5 +++++
>  tools/libxl/xl_cmdimpl.c    |   14 ++++++++++++++
>  4 files changed, 50 insertions(+)
> 
> diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
> index b7d64a6..2220bb6 100644
> --- a/docs/man/xl.cfg.pod.5
> +++ b/docs/man/xl.cfg.pod.5
> @@ -1214,6 +1214,28 @@ you have selected.
>  
>  Assign an XSM security label to the device-model stubdomain.
>  
> +=item B<device_model_pvdevice="PVDEVICE">
> +
> +Selects which variant of the xen-pvdevice should be used for this
> +guest. Valid values are:
> +
> +=over 4
> +
> +=item B<none>
> +
> +The xen-pvdevice should be omitted. This is the default.
> +
> +=item B<xenserver>
> +
> +The xenserver variant of the xen-pvdevice will be specified, enabling
> +the use of XenServer PV drivers in the guest.
> +
> +=back
> +
> +This parameter only takes effect when device_model_version=qemu-xen.
> +
> +=back
> +
>  =item B<device_model_args=[ "ARG", "ARG", ...]>
>  
>  Pass additional arbitrary options on the device-model command
> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index ac1f90e..2924298 100644
> --- a/tools/libxl/libxl_dm.c
> +++ b/tools/libxl/libxl_dm.c
> @@ -647,6 +647,15 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc,
>              flexarray_append(dm_args, "-drive");
>              flexarray_append(dm_args, drive);
>          }
> +
> +        switch (b_info->u.hvm.device_model_pvdevice) {
> +        case LIBXL_DEVICE_MODEL_PVDEVICE_XENSERVER:
> +            flexarray_append(dm_args, "-device");
> +            flexarray_append(dm_args, "xen-pvdevice,device-id=0xc000");
> +            break;
> +        default:
> +            break;
> +        }
>      }
>      flexarray_append(dm_args, NULL);
>      return (char **) flexarray_contents(dm_args);
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index d218a2d..7165139 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -132,6 +132,10 @@ libxl_vga_interface_type = Enumeration("vga_interface_type", [
>      (2, "STD"),
>      ], init_val = 0)
>  
> +libxl_device_model_pvdevice = Enumeration("device_model_pvdevice", [
> +    (0, "NONE"),
> +    (1, "XENSERVER"),
> +    ])
>  #
>  # Complex libxl types
>  #
> @@ -332,6 +336,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
>                                         ("soundhw",          string),
>                                         ("xen_platform_pci", libxl_defbool),
>                                         ("usbdevice_list",   libxl_string_list),
> +                                       ("device_model_pvdevice", libxl_device_model_pvdevice),
>                                         ])),
>                   ("pv", Struct(None, [("kernel", string),
>                                        ("slack_memkb", MemKB),
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 8a478ba..cfaa54e 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -1526,6 +1526,20 @@ skip_vfb:
>              exit (1);
>  
>          }
> +
> +        if (!xlu_cfg_get_string (config, "device_model_pvdevice", &buf, 0)) {
> +            libxl_device_model_pvdevice d;
> +
> +            e = libxl_device_model_pvdevice_from_string(buf, &d);
> +            if (e) {
> +                fprintf(stderr,
> +                        "xl: unknown device_model_pvdevice '%s'\n",
> +                        buf);
> +                exit(-ERROR_FAIL);
> +            }
> +                
> +            b_info->u.hvm.device_model_pvdevice = d;
> +        }
>      }
>  
>      xlu_cfg_destroy(config);

Re: [PATCH] Add device_model_pvdevice parameter for HVM guests

[Stefano Stabellini]

On Thu, 11 Jul 2013, Matt Wilson wrote:
> On Thu, Jul 04, 2013 at 11:50:06AM +0100, Paul Durrant wrote:
> > The parameter determines which, if any, xen-pvdevice is specified on the
> > QEMU command line. The default value is 'none' which means no argument will
> > be passed. A value of 'xenserver' specifies a xen-pvdevice with device-id
> > 0xc000 (the initial value in the xenserver namespace - see
> > docs/misc/pci-device-reservations.txt).
> 
> Why bake this into the toolset API? I don't like the idea of a toolset
> API change every time a new device ID is introduced by a PV driver.
> 
> Can this be a free form text parameter instead?

Think of the users: although I am not against having a device ID based
interface, we should also expose something easier to use like a label
(like this patch does).

> 
> > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> > Cc: Stefano Stabellini <stefano.stabellini@citrix.com>
> > ---
> >  docs/man/xl.cfg.pod.5       |   22 ++++++++++++++++++++++
> >  tools/libxl/libxl_dm.c      |    9 +++++++++
> >  tools/libxl/libxl_types.idl |    5 +++++
> >  tools/libxl/xl_cmdimpl.c    |   14 ++++++++++++++
> >  4 files changed, 50 insertions(+)
> > 
> > diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
> > index b7d64a6..2220bb6 100644
> > --- a/docs/man/xl.cfg.pod.5
> > +++ b/docs/man/xl.cfg.pod.5
> > @@ -1214,6 +1214,28 @@ you have selected.
> >  
> >  Assign an XSM security label to the device-model stubdomain.
> >  
> > +=item B<device_model_pvdevice="PVDEVICE">
> > +
> > +Selects which variant of the xen-pvdevice should be used for this
> > +guest. Valid values are:
> > +
> > +=over 4
> > +
> > +=item B<none>
> > +
> > +The xen-pvdevice should be omitted. This is the default.
> > +
> > +=item B<xenserver>
> > +
> > +The xenserver variant of the xen-pvdevice will be specified, enabling
> > +the use of XenServer PV drivers in the guest.
> > +
> > +=back
> > +
> > +This parameter only takes effect when device_model_version=qemu-xen.
> > +
> > +=back
> > +
> >  =item B<device_model_args=[ "ARG", "ARG", ...]>
> >  
> >  Pass additional arbitrary options on the device-model command
> > diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> > index ac1f90e..2924298 100644
> > --- a/tools/libxl/libxl_dm.c
> > +++ b/tools/libxl/libxl_dm.c
> > @@ -647,6 +647,15 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc,
> >              flexarray_append(dm_args, "-drive");
> >              flexarray_append(dm_args, drive);
> >          }
> > +
> > +        switch (b_info->u.hvm.device_model_pvdevice) {
> > +        case LIBXL_DEVICE_MODEL_PVDEVICE_XENSERVER:
> > +            flexarray_append(dm_args, "-device");
> > +            flexarray_append(dm_args, "xen-pvdevice,device-id=0xc000");
> > +            break;
> > +        default:
> > +            break;
> > +        }
> >      }
> >      flexarray_append(dm_args, NULL);
> >      return (char **) flexarray_contents(dm_args);
> > diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> > index d218a2d..7165139 100644
> > --- a/tools/libxl/libxl_types.idl
> > +++ b/tools/libxl/libxl_types.idl
> > @@ -132,6 +132,10 @@ libxl_vga_interface_type = Enumeration("vga_interface_type", [
> >      (2, "STD"),
> >      ], init_val = 0)
> >  
> > +libxl_device_model_pvdevice = Enumeration("device_model_pvdevice", [
> > +    (0, "NONE"),
> > +    (1, "XENSERVER"),
> > +    ])
> >  #
> >  # Complex libxl types
> >  #
> > @@ -332,6 +336,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
> >                                         ("soundhw",          string),
> >                                         ("xen_platform_pci", libxl_defbool),
> >                                         ("usbdevice_list",   libxl_string_list),
> > +                                       ("device_model_pvdevice", libxl_device_model_pvdevice),
> >                                         ])),
> >                   ("pv", Struct(None, [("kernel", string),
> >                                        ("slack_memkb", MemKB),
> > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> > index 8a478ba..cfaa54e 100644
> > --- a/tools/libxl/xl_cmdimpl.c
> > +++ b/tools/libxl/xl_cmdimpl.c
> > @@ -1526,6 +1526,20 @@ skip_vfb:
> >              exit (1);
> >  
> >          }
> > +
> > +        if (!xlu_cfg_get_string (config, "device_model_pvdevice", &buf, 0)) {
> > +            libxl_device_model_pvdevice d;
> > +
> > +            e = libxl_device_model_pvdevice_from_string(buf, &d);
> > +            if (e) {
> > +                fprintf(stderr,
> > +                        "xl: unknown device_model_pvdevice '%s'\n",
> > +                        buf);
> > +                exit(-ERROR_FAIL);
> > +            }
> > +                
> > +            b_info->u.hvm.device_model_pvdevice = d;
> > +        }
> >      }
> >  
> >      xlu_cfg_destroy(config);
> 

Re: [PATCH] Add device_model_pvdevice parameter for HVM guests

[Paul Durrant]

> -----Original Message-----
> From: Matt Wilson [mailto:msw@amazon.com]
> Sent: 11 July 2013 18:58
> To: Paul Durrant
> Cc: xen-devel@lists.xen.org; Stefano Stabellini
> Subject: Re: [Xen-devel] [PATCH] Add device_model_pvdevice parameter
> for HVM guests
> 
> On Thu, Jul 04, 2013 at 11:50:06AM +0100, Paul Durrant wrote:
> > The parameter determines which, if any, xen-pvdevice is specified on the
> > QEMU command line. The default value is 'none' which means no
> argument will
> > be passed. A value of 'xenserver' specifies a xen-pvdevice with device-id
> > 0xc000 (the initial value in the xenserver namespace - see
> > docs/misc/pci-device-reservations.txt).
> 
> Why bake this into the toolset API? I don't like the idea of a toolset
> API change every time a new device ID is introduced by a PV driver.
> 
> Can this be a free form text parameter instead?
> 

>From a user perspective having a documented list makes life easier. If you want to choose arbitrary values (for development purposes perhaps) then you do still have the option of using the device_model_args[_hvm] parameter in your xl.cfg to specify whatever -device argument you want.

  Paul

Re: [PATCH] Add device_model_pvdevice parameter for HVM guests

[Matt Wilson]

On Fri, Jul 12, 2013 at 07:57:09AM +0000, Paul Durrant wrote:
> > -----Original Message-----
> > From: Matt Wilson [mailto:msw@amazon.com]
> > Sent: 11 July 2013 18:58
> > To: Paul Durrant
> > Cc: xen-devel@lists.xen.org; Stefano Stabellini
> > Subject: Re: [Xen-devel] [PATCH] Add device_model_pvdevice parameter
> > for HVM guests
> > 
> > On Thu, Jul 04, 2013 at 11:50:06AM +0100, Paul Durrant wrote:
> > > The parameter determines which, if any, xen-pvdevice is specified on the
> > > QEMU command line. The default value is 'none' which means no
> > argument will
> > > be passed. A value of 'xenserver' specifies a xen-pvdevice with device-id
> > > 0xc000 (the initial value in the xenserver namespace - see
> > > docs/misc/pci-device-reservations.txt).
> > 
> > Why bake this into the toolset API? I don't like the idea of a toolset
> > API change every time a new device ID is introduced by a PV driver.
> > 
> > Can this be a free form text parameter instead?
> > 
> 
> From a user perspective having a documented list makes life
> easier. If you want to choose arbitrary values (for development
> purposes perhaps) then you do still have the option of using the
> device_model_args[_hvm] parameter in your xl.cfg to specify whatever
> -device argument you want.

I just want to make sure that new versions of PV drivers can be used
without a toolstack update (so long as they're backwards compatible
with the older Xen and driver domain). This seems possible, so from my
perspective:

Acked-by: Matt Wilson <msw@amazon.com>

Re: [PATCH] Add device_model_pvdevice parameter for HVM guests

[Paul Durrant]

> -----Original Message-----
> From: Paul Durrant [mailto:paul.durrant@citrix.com]
> Sent: 04 July 2013 11:50
> To: xen-devel@lists.xen.org
> Cc: Paul Durrant; Stefano Stabellini
> Subject: [PATCH] Add device_model_pvdevice parameter for HVM guests
> 
> The parameter determines which, if any, xen-pvdevice is specified on the
> QEMU command line. The default value is 'none' which means no argument
> will
> be passed. A value of 'xenserver' specifies a xen-pvdevice with device-id
> 0xc000 (the initial value in the xenserver namespace - see
> docs/misc/pci-device-reservations.txt).
> 
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> Cc: Stefano Stabellini <stefano.stabellini@citrix.com>
> ---

Re-ping? Stefano tells me he's waiting for an ack from either IanC or IanJ on this one before dealing with the QEMU end of things. It's aleady been acked by Matt Wilson.

  Paul

>  docs/man/xl.cfg.pod.5       |   22 ++++++++++++++++++++++
>  tools/libxl/libxl_dm.c      |    9 +++++++++
>  tools/libxl/libxl_types.idl |    5 +++++
>  tools/libxl/xl_cmdimpl.c    |   14 ++++++++++++++
>  4 files changed, 50 insertions(+)
> 
> diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
> index b7d64a6..2220bb6 100644
> --- a/docs/man/xl.cfg.pod.5
> +++ b/docs/man/xl.cfg.pod.5
> @@ -1214,6 +1214,28 @@ you have selected.
> 
>  Assign an XSM security label to the device-model stubdomain.
> 
> +=item B<device_model_pvdevice="PVDEVICE">
> +
> +Selects which variant of the xen-pvdevice should be used for this
> +guest. Valid values are:
> +
> +=over 4
> +
> +=item B<none>
> +
> +The xen-pvdevice should be omitted. This is the default.
> +
> +=item B<xenserver>
> +
> +The xenserver variant of the xen-pvdevice will be specified, enabling
> +the use of XenServer PV drivers in the guest.
> +
> +=back
> +
> +This parameter only takes effect when device_model_version=qemu-xen.
> +
> +=back
> +
>  =item B<device_model_args=[ "ARG", "ARG", ...]>
> 
>  Pass additional arbitrary options on the device-model command
> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index ac1f90e..2924298 100644
> --- a/tools/libxl/libxl_dm.c
> +++ b/tools/libxl/libxl_dm.c
> @@ -647,6 +647,15 @@ static char **
> libxl__build_device_model_args_new(libxl__gc *gc,
>              flexarray_append(dm_args, "-drive");
>              flexarray_append(dm_args, drive);
>          }
> +
> +        switch (b_info->u.hvm.device_model_pvdevice) {
> +        case LIBXL_DEVICE_MODEL_PVDEVICE_XENSERVER:
> +            flexarray_append(dm_args, "-device");
> +            flexarray_append(dm_args, "xen-pvdevice,device-id=0xc000");
> +            break;
> +        default:
> +            break;
> +        }
>      }
>      flexarray_append(dm_args, NULL);
>      return (char **) flexarray_contents(dm_args);
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index d218a2d..7165139 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -132,6 +132,10 @@ libxl_vga_interface_type =
> Enumeration("vga_interface_type", [
>      (2, "STD"),
>      ], init_val = 0)
> 
> +libxl_device_model_pvdevice = Enumeration("device_model_pvdevice", [
> +    (0, "NONE"),
> +    (1, "XENSERVER"),
> +    ])
>  #
>  # Complex libxl types
>  #
> @@ -332,6 +336,7 @@ libxl_domain_build_info =
> Struct("domain_build_info",[
>                                         ("soundhw",          string),
>                                         ("xen_platform_pci", libxl_defbool),
>                                         ("usbdevice_list",   libxl_string_list),
> +                                       ("device_model_pvdevice",
> libxl_device_model_pvdevice),
>                                         ])),
>                   ("pv", Struct(None, [("kernel", string),
>                                        ("slack_memkb", MemKB),
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 8a478ba..cfaa54e 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -1526,6 +1526,20 @@ skip_vfb:
>              exit (1);
> 
>          }
> +
> +        if (!xlu_cfg_get_string (config, "device_model_pvdevice", &buf, 0)) {
> +            libxl_device_model_pvdevice d;
> +
> +            e = libxl_device_model_pvdevice_from_string(buf, &d);
> +            if (e) {
> +                fprintf(stderr,
> +                        "xl: unknown device_model_pvdevice '%s'\n",
> +                        buf);
> +                exit(-ERROR_FAIL);
> +            }
> +
> +            b_info->u.hvm.device_model_pvdevice = d;
> +        }
>      }
> 
>      xlu_cfg_destroy(config);
> --
> 1.7.10.4

Re: [PATCH] Add device_model_pvdevice parameter for HVM guests

[Ian Campbell]

On Thu, 2013-07-18 at 11:54 +0100, Paul Durrant wrote:
> > -----Original Message-----
> > From: Paul Durrant [mailto:paul.durrant@citrix.com]
> > Sent: 04 July 2013 11:50
> > To: xen-devel@lists.xen.org
> > Cc: Paul Durrant; Stefano Stabellini
> > Subject: [PATCH] Add device_model_pvdevice parameter for HVM guests
> > 
> > The parameter determines which, if any, xen-pvdevice is specified on the
> > QEMU command line. The default value is 'none' which means no argument
> > will
> > be passed. A value of 'xenserver' specifies a xen-pvdevice with device-id
> > 0xc000 (the initial value in the xenserver namespace - see
> > docs/misc/pci-device-reservations.txt).
> > 
> > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> > Cc: Stefano Stabellini <stefano.stabellini@citrix.com>
> > ---
> 
> Re-ping? Stefano tells me he's waiting for an ack from either IanC or
> IanJ on this one before dealing with the QEMU end of things. It's
> aleady been acked by Matt Wilson.

I was waiting for the qemu side to get settled...

> 
>   Paul
> 
> >  docs/man/xl.cfg.pod.5       |   22 ++++++++++++++++++++++
> >  tools/libxl/libxl_dm.c      |    9 +++++++++
> >  tools/libxl/libxl_types.idl |    5 +++++
> >  tools/libxl/xl_cmdimpl.c    |   14 ++++++++++++++

Also needs to patch libxl.h to have an appropriate LIBXL_HAVE define
(there is a comment describing this and a few example already in the
file).

> > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> > index 8a478ba..cfaa54e 100644
> > --- a/tools/libxl/xl_cmdimpl.c
> > +++ b/tools/libxl/xl_cmdimpl.c
> > @@ -1526,6 +1526,20 @@ skip_vfb:
> >              exit (1);
> > 
> >          }
> > +
> > +        if (!xlu_cfg_get_string (config, "device_model_pvdevice", &buf, 0)) {
> > +            libxl_device_model_pvdevice d;
> > +
> > +            e = libxl_device_model_pvdevice_from_string(buf, &d);
> > +            if (e) {
> > +                fprintf(stderr,
> > +                        "xl: unknown device_model_pvdevice '%s'\n",
> > +                        buf);
> > +                exit(-ERROR_FAIL);
> > +            }
> > +
> > +            b_info->u.hvm.device_model_pvdevice = d;

You could do away with d and just pass the real thing to from_string.

> > +        }
> >      }
> > 
> >      xlu_cfg_destroy(config);
> > --
> > 1.7.10.4
> 

Re: [PATCH] Add device_model_pvdevice parameter for HVM guests

[Ian Jackson]

Paul Durrant writes ("RE: [PATCH] Add device_model_pvdevice parameter for HVM guests"):
> > -----Original Message-----
> > From: Paul Durrant [mailto:paul.durrant@citrix.com]
> > Sent: 04 July 2013 11:50
> > To: xen-devel@lists.xen.org
> > Cc: Paul Durrant; Stefano Stabellini
> > Subject: [PATCH] Add device_model_pvdevice parameter for HVM guests
> > 
> > The parameter determines which, if any, xen-pvdevice is specified on the
> > QEMU command line. The default value is 'none' which means no argument
> > will
> > be passed. A value of 'xenserver' specifies a xen-pvdevice with device-id
> > 0xc000 (the initial value in the xenserver namespace - see
> > docs/misc/pci-device-reservations.txt).
> > 
> > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> > Cc: Stefano Stabellini <stefano.stabellini@citrix.com>
> > ---
> 
> Re-ping? Stefano tells me he's waiting for an ack from either IanC or IanJ on this one before dealing with the QEMU end of things. It's aleady been acked by Matt Wilson.

Thanks for CCing this to me.  I don't seem to have any earlier
versions in my mailbox.

When you say the "xen-pvdevice", you mean what has traditionally been
called the "Xen platform PCI device" ?  Ie, the special PCI device
that HVM guests are provided with to enable them to make hypercalls
etc.

I see that 0xc000 has been reserved for use by Citrix XenServer.  But
that's simply a reservation.  If we're actually going to use this
value in our tree, we need some proper documentation of it.

I'm obviously missing some of the background.

Ian.

Re: [PATCH] Add device_model_pvdevice parameter for HVM guests

[Paul Durrant]

> -----Original Message-----
> From: Ian Campbell
> Sent: 18 July 2013 11:59
> To: Paul Durrant
> Cc: xen-devel@lists.xen.org; Stefano Stabellini; Ian Jackson
> Subject: Re: [PATCH] Add device_model_pvdevice parameter for HVM
> guests
> 
> On Thu, 2013-07-18 at 11:54 +0100, Paul Durrant wrote:
> > > -----Original Message-----
> > > From: Paul Durrant [mailto:paul.durrant@citrix.com]
> > > Sent: 04 July 2013 11:50
> > > To: xen-devel@lists.xen.org
> > > Cc: Paul Durrant; Stefano Stabellini
> > > Subject: [PATCH] Add device_model_pvdevice parameter for HVM
> guests
> > >
> > > The parameter determines which, if any, xen-pvdevice is specified on the
> > > QEMU command line. The default value is 'none' which means no
> argument
> > > will
> > > be passed. A value of 'xenserver' specifies a xen-pvdevice with device-id
> > > 0xc000 (the initial value in the xenserver namespace - see
> > > docs/misc/pci-device-reservations.txt).
> > >
> > > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> > > Cc: Stefano Stabellini <stefano.stabellini@citrix.com>
> > > ---
> >
> > Re-ping? Stefano tells me he's waiting for an ack from either IanC or
> > IanJ on this one before dealing with the QEMU end of things. It's
> > aleady been acked by Matt Wilson.
> 
> I was waiting for the qemu side to get settled...
> 

We seem to have a chicken and egg situation there then. Stefano, can we get the QEMU side of things in place first? That seems like the logical ordering to me.

> >
> >   Paul
> >
> > >  docs/man/xl.cfg.pod.5       |   22 ++++++++++++++++++++++
> > >  tools/libxl/libxl_dm.c      |    9 +++++++++
> > >  tools/libxl/libxl_types.idl |    5 +++++
> > >  tools/libxl/xl_cmdimpl.c    |   14 ++++++++++++++
> 
> Also needs to patch libxl.h to have an appropriate LIBXL_HAVE define
> (there is a comment describing this and a few example already in the
> file).
> 

Ok. I'll have a look at that.

> > > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> > > index 8a478ba..cfaa54e 100644
> > > --- a/tools/libxl/xl_cmdimpl.c
> > > +++ b/tools/libxl/xl_cmdimpl.c
> > > @@ -1526,6 +1526,20 @@ skip_vfb:
> > >              exit (1);
> > >
> > >          }
> > > +
> > > +        if (!xlu_cfg_get_string (config, "device_model_pvdevice", &buf, 0))
> {
> > > +            libxl_device_model_pvdevice d;
> > > +
> > > +            e = libxl_device_model_pvdevice_from_string(buf, &d);
> > > +            if (e) {
> > > +                fprintf(stderr,
> > > +                        "xl: unknown device_model_pvdevice '%s'\n",
> > > +                        buf);
> > > +                exit(-ERROR_FAIL);
> > > +            }
> > > +
> > > +            b_info->u.hvm.device_model_pvdevice = d;
> 
> You could do away with d and just pass the real thing to from_string.
> 

Yeah, I did this for formatting reasons. libxl_device_model_pvdevice_from_string is a very long function name already to adding a lengthy argument name on the end just didn't look nice.

  Paul

> > > +        }
> > >      }
> > >
> > >      xlu_cfg_destroy(config);
> > > --
> > > 1.7.10.4
> >
> 

Re: [PATCH] Add device_model_pvdevice parameter for HVM guests

[Ian Campbell]

On Thu, 2013-07-18 at 12:16 +0100, Paul Durrant wrote:
> > > > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> > > > index 8a478ba..cfaa54e 100644
> > > > --- a/tools/libxl/xl_cmdimpl.c
> > > > +++ b/tools/libxl/xl_cmdimpl.c
> > > > @@ -1526,6 +1526,20 @@ skip_vfb:
> > > >              exit (1);
> > > >
> > > >          }
> > > > +
> > > > +        if (!xlu_cfg_get_string (config, "device_model_pvdevice", &buf, 0))
> > {
> > > > +            libxl_device_model_pvdevice d;
> > > > +
> > > > +            e = libxl_device_model_pvdevice_from_string(buf, &d);
> > > > +            if (e) {
> > > > +                fprintf(stderr,
> > > > +                        "xl: unknown device_model_pvdevice '%s'\n",
> > > > +                        buf);
> > > > +                exit(-ERROR_FAIL);
> > > > +            }
> > > > +
> > > > +            b_info->u.hvm.device_model_pvdevice = d;
> > 
> > You could do away with d and just pass the real thing to from_string.
> > 
> 
> Yeah, I did this for formatting reasons.
> libxl_device_model_pvdevice_from_string is a very long function name
> already to adding a lengthy argument name on the end just didn't look
> nice.

That's OK. FWIW I'd have done
	e = libxl_device_model_pvdevice_from_string(buf,
	        &b_info->xxx);

Up to you though.

Ian.

Re: [PATCH] Add device_model_pvdevice parameter for HVM guests

[Paul Durrant]

> -----Original Message-----
> From: Ian Jackson [mailto:Ian.Jackson@eu.citrix.com]
> Sent: 18 July 2013 12:01
> To: Paul Durrant
> Cc: xen-devel@lists.xen.org; Stefano Stabellini; Ian Campbell
> Subject: RE: [PATCH] Add device_model_pvdevice parameter for HVM
> guests
> 
> Paul Durrant writes ("RE: [PATCH] Add device_model_pvdevice parameter
> for HVM guests"):
> > > -----Original Message-----
> > > From: Paul Durrant [mailto:paul.durrant@citrix.com]
> > > Sent: 04 July 2013 11:50
> > > To: xen-devel@lists.xen.org
> > > Cc: Paul Durrant; Stefano Stabellini
> > > Subject: [PATCH] Add device_model_pvdevice parameter for HVM
> guests
> > >
> > > The parameter determines which, if any, xen-pvdevice is specified on the
> > > QEMU command line. The default value is 'none' which means no
> argument
> > > will
> > > be passed. A value of 'xenserver' specifies a xen-pvdevice with device-id
> > > 0xc000 (the initial value in the xenserver namespace - see
> > > docs/misc/pci-device-reservations.txt).
> > >
> > > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> > > Cc: Stefano Stabellini <stefano.stabellini@citrix.com>
> > > ---
> >
> > Re-ping? Stefano tells me he's waiting for an ack from either IanC or IanJ on
> this one before dealing with the QEMU end of things. It's aleady been acked
> by Matt Wilson.
> 
> Thanks for CCing this to me.  I don't seem to have any earlier
> versions in my mailbox.
> 
> When you say the "xen-pvdevice", you mean what has traditionally been
> called the "Xen platform PCI device" ?  Ie, the special PCI device
> that HVM guests are provided with to enable them to make hypercalls
> etc.
> 

No, this is a new device. See https://lists.gnu.org/archive/html/qemu-devel/2013-07/msg00733.html

> I see that 0xc000 has been reserved for use by Citrix XenServer.  But
> that's simply a reservation.  If we're actually going to use this
> value in our tree, we need some proper documentation of it.
> 
> I'm obviously missing some of the background.
> 

Yep. You should probably read the above thread. In summary though, the xen-pvdevice is a PCI device with configurable vendor, device and revision values, dedicated for use as a binding point for PV drivers in a guest. This patch adds code to libxl to create the 'XenServer' flavour of this device by name rather than number, for user-friendliness. 
I have added some explanation into xl.cfg.pod.5 but not explicitly called out the mapping of name to (vendor,device,revision) there - is that the kind of thing you'd like to see added?

  Paul