* Re: [PATCH] Add device_model_pvdevice parameter for HVM guests
2013-07-04 10:50 [PATCH] Add device_model_pvdevice parameter for HVM guests Paul Durrant
@ 2013-07-11 8:22 ` Paul Durrant
2013-07-11 11:34 ` Stefano Stabellini
2013-07-11 17:58 ` Matt Wilson
2013-07-18 10:54 ` Paul Durrant
2 siblings, 1 reply; 13+ messages in thread
From: Paul Durrant @ 2013-07-11 8:22 UTC (permalink / raw)
To: xen-devel; +Cc: Paul Durrant, Stefano Stabellini
> -----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
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] Add device_model_pvdevice parameter for HVM guests
2013-07-11 8:22 ` Paul Durrant
@ 2013-07-11 11:34 ` Stefano Stabellini
0 siblings, 0 replies; 13+ messages in thread
From: Stefano Stabellini @ 2013-07-11 11:34 UTC (permalink / raw)
To: Paul Durrant; +Cc: Stefano Stabellini, Ian Campbell, xen-devel
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
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] Add device_model_pvdevice parameter for HVM guests
2013-07-04 10:50 [PATCH] Add device_model_pvdevice parameter for HVM guests Paul Durrant
2013-07-11 8:22 ` Paul Durrant
@ 2013-07-11 17:58 ` Matt Wilson
2013-07-11 21:29 ` Stefano Stabellini
2013-07-12 7:57 ` Paul Durrant
2013-07-18 10:54 ` Paul Durrant
2 siblings, 2 replies; 13+ messages in thread
From: Matt Wilson @ 2013-07-11 17:58 UTC (permalink / raw)
To: Paul Durrant; +Cc: Stefano Stabellini, xen-devel
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);
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] Add device_model_pvdevice parameter for HVM guests
2013-07-11 17:58 ` Matt Wilson
@ 2013-07-11 21:29 ` Stefano Stabellini
2013-07-12 7:57 ` Paul Durrant
1 sibling, 0 replies; 13+ messages in thread
From: Stefano Stabellini @ 2013-07-11 21:29 UTC (permalink / raw)
To: Matt Wilson; +Cc: Paul Durrant, Stefano Stabellini, xen-devel
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);
>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] Add device_model_pvdevice parameter for HVM guests
2013-07-11 17:58 ` Matt Wilson
2013-07-11 21:29 ` Stefano Stabellini
@ 2013-07-12 7:57 ` Paul Durrant
2013-07-12 22:34 ` Matt Wilson
1 sibling, 1 reply; 13+ messages in thread
From: Paul Durrant @ 2013-07-12 7:57 UTC (permalink / raw)
To: Matt Wilson; +Cc: Stefano Stabellini, xen-devel
> -----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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] Add device_model_pvdevice parameter for HVM guests
2013-07-12 7:57 ` Paul Durrant
@ 2013-07-12 22:34 ` Matt Wilson
0 siblings, 0 replies; 13+ messages in thread
From: Matt Wilson @ 2013-07-12 22:34 UTC (permalink / raw)
To: Paul Durrant; +Cc: Stefano Stabellini, xen-devel
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>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] Add device_model_pvdevice parameter for HVM guests
2013-07-04 10:50 [PATCH] Add device_model_pvdevice parameter for HVM guests Paul Durrant
2013-07-11 8:22 ` Paul Durrant
2013-07-11 17:58 ` Matt Wilson
@ 2013-07-18 10:54 ` Paul Durrant
2013-07-18 10:58 ` Ian Campbell
2013-07-18 11:00 ` Ian Jackson
2 siblings, 2 replies; 13+ messages in thread
From: Paul Durrant @ 2013-07-18 10:54 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson, Ian Campbell, Stefano Stabellini
> -----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
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] Add device_model_pvdevice parameter for HVM guests
2013-07-18 10:54 ` Paul Durrant
@ 2013-07-18 10:58 ` Ian Campbell
2013-07-18 11:16 ` Paul Durrant
2013-07-18 11:00 ` Ian Jackson
1 sibling, 1 reply; 13+ messages in thread
From: Ian Campbell @ 2013-07-18 10:58 UTC (permalink / raw)
To: Paul Durrant; +Cc: Stefano Stabellini, Ian Jackson, xen-devel
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
>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] Add device_model_pvdevice parameter for HVM guests
2013-07-18 10:58 ` Ian Campbell
@ 2013-07-18 11:16 ` Paul Durrant
2013-07-18 11:23 ` Ian Campbell
0 siblings, 1 reply; 13+ messages in thread
From: Paul Durrant @ 2013-07-18 11:16 UTC (permalink / raw)
To: Ian Campbell; +Cc: Stefano Stabellini, Ian Jackson, xen-devel
> -----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
> >
>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] Add device_model_pvdevice parameter for HVM guests
2013-07-18 11:16 ` Paul Durrant
@ 2013-07-18 11:23 ` Ian Campbell
0 siblings, 0 replies; 13+ messages in thread
From: Ian Campbell @ 2013-07-18 11:23 UTC (permalink / raw)
To: Paul Durrant; +Cc: Stefano Stabellini, Ian Jackson, xen-devel
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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] Add device_model_pvdevice parameter for HVM guests
2013-07-18 10:54 ` Paul Durrant
2013-07-18 10:58 ` Ian Campbell
@ 2013-07-18 11:00 ` Ian Jackson
2013-07-18 11:35 ` Paul Durrant
1 sibling, 1 reply; 13+ messages in thread
From: Ian Jackson @ 2013-07-18 11:00 UTC (permalink / raw)
To: Paul Durrant; +Cc: Stefano Stabellini, Ian Campbell, xen-devel
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.
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] Add device_model_pvdevice parameter for HVM guests
2013-07-18 11:00 ` Ian Jackson
@ 2013-07-18 11:35 ` Paul Durrant
0 siblings, 0 replies; 13+ messages in thread
From: Paul Durrant @ 2013-07-18 11:35 UTC (permalink / raw)
To: Ian Jackson; +Cc: Stefano Stabellini, Ian Campbell, xen-devel
> -----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
^ permalink raw reply [flat|nested] 13+ messages in thread