Re: [Xen-devel] [PATCH] public/sysctl: Clarifications to XEN_SYSCTL_PHYSCAP_hvm_directio

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

On Fri, 2015-12-11 at 03:33 -0700, Jan Beulich wrote:
> > > > On 11.12.15 at 11:20, <andrew.cooper3@xxxxxxxxxx> wrote:
> > On 11/12/15 07:53, Jan Beulich wrote:
> > > > > > On 10.12.15 at 21:07, <andrew.cooper3@xxxxxxxxxx> wrote:
> > > > On 01/12/15 13:35, Jan Beulich wrote:
> > > > > > > > On 01.12.15 at 12:37, <andrew.cooper3@xxxxxxxxxx> wrote:
> > > > > > --- a/xen/include/public/sysctl.h
> > > > > > +++ b/xen/include/public/sysctl.h
> > > > > > @@ -89,7 +89,14 @@
> > > > > > DEFINE_XEN_GUEST_HANDLE(xen_sysctl_tbuf_op_t);
> > > > > > Â /* (x86) The platform supports HVM guests. */
> > > > > > Â#define _XEN_SYSCTL_PHYSCAP_hvmÂÂÂÂÂÂÂÂÂÂ0
> > > > > > Â#define
> > > > > > XEN_SYSCTL_PHYSCAP_hvmÂÂÂÂÂÂÂÂÂÂÂ(1u<<_XEN_SYSCTL_PHYSCAP_hvm)
> > > > > > - /* (x86) The platform supports HVM-guest direct access to I/O
> > > > > > devices. */
> > > > > > + /*
> > > > > > +ÂÂ* (x86) The platform supports guest direct access to I/O
> > > > > > devices.
> > > > > > +ÂÂ*
> > > > > > +ÂÂ* Note that this parameter has been misnamed since its
> > > > > > introduction, and is
> > > > > > +ÂÂ* now too baked into APIs and ABIs to change.ÂÂDespite the
> > > > > > "hvm" in its
> > > > > What do you mean with "too baked into ..."? This is sysctl, which
> > > > > can
> > > > > be changed, and I found just two uses (one in the hypervisor, the
> > > > > other in libxl), so changing the use sites wouldn't seem all that
> > > > > problematic (in the worst case we could also keep to current name
> > > > > behind a __XEN_INTERFACE_VERSION__ conditional).
> > > > It is libxl which is the problem.ÂÂGiven its stable API,
> > > > libxl_physinfo.cap_hvm_directio can't be changed.
> > > But that's only derived from the sysctl interface, i.e. changing the
> > > name in the public headers won't - unless I'm overlooking something -
> > > have any effect on the libxl interface. It's the libxl implementation
> > > which would then need to explain (for itself) that the name doesn't
> > > reflect the function.
> > 
> > This is all true, but it is better to have it consistent everywhere
> > rather than to change just half of it.
> 
> I disagree (we should eliminate as much confusion as possible), but
> I'm fine to be overruled by other REST maintainers.

I can see both sides of this coin.

If it's just libxl being a pain then we could introduce a new (second)
field and deprecate the old one (while keeping it for compatibility) or
maybe (but probably not) do something withÂLIBXL_API_VERSION to have a
single field with a dual personality.

We also have the (rather more extreme) of deprecating libxl_get_physinfo
entirely and adding a new more general interface which doesn't encode
hypervisor specifics quite so hardcodedly. I've no idea what such an
interface would look like though, and it would seem like overkill.

Ian.

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