[PATCH] x86/sysctl: Hide hvm_directio if hvm is not available

[PATCH] x86/sysctl: Hide hvm_directio if hvm is not available

[Andrew Cooper]

If HVM support unavailable, advertising hvm_directio is misleading.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
---
 xen/arch/x86/sysctl.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/sysctl.c b/xen/arch/x86/sysctl.c
index 58cbd70..c440c93 100644
--- a/xen/arch/x86/sysctl.c
+++ b/xen/arch/x86/sysctl.c
@@ -77,9 +77,11 @@ void arch_do_physinfo(xen_sysctl_physinfo_t *pi)
     memcpy(pi->hw_cap, boot_cpu_data.x86_capability,
            min(sizeof(pi->hw_cap), sizeof(boot_cpu_data.x86_capability)));
     if ( hvm_enabled )
+    {
         pi->capabilities |= XEN_SYSCTL_PHYSCAP_hvm;
-    if ( iommu_enabled )
-        pi->capabilities |= XEN_SYSCTL_PHYSCAP_hvm_directio;
+        if ( iommu_enabled )
+            pi->capabilities |= XEN_SYSCTL_PHYSCAP_hvm_directio;
+    }
 }
 
 long arch_do_sysctl(
-- 
2.1.4

Re: [PATCH] x86/sysctl: Hide hvm_directio if hvm is not available

[Jan Beulich]

>>> On 26.11.15 at 18:02, <andrew.cooper3@citrix.com> wrote:
> If HVM support unavailable, advertising hvm_directio is misleading.

Well, perhaps XEN_SYSCTL_PHYSCAP_hvm_directio is the wrong
name for the flag? How would the tool stack know the IOMMU is
available for PV pass-through with that flag hidden?

Jan

[PATCH] public/sysctl: Clarifications to XEN_SYSCTL_PHYSCAP_hvm_directio

[Andrew Cooper]

Specifically that it indicates passthrough support for PV guests as well

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Tim Deegan <tim@xen.org>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 xen/include/public/sysctl.h | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
index 96680eb..cbbeecd 100644
--- 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
+  * name, it simply indicates the presence of an enabled IOMMU, allowing for
+  * passthrough of devices to guests, including PV guests.
+  */
 #define _XEN_SYSCTL_PHYSCAP_hvm_directio 1
 #define XEN_SYSCTL_PHYSCAP_hvm_directio  (1u<<_XEN_SYSCTL_PHYSCAP_hvm_directio)
 struct xen_sysctl_physinfo {
-- 
2.1.4

Re: [PATCH] public/sysctl: Clarifications to XEN_SYSCTL_PHYSCAP_hvm_directio

[Jan Beulich]

>>> On 01.12.15 at 12:37, <andrew.cooper3@citrix.com> 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).

Jan

> +  * name, it simply indicates the presence of an enabled IOMMU, allowing for
> +  * passthrough of devices to guests, including PV guests.
> +  */
>  #define _XEN_SYSCTL_PHYSCAP_hvm_directio 1
>  #define XEN_SYSCTL_PHYSCAP_hvm_directio  (1u<<_XEN_SYSCTL_PHYSCAP_hvm_directio)
>  struct xen_sysctl_physinfo {

Re: [PATCH] public/sysctl: Clarifications to XEN_SYSCTL_PHYSCAP_hvm_directio

[Andrew Cooper]

On 01/12/15 13:35, Jan Beulich wrote:
>>>> On 01.12.15 at 12:37, <andrew.cooper3@citrix.com> 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.

~Andrew

Re: [PATCH] public/sysctl: Clarifications to XEN_SYSCTL_PHYSCAP_hvm_directio

[Jan Beulich]

>>> On 10.12.15 at 21:07, <andrew.cooper3@citrix.com> wrote:
> On 01/12/15 13:35, Jan Beulich wrote:
>>>>> On 01.12.15 at 12:37, <andrew.cooper3@citrix.com> 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.

Jan

Re: [PATCH] public/sysctl: Clarifications to XEN_SYSCTL_PHYSCAP_hvm_directio

[Andrew Cooper]

On 11/12/15 07:53, Jan Beulich wrote:
>>>> On 10.12.15 at 21:07, <andrew.cooper3@citrix.com> wrote:
>> On 01/12/15 13:35, Jan Beulich wrote:
>>>>>> On 01.12.15 at 12:37, <andrew.cooper3@citrix.com> 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.

~Andrew

Re: [PATCH] public/sysctl: Clarifications to XEN_SYSCTL_PHYSCAP_hvm_directio

[Jan Beulich]

>>> On 11.12.15 at 11:20, <andrew.cooper3@citrix.com> wrote:
> On 11/12/15 07:53, Jan Beulich wrote:
>>>>> On 10.12.15 at 21:07, <andrew.cooper3@citrix.com> wrote:
>>> On 01/12/15 13:35, Jan Beulich wrote:
>>>>>>> On 01.12.15 at 12:37, <andrew.cooper3@citrix.com> 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.

Jan

Re: [PATCH] public/sysctl: Clarifications to XEN_SYSCTL_PHYSCAP_hvm_directio

[Ian Campbell]

On Fri, 2015-12-11 at 03:33 -0700, Jan Beulich wrote:
> > > > On 11.12.15 at 11:20, <andrew.cooper3@citrix.com> wrote:
> > On 11/12/15 07:53, Jan Beulich wrote:
> > > > > > On 10.12.15 at 21:07, <andrew.cooper3@citrix.com> wrote:
> > > > On 01/12/15 13:35, Jan Beulich wrote:
> > > > > > > > On 01.12.15 at 12:37, <andrew.cooper3@citrix.com> 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@lists.xen.org
http://lists.xen.org/xen-devel