xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 00/21] libs/guest: new CPUID/MSR interface
@ 2021-04-13 14:01 Roger Pau Monne
  2021-04-13 14:01 ` [PATCH v2 01/21] libxl: don't ignore the return value from xc_cpuid_apply_policy Roger Pau Monne
                   ` (20 more replies)
  0 siblings, 21 replies; 43+ messages in thread
From: Roger Pau Monne @ 2021-04-13 14:01 UTC (permalink / raw)
  To: xen-devel
  Cc: Andrew Cooper, Roger Pau Monne, Ian Jackson, Wei Liu,
	Anthony PERARD, Jan Beulich

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.

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
  tests/cpu-policy: add sorted MSR test
  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: 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                    |   6 +-
 tools/include/xen-tools/libs.h           |   5 +
 tools/include/xenctrl.h                  | 102 ++--
 tools/libs/guest/Makefile                |   2 +-
 tools/libs/guest/xg_cpuid_x86.c          | 710 +++++++++++++++--------
 tools/libs/guest/xg_sr_common_x86.c      |  17 +-
 tools/libs/light/libxl_cpuid.c           | 230 +++++++-
 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 +-
 tools/tests/cpu-policy/test-cpu-policy.c |  17 +
 13 files changed, 821 insertions(+), 335 deletions(-)

-- 
2.30.1



^ permalink raw reply	[flat|nested] 43+ messages in thread

* [PATCH v2 01/21] libxl: don't ignore the return value from xc_cpuid_apply_policy
  2021-04-13 14:01 [PATCH v2 00/21] libs/guest: new CPUID/MSR interface Roger Pau Monne
@ 2021-04-13 14:01 ` Roger Pau Monne
  2021-04-28 15:13   ` Anthony PERARD
  2021-04-13 14:01 ` [PATCH v2 02/21] libs/guest: rename xc_get_cpu_policy_size to xc_cpu_policy_get_size Roger Pau Monne
                   ` (19 subsequent siblings)
  20 siblings, 1 reply; 43+ messages in thread
From: Roger Pau Monne @ 2021-04-13 14:01 UTC (permalink / raw)
  To: xen-devel
  Cc: Andrew Cooper, Roger Pau Monne, Ian Jackson, Wei Liu,
	Anthony PERARD, Jan Beulich

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>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
Changes since 1:
 - Return ERROR_FAIL on error.
---
 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..539fc4869e6 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 ? ERROR_FAIL : 0;
 }
 
 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 0c64268f66d..2faa96d9c68 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 c6a4a187f5b..44a2f3c8fe3 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



^ permalink raw reply related	[flat|nested] 43+ messages in thread

* [PATCH v2 02/21] libs/guest: rename xc_get_cpu_policy_size to xc_cpu_policy_get_size
  2021-04-13 14:01 [PATCH v2 00/21] libs/guest: new CPUID/MSR interface Roger Pau Monne
  2021-04-13 14:01 ` [PATCH v2 01/21] libxl: don't ignore the return value from xc_cpuid_apply_policy Roger Pau Monne
@ 2021-04-13 14:01 ` Roger Pau Monne
  2021-04-13 14:01 ` [PATCH v2 03/21] libs/guest: introduce xc_cpu_policy_t Roger Pau Monne
                   ` (18 subsequent siblings)
  20 siblings, 0 replies; 43+ messages in thread
From: Roger Pau Monne @ 2021-04-13 14:01 UTC (permalink / raw)
  To: xen-devel
  Cc: Andrew Cooper, Roger Pau Monne, Ian Jackson, Wei Liu, Jan Beulich

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>
---
 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 5ea69ad3d51..cc5cae95725 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");
@@ -448,7 +448,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



^ permalink raw reply related	[flat|nested] 43+ messages in thread

* [PATCH v2 03/21] libs/guest: introduce xc_cpu_policy_t
  2021-04-13 14:01 [PATCH v2 00/21] libs/guest: new CPUID/MSR interface Roger Pau Monne
  2021-04-13 14:01 ` [PATCH v2 01/21] libxl: don't ignore the return value from xc_cpuid_apply_policy Roger Pau Monne
  2021-04-13 14:01 ` [PATCH v2 02/21] libs/guest: rename xc_get_cpu_policy_size to xc_cpu_policy_get_size Roger Pau Monne
@ 2021-04-13 14:01 ` Roger Pau Monne
  2021-04-13 14:01 ` [PATCH v2 04/21] libs/guest: introduce helper to fetch a system cpu policy Roger Pau Monne
                   ` (17 subsequent siblings)
  20 siblings, 0 replies; 43+ messages in thread
From: Roger Pau Monne @ 2021-04-13 14:01 UTC (permalink / raw)
  To: xen-devel; +Cc: Andrew Cooper, Roger Pau Monne, Ian Jackson, Wei Liu

Introduce an opaque type that is used to store the CPUID and MSRs
policies of a domain. Such type uses the existing {cpuid,msr}_policy
structures to store the data, but doesn't expose the type to the users
of the xenguest library. There are also two arrays to allow for easier
serialization without requiring an allocation each time.

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>
---
Changes since v1:
 - Introduce two arrays to store serialized versions.
---
 tools/include/xenctrl.h         |  6 ++++++
 tools/libs/guest/xg_cpuid_x86.c | 18 ++++++++++++++++++
 2 files changed, 24 insertions(+)

diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index e91ff92b9b1..1aba814f01c 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 xc_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 cc5cae95725..8e3a1a8cbf2 100644
--- a/tools/libs/guest/xg_cpuid_x86.c
+++ b/tools/libs/guest/xg_cpuid_x86.c
@@ -39,6 +39,13 @@ enum {
 #define bitmaskof(idx)      (1u << ((idx) & 31))
 #define featureword_of(idx) ((idx) >> 5)
 
+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 entries[MSR_MAX_SERIALISED_ENTRIES];
+};
+
 int xc_get_cpu_levelling_caps(xc_interface *xch, uint32_t *caps)
 {
     DECLARE_SYSCTL;
@@ -660,3 +667,14 @@ out:
 
     return rc;
 }
+
+xc_cpu_policy_t xc_cpu_policy_init(void)
+{
+    return calloc(1, sizeof(struct xc_cpu_policy));
+}
+
+void xc_cpu_policy_destroy(xc_cpu_policy_t policy)
+{
+    if ( policy )
+        free(policy);
+}
-- 
2.30.1



^ permalink raw reply related	[flat|nested] 43+ messages in thread

* [PATCH v2 04/21] libs/guest: introduce helper to fetch a system cpu policy
  2021-04-13 14:01 [PATCH v2 00/21] libs/guest: new CPUID/MSR interface Roger Pau Monne
                   ` (2 preceding siblings ...)
  2021-04-13 14:01 ` [PATCH v2 03/21] libs/guest: introduce xc_cpu_policy_t Roger Pau Monne
@ 2021-04-13 14:01 ` Roger Pau Monne
  2021-04-14 13:28   ` Jan Beulich
  2021-04-13 14:01 ` [PATCH v2 05/21] libs/guest: introduce helper to fetch a domain " Roger Pau Monne
                   ` (16 subsequent siblings)
  20 siblings, 1 reply; 43+ messages in thread
From: Roger Pau Monne @ 2021-04-13 14:01 UTC (permalink / raw)
  To: xen-devel; +Cc: Andrew Cooper, Roger Pau Monne, Ian Jackson, Wei Liu

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>
---
Changes since v1:
 - Always return -1 on error from xc_cpu_policy_get_system.
 - Only print detailed error messages if err_leaf or err_msr is != -1.
 - Rename idx function parameter to policy_idx.
---
 tools/include/xenctrl.h         |  4 +++
 tools/libs/guest/xg_cpuid_x86.c | 54 +++++++++++++++++++++++++++++++++
 2 files changed, 58 insertions(+)

diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index 1aba814f01c..187df5c5d2d 100644
--- a/tools/include/xenctrl.h
+++ b/tools/include/xenctrl.h
@@ -2596,6 +2596,10 @@ typedef struct xc_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 policy_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 8e3a1a8cbf2..78fbc7db9d3 100644
--- a/tools/libs/guest/xg_cpuid_x86.c
+++ b/tools/libs/guest/xg_cpuid_x86.c
@@ -678,3 +678,57 @@ void xc_cpu_policy_destroy(xc_cpu_policy_t policy)
     if ( policy )
         free(policy);
 }
+
+static int deserialize_policy(xc_interface *xch, xc_cpu_policy_t policy,
+                              unsigned int nr_leaves, unsigned int nr_entries)
+{
+    uint32_t err_leaf = -1, err_subleaf = -1, err_msr = -1;
+    int rc;
+
+    rc = x86_cpuid_copy_from_buffer(&policy->cpuid, policy->leaves,
+                                    nr_leaves, &err_leaf, &err_subleaf);
+    if ( rc )
+    {
+        if ( err_leaf != -1 )
+            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, policy->entries,
+                                  nr_entries, &err_msr);
+    if ( rc )
+    {
+        if ( err_msr != -1 )
+            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 policy_idx,
+                             xc_cpu_policy_t policy)
+{
+    unsigned int nr_leaves = ARRAY_SIZE(policy->leaves);
+    unsigned int nr_entries = ARRAY_SIZE(policy->entries);
+    int rc;
+
+    rc = xc_get_system_cpu_policy(xch, policy_idx, &nr_leaves, policy->leaves,
+                                  &nr_entries, policy->entries);
+    if ( rc )
+    {
+        PERROR("Failed to obtain %u policy", policy_idx);
+        return rc;
+    }
+
+    rc = deserialize_policy(xch, policy, nr_leaves, nr_entries);
+    if ( rc )
+    {
+        errno = -rc;
+        rc = -1;
+    }
+
+    return rc;
+}
-- 
2.30.1



^ permalink raw reply related	[flat|nested] 43+ messages in thread

* [PATCH v2 05/21] libs/guest: introduce helper to fetch a domain cpu policy
  2021-04-13 14:01 [PATCH v2 00/21] libs/guest: new CPUID/MSR interface Roger Pau Monne
                   ` (3 preceding siblings ...)
  2021-04-13 14:01 ` [PATCH v2 04/21] libs/guest: introduce helper to fetch a system cpu policy Roger Pau Monne
@ 2021-04-13 14:01 ` Roger Pau Monne
  2021-04-13 14:01 ` [PATCH v2 06/21] libs/guest: introduce helper to serialize a " Roger Pau Monne
                   ` (15 subsequent siblings)
  20 siblings, 0 replies; 43+ messages in thread
From: Roger Pau Monne @ 2021-04-13 14:01 UTC (permalink / raw)
  To: xen-devel
  Cc: Andrew Cooper, Roger Pau Monne, Ian Jackson, Wei Liu, Jan Beulich

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>
---
Changes since v1:
 - Uniformly return -1 on error from xc_cpu_policy_get_domain.
---
 tools/include/xenctrl.h         |  2 ++
 tools/libs/guest/xg_cpuid_x86.c | 25 +++++++++++++++++++++++++
 2 files changed, 27 insertions(+)

diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index 187df5c5d2d..34d979d11da 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 policy_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 78fbc7db9d3..1394e503f3d 100644
--- a/tools/libs/guest/xg_cpuid_x86.c
+++ b/tools/libs/guest/xg_cpuid_x86.c
@@ -732,3 +732,28 @@ int xc_cpu_policy_get_system(xc_interface *xch, unsigned int policy_idx,
 
     return rc;
 }
+
+int xc_cpu_policy_get_domain(xc_interface *xch, uint32_t domid,
+                             xc_cpu_policy_t policy)
+{
+    unsigned int nr_leaves = ARRAY_SIZE(policy->leaves);
+    unsigned int nr_entries = ARRAY_SIZE(policy->entries);
+    int rc;
+
+    rc = xc_get_domain_cpu_policy(xch, domid, &nr_leaves, policy->leaves,
+                                  &nr_entries, policy->entries);
+    if ( rc )
+    {
+        PERROR("Failed to obtain domain %u policy", domid);
+        return rc;
+    }
+
+    rc = deserialize_policy(xch, policy, nr_leaves, nr_entries);
+    if ( rc )
+    {
+        errno = -rc;
+        rc = -1;
+    }
+
+    return rc;
+}
-- 
2.30.1



^ permalink raw reply related	[flat|nested] 43+ messages in thread

* [PATCH v2 06/21] libs/guest: introduce helper to serialize a cpu policy
  2021-04-13 14:01 [PATCH v2 00/21] libs/guest: new CPUID/MSR interface Roger Pau Monne
                   ` (4 preceding siblings ...)
  2021-04-13 14:01 ` [PATCH v2 05/21] libs/guest: introduce helper to fetch a domain " Roger Pau Monne
@ 2021-04-13 14:01 ` Roger Pau Monne
  2021-04-13 14:01 ` [PATCH v2 07/21] tools: switch existing users of xc_get_{system,domain}_cpu_policy Roger Pau Monne
                   ` (14 subsequent siblings)
  20 siblings, 0 replies; 43+ messages in thread
From: Roger Pau Monne @ 2021-04-13 14:01 UTC (permalink / raw)
  To: xen-devel; +Cc: Andrew Cooper, Roger Pau Monne, Ian Jackson, Wei Liu

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 34d979d11da..a4827b1ae6a 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 policy_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 1394e503f3d..918591a128c 100644
--- a/tools/libs/guest/xg_cpuid_x86.c
+++ b/tools/libs/guest/xg_cpuid_x86.c
@@ -757,3 +757,35 @@ int xc_cpu_policy_get_domain(xc_interface *xch, uint32_t domid,
 
     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



^ permalink raw reply related	[flat|nested] 43+ messages in thread

* [PATCH v2 07/21] tools: switch existing users of xc_get_{system,domain}_cpu_policy
  2021-04-13 14:01 [PATCH v2 00/21] libs/guest: new CPUID/MSR interface Roger Pau Monne
                   ` (5 preceding siblings ...)
  2021-04-13 14:01 ` [PATCH v2 06/21] libs/guest: introduce helper to serialize a " Roger Pau Monne
@ 2021-04-13 14:01 ` Roger Pau Monne
  2021-04-13 14:01 ` [PATCH v2 08/21] libs/guest: introduce a helper to apply a cpu policy to a domain Roger Pau Monne
                   ` (13 subsequent siblings)
  20 siblings, 0 replies; 43+ messages in thread
From: Roger Pau Monne @ 2021-04-13 14:01 UTC (permalink / raw)
  To: xen-devel
  Cc: Andrew Cooper, Roger Pau Monne, Ian Jackson, Wei Liu, Jan Beulich

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     | 39 ++++++++++++++---------------
 tools/libs/guest/xg_sr_common_x86.c | 15 ++++++++---
 tools/misc/xen-cpuid.c              | 21 ++++++++++------
 4 files changed, 45 insertions(+), 36 deletions(-)

diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index a4827b1ae6a..e9a86d63bad 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 918591a128c..208a247bb6e 100644
--- a/tools/libs/guest/xg_cpuid_x86.c
+++ b/tools/libs/guest/xg_cpuid_x86.c
@@ -135,9 +135,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,
@@ -173,9 +173,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,
@@ -329,7 +329,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);
@@ -340,10 +340,9 @@ static int xc_cpuid_xend_policy(
     /* Get the domain type's default policy. */
     nr_msrs = 0;
     nr_def = nr_leaves;
-    rc = xc_get_system_cpu_policy(xch,
-                                  di.hvm ? XEN_SYSCTL_cpu_policy_hvm_default
-                                         : XEN_SYSCTL_cpu_policy_pv_default,
-                                  &nr_def, def, &nr_msrs, NULL);
+    rc = get_system_cpu_policy(xch, di.hvm ? XEN_SYSCTL_cpu_policy_hvm_default
+                                           : XEN_SYSCTL_cpu_policy_pv_default,
+                               &nr_def, def, &nr_msrs, NULL);
     if ( rc )
     {
         PERROR("Failed to obtain %s def policy", di.hvm ? "hvm" : "pv");
@@ -354,8 +353,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");
@@ -486,9 +485,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");
@@ -715,8 +714,8 @@ int xc_cpu_policy_get_system(xc_interface *xch, unsigned int policy_idx,
     unsigned int nr_entries = ARRAY_SIZE(policy->entries);
     int rc;
 
-    rc = xc_get_system_cpu_policy(xch, policy_idx, &nr_leaves, policy->leaves,
-                                  &nr_entries, policy->entries);
+    rc = get_system_cpu_policy(xch, policy_idx, &nr_leaves, policy->leaves,
+                               &nr_entries, policy->entries);
     if ( rc )
     {
         PERROR("Failed to obtain %u policy", policy_idx);
@@ -740,8 +739,8 @@ int xc_cpu_policy_get_domain(xc_interface *xch, uint32_t domid,
     unsigned int nr_entries = ARRAY_SIZE(policy->entries);
     int rc;
 
-    rc = xc_get_domain_cpu_policy(xch, domid, &nr_leaves, policy->leaves,
-                                  &nr_entries, policy->entries);
+    rc = get_domain_cpu_policy(xch, domid, &nr_leaves, policy->leaves,
+                               &nr_entries, policy->entries);
     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



^ permalink raw reply related	[flat|nested] 43+ messages in thread

* [PATCH v2 08/21] libs/guest: introduce a helper to apply a cpu policy to a domain
  2021-04-13 14:01 [PATCH v2 00/21] libs/guest: new CPUID/MSR interface Roger Pau Monne
                   ` (6 preceding siblings ...)
  2021-04-13 14:01 ` [PATCH v2 07/21] tools: switch existing users of xc_get_{system,domain}_cpu_policy Roger Pau Monne
@ 2021-04-13 14:01 ` Roger Pau Monne
  2021-04-13 14:01 ` [PATCH v2 09/21] libs/guest: allow fetching a specific CPUID leaf from a cpu policy Roger Pau Monne
                   ` (12 subsequent siblings)
  20 siblings, 0 replies; 43+ messages in thread
From: Roger Pau Monne @ 2021-04-13 14:01 UTC (permalink / raw)
  To: xen-devel; +Cc: Andrew Cooper, Roger Pau Monne, Ian Jackson, Wei Liu

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 | 29 +++++++++++++++++++++++++++++
 2 files changed, 31 insertions(+)

diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index e9a86d63bad..27cec1b93ff 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 policy_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 208a247bb6e..8b48c51a8ee 100644
--- a/tools/libs/guest/xg_cpuid_x86.c
+++ b/tools/libs/guest/xg_cpuid_x86.c
@@ -757,6 +757,35 @@ 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 = ARRAY_SIZE(policy->leaves);
+    unsigned int nr_entries = ARRAY_SIZE(policy->entries);
+    int rc;
+
+    rc = xc_cpu_policy_serialise(xch, policy, policy->leaves, &nr_leaves,
+                                 policy->entries, &nr_entries);
+    if ( rc )
+        return rc;
+
+    rc = xc_set_domain_cpu_policy(xch, domid, nr_leaves, policy->leaves,
+                                  nr_entries, policy->entries,
+                                  &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);
+    }
+
+    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



^ permalink raw reply related	[flat|nested] 43+ messages in thread

* [PATCH v2 09/21] libs/guest: allow fetching a specific CPUID leaf from a cpu policy
  2021-04-13 14:01 [PATCH v2 00/21] libs/guest: new CPUID/MSR interface Roger Pau Monne
                   ` (7 preceding siblings ...)
  2021-04-13 14:01 ` [PATCH v2 08/21] libs/guest: introduce a helper to apply a cpu policy to a domain Roger Pau Monne
@ 2021-04-13 14:01 ` Roger Pau Monne
  2021-04-13 14:01 ` [PATCH v2 10/21] tests/cpu-policy: add sorted MSR test Roger Pau Monne
                   ` (11 subsequent siblings)
  20 siblings, 0 replies; 43+ messages in thread
From: Roger Pau Monne @ 2021-04-13 14:01 UTC (permalink / raw)
  To: xen-devel; +Cc: Andrew Cooper, Roger Pau Monne, Ian Jackson, Wei Liu

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.

Note that callers of find_leaf need to be slightly adjusted to use the
new helper parameters.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v1:
 - Use find leaf.
---
 tools/include/xenctrl.h         |  3 +++
 tools/libs/guest/xg_cpuid_x86.c | 38 ++++++++++++++++++++++++++++-----
 2 files changed, 36 insertions(+), 5 deletions(-)

diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index 27cec1b93ff..cbca7209e34 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 8b48c51a8ee..d146c5bbc99 100644
--- a/tools/libs/guest/xg_cpuid_x86.c
+++ b/tools/libs/guest/xg_cpuid_x86.c
@@ -280,9 +280,9 @@ static int compare_leaves(const void *l, const void *r)
 
 static xen_cpuid_leaf_t *find_leaf(
     xen_cpuid_leaf_t *leaves, unsigned int nr_leaves,
-    const struct xc_xend_cpuid *xend)
+    unsigned int leaf, unsigned int subleaf)
 {
-    const xen_cpuid_leaf_t key = { xend->leaf, xend->subleaf };
+    const xen_cpuid_leaf_t key = { leaf, subleaf };
 
     return bsearch(&key, leaves, nr_leaves, sizeof(*leaves), compare_leaves);
 }
@@ -365,9 +365,12 @@ static int xc_cpuid_xend_policy(
     rc = -EINVAL;
     for ( ; xend->leaf != XEN_CPUID_INPUT_UNUSED; ++xend )
     {
-        xen_cpuid_leaf_t *cur_leaf = find_leaf(cur, nr_cur, xend);
-        const xen_cpuid_leaf_t *def_leaf = find_leaf(def, nr_def, xend);
-        const xen_cpuid_leaf_t *host_leaf = find_leaf(host, nr_host, xend);
+        xen_cpuid_leaf_t *cur_leaf = find_leaf(cur, nr_cur,
+                                               xend->leaf, xend->subleaf);
+        const xen_cpuid_leaf_t *def_leaf = find_leaf(def, nr_def,
+                                                     xend->leaf, xend->subleaf);
+        const xen_cpuid_leaf_t *host_leaf = find_leaf(host, nr_host, xend->leaf,
+                                                      xend->subleaf);
 
         if ( cur_leaf == NULL || def_leaf == NULL || host_leaf == NULL )
         {
@@ -817,3 +820,28 @@ 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 = ARRAY_SIZE(policy->leaves);
+    xen_cpuid_leaf_t *tmp;
+    int rc;
+
+    rc = xc_cpu_policy_serialise(xch, policy, policy->leaves, &nr_leaves,
+                                 NULL, 0);
+    if ( rc )
+        return rc;
+
+    tmp = find_leaf(policy->leaves, nr_leaves, leaf, subleaf);
+    if ( !tmp )
+    {
+        /* Unable to find a matching leaf. */
+        errno = ENOENT;
+        return -1;
+    }
+
+    *out = *tmp;
+    return 0;
+}
-- 
2.30.1



^ permalink raw reply related	[flat|nested] 43+ messages in thread

* [PATCH v2 10/21] tests/cpu-policy: add sorted MSR test
  2021-04-13 14:01 [PATCH v2 00/21] libs/guest: new CPUID/MSR interface Roger Pau Monne
                   ` (8 preceding siblings ...)
  2021-04-13 14:01 ` [PATCH v2 09/21] libs/guest: allow fetching a specific CPUID leaf from a cpu policy Roger Pau Monne
@ 2021-04-13 14:01 ` Roger Pau Monne
  2021-04-13 14:01 ` [PATCH v2 11/21] libs/guest: allow fetching a specific MSR entry from a cpu policy Roger Pau Monne
                   ` (10 subsequent siblings)
  20 siblings, 0 replies; 43+ messages in thread
From: Roger Pau Monne @ 2021-04-13 14:01 UTC (permalink / raw)
  To: xen-devel
  Cc: Andrew Cooper, Roger Pau Monne, Jan Beulich, Wei Liu, Ian Jackson

Further changes will rely on MSR entries being sorted, so add a test
to assert it.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v1:
 - New in this version.
---
 tools/tests/cpu-policy/test-cpu-policy.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/tools/tests/cpu-policy/test-cpu-policy.c b/tools/tests/cpu-policy/test-cpu-policy.c
index 0fa209f1ea7..ed450a09970 100644
--- a/tools/tests/cpu-policy/test-cpu-policy.c
+++ b/tools/tests/cpu-policy/test-cpu-policy.c
@@ -86,6 +86,16 @@ static bool leaves_are_sorted(const xen_cpuid_leaf_t *leaves, unsigned int nr)
     return true;
 }
 
+static bool msrs_are_sorted(const xen_msr_entry_t *entries, unsigned int nr)
+{
+    for ( unsigned int i = 1; i < nr; ++i )
+        /* MSR index went backwards => not sorted. */
+        if ( entries[i - 1].idx > entries[i].idx )
+            return false;
+
+    return true;
+}
+
 static void test_cpuid_current(void)
 {
     struct cpuid_policy p;
@@ -270,6 +280,13 @@ static void test_msr_serialise_success(void)
             goto test_done;
         }
 
+        if ( !msrs_are_sorted(msrs, nr) )
+        {
+            fail("  Test %s, MSR entries not sorted\n",
+                 t->name);
+            goto test_done;
+        }
+
     test_done:
         free(msrs);
     }
-- 
2.30.1



^ permalink raw reply related	[flat|nested] 43+ messages in thread

* [PATCH v2 11/21] libs/guest: allow fetching a specific MSR entry from a cpu policy
  2021-04-13 14:01 [PATCH v2 00/21] libs/guest: new CPUID/MSR interface Roger Pau Monne
                   ` (9 preceding siblings ...)
  2021-04-13 14:01 ` [PATCH v2 10/21] tests/cpu-policy: add sorted MSR test Roger Pau Monne
@ 2021-04-13 14:01 ` Roger Pau Monne
  2021-04-13 14:01 ` [PATCH v2 12/21] libs/guest: allow updating a cpu policy CPUID data Roger Pau Monne
                   ` (9 subsequent siblings)
  20 siblings, 0 replies; 43+ messages in thread
From: Roger Pau Monne @ 2021-04-13 14:01 UTC (permalink / raw)
  To: xen-devel; +Cc: Andrew Cooper, Roger Pau Monne, Ian Jackson, Wei Liu

Introduce an interface that returns a specific MSR entry from a cpu
policy in xen_msr_entry_t format. Provide a helper to perform a binary
search against an array of MSR entries.

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>
---
Changes since v1:
 - Introduce a helper to perform a binary search of the MSR entries
   array.
---
 tools/include/xenctrl.h         |  2 ++
 tools/libs/guest/xg_cpuid_x86.c | 42 +++++++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+)

diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index cbca7209e34..605c632cf30 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 d146c5bbc99..a4307d50ddb 100644
--- a/tools/libs/guest/xg_cpuid_x86.c
+++ b/tools/libs/guest/xg_cpuid_x86.c
@@ -845,3 +845,45 @@ int xc_cpu_policy_get_cpuid(xc_interface *xch, const xc_cpu_policy_t policy,
     *out = *tmp;
     return 0;
 }
+
+static int compare_entries(const void *l, const void *r)
+{
+    const xen_msr_entry_t *lhs = l;
+    const xen_msr_entry_t *rhs = r;
+
+    if ( lhs->idx == rhs->idx )
+        return 0;
+    return lhs->idx < rhs->idx ? -1 : 1;
+}
+
+static xen_msr_entry_t *find_entry(xen_msr_entry_t *entries,
+                                   unsigned int nr_entries, unsigned int index)
+{
+    const xen_msr_entry_t key = { index };
+
+    return bsearch(&key, entries, nr_entries, sizeof(*entries), compare_entries);
+}
+
+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_entries = ARRAY_SIZE(policy->entries);
+    xen_msr_entry_t *tmp;
+    int rc;
+
+    rc = xc_cpu_policy_serialise(xch, policy, NULL, 0,
+                                 policy->entries, &nr_entries);
+    if ( rc )
+        return rc;
+
+    tmp = find_entry(policy->entries, nr_entries, msr);
+    if ( !tmp )
+    {
+        /* Unable to find a matching MSR. */
+        errno = ENOENT;
+        return -1;
+    }
+
+    *out = *tmp;
+    return 0;
+}
-- 
2.30.1



^ permalink raw reply related	[flat|nested] 43+ messages in thread

* [PATCH v2 12/21] libs/guest: allow updating a cpu policy CPUID data
  2021-04-13 14:01 [PATCH v2 00/21] libs/guest: new CPUID/MSR interface Roger Pau Monne
                   ` (10 preceding siblings ...)
  2021-04-13 14:01 ` [PATCH v2 11/21] libs/guest: allow fetching a specific MSR entry from a cpu policy Roger Pau Monne
@ 2021-04-13 14:01 ` Roger Pau Monne
  2021-04-13 14:01 ` [PATCH v2 13/21] libs/guest: allow updating a cpu policy MSR data Roger Pau Monne
                   ` (8 subsequent siblings)
  20 siblings, 0 replies; 43+ messages in thread
From: Roger Pau Monne @ 2021-04-13 14:01 UTC (permalink / raw)
  To: xen-devel; +Cc: Andrew Cooper, Roger Pau Monne, Ian Jackson, Wei Liu

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>
---
Changes since v1:
 - Don't use memcpy.
 - Drop logic to update the leaf manually - x86_cpuid_copy_from_buffer
   already does it.
 - Only print a failure message if err_leaf != -1.
---
 tools/include/xenctrl.h         |  3 +++
 tools/libs/guest/xg_cpuid_x86.c | 20 ++++++++++++++++++++
 2 files changed, 23 insertions(+)

diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index 605c632cf30..49f919f16a7 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 a4307d50ddb..00595e14cc3 100644
--- a/tools/libs/guest/xg_cpuid_x86.c
+++ b/tools/libs/guest/xg_cpuid_x86.c
@@ -887,3 +887,23 @@ int xc_cpu_policy_get_msr(xc_interface *xch, const xc_cpu_policy_t policy,
     *out = *tmp;
     return 0;
 }
+
+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;
+    int rc = x86_cpuid_copy_from_buffer(&policy->cpuid, leaves, nr,
+                                        &err_leaf, &err_subleaf);
+
+    if ( rc )
+    {
+        if ( err_leaf != -1 )
+            ERROR("Failed to update CPUID (err leaf %#x, subleaf %#x) (%d = %s)",
+                  err_leaf, err_subleaf, -rc, strerror(-rc));
+        errno = -rc;
+        rc = -1;
+    }
+
+    return rc;
+}
-- 
2.30.1



^ permalink raw reply related	[flat|nested] 43+ messages in thread

* [PATCH v2 13/21] libs/guest: allow updating a cpu policy MSR data
  2021-04-13 14:01 [PATCH v2 00/21] libs/guest: new CPUID/MSR interface Roger Pau Monne
                   ` (11 preceding siblings ...)
  2021-04-13 14:01 ` [PATCH v2 12/21] libs/guest: allow updating a cpu policy CPUID data Roger Pau Monne
@ 2021-04-13 14:01 ` Roger Pau Monne
  2021-04-13 14:01 ` [PATCH v2 14/21] libs/guest: introduce helper to check cpu policy compatibility Roger Pau Monne
                   ` (7 subsequent siblings)
  20 siblings, 0 replies; 43+ messages in thread
From: Roger Pau Monne @ 2021-04-13 14:01 UTC (permalink / raw)
  To: xen-devel; +Cc: Andrew Cooper, Roger Pau Monne, Ian Jackson, Wei Liu

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>
---
Changes since v1:
 - Drop logic to update entries manually.
 - Only print failure message if err_msr != -1.
---
 tools/include/xenctrl.h         |  2 ++
 tools/libs/guest/xg_cpuid_x86.c | 18 ++++++++++++++++++
 2 files changed, 20 insertions(+)

diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index 49f919f16a7..9a6d1b126d8 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 00595e14cc3..26b09322dfa 100644
--- a/tools/libs/guest/xg_cpuid_x86.c
+++ b/tools/libs/guest/xg_cpuid_x86.c
@@ -907,3 +907,21 @@ int xc_cpu_policy_update_cpuid(xc_interface *xch, xc_cpu_policy_t policy,
 
     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;
+    int rc = x86_msr_copy_from_buffer(&policy->msr, msrs, nr, &err_msr);
+
+    if ( rc )
+    {
+        if ( err_msr != -1 )
+            ERROR("Failed to deserialise MSRS (err index %#x) (%d = %s)",
+                  err_msr, -rc, strerror(-rc));
+        errno = -rc;
+        rc = -1;
+    }
+
+    return rc;
+}
-- 
2.30.1



^ permalink raw reply related	[flat|nested] 43+ messages in thread

* [PATCH v2 14/21] libs/guest: introduce helper to check cpu policy compatibility
  2021-04-13 14:01 [PATCH v2 00/21] libs/guest: new CPUID/MSR interface Roger Pau Monne
                   ` (12 preceding siblings ...)
  2021-04-13 14:01 ` [PATCH v2 13/21] libs/guest: allow updating a cpu policy MSR data Roger Pau Monne
@ 2021-04-13 14:01 ` Roger Pau Monne
  2021-04-14 13:36   ` Jan Beulich
  2021-04-13 14:01 ` [PATCH v2 15/21] libs/guest: obtain a compatible cpu policy from two input ones Roger Pau Monne
                   ` (6 subsequent siblings)
  20 siblings, 1 reply; 43+ messages in thread
From: Roger Pau Monne @ 2021-04-13 14:01 UTC (permalink / raw)
  To: xen-devel; +Cc: Andrew Cooper, Roger Pau Monne, Ian Jackson, Wei Liu

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>
---
Changes since v1:
 - Initialize err.
 - Explicitly name parameters as host and guest.
---
 tools/include/xenctrl.h         |  4 ++++
 tools/libs/guest/Makefile       |  2 +-
 tools/libs/guest/xg_cpuid_x86.c | 19 +++++++++++++++++++
 3 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index 9a6d1b126d8..5f699c09509 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 host,
+                                 const xc_cpu_policy_t guest);
+
 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 26b09322dfa..bd2f31dd87f 100644
--- a/tools/libs/guest/xg_cpuid_x86.c
+++ b/tools/libs/guest/xg_cpuid_x86.c
@@ -925,3 +925,22 @@ 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 host,
+                                 const xc_cpu_policy_t guest)
+{
+    struct cpu_policy_errors err = INIT_CPU_POLICY_ERRORS;
+    struct cpu_policy h = { &host->cpuid, &host->msr };
+    struct cpu_policy g = { &guest->cpuid, &guest->msr };
+    int rc = x86_cpu_policies_are_compatible(&h, &g, &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



^ permalink raw reply related	[flat|nested] 43+ messages in thread

* [PATCH v2 15/21] libs/guest: obtain a compatible cpu policy from two input ones
  2021-04-13 14:01 [PATCH v2 00/21] libs/guest: new CPUID/MSR interface Roger Pau Monne
                   ` (13 preceding siblings ...)
  2021-04-13 14:01 ` [PATCH v2 14/21] libs/guest: introduce helper to check cpu policy compatibility Roger Pau Monne
@ 2021-04-13 14:01 ` Roger Pau Monne
  2021-04-14 13:49   ` Jan Beulich
  2021-04-21 10:22   ` Wei Liu
  2021-04-13 14:01 ` [PATCH v2 16/21] libs/guest: make a cpu policy compatible with older Xen versions Roger Pau Monne
                   ` (5 subsequent siblings)
  20 siblings, 2 replies; 43+ messages in thread
From: Roger Pau Monne @ 2021-04-13 14:01 UTC (permalink / raw)
  To: xen-devel
  Cc: Andrew Cooper, Roger Pau Monne, Ian Jackson, Wei Liu, Anthony PERARD

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>
---
Changes since v1:
 - Only AND the feature parts of cpuid.
 - Use a binary search to find the matching leaves and msr entries.
 - Remove default case from MSR level function.
---
 tools/include/xen-tools/libs.h    |   5 ++
 tools/include/xenctrl.h           |   4 +
 tools/libs/guest/xg_cpuid_x86.c   | 128 ++++++++++++++++++++++++++++++
 tools/libs/light/libxl_internal.h |   2 -
 4 files changed, 137 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 5f699c09509..c41d794683c 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 host,
                                  const xc_cpu_policy_t guest);
+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 bd2f31dd87f..6cfa4cb39d1 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>
@@ -944,3 +945,130 @@ bool xc_cpu_policy_is_compatible(xc_interface *xch, const xc_cpu_policy_t host,
 
     return false;
 }
+
+static uint64_t level_msr(unsigned int index, uint64_t val1, uint64_t val2)
+{
+    uint64_t val = val1 & val2;;
+
+    switch ( index )
+    {
+    case MSR_ARCH_CAPABILITIES:
+        /*
+         * 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;
+    }
+
+    return val;
+}
+
+static bool level_leaf(xen_cpuid_leaf_t *l1, xen_cpuid_leaf_t *l2,
+                       xen_cpuid_leaf_t *out)
+{
+    *out = (xen_cpuid_leaf_t){ };
+
+    switch ( l1->leaf )
+    {
+    case 0x1:
+    case 0x80000001:
+        out->c = l1->c & l2->c;
+        out->d = l1->d & l2->d;
+        return true;
+
+    case 0xd:
+        if ( l1->subleaf != 1 )
+            break;
+        out->a = l1->a & l2->a;
+        return true;
+
+    case 0x7:
+        switch ( l1->subleaf )
+        {
+        case 0:
+            out->b = l1->b & l2->b;
+            out->c = l1->c & l2->c;
+            out->d = l1->d & l2->d;
+            return true;
+
+        case 1:
+            out->a = l1->a & l2->a;
+            return true;
+        }
+        break;
+
+    case 0x80000007:
+        out->d = l1->d & l2->d;
+        return true;
+
+    case 0x80000008:
+        out->b = l1->b & l2->b;
+        return true;
+    }
+
+    return false;
+}
+
+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)
+{
+    unsigned int nr_leaves, nr_msrs, i, index;
+    unsigned int p1_nr_leaves, p2_nr_leaves;
+    unsigned int p1_nr_entries, p2_nr_entries;
+    int rc;
+
+    p1_nr_leaves = p2_nr_leaves = ARRAY_SIZE(p1->leaves);
+    p1_nr_entries = p2_nr_entries = ARRAY_SIZE(p1->entries);
+
+    rc = xc_cpu_policy_serialise(xch, p1, p1->leaves, &p1_nr_leaves,
+                                 p1->entries, &p1_nr_entries);
+    if ( rc )
+        return rc;
+    rc = xc_cpu_policy_serialise(xch, p2, p2->leaves, &p2_nr_leaves,
+                                 p2->entries, &p2_nr_entries);
+    if ( rc )
+        return rc;
+
+    index = 0;
+    for ( i = 0; i < p1_nr_leaves; i++ )
+    {
+        xen_cpuid_leaf_t *l1 = &p1->leaves[i];
+        xen_cpuid_leaf_t *l2 = find_leaf(p2->leaves, p2_nr_leaves,
+                                         l1->leaf, l1->subleaf);
+
+        if ( l2 && level_leaf(&out->leaves[index], l1, l2) )
+        {
+            out->leaves[index].leaf = l1->leaf;
+            out->leaves[index].subleaf = l1->subleaf;
+            index++;
+        }
+    }
+    nr_leaves = index;
+
+    index = 0;
+    for ( i = 0; i < p1_nr_entries; i++ )
+    {
+        xen_msr_entry_t *l1 = &p1->entries[i];
+        xen_msr_entry_t *l2 = find_entry(p2->entries, p2_nr_entries, l1->idx);
+
+        if ( !l2 )
+            continue;
+
+        out->entries[index].idx = l1->idx;
+        out->entries[index].val = level_msr(l1->idx, l1->val, l2->val);
+        index++;
+    }
+    nr_msrs = index;
+
+    rc = deserialize_policy(xch, out, nr_leaves, nr_msrs);
+    if ( rc )
+    {
+        errno = -rc;
+        rc = -1;
+    }
+
+    return rc;
+}
diff --git a/tools/libs/light/libxl_internal.h b/tools/libs/light/libxl_internal.h
index 44a2f3c8fe3..5709bcb93fa 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



^ permalink raw reply related	[flat|nested] 43+ messages in thread

* [PATCH v2 16/21] libs/guest: make a cpu policy compatible with older Xen versions
  2021-04-13 14:01 [PATCH v2 00/21] libs/guest: new CPUID/MSR interface Roger Pau Monne
                   ` (14 preceding siblings ...)
  2021-04-13 14:01 ` [PATCH v2 15/21] libs/guest: obtain a compatible cpu policy from two input ones Roger Pau Monne
@ 2021-04-13 14:01 ` Roger Pau Monne
  2021-04-13 14:01 ` [PATCH v2 17/21] libs/guest: introduce helper set cpu topology in cpu policy Roger Pau Monne
                   ` (4 subsequent siblings)
  20 siblings, 0 replies; 43+ messages in thread
From: Roger Pau Monne @ 2021-04-13 14:01 UTC (permalink / raw)
  To: xen-devel; +Cc: Andrew Cooper, Roger Pau Monne, Ian Jackson, Wei Liu

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>
---
Changes since v1:
 - Move comments and explicitly mention pre-4.14 Xen.
---
 tools/include/xenctrl.h         |  4 +++
 tools/libs/guest/xg_cpuid_x86.c | 46 ++++++++++++++++++++++++++++-----
 2 files changed, 43 insertions(+), 7 deletions(-)

diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index c41d794683c..89a73fd6823 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 pre-4.14 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 6cfa4cb39d1..6486ac4c673 100644
--- a/tools/libs/guest/xg_cpuid_x86.c
+++ b/tools/libs/guest/xg_cpuid_x86.c
@@ -446,6 +446,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 xc_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);
@@ -509,17 +510,14 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
     }
 
     /*
-     * Account for feature which have been disabled by default since Xen 4.13,
+     * Account for features which have been disabled by default since Xen 4.13,
      * so migrated-in VM's don't risk seeing features disappearing.
      */
     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);
+        *p = policy.cpuid;
     }
 
     if ( featureset )
@@ -1072,3 +1070,37 @@ 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;
+    }
+
+    /*
+     * Account for features which have been disabled by default since Xen 4.13,
+     * so migrated-in VM's don't risk seeing features disappearing.
+     */
+    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



^ permalink raw reply related	[flat|nested] 43+ messages in thread

* [PATCH v2 17/21] libs/guest: introduce helper set cpu topology in cpu policy
  2021-04-13 14:01 [PATCH v2 00/21] libs/guest: new CPUID/MSR interface Roger Pau Monne
                   ` (15 preceding siblings ...)
  2021-04-13 14:01 ` [PATCH v2 16/21] libs/guest: make a cpu policy compatible with older Xen versions Roger Pau Monne
@ 2021-04-13 14:01 ` Roger Pau Monne
  2021-04-13 14:01 ` [PATCH v2 18/21] libs/guest: rework xc_cpuid_xend_policy Roger Pau Monne
                   ` (3 subsequent siblings)
  20 siblings, 0 replies; 43+ messages in thread
From: Roger Pau Monne @ 2021-04-13 14:01 UTC (permalink / raw)
  To: xen-devel; +Cc: Andrew Cooper, Roger Pau Monne, Ian Jackson, Wei Liu

This logic is pulled out from xc_cpuid_apply_policy and placed into a
separate helper. Note the legacy part of the introduced function, as
long term Xen will require a proper topology setter function capable
of expressing a more diverse set of topologies.

No functional change intended.

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

diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index 89a73fd6823..ec184bccd3f 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 legacy policy topology. */
+int xc_cpu_policy_legacy_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 6486ac4c673..83cd71148f7 100644
--- a/tools/libs/guest/xg_cpuid_x86.c
+++ b/tools/libs/guest/xg_cpuid_x86.c
@@ -443,13 +443,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 xc_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 )
@@ -472,22 +470,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
@@ -575,70 +557,11 @@ 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_legacy_topology(xch, &policy, di.hvm);
+    if ( rc )
+        goto out;
+    *p = policy.cpuid;
 
     rc = x86_cpuid_copy_to_buffer(p, leaves, &nr_leaves);
     if ( rc )
@@ -1104,3 +1027,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_legacy_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



^ permalink raw reply related	[flat|nested] 43+ messages in thread

* [PATCH v2 18/21] libs/guest: rework xc_cpuid_xend_policy
  2021-04-13 14:01 [PATCH v2 00/21] libs/guest: new CPUID/MSR interface Roger Pau Monne
                   ` (16 preceding siblings ...)
  2021-04-13 14:01 ` [PATCH v2 17/21] libs/guest: introduce helper set cpu topology in cpu policy Roger Pau Monne
@ 2021-04-13 14:01 ` Roger Pau Monne
  2021-04-13 14:01 ` [PATCH v2 19/21] libs/guest: apply a featureset into a cpu policy Roger Pau Monne
                   ` (2 subsequent siblings)
  20 siblings, 0 replies; 43+ messages in thread
From: Roger Pau Monne @ 2021-04-13 14:01 UTC (permalink / raw)
  To: xen-devel; +Cc: Andrew Cooper, Roger Pau Monne, Ian Jackson, Wei Liu

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, as 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 | 182 ++++++++++++++------------------
 2 files changed, 84 insertions(+), 102 deletions(-)

diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index ec184bccd3f..ce0785f7654 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_legacy_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 83cd71148f7..06e3e8131d7 100644
--- a/tools/libs/guest/xg_cpuid_x86.c
+++ b/tools/libs/guest/xg_cpuid_x86.c
@@ -288,124 +288,107 @@ static xen_cpuid_leaf_t *find_leaf(
     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, default for the domain type,
-     * and domain current.
-     */
-    xen_cpuid_leaf_t *host = NULL, *def = NULL, *cur = NULL;
-    unsigned int nr_host, nr_def, nr_cur;
-
-    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;
-    }
+    xc_cpu_policy_t host = NULL, def = NULL;
 
-    rc = -ENOMEM;
-    if ( (host = calloc(nr_leaves, sizeof(*host))) == NULL ||
-         (def  = calloc(nr_leaves, sizeof(*def)))  == 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();
+    def = xc_cpu_policy_init();
+    if ( !host || !def )
     {
-        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 type's default policy. */
-    nr_msrs = 0;
-    nr_def = nr_leaves;
-    rc = get_system_cpu_policy(xch, di.hvm ? XEN_SYSCTL_cpu_policy_hvm_default
+    rc = xc_cpu_policy_get_system(xch, hvm ? XEN_SYSCTL_cpu_policy_hvm_default
                                            : XEN_SYSCTL_cpu_policy_pv_default,
-                               &nr_def, def, &nr_msrs, NULL);
+                                  def);
     if ( rc )
     {
-        PERROR("Failed to obtain %s def policy", di.hvm ? "hvm" : "pv");
-        rc = -errno;
-        goto fail;
+        PERROR("Failed to obtain %s def 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->leaf, xend->subleaf);
-        const xen_cpuid_leaf_t *def_leaf = find_leaf(def, nr_def,
-                                                     xend->leaf, xend->subleaf);
-        const xen_cpuid_leaf_t *host_leaf = find_leaf(host, nr_host, xend->leaf,
-                                                      xend->subleaf);
-
-        if ( cur_leaf == NULL || def_leaf == NULL || host_leaf == NULL )
+        xen_cpuid_leaf_t cur_leaf;
+        xen_cpuid_leaf_t def_leaf;
+        xen_cpuid_leaf_t host_leaf;
+
+        rc = xc_cpu_policy_get_cpuid(xch, policy, cpuid->leaf, cpuid->subleaf,
+                                     &cur_leaf);
+        if ( rc )
         {
-            ERROR("Missing leaf %#x, subleaf %#x", xend->leaf, xend->subleaf);
-            goto fail;
+            ERROR("Failed to get current policy leaf %#x subleaf %#x",
+                  cpuid->leaf, cpuid->subleaf);
+            goto out;
+        }
+        rc = xc_cpu_policy_get_cpuid(xch, def, cpuid->leaf, cpuid->subleaf,
+                                     &def_leaf);
+        if ( rc )
+        {
+            ERROR("Failed to get def 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 *def_reg = &def_leaf->a + i;
-            const uint32_t *host_reg = &host_leaf->a + i;
+            uint32_t *cur_reg = &cur_leaf.a + i;
+            const uint32_t *def_reg = &def_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, def_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);
@@ -413,25 +396,19 @@ static int xc_cpuid_xend_policy(
                     set_bit(31 - j, cur_reg);
             }
         }
-    }
 
-    /* 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);
-    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(def);
-    free(host);
+ out:
+    xc_cpu_policy_destroy(def);
+    xc_cpu_policy_destroy(host);
 
     return rc;
 }
@@ -439,7 +416,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;
@@ -563,6 +540,10 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
         goto out;
     *p = policy.cpuid;
 
+    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 )
     {
@@ -580,9 +561,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



^ permalink raw reply related	[flat|nested] 43+ messages in thread

* [PATCH v2 19/21] libs/guest: apply a featureset into a cpu policy
  2021-04-13 14:01 [PATCH v2 00/21] libs/guest: new CPUID/MSR interface Roger Pau Monne
                   ` (17 preceding siblings ...)
  2021-04-13 14:01 ` [PATCH v2 18/21] libs/guest: rework xc_cpuid_xend_policy Roger Pau Monne
@ 2021-04-13 14:01 ` Roger Pau Monne
  2021-04-13 14:01 ` [PATCH v2 20/21] libs/{light,guest}: implement xc_cpuid_apply_policy in libxl Roger Pau Monne
  2021-04-13 14:01 ` [PATCH v2 21/21] libs/guest: (re)move xc_cpu_policy_apply_cpuid Roger Pau Monne
  20 siblings, 0 replies; 43+ messages in thread
From: Roger Pau Monne @ 2021-04-13 14:01 UTC (permalink / raw)
  To: xen-devel; +Cc: Andrew Cooper, Roger Pau Monne, Ian Jackson, Wei Liu

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 | 95 ++++++++++++++++++++-------------
 2 files changed, 62 insertions(+), 38 deletions(-)

diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index ce0785f7654..3fef954d1d1 100644
--- a/tools/include/xenctrl.h
+++ b/tools/include/xenctrl.h
@@ -2639,6 +2639,11 @@ int xc_cpu_policy_legacy_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 06e3e8131d7..b0690d9799d 100644
--- a/tools/libs/guest/xg_cpuid_x86.c
+++ b/tools/libs/guest/xg_cpuid_x86.c
@@ -481,46 +481,15 @@ 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);
+        *p = policy.cpuid;
     }
     else
     {
@@ -1098,3 +1067,53 @@ int xc_cpu_policy_legacy_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



^ permalink raw reply related	[flat|nested] 43+ messages in thread

* [PATCH v2 20/21] libs/{light,guest}: implement xc_cpuid_apply_policy in libxl
  2021-04-13 14:01 [PATCH v2 00/21] libs/guest: new CPUID/MSR interface Roger Pau Monne
                   ` (18 preceding siblings ...)
  2021-04-13 14:01 ` [PATCH v2 19/21] libs/guest: apply a featureset into a cpu policy Roger Pau Monne
@ 2021-04-13 14:01 ` Roger Pau Monne
  2021-04-28 16:19   ` Anthony PERARD
  2021-04-13 14:01 ` [PATCH v2 21/21] libs/guest: (re)move xc_cpu_policy_apply_cpuid Roger Pau Monne
  20 siblings, 1 reply; 43+ messages in thread
From: Roger Pau Monne @ 2021-04-13 14:01 UTC (permalink / raw)
  To: xen-devel
  Cc: Andrew Cooper, Roger Pau Monne, Ian Jackson, Wei Liu, Anthony PERARD

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>
---
 tools/include/xenctrl.h         |  18 -----
 tools/libs/guest/xg_cpuid_x86.c | 126 --------------------------------
 tools/libs/light/libxl_cpuid.c  |  87 +++++++++++++++++++++-
 3 files changed, 83 insertions(+), 148 deletions(-)

diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index 3fef954d1d1..c6ee1142e8e 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 b0690d9799d..70ca4939392 100644
--- a/tools/libs/guest/xg_cpuid_x86.c
+++ b/tools/libs/guest/xg_cpuid_x86.c
@@ -413,132 +413,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 xc_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 features 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);
-        *p = policy.cpuid;
-    }
-
-    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;
-        }
-        *p = policy.cpuid;
-    }
-    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_legacy_topology(xch, &policy, di.hvm);
-    if ( rc )
-        goto out;
-    *p = policy.cpuid;
-
-    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 = xc_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)
 {
     return calloc(1, sizeof(struct xc_cpu_policy));
diff --git a/tools/libs/light/libxl_cpuid.c b/tools/libs/light/libxl_cpuid.c
index 539fc4869e6..cadc8b2a05e 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_legacy_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 ? ERROR_FAIL : 0;
 }
-- 
2.30.1



^ permalink raw reply related	[flat|nested] 43+ messages in thread

* [PATCH v2 21/21] libs/guest: (re)move xc_cpu_policy_apply_cpuid
  2021-04-13 14:01 [PATCH v2 00/21] libs/guest: new CPUID/MSR interface Roger Pau Monne
                   ` (19 preceding siblings ...)
  2021-04-13 14:01 ` [PATCH v2 20/21] libs/{light,guest}: implement xc_cpuid_apply_policy in libxl Roger Pau Monne
@ 2021-04-13 14:01 ` Roger Pau Monne
  2021-04-28 16:45   ` Anthony PERARD
  20 siblings, 1 reply; 43+ messages in thread
From: Roger Pau Monne @ 2021-04-13 14:01 UTC (permalink / raw)
  To: xen-devel
  Cc: Andrew Cooper, Roger Pau Monne, Ian Jackson, Wei Liu, Anthony PERARD

Move the logic from xc_cpu_policy_apply_cpuid into libxl, now that the
xc_cpu_policy_* helpers allow modifying a cpu policy. 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             |   6 +-
 tools/include/xenctrl.h           |  30 -------
 tools/libs/guest/xg_cpuid_x86.c   | 125 ---------------------------
 tools/libs/light/libxl_cpuid.c    | 136 +++++++++++++++++++++++++++++-
 tools/libs/light/libxl_internal.h |  26 ++++++
 5 files changed, 162 insertions(+), 161 deletions(-)

diff --git a/tools/include/libxl.h b/tools/include/libxl.h
index ae7fe27c1f2..150b7ba85ac 100644
--- a/tools/include/libxl.h
+++ b/tools/include/libxl.h
@@ -1375,10 +1375,10 @@ void libxl_bitmap_init(libxl_bitmap *map);
 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.
+ * libxl_cpuid_policy is opaque in the libxl ABI. Users of libxl 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 c6ee1142e8e..044d05321a3 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_legacy_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 70ca4939392..5144b186028 100644
--- a/tools/libs/guest/xg_cpuid_x86.c
+++ b/tools/libs/guest/xg_cpuid_x86.c
@@ -288,131 +288,6 @@ static xen_cpuid_leaf_t *find_leaf(
     return bsearch(&key, leaves, nr_leaves, sizeof(*leaves), compare_leaves);
 }
 
-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, def = NULL;
-
-    host = xc_cpu_policy_init();
-    def = xc_cpu_policy_init();
-    if ( !host || !def )
-    {
-        PERROR("Failed to init policies");
-        rc = -ENOMEM;
-        goto out;
-    }
-
-    /* Get the domain type's default policy. */
-    rc = xc_cpu_policy_get_system(xch, hvm ? XEN_SYSCTL_cpu_policy_hvm_default
-                                           : XEN_SYSCTL_cpu_policy_pv_default,
-                                  def);
-    if ( rc )
-    {
-        PERROR("Failed to obtain %s def 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 def_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, def, cpuid->leaf, cpuid->subleaf,
-                                     &def_leaf);
-        if ( rc )
-        {
-            ERROR("Failed to get def 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 *def_reg = &def_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, def_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(def);
-    xc_cpu_policy_destroy(host);
-
-    return rc;
-}
-
 xc_cpu_policy_t xc_cpu_policy_init(void)
 {
     return calloc(1, sizeof(struct xc_cpu_policy));
diff --git a/tools/libs/light/libxl_cpuid.c b/tools/libs/light/libxl_cpuid.c
index cadc8b2a05e..6be2d773d1d 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,136 @@ 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, def = NULL;
+
+    host = xc_cpu_policy_init();
+    def = xc_cpu_policy_init();
+    if (!host || !def) {
+        LOG(ERROR, "Failed to init policies");
+        rc = ERROR_FAIL;
+        goto out;
+    }
+
+    /* Get the domain type's default policy. */
+    rc = xc_cpu_policy_get_system(ctx->xch,
+                                  hvm ? XEN_SYSCTL_cpu_policy_hvm_default
+                                      : XEN_SYSCTL_cpu_policy_pv_default,
+                                  def);
+    if (rc) {
+        LOGE(ERROR, "Failed to obtain %s def 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 def_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, def, cpuid->leaf, cpuid->subleaf,
+                                     &def_leaf);
+        if (rc) {
+            LOGE(ERROR, "Failed to get def 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 *def_reg = &def_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, def_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(def);
+    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 +665,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 5709bcb93fa..d130c073ac4 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



^ permalink raw reply related	[flat|nested] 43+ messages in thread

* Re: [PATCH v2 04/21] libs/guest: introduce helper to fetch a system cpu policy
  2021-04-13 14:01 ` [PATCH v2 04/21] libs/guest: introduce helper to fetch a system cpu policy Roger Pau Monne
@ 2021-04-14 13:28   ` Jan Beulich
  0 siblings, 0 replies; 43+ messages in thread
From: Jan Beulich @ 2021-04-14 13:28 UTC (permalink / raw)
  To: Roger Pau Monne; +Cc: Andrew Cooper, Ian Jackson, Wei Liu, xen-devel

On 13.04.2021 16:01, 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>


^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH v2 14/21] libs/guest: introduce helper to check cpu policy compatibility
  2021-04-13 14:01 ` [PATCH v2 14/21] libs/guest: introduce helper to check cpu policy compatibility Roger Pau Monne
@ 2021-04-14 13:36   ` Jan Beulich
  2021-04-22  8:22     ` Roger Pau Monné
  0 siblings, 1 reply; 43+ messages in thread
From: Jan Beulich @ 2021-04-14 13:36 UTC (permalink / raw)
  To: Roger Pau Monne; +Cc: Andrew Cooper, Ian Jackson, Wei Liu, xen-devel

On 13.04.2021 16:01, Roger Pau Monne wrote:
> --- a/tools/libs/guest/xg_cpuid_x86.c
> +++ b/tools/libs/guest/xg_cpuid_x86.c
> @@ -925,3 +925,22 @@ 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 host,
> +                                 const xc_cpu_policy_t guest)
> +{
> +    struct cpu_policy_errors err = INIT_CPU_POLICY_ERRORS;
> +    struct cpu_policy h = { &host->cpuid, &host->msr };
> +    struct cpu_policy g = { &guest->cpuid, &guest->msr };
> +    int rc = x86_cpu_policies_are_compatible(&h, &g, &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);

Personally I'm against making assumptions like these ones about what
(in this case) INIT_CPU_POLICY_ERRORS actually expands to (i.e. three
times -1). I can see how alternatives to this are quickly going to
get ugly, so I'll leave it to others to judge.

Jan


^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH v2 15/21] libs/guest: obtain a compatible cpu policy from two input ones
  2021-04-13 14:01 ` [PATCH v2 15/21] libs/guest: obtain a compatible cpu policy from two input ones Roger Pau Monne
@ 2021-04-14 13:49   ` Jan Beulich
  2021-04-22  9:42     ` Roger Pau Monné
  2021-04-21 10:22   ` Wei Liu
  1 sibling, 1 reply; 43+ messages in thread
From: Jan Beulich @ 2021-04-14 13:49 UTC (permalink / raw)
  To: Roger Pau Monne
  Cc: Andrew Cooper, Ian Jackson, Wei Liu, Anthony PERARD, xen-devel

On 13.04.2021 16:01, Roger Pau Monne wrote:
> @@ -944,3 +945,130 @@ bool xc_cpu_policy_is_compatible(xc_interface *xch, const xc_cpu_policy_t host,
>  
>      return false;
>  }
> +
> +static uint64_t level_msr(unsigned int index, uint64_t val1, uint64_t val2)
> +{
> +    uint64_t val = val1 & val2;;

For arbitrary MSRs this isn't going to do any good. If only very
specific MSRs are assumed to make it here, I think this wants
commenting on.

Also, nit: stray semicolon.

> +    switch ( index )
> +    {
> +    case MSR_ARCH_CAPABILITIES:
> +        /*
> +         * 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;
> +    }
> +
> +    return val;
> +}
> +
> +static bool level_leaf(xen_cpuid_leaf_t *l1, xen_cpuid_leaf_t *l2,

const (twice)?

> +                       xen_cpuid_leaf_t *out)
> +{
> +    *out = (xen_cpuid_leaf_t){ };
> +
> +    switch ( l1->leaf )
> +    {
> +    case 0x1:
> +    case 0x80000001:
> +        out->c = l1->c & l2->c;
> +        out->d = l1->d & l2->d;
> +        return true;
> +
> +    case 0xd:
> +        if ( l1->subleaf != 1 )
> +            break;
> +        out->a = l1->a & l2->a;
> +        return true;

Could you explain your thinking behind this (a code comment would
likely help)? You effectively discard everything except subleaf 1
by returning false in that case, don't you?

> +    case 0x7:
> +        switch ( l1->subleaf )
> +        {
> +        case 0:
> +            out->b = l1->b & l2->b;
> +            out->c = l1->c & l2->c;
> +            out->d = l1->d & l2->d;
> +            return true;
> +
> +        case 1:
> +            out->a = l1->a & l2->a;
> +            return true;
> +        }
> +        break;

Can we perhaps assume all subleaves here are going to hold flags,
and hence and both sides together without regard to what subleaf
we're actually dealing with (subleaf 1 remaining special as to
EAX of course)? This would avoid having to remember to make yet
another mechanical change when enabling a new subleaf.

> +    case 0x80000007:
> +        out->d = l1->d & l2->d;
> +        return true;
> +
> +    case 0x80000008:
> +        out->b = l1->b & l2->b;
> +        return true;
> +    }
> +
> +    return false;
> +}

Considering your LFENCE-always-serializing patch, I assume
whichever ends up going in last will take care of adding handling
of that leaf here?

Jan


^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH v2 15/21] libs/guest: obtain a compatible cpu policy from two input ones
  2021-04-13 14:01 ` [PATCH v2 15/21] libs/guest: obtain a compatible cpu policy from two input ones Roger Pau Monne
  2021-04-14 13:49   ` Jan Beulich
@ 2021-04-21 10:22   ` Wei Liu
  2021-04-21 11:26     ` Roger Pau Monné
  1 sibling, 1 reply; 43+ messages in thread
From: Wei Liu @ 2021-04-21 10:22 UTC (permalink / raw)
  To: Roger Pau Monne
  Cc: xen-devel, Andrew Cooper, Ian Jackson, Wei Liu, Anthony PERARD

On Tue, Apr 13, 2021 at 04:01:33PM +0200, 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 thought canonical source for compatibility was to be put into the
hypervisor, thus all this functionality would be in the hypervisor. Am I
misremembering?

Wei.


^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH v2 15/21] libs/guest: obtain a compatible cpu policy from two input ones
  2021-04-21 10:22   ` Wei Liu
@ 2021-04-21 11:26     ` Roger Pau Monné
  2021-04-21 16:42       ` Wei Liu
  0 siblings, 1 reply; 43+ messages in thread
From: Roger Pau Monné @ 2021-04-21 11:26 UTC (permalink / raw)
  To: Wei Liu; +Cc: xen-devel, Andrew Cooper, Ian Jackson, Anthony PERARD

On Wed, Apr 21, 2021 at 10:22:39AM +0000, Wei Liu wrote:
> On Tue, Apr 13, 2021 at 04:01:33PM +0200, 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 thought canonical source for compatibility was to be put into the
> hypervisor, thus all this functionality would be in the hypervisor. Am I
> misremembering?

Andrew said something similar on v1, but I'm not able to figure how
this would be used by the hypervisor.

It's my understating that the toolstack will attempt to generate a CPU
policy and forward it to the hypervisor, which will either accept or
reject it based on the capabilities of the system. I'm not sure I see
why we would need to give the hypervisor two policies in order to
generate a resulting compatible one - it should all be done by the
toolstack AFAICT.

If there's a use case for this being in the hypervisor I'm happy to
add it there, but so far I haven't been able to come up with one
myself, and hence I don't see the need to make the code available.

Thanks, Roger.


^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH v2 15/21] libs/guest: obtain a compatible cpu policy from two input ones
  2021-04-21 11:26     ` Roger Pau Monné
@ 2021-04-21 16:42       ` Wei Liu
  0 siblings, 0 replies; 43+ messages in thread
From: Wei Liu @ 2021-04-21 16:42 UTC (permalink / raw)
  To: Roger Pau Monné
  Cc: Wei Liu, xen-devel, Andrew Cooper, Ian Jackson, Anthony PERARD

On Wed, Apr 21, 2021 at 01:26:49PM +0200, Roger Pau Monné wrote:
> On Wed, Apr 21, 2021 at 10:22:39AM +0000, Wei Liu wrote:
> > On Tue, Apr 13, 2021 at 04:01:33PM +0200, 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 thought canonical source for compatibility was to be put into the
> > hypervisor, thus all this functionality would be in the hypervisor. Am I
> > misremembering?
> 
> Andrew said something similar on v1, but I'm not able to figure how
> this would be used by the hypervisor.
> 
> It's my understating that the toolstack will attempt to generate a CPU
> policy and forward it to the hypervisor, which will either accept or
> reject it based on the capabilities of the system. I'm not sure I see
> why we would need to give the hypervisor two policies in order to
> generate a resulting compatible one - it should all be done by the
> toolstack AFAICT.
> 
> If there's a use case for this being in the hypervisor I'm happy to
> add it there, but so far I haven't been able to come up with one
> myself, and hence I don't see the need to make the code available.

I have no opinion here. Just wanted to get on the same page.

Thanks for the explanation.

Wei.

> 
> Thanks, Roger.


^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH v2 14/21] libs/guest: introduce helper to check cpu policy compatibility
  2021-04-14 13:36   ` Jan Beulich
@ 2021-04-22  8:22     ` Roger Pau Monné
  2021-04-22  8:31       ` Jan Beulich
  0 siblings, 1 reply; 43+ messages in thread
From: Roger Pau Monné @ 2021-04-22  8:22 UTC (permalink / raw)
  To: Jan Beulich; +Cc: Andrew Cooper, Ian Jackson, Wei Liu, xen-devel

On Wed, Apr 14, 2021 at 03:36:54PM +0200, Jan Beulich wrote:
> On 13.04.2021 16:01, Roger Pau Monne wrote:
> > --- a/tools/libs/guest/xg_cpuid_x86.c
> > +++ b/tools/libs/guest/xg_cpuid_x86.c
> > @@ -925,3 +925,22 @@ 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 host,
> > +                                 const xc_cpu_policy_t guest)
> > +{
> > +    struct cpu_policy_errors err = INIT_CPU_POLICY_ERRORS;
> > +    struct cpu_policy h = { &host->cpuid, &host->msr };
> > +    struct cpu_policy g = { &guest->cpuid, &guest->msr };
> > +    int rc = x86_cpu_policies_are_compatible(&h, &g, &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);
> 
> Personally I'm against making assumptions like these ones about what
> (in this case) INIT_CPU_POLICY_ERRORS actually expands to (i.e. three
> times -1). I can see how alternatives to this are quickly going to
> get ugly, so I'll leave it to others to judge.

Would you like me to define a separate POLICY_ERROR? ie:

#define INIT_CPU_POLICY_ERROR -1
#define INIT_CPU_POLICY_ERRORS { INIT_CPU_POLICY_ERROR, \
                                 INIT_CPU_POLICY_ERROR, \
                                 INIT_CPU_POLICY_ERROR }

We already have a bunch of open coded -1 checks anyway, but might
prevent new ones from appearing.

Thanks, Roger.


^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH v2 14/21] libs/guest: introduce helper to check cpu policy compatibility
  2021-04-22  8:22     ` Roger Pau Monné
@ 2021-04-22  8:31       ` Jan Beulich
  0 siblings, 0 replies; 43+ messages in thread
From: Jan Beulich @ 2021-04-22  8:31 UTC (permalink / raw)
  To: Roger Pau Monné, Andrew Cooper; +Cc: Ian Jackson, Wei Liu, xen-devel

On 22.04.2021 10:22, Roger Pau Monné wrote:
> On Wed, Apr 14, 2021 at 03:36:54PM +0200, Jan Beulich wrote:
>> On 13.04.2021 16:01, Roger Pau Monne wrote:
>>> --- a/tools/libs/guest/xg_cpuid_x86.c
>>> +++ b/tools/libs/guest/xg_cpuid_x86.c
>>> @@ -925,3 +925,22 @@ 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 host,
>>> +                                 const xc_cpu_policy_t guest)
>>> +{
>>> +    struct cpu_policy_errors err = INIT_CPU_POLICY_ERRORS;
>>> +    struct cpu_policy h = { &host->cpuid, &host->msr };
>>> +    struct cpu_policy g = { &guest->cpuid, &guest->msr };
>>> +    int rc = x86_cpu_policies_are_compatible(&h, &g, &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);
>>
>> Personally I'm against making assumptions like these ones about what
>> (in this case) INIT_CPU_POLICY_ERRORS actually expands to (i.e. three
>> times -1). I can see how alternatives to this are quickly going to
>> get ugly, so I'll leave it to others to judge.
> 
> Would you like me to define a separate POLICY_ERROR? ie:
> 
> #define INIT_CPU_POLICY_ERROR -1
> #define INIT_CPU_POLICY_ERRORS { INIT_CPU_POLICY_ERROR, \
>                                  INIT_CPU_POLICY_ERROR, \
>                                  INIT_CPU_POLICY_ERROR }

I would prefer this; I'm not sure (nit: properly parenthesized)
-1 is  the value to use though, considering the fields are unsigned.
I wonder what Andrew thinks, as he did introduce all of this.

> We already have a bunch of open coded -1 checks anyway, but might
> prevent new ones from appearing.

Indeed.

Jan


^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH v2 15/21] libs/guest: obtain a compatible cpu policy from two input ones
  2021-04-14 13:49   ` Jan Beulich
@ 2021-04-22  9:42     ` Roger Pau Monné
  2021-04-22  9:58       ` Jan Beulich
  0 siblings, 1 reply; 43+ messages in thread
From: Roger Pau Monné @ 2021-04-22  9:42 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Andrew Cooper, Ian Jackson, Wei Liu, Anthony PERARD, xen-devel

On Wed, Apr 14, 2021 at 03:49:02PM +0200, Jan Beulich wrote:
> On 13.04.2021 16:01, Roger Pau Monne wrote:
> > @@ -944,3 +945,130 @@ bool xc_cpu_policy_is_compatible(xc_interface *xch, const xc_cpu_policy_t host,
> >  
> >      return false;
> >  }
> > +
> > +static uint64_t level_msr(unsigned int index, uint64_t val1, uint64_t val2)
> > +{
> > +    uint64_t val = val1 & val2;;
> 
> For arbitrary MSRs this isn't going to do any good. If only very
> specific MSRs are assumed to make it here, I think this wants
> commenting on.

I've added: "MSRs passed to level_msr are expected to be bitmaps of
features"

> > +                       xen_cpuid_leaf_t *out)
> > +{
> > +    *out = (xen_cpuid_leaf_t){ };
> > +
> > +    switch ( l1->leaf )
> > +    {
> > +    case 0x1:
> > +    case 0x80000001:
> > +        out->c = l1->c & l2->c;
> > +        out->d = l1->d & l2->d;
> > +        return true;
> > +
> > +    case 0xd:
> > +        if ( l1->subleaf != 1 )
> > +            break;
> > +        out->a = l1->a & l2->a;
> > +        return true;
> 
> Could you explain your thinking behind this (a code comment would
> likely help)? You effectively discard everything except subleaf 1
> by returning false in that case, don't you?

Yes, the intent is to only level the features bitfield found in
subleaf 1.

I was planning for level_leaf so far in this series to deal with the
feature leaves part of the featureset only. I guess you would also
like to leverage other parts of the xstate leaf, like the max_size or
the supported bits in xss_{low,high}?

> > +    case 0x7:
> > +        switch ( l1->subleaf )
> > +        {
> > +        case 0:
> > +            out->b = l1->b & l2->b;
> > +            out->c = l1->c & l2->c;
> > +            out->d = l1->d & l2->d;
> > +            return true;
> > +
> > +        case 1:
> > +            out->a = l1->a & l2->a;
> > +            return true;
> > +        }
> > +        break;
> 
> Can we perhaps assume all subleaves here are going to hold flags,
> and hence and both sides together without regard to what subleaf
> we're actually dealing with (subleaf 1 remaining special as to

I think you meant subleaf 0 EAX (the max subleaf value)?

subleaf 1 EAX is just a normal bitfield AFAIK.

> EAX of course)? This would avoid having to remember to make yet
> another mechanical change when enabling a new subleaf.

Sure, seems like a fine approach since leaf 7 will only contain
feature bitmaps.

> > +    case 0x80000007:
> > +        out->d = l1->d & l2->d;
> > +        return true;
> > +
> > +    case 0x80000008:
> > +        out->b = l1->b & l2->b;
> > +        return true;
> > +    }
> > +
> > +    return false;
> > +}
> 
> Considering your LFENCE-always-serializing patch, I assume
> whichever ends up going in last will take care of adding handling
> of that leaf here?

Indeed.

Thanks, Roger.


^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH v2 15/21] libs/guest: obtain a compatible cpu policy from two input ones
  2021-04-22  9:42     ` Roger Pau Monné
@ 2021-04-22  9:58       ` Jan Beulich
  2021-04-22 10:34         ` Roger Pau Monné
  0 siblings, 1 reply; 43+ messages in thread
From: Jan Beulich @ 2021-04-22  9:58 UTC (permalink / raw)
  To: Roger Pau Monné
  Cc: Andrew Cooper, Ian Jackson, Wei Liu, Anthony PERARD, xen-devel

On 22.04.2021 11:42, Roger Pau Monné wrote:
> On Wed, Apr 14, 2021 at 03:49:02PM +0200, Jan Beulich wrote:
>> On 13.04.2021 16:01, Roger Pau Monne wrote:
>>> @@ -944,3 +945,130 @@ bool xc_cpu_policy_is_compatible(xc_interface *xch, const xc_cpu_policy_t host,
>>>  
>>>      return false;
>>>  }
>>> +
>>> +static uint64_t level_msr(unsigned int index, uint64_t val1, uint64_t val2)
>>> +{
>>> +    uint64_t val = val1 & val2;;
>>
>> For arbitrary MSRs this isn't going to do any good. If only very
>> specific MSRs are assumed to make it here, I think this wants
>> commenting on.
> 
> I've added: "MSRs passed to level_msr are expected to be bitmaps of
> features"

How does such a comment help? I.e. how does the caller tell which MSRs
to pass here and which to deal with anyother way?

>>> +                       xen_cpuid_leaf_t *out)
>>> +{
>>> +    *out = (xen_cpuid_leaf_t){ };
>>> +
>>> +    switch ( l1->leaf )
>>> +    {
>>> +    case 0x1:
>>> +    case 0x80000001:
>>> +        out->c = l1->c & l2->c;
>>> +        out->d = l1->d & l2->d;
>>> +        return true;
>>> +
>>> +    case 0xd:
>>> +        if ( l1->subleaf != 1 )
>>> +            break;
>>> +        out->a = l1->a & l2->a;
>>> +        return true;
>>
>> Could you explain your thinking behind this (a code comment would
>> likely help)? You effectively discard everything except subleaf 1
>> by returning false in that case, don't you?
> 
> Yes, the intent is to only level the features bitfield found in
> subleaf 1.
> 
> I was planning for level_leaf so far in this series to deal with the
> feature leaves part of the featureset only. I guess you would also
> like to leverage other parts of the xstate leaf, like the max_size or
> the supported bits in xss_{low,high}?

The latter is clearly one of the things to consider, yes (alongside
the respective bits in sub-leaf 0 for XCR0). Sub-leaves > 1 may also
need dealing with ECX. Yet then again some or all of this may need
handling elsewhere, not the least because of the unusual handling of
leaf 0xd in the hypervisor. What gets checked and/or adjusted where
needs to be settled upon, and then the different parts of code would
imo better cross-reference each other.

>>> +    case 0x7:
>>> +        switch ( l1->subleaf )
>>> +        {
>>> +        case 0:
>>> +            out->b = l1->b & l2->b;
>>> +            out->c = l1->c & l2->c;
>>> +            out->d = l1->d & l2->d;
>>> +            return true;
>>> +
>>> +        case 1:
>>> +            out->a = l1->a & l2->a;
>>> +            return true;
>>> +        }
>>> +        break;
>>
>> Can we perhaps assume all subleaves here are going to hold flags,
>> and hence and both sides together without regard to what subleaf
>> we're actually dealing with (subleaf 1 remaining special as to
> 
> I think you meant subleaf 0 EAX (the max subleaf value)?
> 
> subleaf 1 EAX is just a normal bitfield AFAIK.

Indeed I did.

Jan

>> EAX of course)? This would avoid having to remember to make yet
>> another mechanical change when enabling a new subleaf.
> 
> Sure, seems like a fine approach since leaf 7 will only contain
> feature bitmaps.


^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH v2 15/21] libs/guest: obtain a compatible cpu policy from two input ones
  2021-04-22  9:58       ` Jan Beulich
@ 2021-04-22 10:34         ` Roger Pau Monné
  2021-04-22 10:48           ` Jan Beulich
  0 siblings, 1 reply; 43+ messages in thread
From: Roger Pau Monné @ 2021-04-22 10:34 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Andrew Cooper, Ian Jackson, Wei Liu, Anthony PERARD, xen-devel

On Thu, Apr 22, 2021 at 11:58:45AM +0200, Jan Beulich wrote:
> On 22.04.2021 11:42, Roger Pau Monné wrote:
> > On Wed, Apr 14, 2021 at 03:49:02PM +0200, Jan Beulich wrote:
> >> On 13.04.2021 16:01, Roger Pau Monne wrote:
> >>> @@ -944,3 +945,130 @@ bool xc_cpu_policy_is_compatible(xc_interface *xch, const xc_cpu_policy_t host,
> >>>  
> >>>      return false;
> >>>  }
> >>> +
> >>> +static uint64_t level_msr(unsigned int index, uint64_t val1, uint64_t val2)
> >>> +{
> >>> +    uint64_t val = val1 & val2;;
> >>
> >> For arbitrary MSRs this isn't going to do any good. If only very
> >> specific MSRs are assumed to make it here, I think this wants
> >> commenting on.
> > 
> > I've added: "MSRs passed to level_msr are expected to be bitmaps of
> > features"
> 
> How does such a comment help? I.e. how does the caller tell which MSRs
> to pass here and which to deal with anyother way?

All MSRs should be passed to level_msr, but it's handling logic would
need to be expanded to support MSRs that are not feature bitmaps.

It might be best to restore the previous switch and handle each MSR
specifically?

From your reply to v1 I wrongly misunderstood that you initially
wanted to handle all MSRs as bitmaps:

https://lore.kernel.org/xen-devel/f66e61d5-e4a0-cba3-f15c-73ca54ac4964@suse.com/

> >>> +                       xen_cpuid_leaf_t *out)
> >>> +{
> >>> +    *out = (xen_cpuid_leaf_t){ };
> >>> +
> >>> +    switch ( l1->leaf )
> >>> +    {
> >>> +    case 0x1:
> >>> +    case 0x80000001:
> >>> +        out->c = l1->c & l2->c;
> >>> +        out->d = l1->d & l2->d;
> >>> +        return true;
> >>> +
> >>> +    case 0xd:
> >>> +        if ( l1->subleaf != 1 )
> >>> +            break;
> >>> +        out->a = l1->a & l2->a;
> >>> +        return true;
> >>
> >> Could you explain your thinking behind this (a code comment would
> >> likely help)? You effectively discard everything except subleaf 1
> >> by returning false in that case, don't you?
> > 
> > Yes, the intent is to only level the features bitfield found in
> > subleaf 1.
> > 
> > I was planning for level_leaf so far in this series to deal with the
> > feature leaves part of the featureset only. I guess you would also
> > like to leverage other parts of the xstate leaf, like the max_size or
> > the supported bits in xss_{low,high}?
> 
> The latter is clearly one of the things to consider, yes (alongside
> the respective bits in sub-leaf 0 for XCR0). Sub-leaves > 1 may also
> need dealing with ECX. Yet then again some or all of this may need
> handling elsewhere, not the least because of the unusual handling of
> leaf 0xd in the hypervisor. What gets checked and/or adjusted where
> needs to be settled upon, and then the different parts of code would
> imo better cross-reference each other.

There's a comment in recalculate_xstate that mentions that Da1 leaf is
the only piece of information preserved, and that everything else is
derived from feature state. I don't think it makes sense to try to
level anything apart from Da1 if it's going to be discarded by
recalculate_xstate anyway?

I can add a comment here regarding why only Da1 is taken into account
for leveling so far.

Thanks, Roger.


^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH v2 15/21] libs/guest: obtain a compatible cpu policy from two input ones
  2021-04-22 10:34         ` Roger Pau Monné
@ 2021-04-22 10:48           ` Jan Beulich
  2021-04-22 10:56             ` Roger Pau Monné
  0 siblings, 1 reply; 43+ messages in thread
From: Jan Beulich @ 2021-04-22 10:48 UTC (permalink / raw)
  To: Roger Pau Monné
  Cc: Andrew Cooper, Ian Jackson, Wei Liu, Anthony PERARD, xen-devel

On 22.04.2021 12:34, Roger Pau Monné wrote:
> On Thu, Apr 22, 2021 at 11:58:45AM +0200, Jan Beulich wrote:
>> On 22.04.2021 11:42, Roger Pau Monné wrote:
>>> On Wed, Apr 14, 2021 at 03:49:02PM +0200, Jan Beulich wrote:
>>>> On 13.04.2021 16:01, Roger Pau Monne wrote:
>>>>> @@ -944,3 +945,130 @@ bool xc_cpu_policy_is_compatible(xc_interface *xch, const xc_cpu_policy_t host,
>>>>>  
>>>>>      return false;
>>>>>  }
>>>>> +
>>>>> +static uint64_t level_msr(unsigned int index, uint64_t val1, uint64_t val2)
>>>>> +{
>>>>> +    uint64_t val = val1 & val2;;
>>>>
>>>> For arbitrary MSRs this isn't going to do any good. If only very
>>>> specific MSRs are assumed to make it here, I think this wants
>>>> commenting on.
>>>
>>> I've added: "MSRs passed to level_msr are expected to be bitmaps of
>>> features"
>>
>> How does such a comment help? I.e. how does the caller tell which MSRs
>> to pass here and which to deal with anyother way?
> 
> All MSRs should be passed to level_msr, but it's handling logic would
> need to be expanded to support MSRs that are not feature bitmaps.
> 
> It might be best to restore the previous switch and handle each MSR
> specifically?

I think so, yes. We need to be very careful with what a possible
default case does there, though.

>>>>> +                       xen_cpuid_leaf_t *out)
>>>>> +{
>>>>> +    *out = (xen_cpuid_leaf_t){ };
>>>>> +
>>>>> +    switch ( l1->leaf )
>>>>> +    {
>>>>> +    case 0x1:
>>>>> +    case 0x80000001:
>>>>> +        out->c = l1->c & l2->c;
>>>>> +        out->d = l1->d & l2->d;
>>>>> +        return true;
>>>>> +
>>>>> +    case 0xd:
>>>>> +        if ( l1->subleaf != 1 )
>>>>> +            break;
>>>>> +        out->a = l1->a & l2->a;
>>>>> +        return true;
>>>>
>>>> Could you explain your thinking behind this (a code comment would
>>>> likely help)? You effectively discard everything except subleaf 1
>>>> by returning false in that case, don't you?
>>>
>>> Yes, the intent is to only level the features bitfield found in
>>> subleaf 1.
>>>
>>> I was planning for level_leaf so far in this series to deal with the
>>> feature leaves part of the featureset only. I guess you would also
>>> like to leverage other parts of the xstate leaf, like the max_size or
>>> the supported bits in xss_{low,high}?
>>
>> The latter is clearly one of the things to consider, yes (alongside
>> the respective bits in sub-leaf 0 for XCR0). Sub-leaves > 1 may also
>> need dealing with ECX. Yet then again some or all of this may need
>> handling elsewhere, not the least because of the unusual handling of
>> leaf 0xd in the hypervisor. What gets checked and/or adjusted where
>> needs to be settled upon, and then the different parts of code would
>> imo better cross-reference each other.
> 
> There's a comment in recalculate_xstate that mentions that Da1 leaf is
> the only piece of information preserved, and that everything else is
> derived from feature state. I don't think it makes sense to try to
> level anything apart from Da1 if it's going to be discarded by
> recalculate_xstate anyway?
> 
> I can add a comment here regarding why only Da1 is taken into account
> for leveling so far.

Yes, this would help. Thanks.

Jan


^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH v2 15/21] libs/guest: obtain a compatible cpu policy from two input ones
  2021-04-22 10:48           ` Jan Beulich
@ 2021-04-22 10:56             ` Roger Pau Monné
  2021-04-22 11:05               ` Jan Beulich
  0 siblings, 1 reply; 43+ messages in thread
From: Roger Pau Monné @ 2021-04-22 10:56 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Andrew Cooper, Ian Jackson, Wei Liu, Anthony PERARD, xen-devel

On Thu, Apr 22, 2021 at 12:48:36PM +0200, Jan Beulich wrote:
> On 22.04.2021 12:34, Roger Pau Monné wrote:
> > On Thu, Apr 22, 2021 at 11:58:45AM +0200, Jan Beulich wrote:
> >> On 22.04.2021 11:42, Roger Pau Monné wrote:
> >>> On Wed, Apr 14, 2021 at 03:49:02PM +0200, Jan Beulich wrote:
> >>>> On 13.04.2021 16:01, Roger Pau Monne wrote:
> >>>>> @@ -944,3 +945,130 @@ bool xc_cpu_policy_is_compatible(xc_interface *xch, const xc_cpu_policy_t host,
> >>>>>  
> >>>>>      return false;
> >>>>>  }
> >>>>> +
> >>>>> +static uint64_t level_msr(unsigned int index, uint64_t val1, uint64_t val2)
> >>>>> +{
> >>>>> +    uint64_t val = val1 & val2;;
> >>>>
> >>>> For arbitrary MSRs this isn't going to do any good. If only very
> >>>> specific MSRs are assumed to make it here, I think this wants
> >>>> commenting on.
> >>>
> >>> I've added: "MSRs passed to level_msr are expected to be bitmaps of
> >>> features"
> >>
> >> How does such a comment help? I.e. how does the caller tell which MSRs
> >> to pass here and which to deal with anyother way?
> > 
> > All MSRs should be passed to level_msr, but it's handling logic would
> > need to be expanded to support MSRs that are not feature bitmaps.
> > 
> > It might be best to restore the previous switch and handle each MSR
> > specifically?
> 
> I think so, yes. We need to be very careful with what a possible
> default case does there, though.

Maybe it would be better to handle level_msr in a way similar to
level_leaf: return true/false to notice whether the MSR should be
added to the resulting compatible policy?

And then make the default case in level_msr just return false in order
to prevent adding MSRs not properly leveled to the policy?

Thanks, Roger.


^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH v2 15/21] libs/guest: obtain a compatible cpu policy from two input ones
  2021-04-22 10:56             ` Roger Pau Monné
@ 2021-04-22 11:05               ` Jan Beulich
  2021-04-22 11:37                 ` Roger Pau Monné
  0 siblings, 1 reply; 43+ messages in thread
From: Jan Beulich @ 2021-04-22 11:05 UTC (permalink / raw)
  To: Roger Pau Monné
  Cc: Andrew Cooper, Ian Jackson, Wei Liu, Anthony PERARD, xen-devel

On 22.04.2021 12:56, Roger Pau Monné wrote:
> On Thu, Apr 22, 2021 at 12:48:36PM +0200, Jan Beulich wrote:
>> On 22.04.2021 12:34, Roger Pau Monné wrote:
>>> On Thu, Apr 22, 2021 at 11:58:45AM +0200, Jan Beulich wrote:
>>>> On 22.04.2021 11:42, Roger Pau Monné wrote:
>>>>> On Wed, Apr 14, 2021 at 03:49:02PM +0200, Jan Beulich wrote:
>>>>>> On 13.04.2021 16:01, Roger Pau Monne wrote:
>>>>>>> @@ -944,3 +945,130 @@ bool xc_cpu_policy_is_compatible(xc_interface *xch, const xc_cpu_policy_t host,
>>>>>>>  
>>>>>>>      return false;
>>>>>>>  }
>>>>>>> +
>>>>>>> +static uint64_t level_msr(unsigned int index, uint64_t val1, uint64_t val2)
>>>>>>> +{
>>>>>>> +    uint64_t val = val1 & val2;;
>>>>>>
>>>>>> For arbitrary MSRs this isn't going to do any good. If only very
>>>>>> specific MSRs are assumed to make it here, I think this wants
>>>>>> commenting on.
>>>>>
>>>>> I've added: "MSRs passed to level_msr are expected to be bitmaps of
>>>>> features"
>>>>
>>>> How does such a comment help? I.e. how does the caller tell which MSRs
>>>> to pass here and which to deal with anyother way?
>>>
>>> All MSRs should be passed to level_msr, but it's handling logic would
>>> need to be expanded to support MSRs that are not feature bitmaps.
>>>
>>> It might be best to restore the previous switch and handle each MSR
>>> specifically?
>>
>> I think so, yes. We need to be very careful with what a possible
>> default case does there, though.
> 
> Maybe it would be better to handle level_msr in a way similar to
> level_leaf: return true/false to notice whether the MSR should be
> added to the resulting compatible policy?
> 
> And then make the default case in level_msr just return false in order
> to prevent adding MSRs not properly leveled to the policy?

I'm afraid I'm not clear about the implications. What again is the
(planned?) final effect of an MSR not getting added there?

Jan


^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH v2 15/21] libs/guest: obtain a compatible cpu policy from two input ones
  2021-04-22 11:05               ` Jan Beulich
@ 2021-04-22 11:37                 ` Roger Pau Monné
  2021-04-22 11:42                   ` Jan Beulich
  0 siblings, 1 reply; 43+ messages in thread
From: Roger Pau Monné @ 2021-04-22 11:37 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Andrew Cooper, Ian Jackson, Wei Liu, Anthony PERARD, xen-devel

On Thu, Apr 22, 2021 at 01:05:23PM +0200, Jan Beulich wrote:
> On 22.04.2021 12:56, Roger Pau Monné wrote:
> > On Thu, Apr 22, 2021 at 12:48:36PM +0200, Jan Beulich wrote:
> >> On 22.04.2021 12:34, Roger Pau Monné wrote:
> >>> On Thu, Apr 22, 2021 at 11:58:45AM +0200, Jan Beulich wrote:
> >>>> On 22.04.2021 11:42, Roger Pau Monné wrote:
> >>>>> On Wed, Apr 14, 2021 at 03:49:02PM +0200, Jan Beulich wrote:
> >>>>>> On 13.04.2021 16:01, Roger Pau Monne wrote:
> >>>>>>> @@ -944,3 +945,130 @@ bool xc_cpu_policy_is_compatible(xc_interface *xch, const xc_cpu_policy_t host,
> >>>>>>>  
> >>>>>>>      return false;
> >>>>>>>  }
> >>>>>>> +
> >>>>>>> +static uint64_t level_msr(unsigned int index, uint64_t val1, uint64_t val2)
> >>>>>>> +{
> >>>>>>> +    uint64_t val = val1 & val2;;
> >>>>>>
> >>>>>> For arbitrary MSRs this isn't going to do any good. If only very
> >>>>>> specific MSRs are assumed to make it here, I think this wants
> >>>>>> commenting on.
> >>>>>
> >>>>> I've added: "MSRs passed to level_msr are expected to be bitmaps of
> >>>>> features"
> >>>>
> >>>> How does such a comment help? I.e. how does the caller tell which MSRs
> >>>> to pass here and which to deal with anyother way?
> >>>
> >>> All MSRs should be passed to level_msr, but it's handling logic would
> >>> need to be expanded to support MSRs that are not feature bitmaps.
> >>>
> >>> It might be best to restore the previous switch and handle each MSR
> >>> specifically?
> >>
> >> I think so, yes. We need to be very careful with what a possible
> >> default case does there, though.
> > 
> > Maybe it would be better to handle level_msr in a way similar to
> > level_leaf: return true/false to notice whether the MSR should be
> > added to the resulting compatible policy?
> > 
> > And then make the default case in level_msr just return false in order
> > to prevent adding MSRs not properly leveled to the policy?
> 
> I'm afraid I'm not clear about the implications. What again is the
> (planned?) final effect of an MSR not getting added there?

Adding the MSR with a 0 value will zero out any previous value on the
'out' policy, while not adding it would leave the previous value
there given the current code in xc_cpu_policy_calc_compatible added by
this patch.

I would expect callers of xc_cpu_policy_calc_compatible to pass a
zeroed 'out' policy, so I think the end result should be the same.

Maybe I should also clear 'out' in xc_cpu_policy_calc_compatible, at
which point both options will get the same exact result.

Thanks, Roger.


^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH v2 15/21] libs/guest: obtain a compatible cpu policy from two input ones
  2021-04-22 11:37                 ` Roger Pau Monné
@ 2021-04-22 11:42                   ` Jan Beulich
  2021-04-22 12:07                     ` Roger Pau Monné
  0 siblings, 1 reply; 43+ messages in thread
From: Jan Beulich @ 2021-04-22 11:42 UTC (permalink / raw)
  To: Roger Pau Monné
  Cc: Andrew Cooper, Ian Jackson, Wei Liu, Anthony PERARD, xen-devel

On 22.04.2021 13:37, Roger Pau Monné wrote:
> On Thu, Apr 22, 2021 at 01:05:23PM +0200, Jan Beulich wrote:
>> On 22.04.2021 12:56, Roger Pau Monné wrote:
>>> On Thu, Apr 22, 2021 at 12:48:36PM +0200, Jan Beulich wrote:
>>>> On 22.04.2021 12:34, Roger Pau Monné wrote:
>>>>> On Thu, Apr 22, 2021 at 11:58:45AM +0200, Jan Beulich wrote:
>>>>>> On 22.04.2021 11:42, Roger Pau Monné wrote:
>>>>>>> On Wed, Apr 14, 2021 at 03:49:02PM +0200, Jan Beulich wrote:
>>>>>>>> On 13.04.2021 16:01, Roger Pau Monne wrote:
>>>>>>>>> @@ -944,3 +945,130 @@ bool xc_cpu_policy_is_compatible(xc_interface *xch, const xc_cpu_policy_t host,
>>>>>>>>>  
>>>>>>>>>      return false;
>>>>>>>>>  }
>>>>>>>>> +
>>>>>>>>> +static uint64_t level_msr(unsigned int index, uint64_t val1, uint64_t val2)
>>>>>>>>> +{
>>>>>>>>> +    uint64_t val = val1 & val2;;
>>>>>>>>
>>>>>>>> For arbitrary MSRs this isn't going to do any good. If only very
>>>>>>>> specific MSRs are assumed to make it here, I think this wants
>>>>>>>> commenting on.
>>>>>>>
>>>>>>> I've added: "MSRs passed to level_msr are expected to be bitmaps of
>>>>>>> features"
>>>>>>
>>>>>> How does such a comment help? I.e. how does the caller tell which MSRs
>>>>>> to pass here and which to deal with anyother way?
>>>>>
>>>>> All MSRs should be passed to level_msr, but it's handling logic would
>>>>> need to be expanded to support MSRs that are not feature bitmaps.
>>>>>
>>>>> It might be best to restore the previous switch and handle each MSR
>>>>> specifically?
>>>>
>>>> I think so, yes. We need to be very careful with what a possible
>>>> default case does there, though.
>>>
>>> Maybe it would be better to handle level_msr in a way similar to
>>> level_leaf: return true/false to notice whether the MSR should be
>>> added to the resulting compatible policy?
>>>
>>> And then make the default case in level_msr just return false in order
>>> to prevent adding MSRs not properly leveled to the policy?
>>
>> I'm afraid I'm not clear about the implications. What again is the
>> (planned?) final effect of an MSR not getting added there?
> 
> Adding the MSR with a 0 value will zero out any previous value on the
> 'out' policy, while not adding it would leave the previous value
> there given the current code in xc_cpu_policy_calc_compatible added by
> this patch.
> 
> I would expect callers of xc_cpu_policy_calc_compatible to pass a
> zeroed 'out' policy, so I think the end result should be the same.

But we're not talking about actual MSR values here, as this is all
about policy. So in the end we'll have to see how things need to
be once we have the first non-feature-flag-like entries there. It
feels as if simply zeroing can't be generally the right thing in
such a case. It may e.g. be that min() is wanted instead.

Jan


^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH v2 15/21] libs/guest: obtain a compatible cpu policy from two input ones
  2021-04-22 11:42                   ` Jan Beulich
@ 2021-04-22 12:07                     ` Roger Pau Monné
  2021-04-22 12:08                       ` Jan Beulich
  0 siblings, 1 reply; 43+ messages in thread
From: Roger Pau Monné @ 2021-04-22 12:07 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Andrew Cooper, Ian Jackson, Wei Liu, Anthony PERARD, xen-devel

On Thu, Apr 22, 2021 at 01:42:45PM +0200, Jan Beulich wrote:
> On 22.04.2021 13:37, Roger Pau Monné wrote:
> > On Thu, Apr 22, 2021 at 01:05:23PM +0200, Jan Beulich wrote:
> >> On 22.04.2021 12:56, Roger Pau Monné wrote:
> >>> On Thu, Apr 22, 2021 at 12:48:36PM +0200, Jan Beulich wrote:
> >>>> On 22.04.2021 12:34, Roger Pau Monné wrote:
> >>>>> On Thu, Apr 22, 2021 at 11:58:45AM +0200, Jan Beulich wrote:
> >>>>>> On 22.04.2021 11:42, Roger Pau Monné wrote:
> >>>>>>> On Wed, Apr 14, 2021 at 03:49:02PM +0200, Jan Beulich wrote:
> >>>>>>>> On 13.04.2021 16:01, Roger Pau Monne wrote:
> >>>>>>>>> @@ -944,3 +945,130 @@ bool xc_cpu_policy_is_compatible(xc_interface *xch, const xc_cpu_policy_t host,
> >>>>>>>>>  
> >>>>>>>>>      return false;
> >>>>>>>>>  }
> >>>>>>>>> +
> >>>>>>>>> +static uint64_t level_msr(unsigned int index, uint64_t val1, uint64_t val2)
> >>>>>>>>> +{
> >>>>>>>>> +    uint64_t val = val1 & val2;;
> >>>>>>>>
> >>>>>>>> For arbitrary MSRs this isn't going to do any good. If only very
> >>>>>>>> specific MSRs are assumed to make it here, I think this wants
> >>>>>>>> commenting on.
> >>>>>>>
> >>>>>>> I've added: "MSRs passed to level_msr are expected to be bitmaps of
> >>>>>>> features"
> >>>>>>
> >>>>>> How does such a comment help? I.e. how does the caller tell which MSRs
> >>>>>> to pass here and which to deal with anyother way?
> >>>>>
> >>>>> All MSRs should be passed to level_msr, but it's handling logic would
> >>>>> need to be expanded to support MSRs that are not feature bitmaps.
> >>>>>
> >>>>> It might be best to restore the previous switch and handle each MSR
> >>>>> specifically?
> >>>>
> >>>> I think so, yes. We need to be very careful with what a possible
> >>>> default case does there, though.
> >>>
> >>> Maybe it would be better to handle level_msr in a way similar to
> >>> level_leaf: return true/false to notice whether the MSR should be
> >>> added to the resulting compatible policy?
> >>>
> >>> And then make the default case in level_msr just return false in order
> >>> to prevent adding MSRs not properly leveled to the policy?
> >>
> >> I'm afraid I'm not clear about the implications. What again is the
> >> (planned?) final effect of an MSR not getting added there?
> > 
> > Adding the MSR with a 0 value will zero out any previous value on the
> > 'out' policy, while not adding it would leave the previous value
> > there given the current code in xc_cpu_policy_calc_compatible added by
> > this patch.
> > 
> > I would expect callers of xc_cpu_policy_calc_compatible to pass a
> > zeroed 'out' policy, so I think the end result should be the same.
> 
> But we're not talking about actual MSR values here, as this is all
> about policy. So in the end we'll have to see how things need to
> be once we have the first non-feature-flag-like entries there. It
> feels as if simply zeroing can't be generally the right thing in
> such a case. It may e.g. be that min() is wanted instead.

Maybe level_msr should return an error for MSRs not explicitly
handled, that's propagated to the caller of
xc_cpu_policy_calc_compatible.

That way addition of new MSRs are not likely to miss adding the
required handling in level_msr?

Thanks, Roger.


^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH v2 15/21] libs/guest: obtain a compatible cpu policy from two input ones
  2021-04-22 12:07                     ` Roger Pau Monné
@ 2021-04-22 12:08                       ` Jan Beulich
  0 siblings, 0 replies; 43+ messages in thread
From: Jan Beulich @ 2021-04-22 12:08 UTC (permalink / raw)
  To: Roger Pau Monné
  Cc: Andrew Cooper, Ian Jackson, Wei Liu, Anthony PERARD, xen-devel

On 22.04.2021 14:07, Roger Pau Monné wrote:
> On Thu, Apr 22, 2021 at 01:42:45PM +0200, Jan Beulich wrote:
>> On 22.04.2021 13:37, Roger Pau Monné wrote:
>>> On Thu, Apr 22, 2021 at 01:05:23PM +0200, Jan Beulich wrote:
>>>> On 22.04.2021 12:56, Roger Pau Monné wrote:
>>>>> On Thu, Apr 22, 2021 at 12:48:36PM +0200, Jan Beulich wrote:
>>>>>> On 22.04.2021 12:34, Roger Pau Monné wrote:
>>>>>>> On Thu, Apr 22, 2021 at 11:58:45AM +0200, Jan Beulich wrote:
>>>>>>>> On 22.04.2021 11:42, Roger Pau Monné wrote:
>>>>>>>>> On Wed, Apr 14, 2021 at 03:49:02PM +0200, Jan Beulich wrote:
>>>>>>>>>> On 13.04.2021 16:01, Roger Pau Monne wrote:
>>>>>>>>>>> @@ -944,3 +945,130 @@ bool xc_cpu_policy_is_compatible(xc_interface *xch, const xc_cpu_policy_t host,
>>>>>>>>>>>  
>>>>>>>>>>>      return false;
>>>>>>>>>>>  }
>>>>>>>>>>> +
>>>>>>>>>>> +static uint64_t level_msr(unsigned int index, uint64_t val1, uint64_t val2)
>>>>>>>>>>> +{
>>>>>>>>>>> +    uint64_t val = val1 & val2;;
>>>>>>>>>>
>>>>>>>>>> For arbitrary MSRs this isn't going to do any good. If only very
>>>>>>>>>> specific MSRs are assumed to make it here, I think this wants
>>>>>>>>>> commenting on.
>>>>>>>>>
>>>>>>>>> I've added: "MSRs passed to level_msr are expected to be bitmaps of
>>>>>>>>> features"
>>>>>>>>
>>>>>>>> How does such a comment help? I.e. how does the caller tell which MSRs
>>>>>>>> to pass here and which to deal with anyother way?
>>>>>>>
>>>>>>> All MSRs should be passed to level_msr, but it's handling logic would
>>>>>>> need to be expanded to support MSRs that are not feature bitmaps.
>>>>>>>
>>>>>>> It might be best to restore the previous switch and handle each MSR
>>>>>>> specifically?
>>>>>>
>>>>>> I think so, yes. We need to be very careful with what a possible
>>>>>> default case does there, though.
>>>>>
>>>>> Maybe it would be better to handle level_msr in a way similar to
>>>>> level_leaf: return true/false to notice whether the MSR should be
>>>>> added to the resulting compatible policy?
>>>>>
>>>>> And then make the default case in level_msr just return false in order
>>>>> to prevent adding MSRs not properly leveled to the policy?
>>>>
>>>> I'm afraid I'm not clear about the implications. What again is the
>>>> (planned?) final effect of an MSR not getting added there?
>>>
>>> Adding the MSR with a 0 value will zero out any previous value on the
>>> 'out' policy, while not adding it would leave the previous value
>>> there given the current code in xc_cpu_policy_calc_compatible added by
>>> this patch.
>>>
>>> I would expect callers of xc_cpu_policy_calc_compatible to pass a
>>> zeroed 'out' policy, so I think the end result should be the same.
>>
>> But we're not talking about actual MSR values here, as this is all
>> about policy. So in the end we'll have to see how things need to
>> be once we have the first non-feature-flag-like entries there. It
>> feels as if simply zeroing can't be generally the right thing in
>> such a case. It may e.g. be that min() is wanted instead.
> 
> Maybe level_msr should return an error for MSRs not explicitly
> handled, that's propagated to the caller of
> xc_cpu_policy_calc_compatible.
> 
> That way addition of new MSRs are not likely to miss adding the
> required handling in level_msr?

Perhaps, yes.

Jan


^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH v2 01/21] libxl: don't ignore the return value from xc_cpuid_apply_policy
  2021-04-13 14:01 ` [PATCH v2 01/21] libxl: don't ignore the return value from xc_cpuid_apply_policy Roger Pau Monne
@ 2021-04-28 15:13   ` Anthony PERARD
  0 siblings, 0 replies; 43+ messages in thread
From: Anthony PERARD @ 2021-04-28 15:13 UTC (permalink / raw)
  To: Roger Pau Monne
  Cc: xen-devel, Andrew Cooper, Ian Jackson, Wei Liu, Jan Beulich

On Tue, Apr 13, 2021 at 04:01:19PM +0200, 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>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> ---
> Changes since 1:
>  - Return ERROR_FAIL on error.
> ---
>  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..539fc4869e6 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");

Is logging `errno` is going to be accurate enough ? The
xc_cpuid_apply_policy() seems to only set `rc` and never change `errno`
directly. It would often return "-errno" or an error code. There's one
instance where we have "rc = -EOPNOTSUPP" but `errno` doesn't seems to
be change to the same value (when checking "featureset).

So maybe having "LOGEV(ERROR, -rc, ...)" would be better?

And it should be `r` instead of `rc`. The latter is for libxl error
code, the former for system call/libxc.

> +
> +    GC_FREE;
> +    return rc ? ERROR_FAIL : 0;

The rest looks good.

Thanks,

-- 
Anthony PERARD


^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH v2 20/21] libs/{light,guest}: implement xc_cpuid_apply_policy in libxl
  2021-04-13 14:01 ` [PATCH v2 20/21] libs/{light,guest}: implement xc_cpuid_apply_policy in libxl Roger Pau Monne
@ 2021-04-28 16:19   ` Anthony PERARD
  0 siblings, 0 replies; 43+ messages in thread
From: Anthony PERARD @ 2021-04-28 16:19 UTC (permalink / raw)
  To: Roger Pau Monne; +Cc: xen-devel, Andrew Cooper, Ian Jackson, Wei Liu

On Tue, Apr 13, 2021 at 04:01:38PM +0200, Roger Pau Monne wrote:
> diff --git a/tools/libs/light/libxl_cpuid.c b/tools/libs/light/libxl_cpuid.c
> index 539fc4869e6..cadc8b2a05e 100644
> --- a/tools/libs/light/libxl_cpuid.c
> +++ b/tools/libs/light/libxl_cpuid.c
> @@ -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);

libxc return values should be stored in 'r'.

> +    if (rc) {
> +        LOGE(ERROR, "Failed to fetch domain %u CPU policy", domid);

It's probably better to use LOGED(ERROR, domid, ...)  to log the domid.
libvirt might be able to use that domid information, but I'm not sure
about that. At least, using LOG*D macros would make logging the domid
consistent with the rest of libxl.

Also use the LOG*D variant for other LOG*, to record the domid
every time.

> +        rc = ERROR_FAIL;
> +        goto out;
> +    }
> +
> +    /*
> +     * Account for feature which have been disabled by default since Xen 4.13,

Why the 's' from "features" has been removed? Also, if it must be
removed then I'm pretty sure that we should also change "have been"
to "has been".


Thanks,

-- 
Anthony PERARD


^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH v2 21/21] libs/guest: (re)move xc_cpu_policy_apply_cpuid
  2021-04-13 14:01 ` [PATCH v2 21/21] libs/guest: (re)move xc_cpu_policy_apply_cpuid Roger Pau Monne
@ 2021-04-28 16:45   ` Anthony PERARD
  0 siblings, 0 replies; 43+ messages in thread
From: Anthony PERARD @ 2021-04-28 16:45 UTC (permalink / raw)
  To: Roger Pau Monne; +Cc: xen-devel, Andrew Cooper, Ian Jackson, Wei Liu

On Tue, Apr 13, 2021 at 04:01:39PM +0200, Roger Pau Monne wrote:
> diff --git a/tools/libs/light/libxl_cpuid.c b/tools/libs/light/libxl_cpuid.c
> index cadc8b2a05e..6be2d773d1d 100644
> --- a/tools/libs/light/libxl_cpuid.c
> +++ b/tools/libs/light/libxl_cpuid.c
> @@ -419,6 +419,136 @@ 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,

I'm pretty sure we want `libxl__gc` here instead of `ctx`. `ctx` will
then by available via `CTX`. (and the GC_* macro will not need to be called)


Beside that, there is also the need to store libxc return values in 'r'
instead 'rc', and maybe use LOG*D macros to log the domid on error
messages.

Thanks,

-- 
Anthony PERARD


^ permalink raw reply	[flat|nested] 43+ messages in thread

end of thread, other threads:[~2021-04-28 16:46 UTC | newest]

Thread overview: 43+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-13 14:01 [PATCH v2 00/21] libs/guest: new CPUID/MSR interface Roger Pau Monne
2021-04-13 14:01 ` [PATCH v2 01/21] libxl: don't ignore the return value from xc_cpuid_apply_policy Roger Pau Monne
2021-04-28 15:13   ` Anthony PERARD
2021-04-13 14:01 ` [PATCH v2 02/21] libs/guest: rename xc_get_cpu_policy_size to xc_cpu_policy_get_size Roger Pau Monne
2021-04-13 14:01 ` [PATCH v2 03/21] libs/guest: introduce xc_cpu_policy_t Roger Pau Monne
2021-04-13 14:01 ` [PATCH v2 04/21] libs/guest: introduce helper to fetch a system cpu policy Roger Pau Monne
2021-04-14 13:28   ` Jan Beulich
2021-04-13 14:01 ` [PATCH v2 05/21] libs/guest: introduce helper to fetch a domain " Roger Pau Monne
2021-04-13 14:01 ` [PATCH v2 06/21] libs/guest: introduce helper to serialize a " Roger Pau Monne
2021-04-13 14:01 ` [PATCH v2 07/21] tools: switch existing users of xc_get_{system,domain}_cpu_policy Roger Pau Monne
2021-04-13 14:01 ` [PATCH v2 08/21] libs/guest: introduce a helper to apply a cpu policy to a domain Roger Pau Monne
2021-04-13 14:01 ` [PATCH v2 09/21] libs/guest: allow fetching a specific CPUID leaf from a cpu policy Roger Pau Monne
2021-04-13 14:01 ` [PATCH v2 10/21] tests/cpu-policy: add sorted MSR test Roger Pau Monne
2021-04-13 14:01 ` [PATCH v2 11/21] libs/guest: allow fetching a specific MSR entry from a cpu policy Roger Pau Monne
2021-04-13 14:01 ` [PATCH v2 12/21] libs/guest: allow updating a cpu policy CPUID data Roger Pau Monne
2021-04-13 14:01 ` [PATCH v2 13/21] libs/guest: allow updating a cpu policy MSR data Roger Pau Monne
2021-04-13 14:01 ` [PATCH v2 14/21] libs/guest: introduce helper to check cpu policy compatibility Roger Pau Monne
2021-04-14 13:36   ` Jan Beulich
2021-04-22  8:22     ` Roger Pau Monné
2021-04-22  8:31       ` Jan Beulich
2021-04-13 14:01 ` [PATCH v2 15/21] libs/guest: obtain a compatible cpu policy from two input ones Roger Pau Monne
2021-04-14 13:49   ` Jan Beulich
2021-04-22  9:42     ` Roger Pau Monné
2021-04-22  9:58       ` Jan Beulich
2021-04-22 10:34         ` Roger Pau Monné
2021-04-22 10:48           ` Jan Beulich
2021-04-22 10:56             ` Roger Pau Monné
2021-04-22 11:05               ` Jan Beulich
2021-04-22 11:37                 ` Roger Pau Monné
2021-04-22 11:42                   ` Jan Beulich
2021-04-22 12:07                     ` Roger Pau Monné
2021-04-22 12:08                       ` Jan Beulich
2021-04-21 10:22   ` Wei Liu
2021-04-21 11:26     ` Roger Pau Monné
2021-04-21 16:42       ` Wei Liu
2021-04-13 14:01 ` [PATCH v2 16/21] libs/guest: make a cpu policy compatible with older Xen versions Roger Pau Monne
2021-04-13 14:01 ` [PATCH v2 17/21] libs/guest: introduce helper set cpu topology in cpu policy Roger Pau Monne
2021-04-13 14:01 ` [PATCH v2 18/21] libs/guest: rework xc_cpuid_xend_policy Roger Pau Monne
2021-04-13 14:01 ` [PATCH v2 19/21] libs/guest: apply a featureset into a cpu policy Roger Pau Monne
2021-04-13 14:01 ` [PATCH v2 20/21] libs/{light,guest}: implement xc_cpuid_apply_policy in libxl Roger Pau Monne
2021-04-28 16:19   ` Anthony PERARD
2021-04-13 14:01 ` [PATCH v2 21/21] libs/guest: (re)move xc_cpu_policy_apply_cpuid Roger Pau Monne
2021-04-28 16:45   ` Anthony PERARD

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).