[Xen-devel] [PATCH 0/6] tools: Rationalise legacy CPUID handling

[Xen-devel] [PATCH 0/6] tools: Rationalise legacy CPUID handling

[Andrew Cooper]

This series attempts to remove some of the more insane behaviours from early
CPUID handling.  This is a little RFC to get a feel, but I intend to extend
the same kind of handling to ITSC (and get rid of nomigrate), and provide some
slightly nested-virt behaviour until it can become a domaincreate parameter.

Patch 1 has already been posted twice before.  Others are new.  I'm leaning
towards rebasing the libxl/migration work over this series, because it will
clean up a few of the transformation patches.

Andrew Cooper (6):
  tools/libxl: Remove libxl_cpuid_{set,apply_policy}() from the API
  tools/ocaml: Drop cpuid helpers
  tools/python: Drop cpuid helpers
  tools/libxl: Combine legacy CPUID handling logic
  tools/libx[cl]: Don't use HVM_PARAM_PAE_ENABLED as a function parameter
  xen/public: Obsolete HVM_PARAM_PAE_ENABLED

 tools/libxc/include/xenctrl.h       |  2 +-
 tools/libxc/xc_cpuid_x86.c          | 15 ++----
 tools/libxc/xc_sr_restore_x86_hvm.c | 10 ++++
 tools/libxc/xc_sr_save_x86_hvm.c    |  1 -
 tools/libxl/libxl.h                 | 26 ++++++++--
 tools/libxl/libxl_cpuid.c           | 27 +++++++---
 tools/libxl/libxl_dom.c             |  4 +-
 tools/libxl/libxl_internal.h        |  3 ++
 tools/libxl/libxl_nocpuid.c         |  8 +--
 tools/libxl/libxl_x86.c             |  8 +--
 tools/ocaml/libs/xc/xenctrl.ml      |  7 ---
 tools/ocaml/libs/xc/xenctrl.mli     |  7 ---
 tools/ocaml/libs/xc/xenctrl_stubs.c | 62 -----------------------
 tools/python/xen/lowlevel/xc/xc.c   | 98 -------------------------------------
 xen/arch/arm/hvm.c                  |  3 +-
 xen/arch/x86/hvm/hvm.c              |  2 +
 xen/include/public/hvm/params.h     |  2 +-
 17 files changed, 70 insertions(+), 215 deletions(-)

-- 
2.11.0


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

[Xen-devel] [PATCH 1/6] tools/libxl: Remove libxl_cpuid_{set, apply_policy}() from the API

[Andrew Cooper]

These functions should never have been exposed.  They don't have external
users, and can't usefully be used for several reasons.

Move libxl_cpuid_{set,apply_policy}() to being internal functions, and leave
an equivalent of the nop stubs in the API for caller compatibility.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Ian Jackson <Ian.Jackson@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Anthony PERARD <anthony.perard@citrix.com>

RFC for obvious reasons.  An alternative would be to #if 0 them, which would
result in a compile failure rather than silent stubbing.  I'm not sure which
is least bad, but I don't think either are going to cause a problem in
practice.
---
 tools/libxl/libxl.h          | 26 ++++++++++++++++++++++----
 tools/libxl/libxl_cpuid.c    |  6 +++---
 tools/libxl/libxl_dom.c      |  4 ++--
 tools/libxl/libxl_internal.h |  4 ++++
 tools/libxl/libxl_nocpuid.c  |  6 +++---
 5 files changed, 34 insertions(+), 12 deletions(-)

diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 18c1a2d6bf..d1d31b1e67 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -665,7 +665,7 @@ typedef struct libxl__ctx libxl_ctx;
 #if LIBXL_API_VERSION != 0x040200 && LIBXL_API_VERSION != 0x040300 && \
     LIBXL_API_VERSION != 0x040400 && LIBXL_API_VERSION != 0x040500 && \
     LIBXL_API_VERSION != 0x040700 && LIBXL_API_VERSION != 0x040800 && \
-    LIBXL_API_VERSION != 0x041300
+    LIBXL_API_VERSION != 0x041300 && LIBXL_API_VERSION != 0x041400
 #error Unknown LIBXL_API_VERSION
 #endif
 #endif
@@ -2325,9 +2325,27 @@ libxl_device_pci *libxl_device_pci_assignable_list(libxl_ctx *ctx, int *num);
 int libxl_cpuid_parse_config(libxl_cpuid_policy_list *cpuid, const char* str);
 int libxl_cpuid_parse_config_xend(libxl_cpuid_policy_list *cpuid,
                                   const char* str);
-void libxl_cpuid_apply_policy(libxl_ctx *ctx, uint32_t domid);
-void libxl_cpuid_set(libxl_ctx *ctx, uint32_t domid,
-                     libxl_cpuid_policy_list cpuid);
+#if LIBXL_API_VERSION < 0x041400
+/*
+ * Dropped from the API in Xen 4.14.  At the time of writing, these functions
+ * don't appear to ever have had external callers.
+ *
+ * These have always been used internally during domain construction, and
+ * can't easily be used externally because of their implicit parameters in
+ * other pieces of global state.
+ *
+ * Furthermore, an API user can't usefully determine whether they get
+ * libxl_cpuid (the real implementation) or libxl_nocpuid (no-op stubs).
+ *
+ * The internal behaviour of these functions also needs to change.  Therefore
+ * for simplicitly, provide the no-op stubs.  Yes technically this is an API
+ * change in some cases for existing software, but there is 0 of that in
+ * practice.
+ */
+static inline void libxl_cpuid_apply_policy(libxl_ctx *ctx, uint32_t domid) {}
+static inline void libxl_cpuid_set(libxl_ctx *ctx, uint32_t domid,
+                                   libxl_cpuid_policy_list cpuid) {}
+#endif
 
 /*
  * Functions for allowing users of libxl to store private data
diff --git a/tools/libxl/libxl_cpuid.c b/tools/libxl/libxl_cpuid.c
index 5c52cbe0f9..505ec1b048 100644
--- a/tools/libxl/libxl_cpuid.c
+++ b/tools/libxl/libxl_cpuid.c
@@ -410,13 +410,13 @@ int libxl_cpuid_parse_config_xend(libxl_cpuid_policy_list *cpuid,
     return 0;
 }
 
-void libxl_cpuid_apply_policy(libxl_ctx *ctx, uint32_t domid)
+void libxl__cpuid_apply_policy(libxl_ctx *ctx, uint32_t domid)
 {
     xc_cpuid_apply_policy(ctx->xch, domid, NULL, 0);
 }
 
-void libxl_cpuid_set(libxl_ctx *ctx, uint32_t domid,
-                     libxl_cpuid_policy_list cpuid)
+void libxl__cpuid_set(libxl_ctx *ctx, uint32_t domid,
+                      libxl_cpuid_policy_list cpuid)
 {
     int i;
     char *cpuid_res[4];
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index d9ada8a422..bbb1be75ba 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -454,9 +454,9 @@ int libxl__build_post(libxl__gc *gc, uint32_t domid,
     if (rc)
         return rc;
 
-    libxl_cpuid_apply_policy(ctx, domid);
+    libxl__cpuid_apply_policy(ctx, domid);
     if (info->cpuid != NULL)
-        libxl_cpuid_set(ctx, domid, info->cpuid);
+        libxl__cpuid_set(ctx, domid, info->cpuid);
 
     if (info->type == LIBXL_DOMAIN_TYPE_HVM
         && !libxl_ms_vm_genid_is_zero(&info->u.hvm.ms_vm_genid)) {
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index dd3c08bc14..164d93b89b 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -2056,6 +2056,10 @@ struct libxl__cpuid_policy {
     char *policy[4];
 };
 
+_hidden void libxl__cpuid_apply_policy(libxl_ctx *ctx, uint32_t domid);
+_hidden void libxl__cpuid_set(libxl_ctx *ctx, uint32_t domid,
+                              libxl_cpuid_policy_list cpuid);
+
 /* Calls poll() again - useful to check whether a signaled condition
  * is still true.  Cannot fail.  Returns currently-true revents. */
 _hidden short libxl__fd_poll_recheck(libxl__egc *egc, int fd, short events);
diff --git a/tools/libxl/libxl_nocpuid.c b/tools/libxl/libxl_nocpuid.c
index ef1161c434..a39babe754 100644
--- a/tools/libxl/libxl_nocpuid.c
+++ b/tools/libxl/libxl_nocpuid.c
@@ -34,12 +34,12 @@ int libxl_cpuid_parse_config_xend(libxl_cpuid_policy_list *cpuid,
     return 0;
 }
 
-void libxl_cpuid_apply_policy(libxl_ctx *ctx, uint32_t domid)
+void libxl__cpuid_apply_policy(libxl_ctx *ctx, uint32_t domid)
 {
 }
 
-void libxl_cpuid_set(libxl_ctx *ctx, uint32_t domid,
-                     libxl_cpuid_policy_list cpuid)
+void libxl__cpuid_set(libxl_ctx *ctx, uint32_t domid,
+                      libxl_cpuid_policy_list cpuid)
 {
 }
 
-- 
2.11.0


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

[Xen-devel] [PATCH 2/6] tools/ocaml: Drop cpuid helpers

[Andrew Cooper]

These have no callers, and the underlying infrastructure is about to be
rewritten completely.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Christian Lindig <christian.lindig@citrix.com>
---
 tools/ocaml/libs/xc/xenctrl.ml      |  7 -----
 tools/ocaml/libs/xc/xenctrl.mli     |  7 -----
 tools/ocaml/libs/xc/xenctrl_stubs.c | 62 -------------------------------------
 3 files changed, 76 deletions(-)

diff --git a/tools/ocaml/libs/xc/xenctrl.ml b/tools/ocaml/libs/xc/xenctrl.ml
index e00a74d48d..497ded7ce2 100644
--- a/tools/ocaml/libs/xc/xenctrl.ml
+++ b/tools/ocaml/libs/xc/xenctrl.ml
@@ -256,13 +256,6 @@ external domain_set_memmap_limit: handle -> domid -> int64 -> unit
 external domain_memory_increase_reservation: handle -> domid -> int64 -> unit
        = "stub_xc_domain_memory_increase_reservation"
 
-external domain_cpuid_set: handle -> domid -> (int64 * (int64 option))
-                        -> string option array
-                        -> string option array
-       = "stub_xc_domain_cpuid_set"
-external domain_cpuid_apply_policy: handle -> domid -> unit
-       = "stub_xc_domain_cpuid_apply_policy"
-
 external map_foreign_range: handle -> domid -> int
                          -> nativeint -> Xenmmap.mmap_interface
        = "stub_map_foreign_range"
diff --git a/tools/ocaml/libs/xc/xenctrl.mli b/tools/ocaml/libs/xc/xenctrl.mli
index 0e7049d708..26ec7e59b1 100644
--- a/tools/ocaml/libs/xc/xenctrl.mli
+++ b/tools/ocaml/libs/xc/xenctrl.mli
@@ -219,10 +219,3 @@ external pages_to_kib : int64 -> int64 = "stub_pages_to_kib"
 val pages_to_mib : int64 -> int64
 external watchdog : handle -> int -> int32 -> int
   = "stub_xc_watchdog"
-
-external domain_cpuid_set: handle -> domid -> (int64 * (int64 option))
-                        -> string option array
-                        -> string option array
-       = "stub_xc_domain_cpuid_set"
-external domain_cpuid_apply_policy: handle -> domid -> unit
-       = "stub_xc_domain_cpuid_apply_policy"
diff --git a/tools/ocaml/libs/xc/xenctrl_stubs.c b/tools/ocaml/libs/xc/xenctrl_stubs.c
index 48f39f81d5..904da45c4f 100644
--- a/tools/ocaml/libs/xc/xenctrl_stubs.c
+++ b/tools/ocaml/libs/xc/xenctrl_stubs.c
@@ -48,12 +48,6 @@
 #define string_of_option_array(array, index) \
 	((Field(array, index) == Val_none) ? NULL : String_val(Field(Field(array, index), 0)))
 
-/* maybe here we should check the range of the input instead of blindly
- * casting it to uint32 */
-#define cpuid_input_of_val(i1, i2, input) \
-	i1 = (uint32_t) Int64_val(Field(input, 0)); \
-	i2 = ((Field(input, 1) == Val_none) ? 0xffffffff : (uint32_t) Int64_val(Field(Field(input, 1), 0)));
-
 static void Noreturn failwith_xc(xc_interface *xch)
 {
 	char error_str[XC_MAX_ERROR_MSG_LEN + 6];
@@ -826,62 +820,6 @@ CAMLprim value stub_xc_domain_memory_increase_reservation(value xch,
 	CAMLreturn(Val_unit);
 }
 
-CAMLprim value stub_xc_domain_cpuid_set(value xch, value domid,
-                                        value input,
-                                        value config)
-{
-	CAMLparam4(xch, domid, input, config);
-	CAMLlocal2(array, tmp);
-#if defined(__i386__) || defined(__x86_64__)
-	int r;
-	unsigned int c_input[2];
-	char *c_config[4], *out_config[4];
-
-	c_config[0] = string_of_option_array(config, 0);
-	c_config[1] = string_of_option_array(config, 1);
-	c_config[2] = string_of_option_array(config, 2);
-	c_config[3] = string_of_option_array(config, 3);
-
-	cpuid_input_of_val(c_input[0], c_input[1], input);
-
-	array = caml_alloc(4, 0);
-	for (r = 0; r < 4; r++) {
-		tmp = Val_none;
-		if (c_config[r]) {
-			tmp = caml_alloc_small(1, 0);
-			Field(tmp, 0) = caml_alloc_string(32);
-		}
-		Store_field(array, r, tmp);
-	}
-
-	for (r = 0; r < 4; r++)
-		out_config[r] = (c_config[r]) ? String_val(Field(Field(array, r), 0)) : NULL;
-
-	r = xc_cpuid_set(_H(xch), _D(domid),
-			 c_input, (const char **)c_config, out_config);
-	if (r < 0)
-		failwith_xc(_H(xch));
-#else
-	caml_failwith("xc_domain_cpuid_set: not implemented");
-#endif
-	CAMLreturn(array);
-}
-
-CAMLprim value stub_xc_domain_cpuid_apply_policy(value xch, value domid)
-{
-	CAMLparam2(xch, domid);
-#if defined(__i386__) || defined(__x86_64__)
-	int r;
-
-	r = xc_cpuid_apply_policy(_H(xch), _D(domid), NULL, 0);
-	if (r < 0)
-		failwith_xc(_H(xch));
-#else
-	caml_failwith("xc_domain_cpuid_apply_policy: not implemented");
-#endif
-	CAMLreturn(Val_unit);
-}
-
 CAMLprim value stub_xc_version_version(value xch)
 {
 	CAMLparam1(xch);
-- 
2.11.0


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

[Xen-devel] [PATCH 3/6] tools/python: Drop cpuid helpers

[Andrew Cooper]

These are believed-unused, and the underlying infrastructure is about to be
rewritten completely.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Ian Jackson <Ian.Jackson@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
 tools/python/xen/lowlevel/xc/xc.c | 98 ---------------------------------------
 1 file changed, 98 deletions(-)

diff --git a/tools/python/xen/lowlevel/xc/xc.c b/tools/python/xen/lowlevel/xc/xc.c
index a751e85910..ac0e26a742 100644
--- a/tools/python/xen/lowlevel/xc/xc.c
+++ b/tools/python/xen/lowlevel/xc/xc.c
@@ -694,84 +694,6 @@ static PyObject *pyxc_get_device_group(XcObject *self,
     return Pystr;
 }
 
-#if defined(__i386__) || defined(__x86_64__)
-static void pyxc_dom_extract_cpuid(PyObject *config,
-                                  char **regs)
-{
-    const char *regs_extract[4] = { "eax", "ebx", "ecx", "edx" };
-    PyObject *obj;
-    int i;
-
-    memset(regs, 0, 4*sizeof(*regs));
-
-    if ( !PyDict_Check(config) )
-        return;
-
-    for ( i = 0; i < 4; i++ )
-        if ( (obj = PyDict_GetItemString(config, regs_extract[i])) != NULL )
-            regs[i] = PyBytes_AS_STRING(obj);
-}
-
-static PyObject *pyxc_create_cpuid_dict(char **regs)
-{
-   const char *regs_extract[4] = { "eax", "ebx", "ecx", "edx" };
-   PyObject *dict;
-   int i;
-
-   dict = PyDict_New();
-   for ( i = 0; i < 4; i++ )
-   {
-       if ( regs[i] == NULL )
-           continue;
-       PyDict_SetItemString(dict, regs_extract[i],
-                            PyBytes_FromString(regs[i]));
-       free(regs[i]);
-       regs[i] = NULL;
-   }
-   return dict;
-}
-
-static PyObject *pyxc_dom_set_policy_cpuid(XcObject *self,
-                                           PyObject *args)
-{
-    int domid;
-
-    if ( !PyArg_ParseTuple(args, "i", &domid) )
-        return NULL;
-
-    if ( xc_cpuid_apply_policy(self->xc_handle, domid, NULL, 0) )
-        return pyxc_error_to_exception(self->xc_handle);
-
-    Py_INCREF(zero);
-    return zero;
-}
-
-
-static PyObject *pyxc_dom_set_cpuid(XcObject *self,
-                                    PyObject *args)
-{
-    PyObject *sub_input, *config;
-    unsigned int domid, input[2];
-    char *regs[4], *regs_transform[4];
-
-    if ( !PyArg_ParseTuple(args, "IIOO", &domid,
-                           &input[0], &sub_input, &config) )
-        return NULL;
-
-    pyxc_dom_extract_cpuid(config, regs);
-
-    input[1] = XEN_CPUID_INPUT_UNUSED;
-    if ( PyLong_Check(sub_input) )
-        input[1] = PyLong_AsUnsignedLong(sub_input);
-
-    if ( xc_cpuid_set(self->xc_handle, domid, input, (const char **)regs,
-                      regs_transform) )
-        return pyxc_error_to_exception(self->xc_handle);
-
-    return pyxc_create_cpuid_dict(regs_transform);
-}
-#endif /* __i386__ || __x86_64__ */
-
 static PyObject *pyxc_gnttab_hvm_seed(XcObject *self,
 				      PyObject *args,
 				      PyObject *kwds)
@@ -2406,26 +2328,6 @@ static PyMethodDef pyxc_methods[] = {
       "Inject debug keys into Xen.\n"
       " keys    [str]: String of keys to inject.\n" },
 
-#if defined(__i386__) || defined(__x86_64__)
-    { "domain_set_cpuid", 
-      (PyCFunction)pyxc_dom_set_cpuid, 
-      METH_VARARGS, "\n"
-      "Set cpuid response for an input and a domain.\n"
-      " dom [int]: Identifier of domain.\n"
-      " input [long]: Input for cpuid instruction (eax)\n"
-      " sub_input [long]: Second input (optional, may be None) for cpuid "
-      "                     instruction (ecx)\n"
-      " config [dict]: Dictionary of register\n\n"
-      "Returns: [int] 0 on success; exception on error.\n" },
-
-    { "domain_set_policy_cpuid", 
-      (PyCFunction)pyxc_dom_set_policy_cpuid, 
-      METH_VARARGS, "\n"
-      "Set the default cpuid policy for a domain.\n"
-      " dom [int]: Identifier of domain.\n\n"
-      "Returns: [int] 0 on success; exception on error.\n" },
-#endif
-
     { "dom_set_memshr", 
       (PyCFunction)pyxc_dom_set_memshr,
       METH_VARARGS, "\n"
-- 
2.11.0


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

[Xen-devel] [PATCH 4/6] tools/libxl: Combine legacy CPUID handling logic

[Andrew Cooper]

While we are in the process of overhauling boot time CPUID/MSR handling, the
existing logic is going to have to remain in roughly this form for backwards
compatibility.

Fold libxl__cpuid_apply_policy() and libxl__cpuid_set() together into a single
libxl__cpuid_legacy() to reduce the complexity for callers.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Ian Jackson <Ian.Jackson@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Anthony PERARD <anthony.perard@citrix.com>
---
 tools/libxl/libxl_cpuid.c    | 15 ++++++++-------
 tools/libxl/libxl_dom.c      |  4 +---
 tools/libxl/libxl_internal.h |  5 ++---
 tools/libxl/libxl_nocpuid.c  |  8 ++------
 4 files changed, 13 insertions(+), 19 deletions(-)

diff --git a/tools/libxl/libxl_cpuid.c b/tools/libxl/libxl_cpuid.c
index 505ec1b048..49d3ca5b26 100644
--- a/tools/libxl/libxl_cpuid.c
+++ b/tools/libxl/libxl_cpuid.c
@@ -410,17 +410,18 @@ int libxl_cpuid_parse_config_xend(libxl_cpuid_policy_list *cpuid,
     return 0;
 }
 
-void libxl__cpuid_apply_policy(libxl_ctx *ctx, uint32_t domid)
-{
-    xc_cpuid_apply_policy(ctx->xch, domid, NULL, 0);
-}
-
-void libxl__cpuid_set(libxl_ctx *ctx, uint32_t domid,
-                      libxl_cpuid_policy_list cpuid)
+void libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid,
+                         libxl_domain_build_info *info)
 {
+    libxl_cpuid_policy_list cpuid = info->cpuid;
     int i;
     char *cpuid_res[4];
 
+    xc_cpuid_apply_policy(ctx->xch, domid, NULL, 0);
+
+    if (!cpuid)
+        return;
+
     for (i = 0; cpuid[i].input[0] != XEN_CPUID_INPUT_UNUSED; i++)
         xc_cpuid_set(ctx->xch, domid, cpuid[i].input,
                      (const char**)(cpuid[i].policy), cpuid_res);
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index bbb1be75ba..71cb578923 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -454,9 +454,7 @@ int libxl__build_post(libxl__gc *gc, uint32_t domid,
     if (rc)
         return rc;
 
-    libxl__cpuid_apply_policy(ctx, domid);
-    if (info->cpuid != NULL)
-        libxl__cpuid_set(ctx, domid, info->cpuid);
+    libxl__cpuid_legacy(ctx, domid, info);
 
     if (info->type == LIBXL_DOMAIN_TYPE_HVM
         && !libxl_ms_vm_genid_is_zero(&info->u.hvm.ms_vm_genid)) {
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 164d93b89b..4936446069 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -2056,9 +2056,8 @@ struct libxl__cpuid_policy {
     char *policy[4];
 };
 
-_hidden void libxl__cpuid_apply_policy(libxl_ctx *ctx, uint32_t domid);
-_hidden void libxl__cpuid_set(libxl_ctx *ctx, uint32_t domid,
-                              libxl_cpuid_policy_list cpuid);
+_hidden void libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid,
+                                 libxl_domain_build_info *info);
 
 /* Calls poll() again - useful to check whether a signaled condition
  * is still true.  Cannot fail.  Returns currently-true revents. */
diff --git a/tools/libxl/libxl_nocpuid.c b/tools/libxl/libxl_nocpuid.c
index a39babe754..7776574a0c 100644
--- a/tools/libxl/libxl_nocpuid.c
+++ b/tools/libxl/libxl_nocpuid.c
@@ -34,12 +34,8 @@ int libxl_cpuid_parse_config_xend(libxl_cpuid_policy_list *cpuid,
     return 0;
 }
 
-void libxl__cpuid_apply_policy(libxl_ctx *ctx, uint32_t domid)
-{
-}
-
-void libxl__cpuid_set(libxl_ctx *ctx, uint32_t domid,
-                      libxl_cpuid_policy_list cpuid)
+void libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid,
+                         libxl_domain_config *d_config)
 {
 }
 
-- 
2.11.0


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

[Xen-devel] [PATCH 5/6] tools/libx[cl]: Don't use HVM_PARAM_PAE_ENABLED as a function parameter

[Andrew Cooper]

The sole use of HVM_PARAM_PAE_ENABLED is as a non-standard calling convention
for xc_cpuid_apply_policy().  Pass PAE as a regular parameter instead.

Leave a rather better explaination of why only HVM guests have a choice in PAE
setting.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Ian Jackson <Ian.Jackson@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Anthony PERARD <anthony.perard@citrix.com>
---
 tools/libxc/include/xenctrl.h |  2 +-
 tools/libxc/xc_cpuid_x86.c    | 15 +++++----------
 tools/libxl/libxl_cpuid.c     | 14 +++++++++++++-
 3 files changed, 19 insertions(+), 12 deletions(-)

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 311df1ef0f..4eb4f4c2c6 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -1807,7 +1807,7 @@ int xc_cpuid_set(xc_interface *xch,
 int xc_cpuid_apply_policy(xc_interface *xch,
                           uint32_t domid,
                           const uint32_t *featureset,
-                          unsigned int nr_features);
+                          unsigned int nr_features, bool pae);
 int xc_mca_op(xc_interface *xch, struct xen_mc *mc);
 int xc_mca_op_inject_v2(xc_interface *xch, unsigned int flags,
                         xc_cpumap_t cpumap, unsigned int nr_cpus);
diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c
index 2540aa1e1c..4e74a7ed3b 100644
--- a/tools/libxc/xc_cpuid_x86.c
+++ b/tools/libxc/xc_cpuid_x86.c
@@ -455,7 +455,8 @@ int xc_cpuid_set(
 }
 
 int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid,
-                          const uint32_t *featureset, unsigned int nr_features)
+                          const uint32_t *featureset, unsigned int nr_features,
+                          bool pae)
 {
     int rc;
     xc_dominfo_t di;
@@ -579,8 +580,6 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid,
     }
     else
     {
-        uint64_t val;
-
         /*
          * Topology for HVM guests is entirely controlled by Xen.  For now, we
          * hardcode APIC_ID = vcpu_id * 2 to give the illusion of no SMT.
@@ -635,14 +634,10 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid,
         }
 
         /*
-         * HVM_PARAM_PAE_ENABLED is a parameter to this function, stashed in
-         * Xen.  Nothing else has ever taken notice of the value.
+         * PAE used to be a parameter passed to this function by
+         * HVM_PARAM_PAE_ENABLED.  It is now passed normally.
          */
-        rc = xc_hvm_param_get(xch, domid, HVM_PARAM_PAE_ENABLED, &val);
-        if ( rc )
-            goto out;
-
-        p->basic.pae = val;
+        p->basic.pae = pae;
 
         /*
          * These settings are necessary to cause earlier HVM_PARAM_NESTEDHVM /
diff --git a/tools/libxl/libxl_cpuid.c b/tools/libxl/libxl_cpuid.c
index 49d3ca5b26..8c49e34125 100644
--- a/tools/libxl/libxl_cpuid.c
+++ b/tools/libxl/libxl_cpuid.c
@@ -416,8 +416,20 @@ void libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid,
     libxl_cpuid_policy_list cpuid = info->cpuid;
     int i;
     char *cpuid_res[4];
+    bool pae = true;
+
+    /*
+     * PAE is a Xen-controlled for PV guests (it is the 'p' that causes the
+     * difference between the xen-3.0-x86_32 and xen-3.0-x86_32p ABIs).  It is
+     * mandatory as Xen is running in 64bit mode.
+     *
+     * PVH guests don't have a top-level PAE control, and is treated as
+     * available.
+     */
+    if (info->type == LIBXL_DOMAIN_TYPE_HVM)
+        pae = libxl_defbool_val(info->u.hvm.pae);
 
-    xc_cpuid_apply_policy(ctx->xch, domid, NULL, 0);
+    xc_cpuid_apply_policy(ctx->xch, domid, NULL, 0, pae);
 
     if (!cpuid)
         return;
-- 
2.11.0


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

[Xen-devel] [PATCH 6/6] xen/public: Obsolete HVM_PARAM_PAE_ENABLED

[Andrew Cooper]

HVM_PARAM_PAE_ENABLED is undocumented and Xen has never acted upon its value,
contrary perhaps to expectations based on how other boolean fields work.

It was only ever used as a non-standard calling convention for
xc_cpuid_apply_policy() but that has been fixed now.

Purge its use, and any possible confusion over its behaviour, by having Xen
reject any attempts to use it.  Forgo setting it up in libxl's
hvm_set_conf_params().  The only backwards compatibility necessary is to have
the HVM restore stream discard it if found.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien@xen.org>
CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
CC: Ian Jackson <Ian.Jackson@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Anthony PERARD <anthony.perard@citrix.com>
---
 tools/libxc/xc_sr_restore_x86_hvm.c | 10 ++++++++++
 tools/libxc/xc_sr_save_x86_hvm.c    |  1 -
 tools/libxl/libxl_x86.c             |  8 +-------
 xen/arch/arm/hvm.c                  |  3 ++-
 xen/arch/x86/hvm/hvm.c              |  2 ++
 xen/include/public/hvm/params.h     |  2 +-
 6 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/tools/libxc/xc_sr_restore_x86_hvm.c b/tools/libxc/xc_sr_restore_x86_hvm.c
index 3f78248f32..da1574ce11 100644
--- a/tools/libxc/xc_sr_restore_x86_hvm.c
+++ b/tools/libxc/xc_sr_restore_x86_hvm.c
@@ -72,6 +72,16 @@ static int handle_hvm_params(struct xc_sr_context *ctx,
         case HVM_PARAM_BUFIOREQ_PFN:
             xc_clear_domain_page(xch, ctx->domid, entry->value);
             break;
+
+        case HVM_PARAM_PAE_ENABLED:
+            /*
+             * This HVM_PARAM only ever existed a non-standard calling ABI for
+             * xc_cpuid_apply_policy().  It has now been updated to use a
+             * regular calling convention, making the param obsolete.
+             *
+             * Discard if we find it in an old migration stream.
+             */
+            continue;
         }
 
         rc = xc_hvm_param_set(xch, ctx->domid, entry->index, entry->value);
diff --git a/tools/libxc/xc_sr_save_x86_hvm.c b/tools/libxc/xc_sr_save_x86_hvm.c
index d99efe65e5..7d3f3ddb8f 100644
--- a/tools/libxc/xc_sr_save_x86_hvm.c
+++ b/tools/libxc/xc_sr_save_x86_hvm.c
@@ -71,7 +71,6 @@ static int write_hvm_params(struct xc_sr_context *ctx)
         HVM_PARAM_ACPI_IOPORTS_LOCATION,
         HVM_PARAM_VIRIDIAN,
         HVM_PARAM_IDENT_PT,
-        HVM_PARAM_PAE_ENABLED,
         HVM_PARAM_VM_GENERATION_ID_ADDR,
         HVM_PARAM_IOREQ_SERVER_PFN,
         HVM_PARAM_NR_IOREQ_SERVER_PAGES,
diff --git a/tools/libxl/libxl_x86.c b/tools/libxl/libxl_x86.c
index 1cae0e2b26..f8bc828e62 100644
--- a/tools/libxl/libxl_x86.c
+++ b/tools/libxl/libxl_x86.c
@@ -391,12 +391,10 @@ static int hvm_set_conf_params(libxl__gc *gc, uint32_t domid,
     libxl_ctx *ctx = libxl__gc_owner(gc);
     xc_interface *xch = ctx->xch;
     int ret = ERROR_FAIL;
-    bool pae = true, altp2m = info->altp2m;
+    bool altp2m = info->altp2m;
 
     switch(info->type) {
     case LIBXL_DOMAIN_TYPE_HVM:
-        pae = libxl_defbool_val(info->u.hvm.pae);
-
         /* The config parameter "altp2m" replaces the parameter "altp2mhvm". For
          * legacy reasons, both parameters are accepted on x86 HVM guests.
          *
@@ -425,10 +423,6 @@ static int hvm_set_conf_params(libxl__gc *gc, uint32_t domid,
 
         /* Fallthrough */
     case LIBXL_DOMAIN_TYPE_PVH:
-        if (xc_hvm_param_set(xch, domid, HVM_PARAM_PAE_ENABLED, pae)) {
-            LOG(ERROR, "Couldn't set HVM_PARAM_PAE_ENABLED");
-            goto out;
-        }
         if (xc_hvm_param_set(xch, domid, HVM_PARAM_TIMER_MODE,
                              timer_mode(info))) {
             LOG(ERROR, "Couldn't set HVM_PARAM_TIMER_MODE");
diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c
index 76b27c9168..f3426f37fe 100644
--- a/xen/arch/arm/hvm.c
+++ b/xen/arch/arm/hvm.c
@@ -46,7 +46,8 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
         if ( copy_from_guest(&a, arg, 1) )
             return -EFAULT;
 
-        if ( a.index >= HVM_NR_PARAMS )
+        if ( a.index >= HVM_NR_PARAMS ||
+             a.index == HVM_PARAM_PAE_ENABLED )
             return -EINVAL;
 
         d = rcu_lock_domain_by_any_id(a.domid);
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 00a9e70b7c..2b869ac997 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4104,6 +4104,7 @@ static int hvm_allow_set_param(struct domain *d,
     case HVM_PARAM_X87_FIP_WIDTH:
         break;
     /* The following parameters are deprecated. */
+    case HVM_PARAM_PAE_ENABLED:
     case HVM_PARAM_DM_DOMAIN:
     case HVM_PARAM_BUFIOREQ_EVTCHN:
         rc = -EPERM;
@@ -4410,6 +4411,7 @@ static int hvm_allow_get_param(struct domain *d,
     case HVM_PARAM_X87_FIP_WIDTH:
         break;
     /* The following parameters are deprecated. */
+    case HVM_PARAM_PAE_ENABLED:
     case HVM_PARAM_DM_DOMAIN:
     case HVM_PARAM_BUFIOREQ_EVTCHN:
         rc = -ENODATA;
diff --git a/xen/include/public/hvm/params.h b/xen/include/public/hvm/params.h
index 36832e4b94..faa6bda095 100644
--- a/xen/include/public/hvm/params.h
+++ b/xen/include/public/hvm/params.h
@@ -86,7 +86,7 @@
 #define HVM_PARAM_STORE_PFN    1
 #define HVM_PARAM_STORE_EVTCHN 2
 
-#define HVM_PARAM_PAE_ENABLED  4
+#define HVM_PARAM_PAE_ENABLED  4 /* Obsolete.  Do not use. */
 
 #define HVM_PARAM_IOREQ_PFN    5
 
-- 
2.11.0


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

Re: [Xen-devel] [PATCH 3/6] tools/python: Drop cpuid helpers

[Marek Marczykowski-Górecki]

[-- Attachment #1.1: Type: text/plain, Size: 492 bytes --]

On Wed, Feb 05, 2020 at 04:50:53PM +0000, Andrew Cooper wrote:
> These are believed-unused, and the underlying infrastructure is about to be
> rewritten completely.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

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

Re: [Xen-devel] [PATCH 6/6] xen/public: Obsolete HVM_PARAM_PAE_ENABLED

[Jan Beulich]

On 05.02.2020 17:50, Andrew Cooper wrote:
> --- a/tools/libxc/xc_sr_restore_x86_hvm.c
> +++ b/tools/libxc/xc_sr_restore_x86_hvm.c
> @@ -72,6 +72,16 @@ static int handle_hvm_params(struct xc_sr_context *ctx,
>          case HVM_PARAM_BUFIOREQ_PFN:
>              xc_clear_domain_page(xch, ctx->domid, entry->value);
>              break;
> +
> +        case HVM_PARAM_PAE_ENABLED:
> +            /*
> +             * This HVM_PARAM only ever existed a non-standard calling ABI for
> +             * xc_cpuid_apply_policy().  It has now been updated to use a
> +             * regular calling convention, making the param obsolete.
> +             *
> +             * Discard if we find it in an old migration stream.
> +             */
> +            continue;

Having also looked at the previous patch (the only one in this series
relevant to the adjustments done here afaict) I wonder whether simply
ignoring it (i.e. not even warning anywhere when out of sync with
whatever info->u.hvm.pae gets populated from) is a good approach. But
of course I may be easily missing aspects here that make this quite
fine.

> --- a/xen/include/public/hvm/params.h
> +++ b/xen/include/public/hvm/params.h
> @@ -86,7 +86,7 @@
>  #define HVM_PARAM_STORE_PFN    1
>  #define HVM_PARAM_STORE_EVTCHN 2
>  
> -#define HVM_PARAM_PAE_ENABLED  4
> +#define HVM_PARAM_PAE_ENABLED  4 /* Obsolete.  Do not use. */

I think this should be moved up in the deprecated section. With this
hypervisor parts
Reviewed-by: Jan Beulich <jbeulich@suse.com>

As an aside I also think that section should check for just __XEN__,
not also __XEN_TOOLS__.

Jan

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

Re: [Xen-devel] [PATCH 6/6] xen/public: Obsolete HVM_PARAM_PAE_ENABLED

[Andrew Cooper]

On 06/02/2020 09:28, Jan Beulich wrote:
> On 05.02.2020 17:50, Andrew Cooper wrote:
>> --- a/tools/libxc/xc_sr_restore_x86_hvm.c
>> +++ b/tools/libxc/xc_sr_restore_x86_hvm.c
>> @@ -72,6 +72,16 @@ static int handle_hvm_params(struct xc_sr_context *ctx,
>>          case HVM_PARAM_BUFIOREQ_PFN:
>>              xc_clear_domain_page(xch, ctx->domid, entry->value);
>>              break;
>> +
>> +        case HVM_PARAM_PAE_ENABLED:
>> +            /*
>> +             * This HVM_PARAM only ever existed a non-standard calling ABI for
>> +             * xc_cpuid_apply_policy().  It has now been updated to use a
>> +             * regular calling convention, making the param obsolete.
>> +             *
>> +             * Discard if we find it in an old migration stream.
>> +             */
>> +            continue;
> Having also looked at the previous patch (the only one in this series
> relevant to the adjustments done here afaict)

Correct.

> I wonder whether simply
> ignoring it (i.e. not even warning anywhere when out of sync with
> whatever info->u.hvm.pae gets populated from) is a good approach.

We can't (easily) cross check at all, because info->u.hvm.pae is in a
separate process (as far as the xl/libxl toolstack goes).

On cross checking, you'll find that migration in from pre Xen 4.6
doesn't actually have the data.  If you look back at the 4.5 legacy
migration code, you'll observe that this param is restored but never
saved.  In hindsight, we probably shouldn't have fixed that in migration
v2, but we did.

Upstream was actually fine, because libxl sets HVM_PARAM_PAE_ENABLED
after the migration stream completes, and overwrites whatever was
where.  XenServer did not, and we noticed as a consequence of Xen 4.5
actually cross-checked CPUID settings on a mov to %cr4 emulation.

> But of course I may be easily missing aspects here that make this quite
> fine.

It really is obsolete and needs forgetting, not checking.

>
>> --- a/xen/include/public/hvm/params.h
>> +++ b/xen/include/public/hvm/params.h
>> @@ -86,7 +86,7 @@
>>  #define HVM_PARAM_STORE_PFN    1
>>  #define HVM_PARAM_STORE_EVTCHN 2
>>  
>> -#define HVM_PARAM_PAE_ENABLED  4
>> +#define HVM_PARAM_PAE_ENABLED  4 /* Obsolete.  Do not use. */
> I think this should be moved up in the deprecated section.

There isn't a deprecated section.

The params are currently sorted numerically.  Playing "which param is
this integer?" with an unsorted params.h is an experience I wish never
to repeat again.

~Andrew

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

Re: [Xen-devel] [PATCH 6/6] xen/public: Obsolete HVM_PARAM_PAE_ENABLED

[Jan Beulich]

On 06.02.2020 12:32, Andrew Cooper wrote:
> On 06/02/2020 09:28, Jan Beulich wrote:
>> On 05.02.2020 17:50, Andrew Cooper wrote:
>>> --- a/xen/include/public/hvm/params.h
>>> +++ b/xen/include/public/hvm/params.h
>>> @@ -86,7 +86,7 @@
>>>  #define HVM_PARAM_STORE_PFN    1
>>>  #define HVM_PARAM_STORE_EVTCHN 2
>>>  
>>> -#define HVM_PARAM_PAE_ENABLED  4
>>> +#define HVM_PARAM_PAE_ENABLED  4 /* Obsolete.  Do not use. */
>> I think this should be moved up in the deprecated section.
> 
> There isn't a deprecated section.
> 
> The params are currently sorted numerically.  Playing "which param is
> this integer?" with an unsorted params.h is an experience I wish never
> to repeat again.

You'll find

/* These parameters are deprecated and their meaning is undefined. */

near the top of the file. I can see your concern about the file not
being sorted, but it already isn't. The alternative then is to frame
each deprecated param by #ifdef __XEN__ - I'd be okay with that if
that's preferred by you.

Jan

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

Re: [Xen-devel] [PATCH 6/6] xen/public: Obsolete HVM_PARAM_PAE_ENABLED

[Andrew Cooper]

On 06/02/2020 11:37, Jan Beulich wrote:
> On 06.02.2020 12:32, Andrew Cooper wrote:
>> On 06/02/2020 09:28, Jan Beulich wrote:
>>> On 05.02.2020 17:50, Andrew Cooper wrote:
>>>> --- a/xen/include/public/hvm/params.h
>>>> +++ b/xen/include/public/hvm/params.h
>>>> @@ -86,7 +86,7 @@
>>>>  #define HVM_PARAM_STORE_PFN    1
>>>>  #define HVM_PARAM_STORE_EVTCHN 2
>>>>  
>>>> -#define HVM_PARAM_PAE_ENABLED  4
>>>> +#define HVM_PARAM_PAE_ENABLED  4 /* Obsolete.  Do not use. */
>>> I think this should be moved up in the deprecated section.
>> There isn't a deprecated section.
>>
>> The params are currently sorted numerically.  Playing "which param is
>> this integer?" with an unsorted params.h is an experience I wish never
>> to repeat again.
> You'll find
>
> /* These parameters are deprecated and their meaning is undefined. */
>
> near the top of the file.

Urgh - I was looking at the 4.5 code.

What's currently in stating is a total mess, and we've got multiple
different ways of dealing with reserved values.

~Andrew

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

Re: [Xen-devel] [PATCH 2/6] tools/ocaml: Drop cpuid helpers

[Christian Lindig]

On 05/02/2020 16:50, Andrew Cooper wrote:
> These have no callers, and the underlying infrastructure is about to be
> rewritten completely.
> 
> Signed-off-by: Andrew Cooper<andrew.cooper3@citrix.com>
> ---
> CC: Christian Lindig<christian.lindig@citrix.com>
> ---
>   tools/ocaml/libs/xc/xenctrl.ml      |  7 -----
>   tools/ocaml/libs/xc/xenctrl.mli     |  7 -----
>   tools/ocaml/libs/xc/xenctrl_stubs.c | 62 -------------------------------------
>   3 files changed, 76 deletions(-)

Acked-by: Christian Lindig <christian.lindig@citrix.com>

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

Re: [Xen-devel] [PATCH 6/6] xen/public: Obsolete HVM_PARAM_PAE_ENABLED

[Julien Grall]

Hi Andrew,

On 05/02/2020 16:50, Andrew Cooper wrote:
> diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c
> index 76b27c9168..f3426f37fe 100644
> --- a/xen/arch/arm/hvm.c
> +++ b/xen/arch/arm/hvm.c
> @@ -46,7 +46,8 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
>           if ( copy_from_guest(&a, arg, 1) )
>               return -EFAULT;
>   
> -        if ( a.index >= HVM_NR_PARAMS )
> +        if ( a.index >= HVM_NR_PARAMS ||
> +             a.index == HVM_PARAM_PAE_ENABLED )
>               return -EINVAL;

For the small Arm part:

Acked-by: Julien Grall <julien@xen.org>

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH 6/6] xen/public: Obsolete HVM_PARAM_PAE_ENABLED

[Andrew Cooper]

On 08/02/2020 12:12, Julien Grall wrote:
> Hi Andrew,
>
> On 05/02/2020 16:50, Andrew Cooper wrote:
>> diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c
>> index 76b27c9168..f3426f37fe 100644
>> --- a/xen/arch/arm/hvm.c
>> +++ b/xen/arch/arm/hvm.c
>> @@ -46,7 +46,8 @@ long do_hvm_op(unsigned long op,
>> XEN_GUEST_HANDLE_PARAM(void) arg)
>>           if ( copy_from_guest(&a, arg, 1) )
>>               return -EFAULT;
>>   -        if ( a.index >= HVM_NR_PARAMS )
>> +        if ( a.index >= HVM_NR_PARAMS ||
>> +             a.index == HVM_PARAM_PAE_ENABLED )
>>               return -EINVAL;
>
> For the small Arm part:
>
> Acked-by: Julien Grall <julien@xen.org>
>

So it turns out that, in light of "xen/hvm: Fix handling of obsolete
HVM_PARAMs​", there is even more breakage here, because it is *only* the
libxc code which causes the params to be rejected on ARM.

I'm clearly going to have to sink rather more time into this than I was
initially intending, so I'll dust off my earlier series and sort this
who mess out, once and for all.

~Andrew

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

Re: [Xen-devel] [PATCH 1/6] tools/libxl: Remove libxl_cpuid_{set, apply_policy}() from the API

[Ian Jackson]

Andrew Cooper writes ("[PATCH 1/6] tools/libxl: Remove libxl_cpuid_{set,apply_policy}() from the API"):
> These functions should never have been exposed.  They don't have external
> users, and can't usefully be used for several reasons.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

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

Re: [Xen-devel] [PATCH 3/6] tools/python: Drop cpuid helpers

[Ian Jackson]

Andrew Cooper writes ("[PATCH 3/6] tools/python: Drop cpuid helpers"):
> These are believed-unused, and the underlying infrastructure is about to be
> rewritten completely.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

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

Re: [Xen-devel] [PATCH 4/6] tools/libxl: Combine legacy CPUID handling logic

[Ian Jackson]

Andrew Cooper writes ("[PATCH 4/6] tools/libxl: Combine legacy CPUID handling logic"):
> While we are in the process of overhauling boot time CPUID/MSR handling, the
> existing logic is going to have to remain in roughly this form for backwards
> compatibility.
> 
> Fold libxl__cpuid_apply_policy() and libxl__cpuid_set() together into a single
> libxl__cpuid_legacy() to reduce the complexity for callers.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

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

Re: [Xen-devel] [PATCH 5/6] tools/libx[cl]: Don't use HVM_PARAM_PAE_ENABLED as a function parameter

[Ian Jackson]

Andrew Cooper writes ("[PATCH 5/6] tools/libx[cl]: Don't use HVM_PARAM_PAE_ENABLED as a function parameter"):
> The sole use of HVM_PARAM_PAE_ENABLED is as a non-standard calling convention
> for xc_cpuid_apply_policy().  Pass PAE as a regular parameter instead.
> 
> Leave a rather better explaination of why only HVM guests have a choice in PAE
> setting.

I am inclined believe you that this is right (since you are evidently
familiar with this whole area and I'm not), but the explanations leave
me confused.

>  int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid,
> -                          const uint32_t *featureset, unsigned int nr_features)
> +                          const uint32_t *featureset, unsigned int nr_features,
> +                          bool pae)
>  {
>      int rc;
>      xc_dominfo_t di;
> @@ -579,8 +580,6 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid,
>      }
>      else
>      {
> -        uint64_t val;
> -
>          /*
>           * Topology for HVM guests is entirely controlled by Xen.  For now, we
>           * hardcode APIC_ID = vcpu_id * 2 to give the illusion of no SMT.
> @@ -635,14 +634,10 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid,
>          }
>  
>          /*
> -         * HVM_PARAM_PAE_ENABLED is a parameter to this function, stashed in
> -         * Xen.  Nothing else has ever taken notice of the value.
> +         * PAE used to be a parameter passed to this function by
> +         * HVM_PARAM_PAE_ENABLED.  It is now passed normally.

In particular, I don't understand what these comments mean by
"HVM_PARAM_PAE_ENABLED is a parameter to this function" and "PAE used
to be a parameter passed to this function by HVM_PARAM_PAE_ENABLED".

Maybe this is some loose use of the term "parameter" ?

If you could explain more clearly (ideally, explain the meaning of the
old comment in the commit message, and make the new comment
unambiguous) then that would be great.

Thanks,
Ian.

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

Re: [Xen-devel] [PATCH 6/6] xen/public: Obsolete HVM_PARAM_PAE_ENABLED

[Ian Jackson]

Andrew Cooper writes ("[PATCH 6/6] xen/public: Obsolete HVM_PARAM_PAE_ENABLED"):
> HVM_PARAM_PAE_ENABLED is undocumented and Xen has never acted upon its value,
> contrary perhaps to expectations based on how other boolean fields work.
> 
> It was only ever used as a non-standard calling convention for
> xc_cpuid_apply_policy() but that has been fixed now.
> 
> Purge its use, and any possible confusion over its behaviour, by having Xen
> reject any attempts to use it.  Forgo setting it up in libxl's
> hvm_set_conf_params().  The only backwards compatibility necessary is to have
> the HVM restore stream discard it if found.

This looks plausible too.  But maybe I should be reading this patch
and the previous one together ?  Or maybe they would be better
squashed ?

If you think that is likely to make me less confused I'm happy to try
squashing them locally and reading the result...

Ian.

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

Re: [Xen-devel] [PATCH 5/6] tools/libx[cl]: Don't use HVM_PARAM_PAE_ENABLED as a function parameter

[Andrew Cooper]

On 11/02/2020 17:47, Ian Jackson wrote:
> Andrew Cooper writes ("[PATCH 5/6] tools/libx[cl]: Don't use HVM_PARAM_PAE_ENABLED as a function parameter"):
>> The sole use of HVM_PARAM_PAE_ENABLED is as a non-standard calling convention
>> for xc_cpuid_apply_policy().  Pass PAE as a regular parameter instead.
>>
>> Leave a rather better explaination of why only HVM guests have a choice in PAE
>> setting.
> I am inclined believe you that this is right (since you are evidently
> familiar with this whole area and I'm not), but the explanations leave
> me confused.
>
>>  int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid,
>> -                          const uint32_t *featureset, unsigned int nr_features)
>> +                          const uint32_t *featureset, unsigned int nr_features,
>> +                          bool pae)
>>  {
>>      int rc;
>>      xc_dominfo_t di;
>> @@ -579,8 +580,6 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid,
>>      }
>>      else
>>      {
>> -        uint64_t val;
>> -
>>          /*
>>           * Topology for HVM guests is entirely controlled by Xen.  For now, we
>>           * hardcode APIC_ID = vcpu_id * 2 to give the illusion of no SMT.
>> @@ -635,14 +634,10 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid,
>>          }
>>  
>>          /*
>> -         * HVM_PARAM_PAE_ENABLED is a parameter to this function, stashed in
>> -         * Xen.  Nothing else has ever taken notice of the value.
>> +         * PAE used to be a parameter passed to this function by
>> +         * HVM_PARAM_PAE_ENABLED.  It is now passed normally.
> In particular, I don't understand what these comments mean by
> "HVM_PARAM_PAE_ENABLED is a parameter to this function" and "PAE used
> to be a parameter passed to this function by HVM_PARAM_PAE_ENABLED".
>
> Maybe this is some loose use of the term "parameter" ?
>
> If you could explain more clearly (ideally, explain the meaning of the
> old comment in the commit message, and make the new comment
> unambiguous) then that would be great.

HVM_PARAM_PAE_ENABLED encapsulates a boolean meaning "should I advertise
the PAE feature to the guest?".

It has only ever been used in a way which should have been "bool pae"
passed into xc_cpuid_apply_policy().  This patch tries to do just that.


I think there might be confusion as to which comment the commit message
referred to.

In xc_cpuid_apply_policy(), I want a comment explaining why we have this
weird pae parameter.  It will disappear from the new way of doing CPUID
at boot, but will have to remain for the pre-4.14 compatibility.

The comment I was referring to in the commit message was actually the
libxl comment, explaining why PV and PVH guests don't get a choice to
hide the PAE feature.

~Andrew

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

Re: [Xen-devel] [PATCH 6/6] xen/public: Obsolete HVM_PARAM_PAE_ENABLED

[Andrew Cooper]

On 11/02/2020 17:49, Ian Jackson wrote:
> Andrew Cooper writes ("[PATCH 6/6] xen/public: Obsolete HVM_PARAM_PAE_ENABLED"):
>> HVM_PARAM_PAE_ENABLED is undocumented and Xen has never acted upon its value,
>> contrary perhaps to expectations based on how other boolean fields work.
>>
>> It was only ever used as a non-standard calling convention for
>> xc_cpuid_apply_policy() but that has been fixed now.
>>
>> Purge its use, and any possible confusion over its behaviour, by having Xen
>> reject any attempts to use it.  Forgo setting it up in libxl's
>> hvm_set_conf_params().  The only backwards compatibility necessary is to have
>> the HVM restore stream discard it if found.
> This looks plausible too.  But maybe I should be reading this patch
> and the previous one together ?  Or maybe they would be better
> squashed ?
>
> If you think that is likely to make me less confused I'm happy to try
> squashing them locally and reading the result...

I don't think that is going to help.  They are two logically different
changes.

Patch 5 fixes a libxl=>libxc api which has a (deliberate) side effect of
removing the sole use of HVM_PARAM_PAE_ENABLED.

This patch takes the final steps to remove HVM_PARAM_PAE_ENABLED from
use, everywhere.  This is partly to prevent ever regaining this knobble
on the CPUID handling side of things, and eventually to reduce memory
usage in Xen (by not allocating memory for obsolete params).

~Andrew

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

Re: [Xen-devel] [PATCH 5/6] tools/libx[cl]: Don't use HVM_PARAM_PAE_ENABLED as a function parameter

[Ian Jackson]

Andrew Cooper writes ("[PATCH 5/6] tools/libx[cl]: Don't use HVM_PARAM_PAE_ENABLED as a function parameter"):
> The sole use of HVM_PARAM_PAE_ENABLED is as a non-standard calling
> convention for xc_cpuid_apply_policy().  Pass PAE as a regular
> parameter instead.

Following our conversation on irc, I have tried to write an
explanation in my own words of what this commit is doing.

  The value of HVM_PARAM_PAE_ENABLED is set by the toolstack.  And the
  only place that reads it is also in the toolstack, in
  xc_cpuid_apply_policy.  Effectively, this hypervisor domain
  parameter is used solely as a way to pass this boolean parameter
  from one part of the toolstack to another.

  This is not sensible.

  Replace its use in xc_cpuid_apply_policy with a plain boolean
  parameter, passed directly by the one (in-tree) caller.
  The now-redundant code to set the value in the hypervisor will be
  deleted in the next patch.

> Leave a rather better explaination of why only HVM guests have a
> choice in PAE setting.

I approve of this part of the commit message.

> No functional change.

I would write

   No overall functional change.  The new code fior calculating the
   `pae' value (in libxl__cpuid_legacy) is isomorphic to the
   obselescent code (in libxl_x86.c).

I had a look to see whether this was true and it seemed to be.

>          /*
> -         * HVM_PARAM_PAE_ENABLED is a parameter to this function, stashed in
> -         * Xen.  Nothing else has ever taken notice of the value.
> +         * PAE used to be a parameter passed to this function by
> +         * HVM_PARAM_PAE_ENABLED.  It is now passed normally.
>           */

I find this phrasing confusing, particularly this very loose use of
the word `parameter'.  I would drop this comment entirely and let the
commit message stand as the historical documentation.

>      char *cpuid_res[4];
> +    bool pae = true;
> +
> +    /*
> +     * PAE is a Xen-controlled for PV guests (it is the 'p' that causes the
> +     * difference between the xen-3.0-x86_32 and xen-3.0-x86_32p ABIs).  It\
 is
> +     * mandatory as Xen is running in 64bit mode.
> +     *
> +     * PVH guests don't have a top-level PAE control, and is treated as
> +     * available.
> +     */

I approve of putting a new comment here with an explanation.  However,
it should be wrapped rather more tightly (eg, in my MUA it is now
suffering from wrap damage as I demonstrate above) and it seems to
have some problems with the grammar ?  And I think the 2nd sentence
"It is mandatory" could usefully be re-qualified "for PV guests".  Or
you could write something like this.

   PV guests: PAE is Xen-controlled (it is the 'p' that causes the
   difference between the xen-3.0-x86_32 and xen-3.0-x86_32p ABIs);
   Xen is in 64-bit mode so PAE is mandatory.

   PVH guests: there is no top-level PAE control in the libx domain
   config; we always make it available.

   So only this test only applies to HVM guests:

Thanks,
Ian.

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

[Xen-devel] [PATCH v2 5/6] tools/libx[cl]: Don't use HVM_PARAM_PAE_ENABLED as a function parameter

[Andrew Cooper]

HVM_PARAM_PAE_ENABLED is set and consumed by the toolstack only.  It is in
practice a complicated and non-standard way of passing a boolean parameter
into xc_cpuid_apply_policy().

This is silly.  Pass PAE as a regular parameter instead.

In libxl__cpuid_legacy(), leave a rather better explaination of why only HVM
guests have a choice in PAE setting.

No change in how a guest is constructed.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Ian Jackson <Ian.Jackson@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Anthony PERARD <anthony.perard@citrix.com>

v2:
 * Rewrite commit message and comments.
---
 tools/libxc/include/xenctrl.h |  2 +-
 tools/libxc/xc_cpuid_x86.c    | 15 +++------------
 tools/libxl/libxl_cpuid.c     | 16 +++++++++++++++-
 3 files changed, 19 insertions(+), 14 deletions(-)

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 311df1ef0f..4eb4f4c2c6 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -1807,7 +1807,7 @@ int xc_cpuid_set(xc_interface *xch,
 int xc_cpuid_apply_policy(xc_interface *xch,
                           uint32_t domid,
                           const uint32_t *featureset,
-                          unsigned int nr_features);
+                          unsigned int nr_features, bool pae);
 int xc_mca_op(xc_interface *xch, struct xen_mc *mc);
 int xc_mca_op_inject_v2(xc_interface *xch, unsigned int flags,
                         xc_cpumap_t cpumap, unsigned int nr_cpus);
diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c
index 2540aa1e1c..21b15b86ec 100644
--- a/tools/libxc/xc_cpuid_x86.c
+++ b/tools/libxc/xc_cpuid_x86.c
@@ -455,7 +455,8 @@ int xc_cpuid_set(
 }
 
 int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid,
-                          const uint32_t *featureset, unsigned int nr_features)
+                          const uint32_t *featureset, unsigned int nr_features,
+                          bool pae)
 {
     int rc;
     xc_dominfo_t di;
@@ -579,8 +580,6 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid,
     }
     else
     {
-        uint64_t val;
-
         /*
          * Topology for HVM guests is entirely controlled by Xen.  For now, we
          * hardcode APIC_ID = vcpu_id * 2 to give the illusion of no SMT.
@@ -634,15 +633,7 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid,
             break;
         }
 
-        /*
-         * HVM_PARAM_PAE_ENABLED is a parameter to this function, stashed in
-         * Xen.  Nothing else has ever taken notice of the value.
-         */
-        rc = xc_hvm_param_get(xch, domid, HVM_PARAM_PAE_ENABLED, &val);
-        if ( rc )
-            goto out;
-
-        p->basic.pae = val;
+        p->basic.pae = pae;
 
         /*
          * These settings are necessary to cause earlier HVM_PARAM_NESTEDHVM /
diff --git a/tools/libxl/libxl_cpuid.c b/tools/libxl/libxl_cpuid.c
index 49d3ca5b26..062750102e 100644
--- a/tools/libxl/libxl_cpuid.c
+++ b/tools/libxl/libxl_cpuid.c
@@ -416,8 +416,22 @@ void libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid,
     libxl_cpuid_policy_list cpuid = info->cpuid;
     int i;
     char *cpuid_res[4];
+    bool pae = true;
+
+    /*
+     * For PV guests, PAE is Xen-controlled (it is the 'p' that differentiates
+     * the xen-3.0-x86_32 and xen-3.0-x86_32p ABIs).  It is mandatory as Xen
+     * is 64bit only these days.
+     *
+     * For PVH guests, there is no top-level PAE control in the domain config,
+     * so is treated as always available.
+     *
+     * HVM guests get a top-level choice of whether PAE is available.
+     */
+    if (info->type == LIBXL_DOMAIN_TYPE_HVM)
+        pae = libxl_defbool_val(info->u.hvm.pae);
 
-    xc_cpuid_apply_policy(ctx->xch, domid, NULL, 0);
+    xc_cpuid_apply_policy(ctx->xch, domid, NULL, 0, pae);
 
     if (!cpuid)
         return;
-- 
2.11.0


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

Re: [Xen-devel] [PATCH v2 5/6] tools/libx[cl]: Don't use HVM_PARAM_PAE_ENABLED as a function parameter

[Ian Jackson]

Andrew Cooper writes ("[PATCH v2 5/6] tools/libx[cl]: Don't use HVM_PARAM_PAE_ENABLED as a function parameter"):
> HVM_PARAM_PAE_ENABLED is set and consumed by the toolstack only.  It is in
> practice a complicated and non-standard way of passing a boolean parameter
> into xc_cpuid_apply_policy().
> 
> This is silly.  Pass PAE as a regular parameter instead.
> 
> In libxl__cpuid_legacy(), leave a rather better explaination of why only HVM
> guests have a choice in PAE setting.
> 
> No change in how a guest is constructed.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Ian.

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