[PATCH 00/21] libs/guest: new CPUID/MSR interface

[PATCH 00/21] libs/guest: new CPUID/MSR interface

[Roger Pau Monne]

Hello,

The following series introduces a new CPUID/MSR interface for the
xenguest library. Such interface handles both CPUID and MSRs using the
same opaque object, and provides some helpers for the user to peek or
modify such data without exposing the backing type. This is useful for
future development as CPUID and MSRs are closely related, so it makes
handling those much easier if they are inside the same object (ie: a
change to a CPUID bit might expose or hide an MSR).

In this patch series libxl and other in tree users have been switched to
use the new interface, so it shouldn't result in any functional change
from a user point of view.

Note there are still some missing pieces likely. The way to modify CPUID
data is not ideal, as it requires fetching a leaf and modifying it
directly. We might want some kind of interface in order to set specific
CPUID features more easily, but that's to be discussed, and would be
done as a follow up series.

There are two functions kind of related that I've left alone:

int xc_get_cpu_levelling_caps(xc_interface *xch, uint32_t *caps);
int xc_get_cpu_featureset(xc_interface *xch, uint32_t index,
                          uint32_t *nr_features, uint32_t *featureset);

We might want to rename those to also use the xc_cpu_* prefix at least?
Using xc_cpu_policy_* would be wrong here IMO, as such functions don't
use the newly introduced xc_cpu_policy_t object.

Thanks, Roger.

Roger Pau Monne (21):
  libxl: don't ignore the return value from xc_cpuid_apply_policy
  libs/guest: rename xc_get_cpu_policy_size to xc_cpu_policy_get_size
  libs/guest: introduce xc_cpu_policy_t
  libs/guest: introduce helper to fetch a system cpu policy
  libs/guest: introduce helper to fetch a domain cpu policy
  libs/guest: introduce helper to serialize a cpu policy
  tools: switch existing users of xc_get_{system,domain}_cpu_policy
  libs/guest: introduce a helper to apply a cpu policy to a domain
  libs/guest: allow fetching a specific CPUID leaf from a cpu policy
  libs/guest: allow fetching a specific MSR entry from a cpu policy
  libs/guest: allow updating a cpu policy CPUID data
  libs/guest: allow updating a cpu policy MSR data
  libs/guest: switch users of xc_set_domain_cpu_policy
  libs/guest: introduce helper to check cpu policy compatibility
  libs/guest: obtain a compatible cpu policy from two input ones
  libs/guest: make a cpu policy compatible with older Xen versions
  libs/guest: introduce helper set cpu topology in cpu policy
  libs/guest: rework xc_cpuid_xend_policy
  libs/guest: apply a featureset into a cpu policy
  libs/{light,guest}: implement xc_cpuid_apply_policy in libxl
  libs/guest: (re)move xc_cpu_policy_apply_cpuid

 tools/include/libxl.h               |   4 +-
 tools/include/xen-tools/libs.h      |   5 +
 tools/include/xenctrl.h             | 107 ++--
 tools/libs/guest/Makefile           |   2 +-
 tools/libs/guest/xg_cpuid_x86.c     | 849 +++++++++++++++++++---------
 tools/libs/guest/xg_sr_common_x86.c |  45 +-
 tools/libs/light/libxl_cpuid.c      | 229 +++++++-
 tools/libs/light/libxl_create.c     |   5 +-
 tools/libs/light/libxl_dom.c        |   2 +-
 tools/libs/light/libxl_internal.h   |  32 +-
 tools/libs/light/libxl_nocpuid.c    |   5 +-
 tools/misc/xen-cpuid.c              |  23 +-
 12 files changed, 960 insertions(+), 348 deletions(-)

-- 
2.30.1

[PATCH 01/21] libxl: don't ignore the return value from xc_cpuid_apply_policy

[Roger Pau Monne]

Also change libxl__cpuid_legacy to propagate the error from
xc_cpuid_apply_policy into callers.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 tools/libs/light/libxl_cpuid.c    | 15 +++++++++++----
 tools/libs/light/libxl_create.c   |  5 +++--
 tools/libs/light/libxl_dom.c      |  2 +-
 tools/libs/light/libxl_internal.h |  4 ++--
 tools/libs/light/libxl_nocpuid.c  |  5 +++--
 5 files changed, 20 insertions(+), 11 deletions(-)

diff --git a/tools/libs/light/libxl_cpuid.c b/tools/libs/light/libxl_cpuid.c
index 289c59c7420..a7b33bbcd06 100644
--- a/tools/libs/light/libxl_cpuid.c
+++ b/tools/libs/light/libxl_cpuid.c
@@ -419,11 +419,13 @@ int libxl_cpuid_parse_config_xend(libxl_cpuid_policy_list *cpuid,
     return 0;
 }
 
-void libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid, bool restore,
-                         libxl_domain_build_info *info)
+int libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid, bool restore,
+                        libxl_domain_build_info *info)
 {
+    GC_INIT(ctx);
     bool pae = true;
     bool itsc;
+    int rc;
 
     /*
      * Gross hack.  Using libxl_defbool_val() here causes libvirt to crash in
@@ -462,8 +464,13 @@ void libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid, bool restore,
     itsc = (libxl_defbool_val(info->disable_migrate) ||
             info->tsc_mode == LIBXL_TSC_MODE_ALWAYS_EMULATE);
 
-    xc_cpuid_apply_policy(ctx->xch, domid, restore, NULL, 0,
-                          pae, itsc, nested_virt, info->cpuid);
+    rc = xc_cpuid_apply_policy(ctx->xch, domid, restore, NULL, 0,
+                               pae, itsc, nested_virt, info->cpuid);
+    if (rc)
+        LOGE(ERROR, "Failed to apply CPUID policy");
+
+    GC_FREE;
+    return rc;
 }
 
 static const char *input_names[2] = { "leaf", "subleaf" };
diff --git a/tools/libs/light/libxl_create.c b/tools/libs/light/libxl_create.c
index 1131b2a733e..3b7474979de 100644
--- a/tools/libs/light/libxl_create.c
+++ b/tools/libs/light/libxl_create.c
@@ -1443,6 +1443,7 @@ int libxl__srm_callout_callback_static_data_done(unsigned int missing,
 
     libxl_domain_config *d_config = dcs->guest_config;
     libxl_domain_build_info *info = &d_config->b_info;
+    int rc = 0;
 
     /*
      * CPUID/MSR information is not present in pre Xen-4.14 streams.
@@ -1452,9 +1453,9 @@ int libxl__srm_callout_callback_static_data_done(unsigned int missing,
      * stream doesn't contain any CPUID data.
      */
     if (missing & XGR_SDD_MISSING_CPUID)
-        libxl__cpuid_legacy(ctx, dcs->guest_domid, true, info);
+        rc = libxl__cpuid_legacy(ctx, dcs->guest_domid, true, info);
 
-    return 0;
+    return rc;
 }
 
 void libxl__srm_callout_callback_restore_results(xen_pfn_t store_mfn,
diff --git a/tools/libs/light/libxl_dom.c b/tools/libs/light/libxl_dom.c
index 842a51c86cb..e9f58ee4b2b 100644
--- a/tools/libs/light/libxl_dom.c
+++ b/tools/libs/light/libxl_dom.c
@@ -384,7 +384,7 @@ int libxl__build_pre(libxl__gc *gc, uint32_t domid,
      * being migrated-in/restored have CPUID handled during the
      * static_data_done() callback. */
     if (!state->restore)
-        libxl__cpuid_legacy(ctx, domid, false, info);
+        rc = libxl__cpuid_legacy(ctx, domid, false, info);
 
     return rc;
 }
diff --git a/tools/libs/light/libxl_internal.h b/tools/libs/light/libxl_internal.h
index 028bc013d9c..22b1775b752 100644
--- a/tools/libs/light/libxl_internal.h
+++ b/tools/libs/light/libxl_internal.h
@@ -2052,8 +2052,8 @@ typedef yajl_gen_status (*libxl__gen_json_callback)(yajl_gen hand, void *);
 _hidden char *libxl__object_to_json(libxl_ctx *ctx, const char *type,
                                     libxl__gen_json_callback gen, void *p);
 
-_hidden void libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid, bool retore,
-                                 libxl_domain_build_info *info);
+_hidden int libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid, bool retore,
+                                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/libs/light/libxl_nocpuid.c b/tools/libs/light/libxl_nocpuid.c
index f47336565b9..0630959e760 100644
--- a/tools/libs/light/libxl_nocpuid.c
+++ b/tools/libs/light/libxl_nocpuid.c
@@ -34,9 +34,10 @@ int libxl_cpuid_parse_config_xend(libxl_cpuid_policy_list *cpuid,
     return 0;
 }
 
-void libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid, bool restore,
-                         libxl_domain_build_info *info)
+int libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid, bool restore,
+                        libxl_domain_build_info *info)
 {
+    return 0;
 }
 
 yajl_gen_status libxl_cpuid_policy_list_gen_json(yajl_gen hand,
-- 
2.30.1

[PATCH 02/21] libs/guest: rename xc_get_cpu_policy_size to xc_cpu_policy_get_size

[Roger Pau Monne]

Preparatory change to introduce a new set of xc_cpu_policy_* functions
that will replace the current CPUID/MSR helpers.

No functional change intended.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 tools/include/xenctrl.h             | 2 +-
 tools/libs/guest/xg_cpuid_x86.c     | 6 +++---
 tools/libs/guest/xg_sr_common_x86.c | 2 +-
 tools/misc/xen-cpuid.c              | 2 +-
 4 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index 318920166c5..e91ff92b9b1 100644
--- a/tools/include/xenctrl.h
+++ b/tools/include/xenctrl.h
@@ -2594,7 +2594,7 @@ int xc_get_cpu_levelling_caps(xc_interface *xch, uint32_t *caps);
 int xc_get_cpu_featureset(xc_interface *xch, uint32_t index,
                           uint32_t *nr_features, uint32_t *featureset);
 
-int xc_get_cpu_policy_size(xc_interface *xch, uint32_t *nr_leaves,
+int xc_cpu_policy_get_size(xc_interface *xch, uint32_t *nr_leaves,
                            uint32_t *nr_msrs);
 int xc_get_system_cpu_policy(xc_interface *xch, uint32_t index,
                              uint32_t *nr_leaves, xen_cpuid_leaf_t *leaves,
diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
index aae6931a114..9846f81e1f1 100644
--- a/tools/libs/guest/xg_cpuid_x86.c
+++ b/tools/libs/guest/xg_cpuid_x86.c
@@ -109,7 +109,7 @@ const uint32_t *xc_get_static_cpu_featuremask(
     return masks[mask];
 }
 
-int xc_get_cpu_policy_size(xc_interface *xch, uint32_t *nr_leaves,
+int xc_cpu_policy_get_size(xc_interface *xch, uint32_t *nr_leaves,
                            uint32_t *nr_msrs)
 {
     struct xen_sysctl sysctl = {};
@@ -302,7 +302,7 @@ static int xc_cpuid_xend_policy(
         goto fail;
     }
 
-    rc = xc_get_cpu_policy_size(xch, &nr_leaves, &nr_msrs);
+    rc = xc_cpu_policy_get_size(xch, &nr_leaves, &nr_msrs);
     if ( rc )
     {
         PERROR("Failed to obtain policy info size");
@@ -447,7 +447,7 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
         goto out;
     }
 
-    rc = xc_get_cpu_policy_size(xch, &nr_leaves, &nr_msrs);
+    rc = xc_cpu_policy_get_size(xch, &nr_leaves, &nr_msrs);
     if ( rc )
     {
         PERROR("Failed to obtain policy info size");
diff --git a/tools/libs/guest/xg_sr_common_x86.c b/tools/libs/guest/xg_sr_common_x86.c
index 3168c5485fd..4982519e055 100644
--- a/tools/libs/guest/xg_sr_common_x86.c
+++ b/tools/libs/guest/xg_sr_common_x86.c
@@ -50,7 +50,7 @@ int write_x86_cpu_policy_records(struct xc_sr_context *ctx)
     uint32_t nr_leaves = 0, nr_msrs = 0;
     int rc;
 
-    if ( xc_get_cpu_policy_size(xch, &nr_leaves, &nr_msrs) < 0 )
+    if ( xc_cpu_policy_get_size(xch, &nr_leaves, &nr_msrs) < 0 )
     {
         PERROR("Unable to get CPU Policy size");
         return -1;
diff --git a/tools/misc/xen-cpuid.c b/tools/misc/xen-cpuid.c
index 2d04162d8d8..52596c08c90 100644
--- a/tools/misc/xen-cpuid.c
+++ b/tools/misc/xen-cpuid.c
@@ -462,7 +462,7 @@ int main(int argc, char **argv)
         if ( !xch )
             err(1, "xc_interface_open");
 
-        if ( xc_get_cpu_policy_size(xch, &max_leaves, &max_msrs) )
+        if ( xc_cpu_policy_get_size(xch, &max_leaves, &max_msrs) )
             err(1, "xc_get_cpu_policy_size(...)");
         if ( domid == -1 )
             printf("Xen reports there are maximum %u leaves and %u MSRs\n",
-- 
2.30.1

[PATCH 03/21] libs/guest: introduce xc_cpu_policy_t

[Roger Pau Monne]

Introduce an opaque type that is used to store the CPUID and MSRs
policies of a domain. Such type uses the existing cpu_policy structure
to store the data, but doesn't expose the type to the users of the
xenguest library.

Introduce an allocation (init) and freeing function (destroy) to
manage the type.

Note the type is not yet used anywhere.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 tools/include/xenctrl.h         |  6 ++++++
 tools/libs/guest/xg_cpuid_x86.c | 28 ++++++++++++++++++++++++++++
 2 files changed, 34 insertions(+)

diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index e91ff92b9b1..ffb3024bfeb 100644
--- a/tools/include/xenctrl.h
+++ b/tools/include/xenctrl.h
@@ -2590,6 +2590,12 @@ int xc_psr_get_domain_data(xc_interface *xch, uint32_t domid,
 int xc_psr_get_hw_info(xc_interface *xch, uint32_t socket,
                        xc_psr_feat_type type, xc_psr_hw_info *hw_info);
 
+typedef struct cpu_policy *xc_cpu_policy_t;
+
+/* Create and free a xc_cpu_policy object. */
+xc_cpu_policy_t xc_cpu_policy_init(void);
+void xc_cpu_policy_destroy(xc_cpu_policy_t policy);
+
 int xc_get_cpu_levelling_caps(xc_interface *xch, uint32_t *caps);
 int xc_get_cpu_featureset(xc_interface *xch, uint32_t index,
                           uint32_t *nr_features, uint32_t *featureset);
diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
index 9846f81e1f1..ade5281c178 100644
--- a/tools/libs/guest/xg_cpuid_x86.c
+++ b/tools/libs/guest/xg_cpuid_x86.c
@@ -659,3 +659,31 @@ out:
 
     return rc;
 }
+
+xc_cpu_policy_t xc_cpu_policy_init(void)
+{
+    xc_cpu_policy_t policy = calloc(1, sizeof(*policy));
+
+    if ( !policy )
+        return NULL;
+
+    policy->cpuid = calloc(1, sizeof(*policy->cpuid));
+    policy->msr = calloc(1, sizeof(*policy->msr));
+    if ( !policy->cpuid || !policy->msr )
+    {
+        xc_cpu_policy_destroy(policy);
+        return NULL;
+    }
+
+    return policy;
+}
+
+void xc_cpu_policy_destroy(xc_cpu_policy_t policy)
+{
+    if ( !policy )
+        return;
+
+    free(policy->cpuid);
+    free(policy->msr);
+    free(policy);
+}
-- 
2.30.1

[PATCH 04/21] libs/guest: introduce helper to fetch a system cpu policy

[Roger Pau Monne]

Such helper is based on the existing functions to fetch a CPUID and
MSR policies, but uses the xc_cpu_policy_t type to return the data to
the caller.

Note some helper functions are introduced, those are split from
xc_cpu_policy_get_system because they will be used by other functions
also.

No user of the interface introduced on the patch.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 tools/include/xenctrl.h         |  4 ++
 tools/libs/guest/xg_cpuid_x86.c | 90 +++++++++++++++++++++++++++++++++
 2 files changed, 94 insertions(+)

diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index ffb3024bfeb..fc8e4b28781 100644
--- a/tools/include/xenctrl.h
+++ b/tools/include/xenctrl.h
@@ -2596,6 +2596,10 @@ typedef struct cpu_policy *xc_cpu_policy_t;
 xc_cpu_policy_t xc_cpu_policy_init(void);
 void xc_cpu_policy_destroy(xc_cpu_policy_t policy);
 
+/* Retrieve a system policy, or get/set a domains policy. */
+int xc_cpu_policy_get_system(xc_interface *xch, unsigned int idx,
+                             xc_cpu_policy_t policy);
+
 int xc_get_cpu_levelling_caps(xc_interface *xch, uint32_t *caps);
 int xc_get_cpu_featureset(xc_interface *xch, uint32_t index,
                           uint32_t *nr_features, uint32_t *featureset);
diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
index ade5281c178..3710fb63839 100644
--- a/tools/libs/guest/xg_cpuid_x86.c
+++ b/tools/libs/guest/xg_cpuid_x86.c
@@ -687,3 +687,93 @@ void xc_cpu_policy_destroy(xc_cpu_policy_t policy)
     free(policy->msr);
     free(policy);
 }
+
+static int allocate_buffers(xc_interface *xch,
+                            unsigned int *nr_leaves, xen_cpuid_leaf_t **leaves,
+                            unsigned int *nr_msrs, xen_msr_entry_t **msrs)
+{
+    int rc;
+
+    *leaves = NULL;
+    *msrs = NULL;
+
+    rc = xc_cpu_policy_get_size(xch, nr_leaves, nr_msrs);
+    if ( rc )
+    {
+        PERROR("Failed to obtain policy info size");
+        return -errno;
+    }
+
+    *leaves = calloc(*nr_leaves, sizeof(**leaves));
+    *msrs = calloc(*nr_msrs, sizeof(**msrs));
+    if ( !*leaves || !*msrs )
+    {
+        PERROR("Failed to allocate resources");
+        free(*leaves);
+        free(*msrs);
+        return -ENOMEM;
+    }
+
+    return 0;
+}
+
+static int deserialize_policy(xc_interface *xch, xc_cpu_policy_t policy,
+                              unsigned int nr_leaves,
+                              const xen_cpuid_leaf_t *leaves,
+                              unsigned int nr_msrs, const xen_msr_entry_t *msrs)
+{
+    uint32_t err_leaf = -1, err_subleaf = -1, err_msr = -1;
+    int rc;
+
+    rc = x86_cpuid_copy_from_buffer(policy->cpuid, leaves, nr_leaves,
+                                    &err_leaf, &err_subleaf);
+    if ( rc )
+    {
+        ERROR("Failed to deserialise CPUID (err leaf %#x, subleaf %#x) (%d = %s)",
+              err_leaf, err_subleaf, -rc, strerror(-rc));
+        return rc;
+    }
+
+    rc = x86_msr_copy_from_buffer(policy->msr, msrs, nr_msrs, &err_msr);
+    if ( rc )
+    {
+        ERROR("Failed to deserialise MSR (err MSR %#x) (%d = %s)",
+              err_msr, -rc, strerror(-rc));
+        return rc;
+    }
+
+    return 0;
+}
+
+int xc_cpu_policy_get_system(xc_interface *xch, unsigned int idx,
+                             xc_cpu_policy_t policy)
+{
+    unsigned int nr_leaves, nr_msrs;
+    xen_cpuid_leaf_t *leaves = NULL;
+    xen_msr_entry_t *msrs = NULL;
+    int rc;
+
+    rc = allocate_buffers(xch, &nr_leaves, &leaves, &nr_msrs, &msrs);
+    if ( rc )
+    {
+        errno = -rc;
+        return -1;
+    }
+
+    rc = xc_get_system_cpu_policy(xch, idx, &nr_leaves, leaves, &nr_msrs, msrs);
+    if ( rc )
+    {
+        PERROR("Failed to obtain %u policy", idx);
+        rc = -1;
+        goto out;
+    }
+
+    rc = deserialize_policy(xch, policy, nr_leaves, leaves, nr_msrs, msrs);
+    if ( rc )
+        errno = -rc;
+
+ out:
+    free(leaves);
+    free(msrs);
+    return rc;
+}
-- 
2.30.1

[PATCH 05/21] libs/guest: introduce helper to fetch a domain cpu policy

[Roger Pau Monne]

Such helper is based on the existing functions to fetch a CPUID and
MSR policies, but uses the xc_cpu_policy_t type to return the data to
the caller.

No user of the interface introduced on the patch.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 tools/include/xenctrl.h         |  2 ++
 tools/libs/guest/xg_cpuid_x86.c | 34 +++++++++++++++++++++++++++++++++
 2 files changed, 36 insertions(+)

diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index fc8e4b28781..8b8b30a2764 100644
--- a/tools/include/xenctrl.h
+++ b/tools/include/xenctrl.h
@@ -2599,6 +2599,8 @@ void xc_cpu_policy_destroy(xc_cpu_policy_t policy);
 /* Retrieve a system policy, or get/set a domains policy. */
 int xc_cpu_policy_get_system(xc_interface *xch, unsigned int idx,
                              xc_cpu_policy_t policy);
+int xc_cpu_policy_get_domain(xc_interface *xch, uint32_t domid,
+                             xc_cpu_policy_t policy);
 
 int xc_get_cpu_levelling_caps(xc_interface *xch, uint32_t *caps);
 int xc_get_cpu_featureset(xc_interface *xch, uint32_t index,
diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
index 3710fb63839..75ac70996ac 100644
--- a/tools/libs/guest/xg_cpuid_x86.c
+++ b/tools/libs/guest/xg_cpuid_x86.c
@@ -777,3 +777,37 @@ int xc_cpu_policy_get_system(xc_interface *xch, unsigned int idx,
     free(msrs);
     return rc;
 }
+
+int xc_cpu_policy_get_domain(xc_interface *xch, uint32_t domid,
+                             xc_cpu_policy_t policy)
+{
+    unsigned int nr_leaves, nr_msrs;
+    xen_cpuid_leaf_t *leaves = NULL;
+    xen_msr_entry_t *msrs = NULL;
+    int rc;
+
+    rc = allocate_buffers(xch, &nr_leaves, &leaves, &nr_msrs, &msrs);
+    if ( rc )
+    {
+        errno = -rc;
+        return -1;
+    }
+
+    rc = xc_get_domain_cpu_policy(xch, domid, &nr_leaves, leaves, &nr_msrs,
+                                  msrs);
+    if ( rc )
+    {
+        PERROR("Failed to obtain domain %u policy", domid);
+        rc = -1;
+        goto out;
+    }
+
+    rc = deserialize_policy(xch, policy, nr_leaves, leaves, nr_msrs, msrs);
+    if ( rc )
+        errno = -rc;
+
+ out:
+    free(leaves);
+    free(msrs);
+    return rc;
+}
-- 
2.30.1

[PATCH 06/21] libs/guest: introduce helper to serialize a cpu policy

[Roger Pau Monne]

Such helper allow converting a cpu policy into an array of
xen_cpuid_leaf_t and xen_msr_entry_t elements, which matches the
current interface of the CPUID/MSR functions. This is required in
order for the user to be able to parse the CPUID/MSR data.

No user of the interface introduced in this patch.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 tools/include/xenctrl.h         |  5 +++++
 tools/libs/guest/xg_cpuid_x86.c | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 37 insertions(+)

diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index 8b8b30a2764..983bb027a04 100644
--- a/tools/include/xenctrl.h
+++ b/tools/include/xenctrl.h
@@ -2602,6 +2602,11 @@ int xc_cpu_policy_get_system(xc_interface *xch, unsigned int idx,
 int xc_cpu_policy_get_domain(xc_interface *xch, uint32_t domid,
                              xc_cpu_policy_t policy);
 
+/* Manipulate a policy via architectural representations. */
+int xc_cpu_policy_serialise(xc_interface *xch, const xc_cpu_policy_t policy,
+                            xen_cpuid_leaf_t *leaves, uint32_t *nr_leaves,
+                            xen_msr_entry_t *msrs, uint32_t *nr_msrs);
+
 int xc_get_cpu_levelling_caps(xc_interface *xch, uint32_t *caps);
 int xc_get_cpu_featureset(xc_interface *xch, uint32_t index,
                           uint32_t *nr_features, uint32_t *featureset);
diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
index 75ac70996ac..812ef14fbcd 100644
--- a/tools/libs/guest/xg_cpuid_x86.c
+++ b/tools/libs/guest/xg_cpuid_x86.c
@@ -811,3 +811,35 @@ int xc_cpu_policy_get_domain(xc_interface *xch, uint32_t domid,
     free(msrs);
     return rc;
 }
+
+int xc_cpu_policy_serialise(xc_interface *xch, const xc_cpu_policy_t p,
+                            xen_cpuid_leaf_t *leaves, uint32_t *nr_leaves,
+                            xen_msr_entry_t *msrs, uint32_t *nr_msrs)
+{
+    int rc;
+
+    if ( leaves )
+    {
+        rc = x86_cpuid_copy_to_buffer(p->cpuid, leaves, nr_leaves);
+        if ( rc )
+        {
+            ERROR("Failed to serialize CPUID policy");
+            errno = -rc;
+            return -1;
+        }
+    }
+
+    if ( msrs )
+    {
+        rc = x86_msr_copy_to_buffer(p->msr, msrs, nr_msrs);
+        if ( rc )
+        {
+            ERROR("Failed to serialize MSR policy");
+            errno = -rc;
+            return -1;
+        }
+    }
+
+    errno = 0;
+    return 0;
+}
-- 
2.30.1

[PATCH 07/21] tools: switch existing users of xc_get_{system,domain}_cpu_policy

[Roger Pau Monne]

With the introduction of xc_cpu_policy_get_{system,domain} and
xc_cpu_policy_serialise the current users of
xc_get_{system,domain}_cpu_policy can be switched to the new
interface.

Note that xc_get_{system,domain}_cpu_policy is removed from the public
interface and the functions are made static, since there are still
internal consumers in xg_cpuid_x86.c

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 tools/include/xenctrl.h             |  6 -----
 tools/libs/guest/xg_cpuid_x86.c     | 36 ++++++++++++++---------------
 tools/libs/guest/xg_sr_common_x86.c | 15 +++++++++---
 tools/misc/xen-cpuid.c              | 21 +++++++++++------
 4 files changed, 44 insertions(+), 34 deletions(-)

diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index 983bb027a04..161dafd234b 100644
--- a/tools/include/xenctrl.h
+++ b/tools/include/xenctrl.h
@@ -2613,12 +2613,6 @@ int xc_get_cpu_featureset(xc_interface *xch, uint32_t index,
 
 int xc_cpu_policy_get_size(xc_interface *xch, uint32_t *nr_leaves,
                            uint32_t *nr_msrs);
-int xc_get_system_cpu_policy(xc_interface *xch, uint32_t index,
-                             uint32_t *nr_leaves, xen_cpuid_leaf_t *leaves,
-                             uint32_t *nr_msrs, xen_msr_entry_t *msrs);
-int xc_get_domain_cpu_policy(xc_interface *xch, uint32_t domid,
-                             uint32_t *nr_leaves, xen_cpuid_leaf_t *leaves,
-                             uint32_t *nr_msrs, xen_msr_entry_t *msrs);
 int xc_set_domain_cpu_policy(xc_interface *xch, uint32_t domid,
                              uint32_t nr_leaves, xen_cpuid_leaf_t *leaves,
                              uint32_t nr_msrs, xen_msr_entry_t *msrs,
diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
index 812ef14fbcd..a8133d5cd3f 100644
--- a/tools/libs/guest/xg_cpuid_x86.c
+++ b/tools/libs/guest/xg_cpuid_x86.c
@@ -128,9 +128,9 @@ int xc_cpu_policy_get_size(xc_interface *xch, uint32_t *nr_leaves,
     return ret;
 }
 
-int xc_get_system_cpu_policy(xc_interface *xch, uint32_t index,
-                             uint32_t *nr_leaves, xen_cpuid_leaf_t *leaves,
-                             uint32_t *nr_msrs, xen_msr_entry_t *msrs)
+static int get_system_cpu_policy(xc_interface *xch, uint32_t index,
+                                 uint32_t *nr_leaves, xen_cpuid_leaf_t *leaves,
+                                 uint32_t *nr_msrs, xen_msr_entry_t *msrs)
 {
     struct xen_sysctl sysctl = {};
     DECLARE_HYPERCALL_BOUNCE(leaves,
@@ -166,9 +166,9 @@ int xc_get_system_cpu_policy(xc_interface *xch, uint32_t index,
     return ret;
 }
 
-int xc_get_domain_cpu_policy(xc_interface *xch, uint32_t domid,
-                             uint32_t *nr_leaves, xen_cpuid_leaf_t *leaves,
-                             uint32_t *nr_msrs, xen_msr_entry_t *msrs)
+static int get_domain_cpu_policy(xc_interface *xch, uint32_t domid,
+                                 uint32_t *nr_leaves, xen_cpuid_leaf_t *leaves,
+                                 uint32_t *nr_msrs, xen_msr_entry_t *msrs)
 {
     DECLARE_DOMCTL;
     DECLARE_HYPERCALL_BOUNCE(leaves,
@@ -322,7 +322,7 @@ static int xc_cpuid_xend_policy(
     /* Get the domain's current policy. */
     nr_msrs = 0;
     nr_cur = nr_leaves;
-    rc = xc_get_domain_cpu_policy(xch, domid, &nr_cur, cur, &nr_msrs, NULL);
+    rc = get_domain_cpu_policy(xch, domid, &nr_cur, cur, &nr_msrs, NULL);
     if ( rc )
     {
         PERROR("Failed to obtain d%d current policy", domid);
@@ -333,9 +333,9 @@ static int xc_cpuid_xend_policy(
     /* Get the domain's max policy. */
     nr_msrs = 0;
     nr_max = nr_leaves;
-    rc = xc_get_system_cpu_policy(xch, di.hvm ? XEN_SYSCTL_cpu_policy_hvm_max
-                                              : XEN_SYSCTL_cpu_policy_pv_max,
-                                  &nr_max, max, &nr_msrs, NULL);
+    rc = get_system_cpu_policy(xch, di.hvm ? XEN_SYSCTL_cpu_policy_hvm_max
+                                           : XEN_SYSCTL_cpu_policy_pv_max,
+                               &nr_max, max, &nr_msrs, NULL);
     if ( rc )
     {
         PERROR("Failed to obtain %s max policy", di.hvm ? "hvm" : "pv");
@@ -346,8 +346,8 @@ static int xc_cpuid_xend_policy(
     /* Get the host policy. */
     nr_msrs = 0;
     nr_host = nr_leaves;
-    rc = xc_get_system_cpu_policy(xch, XEN_SYSCTL_cpu_policy_host,
-                                  &nr_host, host, &nr_msrs, NULL);
+    rc = get_system_cpu_policy(xch, XEN_SYSCTL_cpu_policy_host,
+                               &nr_host, host, &nr_msrs, NULL);
     if ( rc )
     {
         PERROR("Failed to obtain host policy");
@@ -478,9 +478,9 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
 
     /* Get the domain's default policy. */
     nr_msrs = 0;
-    rc = xc_get_system_cpu_policy(xch, di.hvm ? XEN_SYSCTL_cpu_policy_hvm_default
-                                              : XEN_SYSCTL_cpu_policy_pv_default,
-                                  &nr_leaves, leaves, &nr_msrs, NULL);
+    rc = get_system_cpu_policy(xch, di.hvm ? XEN_SYSCTL_cpu_policy_hvm_default
+                                           : XEN_SYSCTL_cpu_policy_pv_default,
+                               &nr_leaves, leaves, &nr_msrs, NULL);
     if ( rc )
     {
         PERROR("Failed to obtain %s default policy", di.hvm ? "hvm" : "pv");
@@ -760,7 +760,7 @@ int xc_cpu_policy_get_system(xc_interface *xch, unsigned int idx,
         return -1;
     }
 
-    rc = xc_get_system_cpu_policy(xch, idx, &nr_leaves, leaves, &nr_msrs, msrs);
+    rc = get_system_cpu_policy(xch, idx, &nr_leaves, leaves, &nr_msrs, msrs);
     if ( rc )
     {
         PERROR("Failed to obtain %u policy", idx);
@@ -793,8 +793,8 @@ int xc_cpu_policy_get_domain(xc_interface *xch, uint32_t domid,
         return -1;
     }
 
-    rc = xc_get_domain_cpu_policy(xch, domid, &nr_leaves, leaves, &nr_msrs,
-                                  msrs);
+    rc = get_domain_cpu_policy(xch, domid, &nr_leaves, leaves, &nr_msrs,
+                               msrs);
     if ( rc )
     {
         PERROR("Failed to obtain domain %u policy", domid);
diff --git a/tools/libs/guest/xg_sr_common_x86.c b/tools/libs/guest/xg_sr_common_x86.c
index 4982519e055..15265e7a331 100644
--- a/tools/libs/guest/xg_sr_common_x86.c
+++ b/tools/libs/guest/xg_sr_common_x86.c
@@ -48,6 +48,7 @@ int write_x86_cpu_policy_records(struct xc_sr_context *ctx)
     struct xc_sr_record cpuid = { .type = REC_TYPE_X86_CPUID_POLICY, };
     struct xc_sr_record msrs  = { .type = REC_TYPE_X86_MSR_POLICY, };
     uint32_t nr_leaves = 0, nr_msrs = 0;
+    xc_cpu_policy_t policy = NULL;
     int rc;
 
     if ( xc_cpu_policy_get_size(xch, &nr_leaves, &nr_msrs) < 0 )
@@ -58,20 +59,27 @@ int write_x86_cpu_policy_records(struct xc_sr_context *ctx)
 
     cpuid.data = malloc(nr_leaves * sizeof(xen_cpuid_leaf_t));
     msrs.data  = malloc(nr_msrs   * sizeof(xen_msr_entry_t));
-    if ( !cpuid.data || !msrs.data )
+    policy = xc_cpu_policy_init();
+    if ( !cpuid.data || !msrs.data || !policy )
     {
         ERROR("Cannot allocate memory for CPU Policy");
         rc = -1;
         goto out;
     }
 
-    if ( xc_get_domain_cpu_policy(xch, ctx->domid, &nr_leaves, cpuid.data,
-                                  &nr_msrs, msrs.data) )
+    if ( xc_cpu_policy_get_domain(xch, ctx->domid, policy) )
     {
         PERROR("Unable to get d%d CPU Policy", ctx->domid);
         rc = -1;
         goto out;
     }
+    if ( xc_cpu_policy_serialise(xch, policy, cpuid.data, &nr_leaves,
+                                 msrs.data, &nr_msrs) )
+    {
+        PERROR("Unable to serialize d%d CPU Policy", ctx->domid);
+        rc = -1;
+        goto out;
+    }
 
     cpuid.length = nr_leaves * sizeof(xen_cpuid_leaf_t);
     if ( cpuid.length )
@@ -94,6 +102,7 @@ int write_x86_cpu_policy_records(struct xc_sr_context *ctx)
  out:
     free(cpuid.data);
     free(msrs.data);
+    xc_cpu_policy_destroy(policy);
 
     return rc;
 }
diff --git a/tools/misc/xen-cpuid.c b/tools/misc/xen-cpuid.c
index 52596c08c90..8ac25581d68 100644
--- a/tools/misc/xen-cpuid.c
+++ b/tools/misc/xen-cpuid.c
@@ -458,9 +458,12 @@ int main(int argc, char **argv)
         uint32_t i, max_leaves, max_msrs;
 
         xc_interface *xch = xc_interface_open(0, 0, 0);
+        xc_cpu_policy_t policy = xc_cpu_policy_init();
 
         if ( !xch )
             err(1, "xc_interface_open");
+        if ( !policy )
+            err(1, "xc_cpu_policy_init");
 
         if ( xc_cpu_policy_get_size(xch, &max_leaves, &max_msrs) )
             err(1, "xc_get_cpu_policy_size(...)");
@@ -481,10 +484,11 @@ int main(int argc, char **argv)
             uint32_t nr_leaves = max_leaves;
             uint32_t nr_msrs = max_msrs;
 
-            if ( xc_get_domain_cpu_policy(xch, domid, &nr_leaves, leaves,
-                                          &nr_msrs, msrs) )
-                err(1, "xc_get_domain_cpu_policy(, %d, %d,, %d,)",
-                    domid, nr_leaves, nr_msrs);
+            if ( xc_cpu_policy_get_domain(xch, domid, policy) )
+                err(1, "xc_cpu_policy_get_domain(, %d, )", domid);
+            if ( xc_cpu_policy_serialise(xch, policy, leaves, &nr_leaves,
+                                         msrs, &nr_msrs) )
+                err(1, "xc_cpu_policy_serialise");
 
             snprintf(name, sizeof(name), "Domain %d", domid);
             print_policy(name, leaves, nr_leaves, msrs, nr_msrs);
@@ -497,8 +501,7 @@ int main(int argc, char **argv)
                 uint32_t nr_leaves = max_leaves;
                 uint32_t nr_msrs = max_msrs;
 
-                if ( xc_get_system_cpu_policy(xch, i, &nr_leaves, leaves,
-                                              &nr_msrs, msrs) )
+                if ( xc_cpu_policy_get_system(xch, i, policy) )
                 {
                     if ( errno == EOPNOTSUPP )
                     {
@@ -507,14 +510,18 @@ int main(int argc, char **argv)
                         continue;
                     }
 
-                    err(1, "xc_get_system_cpu_policy(, %s,,)", sys_policies[i]);
+                    err(1, "xc_cpu_policy_get_system(, %s, )", sys_policies[i]);
                 }
+                if ( xc_cpu_policy_serialise(xch, policy, leaves, &nr_leaves,
+                                             msrs, &nr_msrs) )
+                    err(1, "xc_cpu_policy_serialise");
 
                 print_policy(sys_policies[i], leaves, nr_leaves,
                              msrs, nr_msrs);
             }
         }
 
+        xc_cpu_policy_destroy(policy);
         free(leaves);
         free(msrs);
         xc_interface_close(xch);
-- 
2.30.1

[PATCH 08/21] libs/guest: introduce a helper to apply a cpu policy to a domain

[Roger Pau Monne]

Such helper is very similar to the existing xc_set_domain_cpu_policy
interface, but takes an opaque xc_cpu_policy_t instead of arrays of
CPUID leaves and MSRs.

No callers of the interface introduced in this patch.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 tools/include/xenctrl.h         |  2 ++
 tools/libs/guest/xg_cpuid_x86.c | 40 +++++++++++++++++++++++++++++++++
 2 files changed, 42 insertions(+)

diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index 161dafd234b..d82c99b2f0d 100644
--- a/tools/include/xenctrl.h
+++ b/tools/include/xenctrl.h
@@ -2601,6 +2601,8 @@ int xc_cpu_policy_get_system(xc_interface *xch, unsigned int idx,
                              xc_cpu_policy_t policy);
 int xc_cpu_policy_get_domain(xc_interface *xch, uint32_t domid,
                              xc_cpu_policy_t policy);
+int xc_cpu_policy_set_domain(xc_interface *xch, uint32_t domid,
+                             const xc_cpu_policy_t policy);
 
 /* Manipulate a policy via architectural representations. */
 int xc_cpu_policy_serialise(xc_interface *xch, const xc_cpu_policy_t policy,
diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
index a8133d5cd3f..48351f1c4c6 100644
--- a/tools/libs/guest/xg_cpuid_x86.c
+++ b/tools/libs/guest/xg_cpuid_x86.c
@@ -812,6 +812,46 @@ int xc_cpu_policy_get_domain(xc_interface *xch, uint32_t domid,
     return rc;
 }
 
+int xc_cpu_policy_set_domain(xc_interface *xch, uint32_t domid,
+                             const xc_cpu_policy_t policy)
+{
+    uint32_t err_leaf = -1, err_subleaf = -1, err_msr = -1;
+    unsigned int nr_leaves, nr_msrs;
+    xen_cpuid_leaf_t *leaves = NULL;
+    xen_msr_entry_t *msrs = NULL;
+    int rc;
+
+    rc = allocate_buffers(xch, &nr_leaves, &leaves, &nr_msrs, &msrs);
+    if ( rc )
+    {
+        errno = -rc;
+        return -1;
+    }
+
+    rc = xc_cpu_policy_serialise(xch, policy, leaves, &nr_leaves,
+                                 msrs, &nr_msrs);
+    if ( rc )
+        goto out;
+
+    rc = xc_set_domain_cpu_policy(xch, domid, nr_leaves, leaves, nr_msrs, msrs,
+                                  &err_leaf, &err_subleaf, &err_msr);
+    if ( rc )
+    {
+        ERROR("Failed to set domain %u policy (%d = %s)", domid, -rc,
+              strerror(-rc));
+        if ( err_leaf != -1 )
+            ERROR("CPUID leaf %u subleaf %u", err_leaf, err_subleaf);
+        if ( err_msr != -1 )
+            ERROR("MSR index %#x\n", err_msr);
+        goto out;
+    }
+
+ out:
+    free(leaves);
+    free(msrs);
+    return rc;
+}
+
 int xc_cpu_policy_serialise(xc_interface *xch, const xc_cpu_policy_t p,
                             xen_cpuid_leaf_t *leaves, uint32_t *nr_leaves,
                             xen_msr_entry_t *msrs, uint32_t *nr_msrs)
-- 
2.30.1

[PATCH 09/21] libs/guest: allow fetching a specific CPUID leaf from a cpu policy

[Roger Pau Monne]

Introduce an interface that returns a specific leaf/subleaf from a cpu
policy in xen_cpuid_leaf_t format.

This is useful to callers can peek data from the opaque
xc_cpu_policy_t type.

No caller of the interface introduced on this patch.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 tools/include/xenctrl.h         |  3 +++
 tools/libs/guest/xg_cpuid_x86.c | 42 +++++++++++++++++++++++++++++++++
 2 files changed, 45 insertions(+)

diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index d82c99b2f0d..983e4c11d93 100644
--- a/tools/include/xenctrl.h
+++ b/tools/include/xenctrl.h
@@ -2608,6 +2608,9 @@ int xc_cpu_policy_set_domain(xc_interface *xch, uint32_t domid,
 int xc_cpu_policy_serialise(xc_interface *xch, const xc_cpu_policy_t policy,
                             xen_cpuid_leaf_t *leaves, uint32_t *nr_leaves,
                             xen_msr_entry_t *msrs, uint32_t *nr_msrs);
+int xc_cpu_policy_get_cpuid(xc_interface *xch, const xc_cpu_policy_t policy,
+                            uint32_t leaf, uint32_t subleaf,
+                            xen_cpuid_leaf_t *out);
 
 int xc_get_cpu_levelling_caps(xc_interface *xch, uint32_t *caps);
 int xc_get_cpu_featureset(xc_interface *xch, uint32_t index,
diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
index 48351f1c4c6..a1e1bf10d5c 100644
--- a/tools/libs/guest/xg_cpuid_x86.c
+++ b/tools/libs/guest/xg_cpuid_x86.c
@@ -883,3 +883,45 @@ int xc_cpu_policy_serialise(xc_interface *xch, const xc_cpu_policy_t p,
     errno = 0;
     return 0;
 }
+
+int xc_cpu_policy_get_cpuid(xc_interface *xch, const xc_cpu_policy_t policy,
+                            uint32_t leaf, uint32_t subleaf,
+                            xen_cpuid_leaf_t *out)
+{
+    unsigned int nr_leaves, nr_msrs, i;
+    xen_cpuid_leaf_t *leaves;
+    int rc = xc_cpu_policy_get_size(xch, &nr_leaves, &nr_msrs);
+
+    if ( rc )
+    {
+        PERROR("Failed to obtain policy info size");
+        return -1;
+    }
+
+    leaves = calloc(nr_leaves, sizeof(*leaves));
+    if ( !leaves )
+    {
+        PERROR("Failed to allocate resources");
+        errno = ENOMEM;
+        return -1;
+    }
+
+    rc = xc_cpu_policy_serialise(xch, policy, leaves, &nr_leaves, NULL, 0);
+    if ( rc )
+        goto out;
+
+    for ( i = 0; i < nr_leaves; i++ )
+        if ( leaves[i].leaf == leaf && leaves[i].subleaf == subleaf )
+        {
+            *out = leaves[i];
+            goto out;
+        }
+
+    /* Unable to find a matching leaf. */
+    errno = ENOENT;
+    rc = -1;
+
+ out:
+    free(leaves);
+    return rc;
+}
-- 
2.30.1

[PATCH 10/21] libs/guest: allow fetching a specific MSR entry from a cpu policy

[Roger Pau Monne]

Introduce an interface that returns a specific MSR entry from a cpu
policy in xen_msr_entry_t format.

This is useful to callers can peek data from the opaque
xc_cpu_policy_t type.

No caller of the interface introduced on this patch.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 tools/include/xenctrl.h         |  2 ++
 tools/libs/guest/xg_cpuid_x86.c | 41 +++++++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+)

diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index 983e4c11d93..ab34df1dc98 100644
--- a/tools/include/xenctrl.h
+++ b/tools/include/xenctrl.h
@@ -2611,6 +2611,8 @@ int xc_cpu_policy_serialise(xc_interface *xch, const xc_cpu_policy_t policy,
 int xc_cpu_policy_get_cpuid(xc_interface *xch, const xc_cpu_policy_t policy,
                             uint32_t leaf, uint32_t subleaf,
                             xen_cpuid_leaf_t *out);
+int xc_cpu_policy_get_msr(xc_interface *xch, const xc_cpu_policy_t policy,
+                          uint32_t msr, xen_msr_entry_t *out);
 
 int xc_get_cpu_levelling_caps(xc_interface *xch, uint32_t *caps);
 int xc_get_cpu_featureset(xc_interface *xch, uint32_t index,
diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
index a1e1bf10d5c..091aeb70c9c 100644
--- a/tools/libs/guest/xg_cpuid_x86.c
+++ b/tools/libs/guest/xg_cpuid_x86.c
@@ -925,3 +925,44 @@ int xc_cpu_policy_get_cpuid(xc_interface *xch, const xc_cpu_policy_t policy,
     free(leaves);
     return rc;
 }
+
+int xc_cpu_policy_get_msr(xc_interface *xch, const xc_cpu_policy_t policy,
+                          uint32_t msr, xen_msr_entry_t *out)
+{
+    unsigned int nr_leaves, nr_msrs, i;
+    xen_msr_entry_t *msrs;
+    int rc = xc_cpu_policy_get_size(xch, &nr_leaves, &nr_msrs);
+
+    if ( rc )
+    {
+        PERROR("Failed to obtain policy info size");
+        return -1;
+    }
+
+    msrs = calloc(nr_msrs, sizeof(*msrs));
+    if ( !msrs )
+    {
+        PERROR("Failed to allocate resources");
+        errno = ENOMEM;
+        return -1;
+    }
+
+    rc = xc_cpu_policy_serialise(xch, policy, NULL, 0, msrs, &nr_msrs);
+    if ( rc )
+        goto out;
+
+    for ( i = 0; i < nr_msrs; i++ )
+        if ( msrs[i].idx == msr )
+        {
+            *out = msrs[i];
+            goto out;
+        }
+
+    /* Unable to find a matching MSR. */
+    errno = ENOENT;
+    rc = -1;
+
+ out:
+    free(msrs);
+    return rc;
+}
-- 
2.30.1

[PATCH 11/21] libs/guest: allow updating a cpu policy CPUID data

[Roger Pau Monne]

Introduce a helper to update the CPUID policy using an array of
xen_cpuid_leaf_t entries. Note the leaves present in the input
xen_cpuid_leaf_t array will replace any existing leaves on the policy.

No user of the interface introduced on this patch.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 tools/include/xenctrl.h         |  3 ++
 tools/libs/guest/xg_cpuid_x86.c | 67 +++++++++++++++++++++++++++++++++
 2 files changed, 70 insertions(+)

diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index ab34df1dc98..2143478fe4b 100644
--- a/tools/include/xenctrl.h
+++ b/tools/include/xenctrl.h
@@ -2613,6 +2613,9 @@ int xc_cpu_policy_get_cpuid(xc_interface *xch, const xc_cpu_policy_t policy,
                             xen_cpuid_leaf_t *out);
 int xc_cpu_policy_get_msr(xc_interface *xch, const xc_cpu_policy_t policy,
                           uint32_t msr, xen_msr_entry_t *out);
+int xc_cpu_policy_update_cpuid(xc_interface *xch, xc_cpu_policy_t policy,
+                               const xen_cpuid_leaf_t *leaves,
+                               uint32_t nr);
 
 int xc_get_cpu_levelling_caps(xc_interface *xch, uint32_t *caps);
 int xc_get_cpu_featureset(xc_interface *xch, uint32_t index,
diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
index 091aeb70c9c..13c2972ccd3 100644
--- a/tools/libs/guest/xg_cpuid_x86.c
+++ b/tools/libs/guest/xg_cpuid_x86.c
@@ -966,3 +966,70 @@ int xc_cpu_policy_get_msr(xc_interface *xch, const xc_cpu_policy_t policy,
     free(msrs);
     return rc;
 }
+
+int xc_cpu_policy_update_cpuid(xc_interface *xch, xc_cpu_policy_t policy,
+                               const xen_cpuid_leaf_t *leaves,
+                               uint32_t nr)
+{
+    unsigned int err_leaf = -1, err_subleaf = -1;
+    unsigned int nr_leaves, nr_msrs, i, j;
+    xen_cpuid_leaf_t *current;
+    int rc = xc_cpu_policy_get_size(xch, &nr_leaves, &nr_msrs);
+
+    if ( rc )
+    {
+        PERROR("Failed to obtain policy info size");
+        return -1;
+    }
+
+    current = calloc(nr_leaves, sizeof(*current));
+    if ( !current )
+    {
+        PERROR("Failed to allocate resources");
+        errno = ENOMEM;
+        return -1;
+    }
+
+    rc = xc_cpu_policy_serialise(xch, policy, current, &nr_leaves, NULL, 0);
+    if ( rc )
+        goto out;
+
+    for ( i = 0; i < nr; i++ )
+    {
+        const xen_cpuid_leaf_t *update = &leaves[i];
+
+        for ( j = 0; j < nr_leaves; j++ )
+            if ( current[j].leaf == update->leaf &&
+                 current[j].subleaf == update->subleaf )
+            {
+                /*
+                 * NB: cannot use an assignation because of the const vs
+                 * non-const difference.
+                 */
+                memcpy(&current[j], update, sizeof(*update));
+                break;
+            }
+
+        if ( j == nr_leaves )
+        {
+            /* Failed to find a matching leaf, append to the end. */
+            current = realloc(current, (nr_leaves + 1) * sizeof(*current));
+            memcpy(&current[nr_leaves], update, sizeof(*update));
+            nr_leaves++;
+        }
+    }
+
+    rc = x86_cpuid_copy_from_buffer(policy->cpuid, current, nr_leaves,
+                                    &err_leaf, &err_subleaf);
+    if ( rc )
+    {
+        ERROR("Failed to deserialise CPUID (err leaf %#x, subleaf %#x) (%d = %s)",
+              err_leaf, err_subleaf, -rc, strerror(-rc));
+        errno = -rc;
+        rc = -1;
+    }
+
+ out:
+    free(current);
+    return rc;
+}
-- 
2.30.1

[PATCH 12/21] libs/guest: allow updating a cpu policy MSR data

[Roger Pau Monne]

Introduce a helper to update the MSR policy using an array of
xen_msr_entry_t entries. Note the MSRs present in the input
xen_msr_entry_t array will replace any existing entries on the
policy.

No user of the interface introduced on this patch.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 tools/include/xenctrl.h         |  2 +
 tools/libs/guest/xg_cpuid_x86.c | 65 +++++++++++++++++++++++++++++++++
 2 files changed, 67 insertions(+)

diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index 2143478fe4b..46f5026081c 100644
--- a/tools/include/xenctrl.h
+++ b/tools/include/xenctrl.h
@@ -2616,6 +2616,8 @@ int xc_cpu_policy_get_msr(xc_interface *xch, const xc_cpu_policy_t policy,
 int xc_cpu_policy_update_cpuid(xc_interface *xch, xc_cpu_policy_t policy,
                                const xen_cpuid_leaf_t *leaves,
                                uint32_t nr);
+int xc_cpu_policy_update_msrs(xc_interface *xch, xc_cpu_policy_t policy,
+                              const xen_msr_entry_t *msrs, uint32_t nr);
 
 int xc_get_cpu_levelling_caps(xc_interface *xch, uint32_t *caps);
 int xc_get_cpu_featureset(xc_interface *xch, uint32_t index,
diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
index 13c2972ccd3..07756743e76 100644
--- a/tools/libs/guest/xg_cpuid_x86.c
+++ b/tools/libs/guest/xg_cpuid_x86.c
@@ -1033,3 +1033,68 @@ int xc_cpu_policy_update_cpuid(xc_interface *xch, xc_cpu_policy_t policy,
     free(current);
     return rc;
 }
+
+int xc_cpu_policy_update_msrs(xc_interface *xch, xc_cpu_policy_t policy,
+                              const xen_msr_entry_t *msrs, uint32_t nr)
+{
+    unsigned int err_msr = -1;
+    unsigned int nr_leaves, nr_msrs, i, j;
+    xen_msr_entry_t *current;
+    int rc = xc_cpu_policy_get_size(xch, &nr_leaves, &nr_msrs);
+
+    if ( rc )
+    {
+        PERROR("Failed to obtain policy info size");
+        return -1;
+    }
+
+    current = calloc(nr_msrs, sizeof(*current));
+    if ( !current )
+    {
+        PERROR("Failed to allocate resources");
+        errno = ENOMEM;
+        return -1;
+    }
+
+    rc = xc_cpu_policy_serialise(xch, policy, NULL, 0, current, &nr_msrs);
+    if ( rc )
+        goto out;
+
+    for ( i = 0; i < nr; i++ )
+    {
+        const xen_msr_entry_t *update = &msrs[i];
+
+        for ( j = 0; j < nr_msrs; j++ )
+            if ( current[j].idx == update->idx )
+            {
+                /*
+                 * NB: cannot use an assignation because of the const vs
+                 * non-const difference.
+                 */
+                memcpy(&current[j], update, sizeof(*update));
+                break;
+            }
+
+        if ( j == nr_msrs )
+        {
+            /* Failed to find a matching MSR, append to the end. */
+            current = realloc(current, (nr_msrs + 1) * sizeof(*current));
+            memcpy(&current[nr_msrs], update, sizeof(*update));
+            nr_msrs++;
+        }
+    }
+
+    rc = x86_msr_copy_from_buffer(policy->msr, current, nr_msrs, &err_msr);
+    if ( rc )
+    {
+        ERROR("Failed to deserialise MSRS (err index %#x) (%d = %s)",
+              err_msr, -rc, strerror(-rc));
+        errno = -rc;
+        rc = -1;
+    }
+
+ out:
+    free(current);
+    return rc;
+
+}
-- 
2.30.1

[PATCH 13/21] libs/guest: switch users of xc_set_domain_cpu_policy

[Roger Pau Monne]

To use the new cpu policy interface xc_cpu_policy_set_domain. Note
that xc_set_domain_cpu_policy is removed from the interface and the
function is made static to xg_cpuid_x86.c for it's internal callers.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 tools/include/xenctrl.h             |  5 -----
 tools/libs/guest/xg_cpuid_x86.c     | 22 +++++++++++-----------
 tools/libs/guest/xg_sr_common_x86.c | 28 +++++++++++++++++++++-------
 3 files changed, 32 insertions(+), 23 deletions(-)

diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index 46f5026081c..164a287b367 100644
--- a/tools/include/xenctrl.h
+++ b/tools/include/xenctrl.h
@@ -2625,11 +2625,6 @@ int xc_get_cpu_featureset(xc_interface *xch, uint32_t index,
 
 int xc_cpu_policy_get_size(xc_interface *xch, uint32_t *nr_leaves,
                            uint32_t *nr_msrs);
-int xc_set_domain_cpu_policy(xc_interface *xch, uint32_t domid,
-                             uint32_t nr_leaves, xen_cpuid_leaf_t *leaves,
-                             uint32_t nr_msrs, xen_msr_entry_t *msrs,
-                             uint32_t *err_leaf_p, uint32_t *err_subleaf_p,
-                             uint32_t *err_msr_p);
 
 uint32_t xc_get_cpu_featureset_size(void);
 
diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
index 07756743e76..f7b662f31aa 100644
--- a/tools/libs/guest/xg_cpuid_x86.c
+++ b/tools/libs/guest/xg_cpuid_x86.c
@@ -204,11 +204,11 @@ static int get_domain_cpu_policy(xc_interface *xch, uint32_t domid,
     return ret;
 }
 
-int xc_set_domain_cpu_policy(xc_interface *xch, uint32_t domid,
-                             uint32_t nr_leaves, xen_cpuid_leaf_t *leaves,
-                             uint32_t nr_msrs, xen_msr_entry_t *msrs,
-                             uint32_t *err_leaf_p, uint32_t *err_subleaf_p,
-                             uint32_t *err_msr_p)
+static int set_domain_cpu_policy(xc_interface *xch, uint32_t domid,
+                                 uint32_t nr_leaves, xen_cpuid_leaf_t *leaves,
+                                 uint32_t nr_msrs, xen_msr_entry_t *msrs,
+                                 uint32_t *err_leaf_p, uint32_t *err_subleaf_p,
+                                 uint32_t *err_msr_p)
 {
     DECLARE_DOMCTL;
     DECLARE_HYPERCALL_BOUNCE(leaves,
@@ -405,8 +405,8 @@ static int xc_cpuid_xend_policy(
     }
 
     /* Feed the transformed currrent policy back up to Xen. */
-    rc = xc_set_domain_cpu_policy(xch, domid, nr_cur, cur, 0, NULL,
-                                  &err_leaf, &err_subleaf, &err_msr);
+    rc = set_domain_cpu_policy(xch, domid, nr_cur, cur, 0, NULL,
+                               &err_leaf, &err_subleaf, &err_msr);
     if ( rc )
     {
         PERROR("Failed to set d%d's policy (err leaf %#x, subleaf %#x, msr %#x)",
@@ -638,8 +638,8 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
         goto out;
     }
 
-    rc = xc_set_domain_cpu_policy(xch, domid, nr_leaves, leaves, 0, NULL,
-                                  &err_leaf, &err_subleaf, &err_msr);
+    rc = set_domain_cpu_policy(xch, domid, nr_leaves, leaves, 0, NULL,
+                               &err_leaf, &err_subleaf, &err_msr);
     if ( rc )
     {
         PERROR("Failed to set d%d's policy (err leaf %#x, subleaf %#x, msr %#x)",
@@ -833,8 +833,8 @@ int xc_cpu_policy_set_domain(xc_interface *xch, uint32_t domid,
     if ( rc )
         goto out;
 
-    rc = xc_set_domain_cpu_policy(xch, domid, nr_leaves, leaves, nr_msrs, msrs,
-                                  &err_leaf, &err_subleaf, &err_msr);
+    rc = set_domain_cpu_policy(xch, domid, nr_leaves, leaves, nr_msrs, msrs,
+                               &err_leaf, &err_subleaf, &err_msr);
     if ( rc )
     {
         ERROR("Failed to set domain %u policy (%d = %s)", domid, -rc,
diff --git a/tools/libs/guest/xg_sr_common_x86.c b/tools/libs/guest/xg_sr_common_x86.c
index 15265e7a331..02f0c3ae9ed 100644
--- a/tools/libs/guest/xg_sr_common_x86.c
+++ b/tools/libs/guest/xg_sr_common_x86.c
@@ -151,7 +151,10 @@ int x86_static_data_complete(struct xc_sr_context *ctx, unsigned int *missing)
 {
     xc_interface *xch = ctx->xch;
     uint32_t nr_leaves = 0, nr_msrs = 0;
-    uint32_t err_l = ~0, err_s = ~0, err_m = ~0;
+    xc_cpu_policy_t policy = xc_cpu_policy_init();
+
+    if ( !policy )
+        return -1;
 
     if ( ctx->x86.restore.cpuid.ptr )
         nr_leaves = ctx->x86.restore.cpuid.size / sizeof(xen_cpuid_leaf_t);
@@ -163,14 +166,25 @@ int x86_static_data_complete(struct xc_sr_context *ctx, unsigned int *missing)
     else
         *missing |= XGR_SDD_MISSING_MSR;
 
+    if ( nr_leaves &&
+         xc_cpu_policy_update_cpuid(xch, policy,
+                                    ctx->x86.restore.cpuid.ptr, nr_leaves) )
+    {
+        PERROR("Failed to update CPUID policy");
+        return -1;
+    }
+    if ( nr_msrs &&
+         xc_cpu_policy_update_msrs(xch, policy,
+                                   ctx->x86.restore.msr.ptr, nr_msrs) )
+    {
+        PERROR("Failed to update MSR policy");
+        return -1;
+    }
+
     if ( (nr_leaves || nr_msrs) &&
-         xc_set_domain_cpu_policy(xch, ctx->domid,
-                                  nr_leaves, ctx->x86.restore.cpuid.ptr,
-                                  nr_msrs,   ctx->x86.restore.msr.ptr,
-                                  &err_l, &err_s, &err_m) )
+         xc_cpu_policy_set_domain(xch, ctx->domid, policy) )
     {
-        PERROR("Failed to set CPUID policy: leaf %08x, subleaf %08x, msr %08x",
-               err_l, err_s, err_m);
+        PERROR("Failed to set CPUID policy");
         return -1;
     }
 
-- 
2.30.1

[PATCH 14/21] libs/guest: introduce helper to check cpu policy compatibility

[Roger Pau Monne]

Such helpers is just a wrapper to the existing
x86_cpu_policies_are_compatible function. This requires building
policy.c from libx86 on user land also.

No user of the interface introduced.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 tools/include/xenctrl.h         |  4 ++++
 tools/libs/guest/Makefile       |  2 +-
 tools/libs/guest/xg_cpuid_x86.c | 17 +++++++++++++++++
 3 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index 164a287b367..165beff330f 100644
--- a/tools/include/xenctrl.h
+++ b/tools/include/xenctrl.h
@@ -2619,6 +2619,10 @@ int xc_cpu_policy_update_cpuid(xc_interface *xch, xc_cpu_policy_t policy,
 int xc_cpu_policy_update_msrs(xc_interface *xch, xc_cpu_policy_t policy,
                               const xen_msr_entry_t *msrs, uint32_t nr);
 
+/* Compatibility calculations. */
+bool xc_cpu_policy_is_compatible(xc_interface *xch, const xc_cpu_policy_t p1,
+                                 const xc_cpu_policy_t p2);
+
 int xc_get_cpu_levelling_caps(xc_interface *xch, uint32_t *caps);
 int xc_get_cpu_featureset(xc_interface *xch, uint32_t index,
                           uint32_t *nr_features, uint32_t *featureset);
diff --git a/tools/libs/guest/Makefile b/tools/libs/guest/Makefile
index 604e1695d6a..6d2a1d5bbc0 100644
--- a/tools/libs/guest/Makefile
+++ b/tools/libs/guest/Makefile
@@ -40,7 +40,7 @@ $(patsubst %.c,%.opic,$(ELF_SRCS-y)): CFLAGS += -Wno-pointer-sign
 ifeq ($(CONFIG_X86),y) # Add libx86 to the build
 vpath %.c ../../../xen/lib/x86
 
-SRCS-y                 += cpuid.c msr.c
+SRCS-y                 += cpuid.c msr.c policy.c
 endif
 
 # new domain builder
diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
index f7b662f31aa..30ea02a0f31 100644
--- a/tools/libs/guest/xg_cpuid_x86.c
+++ b/tools/libs/guest/xg_cpuid_x86.c
@@ -1098,3 +1098,20 @@ int xc_cpu_policy_update_msrs(xc_interface *xch, xc_cpu_policy_t policy,
     return rc;
 
 }
+
+bool xc_cpu_policy_is_compatible(xc_interface *xch, const xc_cpu_policy_t p1,
+                                 const xc_cpu_policy_t p2)
+{
+    struct cpu_policy_errors err;
+    int rc = x86_cpu_policies_are_compatible(p1, p2, &err);
+
+    if ( !rc )
+        return true;
+
+    if ( err.leaf != -1 )
+        ERROR("Leaf %#x subleaf %#x is not compatible", err.leaf, err.subleaf);
+    if ( err.msr != -1 )
+        ERROR("MSR index %#x is not compatible", err.msr);
+
+    return false;
+}
-- 
2.30.1

[PATCH 15/21] libs/guest: obtain a compatible cpu policy from two input ones

[Roger Pau Monne]

Introduce a helper to obtain a compatible cpu policy based on two
input cpu policies. Currently this is done by and'ing all CPUID leaves
and MSR entries, except for MSR_ARCH_CAPABILITIES which has the RSBA
bit or'ed.

The _AC macro is pulled from libxl_internal.h into xen-tools/libs.h
since it's required in order to use the msr-index.h header.

Note there's no need to place this helper in libx86, since the
calculation of a compatible policy shouldn't be done from the
hypervisor.

No callers of the interface introduced.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 tools/include/xen-tools/libs.h    |   5 ++
 tools/include/xenctrl.h           |   4 ++
 tools/libs/guest/xg_cpuid_x86.c   | 115 ++++++++++++++++++++++++++++++
 tools/libs/light/libxl_internal.h |   2 -
 4 files changed, 124 insertions(+), 2 deletions(-)

diff --git a/tools/include/xen-tools/libs.h b/tools/include/xen-tools/libs.h
index a16e0c38070..b9e89f9a711 100644
--- a/tools/include/xen-tools/libs.h
+++ b/tools/include/xen-tools/libs.h
@@ -63,4 +63,9 @@
 #define ROUNDUP(_x,_w) (((unsigned long)(_x)+(1UL<<(_w))-1) & ~((1UL<<(_w))-1))
 #endif
 
+#ifndef _AC
+#define __AC(X,Y)   (X##Y)
+#define _AC(X,Y)    __AC(X,Y)
+#endif
+
 #endif	/* __XEN_TOOLS_LIBS__ */
diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index 165beff330f..5f3e5e17e9d 100644
--- a/tools/include/xenctrl.h
+++ b/tools/include/xenctrl.h
@@ -2622,6 +2622,10 @@ int xc_cpu_policy_update_msrs(xc_interface *xch, xc_cpu_policy_t policy,
 /* Compatibility calculations. */
 bool xc_cpu_policy_is_compatible(xc_interface *xch, const xc_cpu_policy_t p1,
                                  const xc_cpu_policy_t p2);
+int xc_cpu_policy_calc_compatible(xc_interface *xch,
+                                  const xc_cpu_policy_t p1,
+                                  const xc_cpu_policy_t p2,
+                                  xc_cpu_policy_t out);
 
 int xc_get_cpu_levelling_caps(xc_interface *xch, uint32_t *caps);
 int xc_get_cpu_featureset(xc_interface *xch, uint32_t index,
diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
index 30ea02a0f31..4afca3249ba 100644
--- a/tools/libs/guest/xg_cpuid_x86.c
+++ b/tools/libs/guest/xg_cpuid_x86.c
@@ -32,6 +32,7 @@ enum {
 #include <xen/arch-x86/cpufeatureset.h>
 };
 
+#include <xen/asm/msr-index.h>
 #include <xen/asm/x86-vendors.h>
 
 #include <xen/lib/x86/cpu-policy.h>
@@ -1115,3 +1116,117 @@ bool xc_cpu_policy_is_compatible(xc_interface *xch, const xc_cpu_policy_t p1,
 
     return false;
 }
+
+static uint64_t level_msr(unsigned int index, uint64_t val1, uint64_t val2)
+{
+    uint64_t val;
+
+    switch( index )
+    {
+    case MSR_ARCH_CAPABILITIES:
+        val = val1 & val2;
+        /*
+         * Set RSBA if present on any of the input values to notice the guest
+         * might run on vulnerable hardware at some point.
+         */
+        val |= (val1 | val2) & ARCH_CAPS_RSBA;
+        break;
+
+    default:
+        val = val1 & val2;
+        break;
+    }
+
+    return val;
+}
+
+int xc_cpu_policy_calc_compatible(xc_interface *xch,
+                                  const xc_cpu_policy_t p1,
+                                  const xc_cpu_policy_t p2,
+                                  xc_cpu_policy_t out)
+{
+    xen_cpuid_leaf_t *leaves = NULL, *p1_leaves = NULL, *p2_leaves = NULL;
+    xen_msr_entry_t *msrs = NULL, *p1_msrs = NULL, *p2_msrs = NULL;
+    unsigned int nr_leaves, nr_msrs, i, j, index;
+    unsigned int p1_nr_leaves, p1_nr_msrs, p2_nr_leaves, p2_nr_msrs;
+    int rc;
+
+    if ( xc_cpu_policy_get_size(xch, &nr_leaves, &nr_msrs) )
+    {
+        PERROR("Failed to obtain policy info size");
+        return -1;
+    }
+
+    leaves = calloc(nr_leaves, sizeof(*leaves));
+    p1_leaves = calloc(nr_leaves, sizeof(*p1_leaves));
+    p2_leaves = calloc(nr_leaves, sizeof(*p2_leaves));
+    msrs = calloc(nr_msrs, sizeof(*msrs));
+    p1_msrs = calloc(nr_msrs, sizeof(*p1_msrs));
+    p2_msrs = calloc(nr_msrs, sizeof(*p2_msrs));
+
+    p1_nr_leaves = p2_nr_leaves = nr_leaves;
+    p1_nr_msrs = p2_nr_msrs = nr_msrs;
+
+    if ( !leaves || !p1_leaves || !p2_leaves ||
+         !msrs || !p1_msrs || !p2_msrs )
+    {
+        ERROR("Failed to allocate resources");
+        errno = ENOMEM;
+        rc = -1;
+        goto out;
+    }
+
+    rc = xc_cpu_policy_serialise(xch, p1, p1_leaves, &p1_nr_leaves,
+                                 p1_msrs, &p1_nr_msrs);
+    if ( rc )
+        goto out;
+    rc = xc_cpu_policy_serialise(xch, p2, p2_leaves, &p2_nr_leaves,
+                                 p2_msrs, &p2_nr_msrs);
+    if ( rc )
+        goto out;
+
+    index = 0;
+    for ( i = 0; i < p1_nr_leaves; i++ )
+        for ( j = 0; j < p2_nr_leaves; j++ )
+            if ( p1_leaves[i].leaf == p2_leaves[j].leaf &&
+                 p1_leaves[i].subleaf == p2_leaves[j].subleaf )
+            {
+                leaves[index].leaf = p1_leaves[i].leaf;
+                leaves[index].subleaf = p1_leaves[i].subleaf;
+                leaves[index].a = p1_leaves[i].a & p2_leaves[j].a;
+                leaves[index].b = p1_leaves[i].b & p2_leaves[j].b;
+                leaves[index].c = p1_leaves[i].c & p2_leaves[j].c;
+                leaves[index].d = p1_leaves[i].d & p2_leaves[j].d;
+                index++;
+            }
+    nr_leaves = index;
+
+    index = 0;
+    for ( i = 0; i < p1_nr_msrs; i++ )
+        for ( j = 0; j < p2_nr_msrs; j++ )
+            if ( p1_msrs[i].idx == p2_msrs[j].idx )
+            {
+                msrs[index].idx = p1_msrs[i].idx;
+                msrs[index].val = level_msr(p1_msrs[i].idx,
+                                            p1_msrs[i].val, p2_msrs[j].val);
+                index++;
+            }
+    nr_msrs = index;
+
+    rc = deserialize_policy(xch, out, nr_leaves, leaves, nr_msrs, msrs);
+    if ( rc )
+    {
+        errno = -rc;
+        rc = -1;
+    }
+
+ out:
+    free(leaves);
+    free(p1_leaves);
+    free(p2_leaves);
+    free(msrs);
+    free(p1_msrs);
+    free(p2_msrs);
+
+    return rc;
+}
diff --git a/tools/libs/light/libxl_internal.h b/tools/libs/light/libxl_internal.h
index 22b1775b752..53b8939fb5a 100644
--- a/tools/libs/light/libxl_internal.h
+++ b/tools/libs/light/libxl_internal.h
@@ -126,8 +126,6 @@
 #define PVSHIM_CMDLINE "pv-shim console=xen,pv"
 
 /* Size macros. */
-#define __AC(X,Y)   (X##Y)
-#define _AC(X,Y)    __AC(X,Y)
 #define MB(_mb)     (_AC(_mb, ULL) << 20)
 #define GB(_gb)     (_AC(_gb, ULL) << 30)
 
-- 
2.30.1

[PATCH 16/21] libs/guest: make a cpu policy compatible with older Xen versions

[Roger Pau Monne]

Older Xen versions used to expose some CPUID bits which are no longer
exposed by default. In order to keep a compatible behavior with
guests migrated from versions of Xen that don't encode the CPUID data
on the migration stream introduce a function that sets the same bits
as older Xen versions.

This is pulled out from xc_cpuid_apply_policy which already has this
logic present.

No functional change intended.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 tools/include/xenctrl.h         |  4 ++++
 tools/libs/guest/xg_cpuid_x86.c | 39 ++++++++++++++++++++++++++++-----
 2 files changed, 37 insertions(+), 6 deletions(-)

diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index 5f3e5e17e9d..6f7158156fa 100644
--- a/tools/include/xenctrl.h
+++ b/tools/include/xenctrl.h
@@ -2627,6 +2627,10 @@ int xc_cpu_policy_calc_compatible(xc_interface *xch,
                                   const xc_cpu_policy_t p2,
                                   xc_cpu_policy_t out);
 
+/* Make a policy compatible with previous Xen versions. */
+int xc_cpu_policy_make_compatible(xc_interface *xch, xc_cpu_policy_t policy,
+                                  bool hvm);
+
 int xc_get_cpu_levelling_caps(xc_interface *xch, uint32_t *caps);
 int xc_get_cpu_featureset(xc_interface *xch, uint32_t index,
                           uint32_t *nr_features, uint32_t *featureset);
diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
index 4afca3249ba..2abaf400a2b 100644
--- a/tools/libs/guest/xg_cpuid_x86.c
+++ b/tools/libs/guest/xg_cpuid_x86.c
@@ -436,6 +436,7 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
     unsigned int i, nr_leaves, nr_msrs;
     xen_cpuid_leaf_t *leaves = NULL;
     struct cpuid_policy *p = NULL;
+    struct cpu_policy policy = { };
     uint32_t err_leaf = -1, err_subleaf = -1, err_msr = -1;
     uint32_t host_featureset[FEATURESET_NR_ENTRIES] = {};
     uint32_t len = ARRAY_SIZE(host_featureset);
@@ -504,12 +505,8 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
      */
     if ( restore )
     {
-        p->basic.rdrand = test_bit(X86_FEATURE_RDRAND, host_featureset);
-
-        if ( di.hvm )
-        {
-            p->feat.mpx = test_bit(X86_FEATURE_MPX, host_featureset);
-        }
+        policy.cpuid = p;
+        xc_cpu_policy_make_compatible(xch, &policy, di.hvm);
     }
 
     if ( featureset )
@@ -1230,3 +1227,33 @@ int xc_cpu_policy_calc_compatible(xc_interface *xch,
 
     return rc;
 }
+
+int xc_cpu_policy_make_compatible(xc_interface *xch, xc_cpu_policy_t policy,
+                                  bool hvm)
+{
+    xc_cpu_policy_t host;
+    int rc;
+
+    host = xc_cpu_policy_init();
+    if ( !host )
+    {
+        errno = ENOMEM;
+        return -1;
+    }
+
+    rc = xc_cpu_policy_get_system(xch, XEN_SYSCTL_cpu_policy_host, host);
+    if ( rc )
+    {
+        ERROR("Failed to get host policy");
+        goto out;
+    }
+
+    policy->cpuid->basic.rdrand = host->cpuid->basic.rdrand;
+
+    if ( hvm )
+        policy->cpuid->feat.mpx = host->cpuid->feat.mpx;
+
+ out:
+    xc_cpu_policy_destroy(host);
+    return rc;
+}
-- 
2.30.1

[PATCH 17/21] libs/guest: introduce helper set cpu topology in cpu policy

[Roger Pau Monne]

This logic is pulled out from xc_cpuid_apply_policy and placed into a
separate helper.

No functional change intended.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 tools/include/xenctrl.h         |   4 +
 tools/libs/guest/xg_cpuid_x86.c | 181 +++++++++++++++++---------------
 2 files changed, 102 insertions(+), 83 deletions(-)

diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index 6f7158156fa..9f94e61523e 100644
--- a/tools/include/xenctrl.h
+++ b/tools/include/xenctrl.h
@@ -2631,6 +2631,10 @@ int xc_cpu_policy_calc_compatible(xc_interface *xch,
 int xc_cpu_policy_make_compatible(xc_interface *xch, xc_cpu_policy_t policy,
                                   bool hvm);
 
+/* Setup the policy topology. */
+int xc_cpu_policy_topology(xc_interface *xch, xc_cpu_policy_t policy,
+                           bool hvm);
+
 int xc_get_cpu_levelling_caps(xc_interface *xch, uint32_t *caps);
 int xc_get_cpu_featureset(xc_interface *xch, uint32_t index,
                           uint32_t *nr_features, uint32_t *featureset);
diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
index 2abaf400a2b..d50822c0abb 100644
--- a/tools/libs/guest/xg_cpuid_x86.c
+++ b/tools/libs/guest/xg_cpuid_x86.c
@@ -433,13 +433,11 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
 {
     int rc;
     xc_dominfo_t di;
-    unsigned int i, nr_leaves, nr_msrs;
+    unsigned int nr_leaves, nr_msrs;
     xen_cpuid_leaf_t *leaves = NULL;
     struct cpuid_policy *p = NULL;
     struct cpu_policy policy = { };
     uint32_t err_leaf = -1, err_subleaf = -1, err_msr = -1;
-    uint32_t host_featureset[FEATURESET_NR_ENTRIES] = {};
-    uint32_t len = ARRAY_SIZE(host_featureset);
 
     if ( xc_domain_getinfo(xch, domid, 1, &di) != 1 ||
          di.domid != domid )
@@ -462,22 +460,6 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
          (p = calloc(1, sizeof(*p))) == NULL )
         goto out;
 
-    /* Get the host policy. */
-    rc = xc_get_cpu_featureset(xch, XEN_SYSCTL_cpu_featureset_host,
-                               &len, host_featureset);
-    if ( rc )
-    {
-        /* Tolerate "buffer too small", as we've got the bits we need. */
-        if ( errno == ENOBUFS )
-            rc = 0;
-        else
-        {
-            PERROR("Failed to obtain host featureset");
-            rc = -errno;
-            goto out;
-        }
-    }
-
     /* Get the domain's default policy. */
     nr_msrs = 0;
     rc = get_system_cpu_policy(xch, di.hvm ? XEN_SYSCTL_cpu_policy_hvm_default
@@ -564,70 +546,10 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
         }
     }
 
-    if ( !di.hvm )
-    {
-        /*
-         * On hardware without CPUID Faulting, PV guests see real topology.
-         * As a consequence, they also need to see the host htt/cmp fields.
-         */
-        p->basic.htt       = test_bit(X86_FEATURE_HTT, host_featureset);
-        p->extd.cmp_legacy = test_bit(X86_FEATURE_CMP_LEGACY, host_featureset);
-    }
-    else
-    {
-        /*
-         * 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.
-         */
-        p->basic.htt = true;
-        p->extd.cmp_legacy = false;
-
-        /*
-         * Leaf 1 EBX[23:16] is Maximum Logical Processors Per Package.
-         * Update to reflect vLAPIC_ID = vCPU_ID * 2, but make sure to avoid
-         * overflow.
-         */
-        if ( !(p->basic.lppp & 0x80) )
-            p->basic.lppp *= 2;
-
-        switch ( p->x86_vendor )
-        {
-        case X86_VENDOR_INTEL:
-            for ( i = 0; (p->cache.subleaf[i].type &&
-                          i < ARRAY_SIZE(p->cache.raw)); ++i )
-            {
-                p->cache.subleaf[i].cores_per_package =
-                    (p->cache.subleaf[i].cores_per_package << 1) | 1;
-                p->cache.subleaf[i].threads_per_cache = 0;
-            }
-            break;
-
-        case X86_VENDOR_AMD:
-        case X86_VENDOR_HYGON:
-            /*
-             * Leaf 0x80000008 ECX[15:12] is ApicIdCoreSize.
-             * Leaf 0x80000008 ECX[7:0] is NumberOfCores (minus one).
-             * Update to reflect vLAPIC_ID = vCPU_ID * 2.  But avoid
-             * - overflow,
-             * - going out of sync with leaf 1 EBX[23:16],
-             * - incrementing ApicIdCoreSize when it's zero (which changes the
-             *   meaning of bits 7:0).
-             *
-             * UPDATE: I addition to avoiding overflow, some
-             * proprietary operating systems have trouble with
-             * apic_id_size values greater than 7.  Limit the value to
-             * 7 for now.
-             */
-            if ( p->extd.nc < 0x7f )
-            {
-                if ( p->extd.apic_id_size != 0 && p->extd.apic_id_size < 0x7 )
-                    p->extd.apic_id_size++;
-
-                p->extd.nc = (p->extd.nc << 1) | 1;
-            }
-            break;
-        }
-    }
+    policy.cpuid = p;
+    rc = xc_cpu_policy_topology(xch, &policy, di.hvm);
+    if ( rc )
+        goto out;
 
     rc = x86_cpuid_copy_to_buffer(p, leaves, &nr_leaves);
     if ( rc )
@@ -1257,3 +1179,96 @@ int xc_cpu_policy_make_compatible(xc_interface *xch, xc_cpu_policy_t policy,
     xc_cpu_policy_destroy(host);
     return rc;
 }
+
+int xc_cpu_policy_topology(xc_interface *xch, xc_cpu_policy_t policy,
+                           bool hvm)
+{
+    if ( !hvm )
+    {
+        xc_cpu_policy_t host;
+        int rc;
+
+        host = xc_cpu_policy_init();
+        if ( !host )
+        {
+            errno = ENOMEM;
+            return -1;
+        }
+
+        rc = xc_cpu_policy_get_system(xch, XEN_SYSCTL_cpu_policy_host, host);
+        if ( rc )
+        {
+            ERROR("Failed to get host policy");
+            xc_cpu_policy_destroy(host);
+            return rc;
+        }
+
+
+        /*
+         * On hardware without CPUID Faulting, PV guests see real topology.
+         * As a consequence, they also need to see the host htt/cmp fields.
+         */
+        policy->cpuid->basic.htt = host->cpuid->basic.htt;
+        policy->cpuid->extd.cmp_legacy = host->cpuid->extd.cmp_legacy;
+    }
+    else
+    {
+        unsigned int i;
+
+        /*
+         * 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.
+         */
+        policy->cpuid->basic.htt = true;
+        policy->cpuid->extd.cmp_legacy = false;
+
+        /*
+         * Leaf 1 EBX[23:16] is Maximum Logical Processors Per Package.
+         * Update to reflect vLAPIC_ID = vCPU_ID * 2, but make sure to avoid
+         * overflow.
+         */
+        if ( !(policy->cpuid->basic.lppp & 0x80) )
+            policy->cpuid->basic.lppp *= 2;
+
+        switch ( policy->cpuid->x86_vendor )
+        {
+        case X86_VENDOR_INTEL:
+            for ( i = 0; (policy->cpuid->cache.subleaf[i].type &&
+                          i < ARRAY_SIZE(policy->cpuid->cache.raw)); ++i )
+            {
+                policy->cpuid->cache.subleaf[i].cores_per_package =
+                  (policy->cpuid->cache.subleaf[i].cores_per_package << 1) | 1;
+                policy->cpuid->cache.subleaf[i].threads_per_cache = 0;
+            }
+            break;
+
+        case X86_VENDOR_AMD:
+        case X86_VENDOR_HYGON:
+            /*
+             * Leaf 0x80000008 ECX[15:12] is ApicIdCoreSize.
+             * Leaf 0x80000008 ECX[7:0] is NumberOfCores (minus one).
+             * Update to reflect vLAPIC_ID = vCPU_ID * 2.  But avoid
+             * - overflow,
+             * - going out of sync with leaf 1 EBX[23:16],
+             * - incrementing ApicIdCoreSize when it's zero (which changes the
+             *   meaning of bits 7:0).
+             *
+             * UPDATE: I addition to avoiding overflow, some
+             * proprietary operating systems have trouble with
+             * apic_id_size values greater than 7.  Limit the value to
+             * 7 for now.
+             */
+            if ( policy->cpuid->extd.nc < 0x7f )
+            {
+                if ( policy->cpuid->extd.apic_id_size != 0 &&
+                     policy->cpuid->extd.apic_id_size < 0x7 )
+                    policy->cpuid->extd.apic_id_size++;
+
+                policy->cpuid->extd.nc = (policy->cpuid->extd.nc << 1) | 1;
+            }
+            break;
+        }
+    }
+
+    return 0;
+}
-- 
2.30.1

[PATCH 18/21] libs/guest: rework xc_cpuid_xend_policy

[Roger Pau Monne]

Rename xc_cpuid_xend_policy to xc_cpu_policy_apply_cpuid and make it
public. Modify the function internally to use the new xc_cpu_policy_*
set of functions. Also don't apply the passed policy to a domain
directly, and instead modify the provided xc_cpu_policy_t. The caller
will be responsible of applying the modified cpu policy to the domain.

Note that further patches will end up removing this function, since
the parsing of a cpu policy in xend format is a layering violation,
now the callers should have the necessary helpers to modify an
xc_cpu_policy_t themselves.

No functional change intended.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 tools/include/xenctrl.h         |   4 +
 tools/libs/guest/xg_cpuid_x86.c | 200 +++++++++++++-------------------
 2 files changed, 83 insertions(+), 121 deletions(-)

diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index 9f94e61523e..07b8bfc08aa 100644
--- a/tools/include/xenctrl.h
+++ b/tools/include/xenctrl.h
@@ -2635,6 +2635,10 @@ int xc_cpu_policy_make_compatible(xc_interface *xch, xc_cpu_policy_t policy,
 int xc_cpu_policy_topology(xc_interface *xch, xc_cpu_policy_t policy,
                            bool hvm);
 
+/* Apply an xc_xend_cpuid object to the policy. */
+int xc_cpu_policy_apply_cpuid(xc_interface *xch, xc_cpu_policy_t policy,
+                              const struct xc_xend_cpuid *cpuid, bool hvm);
+
 int xc_get_cpu_levelling_caps(xc_interface *xch, uint32_t *caps);
 int xc_get_cpu_featureset(xc_interface *xch, uint32_t index,
                           uint32_t *nr_features, uint32_t *featureset);
diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
index d50822c0abb..ce4a4a1a436 100644
--- a/tools/libs/guest/xg_cpuid_x86.c
+++ b/tools/libs/guest/xg_cpuid_x86.c
@@ -258,144 +258,107 @@ static int set_domain_cpu_policy(xc_interface *xch, uint32_t domid,
     return ret;
 }
 
-static int compare_leaves(const void *l, const void *r)
-{
-    const xen_cpuid_leaf_t *lhs = l;
-    const xen_cpuid_leaf_t *rhs = r;
-
-    if ( lhs->leaf != rhs->leaf )
-        return lhs->leaf < rhs->leaf ? -1 : 1;
-
-    if ( lhs->subleaf != rhs->subleaf )
-        return lhs->subleaf < rhs->subleaf ? -1 : 1;
-
-    return 0;
-}
-
-static xen_cpuid_leaf_t *find_leaf(
-    xen_cpuid_leaf_t *leaves, unsigned int nr_leaves,
-    const struct xc_xend_cpuid *xend)
-{
-    const xen_cpuid_leaf_t key = { xend->leaf, xend->subleaf };
-
-    return bsearch(&key, leaves, nr_leaves, sizeof(*leaves), compare_leaves);
-}
-
-static int xc_cpuid_xend_policy(
-    xc_interface *xch, uint32_t domid, const struct xc_xend_cpuid *xend)
+int xc_cpu_policy_apply_cpuid(xc_interface *xch, xc_cpu_policy_t policy,
+                              const struct xc_xend_cpuid *cpuid, bool hvm)
 {
     int rc;
-    xc_dominfo_t di;
-    unsigned int nr_leaves, nr_msrs;
-    uint32_t err_leaf = -1, err_subleaf = -1, err_msr = -1;
-    /*
-     * Three full policies.  The host, domain max, and domain current for the
-     * domain type.
-     */
-    xen_cpuid_leaf_t *host = NULL, *max = NULL, *cur = NULL;
-    unsigned int nr_host, nr_max, nr_cur;
+    xc_cpu_policy_t host = NULL, max = NULL;
 
-    if ( xc_domain_getinfo(xch, domid, 1, &di) != 1 ||
-         di.domid != domid )
-    {
-        ERROR("Failed to obtain d%d info", domid);
-        rc = -ESRCH;
-        goto fail;
-    }
-
-    rc = xc_cpu_policy_get_size(xch, &nr_leaves, &nr_msrs);
-    if ( rc )
-    {
-        PERROR("Failed to obtain policy info size");
-        rc = -errno;
-        goto fail;
-    }
-
-    rc = -ENOMEM;
-    if ( (host = calloc(nr_leaves, sizeof(*host))) == NULL ||
-         (max  = calloc(nr_leaves, sizeof(*max)))  == NULL ||
-         (cur  = calloc(nr_leaves, sizeof(*cur)))  == NULL )
-    {
-        ERROR("Unable to allocate memory for %u CPUID leaves", nr_leaves);
-        goto fail;
-    }
-
-    /* Get the domain's current policy. */
-    nr_msrs = 0;
-    nr_cur = nr_leaves;
-    rc = get_domain_cpu_policy(xch, domid, &nr_cur, cur, &nr_msrs, NULL);
-    if ( rc )
+    host = xc_cpu_policy_init();
+    max = xc_cpu_policy_init();
+    if ( !host || !max )
     {
-        PERROR("Failed to obtain d%d current policy", domid);
-        rc = -errno;
-        goto fail;
+        PERROR("Failed to init policies");
+        rc = -ENOMEM;
+        goto out;
     }
 
     /* Get the domain's max policy. */
-    nr_msrs = 0;
-    nr_max = nr_leaves;
-    rc = get_system_cpu_policy(xch, di.hvm ? XEN_SYSCTL_cpu_policy_hvm_max
+    rc = xc_cpu_policy_get_system(xch, hvm ? XEN_SYSCTL_cpu_policy_hvm_max
                                            : XEN_SYSCTL_cpu_policy_pv_max,
-                               &nr_max, max, &nr_msrs, NULL);
+                                  max);
     if ( rc )
     {
-        PERROR("Failed to obtain %s max policy", di.hvm ? "hvm" : "pv");
-        rc = -errno;
-        goto fail;
+        PERROR("Failed to obtain %s max policy", hvm ? "hvm" : "pv");
+        goto out;
     }
 
     /* Get the host policy. */
-    nr_msrs = 0;
-    nr_host = nr_leaves;
-    rc = get_system_cpu_policy(xch, XEN_SYSCTL_cpu_policy_host,
-                               &nr_host, host, &nr_msrs, NULL);
+    rc = xc_cpu_policy_get_system(xch, XEN_SYSCTL_cpu_policy_host, host);
     if ( rc )
     {
         PERROR("Failed to obtain host policy");
-        rc = -errno;
-        goto fail;
+        goto out;
     }
 
     rc = -EINVAL;
-    for ( ; xend->leaf != XEN_CPUID_INPUT_UNUSED; ++xend )
+    for ( ; cpuid->leaf != XEN_CPUID_INPUT_UNUSED; ++cpuid )
     {
-        xen_cpuid_leaf_t *cur_leaf = find_leaf(cur, nr_cur, xend);
-        const xen_cpuid_leaf_t *max_leaf = find_leaf(max, nr_max, xend);
-        const xen_cpuid_leaf_t *host_leaf = find_leaf(host, nr_host, xend);
+        xen_cpuid_leaf_t cur_leaf;
+        xen_cpuid_leaf_t max_leaf;
+        xen_cpuid_leaf_t host_leaf;
 
-        if ( cur_leaf == NULL || max_leaf == NULL || host_leaf == NULL )
+        rc = xc_cpu_policy_get_cpuid(xch, policy, cpuid->leaf, cpuid->subleaf,
+                                     &cur_leaf);
+        if ( rc )
+        {
+            ERROR("Failed to get current policy leaf %#x subleaf %#x",
+                  cpuid->leaf, cpuid->subleaf);
+            goto out;
+        }
+        rc = xc_cpu_policy_get_cpuid(xch, max, cpuid->leaf, cpuid->subleaf,
+                                     &max_leaf);
+        if ( rc )
         {
-            ERROR("Missing leaf %#x, subleaf %#x", xend->leaf, xend->subleaf);
-            goto fail;
+            ERROR("Failed to get max policy leaf %#x subleaf %#x",
+                  cpuid->leaf, cpuid->subleaf);
+            goto out;
+        }
+        rc = xc_cpu_policy_get_cpuid(xch, host, cpuid->leaf, cpuid->subleaf,
+                                     &host_leaf);
+        if ( rc )
+        {
+            ERROR("Failed to get host policy leaf %#x subleaf %#x",
+                  cpuid->leaf, cpuid->subleaf);
+            goto out;
         }
 
-        for ( unsigned int i = 0; i < ARRAY_SIZE(xend->policy); i++ )
+        for ( unsigned int i = 0; i < ARRAY_SIZE(cpuid->policy); i++ )
         {
-            uint32_t *cur_reg = &cur_leaf->a + i;
-            const uint32_t *max_reg = &max_leaf->a + i;
-            const uint32_t *host_reg = &host_leaf->a + i;
+            uint32_t *cur_reg = &cur_leaf.a + i;
+            const uint32_t *max_reg = &max_leaf.a + i;
+            const uint32_t *host_reg = &host_leaf.a + i;
 
-            if ( xend->policy[i] == NULL )
+            if ( cpuid->policy[i] == NULL )
                 continue;
 
             for ( unsigned int j = 0; j < 32; j++ )
             {
                 bool val;
 
-                if ( xend->policy[i][j] == '1' )
+                switch ( cpuid->policy[i][j] )
+                {
+                case '1':
                     val = true;
-                else if ( xend->policy[i][j] == '0' )
+                    break;
+
+                case '0':
                     val = false;
-                else if ( xend->policy[i][j] == 'x' )
+                    break;
+
+                case 'x':
                     val = test_bit(31 - j, max_reg);
-                else if ( xend->policy[i][j] == 'k' ||
-                          xend->policy[i][j] == 's' )
+                    break;
+
+                case 'k':
+                case 's':
                     val = test_bit(31 - j, host_reg);
-                else
-                {
+                    break;
+
+                default:
                     ERROR("Bad character '%c' in policy[%d] string '%s'",
-                          xend->policy[i][j], i, xend->policy[i]);
-                    goto fail;
+                          cpuid->policy[i][j], i, cpuid->policy[i]);
+                    goto out;
                 }
 
                 clear_bit(31 - j, cur_reg);
@@ -403,25 +366,19 @@ static int xc_cpuid_xend_policy(
                     set_bit(31 - j, cur_reg);
             }
         }
-    }
 
-    /* Feed the transformed currrent policy back up to Xen. */
-    rc = set_domain_cpu_policy(xch, domid, nr_cur, cur, 0, NULL,
-                               &err_leaf, &err_subleaf, &err_msr);
-    if ( rc )
-    {
-        PERROR("Failed to set d%d's policy (err leaf %#x, subleaf %#x, msr %#x)",
-               domid, err_leaf, err_subleaf, err_msr);
-        rc = -errno;
-        goto fail;
+        rc = xc_cpu_policy_update_cpuid(xch, policy, &cur_leaf, 1);
+        if ( rc )
+        {
+            PERROR("Failed to set policy leaf %#x subleaf %#x",
+                   cpuid->leaf, cpuid->subleaf);
+            goto out;
+        }
     }
 
-    /* Success! */
-
- fail:
-    free(cur);
-    free(max);
-    free(host);
+ out:
+    xc_cpu_policy_destroy(max);
+    xc_cpu_policy_destroy(host);
 
     return rc;
 }
@@ -429,7 +386,7 @@ static int xc_cpuid_xend_policy(
 int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
                           const uint32_t *featureset, unsigned int nr_features,
                           bool pae, bool itsc, bool nested_virt,
-                          const struct xc_xend_cpuid *xend)
+                          const struct xc_xend_cpuid *cpuid)
 {
     int rc;
     xc_dominfo_t di;
@@ -551,6 +508,10 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
     if ( rc )
         goto out;
 
+    rc = xc_cpu_policy_apply_cpuid(xch, &policy, cpuid, di.hvm);
+    if ( rc )
+        goto out;
+
     rc = x86_cpuid_copy_to_buffer(p, leaves, &nr_leaves);
     if ( rc )
     {
@@ -568,9 +529,6 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
         goto out;
     }
 
-    if ( xend && (rc = xc_cpuid_xend_policy(xch, domid, xend)) )
-        goto out;
-
     rc = 0;
 
 out:
-- 
2.30.1

[PATCH 19/21] libs/guest: apply a featureset into a cpu policy

[Roger Pau Monne]

Pull out the code from xc_cpuid_apply_policy that applies a featureset
to a cpu policy and place it on it's own standalone function that's
part of the public interface.

No functional change intended.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 tools/include/xenctrl.h         |  5 ++
 tools/libs/guest/xg_cpuid_x86.c | 94 ++++++++++++++++++++-------------
 2 files changed, 61 insertions(+), 38 deletions(-)

diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index 07b8bfc08aa..a830fac1d12 100644
--- a/tools/include/xenctrl.h
+++ b/tools/include/xenctrl.h
@@ -2639,6 +2639,11 @@ int xc_cpu_policy_topology(xc_interface *xch, xc_cpu_policy_t policy,
 int xc_cpu_policy_apply_cpuid(xc_interface *xch, xc_cpu_policy_t policy,
                               const struct xc_xend_cpuid *cpuid, bool hvm);
 
+/* Apply a featureset to the policy. */
+int xc_cpu_policy_apply_featureset(xc_interface *xch, xc_cpu_policy_t policy,
+                                   const uint32_t *featureset,
+                                   unsigned int nr_features);
+
 int xc_get_cpu_levelling_caps(xc_interface *xch, uint32_t *caps);
 int xc_get_cpu_featureset(xc_interface *xch, uint32_t index,
                           uint32_t *nr_features, uint32_t *featureset);
diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
index ce4a4a1a436..96b969342fa 100644
--- a/tools/libs/guest/xg_cpuid_x86.c
+++ b/tools/libs/guest/xg_cpuid_x86.c
@@ -450,46 +450,14 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
 
     if ( featureset )
     {
-        uint32_t disabled_features[FEATURESET_NR_ENTRIES],
-            feat[FEATURESET_NR_ENTRIES] = {};
-        static const uint32_t deep_features[] = INIT_DEEP_FEATURES;
-        unsigned int i, b;
-
-        /*
-         * The user supplied featureset may be shorter or longer than
-         * FEATURESET_NR_ENTRIES.  Shorter is fine, and we will zero-extend.
-         * Longer is fine, so long as it only padded with zeros.
-         */
-        unsigned int user_len = min(FEATURESET_NR_ENTRIES + 0u, nr_features);
-
-        /* Check for truncated set bits. */
-        rc = -EOPNOTSUPP;
-        for ( i = user_len; i < nr_features; ++i )
-            if ( featureset[i] != 0 )
-                goto out;
-
-        memcpy(feat, featureset, sizeof(*featureset) * user_len);
-
-        /* Disable deep dependencies of disabled features. */
-        for ( i = 0; i < ARRAY_SIZE(disabled_features); ++i )
-            disabled_features[i] = ~feat[i] & deep_features[i];
-
-        for ( b = 0; b < sizeof(disabled_features) * CHAR_BIT; ++b )
+        policy.cpuid = p;
+        rc = xc_cpu_policy_apply_featureset(xch, &policy, featureset,
+                                            nr_features);
+        if ( rc )
         {
-            const uint32_t *dfs;
-
-            if ( !test_bit(b, disabled_features) ||
-                 !(dfs = x86_cpuid_lookup_deep_deps(b)) )
-                continue;
-
-            for ( i = 0; i < ARRAY_SIZE(disabled_features); ++i )
-            {
-                feat[i] &= ~dfs[i];
-                disabled_features[i] &= ~dfs[i];
-            }
+            ERROR("Failed to apply featureset to policy");
+            goto out;
         }
-
-        cpuid_featureset_to_policy(feat, p);
     }
     else
     {
@@ -1230,3 +1198,53 @@ int xc_cpu_policy_topology(xc_interface *xch, xc_cpu_policy_t policy,
 
     return 0;
 }
+
+int xc_cpu_policy_apply_featureset(xc_interface *xch, xc_cpu_policy_t policy,
+                                   const uint32_t *featureset,
+                                   unsigned int nr_features)
+{
+    uint32_t disabled_features[FEATURESET_NR_ENTRIES],
+        feat[FEATURESET_NR_ENTRIES] = {};
+    static const uint32_t deep_features[] = INIT_DEEP_FEATURES;
+    unsigned int i, b;
+
+    /*
+     * The user supplied featureset may be shorter or longer than
+     * FEATURESET_NR_ENTRIES.  Shorter is fine, and we will zero-extend.
+     * Longer is fine, so long as it only padded with zeros.
+     */
+    unsigned int user_len = min(FEATURESET_NR_ENTRIES + 0u, nr_features);
+
+    /* Check for truncated set bits. */
+    for ( i = user_len; i < nr_features; ++i )
+        if ( featureset[i] != 0 )
+        {
+            errno = EOPNOTSUPP;
+            return -1;
+        }
+
+    memcpy(feat, featureset, sizeof(*featureset) * user_len);
+
+    /* Disable deep dependencies of disabled features. */
+    for ( i = 0; i < ARRAY_SIZE(disabled_features); ++i )
+        disabled_features[i] = ~feat[i] & deep_features[i];
+
+    for ( b = 0; b < sizeof(disabled_features) * CHAR_BIT; ++b )
+    {
+        const uint32_t *dfs;
+
+        if ( !test_bit(b, disabled_features) ||
+             !(dfs = x86_cpuid_lookup_deep_deps(b)) )
+            continue;
+
+        for ( i = 0; i < ARRAY_SIZE(disabled_features); ++i )
+        {
+            feat[i] &= ~dfs[i];
+            disabled_features[i] &= ~dfs[i];
+        }
+    }
+
+    cpuid_featureset_to_policy(feat, policy->cpuid);
+
+    return 0;
+}
-- 
2.30.1

[PATCH 20/21] libs/{light,guest}: implement xc_cpuid_apply_policy in libxl

[Roger Pau Monne]

With the addition of the xc_cpu_policy_* now libxl can have better
control over the cpu policy, this allows removing the
xc_cpuid_apply_policy function and instead coding the required bits by
libxl in libxl__cpuid_legacy directly.

Remove xc_cpuid_apply_policy.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
I'm unsure why this is called libxl__cpuid_legacy, mostly because I
don't seem to be able to spot a libxl__cpuid (ie: non-legacy version).
Should this be renamed to libxl__cpuid?

1b3cec69bf300e012a mentions the process of switching to a new cpuid
interface, but I'm not sure we need to keep libxl__cpuid_legacy, since
that's an internal interface that's not exposed to libxl clients
anyway.
---
 tools/include/xenctrl.h         |  18 -----
 tools/libs/guest/xg_cpuid_x86.c | 123 --------------------------------
 tools/libs/light/libxl_cpuid.c  |  87 ++++++++++++++++++++--
 3 files changed, 83 insertions(+), 145 deletions(-)

diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index a830fac1d12..5a576f72b4d 100644
--- a/tools/include/xenctrl.h
+++ b/tools/include/xenctrl.h
@@ -1890,24 +1890,6 @@ struct xc_xend_cpuid {
     char *policy[4];
 };
 
-/*
- * Make adjustments to the CPUID settings for a domain.
- *
- * This path is used in two cases.  First, for fresh boots of the domain, and
- * secondly for migrate-in/restore of pre-4.14 guests (where CPUID data was
- * missing from the stream).  The @restore parameter distinguishes these
- * cases, and the generated policy must be compatible with a 4.13.
- *
- * Either pass a full new @featureset (and @nr_features), or adjust individual
- * features (@pae, @itsc, @nested_virt).
- *
- * Then (optionally) apply legacy XEND overrides (@xend) to the result.
- */
-int xc_cpuid_apply_policy(xc_interface *xch,
-                          uint32_t domid, bool restore,
-                          const uint32_t *featureset,
-                          unsigned int nr_features, bool pae, bool itsc,
-                          bool nested_virt, const struct xc_xend_cpuid *xend);
 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/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
index 96b969342fa..dffb9923b33 100644
--- a/tools/libs/guest/xg_cpuid_x86.c
+++ b/tools/libs/guest/xg_cpuid_x86.c
@@ -383,129 +383,6 @@ int xc_cpu_policy_apply_cpuid(xc_interface *xch, xc_cpu_policy_t policy,
     return rc;
 }
 
-int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
-                          const uint32_t *featureset, unsigned int nr_features,
-                          bool pae, bool itsc, bool nested_virt,
-                          const struct xc_xend_cpuid *cpuid)
-{
-    int rc;
-    xc_dominfo_t di;
-    unsigned int nr_leaves, nr_msrs;
-    xen_cpuid_leaf_t *leaves = NULL;
-    struct cpuid_policy *p = NULL;
-    struct cpu_policy policy = { };
-    uint32_t err_leaf = -1, err_subleaf = -1, err_msr = -1;
-
-    if ( xc_domain_getinfo(xch, domid, 1, &di) != 1 ||
-         di.domid != domid )
-    {
-        ERROR("Failed to obtain d%d info", domid);
-        rc = -ESRCH;
-        goto out;
-    }
-
-    rc = xc_cpu_policy_get_size(xch, &nr_leaves, &nr_msrs);
-    if ( rc )
-    {
-        PERROR("Failed to obtain policy info size");
-        rc = -errno;
-        goto out;
-    }
-
-    rc = -ENOMEM;
-    if ( (leaves = calloc(nr_leaves, sizeof(*leaves))) == NULL ||
-         (p = calloc(1, sizeof(*p))) == NULL )
-        goto out;
-
-    /* Get the domain's default policy. */
-    nr_msrs = 0;
-    rc = get_system_cpu_policy(xch, di.hvm ? XEN_SYSCTL_cpu_policy_hvm_default
-                                           : XEN_SYSCTL_cpu_policy_pv_default,
-                               &nr_leaves, leaves, &nr_msrs, NULL);
-    if ( rc )
-    {
-        PERROR("Failed to obtain %s default policy", di.hvm ? "hvm" : "pv");
-        rc = -errno;
-        goto out;
-    }
-
-    rc = x86_cpuid_copy_from_buffer(p, leaves, nr_leaves,
-                                    &err_leaf, &err_subleaf);
-    if ( rc )
-    {
-        ERROR("Failed to deserialise CPUID (err leaf %#x, subleaf %#x) (%d = %s)",
-              err_leaf, err_subleaf, -rc, strerror(-rc));
-        goto out;
-    }
-
-    /*
-     * Account for feature which have been disabled by default since Xen 4.13,
-     * so migrated-in VM's don't risk seeing features disappearing.
-     */
-    if ( restore )
-    {
-        policy.cpuid = p;
-        xc_cpu_policy_make_compatible(xch, &policy, di.hvm);
-    }
-
-    if ( featureset )
-    {
-        policy.cpuid = p;
-        rc = xc_cpu_policy_apply_featureset(xch, &policy, featureset,
-                                            nr_features);
-        if ( rc )
-        {
-            ERROR("Failed to apply featureset to policy");
-            goto out;
-        }
-    }
-    else
-    {
-        p->extd.itsc = itsc;
-
-        if ( di.hvm )
-        {
-            p->basic.pae = pae;
-            p->basic.vmx = nested_virt;
-            p->extd.svm = nested_virt;
-        }
-    }
-
-    policy.cpuid = p;
-    rc = xc_cpu_policy_topology(xch, &policy, di.hvm);
-    if ( rc )
-        goto out;
-
-    rc = xc_cpu_policy_apply_cpuid(xch, &policy, cpuid, di.hvm);
-    if ( rc )
-        goto out;
-
-    rc = x86_cpuid_copy_to_buffer(p, leaves, &nr_leaves);
-    if ( rc )
-    {
-        ERROR("Failed to serialise CPUID (%d = %s)", -rc, strerror(-rc));
-        goto out;
-    }
-
-    rc = set_domain_cpu_policy(xch, domid, nr_leaves, leaves, 0, NULL,
-                               &err_leaf, &err_subleaf, &err_msr);
-    if ( rc )
-    {
-        PERROR("Failed to set d%d's policy (err leaf %#x, subleaf %#x, msr %#x)",
-               domid, err_leaf, err_subleaf, err_msr);
-        rc = -errno;
-        goto out;
-    }
-
-    rc = 0;
-
-out:
-    free(p);
-    free(leaves);
-
-    return rc;
-}
-
 xc_cpu_policy_t xc_cpu_policy_init(void)
 {
     xc_cpu_policy_t policy = calloc(1, sizeof(*policy));
diff --git a/tools/libs/light/libxl_cpuid.c b/tools/libs/light/libxl_cpuid.c
index a7b33bbcd06..f1418382b62 100644
--- a/tools/libs/light/libxl_cpuid.c
+++ b/tools/libs/light/libxl_cpuid.c
@@ -423,6 +423,8 @@ int libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid, bool restore,
                         libxl_domain_build_info *info)
 {
     GC_INIT(ctx);
+    xc_cpu_policy_t policy = NULL;
+    bool hvm = info->type == LIBXL_DOMAIN_TYPE_HVM;
     bool pae = true;
     bool itsc;
     int rc;
@@ -436,6 +438,42 @@ int libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid, bool restore,
      */
     bool nested_virt = info->nested_hvm.val > 0;
 
+    policy = xc_cpu_policy_init();
+    if (!policy) {
+        LOGE(ERROR, "Failed to init CPU policy");
+        rc = ERROR_FAIL;
+        goto out;
+    }
+
+    rc = xc_cpu_policy_get_domain(ctx->xch, domid, policy);
+    if (rc) {
+        LOGE(ERROR, "Failed to fetch domain %u CPU policy", domid);
+        rc = ERROR_FAIL;
+        goto out;
+    }
+
+    /*
+     * Account for feature which have been disabled by default since Xen 4.13,
+     * so migrated-in VM's don't risk seeing features disappearing.
+     */
+    if (restore) {
+        rc = xc_cpu_policy_make_compatible(ctx->xch, policy, hvm);
+        if (rc) {
+            LOGE(ERROR, "Failed to setup compatible CPU policy for domain  %u",
+                 domid);
+            rc = ERROR_FAIL;
+            goto out;
+        }
+    }
+
+    rc = xc_cpu_policy_topology(ctx->xch, policy, hvm);
+    if (rc) {
+        LOGE(ERROR, "Failed to setup CPU policy topology for domain  %u",
+             domid);
+        rc = ERROR_FAIL;
+        goto out;
+    }
+
     /*
      * 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
@@ -446,8 +484,15 @@ int libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid, bool restore,
      *
      * HVM guests get a top-level choice of whether PAE is available.
      */
-    if (info->type == LIBXL_DOMAIN_TYPE_HVM)
+    if (hvm)
         pae = libxl_defbool_val(info->u.hvm.pae);
+    rc = libxl_cpuid_parse_config(&info->cpuid, GCSPRINTF("pae=%d", pae));
+    if (rc) {
+        LOG(ERROR, "Unable to set PAE CPUID flag: %d", rc);
+        rc = ERROR_FAIL;
+        goto out;
+    }
+
 
     /*
      * Advertising Invariant TSC to a guest means that the TSC frequency won't
@@ -463,12 +508,46 @@ int libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid, bool restore,
      */
     itsc = (libxl_defbool_val(info->disable_migrate) ||
             info->tsc_mode == LIBXL_TSC_MODE_ALWAYS_EMULATE);
+    rc = libxl_cpuid_parse_config(&info->cpuid, GCSPRINTF("invtsc=%d", itsc));
+    if (rc) {
+        LOG(ERROR, "Unable to set Invariant TSC CPUID flag: %d", rc);
+        rc = ERROR_FAIL;
+        goto out;
+    }
+
+    /* Set Nested virt CPUID bits for HVM. */
+    if (hvm) {
+        rc = libxl_cpuid_parse_config(&info->cpuid, GCSPRINTF("vmx=%d",
+                                                              nested_virt));
+        if (rc) {
+            LOG(ERROR, "Unable to set VMX CPUID flag: %d", rc);
+            rc = ERROR_FAIL;
+            goto out;
+        }
+
+        rc = libxl_cpuid_parse_config(&info->cpuid, GCSPRINTF("svm=%d",
+                                                              nested_virt));
+        if (rc) {
+            LOG(ERROR, "Unable to set SVM CPUID flag: %d", rc);
+            rc = ERROR_FAIL;
+            goto out;
+        }
+    }
+
+    /* Apply the bits from info->cpuid if any. */
+    rc = xc_cpu_policy_apply_cpuid(ctx->xch, policy, info->cpuid, hvm);
+    if (rc) {
+        LOGE(ERROR, "Failed to apply CPUID changes");
+        rc = ERROR_FAIL;
+        goto out;
+    }
 
-    rc = xc_cpuid_apply_policy(ctx->xch, domid, restore, NULL, 0,
-                               pae, itsc, nested_virt, info->cpuid);
+    rc = xc_cpu_policy_set_domain(ctx->xch, domid, policy);
     if (rc)
-        LOGE(ERROR, "Failed to apply CPUID policy");
+        LOGE(ERROR, "Failed to set domain %u CPUID policy", domid);
 
+ out:
+    xc_cpu_policy_destroy(policy);
     GC_FREE;
     return rc;
 }
-- 
2.30.1

[PATCH 21/21] libs/guest: (re)move xc_cpu_policy_apply_cpuid

[Roger Pau Monne]

Move the logic from xc_cpu_policy_apply_cpuid into libxl, now that the
xc_cpu_policy_* helpers allow modifying a cpu policy. Having xenguest
parsing CPUID data in xend format was a layering violation, and by
moving such parsing into libxl directly we can get rid of
xc_xend_cpuid, as libxl will now implement it's own private type for
storing CPUID information, which currently matches xc_xend_cpuid.

Note the function logic is moved as-is, but requires adapting to the
libxl coding style.

No functional change intended.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 tools/include/libxl.h             |   4 +-
 tools/include/xenctrl.h           |  30 -------
 tools/libs/guest/xg_cpuid_x86.c   | 125 ---------------------------
 tools/libs/light/libxl_cpuid.c    | 135 +++++++++++++++++++++++++++++-
 tools/libs/light/libxl_internal.h |  26 ++++++
 5 files changed, 160 insertions(+), 160 deletions(-)

diff --git a/tools/include/libxl.h b/tools/include/libxl.h
index ae7fe27c1f2..923a931fa67 100644
--- a/tools/include/libxl.h
+++ b/tools/include/libxl.h
@@ -1376,9 +1376,9 @@ void libxl_bitmap_dispose(libxl_bitmap *map);
 
 /*
  * libxl_cpuid_policy is opaque in the libxl ABI.  Users of both libxl and
- * libxc may not make assumptions about xc_xend_cpuid.
+ * libxc may not make assumptions about libxl__cpuid_policy.
  */
-typedef struct xc_xend_cpuid libxl_cpuid_policy;
+typedef struct libxl__cpuid_policy libxl_cpuid_policy;
 typedef libxl_cpuid_policy * libxl_cpuid_policy_list;
 void libxl_cpuid_dispose(libxl_cpuid_policy_list *cpuid_list);
 int libxl_cpuid_policy_list_length(const libxl_cpuid_policy_list *l);
diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index 5a576f72b4d..a57e32513d4 100644
--- a/tools/include/xenctrl.h
+++ b/tools/include/xenctrl.h
@@ -1864,32 +1864,6 @@ int xc_domain_debug_control(xc_interface *xch,
 
 #if defined(__i386__) || defined(__x86_64__)
 
-/*
- * CPUID policy data, expressed in the legacy XEND format.
- *
- * Policy is an array of strings, 32 chars long:
- *   policy[0] = eax
- *   policy[1] = ebx
- *   policy[2] = ecx
- *   policy[3] = edx
- *
- * The format of the string is the following:
- *   '1' -> force to 1
- *   '0' -> force to 0
- *   'x' -> we don't care (use default)
- *   'k' -> pass through host value
- *   's' -> legacy alias for 'k'
- */
-struct xc_xend_cpuid {
-    union {
-        struct {
-            uint32_t leaf, subleaf;
-        };
-        uint32_t input[2];
-    };
-    char *policy[4];
-};
-
 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);
@@ -2617,10 +2591,6 @@ int xc_cpu_policy_make_compatible(xc_interface *xch, xc_cpu_policy_t policy,
 int xc_cpu_policy_topology(xc_interface *xch, xc_cpu_policy_t policy,
                            bool hvm);
 
-/* Apply an xc_xend_cpuid object to the policy. */
-int xc_cpu_policy_apply_cpuid(xc_interface *xch, xc_cpu_policy_t policy,
-                              const struct xc_xend_cpuid *cpuid, bool hvm);
-
 /* Apply a featureset to the policy. */
 int xc_cpu_policy_apply_featureset(xc_interface *xch, xc_cpu_policy_t policy,
                                    const uint32_t *featureset,
diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
index dffb9923b33..51b0ab66a80 100644
--- a/tools/libs/guest/xg_cpuid_x86.c
+++ b/tools/libs/guest/xg_cpuid_x86.c
@@ -258,131 +258,6 @@ static int set_domain_cpu_policy(xc_interface *xch, uint32_t domid,
     return ret;
 }
 
-int xc_cpu_policy_apply_cpuid(xc_interface *xch, xc_cpu_policy_t policy,
-                              const struct xc_xend_cpuid *cpuid, bool hvm)
-{
-    int rc;
-    xc_cpu_policy_t host = NULL, max = NULL;
-
-    host = xc_cpu_policy_init();
-    max = xc_cpu_policy_init();
-    if ( !host || !max )
-    {
-        PERROR("Failed to init policies");
-        rc = -ENOMEM;
-        goto out;
-    }
-
-    /* Get the domain's max policy. */
-    rc = xc_cpu_policy_get_system(xch, hvm ? XEN_SYSCTL_cpu_policy_hvm_max
-                                           : XEN_SYSCTL_cpu_policy_pv_max,
-                                  max);
-    if ( rc )
-    {
-        PERROR("Failed to obtain %s max policy", hvm ? "hvm" : "pv");
-        goto out;
-    }
-
-    /* Get the host policy. */
-    rc = xc_cpu_policy_get_system(xch, XEN_SYSCTL_cpu_policy_host, host);
-    if ( rc )
-    {
-        PERROR("Failed to obtain host policy");
-        goto out;
-    }
-
-    rc = -EINVAL;
-    for ( ; cpuid->leaf != XEN_CPUID_INPUT_UNUSED; ++cpuid )
-    {
-        xen_cpuid_leaf_t cur_leaf;
-        xen_cpuid_leaf_t max_leaf;
-        xen_cpuid_leaf_t host_leaf;
-
-        rc = xc_cpu_policy_get_cpuid(xch, policy, cpuid->leaf, cpuid->subleaf,
-                                     &cur_leaf);
-        if ( rc )
-        {
-            ERROR("Failed to get current policy leaf %#x subleaf %#x",
-                  cpuid->leaf, cpuid->subleaf);
-            goto out;
-        }
-        rc = xc_cpu_policy_get_cpuid(xch, max, cpuid->leaf, cpuid->subleaf,
-                                     &max_leaf);
-        if ( rc )
-        {
-            ERROR("Failed to get max policy leaf %#x subleaf %#x",
-                  cpuid->leaf, cpuid->subleaf);
-            goto out;
-        }
-        rc = xc_cpu_policy_get_cpuid(xch, host, cpuid->leaf, cpuid->subleaf,
-                                     &host_leaf);
-        if ( rc )
-        {
-            ERROR("Failed to get host policy leaf %#x subleaf %#x",
-                  cpuid->leaf, cpuid->subleaf);
-            goto out;
-        }
-
-        for ( unsigned int i = 0; i < ARRAY_SIZE(cpuid->policy); i++ )
-        {
-            uint32_t *cur_reg = &cur_leaf.a + i;
-            const uint32_t *max_reg = &max_leaf.a + i;
-            const uint32_t *host_reg = &host_leaf.a + i;
-
-            if ( cpuid->policy[i] == NULL )
-                continue;
-
-            for ( unsigned int j = 0; j < 32; j++ )
-            {
-                bool val;
-
-                switch ( cpuid->policy[i][j] )
-                {
-                case '1':
-                    val = true;
-                    break;
-
-                case '0':
-                    val = false;
-                    break;
-
-                case 'x':
-                    val = test_bit(31 - j, max_reg);
-                    break;
-
-                case 'k':
-                case 's':
-                    val = test_bit(31 - j, host_reg);
-                    break;
-
-                default:
-                    ERROR("Bad character '%c' in policy[%d] string '%s'",
-                          cpuid->policy[i][j], i, cpuid->policy[i]);
-                    goto out;
-                }
-
-                clear_bit(31 - j, cur_reg);
-                if ( val )
-                    set_bit(31 - j, cur_reg);
-            }
-        }
-
-        rc = xc_cpu_policy_update_cpuid(xch, policy, &cur_leaf, 1);
-        if ( rc )
-        {
-            PERROR("Failed to set policy leaf %#x subleaf %#x",
-                   cpuid->leaf, cpuid->subleaf);
-            goto out;
-        }
-    }
-
- out:
-    xc_cpu_policy_destroy(max);
-    xc_cpu_policy_destroy(host);
-
-    return rc;
-}
-
 xc_cpu_policy_t xc_cpu_policy_init(void)
 {
     xc_cpu_policy_t policy = calloc(1, sizeof(*policy));
diff --git a/tools/libs/light/libxl_cpuid.c b/tools/libs/light/libxl_cpuid.c
index f1418382b62..ffa256b62e2 100644
--- a/tools/libs/light/libxl_cpuid.c
+++ b/tools/libs/light/libxl_cpuid.c
@@ -291,7 +291,7 @@ int libxl_cpuid_parse_config(libxl_cpuid_policy_list *cpuid, const char* str)
     char *sep, *val, *endptr;
     int i;
     const struct cpuid_flags *flag;
-    struct xc_xend_cpuid *entry;
+    struct libxl__cpuid_policy *entry;
     unsigned long num;
     char flags[33], *resstr;
 
@@ -369,7 +369,7 @@ int libxl_cpuid_parse_config_xend(libxl_cpuid_policy_list *cpuid,
     char *endptr;
     unsigned long value;
     uint32_t leaf, subleaf = XEN_CPUID_INPUT_UNUSED;
-    struct xc_xend_cpuid *entry;
+    struct libxl__cpuid_policy *entry;
 
     /* parse the leaf number */
     value = strtoul(str, &endptr, 0);
@@ -419,6 +419,135 @@ int libxl_cpuid_parse_config_xend(libxl_cpuid_policy_list *cpuid,
     return 0;
 }
 
+static int apply_cpuid(libxl_ctx *ctx, xc_cpu_policy_t policy,
+                       libxl_cpuid_policy_list cpuid, bool hvm)
+{
+    GC_INIT(ctx);
+    int rc;
+    xc_cpu_policy_t host = NULL, max = NULL;
+
+    host = xc_cpu_policy_init();
+    max = xc_cpu_policy_init();
+    if (!host || !max) {
+        LOG(ERROR, "Failed to init policies");
+        rc = ERROR_FAIL;
+        goto out;
+    }
+
+    /* Get the domain's max policy. */
+    rc = xc_cpu_policy_get_system(ctx->xch, hvm ? XEN_SYSCTL_cpu_policy_hvm_max
+                                                : XEN_SYSCTL_cpu_policy_pv_max,
+                                  max);
+    if (rc) {
+        LOGE(ERROR, "Failed to obtain %s max policy", hvm ? "hvm" : "pv");
+        rc = ERROR_FAIL;
+        goto out;
+    }
+
+    /* Get the host policy. */
+    rc = xc_cpu_policy_get_system(ctx->xch, XEN_SYSCTL_cpu_policy_host, host);
+    if (rc) {
+        LOGE(ERROR, "Failed to obtain host policy");
+        rc = ERROR_FAIL;
+        goto out;
+    }
+
+    for (; cpuid->leaf != XEN_CPUID_INPUT_UNUSED; ++cpuid) {
+        xen_cpuid_leaf_t cur_leaf;
+        xen_cpuid_leaf_t max_leaf;
+        xen_cpuid_leaf_t host_leaf;
+
+        rc = xc_cpu_policy_get_cpuid(ctx->xch, policy, cpuid->leaf,
+                                     cpuid->subleaf, &cur_leaf);
+        if (rc) {
+            LOGE(ERROR, "Failed to get current policy leaf %#x subleaf %#x",
+                 cpuid->leaf, cpuid->subleaf);
+            rc = ERROR_FAIL;
+            goto out;
+        }
+        rc = xc_cpu_policy_get_cpuid(ctx->xch, max, cpuid->leaf, cpuid->subleaf,
+                                     &max_leaf);
+        if (rc) {
+            LOGE(ERROR, "Failed to get max policy leaf %#x subleaf %#x",
+                 cpuid->leaf, cpuid->subleaf);
+            rc = ERROR_FAIL;
+            goto out;
+        }
+        rc = xc_cpu_policy_get_cpuid(ctx->xch, host, cpuid->leaf,
+                                     cpuid->subleaf, &host_leaf);
+        if (rc) {
+            LOGE(ERROR,"Failed to get host policy leaf %#x subleaf %#x",
+                 cpuid->leaf, cpuid->subleaf);
+            rc = ERROR_FAIL;
+            goto out;
+        }
+
+        for (unsigned int i = 0; i < ARRAY_SIZE(cpuid->policy); i++)
+        {
+            uint32_t *cur_reg = &cur_leaf.a + i;
+            const uint32_t *max_reg = &max_leaf.a + i;
+            const uint32_t *host_reg = &host_leaf.a + i;
+
+            if (cpuid->policy[i] == NULL)
+                continue;
+
+#define test_bit(i, r) !!(*(r) & (1u << (i)))
+#define set_bit(i, r) (*(r) |= (1u << (i)))
+#define clear_bit(i, r)  (*(r) &= ~(1u << (i)))
+            for (unsigned int j = 0; j < 32; j++) {
+                bool val;
+
+                switch (cpuid->policy[i][j]) {
+                case '1':
+                    val = true;
+                    break;
+
+                case '0':
+                    val = false;
+                    break;
+
+                case 'x':
+                    val = test_bit(31 - j, max_reg);
+                    break;
+
+                case 'k':
+                case 's':
+                    val = test_bit(31 - j, host_reg);
+                    break;
+
+                default:
+                    LOG(ERROR,"Bad character '%c' in policy[%d] string '%s'",
+                        cpuid->policy[i][j], i, cpuid->policy[i]);
+                    rc = ERROR_FAIL;
+                    goto out;
+                }
+
+                clear_bit(31 - j, cur_reg);
+                if (val)
+                    set_bit(31 - j, cur_reg);
+            }
+#undef clear_bit
+#undef set_bit
+#undef test_bit
+        }
+
+        rc = xc_cpu_policy_update_cpuid(ctx->xch, policy, &cur_leaf, 1);
+        if ( rc )
+        {
+            LOGE(ERROR,"Failed to set policy leaf %#x subleaf %#x",
+                 cpuid->leaf, cpuid->subleaf);
+            rc = ERROR_FAIL;
+            goto out;
+        }
+    }
+
+ out:
+    xc_cpu_policy_destroy(max);
+    xc_cpu_policy_destroy(host);
+    GC_FREE;
+    return rc;
+}
+
 int libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid, bool restore,
                         libxl_domain_build_info *info)
 {
@@ -535,7 +664,7 @@ int libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid, bool restore,
     }
 
     /* Apply the bits from info->cpuid if any. */
-    rc = xc_cpu_policy_apply_cpuid(ctx->xch, policy, info->cpuid, hvm);
+    rc = apply_cpuid(ctx, policy, info->cpuid, hvm);
     if (rc) {
         LOGE(ERROR, "Failed to apply CPUID changes");
         rc = ERROR_FAIL;
diff --git a/tools/libs/light/libxl_internal.h b/tools/libs/light/libxl_internal.h
index 53b8939fb5a..5b9a220004a 100644
--- a/tools/libs/light/libxl_internal.h
+++ b/tools/libs/light/libxl_internal.h
@@ -2050,6 +2050,32 @@ typedef yajl_gen_status (*libxl__gen_json_callback)(yajl_gen hand, void *);
 _hidden char *libxl__object_to_json(libxl_ctx *ctx, const char *type,
                                     libxl__gen_json_callback gen, void *p);
 
+/*
+ * CPUID policy data, expressed in the internal libxl format.
+ *
+ * Policy is an array of strings, 32 chars long:
+ *   policy[0] = eax
+ *   policy[1] = ebx
+ *   policy[2] = ecx
+ *   policy[3] = edx
+ *
+ * The format of the string is the following:
+ *   '1' -> force to 1
+ *   '0' -> force to 0
+ *   'x' -> we don't care (use default)
+ *   'k' -> pass through host value
+ *   's' -> legacy alias for 'k'
+ */
+struct libxl__cpuid_policy {
+    union {
+        struct {
+            uint32_t leaf, subleaf;
+        };
+        uint32_t input[2];
+    };
+    char *policy[4];
+};
+
 _hidden int libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid, bool retore,
                                 libxl_domain_build_info *info);
 
-- 
2.30.1

Re: [PATCH 01/21] libxl: don't ignore the return value from xc_cpuid_apply_policy

[Jan Beulich]

On 23.03.2021 10:58, Roger Pau Monne wrote:
> Also change libxl__cpuid_legacy to propagate the error from
> xc_cpuid_apply_policy into callers.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

This looks to do what it means to do and also none of the present
callers of the functions modified here ignore the return values, so
Reviewed-by: Jan Beulich <jbeulich@suse.com>

I wonder however how to ensure similar problems won't get
re-introduced down the road. In the hypervisor I'd recommend adding
__must_check everywhere, but I have no idea what the tool stack
policy is in this regard.

Jan

Re: [PATCH 04/21] libs/guest: introduce helper to fetch a system cpu policy

[Jan Beulich]

On 23.03.2021 10:58, Roger Pau Monne wrote:
> Such helper is based on the existing functions to fetch a CPUID and
> MSR policies, but uses the xc_cpu_policy_t type to return the data to
> the caller.
> 
> Note some helper functions are introduced, those are split from
> xc_cpu_policy_get_system because they will be used by other functions
> also.
> 
> No user of the interface introduced on the patch.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>
with just one minor remark below:

> --- a/tools/include/xenctrl.h
> +++ b/tools/include/xenctrl.h
> @@ -2596,6 +2596,10 @@ typedef struct cpu_policy *xc_cpu_policy_t;
>  xc_cpu_policy_t xc_cpu_policy_init(void);
>  void xc_cpu_policy_destroy(xc_cpu_policy_t policy);
>  
> +/* Retrieve a system policy, or get/set a domains policy. */
> +int xc_cpu_policy_get_system(xc_interface *xch, unsigned int idx,
> +                             xc_cpu_policy_t policy);
> +
>  int xc_get_cpu_levelling_caps(xc_interface *xch, uint32_t *caps);
>  int xc_get_cpu_featureset(xc_interface *xch, uint32_t index,
>                            uint32_t *nr_features, uint32_t *featureset);
> diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
> index ade5281c178..3710fb63839 100644
> --- a/tools/libs/guest/xg_cpuid_x86.c
> +++ b/tools/libs/guest/xg_cpuid_x86.c
> @@ -687,3 +687,93 @@ void xc_cpu_policy_destroy(xc_cpu_policy_t policy)
>      free(policy->msr);
>      free(policy);
>  }
> +
> +static int allocate_buffers(xc_interface *xch,
> +                            unsigned int *nr_leaves, xen_cpuid_leaf_t **leaves,
> +                            unsigned int *nr_msrs, xen_msr_entry_t **msrs)
> +{
> +    int rc;
> +
> +    *leaves = NULL;
> +    *msrs = NULL;
> +
> +    rc = xc_cpu_policy_get_size(xch, nr_leaves, nr_msrs);
> +    if ( rc )
> +    {
> +        PERROR("Failed to obtain policy info size");
> +        return -errno;
> +    }
> +
> +    *leaves = calloc(*nr_leaves, sizeof(**leaves));
> +    *msrs = calloc(*nr_msrs, sizeof(**msrs));
> +    if ( !*leaves || !*msrs )
> +    {
> +        PERROR("Failed to allocate resources");
> +        free(*leaves);
> +        free(*msrs);
> +        return -ENOMEM;
> +    }
> +
> +    return 0;
> +}
> +
> +static int deserialize_policy(xc_interface *xch, xc_cpu_policy_t policy,
> +                              unsigned int nr_leaves,
> +                              const xen_cpuid_leaf_t *leaves,
> +                              unsigned int nr_msrs, const xen_msr_entry_t *msrs)
> +{
> +    uint32_t err_leaf = -1, err_subleaf = -1, err_msr = -1;
> +    int rc;
> +
> +    rc = x86_cpuid_copy_from_buffer(policy->cpuid, leaves, nr_leaves,
> +                                    &err_leaf, &err_subleaf);
> +    if ( rc )
> +    {
> +        ERROR("Failed to deserialise CPUID (err leaf %#x, subleaf %#x) (%d = %s)",
> +              err_leaf, err_subleaf, -rc, strerror(-rc));
> +        return rc;
> +    }
> +
> +    rc = x86_msr_copy_from_buffer(policy->msr, msrs, nr_msrs, &err_msr);
> +    if ( rc )
> +    {
> +        ERROR("Failed to deserialise MSR (err MSR %#x) (%d = %s)",
> +              err_msr, -rc, strerror(-rc));
> +        return rc;
> +    }
> +
> +    return 0;
> +}
> +
> +int xc_cpu_policy_get_system(xc_interface *xch, unsigned int idx,
> +                             xc_cpu_policy_t policy)
> +{
> +    unsigned int nr_leaves, nr_msrs;
> +    xen_cpuid_leaf_t *leaves = NULL;
> +    xen_msr_entry_t *msrs = NULL;
> +    int rc;
> +
> +    rc = allocate_buffers(xch, &nr_leaves, &leaves, &nr_msrs, &msrs);
> +    if ( rc )
> +    {
> +        errno = -rc;
> +        return -1;
> +    }
> +
> +    rc = xc_get_system_cpu_policy(xch, idx, &nr_leaves, leaves, &nr_msrs, msrs);
> +    if ( rc )
> +    {
> +        PERROR("Failed to obtain %u policy", idx);
> +        rc = -1;
> +        goto out;
> +    }
> +
> +    rc = deserialize_policy(xch, policy, nr_leaves, leaves, nr_msrs, msrs);
> +    if ( rc )
> +        errno = -rc;

Perhaps also set rc to -1 here for consistency?

Jan

Re: [PATCH 05/21] libs/guest: introduce helper to fetch a domain cpu policy

[Jan Beulich]

On 23.03.2021 10:58, Roger Pau Monne wrote:
> Such helper is based on the existing functions to fetch a CPUID and
> MSR policies, but uses the xc_cpu_policy_t type to return the data to
> the caller.
> 
> No user of the interface introduced on the patch.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>
with again a minor remark (plus of course the same that I made for
patch 4):

> --- a/tools/include/xenctrl.h
> +++ b/tools/include/xenctrl.h
> @@ -2599,6 +2599,8 @@ void xc_cpu_policy_destroy(xc_cpu_policy_t policy);
>  /* Retrieve a system policy, or get/set a domains policy. */
>  int xc_cpu_policy_get_system(xc_interface *xch, unsigned int idx,
>                               xc_cpu_policy_t policy);
> +int xc_cpu_policy_get_domain(xc_interface *xch, uint32_t domid,
> +                             xc_cpu_policy_t policy);

Generally I'd expect domid_t to be used for domain IDs.

Jan

Re: [PATCH 01/21] libxl: don't ignore the return value from xc_cpuid_apply_policy

[Andrew Cooper]

On 23/03/2021 09:58, Roger Pau Monne wrote:
> Also change libxl__cpuid_legacy to propagate the error from
> xc_cpuid_apply_policy into callers.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

This path has never previously had error checking.

The set-cpu-policy hypercall, in principle, returns a triple of (leaf,
subleaf, msr) to try and help the caller fractionally more than just
getting EINVAL, but doesn't actually fail yet for interesting reasons.

My plan was only to wire up the error handling with the new interface,
which requires plumbing the extra failure information through into
suitable locations, and ideally also looking up the offending values to
render into error messages.

~Andrew

Re: [PATCH 11/21] libs/guest: allow updating a cpu policy CPUID data

[Jan Beulich]

On 23.03.2021 10:58, Roger Pau Monne wrote:
> --- a/tools/libs/guest/xg_cpuid_x86.c
> +++ b/tools/libs/guest/xg_cpuid_x86.c
> @@ -966,3 +966,70 @@ int xc_cpu_policy_get_msr(xc_interface *xch, const xc_cpu_policy_t policy,
>      free(msrs);
>      return rc;
>  }
> +
> +int xc_cpu_policy_update_cpuid(xc_interface *xch, xc_cpu_policy_t policy,
> +                               const xen_cpuid_leaf_t *leaves,
> +                               uint32_t nr)
> +{
> +    unsigned int err_leaf = -1, err_subleaf = -1;
> +    unsigned int nr_leaves, nr_msrs, i, j;
> +    xen_cpuid_leaf_t *current;
> +    int rc = xc_cpu_policy_get_size(xch, &nr_leaves, &nr_msrs);
> +
> +    if ( rc )
> +    {
> +        PERROR("Failed to obtain policy info size");
> +        return -1;
> +    }
> +
> +    current = calloc(nr_leaves, sizeof(*current));
> +    if ( !current )
> +    {
> +        PERROR("Failed to allocate resources");
> +        errno = ENOMEM;
> +        return -1;
> +    }
> +
> +    rc = xc_cpu_policy_serialise(xch, policy, current, &nr_leaves, NULL, 0);
> +    if ( rc )
> +        goto out;
> +
> +    for ( i = 0; i < nr; i++ )
> +    {
> +        const xen_cpuid_leaf_t *update = &leaves[i];
> +
> +        for ( j = 0; j < nr_leaves; j++ )
> +            if ( current[j].leaf == update->leaf &&
> +                 current[j].subleaf == update->subleaf )
> +            {
> +                /*
> +                 * NB: cannot use an assignation because of the const vs
> +                 * non-const difference.
> +                 */
> +                memcpy(&current[j], update, sizeof(*update));

I'm having trouble understanding the comment. In

    current[j] = *update;

the lvalue is xen_cpuid_leaf_t and the rvalue is const xen_cpuid_leaf_t.
That the usual (and permitted) arrangement afaics.

Jan

Re: [PATCH 14/21] libs/guest: introduce helper to check cpu policy compatibility

[Jan Beulich]

On 23.03.2021 10:58, Roger Pau Monne wrote:
> --- a/tools/libs/guest/xg_cpuid_x86.c
> +++ b/tools/libs/guest/xg_cpuid_x86.c
> @@ -1098,3 +1098,20 @@ int xc_cpu_policy_update_msrs(xc_interface *xch, xc_cpu_policy_t policy,
>      return rc;
>  
>  }
> +
> +bool xc_cpu_policy_is_compatible(xc_interface *xch, const xc_cpu_policy_t p1,
> +                                 const xc_cpu_policy_t p2)
> +{
> +    struct cpu_policy_errors err;

Don't you need an initializer here for ...

> +    int rc = x86_cpu_policies_are_compatible(p1, p2, &err);
> +
> +    if ( !rc )
> +        return true;
> +
> +    if ( err.leaf != -1 )
> +        ERROR("Leaf %#x subleaf %#x is not compatible", err.leaf, err.subleaf);
> +    if ( err.msr != -1 )
> +        ERROR("MSR index %#x is not compatible", err.msr);

... these checks to have a chance of actually triggering? (I'm also
not certain -1 is a good indicator, but I guess we have been using it
elsewhere as well.)

Jan

Re: [PATCH 05/21] libs/guest: introduce helper to fetch a domain cpu policy

[Roger Pau Monné]

On Tue, Mar 30, 2021 at 05:37:02PM +0200, Jan Beulich wrote:
> On 23.03.2021 10:58, Roger Pau Monne wrote:
> > Such helper is based on the existing functions to fetch a CPUID and
> > MSR policies, but uses the xc_cpu_policy_t type to return the data to
> > the caller.
> > 
> > No user of the interface introduced on the patch.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> with again a minor remark (plus of course the same that I made for
> patch 4):
> 
> > --- a/tools/include/xenctrl.h
> > +++ b/tools/include/xenctrl.h
> > @@ -2599,6 +2599,8 @@ void xc_cpu_policy_destroy(xc_cpu_policy_t policy);
> >  /* Retrieve a system policy, or get/set a domains policy. */
> >  int xc_cpu_policy_get_system(xc_interface *xch, unsigned int idx,
> >                               xc_cpu_policy_t policy);
> > +int xc_cpu_policy_get_domain(xc_interface *xch, uint32_t domid,
> > +                             xc_cpu_policy_t policy);
> 
> Generally I'd expect domid_t to be used for domain IDs.

Me too, but xenctrl.h seems to consistently use uint32_t for domain
ids. I'm fine to use domid_t here, but I assumed there was a reason
for using uint32_t uniformly there.

Thanks, Roger.

Re: [PATCH 14/21] libs/guest: introduce helper to check cpu policy compatibility

[Roger Pau Monné]

On Tue, Mar 30, 2021 at 06:02:45PM +0200, Jan Beulich wrote:
> On 23.03.2021 10:58, Roger Pau Monne wrote:
> > --- a/tools/libs/guest/xg_cpuid_x86.c
> > +++ b/tools/libs/guest/xg_cpuid_x86.c
> > @@ -1098,3 +1098,20 @@ int xc_cpu_policy_update_msrs(xc_interface *xch, xc_cpu_policy_t policy,
> >      return rc;
> >  
> >  }
> > +
> > +bool xc_cpu_policy_is_compatible(xc_interface *xch, const xc_cpu_policy_t p1,
> > +                                 const xc_cpu_policy_t p2)
> > +{
> > +    struct cpu_policy_errors err;
> 
> Don't you need an initializer here for ...
> 
> > +    int rc = x86_cpu_policies_are_compatible(p1, p2, &err);
> > +
> > +    if ( !rc )
> > +        return true;
> > +
> > +    if ( err.leaf != -1 )
> > +        ERROR("Leaf %#x subleaf %#x is not compatible", err.leaf, err.subleaf);
> > +    if ( err.msr != -1 )
> > +        ERROR("MSR index %#x is not compatible", err.msr);
> 
> ... these checks to have a chance of actually triggering? (I'm also
> not certain -1 is a good indicator, but I guess we have been using it
> elsewhere as well.)

Well, this is strictly the error path, at which point I would expect
err to be properly set by x86_cpu_policies_are_compatible. I can
however initialize err for safety here.

Thanks, Roger.

Re: [PATCH 11/21] libs/guest: allow updating a cpu policy CPUID data

[Roger Pau Monné]

On Tue, Mar 30, 2021 at 05:56:35PM +0200, Jan Beulich wrote:
> On 23.03.2021 10:58, Roger Pau Monne wrote:
> > --- a/tools/libs/guest/xg_cpuid_x86.c
> > +++ b/tools/libs/guest/xg_cpuid_x86.c
> > @@ -966,3 +966,70 @@ int xc_cpu_policy_get_msr(xc_interface *xch, const xc_cpu_policy_t policy,
> >      free(msrs);
> >      return rc;
> >  }
> > +
> > +int xc_cpu_policy_update_cpuid(xc_interface *xch, xc_cpu_policy_t policy,
> > +                               const xen_cpuid_leaf_t *leaves,
> > +                               uint32_t nr)
> > +{
> > +    unsigned int err_leaf = -1, err_subleaf = -1;
> > +    unsigned int nr_leaves, nr_msrs, i, j;
> > +    xen_cpuid_leaf_t *current;
> > +    int rc = xc_cpu_policy_get_size(xch, &nr_leaves, &nr_msrs);
> > +
> > +    if ( rc )
> > +    {
> > +        PERROR("Failed to obtain policy info size");
> > +        return -1;
> > +    }
> > +
> > +    current = calloc(nr_leaves, sizeof(*current));
> > +    if ( !current )
> > +    {
> > +        PERROR("Failed to allocate resources");
> > +        errno = ENOMEM;
> > +        return -1;
> > +    }
> > +
> > +    rc = xc_cpu_policy_serialise(xch, policy, current, &nr_leaves, NULL, 0);
> > +    if ( rc )
> > +        goto out;
> > +
> > +    for ( i = 0; i < nr; i++ )
> > +    {
> > +        const xen_cpuid_leaf_t *update = &leaves[i];
> > +
> > +        for ( j = 0; j < nr_leaves; j++ )
> > +            if ( current[j].leaf == update->leaf &&
> > +                 current[j].subleaf == update->subleaf )
> > +            {
> > +                /*
> > +                 * NB: cannot use an assignation because of the const vs
> > +                 * non-const difference.
> > +                 */
> > +                memcpy(&current[j], update, sizeof(*update));
> 
> I'm having trouble understanding the comment. In
> 
>     current[j] = *update;
> 
> the lvalue is xen_cpuid_leaf_t and the rvalue is const xen_cpuid_leaf_t.
> That the usual (and permitted) arrangement afaics.

I'm sure I was doing something really stupid, and as a bonus I failed
to properly parse the error message I got from the compiler. It's now
fixed here and below.

Thanks, Roger.

Re: [PATCH 14/21] libs/guest: introduce helper to check cpu policy compatibility

[Jan Beulich]

On 31.03.2021 14:40, Roger Pau Monné wrote:
> On Tue, Mar 30, 2021 at 06:02:45PM +0200, Jan Beulich wrote:
>> On 23.03.2021 10:58, Roger Pau Monne wrote:
>>> --- a/tools/libs/guest/xg_cpuid_x86.c
>>> +++ b/tools/libs/guest/xg_cpuid_x86.c
>>> @@ -1098,3 +1098,20 @@ int xc_cpu_policy_update_msrs(xc_interface *xch, xc_cpu_policy_t policy,
>>>      return rc;
>>>  
>>>  }
>>> +
>>> +bool xc_cpu_policy_is_compatible(xc_interface *xch, const xc_cpu_policy_t p1,
>>> +                                 const xc_cpu_policy_t p2)
>>> +{
>>> +    struct cpu_policy_errors err;
>>
>> Don't you need an initializer here for ...
>>
>>> +    int rc = x86_cpu_policies_are_compatible(p1, p2, &err);
>>> +
>>> +    if ( !rc )
>>> +        return true;
>>> +
>>> +    if ( err.leaf != -1 )
>>> +        ERROR("Leaf %#x subleaf %#x is not compatible", err.leaf, err.subleaf);
>>> +    if ( err.msr != -1 )
>>> +        ERROR("MSR index %#x is not compatible", err.msr);
>>
>> ... these checks to have a chance of actually triggering? (I'm also
>> not certain -1 is a good indicator, but I guess we have been using it
>> elsewhere as well.)
> 
> Well, this is strictly the error path, at which point I would expect
> err to be properly set by x86_cpu_policies_are_compatible. I can
> however initialize err for safety here.

Aiui in the error case one of the two, but not both fields would
be set. Without initializer you'd likely find both of them != -1,
though.

Jan

Re: [PATCH 15/21] libs/guest: obtain a compatible cpu policy from two input ones

[Jan Beulich]

On 23.03.2021 10:58, Roger Pau Monne wrote:
> Introduce a helper to obtain a compatible cpu policy based on two
> input cpu policies. Currently this is done by and'ing all CPUID leaves
> and MSR entries, except for MSR_ARCH_CAPABILITIES which has the RSBA
> bit or'ed.

I'm afraid this is too simplistic. It might do as an initial
approximation if limited to the feature flag leaves, but you
can't reasonably AND together e.g. leaf 0 output.

> @@ -1115,3 +1116,117 @@ bool xc_cpu_policy_is_compatible(xc_interface *xch, const xc_cpu_policy_t p1,
>  
>      return false;
>  }
> +
> +static uint64_t level_msr(unsigned int index, uint64_t val1, uint64_t val2)
> +{
> +    uint64_t val;
> +
> +    switch( index )
> +    {
> +    case MSR_ARCH_CAPABILITIES:
> +        val = val1 & val2;

Considering you need this even here, how about making this the
initializer of the variable, allowing to drop "default:"
altogether?

Also, nit: Missing blank after "switch".

> +    index = 0;
> +    for ( i = 0; i < p1_nr_leaves; i++ )
> +        for ( j = 0; j < p2_nr_leaves; j++ )
> +            if ( p1_leaves[i].leaf == p2_leaves[j].leaf &&
> +                 p1_leaves[i].subleaf == p2_leaves[j].subleaf )
> +            {
> +                leaves[index].leaf = p1_leaves[i].leaf;
> +                leaves[index].subleaf = p1_leaves[i].subleaf;
> +                leaves[index].a = p1_leaves[i].a & p2_leaves[j].a;
> +                leaves[index].b = p1_leaves[i].b & p2_leaves[j].b;
> +                leaves[index].c = p1_leaves[i].c & p2_leaves[j].c;
> +                leaves[index].d = p1_leaves[i].d & p2_leaves[j].d;
> +                index++;
> +            }
> +    nr_leaves = index;
> +
> +    index = 0;
> +    for ( i = 0; i < p1_nr_msrs; i++ )
> +        for ( j = 0; j < p2_nr_msrs; j++ )
> +            if ( p1_msrs[i].idx == p2_msrs[j].idx )
> +            {
> +                msrs[index].idx = p1_msrs[i].idx;
> +                msrs[index].val = level_msr(p1_msrs[i].idx,
> +                                            p1_msrs[i].val, p2_msrs[j].val);
> +                index++;
> +            }
> +    nr_msrs = index;

Mid- to long-term I'd be afraid of this going to take too long for
at least the MSRs. Can't we build on some similarity in the ordering
of both arrays, to avoid the double for()s?

Jan

Re: [PATCH 16/21] libs/guest: make a cpu policy compatible with older Xen versions

[Jan Beulich]

On 23.03.2021 10:58, Roger Pau Monne wrote:
> --- a/tools/libs/guest/xg_cpuid_x86.c
> +++ b/tools/libs/guest/xg_cpuid_x86.c
> @@ -436,6 +436,7 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
>      unsigned int i, nr_leaves, nr_msrs;
>      xen_cpuid_leaf_t *leaves = NULL;
>      struct cpuid_policy *p = NULL;
> +    struct cpu_policy policy = { };
>      uint32_t err_leaf = -1, err_subleaf = -1, err_msr = -1;
>      uint32_t host_featureset[FEATURESET_NR_ENTRIES] = {};
>      uint32_t len = ARRAY_SIZE(host_featureset);
> @@ -504,12 +505,8 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
>       */
>      if ( restore )
>      {
> -        p->basic.rdrand = test_bit(X86_FEATURE_RDRAND, host_featureset);
> -
> -        if ( di.hvm )
> -        {
> -            p->feat.mpx = test_bit(X86_FEATURE_MPX, host_featureset);
> -        }
> +        policy.cpuid = p;
> +        xc_cpu_policy_make_compatible(xch, &policy, di.hvm);
>      }

The comment ahead of this if() wants moving to ...

> @@ -1230,3 +1227,33 @@ int xc_cpu_policy_calc_compatible(xc_interface *xch,
>  
>      return rc;
>  }
> +
> +int xc_cpu_policy_make_compatible(xc_interface *xch, xc_cpu_policy_t policy,
> +                                  bool hvm)
> +{
> +    xc_cpu_policy_t host;
> +    int rc;
> +
> +    host = xc_cpu_policy_init();
> +    if ( !host )
> +    {
> +        errno = ENOMEM;
> +        return -1;
> +    }
> +
> +    rc = xc_cpu_policy_get_system(xch, XEN_SYSCTL_cpu_policy_host, host);
> +    if ( rc )
> +    {
> +        ERROR("Failed to get host policy");
> +        goto out;
> +    }
> +
> +    policy->cpuid->basic.rdrand = host->cpuid->basic.rdrand;
> +
> +    if ( hvm )
> +        policy->cpuid->feat.mpx = host->cpuid->feat.mpx;

... or cloning ahead of these two.

Jan

Re: [PATCH 01/21] libxl: don't ignore the return value from xc_cpuid_apply_policy

[Andrew Cooper]

On 23/03/2021 09:58, Roger Pau Monne wrote:
> @@ -462,8 +464,13 @@ void libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid, bool restore,
>      itsc = (libxl_defbool_val(info->disable_migrate) ||
>              info->tsc_mode == LIBXL_TSC_MODE_ALWAYS_EMULATE);
>  
> -    xc_cpuid_apply_policy(ctx->xch, domid, restore, NULL, 0,
> -                          pae, itsc, nested_virt, info->cpuid);
> +    rc = xc_cpuid_apply_policy(ctx->xch, domid, restore, NULL, 0,
> +                               pae, itsc, nested_virt, info->cpuid);
> +    if (rc)
> +        LOGE(ERROR, "Failed to apply CPUID policy");

If we are planning to take this patch, then you need to convert from xc
errors (-errno) to libxl errors here, or the caller is going to receive
gibberish.

~Andrew

> +
> +    GC_FREE;
> +    return rc;
>  }

Re: [PATCH 02/21] libs/guest: rename xc_get_cpu_policy_size to xc_cpu_policy_get_size

[Andrew Cooper]

On 23/03/2021 09:58, Roger Pau Monne wrote:
> Preparatory change to introduce a new set of xc_cpu_policy_* functions
> that will replace the current CPUID/MSR helpers.
>
> No functional change intended.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

Re: [PATCH 03/21] libs/guest: introduce xc_cpu_policy_t

[Andrew Cooper]

On 23/03/2021 09:58, Roger Pau Monne wrote:
> Introduce an opaque type that is used to store the CPUID and MSRs
> policies of a domain. Such type uses the existing cpu_policy structure
> to store the data, but doesn't expose the type to the users of the
> xenguest library.
>
> Introduce an allocation (init) and freeing function (destroy) to
> manage the type.
>
> Note the type is not yet used anywhere.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
>  tools/include/xenctrl.h         |  6 ++++++
>  tools/libs/guest/xg_cpuid_x86.c | 28 ++++++++++++++++++++++++++++
>  2 files changed, 34 insertions(+)
>
> diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
> index e91ff92b9b1..ffb3024bfeb 100644
> --- a/tools/include/xenctrl.h
> +++ b/tools/include/xenctrl.h
> @@ -2590,6 +2590,12 @@ int xc_psr_get_domain_data(xc_interface *xch, uint32_t domid,
>  int xc_psr_get_hw_info(xc_interface *xch, uint32_t socket,
>                         xc_psr_feat_type type, xc_psr_hw_info *hw_info);
>  
> +typedef struct cpu_policy *xc_cpu_policy_t;
> +
> +/* Create and free a xc_cpu_policy object. */
> +xc_cpu_policy_t xc_cpu_policy_init(void);
> +void xc_cpu_policy_destroy(xc_cpu_policy_t policy);
> +
>  int xc_get_cpu_levelling_caps(xc_interface *xch, uint32_t *caps);
>  int xc_get_cpu_featureset(xc_interface *xch, uint32_t index,
>                            uint32_t *nr_features, uint32_t *featureset);
> diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
> index 9846f81e1f1..ade5281c178 100644
> --- a/tools/libs/guest/xg_cpuid_x86.c
> +++ b/tools/libs/guest/xg_cpuid_x86.c
> @@ -659,3 +659,31 @@ out:
>  
>      return rc;
>  }
> +
> +xc_cpu_policy_t xc_cpu_policy_init(void)
> +{
> +    xc_cpu_policy_t policy = calloc(1, sizeof(*policy));
> +
> +    if ( !policy )
> +        return NULL;
> +
> +    policy->cpuid = calloc(1, sizeof(*policy->cpuid));
> +    policy->msr = calloc(1, sizeof(*policy->msr));
> +    if ( !policy->cpuid || !policy->msr )
> +    {
> +        xc_cpu_policy_destroy(policy);
> +        return NULL;
> +    }
> +
> +    return policy;
> +}
> +
> +void xc_cpu_policy_destroy(xc_cpu_policy_t policy)
> +{
> +    if ( !policy )
> +        return;
> +
> +    free(policy->cpuid);
> +    free(policy->msr);
> +    free(policy);
> +}

Looking at the series as a whole, we have a fair quantity of complexity
from short-lived dynamic allocations.

I suspect that the code would be rather better if we had

struct xc_cpu_policy {
    struct cpuid_policy cpuid;
    struct msr_policy msr;
    xen_cpuid_leaf_t leaves[CPUID_MAX_SERIALISED_LEAVES];
    xen_msr_entry_t msrs[MSR_MAX_SERIALISED_ENTRIES];
    /* Names perhaps subject to improvement */
};

and just made one memory allocation.

This is userspace after all, and we're taking about <4k at the moment.

All operations with Xen need to bounce through the leaves/msrs encoding
(so we're using the space a minimum of twice for any logical operation
at the higher level), and several userspace-only operations use them too.

~Andrew

Re: [PATCH 03/21] libs/guest: introduce xc_cpu_policy_t

[Roger Pau Monné]

On Wed, Mar 31, 2021 at 09:10:13PM +0100, Andrew Cooper wrote:
> On 23/03/2021 09:58, Roger Pau Monne wrote:
> > Introduce an opaque type that is used to store the CPUID and MSRs
> > policies of a domain. Such type uses the existing cpu_policy structure
> > to store the data, but doesn't expose the type to the users of the
> > xenguest library.
> >
> > Introduce an allocation (init) and freeing function (destroy) to
> > manage the type.
> >
> > Note the type is not yet used anywhere.
> >
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > ---
> >  tools/include/xenctrl.h         |  6 ++++++
> >  tools/libs/guest/xg_cpuid_x86.c | 28 ++++++++++++++++++++++++++++
> >  2 files changed, 34 insertions(+)
> >
> > diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
> > index e91ff92b9b1..ffb3024bfeb 100644
> > --- a/tools/include/xenctrl.h
> > +++ b/tools/include/xenctrl.h
> > @@ -2590,6 +2590,12 @@ int xc_psr_get_domain_data(xc_interface *xch, uint32_t domid,
> >  int xc_psr_get_hw_info(xc_interface *xch, uint32_t socket,
> >                         xc_psr_feat_type type, xc_psr_hw_info *hw_info);
> >  
> > +typedef struct cpu_policy *xc_cpu_policy_t;
> > +
> > +/* Create and free a xc_cpu_policy object. */
> > +xc_cpu_policy_t xc_cpu_policy_init(void);
> > +void xc_cpu_policy_destroy(xc_cpu_policy_t policy);
> > +
> >  int xc_get_cpu_levelling_caps(xc_interface *xch, uint32_t *caps);
> >  int xc_get_cpu_featureset(xc_interface *xch, uint32_t index,
> >                            uint32_t *nr_features, uint32_t *featureset);
> > diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
> > index 9846f81e1f1..ade5281c178 100644
> > --- a/tools/libs/guest/xg_cpuid_x86.c
> > +++ b/tools/libs/guest/xg_cpuid_x86.c
> > @@ -659,3 +659,31 @@ out:
> >  
> >      return rc;
> >  }
> > +
> > +xc_cpu_policy_t xc_cpu_policy_init(void)
> > +{
> > +    xc_cpu_policy_t policy = calloc(1, sizeof(*policy));
> > +
> > +    if ( !policy )
> > +        return NULL;
> > +
> > +    policy->cpuid = calloc(1, sizeof(*policy->cpuid));
> > +    policy->msr = calloc(1, sizeof(*policy->msr));
> > +    if ( !policy->cpuid || !policy->msr )
> > +    {
> > +        xc_cpu_policy_destroy(policy);
> > +        return NULL;
> > +    }
> > +
> > +    return policy;
> > +}
> > +
> > +void xc_cpu_policy_destroy(xc_cpu_policy_t policy)
> > +{
> > +    if ( !policy )
> > +        return;
> > +
> > +    free(policy->cpuid);
> > +    free(policy->msr);
> > +    free(policy);
> > +}
> 
> Looking at the series as a whole, we have a fair quantity of complexity
> from short-lived dynamic allocations.
> 
> I suspect that the code would be rather better if we had
> 
> struct xc_cpu_policy {
>     struct cpuid_policy cpuid;
>     struct msr_policy msr;
>     xen_cpuid_leaf_t leaves[CPUID_MAX_SERIALISED_LEAVES];
>     xen_msr_entry_t msrs[MSR_MAX_SERIALISED_ENTRIES];
>     /* Names perhaps subject to improvement */
> };
> 
> and just made one memory allocation.
> 
> This is userspace after all, and we're taking about <4k at the moment.
> 
> All operations with Xen need to bounce through the leaves/msrs encoding
> (so we're using the space a minimum of twice for any logical operation
> at the higher level), and several userspace-only operations use them too.

We would still need to do some allocations for the system policies,
but yes, it would prevent some of the short-lived allocations. I
didn't care much because it's all user-space, but removing them will
likely make the code simpler.

Thanks, Roger.

Re: [PATCH 05/21] libs/guest: introduce helper to fetch a domain cpu policy

[Andrew Cooper]

On 31/03/2021 12:06, Roger Pau Monné wrote:
> On Tue, Mar 30, 2021 at 05:37:02PM +0200, Jan Beulich wrote:
>> On 23.03.2021 10:58, Roger Pau Monne wrote:
>>> Such helper is based on the existing functions to fetch a CPUID and
>>> MSR policies, but uses the xc_cpu_policy_t type to return the data to
>>> the caller.
>>>
>>> No user of the interface introduced on the patch.
>>>
>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>> with again a minor remark (plus of course the same that I made for
>> patch 4):
>>
>>> --- a/tools/include/xenctrl.h
>>> +++ b/tools/include/xenctrl.h
>>> @@ -2599,6 +2599,8 @@ void xc_cpu_policy_destroy(xc_cpu_policy_t policy);
>>>  /* Retrieve a system policy, or get/set a domains policy. */
>>>  int xc_cpu_policy_get_system(xc_interface *xch, unsigned int idx,
>>>                               xc_cpu_policy_t policy);
>>> +int xc_cpu_policy_get_domain(xc_interface *xch, uint32_t domid,
>>> +                             xc_cpu_policy_t policy);
>> Generally I'd expect domid_t to be used for domain IDs.
> Me too, but xenctrl.h seems to consistently use uint32_t for domain
> ids. I'm fine to use domid_t here, but I assumed there was a reason
> for using uint32_t uniformly there.

There was a tools-wide change making everything uint32_t a while ago,
but libxc itself has never used domid_t.  IIRC, it was to do with
problems concerning the INVALID_DOMID constant.

~Andrew

Re: [PATCH 04/21] libs/guest: introduce helper to fetch a system cpu policy

[Andrew Cooper]

On 23/03/2021 09:58, Roger Pau Monne wrote:
> Such helper is based on the existing functions to fetch a CPUID and
> MSR policies, but uses the xc_cpu_policy_t type to return the data to
> the caller.
>
> Note some helper functions are introduced, those are split from
> xc_cpu_policy_get_system because they will be used by other functions
> also.
>
> No user of the interface introduced on the patch.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
>  tools/include/xenctrl.h         |  4 ++
>  tools/libs/guest/xg_cpuid_x86.c | 90 +++++++++++++++++++++++++++++++++
>  2 files changed, 94 insertions(+)
>
> diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
> index ffb3024bfeb..fc8e4b28781 100644
> --- a/tools/include/xenctrl.h
> +++ b/tools/include/xenctrl.h
> @@ -2596,6 +2596,10 @@ typedef struct cpu_policy *xc_cpu_policy_t;
>  xc_cpu_policy_t xc_cpu_policy_init(void);
>  void xc_cpu_policy_destroy(xc_cpu_policy_t policy);
>  
> +/* Retrieve a system policy, or get/set a domains policy. */
> +int xc_cpu_policy_get_system(xc_interface *xch, unsigned int idx,
> +                             xc_cpu_policy_t policy);

I'd recommend "policy_idx" as the parameter name.

> +static int deserialize_policy(xc_interface *xch, xc_cpu_policy_t policy,
> +                              unsigned int nr_leaves,
> +                              const xen_cpuid_leaf_t *leaves,
> +                              unsigned int nr_msrs, const xen_msr_entry_t *msrs)
> +{
> +    uint32_t err_leaf = -1, err_subleaf = -1, err_msr = -1;
> +    int rc;
> +
> +    rc = x86_cpuid_copy_from_buffer(policy->cpuid, leaves, nr_leaves,
> +                                    &err_leaf, &err_subleaf);
> +    if ( rc )
> +    {
> +        ERROR("Failed to deserialise CPUID (err leaf %#x, subleaf %#x) (%d = %s)",
> +              err_leaf, err_subleaf, -rc, strerror(-rc));
> +        return rc;
> +    }
> +
> +    rc = x86_msr_copy_from_buffer(policy->msr, msrs, nr_msrs, &err_msr);
> +    if ( rc )
> +    {
> +        ERROR("Failed to deserialise MSR (err MSR %#x) (%d = %s)",
> +              err_msr, -rc, strerror(-rc));

We possibly want to split out helpers to render the error information. 
For MSRs, it ought to be something like

if ( msr == -1 )
    // general -ENOMEM etc
else
    // MSR-specific error.  render index and value.

I think we can probably even depend on MSR-specific errors always being
-EINVAL.

~Andrew

Re: [PATCH 09/21] libs/guest: allow fetching a specific CPUID leaf from a cpu policy

[Andrew Cooper]

On 23/03/2021 09:58, Roger Pau Monne wrote:
> diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
> index 48351f1c4c6..a1e1bf10d5c 100644
> --- a/tools/libs/guest/xg_cpuid_x86.c
> +++ b/tools/libs/guest/xg_cpuid_x86.c
> @@ -883,3 +883,45 @@ int xc_cpu_policy_serialise(xc_interface *xch, const xc_cpu_policy_t p,
>      errno = 0;
>      return 0;
>  }
> +
> +int xc_cpu_policy_get_cpuid(xc_interface *xch, const xc_cpu_policy_t policy,
> +                            uint32_t leaf, uint32_t subleaf,
> +                            xen_cpuid_leaf_t *out)
> +{
> +    unsigned int nr_leaves, nr_msrs, i;
> +    xen_cpuid_leaf_t *leaves;
> +    int rc = xc_cpu_policy_get_size(xch, &nr_leaves, &nr_msrs);
> +
> +    if ( rc )
> +    {
> +        PERROR("Failed to obtain policy info size");
> +        return -1;
> +    }
> +
> +    leaves = calloc(nr_leaves, sizeof(*leaves));
> +    if ( !leaves )
> +    {
> +        PERROR("Failed to allocate resources");
> +        errno = ENOMEM;
> +        return -1;
> +    }
> +
> +    rc = xc_cpu_policy_serialise(xch, policy, leaves, &nr_leaves, NULL, 0);
> +    if ( rc )
> +        goto out;
> +
> +    for ( i = 0; i < nr_leaves; i++ )
> +        if ( leaves[i].leaf == leaf && leaves[i].subleaf == subleaf )
> +        {
> +            *out = leaves[i];
> +            goto out;
> +        }

Please adapt find_leaf(), probably by dropping xc_xend_cpuid and passing
in leaf/subleaf parameters.

Serialised leaves are sorted and there are plenty of them, so a log
search is better.

How frequent is this call going to be for the same policy?  With the
arrays embedded in a policy, they're still around, and serialise is an
expensive operation.

I wonder if it makes sense to try and keep both forms in sync, so we can
avoid redundant calls like this?

~Andrew

Re: [PATCH 11/21] libs/guest: allow updating a cpu policy CPUID data

[Andrew Cooper]

On 23/03/2021 09:58, Roger Pau Monne wrote:
> diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
> index 091aeb70c9c..13c2972ccd3 100644
> --- a/tools/libs/guest/xg_cpuid_x86.c
> +++ b/tools/libs/guest/xg_cpuid_x86.c
> @@ -966,3 +966,70 @@ int xc_cpu_policy_get_msr(xc_interface *xch, const xc_cpu_policy_t policy,
>      free(msrs);
>      return rc;
>  }
> +
> +int xc_cpu_policy_update_cpuid(xc_interface *xch, xc_cpu_policy_t policy,
> +                               const xen_cpuid_leaf_t *leaves,
> +                               uint32_t nr)
> +{
> +    unsigned int err_leaf = -1, err_subleaf = -1;
> +    unsigned int nr_leaves, nr_msrs, i, j;
> +    xen_cpuid_leaf_t *current;
> +    int rc = xc_cpu_policy_get_size(xch, &nr_leaves, &nr_msrs);
> +
> +    if ( rc )
> +    {
> +        PERROR("Failed to obtain policy info size");
> +        return -1;
> +    }
> +
> +    current = calloc(nr_leaves, sizeof(*current));
> +    if ( !current )
> +    {
> +        PERROR("Failed to allocate resources");
> +        errno = ENOMEM;
> +        return -1;
> +    }
> +
> +    rc = xc_cpu_policy_serialise(xch, policy, current, &nr_leaves, NULL, 0);
> +    if ( rc )
> +        goto out;
> +
> +    for ( i = 0; i < nr; i++ )
> +    {
> +        const xen_cpuid_leaf_t *update = &leaves[i];
> +
> +        for ( j = 0; j < nr_leaves; j++ )
> +            if ( current[j].leaf == update->leaf &&
> +                 current[j].subleaf == update->subleaf )
> +            {
> +                /*
> +                 * NB: cannot use an assignation because of the const vs
> +                 * non-const difference.
> +                 */
> +                memcpy(&current[j], update, sizeof(*update));
> +                break;
> +            }
> +
> +        if ( j == nr_leaves )
> +        {
> +            /* Failed to find a matching leaf, append to the end. */
> +            current = realloc(current, (nr_leaves + 1) * sizeof(*current));
> +            memcpy(&current[nr_leaves], update, sizeof(*update));
> +            nr_leaves++;
> +        }
> +    }
> +
> +    rc = x86_cpuid_copy_from_buffer(policy->cpuid, current, nr_leaves,
> +                                    &err_leaf, &err_subleaf);

Why do you need any earlier logic?  x86_cpuid_copy_from_buffer() already
does exactly this operation.

~Andrew

Re: [PATCH 13/21] libs/guest: switch users of xc_set_domain_cpu_policy

[Andrew Cooper]

On 23/03/2021 09:58, Roger Pau Monne wrote:
> To use the new cpu policy interface xc_cpu_policy_set_domain. Note
> that xc_set_domain_cpu_policy is removed from the interface and the
> function is made static to xg_cpuid_x86.c for it's internal callers.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
>  tools/include/xenctrl.h             |  5 -----
>  tools/libs/guest/xg_cpuid_x86.c     | 22 +++++++++++-----------
>  tools/libs/guest/xg_sr_common_x86.c | 28 +++++++++++++++++++++-------
>  3 files changed, 32 insertions(+), 23 deletions(-)
>
> diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
> index 46f5026081c..164a287b367 100644
> --- a/tools/include/xenctrl.h
> +++ b/tools/include/xenctrl.h
> @@ -2625,11 +2625,6 @@ int xc_get_cpu_featureset(xc_interface *xch, uint32_t index,
>  
>  int xc_cpu_policy_get_size(xc_interface *xch, uint32_t *nr_leaves,
>                             uint32_t *nr_msrs);
> -int xc_set_domain_cpu_policy(xc_interface *xch, uint32_t domid,
> -                             uint32_t nr_leaves, xen_cpuid_leaf_t *leaves,
> -                             uint32_t nr_msrs, xen_msr_entry_t *msrs,
> -                             uint32_t *err_leaf_p, uint32_t *err_subleaf_p,
> -                             uint32_t *err_msr_p);
>  
>  uint32_t xc_get_cpu_featureset_size(void);
>  
> diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
> index 07756743e76..f7b662f31aa 100644
> --- a/tools/libs/guest/xg_cpuid_x86.c
> +++ b/tools/libs/guest/xg_cpuid_x86.c
> @@ -204,11 +204,11 @@ static int get_domain_cpu_policy(xc_interface *xch, uint32_t domid,
>      return ret;
>  }
>  
> -int xc_set_domain_cpu_policy(xc_interface *xch, uint32_t domid,
> -                             uint32_t nr_leaves, xen_cpuid_leaf_t *leaves,
> -                             uint32_t nr_msrs, xen_msr_entry_t *msrs,
> -                             uint32_t *err_leaf_p, uint32_t *err_subleaf_p,
> -                             uint32_t *err_msr_p)
> +static int set_domain_cpu_policy(xc_interface *xch, uint32_t domid,
> +                                 uint32_t nr_leaves, xen_cpuid_leaf_t *leaves,
> +                                 uint32_t nr_msrs, xen_msr_entry_t *msrs,
> +                                 uint32_t *err_leaf_p, uint32_t *err_subleaf_p,
> +                                 uint32_t *err_msr_p)
>  {
>      DECLARE_DOMCTL;
>      DECLARE_HYPERCALL_BOUNCE(leaves,
> @@ -405,8 +405,8 @@ static int xc_cpuid_xend_policy(
>      }
>  
>      /* Feed the transformed currrent policy back up to Xen. */
> -    rc = xc_set_domain_cpu_policy(xch, domid, nr_cur, cur, 0, NULL,
> -                                  &err_leaf, &err_subleaf, &err_msr);
> +    rc = set_domain_cpu_policy(xch, domid, nr_cur, cur, 0, NULL,
> +                               &err_leaf, &err_subleaf, &err_msr);
>      if ( rc )
>      {
>          PERROR("Failed to set d%d's policy (err leaf %#x, subleaf %#x, msr %#x)",
> @@ -638,8 +638,8 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
>          goto out;
>      }
>  
> -    rc = xc_set_domain_cpu_policy(xch, domid, nr_leaves, leaves, 0, NULL,
> -                                  &err_leaf, &err_subleaf, &err_msr);
> +    rc = set_domain_cpu_policy(xch, domid, nr_leaves, leaves, 0, NULL,
> +                               &err_leaf, &err_subleaf, &err_msr);
>      if ( rc )
>      {
>          PERROR("Failed to set d%d's policy (err leaf %#x, subleaf %#x, msr %#x)",
> @@ -833,8 +833,8 @@ int xc_cpu_policy_set_domain(xc_interface *xch, uint32_t domid,
>      if ( rc )
>          goto out;
>  
> -    rc = xc_set_domain_cpu_policy(xch, domid, nr_leaves, leaves, nr_msrs, msrs,
> -                                  &err_leaf, &err_subleaf, &err_msr);
> +    rc = set_domain_cpu_policy(xch, domid, nr_leaves, leaves, nr_msrs, msrs,
> +                               &err_leaf, &err_subleaf, &err_msr);
>      if ( rc )
>      {
>          ERROR("Failed to set domain %u policy (%d = %s)", domid, -rc,
> diff --git a/tools/libs/guest/xg_sr_common_x86.c b/tools/libs/guest/xg_sr_common_x86.c
> index 15265e7a331..02f0c3ae9ed 100644
> --- a/tools/libs/guest/xg_sr_common_x86.c
> +++ b/tools/libs/guest/xg_sr_common_x86.c
> @@ -151,7 +151,10 @@ int x86_static_data_complete(struct xc_sr_context *ctx, unsigned int *missing)
>  {
>      xc_interface *xch = ctx->xch;
>      uint32_t nr_leaves = 0, nr_msrs = 0;
> -    uint32_t err_l = ~0, err_s = ~0, err_m = ~0;
> +    xc_cpu_policy_t policy = xc_cpu_policy_init();
> +
> +    if ( !policy )
> +        return -1;
>  
>      if ( ctx->x86.restore.cpuid.ptr )
>          nr_leaves = ctx->x86.restore.cpuid.size / sizeof(xen_cpuid_leaf_t);
> @@ -163,14 +166,25 @@ int x86_static_data_complete(struct xc_sr_context *ctx, unsigned int *missing)
>      else
>          *missing |= XGR_SDD_MISSING_MSR;
>  
> +    if ( nr_leaves &&
> +         xc_cpu_policy_update_cpuid(xch, policy,
> +                                    ctx->x86.restore.cpuid.ptr, nr_leaves) )
> +    {
> +        PERROR("Failed to update CPUID policy");
> +        return -1;
> +    }
> +    if ( nr_msrs &&
> +         xc_cpu_policy_update_msrs(xch, policy,
> +                                   ctx->x86.restore.msr.ptr, nr_msrs) )
> +    {
> +        PERROR("Failed to update MSR policy");
> +        return -1;
> +    }
> +
>      if ( (nr_leaves || nr_msrs) &&
> -         xc_set_domain_cpu_policy(xch, ctx->domid,
> -                                  nr_leaves, ctx->x86.restore.cpuid.ptr,
> -                                  nr_msrs,   ctx->x86.restore.msr.ptr,
> -                                  &err_l, &err_s, &err_m) )
> +         xc_cpu_policy_set_domain(xch, ctx->domid, policy) )
>      {
> -        PERROR("Failed to set CPUID policy: leaf %08x, subleaf %08x, msr %08x",
> -               err_l, err_s, err_m);
> +        PERROR("Failed to set CPUID policy");
>          return -1;
>      }

I don't think this is a good move.

All it does is waste time shuffling data in userspace during the VM
downtime window.  The format of CPUID/MSR data in the migration stream
is (deliberately) already in the form to hand it straight back to Xen,
and there will never be additional policy changes here (because that
would break the VM).

I'd just drop the patch entirely.  I'm not even certain if we want to
remove the thin hypercall wrappers - I'm pretty sure some of my low
level unit testing plans will want the raw accessors without being
forced through the xc_cpuid_policy_t interface.

~Andrew

Re: [PATCH 14/21] libs/guest: introduce helper to check cpu policy compatibility

[Andrew Cooper]

On 23/03/2021 09:58, Roger Pau Monne wrote:
> Such helpers is just a wrapper to the existing
> x86_cpu_policies_are_compatible function. This requires building
> policy.c from libx86 on user land also.
>
> No user of the interface introduced.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
>  tools/include/xenctrl.h         |  4 ++++
>  tools/libs/guest/Makefile       |  2 +-
>  tools/libs/guest/xg_cpuid_x86.c | 17 +++++++++++++++++
>  3 files changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
> index 164a287b367..165beff330f 100644
> --- a/tools/include/xenctrl.h
> +++ b/tools/include/xenctrl.h
> @@ -2619,6 +2619,10 @@ int xc_cpu_policy_update_cpuid(xc_interface *xch, xc_cpu_policy_t policy,
>  int xc_cpu_policy_update_msrs(xc_interface *xch, xc_cpu_policy_t policy,
>                                const xen_msr_entry_t *msrs, uint32_t nr);
>  
> +/* Compatibility calculations. */
> +bool xc_cpu_policy_is_compatible(xc_interface *xch, const xc_cpu_policy_t p1,
> +                                 const xc_cpu_policy_t p2);

Exposing this in the interface is useful and necessary.

However, the operation is not commutative.  p1 and p2 are not
interchangeable, is why the libx86 function has specifically named
parameters.

This distinction needs making clear at this level as well.

~Andrew

Re: [PATCH 15/21] libs/guest: obtain a compatible cpu policy from two input ones

[Andrew Cooper]

On 23/03/2021 09:58, Roger Pau Monne wrote:
> Introduce a helper to obtain a compatible cpu policy based on two
> input cpu policies. Currently this is done by and'ing all CPUID leaves
> and MSR entries, except for MSR_ARCH_CAPABILITIES which has the RSBA
> bit or'ed.
>
> The _AC macro is pulled from libxl_internal.h into xen-tools/libs.h
> since it's required in order to use the msr-index.h header.
>
> Note there's no need to place this helper in libx86, since the
> calculation of a compatible policy shouldn't be done from the
> hypervisor.
>
> No callers of the interface introduced.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

I think this wants to be in libx86, because we'll want it to replace the
opencoded derivation logic in init_cpuid()

Also, we absolutely want to unit test it.  (I could have sworn I already
started some work there - maybe its hidden in one of my WIP branches).

~Andrew

Re: [PATCH 16/21] libs/guest: make a cpu policy compatible with older Xen versions

[Andrew Cooper]

On 23/03/2021 09:58, Roger Pau Monne wrote:
> Older Xen versions used to expose some CPUID bits which are no longer
> exposed by default. In order to keep a compatible behavior with
> guests migrated from versions of Xen that don't encode the CPUID data
> on the migration stream introduce a function that sets the same bits
> as older Xen versions.
>
> This is pulled out from xc_cpuid_apply_policy which already has this
> logic present.
>
> No functional change intended.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
>  tools/include/xenctrl.h         |  4 ++++
>  tools/libs/guest/xg_cpuid_x86.c | 39 ++++++++++++++++++++++++++++-----
>  2 files changed, 37 insertions(+), 6 deletions(-)
>
> diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
> index 5f3e5e17e9d..6f7158156fa 100644
> --- a/tools/include/xenctrl.h
> +++ b/tools/include/xenctrl.h
> @@ -2627,6 +2627,10 @@ int xc_cpu_policy_calc_compatible(xc_interface *xch,
>                                    const xc_cpu_policy_t p2,
>                                    xc_cpu_policy_t out);
>  
> +/* Make a policy compatible with previous Xen versions. */
> +int xc_cpu_policy_make_compatible(xc_interface *xch, xc_cpu_policy_t policy,
> +                                  bool hvm);

I think this probably wants  "pre-4.14(?)" somewhere obvious, because
"compatible" on its own is very ambiguous.

~Andrew

Re: [PATCH 17/21] libs/guest: introduce helper set cpu topology in cpu policy

[Andrew Cooper]

On 23/03/2021 09:58, Roger Pau Monne wrote:
> This logic is pulled out from xc_cpuid_apply_policy and placed into a
> separate helper.
>
> No functional change intended.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
>  tools/include/xenctrl.h         |   4 +
>  tools/libs/guest/xg_cpuid_x86.c | 181 +++++++++++++++++---------------
>  2 files changed, 102 insertions(+), 83 deletions(-)
>
> diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
> index 6f7158156fa..9f94e61523e 100644
> --- a/tools/include/xenctrl.h
> +++ b/tools/include/xenctrl.h
> @@ -2631,6 +2631,10 @@ int xc_cpu_policy_calc_compatible(xc_interface *xch,
>  int xc_cpu_policy_make_compatible(xc_interface *xch, xc_cpu_policy_t policy,
>                                    bool hvm);
>  
> +/* Setup the policy topology. */
> +int xc_cpu_policy_topology(xc_interface *xch, xc_cpu_policy_t policy,
> +                           bool hvm);

I'm not sure how I feel about this.  It's repeating the mistake we've
currently got with topology handling.

One part of it needs to be part of "compatible".  We need to run the
below logic, *in this form* as part of magic-ing a policy out of thin
air for the incoming VM with no data.

However, for any non-broken logic, the caller needs to specify the
topology which wishes to be expressed.

Do we want SMT at all?  Do we want 1, 2, 4 or other threads/core.
How many cores per socket?  Its very common these days for
non-power-of-2 numbers.  Our default case ought to be to match the host
topology.

Do we want to support 3 threads/core?  Sure - its weird to think about,
but its semantically equivalent to using non-power-of-2 numbers at other
levels, and would certainly be useful to express for testing purposes.

What about Intel's leaf 0x1f with the SMT > Core > Module > Tile > Die
topology layout?

The answers to these questions also need to fix Xen so that APIC_ID
isn't vcpu_id * 2 (which is horribly broken on non-Intel or Intel
Knight* hardware).  It also needs to change how the MADT is written for
guests, and how the IO-APIC IDs are assigned (matters for the AMD
topology algorithms).

There are further implications.  Should we prohibit creating a 4-vcpu VM
with cores/socket=128?  A regular kernel will demand an IOMMU for this
configuration as we end up with APIC IDs above 255.  OTOH, there are
also virtualisation schemes now to support 32k vcpus without an IOMMU,
which KVM and HyperV now speak.

Fixing our topology problems is a monumental can of worms.  While we
should keep it in mind, we should try not to conflate it with "make
libxl/libxc's CPUID logic more sane, and include MSRs", which is large
enough task on its own.

What I suspect we want in the short term is
xc_cpu_policy_legacy_adjust() or equivalent, which is very clear that it
is a transitional API only, which for now can be used everywhere where
xc_cpuid_apply_policy() is used.  As we pull various logical areas out
of this, we'll adjust the callers appropriately, and eventually delete
this function.

~Andrew

Re: [PATCH 20/21] libs/{light,guest}: implement xc_cpuid_apply_policy in libxl

[Andrew Cooper]

On 23/03/2021 09:58, Roger Pau Monne wrote:
> With the addition of the xc_cpu_policy_* now libxl can have better
> control over the cpu policy, this allows removing the
> xc_cpuid_apply_policy function and instead coding the required bits by
> libxl in libxl__cpuid_legacy directly.
>
> Remove xc_cpuid_apply_policy.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> I'm unsure why this is called libxl__cpuid_legacy, mostly because I
> don't seem to be able to spot a libxl__cpuid (ie: non-legacy version).
> Should this be renamed to libxl__cpuid?
>
> 1b3cec69bf300e012a mentions the process of switching to a new cpuid
> interface, but I'm not sure we need to keep libxl__cpuid_legacy, since
> that's an internal interface that's not exposed to libxl clients
> anyway.

"legacy" was referring to the pre-4.14 migrate-in problem.  It was the
best I could come up with without having a firm plan of how this series
was going to look like.

That said, the resulting function is still very definitely doing thing's
I'd consider "legacy", so perhaps it wants to stay named like this until
we've got a better plan for how to enable non-trivial features.

~Andrew

Re: [PATCH 21/21] libs/guest: (re)move xc_cpu_policy_apply_cpuid

[Andrew Cooper]

On 23/03/2021 09:58, Roger Pau Monne wrote:
> Move the logic from xc_cpu_policy_apply_cpuid into libxl, now that the
> xc_cpu_policy_* helpers allow modifying a cpu policy. Having xenguest
> parsing CPUID data in xend format was a layering violation, and by
> moving such parsing into libxl directly we can get rid of
> xc_xend_cpuid, as libxl will now implement it's own private type for
> storing CPUID information, which currently matches xc_xend_cpuid.
>
> Note the function logic is moved as-is, but requires adapting to the
> libxl coding style.
>
> No functional change intended.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Its not a layering violation IMO.

It was only in 4.14 that I dropped the Python and Ocaml bindings for
xend-format strings, in an effort to simplify this work.  Before then,
libxc was absolutely the correct place for the logic to live.

I do intend to re-add python bindings in due course, but I really don't
expect that to extend to the xend format.  So moving this logic probably
fine, unless we're expecting to re-introduce it elsewhere?

~Andrew

Re: [PATCH 09/21] libs/guest: allow fetching a specific CPUID leaf from a cpu policy

[Roger Pau Monné]

On Thu, Apr 01, 2021 at 03:47:20PM +0100, Andrew Cooper wrote:
> On 23/03/2021 09:58, Roger Pau Monne wrote:
> > diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
> > index 48351f1c4c6..a1e1bf10d5c 100644
> > --- a/tools/libs/guest/xg_cpuid_x86.c
> > +++ b/tools/libs/guest/xg_cpuid_x86.c
> > @@ -883,3 +883,45 @@ int xc_cpu_policy_serialise(xc_interface *xch, const xc_cpu_policy_t p,
> >      errno = 0;
> >      return 0;
> >  }
> > +
> > +int xc_cpu_policy_get_cpuid(xc_interface *xch, const xc_cpu_policy_t policy,
> > +                            uint32_t leaf, uint32_t subleaf,
> > +                            xen_cpuid_leaf_t *out)
> > +{
> > +    unsigned int nr_leaves, nr_msrs, i;
> > +    xen_cpuid_leaf_t *leaves;
> > +    int rc = xc_cpu_policy_get_size(xch, &nr_leaves, &nr_msrs);
> > +
> > +    if ( rc )
> > +    {
> > +        PERROR("Failed to obtain policy info size");
> > +        return -1;
> > +    }
> > +
> > +    leaves = calloc(nr_leaves, sizeof(*leaves));
> > +    if ( !leaves )
> > +    {
> > +        PERROR("Failed to allocate resources");
> > +        errno = ENOMEM;
> > +        return -1;
> > +    }
> > +
> > +    rc = xc_cpu_policy_serialise(xch, policy, leaves, &nr_leaves, NULL, 0);
> > +    if ( rc )
> > +        goto out;
> > +
> > +    for ( i = 0; i < nr_leaves; i++ )
> > +        if ( leaves[i].leaf == leaf && leaves[i].subleaf == subleaf )
> > +        {
> > +            *out = leaves[i];
> > +            goto out;
> > +        }
> 
> Please adapt find_leaf(), probably by dropping xc_xend_cpuid and passing
> in leaf/subleaf parameters.
> 
> Serialised leaves are sorted and there are plenty of them, so a log
> search is better.
> 
> How frequent is this call going to be for the same policy?  With the
> arrays embedded in a policy, they're still around, and serialise is an
> expensive operation.
> 
> I wonder if it makes sense to try and keep both forms in sync, so we can
> avoid redundant calls like this?

I think we would then need to call xc_cpu_policy_serialise whenever we
update the CPUID policy in order to re-generate the full serialized
version, as I expect changes to a specific leaf can affect others
leaves, and thus the helpers should not try to update the serialized
version manually.

Hence I'm not sure it's worth to try to keep both versions in sync, as
it seems easier to load the policy, modify the policy object and then
serialize back only when needed to upload to Xen or to export.

Thanks, Roger.

Re: [PATCH 15/21] libs/guest: obtain a compatible cpu policy from two input ones

[Roger Pau Monné]

On Thu, Apr 01, 2021 at 05:26:03PM +0100, Andrew Cooper wrote:
> On 23/03/2021 09:58, Roger Pau Monne wrote:
> > Introduce a helper to obtain a compatible cpu policy based on two
> > input cpu policies. Currently this is done by and'ing all CPUID leaves
> > and MSR entries, except for MSR_ARCH_CAPABILITIES which has the RSBA
> > bit or'ed.
> >
> > The _AC macro is pulled from libxl_internal.h into xen-tools/libs.h
> > since it's required in order to use the msr-index.h header.
> >
> > Note there's no need to place this helper in libx86, since the
> > calculation of a compatible policy shouldn't be done from the
> > hypervisor.
> >
> > No callers of the interface introduced.
> >
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> I think this wants to be in libx86, because we'll want it to replace the
> opencoded derivation logic in init_cpuid()

I think you mean init_domain_cpuid_policy or recalculate_cpuid_policy?
I cannot find any init_cpuid.

I'm not convinced we need this in libx86 for the hypervisor, as I
don't know exactly how we would use it there. I expect the hypervisor
to validate policies provided by the toolstack, but not for it to
create such policies unless for very specific domains (ie: dom0 or
similar) which shouldn't require any leveling.

I'm happy to place it libx86, but I think I need to understand better
why it needs to be there.

> Also, we absolutely want to unit test it.  (I could have sworn I already
> started some work there - maybe its hidden in one of my WIP branches).

I don't think we have unit tests for the xenguest library?

Thanks, Roger.

Re: [PATCH 20/21] libs/{light,guest}: implement xc_cpuid_apply_policy in libxl

[Roger Pau Monné]

On Thu, Apr 01, 2021 at 06:44:02PM +0100, Andrew Cooper wrote:
> On 23/03/2021 09:58, Roger Pau Monne wrote:
> > With the addition of the xc_cpu_policy_* now libxl can have better
> > control over the cpu policy, this allows removing the
> > xc_cpuid_apply_policy function and instead coding the required bits by
> > libxl in libxl__cpuid_legacy directly.
> >
> > Remove xc_cpuid_apply_policy.
> >
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > ---
> > I'm unsure why this is called libxl__cpuid_legacy, mostly because I
> > don't seem to be able to spot a libxl__cpuid (ie: non-legacy version).
> > Should this be renamed to libxl__cpuid?
> >
> > 1b3cec69bf300e012a mentions the process of switching to a new cpuid
> > interface, but I'm not sure we need to keep libxl__cpuid_legacy, since
> > that's an internal interface that's not exposed to libxl clients
> > anyway.
> 
> "legacy" was referring to the pre-4.14 migrate-in problem.  It was the
> best I could come up with without having a firm plan of how this series
> was going to look like.
> 
> That said, the resulting function is still very definitely doing thing's
> I'd consider "legacy", so perhaps it wants to stay named like this until
> we've got a better plan for how to enable non-trivial features.

I don't have a strong opinion either way, but as said this is an
internal libxl function, so naming it libxl__cpuid or some such and go
expanding it as required would be perfectly fine IMO, even if the
current parameters are going to change as part of expanding it.

In any case we would not have a libxl__cpuid_legacy and a libxl__cpuid
at the same time in libxl, as those are internal helpers so at some
point when we are happy with the logic in there the _legacy suffix
would just be dropped.

Thanks, Roger.