[Xen-devel] [PATCH] x86/libxl: choose a sane default for HAP

[Xen-devel] [PATCH] x86/libxl: choose a sane default for HAP

[Roger Pau Monne]

Current libxl code will always enable Hardware Assisted Paging (HAP),
expecting that the hypervisor will fallback to shadow if HAP is not
available. With the changes to the domain builder that's not the case
any longer, and the hypervisor will raise an error if HAP is not
available instead of silently falling back to shadow.

In order to keep the previous functionality report whether HAP is
available or not in XEN_SYSCTL_physinfo, so that the toolstack can
select a sane default if there's no explicit user selection of whether
HAP should be used.

Fixes: d0c0ba7d3de ('x86/hvm/domain: remove the 'hap_enabled' flag')
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Paul Durrant <Paul.Durrant@citrix.com>
---
 tools/libxl/libxl.c         |  1 +
 tools/libxl/libxl_arch.h    |  4 ++--
 tools/libxl/libxl_arm.c     |  7 +++++--
 tools/libxl/libxl_create.c  | 10 ++++++----
 tools/libxl/libxl_types.idl |  1 +
 tools/libxl/libxl_x86.c     | 15 +++++++++++++--
 xen/arch/x86/sysctl.c       |  2 ++
 xen/include/public/sysctl.h |  4 ++++
 8 files changed, 34 insertions(+), 10 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index ec71574e99..5c0fcf320e 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -399,6 +399,7 @@ int libxl_get_physinfo(libxl_ctx *ctx, libxl_physinfo *physinfo)
     physinfo->cap_pv = !!(xcphysinfo.capabilities & XEN_SYSCTL_PHYSCAP_pv);
     physinfo->cap_hvm_directio =
         !!(xcphysinfo.capabilities & XEN_SYSCTL_PHYSCAP_directio);
+    physinfo->cap_hap = !!(xcphysinfo.capabilities & XEN_SYSCTL_PHYSCAP_hap);
 
     GC_FREE;
     return 0;
diff --git a/tools/libxl/libxl_arch.h b/tools/libxl/libxl_arch.h
index d624159e53..3f59e790b7 100644
--- a/tools/libxl/libxl_arch.h
+++ b/tools/libxl/libxl_arch.h
@@ -65,8 +65,8 @@ _hidden
 int libxl__arch_domain_map_irq(libxl__gc *gc, uint32_t domid, int irq);
 
 _hidden
-void libxl__arch_domain_create_info_setdefault(libxl__gc *gc,
-                                               libxl_domain_create_info *c_info);
+int libxl__arch_domain_create_info_setdefault(libxl__gc *gc,
+                                              libxl_domain_create_info *c_info);
 
 _hidden
 void libxl__arch_domain_build_info_setdefault(libxl__gc *gc,
diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
index 141e159043..b067869154 100644
--- a/tools/libxl/libxl_arm.c
+++ b/tools/libxl/libxl_arm.c
@@ -1114,8 +1114,8 @@ int libxl__arch_domain_map_irq(libxl__gc *gc, uint32_t domid, int irq)
     return xc_domain_bind_pt_spi_irq(CTX->xch, domid, irq, irq);
 }
 
-void libxl__arch_domain_create_info_setdefault(libxl__gc *gc,
-                                               libxl_domain_create_info *c_info)
+int libxl__arch_domain_create_info_setdefault(libxl__gc *gc,
+                                              libxl_domain_create_info *c_info)
 {
     /*
      * Arm guest are now considered as PVH by the toolstack. To allow
@@ -1130,6 +1130,9 @@ void libxl__arch_domain_create_info_setdefault(libxl__gc *gc,
         c_info->type = LIBXL_DOMAIN_TYPE_PVH;
         /* All other fields can remain untouched */
     }
+    libxl_defbool_setdefault(&c_info->hap, true);
+
+    return 0;
 }
 
 void libxl__arch_domain_build_info_setdefault(libxl__gc *gc,
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 03ce166f4f..431ead2067 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -30,17 +30,19 @@
 int libxl__domain_create_info_setdefault(libxl__gc *gc,
                                          libxl_domain_create_info *c_info)
 {
+    int rc;
+
     if (!c_info->type) {
         LOG(ERROR, "domain type unspecified");
         return ERROR_INVAL;
     }
 
-    libxl__arch_domain_create_info_setdefault(gc, c_info);
+    rc = libxl__arch_domain_create_info_setdefault(gc, c_info);
+    if (rc)
+        return rc;
 
-    if (c_info->type != LIBXL_DOMAIN_TYPE_PV) {
-        libxl_defbool_setdefault(&c_info->hap, true);
+    if (c_info->type != LIBXL_DOMAIN_TYPE_PV)
         libxl_defbool_setdefault(&c_info->oos, true);
-    }
 
     libxl_defbool_setdefault(&c_info->run_hotplug_scripts, true);
     libxl_defbool_setdefault(&c_info->driver_domain, false);
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index b61399ce36..9e1f8515d3 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -1025,6 +1025,7 @@ libxl_physinfo = Struct("physinfo", [
     ("cap_hvm", bool),
     ("cap_pv", bool),
     ("cap_hvm_directio", bool), # No longer HVM specific
+    ("cap_hap", bool),
     ], dir=DIR_OUT)
 
 libxl_connectorinfo = Struct("connectorinfo", [
diff --git a/tools/libxl/libxl_x86.c b/tools/libxl/libxl_x86.c
index c0f88a7eaa..bdc9d9adfe 100644
--- a/tools/libxl/libxl_x86.c
+++ b/tools/libxl/libxl_x86.c
@@ -620,9 +620,20 @@ int libxl__arch_domain_finalise_hw_description(libxl__gc *gc,
     return rc;
 }
 
-void libxl__arch_domain_create_info_setdefault(libxl__gc *gc,
-                                               libxl_domain_create_info *c_info)
+int libxl__arch_domain_create_info_setdefault(libxl__gc *gc,
+                                              libxl_domain_create_info *c_info)
 {
+    libxl_physinfo pi;
+    int rc = libxl_get_physinfo(CTX, &pi);
+
+    if (rc) {
+        LOG(ERROR, "unable to get physinfo");
+        return rc;
+    }
+
+    libxl_defbool_setdefault(&c_info->hap, pi.cap_hap);
+
+    return 0;
 }
 
 void libxl__arch_domain_build_info_setdefault(libxl__gc *gc,
diff --git a/xen/arch/x86/sysctl.c b/xen/arch/x86/sysctl.c
index c50d910a1c..74ea184087 100644
--- a/xen/arch/x86/sysctl.c
+++ b/xen/arch/x86/sysctl.c
@@ -165,6 +165,8 @@ void arch_do_physinfo(struct xen_sysctl_physinfo *pi)
         pi->capabilities |= XEN_SYSCTL_PHYSCAP_pv;
     if ( iommu_enabled )
         pi->capabilities |= XEN_SYSCTL_PHYSCAP_directio;
+    if ( hvm_hap_supported() )
+        pi->capabilities |= XEN_SYSCTL_PHYSCAP_hap;
 }
 
 long arch_do_sysctl(
diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
index 91c48dcae0..6c457625e9 100644
--- a/xen/include/public/sysctl.h
+++ b/xen/include/public/sysctl.h
@@ -90,6 +90,10 @@ struct xen_sysctl_tbuf_op {
  /* (x86) The platform supports direct access to I/O devices with IOMMU. */
 #define _XEN_SYSCTL_PHYSCAP_directio     2
 #define XEN_SYSCTL_PHYSCAP_directio  (1u<<_XEN_SYSCTL_PHYSCAP_directio)
+/* (x86) The platform supports Hardware Assisted Paging. */
+#define _XEN_SYSCTL_PHYSCAP_hap          3
+#define XEN_SYSCTL_PHYSCAP_hap           (1u<<_XEN_SYSCTL_PHYSCAP_hap)
+
 struct xen_sysctl_physinfo {
     uint32_t threads_per_core;
     uint32_t cores_per_socket;
-- 
2.22.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] x86/libxl: choose a sane default for HAP

[Paul Durrant]

> -----Original Message-----
[snip]
> -void libxl__arch_domain_create_info_setdefault(libxl__gc *gc,
> -                                               libxl_domain_create_info *c_info)
> +int libxl__arch_domain_create_info_setdefault(libxl__gc *gc,
> +                                              libxl_domain_create_info *c_info)
>  {
> +    libxl_physinfo pi;
> +    int rc = libxl_get_physinfo(CTX, &pi);
> +
> +    if (rc) {
> +        LOG(ERROR, "unable to get physinfo");
> +        return rc;
> +    }
> +
> +    libxl_defbool_setdefault(&c_info->hap, pi.cap_hap);

Is this going to work on ARM (where CDF_hap is required)? Because...

> +
> +    return 0;
>  }
> 
>  void libxl__arch_domain_build_info_setdefault(libxl__gc *gc,
> diff --git a/xen/arch/x86/sysctl.c b/xen/arch/x86/sysctl.c
> index c50d910a1c..74ea184087 100644
> --- a/xen/arch/x86/sysctl.c
> +++ b/xen/arch/x86/sysctl.c
> @@ -165,6 +165,8 @@ void arch_do_physinfo(struct xen_sysctl_physinfo *pi)
>          pi->capabilities |= XEN_SYSCTL_PHYSCAP_pv;
>      if ( iommu_enabled )
>          pi->capabilities |= XEN_SYSCTL_PHYSCAP_directio;
> +    if ( hvm_hap_supported() )
> +        pi->capabilities |= XEN_SYSCTL_PHYSCAP_hap;

...this is x86-only code, and I don't see an equivalent hunk for ARM.

  Paul

>  }
> 
>  long arch_do_sysctl(
> diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
> index 91c48dcae0..6c457625e9 100644
> --- a/xen/include/public/sysctl.h
> +++ b/xen/include/public/sysctl.h
> @@ -90,6 +90,10 @@ struct xen_sysctl_tbuf_op {
>   /* (x86) The platform supports direct access to I/O devices with IOMMU. */
>  #define _XEN_SYSCTL_PHYSCAP_directio     2
>  #define XEN_SYSCTL_PHYSCAP_directio  (1u<<_XEN_SYSCTL_PHYSCAP_directio)
> +/* (x86) The platform supports Hardware Assisted Paging. */
> +#define _XEN_SYSCTL_PHYSCAP_hap          3
> +#define XEN_SYSCTL_PHYSCAP_hap           (1u<<_XEN_SYSCTL_PHYSCAP_hap)
> +
>  struct xen_sysctl_physinfo {
>      uint32_t threads_per_core;
>      uint32_t cores_per_socket;
> --
> 2.22.0

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] x86/libxl: choose a sane default for HAP

[Jan Beulich]

On 05.09.2019 11:34, Roger Pau Monne wrote:
> Current libxl code will always enable Hardware Assisted Paging (HAP),
> expecting that the hypervisor will fallback to shadow if HAP is not
> available. With the changes to the domain builder that's not the case
> any longer, and the hypervisor will raise an error if HAP is not
> available instead of silently falling back to shadow.

Would it really be much more involved than the change here to restore
silent defaulting to shadow?

> --- a/xen/include/public/sysctl.h
> +++ b/xen/include/public/sysctl.h
> @@ -90,6 +90,10 @@ struct xen_sysctl_tbuf_op {
>   /* (x86) The platform supports direct access to I/O devices with IOMMU. */
>  #define _XEN_SYSCTL_PHYSCAP_directio     2
>  #define XEN_SYSCTL_PHYSCAP_directio  (1u<<_XEN_SYSCTL_PHYSCAP_directio)
> +/* (x86) The platform supports Hardware Assisted Paging. */
> +#define _XEN_SYSCTL_PHYSCAP_hap          3
> +#define XEN_SYSCTL_PHYSCAP_hap           (1u<<_XEN_SYSCTL_PHYSCAP_hap)

I think this bit wants to be universal (i.e. "(x86)" dropped), and
be set unconditionally on Arm. Irrespective of the question regarding
an alternative solution I think it is quite sensible to expose
availability of HAP to the tools. In fact I think "xl info" should
show this alongside other properties.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] x86/libxl: choose a sane default for HAP

[Roger Pau Monné]

On Thu, Sep 05, 2019 at 11:40:19AM +0200, Paul Durrant wrote:
> > -----Original Message-----
> [snip]
> > -void libxl__arch_domain_create_info_setdefault(libxl__gc *gc,
> > -                                               libxl_domain_create_info *c_info)
> > +int libxl__arch_domain_create_info_setdefault(libxl__gc *gc,
> > +                                              libxl_domain_create_info *c_info)
> >  {
> > +    libxl_physinfo pi;
> > +    int rc = libxl_get_physinfo(CTX, &pi);
> > +
> > +    if (rc) {
> > +        LOG(ERROR, "unable to get physinfo");
> > +        return rc;
> > +    }
> > +
> > +    libxl_defbool_setdefault(&c_info->hap, pi.cap_hap);
> 
> Is this going to work on ARM (where CDF_hap is required)? Because...

It should, libxl__arch_domain_create_info_setdefault for ARM sets hap
to true unconditionally.

> > +
> > +    return 0;
> >  }
> > 
> >  void libxl__arch_domain_build_info_setdefault(libxl__gc *gc,
> > diff --git a/xen/arch/x86/sysctl.c b/xen/arch/x86/sysctl.c
> > index c50d910a1c..74ea184087 100644
> > --- a/xen/arch/x86/sysctl.c
> > +++ b/xen/arch/x86/sysctl.c
> > @@ -165,6 +165,8 @@ void arch_do_physinfo(struct xen_sysctl_physinfo *pi)
> >          pi->capabilities |= XEN_SYSCTL_PHYSCAP_pv;
> >      if ( iommu_enabled )
> >          pi->capabilities |= XEN_SYSCTL_PHYSCAP_directio;
> > +    if ( hvm_hap_supported() )
> > +        pi->capabilities |= XEN_SYSCTL_PHYSCAP_hap;
> 
> ...this is x86-only code, and I don't see an equivalent hunk for ARM.

Yes, that flag is x86 only ATM (like all other capability flags).

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] x86/libxl: choose a sane default for HAP

[Andrew Cooper]

On 05/09/2019 10:52, Jan Beulich wrote:
> On 05.09.2019 11:34, Roger Pau Monne wrote:
>> Current libxl code will always enable Hardware Assisted Paging (HAP),
>> expecting that the hypervisor will fallback to shadow if HAP is not
>> available. With the changes to the domain builder that's not the case
>> any longer, and the hypervisor will raise an error if HAP is not
>> available instead of silently falling back to shadow.
> Would it really be much more involved than the change here to restore
> silent defaulting to shadow?

We could, but I would object to doing so.

It is not sensible behaviour to have, because the only thing it does is
hide (mis)configuration issues.

>
>> --- a/xen/include/public/sysctl.h
>> +++ b/xen/include/public/sysctl.h
>> @@ -90,6 +90,10 @@ struct xen_sysctl_tbuf_op {
>>   /* (x86) The platform supports direct access to I/O devices with IOMMU. */
>>  #define _XEN_SYSCTL_PHYSCAP_directio     2
>>  #define XEN_SYSCTL_PHYSCAP_directio  (1u<<_XEN_SYSCTL_PHYSCAP_directio)
>> +/* (x86) The platform supports Hardware Assisted Paging. */
>> +#define _XEN_SYSCTL_PHYSCAP_hap          3
>> +#define XEN_SYSCTL_PHYSCAP_hap           (1u<<_XEN_SYSCTL_PHYSCAP_hap)
> I think this bit wants to be universal (i.e. "(x86)" dropped), and
> be set unconditionally on Arm. Irrespective of the question regarding
> an alternative solution I think it is quite sensible to expose
> availability of HAP to the tools. In fact I think "xl info" should
> show this alongside other properties.

I agree.  While terms like HVM and HAP may have come about in the x86
world, they are deliberately vendor and technology neutral.

ARM *should* be claim to be, and behave as, HVM-only HAP-only system,
per this nomenclature.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] x86/libxl: choose a sane default for HAP

[Roger Pau Monné]

On Thu, Sep 05, 2019 at 11:52:59AM +0200, Jan Beulich wrote:
> On 05.09.2019 11:34, Roger Pau Monne wrote:
> > Current libxl code will always enable Hardware Assisted Paging (HAP),
> > expecting that the hypervisor will fallback to shadow if HAP is not
> > available. With the changes to the domain builder that's not the case
> > any longer, and the hypervisor will raise an error if HAP is not
> > available instead of silently falling back to shadow.
> 
> Would it really be much more involved than the change here to restore
> silent defaulting to shadow?

But that would mean that a user having selected hap=1 on the config
file would get silently defaulted to shadow, which is wrong IMO.

> > --- a/xen/include/public/sysctl.h
> > +++ b/xen/include/public/sysctl.h
> > @@ -90,6 +90,10 @@ struct xen_sysctl_tbuf_op {
> >   /* (x86) The platform supports direct access to I/O devices with IOMMU. */
> >  #define _XEN_SYSCTL_PHYSCAP_directio     2
> >  #define XEN_SYSCTL_PHYSCAP_directio  (1u<<_XEN_SYSCTL_PHYSCAP_directio)
> > +/* (x86) The platform supports Hardware Assisted Paging. */
> > +#define _XEN_SYSCTL_PHYSCAP_hap          3
> > +#define XEN_SYSCTL_PHYSCAP_hap           (1u<<_XEN_SYSCTL_PHYSCAP_hap)
> 
> I think this bit wants to be universal (i.e. "(x86)" dropped), and
> be set unconditionally on Arm. Irrespective of the question regarding
> an alternative solution I think it is quite sensible to expose
> availability of HAP to the tools. In fact I think "xl info" should
> show this alongside other properties.

I can indeed make this available to both x86 and ARM. AFAICT it's
always going to be true on ARM. I can expand this a bit so it's also
printed in `xl info` together with the rest of the virt_caps.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] x86/libxl: choose a sane default for HAP

[Jan Beulich]

On 05.09.2019 12:01, Roger Pau Monné  wrote:
> On Thu, Sep 05, 2019 at 11:52:59AM +0200, Jan Beulich wrote:
>> On 05.09.2019 11:34, Roger Pau Monne wrote:
>>> Current libxl code will always enable Hardware Assisted Paging (HAP),
>>> expecting that the hypervisor will fallback to shadow if HAP is not
>>> available. With the changes to the domain builder that's not the case
>>> any longer, and the hypervisor will raise an error if HAP is not
>>> available instead of silently falling back to shadow.
>>
>> Would it really be much more involved than the change here to restore
>> silent defaulting to shadow?
> 
> But that would mean that a user having selected hap=1 on the config
> file would get silently defaulted to shadow, which is wrong IMO.

Since you and Andrew agree, so be it. Personally I'd like it better
if "hap=1" didn't prevent a domain from starting on a HAP-incapable
host. I wouldn't mind a warning though.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] x86/libxl: choose a sane default for HAP

[Andrew Cooper]

On 05/09/2019 11:32, Jan Beulich wrote:
> On 05.09.2019 12:01, Roger Pau Monné  wrote:
>> On Thu, Sep 05, 2019 at 11:52:59AM +0200, Jan Beulich wrote:
>>> On 05.09.2019 11:34, Roger Pau Monne wrote:
>>>> Current libxl code will always enable Hardware Assisted Paging (HAP),
>>>> expecting that the hypervisor will fallback to shadow if HAP is not
>>>> available. With the changes to the domain builder that's not the case
>>>> any longer, and the hypervisor will raise an error if HAP is not
>>>> available instead of silently falling back to shadow.
>>> Would it really be much more involved than the change here to restore
>>> silent defaulting to shadow?
>> But that would mean that a user having selected hap=1 on the config
>> file would get silently defaulted to shadow, which is wrong IMO.
> Since you and Andrew agree, so be it. Personally I'd like it better
> if "hap=1" didn't prevent a domain from starting on a HAP-incapable
> host. I wouldn't mind a warning though.

Behaviour like that could be arranged in libxl, which now has all the
requisite pieces.

What I'm not happy with is xc_domain_create() asking for a HAP domain
and getting silently getting a shadow one.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] x86/libxl: choose a sane default for HAP

[George Dunlap]

> On Sep 5, 2019, at 11:01 AM, Roger Pau Monne <roger.pau@citrix.com> wrote:
> 
> On Thu, Sep 05, 2019 at 11:52:59AM +0200, Jan Beulich wrote:
>> On 05.09.2019 11:34, Roger Pau Monne wrote:
>>> Current libxl code will always enable Hardware Assisted Paging (HAP),
>>> expecting that the hypervisor will fallback to shadow if HAP is not
>>> available. With the changes to the domain builder that's not the case
>>> any longer, and the hypervisor will raise an error if HAP is not
>>> available instead of silently falling back to shadow.
>> 
>> Would it really be much more involved than the change here to restore
>> silent defaulting to shadow?
> 
> But that would mean that a user having selected hap=1 on the config
> file would get silently defaulted to shadow, which is wrong IMO.

At the libxl layer, aren’t the options tristate?  I.e., this would be “hap”, “shadow”, or “not specified”?

The user needs to be able to specify “always use shadow”, “always use HAP”, or “use HAP if available, otherwise use shadow”.  At the moment, leaving it empty should be “use HAP if available, otherwise use shadow”; so “hap = 1” should fail if HAP is not available.

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] x86/libxl: choose a sane default for HAP

[Roger Pau Monné]

On Thu, Sep 05, 2019 at 12:34:11PM +0200, George Dunlap wrote:
> 
> 
> > On Sep 5, 2019, at 11:01 AM, Roger Pau Monne <roger.pau@citrix.com> wrote:
> > 
> > On Thu, Sep 05, 2019 at 11:52:59AM +0200, Jan Beulich wrote:
> >> On 05.09.2019 11:34, Roger Pau Monne wrote:
> >>> Current libxl code will always enable Hardware Assisted Paging (HAP),
> >>> expecting that the hypervisor will fallback to shadow if HAP is not
> >>> available. With the changes to the domain builder that's not the case
> >>> any longer, and the hypervisor will raise an error if HAP is not
> >>> available instead of silently falling back to shadow.
> >> 
> >> Would it really be much more involved than the change here to restore
> >> silent defaulting to shadow?
> > 
> > But that would mean that a user having selected hap=1 on the config
> > file would get silently defaulted to shadow, which is wrong IMO.
> 
> At the libxl layer, aren’t the options tristate?  I.e., this would be “hap”, “shadow”, or “not specified”?
> 
> The user needs to be able to specify “always use shadow”, “always use HAP”, or “use HAP if available, otherwise use shadow”.

The "use HAP if available, otherwise use shadow" is currently only
possibly expressed by not setting the hap option in the config file.

> At the moment, leaving it empty should be “use HAP if available, otherwise use shadow”; so “hap = 1” should fail if HAP is not available.

Right, this is what this patch is trying to accomplish.

I have a v2 series which fills the capabilities field for ARM and also
reports the hap capability in `xl info`.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] x86/libxl: choose a sane default for HAP

[George Dunlap]

On 9/5/19 12:01 PM, Roger Pau Monné wrote:
> On Thu, Sep 05, 2019 at 12:34:11PM +0200, George Dunlap wrote:
>>
>>
>>> On Sep 5, 2019, at 11:01 AM, Roger Pau Monne <roger.pau@citrix.com> wrote:
>>>
>>> On Thu, Sep 05, 2019 at 11:52:59AM +0200, Jan Beulich wrote:
>>>> On 05.09.2019 11:34, Roger Pau Monne wrote:
>>>>> Current libxl code will always enable Hardware Assisted Paging (HAP),
>>>>> expecting that the hypervisor will fallback to shadow if HAP is not
>>>>> available. With the changes to the domain builder that's not the case
>>>>> any longer, and the hypervisor will raise an error if HAP is not
>>>>> available instead of silently falling back to shadow.
>>>>
>>>> Would it really be much more involved than the change here to restore
>>>> silent defaulting to shadow?
>>>
>>> But that would mean that a user having selected hap=1 on the config
>>> file would get silently defaulted to shadow, which is wrong IMO.
>>
>> At the libxl layer, aren’t the options tristate?  I.e., this would be “hap”, “shadow”, or “not specified”?
>>
>> The user needs to be able to specify “always use shadow”, “always use HAP”, or “use HAP if available, otherwise use shadow”.
> 
> The "use HAP if available, otherwise use shadow" is currently only
> possibly expressed by not setting the hap option in the config file.
> 
>> At the moment, leaving it empty should be “use HAP if available, otherwise use shadow”; so “hap = 1” should fail if HAP is not available.
> 
> Right, this is what this patch is trying to accomplish.

Right; I wasn't trying to contradict you so much as "weigh in" (and
basically agree with you).

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel