Re: [Xen-devel] [PATCH v2] Add device_model_pvdevice parameter for HVM guests

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Ian Campbell
> Sent: 02 August 2013 16:37
> To: Paul Durrant
> Cc: xen-devel@xxxxxxxxxxxxx; Stefano Stabellini; Ian Jackson
> Subject: Re: [PATCH v2] Add device_model_pvdevice parameter for HVM
> guests
> 
> On Fri, 2013-08-02 at 15:49 +0100, Paul Durrant wrote:
> > > -----Original Message-----
> > > From: Ian Campbell
> > > Sent: 02 August 2013 15:32
> > > To: Paul Durrant
> > > Cc: xen-devel@xxxxxxxxxxxxx; Stefano Stabellini; Ian Jackson
> > > Subject: Re: [PATCH v2] Add device_model_pvdevice parameter for HVM
> > > guests
> > >
> > > On Thu, 2013-07-18 at 14:47 +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).
> > > >
> > > > Signed-off-by: Paul Durrant <paul.durrant@xxxxxxxxxx>
> > > > Cc: Stefano Stabellini <stefano.stabellini@xxxxxxxxxx>
> > > > Cc: Ian Campbell <ian.campbell@xxxxxxxxxx>
> > > > Cc: Ian Jackson <ian.jackson@xxxxxxxxxx>
> > > > ---
> > > > v2:
> > > >  - Added LIBXL_HAVE_PVDEVICE to libxl.h
> > > >  - Added more text to xl.cfg manpage to call out the device-id of
> > > >    the xenserver pvdevice and point readers at
> > > >    docs/misc/pci-device-reservations.txt
> > > >
> > > >  docs/man/xl.cfg.pod.5       |   23 +++++++++++++++++++++++
> > > >  tools/libxl/libxl.h         |    8 ++++++++
> > > >  tools/libxl/libxl_dm.c      |    9 +++++++++
> > > >  tools/libxl/libxl_types.idl |    5 +++++
> > > >  tools/libxl/xl_cmdimpl.c    |   14 ++++++++++++++
> > > >  5 files changed, 59 insertions(+)
> > > >
> > > > diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
> > > > index 069b73f..bf6760f 100644
> > > > --- a/docs/man/xl.cfg.pod.5
> > > > +++ b/docs/man/xl.cfg.pod.5
> > > > @@ -1227,6 +1227,29 @@ 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 (device-id=C000) 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.
> > > > +See F<docs/misc/pci-device-reservations.txt> for more information.
> > > > +
> > > > +=back
> > > > +
> > > >  =item B<device_model_args=[ "ARG", "ARG", ...]>
> > > >
> > > >  Pass additional arbitrary options on the device-model command
> > > > diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> > > > index 37e4d82..4170861 100644
> > > > --- a/tools/libxl/libxl.h
> > > > +++ b/tools/libxl/libxl.h
> > > > @@ -82,6 +82,14 @@
> > > >  #define LIBXL_HAVE_DOMAIN_NODEAFFINITY 1
> > > >
> > > >  /*
> > > > + * LIBXL_HAVE_PVDEVICE indicates that the
> libxl_device_model_pvdevice
> > > > + * field is present in the hvm sections of libxl_domain_build_info.
> > > > + * This field tells libxl which flavour of xen-pvdevice to enable in
> > > > + * QEMU.
> > > > + */
> > > > +#define LIBXL_HAVE_PVDEVICE 1
> > >
> > > Can we expand this name a bit, to make it clear what PVDEVICE refers to?
> > > LIBXL_HAVE_PLATFORM_PVDEVICE_TYPES? or _VARIANTS? Bit clumsy?
> > >
> > > While at it I'd be tempted to
> > > s/device_model_pvdevice/platform_device_type/ or something in the
> API
> > > (both the Enum and the field). The related overall boolean control is
> > > xen_platform_pci, so perhaps something derived from that?
> > >
> >
> > This is orthogonal to xen_platform_pci. The exists regardless of the
> > xen_platform_pci boolean (and, in fact, the platform device also
> > exists in upstream qemu regardless of that boolean). Do we really want
> > to start confusing the two things (again)?
> 
> Ah, I mistakenly thought this overrode that one or was conditional on
> it, that explains the NONE in the Enum which I vaguely wondered about
> too.
> 
> I'd like to avoid "device_model" in the name since that is an
> implementation detail and not something which is really relevant to the
> user who is asking for a domain to be created with certain properties.
> 

Ok, I can drop that. I was only following suit. I prefer shorter names anyway 
:-)

> > So, doesn't LIBXL_HAVE_PVDEVICE cover it? The intention is there's
> > only ever going to be one of them.
> 
> PVDEVICE is a very broad term, does it mean PVNIC, PVCONSOLE, PVDISK
> etc? How does it relate to a PVDOMAIN etc. I know we both know the
> answer to what this is but the API should be less ambiguous.
> 

How about VENDOR_DEVICE? That probably captures the intention (and makes more 
sense when you consider the only available value apart fromn 'none' is 
'citrix').

  Paul
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel