diff mbox series

[v4,1/2] sysctl/libxl: choose a sane default for HAP

Message ID 20190910152538.36921-1-roger.pau@citrix.com (mailing list archive)
State New, archived
Headers show
Series [v4,1/2] sysctl/libxl: choose a sane default for HAP | expand

Commit Message

Roger Pau Monne Sept. 10, 2019, 3:25 p.m. UTC
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 DOMCTL_createdomain 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.

Note that on ARM hardware HAP capability is always reported since it's
a required feature in order to run Xen.

Fixes: d0c0ba7d3de ('x86/hvm/domain: remove the 'hap_enabled' flag')
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Paul Durrant <paul.durrant@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Julien Grall <julien.grall@arm.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
Cc: Paul Durrant <Paul.Durrant@citrix.com>
---
Changes since v3:
 - Add ocaml flags.

Changes since v2:
 - Add a LIBXL_HAVE_PHYSINFO_CAP_HAP for compatibility.

Changes since v1:
 - Also report HAP capability for ARM.
 - Print hap capability in xl info.
---
 tools/libxl/libxl.c             | 1 +
 tools/libxl/libxl.h             | 7 +++++++
 tools/libxl/libxl_create.c      | 8 +++++++-
 tools/libxl/libxl_types.idl     | 1 +
 tools/ocaml/libs/xc/xenctrl.ml  | 1 +
 tools/ocaml/libs/xc/xenctrl.mli | 1 +
 tools/xl/xl_info.c              | 5 +++--
 xen/arch/arm/sysctl.c           | 2 +-
 xen/arch/x86/sysctl.c           | 2 ++
 xen/include/public/sysctl.h     | 5 ++++-
 10 files changed, 28 insertions(+), 5 deletions(-)

Comments

Christian Lindig Sept. 10, 2019, 3:27 p.m. UTC | #1
> On 10 Sep 2019, at 16:25, Roger Pau Monne <roger.pau@citrix.com> wrote:
> 
> diff --git a/tools/ocaml/libs/xc/xenctrl.ml b/tools/ocaml/libs/xc/xenctrl.ml
> index e544ef84da..a5e77c943a 100644
> --- a/tools/ocaml/libs/xc/xenctrl.ml
> +++ b/tools/ocaml/libs/xc/xenctrl.ml
> @@ -107,6 +107,7 @@ type physinfo_cap_flag =
> 	| CAP_HVM
> 	| CAP_PV
> 	| CAP_DirectIO
> +	| CAP_hap
> 
> type physinfo =
> {
> diff --git a/tools/ocaml/libs/xc/xenctrl.mli b/tools/ocaml/libs/xc/xenctrl.mli
> index 5a35000761..e92256654b 100644
> --- a/tools/ocaml/libs/xc/xenctrl.mli
> +++ b/tools/ocaml/libs/xc/xenctrl.mli
> @@ -92,6 +92,7 @@ type physinfo_cap_flag =
>   | CAP_HVM
>   | CAP_PV
>   | CAP_DirectIO
> +  | CAP_hap

Acked-by: Christian Lindig <christian.lindig@citrix.com>
Andrew Cooper Sept. 10, 2019, 3:40 p.m. UTC | #2
On 10/09/2019 16:25, Roger Pau Monne wrote:
> diff --git a/tools/ocaml/libs/xc/xenctrl.ml b/tools/ocaml/libs/xc/xenctrl.ml
> index e544ef84da..a5e77c943a 100644
> --- a/tools/ocaml/libs/xc/xenctrl.ml
> +++ b/tools/ocaml/libs/xc/xenctrl.ml
> @@ -107,6 +107,7 @@ type physinfo_cap_flag =
>  	| CAP_HVM
>  	| CAP_PV
>  	| CAP_DirectIO
> +	| CAP_hap

HAP is an initialism just like HVM, so definitely should be capitalised.

IMO, DirectIO being not all caps is a bug, but its in the API now.

~Andrew
Roger Pau Monne Sept. 10, 2019, 3:44 p.m. UTC | #3
On Tue, Sep 10, 2019 at 04:40:39PM +0100, Andrew Cooper wrote:
> On 10/09/2019 16:25, Roger Pau Monne wrote:
> > diff --git a/tools/ocaml/libs/xc/xenctrl.ml b/tools/ocaml/libs/xc/xenctrl.ml
> > index e544ef84da..a5e77c943a 100644
> > --- a/tools/ocaml/libs/xc/xenctrl.ml
> > +++ b/tools/ocaml/libs/xc/xenctrl.ml
> > @@ -107,6 +107,7 @@ type physinfo_cap_flag =
> >  	| CAP_HVM
> >  	| CAP_PV
> >  	| CAP_DirectIO
> > +	| CAP_hap
> 
> HAP is an initialism just like HVM, so definitely should be capitalised.

Funny thing, I had it all caps and then switched to this form
to match the sysctl define (XEN_SYSCTL_PHYSCAP_hap).

Would you like me to resend with all caps?

Thanks, Roger.
diff mbox series

Patch

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.h b/tools/libxl/libxl.h
index 9bacfb97f0..3ff67792a7 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -394,6 +394,13 @@ 
  */
 #define LIBXL_HAVE_EXTENDED_VKB 1
 
+/*
+ * LIBXL_HAVE_PHYSINFO_CAP_HAP indicates that libxl_physinfo has a cap_hap
+ * field that indicates whether the hardware supports Hardware Assisted
+ * Paging.
+ */
+#define LIBXL_HAVE_PHYSINFO_CAP_HAP 1
+
 /*
  * libxl ABI compatibility
  *
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 79e010da72..3b45065597 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -38,7 +38,13 @@  int libxl__domain_create_info_setdefault(libxl__gc *gc,
     libxl__arch_domain_create_info_setdefault(gc, c_info);
 
     if (c_info->type != LIBXL_DOMAIN_TYPE_PV) {
-        libxl_defbool_setdefault(&c_info->hap, true);
+        libxl_physinfo info;
+        int rc = libxl_get_physinfo(CTX, &info);
+
+        if (rc)
+            return rc;
+
+        libxl_defbool_setdefault(&c_info->hap, info.cap_hap);
         libxl_defbool_setdefault(&c_info->oos, true);
     }
 
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/ocaml/libs/xc/xenctrl.ml b/tools/ocaml/libs/xc/xenctrl.ml
index e544ef84da..a5e77c943a 100644
--- a/tools/ocaml/libs/xc/xenctrl.ml
+++ b/tools/ocaml/libs/xc/xenctrl.ml
@@ -107,6 +107,7 @@  type physinfo_cap_flag =
 	| CAP_HVM
 	| CAP_PV
 	| CAP_DirectIO
+	| CAP_hap
 
 type physinfo =
 {
diff --git a/tools/ocaml/libs/xc/xenctrl.mli b/tools/ocaml/libs/xc/xenctrl.mli
index 5a35000761..e92256654b 100644
--- a/tools/ocaml/libs/xc/xenctrl.mli
+++ b/tools/ocaml/libs/xc/xenctrl.mli
@@ -92,6 +92,7 @@  type physinfo_cap_flag =
   | CAP_HVM
   | CAP_PV
   | CAP_DirectIO
+  | CAP_hap
 type physinfo = {
   threads_per_core : int;
   cores_per_socket : int;
diff --git a/tools/xl/xl_info.c b/tools/xl/xl_info.c
index 46d9c9f712..aa6724bc7f 100644
--- a/tools/xl/xl_info.c
+++ b/tools/xl/xl_info.c
@@ -210,11 +210,12 @@  static void output_physinfo(void)
          info.hw_cap[4], info.hw_cap[5], info.hw_cap[6], info.hw_cap[7]
         );
 
-    maybe_printf("virt_caps              :%s%s%s%s\n",
+    maybe_printf("virt_caps              :%s%s%s%s%s\n",
          info.cap_pv ? " pv" : "",
          info.cap_hvm ? " hvm" : "",
          info.cap_hvm && info.cap_hvm_directio ? " hvm_directio" : "",
-         info.cap_pv && info.cap_hvm_directio ? " pv_directio" : ""
+         info.cap_pv && info.cap_hvm_directio ? " pv_directio" : "",
+         info.cap_hap ? " hap" : ""
         );
 
     vinfo = libxl_get_version_info(ctx);
diff --git a/xen/arch/arm/sysctl.c b/xen/arch/arm/sysctl.c
index 92ac99c928..f87944e847 100644
--- a/xen/arch/arm/sysctl.c
+++ b/xen/arch/arm/sysctl.c
@@ -14,7 +14,7 @@ 
 
 void arch_do_physinfo(struct xen_sysctl_physinfo *pi)
 {
-    pi->capabilities |= XEN_SYSCTL_PHYSCAP_hvm;
+    pi->capabilities |= XEN_SYSCTL_PHYSCAP_hvm | XEN_SYSCTL_PHYSCAP_hap;
 }
 
 long arch_do_sysctl(struct xen_sysctl *sysctl,
diff --git a/xen/arch/x86/sysctl.c b/xen/arch/x86/sysctl.c
index 7ec6174e6b..5777a05ffc 100644
--- a/xen/arch/x86/sysctl.c
+++ b/xen/arch/x86/sysctl.c
@@ -163,6 +163,8 @@  void arch_do_physinfo(struct xen_sysctl_physinfo *pi)
         pi->capabilities |= XEN_SYSCTL_PHYSCAP_hvm;
     if ( IS_ENABLED(CONFIG_PV) )
         pi->capabilities |= XEN_SYSCTL_PHYSCAP_pv;
+    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 5401f9c2fe..d4b455619c 100644
--- a/xen/include/public/sysctl.h
+++ b/xen/include/public/sysctl.h
@@ -90,9 +90,12 @@  struct xen_sysctl_tbuf_op {
  /* 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)
+/* The platform supports Hardware Assisted Paging. */
+#define _XEN_SYSCTL_PHYSCAP_hap          3
+#define XEN_SYSCTL_PHYSCAP_hap           (1u<<_XEN_SYSCTL_PHYSCAP_hap)
 
 /* Max XEN_SYSCTL_PHYSCAP_* constant.  Used for ABI checking. */
-#define XEN_SYSCTL_PHYSCAP_MAX XEN_SYSCTL_PHYSCAP_directio
+#define XEN_SYSCTL_PHYSCAP_MAX XEN_SYSCTL_PHYSCAP_hap
 
 struct xen_sysctl_physinfo {
     uint32_t threads_per_core;