* [PATCH v6 00/12] libs/guest: new CPUID/MSR interface
@ 2022-01-17 9:48 Roger Pau Monne
2022-01-17 9:48 ` [PATCH v6 01/12] libs/guest: move cpu policy related prototypes to xenguest.h Roger Pau Monne
` (11 more replies)
0 siblings, 12 replies; 24+ messages in thread
From: Roger Pau Monne @ 2022-01-17 9:48 UTC (permalink / raw)
To: xen-devel
Cc: Roger Pau Monne, Wei Liu, Anthony PERARD, Juergen Gross,
Jan Beulich, Andrew Cooper
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.
The addition of a helper to generate compatible policies given two
inputs has been removed from this iteration, sine Andrew Cooper has
posted a patch to set the foundation for that, and further work should
be done against that baseline.
Thanks, Roger.
Jan Beulich (1):
x86/CPUID: shrink max_{,sub}leaf fields according to actual leaf
contents
Roger Pau Monne (11):
libs/guest: move cpu policy related prototypes to xenguest.h
libx86: introduce helper to fetch cpuid leaf
libs/guest: allow fetching a specific CPUID leaf from a cpu policy
libx86: introduce helper to fetch msr entry
libs/guest: allow fetching a specific MSR entry from a cpu policy
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/xenctrl.h | 99 ----
tools/include/xenguest.h | 77 +++
tools/libs/ctrl/xc_bitops.h | 6 +-
tools/libs/guest/xg_cpuid_x86.c | 645 ++++++++---------------
tools/libs/guest/xg_private.h | 1 +
tools/libs/light/libxl_cpuid.c | 233 +++++++-
tools/libs/light/libxl_internal.h | 26 +
tools/misc/xen-cpuid.c | 1 +
tools/tests/cpu-policy/test-cpu-policy.c | 224 +++++++-
xen/arch/x86/cpuid.c | 55 +-
xen/include/xen/lib/x86/cpuid.h | 26 +
xen/include/xen/lib/x86/msr.h | 20 +-
xen/lib/x86/cpuid.c | 91 ++++
xen/lib/x86/msr.c | 41 +-
15 files changed, 946 insertions(+), 605 deletions(-)
--
2.34.1
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v6 01/12] libs/guest: move cpu policy related prototypes to xenguest.h
2022-01-17 9:48 [PATCH v6 00/12] libs/guest: new CPUID/MSR interface Roger Pau Monne
@ 2022-01-17 9:48 ` Roger Pau Monne
2022-01-18 9:55 ` Anthony PERARD
2022-01-17 9:48 ` [PATCH v6 02/12] libx86: introduce helper to fetch cpuid leaf Roger Pau Monne
` (10 subsequent siblings)
11 siblings, 1 reply; 24+ messages in thread
From: Roger Pau Monne @ 2022-01-17 9:48 UTC (permalink / raw)
To: xen-devel
Cc: Roger Pau Monne, Wei Liu, Anthony PERARD, Juergen Gross,
Jan Beulich, Andrew Cooper
Do this before adding any more stuff to xg_cpuid_x86.c.
The placement in xenctrl.h is wrong, as they are implemented by the
xenguest library. Note that xg_cpuid_x86.c needs to include
xg_private.h, and in turn also fix xg_private.h to include
xc_bitops.h. The bitops definition of BITS_PER_LONG needs to be
changed to not be an expression, so that xxhash.h can use it in a
preprocessor if directive.
As a result also modify xen-cpuid and the ocaml stubs to include
xenguest.h.
Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
Changes since v1:
- Include xenguest.h in ocaml stubs.
- Change BITS_PER_LONG definition in xc_bitops.h.
---
tools/include/xenctrl.h | 55 --------------------------------
tools/include/xenguest.h | 56 +++++++++++++++++++++++++++++++++
tools/libs/ctrl/xc_bitops.h | 6 +++-
tools/libs/guest/xg_cpuid_x86.c | 1 -
tools/libs/guest/xg_private.h | 1 +
tools/misc/xen-cpuid.c | 1 +
6 files changed, 63 insertions(+), 57 deletions(-)
diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index 07b96e6671..95bd5eca67 100644
--- a/tools/include/xenctrl.h
+++ b/tools/include/xenctrl.h
@@ -2528,61 +2528,6 @@ int xc_psr_get_domain_data(xc_interface *xch, uint32_t domid,
uint64_t *data);
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);
-
-/* 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_cpu_policy_set_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_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);
-
-/* Compatibility calculations. */
-bool xc_cpu_policy_is_compatible(xc_interface *xch, xc_cpu_policy_t *host,
- 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);
-
-int xc_cpu_policy_get_size(xc_interface *xch, uint32_t *nr_leaves,
- uint32_t *nr_msrs);
-int xc_set_domain_cpu_policy(xc_interface *xch, uint32_t domid,
- uint32_t nr_leaves, xen_cpuid_leaf_t *leaves,
- uint32_t nr_msrs, xen_msr_entry_t *msrs,
- uint32_t *err_leaf_p, uint32_t *err_subleaf_p,
- uint32_t *err_msr_p);
-
-uint32_t xc_get_cpu_featureset_size(void);
-
-enum xc_static_cpu_featuremask {
- XC_FEATUREMASK_KNOWN,
- XC_FEATUREMASK_SPECIAL,
- XC_FEATUREMASK_PV_MAX,
- XC_FEATUREMASK_PV_DEF,
- XC_FEATUREMASK_HVM_SHADOW_MAX,
- XC_FEATUREMASK_HVM_SHADOW_DEF,
- XC_FEATUREMASK_HVM_HAP_MAX,
- XC_FEATUREMASK_HVM_HAP_DEF,
-};
-const uint32_t *xc_get_static_cpu_featuremask(enum xc_static_cpu_featuremask);
-
#endif
int xc_livepatch_upload(xc_interface *xch,
diff --git a/tools/include/xenguest.h b/tools/include/xenguest.h
index 61d0a82f48..e01f494b77 100644
--- a/tools/include/xenguest.h
+++ b/tools/include/xenguest.h
@@ -782,4 +782,60 @@ xen_pfn_t *xc_map_m2p(xc_interface *xch,
unsigned long max_mfn,
int prot,
unsigned long *mfn0);
+
+#if defined(__i386__) || defined(__x86_64__)
+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);
+
+/* 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_cpu_policy_set_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_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);
+
+/* Compatibility calculations. */
+bool xc_cpu_policy_is_compatible(xc_interface *xch, xc_cpu_policy_t *host,
+ 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);
+
+int xc_cpu_policy_get_size(xc_interface *xch, uint32_t *nr_leaves,
+ uint32_t *nr_msrs);
+int xc_set_domain_cpu_policy(xc_interface *xch, uint32_t domid,
+ uint32_t nr_leaves, xen_cpuid_leaf_t *leaves,
+ uint32_t nr_msrs, xen_msr_entry_t *msrs,
+ uint32_t *err_leaf_p, uint32_t *err_subleaf_p,
+ uint32_t *err_msr_p);
+
+uint32_t xc_get_cpu_featureset_size(void);
+
+enum xc_static_cpu_featuremask {
+ XC_FEATUREMASK_KNOWN,
+ XC_FEATUREMASK_SPECIAL,
+ XC_FEATUREMASK_PV_MAX,
+ XC_FEATUREMASK_PV_DEF,
+ XC_FEATUREMASK_HVM_SHADOW_MAX,
+ XC_FEATUREMASK_HVM_SHADOW_DEF,
+ XC_FEATUREMASK_HVM_HAP_MAX,
+ XC_FEATUREMASK_HVM_HAP_DEF,
+};
+const uint32_t *xc_get_static_cpu_featuremask(enum xc_static_cpu_featuremask);
+#endif /* __i386__ || __x86_64__ */
#endif /* XENGUEST_H */
diff --git a/tools/libs/ctrl/xc_bitops.h b/tools/libs/ctrl/xc_bitops.h
index f0bac4a071..4a776dc3a5 100644
--- a/tools/libs/ctrl/xc_bitops.h
+++ b/tools/libs/ctrl/xc_bitops.h
@@ -6,7 +6,11 @@
#include <stdlib.h>
#include <string.h>
-#define BITS_PER_LONG (sizeof(unsigned long) * 8)
+#ifdef __LP64__
+#define BITS_PER_LONG 64
+#else
+#define BITS_PER_LONG 32
+#endif
#define BITMAP_ENTRY(_nr,_bmap) ((_bmap))[(_nr) / 8]
#define BITMAP_SHIFT(_nr) ((_nr) % 8)
diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
index 198892ebdf..b9e827ce7e 100644
--- a/tools/libs/guest/xg_cpuid_x86.c
+++ b/tools/libs/guest/xg_cpuid_x86.c
@@ -23,7 +23,6 @@
#include <stdbool.h>
#include <limits.h>
#include "xg_private.h"
-#include "xc_bitops.h"
#include <xen/hvm/params.h>
#include <xen-tools/libs.h>
diff --git a/tools/libs/guest/xg_private.h b/tools/libs/guest/xg_private.h
index 28441ee13f..09e24f1227 100644
--- a/tools/libs/guest/xg_private.h
+++ b/tools/libs/guest/xg_private.h
@@ -27,6 +27,7 @@
#include <sys/stat.h>
#include "xc_private.h"
+#include "xc_bitops.h"
#include "xenguest.h"
#include <xen/memory.h>
diff --git a/tools/misc/xen-cpuid.c b/tools/misc/xen-cpuid.c
index fb36cac07b..a3003245f1 100644
--- a/tools/misc/xen-cpuid.c
+++ b/tools/misc/xen-cpuid.c
@@ -8,6 +8,7 @@
#include <inttypes.h>
#include <xenctrl.h>
+#include <xenguest.h>
#include <xen-tools/libs.h>
--
2.34.1
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v6 02/12] libx86: introduce helper to fetch cpuid leaf
2022-01-17 9:48 [PATCH v6 00/12] libs/guest: new CPUID/MSR interface Roger Pau Monne
2022-01-17 9:48 ` [PATCH v6 01/12] libs/guest: move cpu policy related prototypes to xenguest.h Roger Pau Monne
@ 2022-01-17 9:48 ` Roger Pau Monne
2022-01-18 12:26 ` Andrew Cooper
2022-01-17 9:48 ` [PATCH v6 03/12] libs/guest: allow fetching a specific CPUID leaf from a cpu policy Roger Pau Monne
` (9 subsequent siblings)
11 siblings, 1 reply; 24+ messages in thread
From: Roger Pau Monne @ 2022-01-17 9:48 UTC (permalink / raw)
To: xen-devel
Cc: Roger Pau Monne, Jan Beulich, Andrew Cooper, Wei Liu, Anthony PERARD
Introduce a helper based on the current Xen guest_cpuid code in order
to fetch a cpuid leaf from a policy. The newly introduced function in
cpuid.c should not be directly called and instead the provided
x86_cpuid_get_leaf macro should be used that will properly deal with
const and non-const inputs.
Also add a test to check that the introduced helper doesn't go over
the bounds of the policy.
Note the code in x86_cpuid_copy_from_buffer is not switched to use the
new function because of the boundary checks against the max fields of
the policy, which might not be properly set at the point where
x86_cpuid_copy_from_buffer get called, for example when filling an
empty policy from scratch.
Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
Changes since v4:
- Rename _x86_cpuid_get_leaf to x86_cpuid_get_leaf_const.
Changes since v3:
- New in this version.
---
Regarding safety of the usage of array_access_nospec to obtain a
pointer to an element of an array, there are already other instances
of this usage, for example in viridian_time_wrmsr, so I would assume
this is fine.
---
tools/tests/cpu-policy/test-cpu-policy.c | 75 ++++++++++++++++++++++++
xen/arch/x86/cpuid.c | 55 +++--------------
xen/include/xen/lib/x86/cpuid.h | 19 ++++++
xen/lib/x86/cpuid.c | 52 ++++++++++++++++
4 files changed, 153 insertions(+), 48 deletions(-)
diff --git a/tools/tests/cpu-policy/test-cpu-policy.c b/tools/tests/cpu-policy/test-cpu-policy.c
index ed450a0997..3f777fc1fc 100644
--- a/tools/tests/cpu-policy/test-cpu-policy.c
+++ b/tools/tests/cpu-policy/test-cpu-policy.c
@@ -570,6 +570,80 @@ static void test_cpuid_out_of_range_clearing(void)
}
}
+static void test_cpuid_get_leaf_failure(void)
+{
+ static const struct test {
+ struct cpuid_policy p;
+ const char *name;
+ uint32_t leaf, subleaf;
+ } tests[] = {
+ /* Bound checking logic. */
+ {
+ .name = "Basic max leaf >= array size",
+ .p = {
+ .basic.max_leaf = CPUID_GUEST_NR_BASIC,
+ },
+ },
+ {
+ .name = "Feature max leaf >= array size",
+ .p = {
+ .basic.max_leaf = CPUID_GUEST_NR_BASIC - 1,
+ .feat.max_subleaf = CPUID_GUEST_NR_FEAT,
+ },
+ .leaf = 0x00000007,
+ },
+ {
+ .name = "Extended max leaf >= array size",
+ .p = {
+ .extd.max_leaf = 0x80000000 + CPUID_GUEST_NR_EXTD,
+ },
+ .leaf = 0x80000000,
+ },
+
+ {
+ .name = "Basic leaf >= max leaf",
+ .p = {
+ .basic.max_leaf = CPUID_GUEST_NR_BASIC - 1,
+ },
+ .leaf = CPUID_GUEST_NR_BASIC,
+ },
+ {
+ .name = "Feature leaf >= max leaf",
+ .p = {
+ .basic.max_leaf = CPUID_GUEST_NR_BASIC - 1,
+ .feat.max_subleaf = CPUID_GUEST_NR_FEAT - 1,
+ },
+ .leaf = 0x00000007,
+ .subleaf = CPUID_GUEST_NR_FEAT,
+ },
+ {
+ .name = "Extended leaf >= max leaf",
+ .p = {
+ .extd.max_leaf = 0x80000000 + CPUID_GUEST_NR_EXTD - 1,
+ },
+ .leaf = 0x80000000 + CPUID_GUEST_NR_EXTD,
+ },
+ };
+ const struct cpuid_policy pc;
+ const struct cpuid_leaf *lc;
+ struct cpuid_policy p;
+ struct cpuid_leaf *l;
+
+ /* Constness build test. */
+ lc = x86_cpuid_get_leaf(&pc, 0, 0);
+ l = x86_cpuid_get_leaf(&p, 0, 0);
+
+ printf("Testing CPUID get leaf bound checking:\n");
+
+ for ( size_t i = 0; i < ARRAY_SIZE(tests); ++i )
+ {
+ const struct test *t = &tests[i];
+
+ if ( x86_cpuid_get_leaf(&t->p, t->leaf, t->subleaf) )
+ fail(" Test %s get leaf fail\n", t->name);
+ }
+}
+
static void test_is_compatible_success(void)
{
static struct test {
@@ -685,6 +759,7 @@ int main(int argc, char **argv)
test_cpuid_serialise_success();
test_cpuid_deserialise_failure();
test_cpuid_out_of_range_clearing();
+ test_cpuid_get_leaf_failure();
test_msr_serialise_success();
test_msr_deserialise_failure();
diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
index b5af48324a..0407a54626 100644
--- a/xen/arch/x86/cpuid.c
+++ b/xen/arch/x86/cpuid.c
@@ -825,48 +825,16 @@ void guest_cpuid(const struct vcpu *v, uint32_t leaf,
switch ( leaf )
{
case 0 ... CPUID_GUEST_NR_BASIC - 1:
- ASSERT(p->basic.max_leaf < ARRAY_SIZE(p->basic.raw));
- if ( leaf > min_t(uint32_t, p->basic.max_leaf,
- ARRAY_SIZE(p->basic.raw) - 1) )
- return;
-
- switch ( leaf )
- {
- case 0x4:
- if ( subleaf >= ARRAY_SIZE(p->cache.raw) )
- return;
-
- *res = array_access_nospec(p->cache.raw, subleaf);
- break;
-
- case 0x7:
- ASSERT(p->feat.max_subleaf < ARRAY_SIZE(p->feat.raw));
- if ( subleaf > min_t(uint32_t, p->feat.max_subleaf,
- ARRAY_SIZE(p->feat.raw) - 1) )
- return;
-
- *res = array_access_nospec(p->feat.raw, subleaf);
- break;
-
- case 0xb:
- if ( subleaf >= ARRAY_SIZE(p->topo.raw) )
- return;
-
- *res = array_access_nospec(p->topo.raw, subleaf);
- break;
-
- case XSTATE_CPUID:
- if ( !p->basic.xsave || subleaf >= ARRAY_SIZE(p->xstate.raw) )
- return;
+ case 0x80000000 ... 0x80000000 + CPUID_GUEST_NR_EXTD - 1:
+ {
+ const struct cpuid_leaf *tmp = x86_cpuid_get_leaf(p, leaf, subleaf);
- *res = array_access_nospec(p->xstate.raw, subleaf);
- break;
+ if ( !tmp )
+ return;
- default:
- *res = array_access_nospec(p->basic.raw, leaf);
- break;
- }
+ *res = *tmp;
break;
+ }
case 0x40000000 ... 0x400000ff:
if ( is_viridian_domain(d) )
@@ -881,15 +849,6 @@ void guest_cpuid(const struct vcpu *v, uint32_t leaf,
case 0x40000100 ... 0x400001ff:
return cpuid_hypervisor_leaves(v, leaf, subleaf, res);
- case 0x80000000 ... 0x80000000 + CPUID_GUEST_NR_EXTD - 1:
- ASSERT((p->extd.max_leaf & 0xffff) < ARRAY_SIZE(p->extd.raw));
- if ( (leaf & 0xffff) > min_t(uint32_t, p->extd.max_leaf & 0xffff,
- ARRAY_SIZE(p->extd.raw) - 1) )
- return;
-
- *res = array_access_nospec(p->extd.raw, leaf & 0xffff);
- break;
-
default:
return;
}
diff --git a/xen/include/xen/lib/x86/cpuid.h b/xen/include/xen/lib/x86/cpuid.h
index a4d254ea96..050cd4f9d1 100644
--- a/xen/include/xen/lib/x86/cpuid.h
+++ b/xen/include/xen/lib/x86/cpuid.h
@@ -431,6 +431,25 @@ int x86_cpuid_copy_from_buffer(struct cpuid_policy *policy,
uint32_t nr_entries, uint32_t *err_leaf,
uint32_t *err_subleaf);
+/**
+ * Get a cpuid leaf from a policy object.
+ *
+ * @param policy The cpuid_policy object.
+ * @param leaf The leaf index.
+ * @param subleaf The subleaf index.
+ * @returns a pointer to the requested leaf or NULL in case of error.
+ *
+ * The function will perform out of bound checks. Do not call this function
+ * directly and instead use x86_cpuid_get_leaf that will deal with both const
+ * and non-const policies returning a pointer with constness matching that of
+ * the input.
+ */
+const struct cpuid_leaf *x86_cpuid_get_leaf_const(const struct cpuid_policy *p,
+ uint32_t leaf,
+ uint32_t subleaf);
+#define x86_cpuid_get_leaf(p, l, s) \
+ ((__typeof__(&(p)->basic.raw[0]))x86_cpuid_get_leaf_const(p, l, s))
+
#endif /* !XEN_LIB_X86_CPUID_H */
/*
diff --git a/xen/lib/x86/cpuid.c b/xen/lib/x86/cpuid.c
index 8eb88314f5..924f882fc4 100644
--- a/xen/lib/x86/cpuid.c
+++ b/xen/lib/x86/cpuid.c
@@ -493,6 +493,58 @@ int x86_cpuid_copy_from_buffer(struct cpuid_policy *p,
return -ERANGE;
}
+const struct cpuid_leaf *x86_cpuid_get_leaf_const(const struct cpuid_policy *p,
+ uint32_t leaf,
+ uint32_t subleaf)
+{
+ switch ( leaf )
+ {
+ case 0 ... CPUID_GUEST_NR_BASIC - 1:
+ if ( p->basic.max_leaf >= ARRAY_SIZE(p->basic.raw) ||
+ leaf > p->basic.max_leaf )
+ return NULL;
+
+ switch ( leaf )
+ {
+ case 0x4:
+ if ( subleaf >= ARRAY_SIZE(p->cache.raw) )
+ return NULL;
+
+ return &array_access_nospec(p->cache.raw, subleaf);
+
+ case 0x7:
+ if ( p->feat.max_subleaf >= ARRAY_SIZE(p->feat.raw) ||
+ subleaf > p->feat.max_subleaf )
+ return NULL;
+
+ return &array_access_nospec(p->feat.raw, subleaf);
+
+ case 0xb:
+ if ( subleaf >= ARRAY_SIZE(p->topo.raw) )
+ return NULL;
+
+ return &array_access_nospec(p->topo.raw, subleaf);
+
+ case 0xd:
+ if ( !p->basic.xsave || subleaf >= ARRAY_SIZE(p->xstate.raw) )
+ return NULL;
+
+ return &array_access_nospec(p->xstate.raw, subleaf);
+ }
+
+ return &array_access_nospec(p->basic.raw, leaf);
+
+ case 0x80000000 ... 0x80000000 + CPUID_GUEST_NR_EXTD - 1:
+ if ( (p->extd.max_leaf & 0xffff) >= ARRAY_SIZE(p->extd.raw) ||
+ leaf > p->extd.max_leaf )
+ return NULL;
+
+ return &array_access_nospec(p->extd.raw, leaf & 0xffff);
+ }
+
+ return NULL;
+}
+
/*
* Local variables:
* mode: C
--
2.34.1
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v6 03/12] libs/guest: allow fetching a specific CPUID leaf from a cpu policy
2022-01-17 9:48 [PATCH v6 00/12] libs/guest: new CPUID/MSR interface Roger Pau Monne
2022-01-17 9:48 ` [PATCH v6 01/12] libs/guest: move cpu policy related prototypes to xenguest.h Roger Pau Monne
2022-01-17 9:48 ` [PATCH v6 02/12] libx86: introduce helper to fetch cpuid leaf Roger Pau Monne
@ 2022-01-17 9:48 ` Roger Pau Monne
2022-01-17 12:28 ` Jan Beulich
2022-01-17 9:48 ` [PATCH v6 04/12] libx86: introduce helper to fetch msr entry Roger Pau Monne
` (8 subsequent siblings)
11 siblings, 1 reply; 24+ messages in thread
From: Roger Pau Monne @ 2022-01-17 9:48 UTC (permalink / raw)
To: xen-devel; +Cc: Roger Pau Monne, Wei Liu, Anthony PERARD, Juergen Gross
Introduce an interface that returns a specific leaf/subleaf from a cpu
policy in xen_cpuid_leaf_t format.
This is useful to callers can peek data from the opaque
xc_cpu_policy_t type.
No caller of the interface introduced on this patch.
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v5:
- Zero out parameter.
Changes since v3:
- Use x86_cpuid_get_leaf.
Changes since v1:
- Use find leaf.
---
tools/include/xenguest.h | 3 +++
tools/libs/guest/xg_cpuid_x86.c | 25 +++++++++++++++++++++++++
2 files changed, 28 insertions(+)
diff --git a/tools/include/xenguest.h b/tools/include/xenguest.h
index e01f494b77..0a6fd99306 100644
--- a/tools/include/xenguest.h
+++ b/tools/include/xenguest.h
@@ -807,6 +807,9 @@ int xc_cpu_policy_update_cpuid(xc_interface *xch, xc_cpu_policy_t *policy,
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_cpu_policy_get_cpuid(xc_interface *xch, const xc_cpu_policy_t *policy,
+ uint32_t leaf, uint32_t subleaf,
+ xen_cpuid_leaf_t *out);
/* Compatibility calculations. */
bool xc_cpu_policy_is_compatible(xc_interface *xch, xc_cpu_policy_t *host,
diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
index b9e827ce7e..aff4efe78d 100644
--- a/tools/libs/guest/xg_cpuid_x86.c
+++ b/tools/libs/guest/xg_cpuid_x86.c
@@ -855,6 +855,31 @@ int xc_cpu_policy_update_msrs(xc_interface *xch, xc_cpu_policy_t *policy,
return rc;
}
+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)
+{
+ const struct cpuid_leaf *tmp;
+
+ *out = (xen_cpuid_leaf_t){};
+
+ tmp = x86_cpuid_get_leaf(&policy->cpuid, leaf, subleaf);
+ if ( !tmp )
+ {
+ /* Unable to find a matching leaf. */
+ errno = ENOENT;
+ return -1;
+ }
+
+ out->leaf = leaf;
+ out->subleaf = subleaf;
+ out->a = tmp->a;
+ out->b = tmp->b;
+ out->c = tmp->c;
+ out->d = tmp->d;
+ return 0;
+}
+
bool xc_cpu_policy_is_compatible(xc_interface *xch, xc_cpu_policy_t *host,
xc_cpu_policy_t *guest)
{
--
2.34.1
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v6 04/12] libx86: introduce helper to fetch msr entry
2022-01-17 9:48 [PATCH v6 00/12] libs/guest: new CPUID/MSR interface Roger Pau Monne
` (2 preceding siblings ...)
2022-01-17 9:48 ` [PATCH v6 03/12] libs/guest: allow fetching a specific CPUID leaf from a cpu policy Roger Pau Monne
@ 2022-01-17 9:48 ` Roger Pau Monne
2022-01-17 9:48 ` [PATCH v6 05/12] libs/guest: allow fetching a specific MSR entry from a cpu policy Roger Pau Monne
` (7 subsequent siblings)
11 siblings, 0 replies; 24+ messages in thread
From: Roger Pau Monne @ 2022-01-17 9:48 UTC (permalink / raw)
To: xen-devel
Cc: Roger Pau Monne, Jan Beulich, Andrew Cooper, Wei Liu, Anthony PERARD
Use such helper in order to replace the code in
x86_msr_copy_from_buffer. Note the introduced helper should not be
directly called and instead x86_msr_get_entry should be used that will
properly deal with const and non-const inputs.
Note this requires making the raw fields uint64_t so that it can
accommodate the maximum size of MSRs values, and in turn removing the
truncation tests.
Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v4:
- Rename _x86_msr_get_entry to x86_msr_get_entry_const.
- Add newline before endif.
Changes since v3:
- New in this version.
---
tools/tests/cpu-policy/test-cpu-policy.c | 48 +++++++++++++++++++-----
xen/include/xen/lib/x86/msr.h | 20 +++++++++-
xen/lib/x86/msr.c | 41 ++++++++++----------
3 files changed, 76 insertions(+), 33 deletions(-)
diff --git a/tools/tests/cpu-policy/test-cpu-policy.c b/tools/tests/cpu-policy/test-cpu-policy.c
index 3f777fc1fc..686d7a886c 100644
--- a/tools/tests/cpu-policy/test-cpu-policy.c
+++ b/tools/tests/cpu-policy/test-cpu-policy.c
@@ -386,16 +386,6 @@ static void test_msr_deserialise_failure(void)
.msr = { .idx = 0xce, .flags = 1 },
.rc = -EINVAL,
},
- {
- .name = "truncated val",
- .msr = { .idx = 0xce, .val = ~0ull },
- .rc = -EOVERFLOW,
- },
- {
- .name = "truncated val",
- .msr = { .idx = 0x10a, .val = ~0ull },
- .rc = -EOVERFLOW,
- },
};
printf("Testing MSR deserialise failure:\n");
@@ -644,6 +634,43 @@ static void test_cpuid_get_leaf_failure(void)
}
}
+static void test_msr_get_entry(void)
+{
+ static const struct test {
+ const char *name;
+ unsigned int idx;
+ bool success;
+ } tests[] = {
+ {
+ .name = "bad msr index",
+ .idx = -1,
+ },
+ {
+ .name = "good msr index",
+ .idx = 0xce,
+ .success = true,
+ },
+ };
+ const struct msr_policy pc;
+ const uint64_t *ec;
+ struct msr_policy p;
+ uint64_t *e;
+
+ /* Constness build test. */
+ ec = x86_msr_get_entry(&pc, 0);
+ e = x86_msr_get_entry(&p, 0);
+
+ printf("Testing MSR get leaf:\n");
+
+ for ( size_t i = 0; i < ARRAY_SIZE(tests); ++i )
+ {
+ const struct test *t = &tests[i];
+
+ if ( !!x86_msr_get_entry(&pc, t->idx) != t->success )
+ fail(" Test %s failed\n", t->name);
+ }
+}
+
static void test_is_compatible_success(void)
{
static struct test {
@@ -763,6 +790,7 @@ int main(int argc, char **argv)
test_msr_serialise_success();
test_msr_deserialise_failure();
+ test_msr_get_entry();
test_is_compatible_success();
test_is_compatible_failure();
diff --git a/xen/include/xen/lib/x86/msr.h b/xen/include/xen/lib/x86/msr.h
index 48ba4a59c0..4d84b7cf27 100644
--- a/xen/include/xen/lib/x86/msr.h
+++ b/xen/include/xen/lib/x86/msr.h
@@ -17,7 +17,7 @@ struct msr_policy
* is dependent on real hardware support.
*/
union {
- uint32_t raw;
+ uint64_t raw;
struct {
uint32_t :31;
bool cpuid_faulting:1;
@@ -32,7 +32,7 @@ struct msr_policy
* fixed in hardware.
*/
union {
- uint32_t raw;
+ uint64_t raw;
struct {
bool rdcl_no:1;
bool ibrs_all:1;
@@ -91,6 +91,22 @@ int x86_msr_copy_from_buffer(struct msr_policy *policy,
const msr_entry_buffer_t msrs, uint32_t nr_entries,
uint32_t *err_msr);
+/**
+ * Get a MSR entry from a policy object.
+ *
+ * @param policy The msr_policy object.
+ * @param idx The index.
+ * @returns a pointer to the requested leaf or NULL in case of error.
+ *
+ * Do not call this function directly and instead use x86_msr_get_entry that
+ * will deal with both const and non-const policies returning a pointer with
+ * constness matching that of the input.
+ */
+const uint64_t *x86_msr_get_entry_const(const struct msr_policy *policy,
+ uint32_t idx);
+#define x86_msr_get_entry(p, i) \
+ ((__typeof__(&(p)->platform_info.raw))x86_msr_get_entry_const(p, i))
+
#endif /* !XEN_LIB_X86_MSR_H */
/*
diff --git a/xen/lib/x86/msr.c b/xen/lib/x86/msr.c
index 7d71e92a38..e9b337dd70 100644
--- a/xen/lib/x86/msr.c
+++ b/xen/lib/x86/msr.c
@@ -74,6 +74,8 @@ int x86_msr_copy_from_buffer(struct msr_policy *p,
for ( i = 0; i < nr_entries; i++ )
{
+ uint64_t *val;
+
if ( copy_from_buffer_offset(&data, msrs, i, 1) )
return -EFAULT;
@@ -83,31 +85,13 @@ int x86_msr_copy_from_buffer(struct msr_policy *p,
goto err;
}
- switch ( data.idx )
+ val = x86_msr_get_entry(p, data.idx);
+ if ( !val )
{
- /*
- * Assign data.val to p->field, checking for truncation if the
- * backing storage for field is smaller than uint64_t
- */
-#define ASSIGN(field) \
-({ \
- if ( (typeof(p->field))data.val != data.val ) \
- { \
- rc = -EOVERFLOW; \
- goto err; \
- } \
- p->field = data.val; \
-})
-
- case MSR_INTEL_PLATFORM_INFO: ASSIGN(platform_info.raw); break;
- case MSR_ARCH_CAPABILITIES: ASSIGN(arch_caps.raw); break;
-
-#undef ASSIGN
-
- default:
rc = -ERANGE;
goto err;
}
+ *val = data.val;
}
return 0;
@@ -119,6 +103,21 @@ int x86_msr_copy_from_buffer(struct msr_policy *p,
return rc;
}
+const uint64_t *x86_msr_get_entry_const(const struct msr_policy *policy,
+ uint32_t idx)
+{
+ switch ( idx )
+ {
+ case MSR_INTEL_PLATFORM_INFO:
+ return &policy->platform_info.raw;
+
+ case MSR_ARCH_CAPABILITIES:
+ return &policy->arch_caps.raw;
+ }
+
+ return NULL;
+}
+
/*
* Local variables:
* mode: C
--
2.34.1
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v6 05/12] libs/guest: allow fetching a specific MSR entry from a cpu policy
2022-01-17 9:48 [PATCH v6 00/12] libs/guest: new CPUID/MSR interface Roger Pau Monne
` (3 preceding siblings ...)
2022-01-17 9:48 ` [PATCH v6 04/12] libx86: introduce helper to fetch msr entry Roger Pau Monne
@ 2022-01-17 9:48 ` Roger Pau Monne
2022-01-17 12:29 ` Jan Beulich
2022-01-17 9:48 ` [PATCH v6 06/12] libs/guest: make a cpu policy compatible with older Xen versions Roger Pau Monne
` (6 subsequent siblings)
11 siblings, 1 reply; 24+ messages in thread
From: Roger Pau Monne @ 2022-01-17 9:48 UTC (permalink / raw)
To: xen-devel; +Cc: Roger Pau Monne, Wei Liu, Anthony PERARD, Juergen Gross
Introduce an interface that returns a specific MSR entry from a cpu
policy in xen_msr_entry_t format.
This is useful to callers can peek data from the opaque
xc_cpu_policy_t type.
No caller of the interface introduced on this patch.
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v3:
- Use x86_msr_get_entry.
Changes since v1:
- Introduce a helper to perform a binary search of the MSR entries
array.
---
tools/include/xenguest.h | 2 ++
tools/libs/guest/xg_cpuid_x86.c | 20 ++++++++++++++++++++
2 files changed, 22 insertions(+)
diff --git a/tools/include/xenguest.h b/tools/include/xenguest.h
index 0a6fd99306..2672fd043c 100644
--- a/tools/include/xenguest.h
+++ b/tools/include/xenguest.h
@@ -810,6 +810,8 @@ int xc_cpu_policy_update_msrs(xc_interface *xch, 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);
/* Compatibility calculations. */
bool xc_cpu_policy_is_compatible(xc_interface *xch, xc_cpu_policy_t *host,
diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
index aff4efe78d..67981b1711 100644
--- a/tools/libs/guest/xg_cpuid_x86.c
+++ b/tools/libs/guest/xg_cpuid_x86.c
@@ -880,6 +880,26 @@ int xc_cpu_policy_get_cpuid(xc_interface *xch, const xc_cpu_policy_t *policy,
return 0;
}
+int xc_cpu_policy_get_msr(xc_interface *xch, const xc_cpu_policy_t *policy,
+ uint32_t msr, xen_msr_entry_t *out)
+{
+ const uint64_t *val;
+
+ *out = (xen_msr_entry_t){};
+
+ val = x86_msr_get_entry(&policy->msr, msr);
+ if ( !val )
+ {
+ errno = ENOENT;
+ return -1;
+ }
+
+ out->idx = msr;
+ out->val = *val;
+
+ return 0;
+}
+
bool xc_cpu_policy_is_compatible(xc_interface *xch, xc_cpu_policy_t *host,
xc_cpu_policy_t *guest)
{
--
2.34.1
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v6 06/12] libs/guest: make a cpu policy compatible with older Xen versions
2022-01-17 9:48 [PATCH v6 00/12] libs/guest: new CPUID/MSR interface Roger Pau Monne
` (4 preceding siblings ...)
2022-01-17 9:48 ` [PATCH v6 05/12] libs/guest: allow fetching a specific MSR entry from a cpu policy Roger Pau Monne
@ 2022-01-17 9:48 ` Roger Pau Monne
2022-01-17 9:48 ` [PATCH v6 07/12] libs/guest: introduce helper set cpu topology in cpu policy Roger Pau Monne
` (5 subsequent siblings)
11 siblings, 0 replies; 24+ messages in thread
From: Roger Pau Monne @ 2022-01-17 9:48 UTC (permalink / raw)
To: xen-devel; +Cc: Roger Pau Monne, Wei Liu, Anthony PERARD, Juergen Gross
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 v3:
- Rename function to xc_cpu_policy_make_compat_4_12.
Changes since v1:
- Move comments and explicitly mention pre-4.13 Xen.
---
tools/include/xenguest.h | 4 +++
tools/libs/guest/xg_cpuid_x86.c | 62 ++++++++++++++++++++++++---------
2 files changed, 49 insertions(+), 17 deletions(-)
diff --git a/tools/include/xenguest.h b/tools/include/xenguest.h
index 2672fd043c..281454dc60 100644
--- a/tools/include/xenguest.h
+++ b/tools/include/xenguest.h
@@ -817,6 +817,10 @@ int xc_cpu_policy_get_msr(xc_interface *xch, const xc_cpu_policy_t *policy,
bool xc_cpu_policy_is_compatible(xc_interface *xch, xc_cpu_policy_t *host,
xc_cpu_policy_t *guest);
+/* Make a policy compatible with pre-4.13 Xen versions. */
+int xc_cpu_policy_make_compat_4_12(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 67981b1711..f1115ad480 100644
--- a/tools/libs/guest/xg_cpuid_x86.c
+++ b/tools/libs/guest/xg_cpuid_x86.c
@@ -432,6 +432,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);
@@ -496,23 +497,9 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
if ( restore )
{
- /*
- * Account for feature which have been disabled by default since Xen 4.13,
- * so migrated-in VM's don't risk seeing features disappearing.
- */
- p->basic.rdrand = test_bit(X86_FEATURE_RDRAND, host_featureset);
- p->feat.hle = test_bit(X86_FEATURE_HLE, host_featureset);
- p->feat.rtm = test_bit(X86_FEATURE_RTM, host_featureset);
-
- if ( di.hvm )
- {
- p->feat.mpx = test_bit(X86_FEATURE_MPX, host_featureset);
- }
-
- /* Clamp maximum leaves to the ones supported on 4.12. */
- p->basic.max_leaf = min(p->basic.max_leaf, 0xdu);
- p->feat.max_subleaf = 0;
- p->extd.max_leaf = min(p->extd.max_leaf, 0x8000001c);
+ policy.cpuid = *p;
+ xc_cpu_policy_make_compat_4_12(xch, &policy, di.hvm);
+ *p = policy.cpuid;
}
if ( featureset )
@@ -918,3 +905,44 @@ bool xc_cpu_policy_is_compatible(xc_interface *xch, xc_cpu_policy_t *host,
return false;
}
+
+int xc_cpu_policy_make_compat_4_12(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;
+ policy->cpuid.feat.hle = host->cpuid.feat.hle;
+ policy->cpuid.feat.rtm = host->cpuid.feat.rtm;
+
+ if ( hvm )
+ policy->cpuid.feat.mpx = host->cpuid.feat.mpx;
+
+ /* Clamp maximum leaves to the ones supported on pre-4.13. */
+ policy->cpuid.basic.max_leaf = min(policy->cpuid.basic.max_leaf, 0xdu);
+ policy->cpuid.feat.max_subleaf = 0;
+ policy->cpuid.extd.max_leaf = min(policy->cpuid.extd.max_leaf, 0x8000001c);
+
+ out:
+ xc_cpu_policy_destroy(host);
+ return rc;
+}
--
2.34.1
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v6 07/12] libs/guest: introduce helper set cpu topology in cpu policy
2022-01-17 9:48 [PATCH v6 00/12] libs/guest: new CPUID/MSR interface Roger Pau Monne
` (5 preceding siblings ...)
2022-01-17 9:48 ` [PATCH v6 06/12] libs/guest: make a cpu policy compatible with older Xen versions Roger Pau Monne
@ 2022-01-17 9:48 ` Roger Pau Monne
2022-01-17 9:48 ` [PATCH v6 08/12] libs/guest: rework xc_cpuid_xend_policy Roger Pau Monne
` (4 subsequent siblings)
11 siblings, 0 replies; 24+ messages in thread
From: Roger Pau Monne @ 2022-01-17 9:48 UTC (permalink / raw)
To: xen-devel; +Cc: Roger Pau Monne, Wei Liu, Anthony PERARD, Juergen Gross
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>
---
Changes since v5:
- Keep using the host featureset.
- Fix copied comment typo.
Changes since v4:
- s/xc_cpu_policy_topology/xc_cpu_policy_legacy_topology/
---
tools/include/xenguest.h | 4 +
tools/libs/guest/xg_cpuid_x86.c | 182 +++++++++++++++++---------------
2 files changed, 101 insertions(+), 85 deletions(-)
diff --git a/tools/include/xenguest.h b/tools/include/xenguest.h
index 281454dc60..bea02cb542 100644
--- a/tools/include/xenguest.h
+++ b/tools/include/xenguest.h
@@ -821,6 +821,10 @@ bool xc_cpu_policy_is_compatible(xc_interface *xch, xc_cpu_policy_t *host,
int xc_cpu_policy_make_compat_4_12(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 f1115ad480..e7ae133d8d 100644
--- a/tools/libs/guest/xg_cpuid_x86.c
+++ b/tools/libs/guest/xg_cpuid_x86.c
@@ -429,13 +429,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 )
@@ -458,22 +456,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
@@ -557,72 +539,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 )
- p->basic.lppp = 2;
- else 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 )
@@ -946,3 +867,94 @@ int xc_cpu_policy_make_compat_4_12(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 )
+ {
+ uint32_t host_featureset[FEATURESET_NR_ENTRIES] = {};
+ uint32_t len = ARRAY_SIZE(host_featureset);
+ int rc;
+
+ /* Get the host policy. */
+ rc = xc_get_cpu_featureset(xch, XEN_SYSCTL_cpu_featureset_host,
+ &len, host_featureset);
+ if ( rc && errno != ENOBUFS )
+ {
+ /* Tolerate "buffer too small", as we've got the bits we need. */
+ ERROR("Failed to obtain host featureset");
+ 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 = test_bit(X86_FEATURE_HTT, host_featureset);
+ policy->cpuid.extd.cmp_legacy = test_bit(X86_FEATURE_CMP_LEGACY,
+ host_featureset);
+ }
+ 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 )
+ policy->cpuid.basic.lppp = 2;
+ else 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: In 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.34.1
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v6 08/12] libs/guest: rework xc_cpuid_xend_policy
2022-01-17 9:48 [PATCH v6 00/12] libs/guest: new CPUID/MSR interface Roger Pau Monne
` (6 preceding siblings ...)
2022-01-17 9:48 ` [PATCH v6 07/12] libs/guest: introduce helper set cpu topology in cpu policy Roger Pau Monne
@ 2022-01-17 9:48 ` Roger Pau Monne
2022-01-18 14:06 ` Andrew Cooper
2022-01-17 9:48 ` [PATCH v6 09/12] libs/guest: apply a featureset into a cpu policy Roger Pau Monne
` (3 subsequent siblings)
11 siblings, 1 reply; 24+ messages in thread
From: Roger Pau Monne @ 2022-01-17 9:48 UTC (permalink / raw)
To: xen-devel; +Cc: Roger Pau Monne, Wei Liu, Anthony PERARD, Juergen Gross
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.
The find_leaf helper and related comparison function is also removed,
as it's no longer needed to search for cpuid leafs as finding the
matching leaves is now done using xc_cpu_policy_get_cpuid.
No functional change intended.
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v3:
- Drop find_leaf and comparison helper.
---
tools/include/xenguest.h | 4 +
tools/libs/guest/xg_cpuid_x86.c | 200 +++++++++++++-------------------
2 files changed, 83 insertions(+), 121 deletions(-)
diff --git a/tools/include/xenguest.h b/tools/include/xenguest.h
index bea02cb542..9912116a51 100644
--- a/tools/include/xenguest.h
+++ b/tools/include/xenguest.h
@@ -825,6 +825,10 @@ int xc_cpu_policy_make_compat_4_12(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 e7ae133d8d..9060a2f763 100644
--- a/tools/libs/guest/xg_cpuid_x86.c
+++ b/tools/libs/guest/xg_cpuid_x86.c
@@ -254,144 +254,107 @@ int xc_set_domain_cpu_policy(xc_interface *xch, uint32_t domid,
return ret;
}
-static int compare_leaves(const void *l, const void *r)
-{
- const xen_cpuid_leaf_t *lhs = l;
- const xen_cpuid_leaf_t *rhs = r;
-
- if ( lhs->leaf != rhs->leaf )
- return lhs->leaf < rhs->leaf ? -1 : 1;
-
- if ( lhs->subleaf != rhs->subleaf )
- return lhs->subleaf < rhs->subleaf ? -1 : 1;
-
- return 0;
-}
-
-static xen_cpuid_leaf_t *find_leaf(
- xen_cpuid_leaf_t *leaves, unsigned int nr_leaves,
- const struct xc_xend_cpuid *xend)
-{
- const xen_cpuid_leaf_t key = { xend->leaf, xend->subleaf };
-
- return bsearch(&key, leaves, nr_leaves, sizeof(*leaves), compare_leaves);
-}
-
-static int xc_cpuid_xend_policy(
- xc_interface *xch, uint32_t domid, const struct xc_xend_cpuid *xend)
+int xc_cpu_policy_apply_cpuid(xc_interface *xch, xc_cpu_policy_t *policy,
+ const struct xc_xend_cpuid *cpuid, bool hvm)
{
int rc;
- xc_dominfo_t di;
- unsigned int nr_leaves, nr_msrs;
- uint32_t err_leaf = -1, err_subleaf = -1, err_msr = -1;
- /*
- * Three full policies. The host, 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;
+ xc_cpu_policy_t *host = NULL, *def = NULL;
- if ( xc_domain_getinfo(xch, domid, 1, &di) != 1 ||
- di.domid != domid )
- {
- ERROR("Failed to obtain d%d info", domid);
- rc = -ESRCH;
- goto fail;
- }
-
- rc = xc_cpu_policy_get_size(xch, &nr_leaves, &nr_msrs);
- if ( rc )
- {
- PERROR("Failed to obtain policy info size");
- rc = -errno;
- goto fail;
- }
-
- rc = -ENOMEM;
- if ( (host = calloc(nr_leaves, sizeof(*host))) == NULL ||
- (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);
- 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;
+ xen_cpuid_leaf_t def_leaf;
+ xen_cpuid_leaf_t host_leaf;
- if ( cur_leaf == NULL || def_leaf == NULL || host_leaf == NULL )
+ rc = xc_cpu_policy_get_cpuid(xch, policy, cpuid->leaf, cpuid->subleaf,
+ &cur_leaf);
+ if ( rc )
+ {
+ ERROR("Failed to get current policy leaf %#x subleaf %#x",
+ cpuid->leaf, cpuid->subleaf);
+ goto out;
+ }
+ rc = xc_cpu_policy_get_cpuid(xch, 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("Missing leaf %#x, subleaf %#x", xend->leaf, xend->subleaf);
- goto fail;
+ 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);
@@ -399,25 +362,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;
}
@@ -425,7 +382,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;
@@ -545,6 +502,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 )
{
@@ -562,9 +523,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.34.1
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v6 09/12] libs/guest: apply a featureset into a cpu policy
2022-01-17 9:48 [PATCH v6 00/12] libs/guest: new CPUID/MSR interface Roger Pau Monne
` (7 preceding siblings ...)
2022-01-17 9:48 ` [PATCH v6 08/12] libs/guest: rework xc_cpuid_xend_policy Roger Pau Monne
@ 2022-01-17 9:48 ` Roger Pau Monne
2022-01-17 9:48 ` [PATCH v6 10/12] libs/{light,guest}: implement xc_cpuid_apply_policy in libxl Roger Pau Monne
` (2 subsequent siblings)
11 siblings, 0 replies; 24+ messages in thread
From: Roger Pau Monne @ 2022-01-17 9:48 UTC (permalink / raw)
To: xen-devel
Cc: Roger Pau Monne, Wei Liu, Anthony PERARD, Juergen Gross, Jan Beulich
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>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
tools/include/xenguest.h | 5 ++
tools/libs/guest/xg_cpuid_x86.c | 95 ++++++++++++++++++++-------------
2 files changed, 62 insertions(+), 38 deletions(-)
diff --git a/tools/include/xenguest.h b/tools/include/xenguest.h
index 9912116a51..8f05d8aa66 100644
--- a/tools/include/xenguest.h
+++ b/tools/include/xenguest.h
@@ -829,6 +829,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 9060a2f763..cf202671ed 100644
--- a/tools/libs/guest/xg_cpuid_x86.c
+++ b/tools/libs/guest/xg_cpuid_x86.c
@@ -443,46 +443,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
{
@@ -916,3 +885,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.34.1
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v6 10/12] libs/{light,guest}: implement xc_cpuid_apply_policy in libxl
2022-01-17 9:48 [PATCH v6 00/12] libs/guest: new CPUID/MSR interface Roger Pau Monne
` (8 preceding siblings ...)
2022-01-17 9:48 ` [PATCH v6 09/12] libs/guest: apply a featureset into a cpu policy Roger Pau Monne
@ 2022-01-17 9:48 ` Roger Pau Monne
2022-01-18 13:37 ` Andrew Cooper
2022-01-17 9:48 ` [PATCH v6 11/12] libs/guest: (re)move xc_cpu_policy_apply_cpuid Roger Pau Monne
2022-01-17 9:48 ` [PATCH v6 12/12] x86/CPUID: shrink max_{,sub}leaf fields according to actual leaf contents Roger Pau Monne
11 siblings, 1 reply; 24+ messages in thread
From: Roger Pau Monne @ 2022-01-17 9:48 UTC (permalink / raw)
To: xen-devel; +Cc: Roger Pau Monne, Wei Liu, Anthony PERARD, Juergen Gross
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>
Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>
---
Changes since v4:
- Correctly account for PVH guests being HVM in libxl__cpuid_legacy.
- PAE option is only available to HVM guests (_not_ including PVH).
Changes since v2:
- Use 'r' for libxc return values.
- Fix comment about making a cpu policy compatible.
- Use LOG*D macros.
---
tools/include/xenctrl.h | 18 -----
tools/libs/guest/xg_cpuid_x86.c | 122 --------------------------------
tools/libs/light/libxl_cpuid.c | 92 ++++++++++++++++++++++--
3 files changed, 86 insertions(+), 146 deletions(-)
diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index 95bd5eca67..745d67c970 100644
--- a/tools/include/xenctrl.h
+++ b/tools/include/xenctrl.h
@@ -1829,24 +1829,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 cf202671ed..974549c0db 100644
--- a/tools/libs/guest/xg_cpuid_x86.c
+++ b/tools/libs/guest/xg_cpuid_x86.c
@@ -379,128 +379,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;
- }
-
- if ( restore )
- {
- policy.cpuid = *p;
- xc_cpu_policy_make_compat_4_12(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 e1acf6648d..7dcdb35a4c 100644
--- a/tools/libs/light/libxl_cpuid.c
+++ b/tools/libs/light/libxl_cpuid.c
@@ -441,9 +441,11 @@ 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_PV;
bool pae = true;
bool itsc;
- int r;
+ int r, rc = 0;
/*
* Gross hack. Using libxl_defbool_val() here causes libvirt to crash in
@@ -454,6 +456,41 @@ 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) {
+ LOGED(ERROR, domid, "Failed to init CPU policy");
+ rc = ERROR_FAIL;
+ goto out;
+ }
+
+ r = xc_cpu_policy_get_domain(ctx->xch, domid, policy);
+ if (r) {
+ LOGED(ERROR, domid, "Failed to fetch domain CPU policy");
+ rc = ERROR_FAIL;
+ goto out;
+ }
+
+ if (restore) {
+ /*
+ * Make sure the policy is compatible with pre Xen 4.13. Note that
+ * newer Xen versions will pass policy data on the restore stream, so
+ * any adjustments done here will be superseded.
+ */
+ r = xc_cpu_policy_make_compat_4_12(ctx->xch, policy, hvm);
+ if (r) {
+ LOGED(ERROR, domid, "Failed to setup compatible CPU policy");
+ rc = ERROR_FAIL;
+ goto out;
+ }
+ }
+
+ r = xc_cpu_policy_legacy_topology(ctx->xch, policy, hvm);
+ if (r) {
+ LOGED(ERROR, domid, "Failed to setup CPU policy topology");
+ 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
@@ -466,6 +503,13 @@ int libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid, bool restore,
*/
if (info->type == LIBXL_DOMAIN_TYPE_HVM)
pae = libxl_defbool_val(info->u.hvm.pae);
+ rc = libxl_cpuid_parse_config(&info->cpuid, GCSPRINTF("pae=%d", pae));
+ if (rc) {
+ LOGD(ERROR, domid, "Failed to set PAE CPUID flag");
+ rc = ERROR_FAIL;
+ goto out;
+ }
+
/*
* Advertising Invariant TSC to a guest means that the TSC frequency won't
@@ -481,14 +525,50 @@ 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) {
+ LOGD(ERROR, domid, "Failed to set Invariant TSC CPUID flag");
+ rc = ERROR_FAIL;
+ goto out;
+ }
- r = xc_cpuid_apply_policy(ctx->xch, domid, restore, NULL, 0,
- pae, itsc, nested_virt, info->cpuid);
- if (r)
- LOGEVD(ERROR, -r, domid, "Failed to apply CPUID policy");
+ /* Set Nested virt CPUID bits for HVM. */
+ if (hvm) {
+ rc = libxl_cpuid_parse_config(&info->cpuid, GCSPRINTF("vmx=%d",
+ nested_virt));
+ if (rc) {
+ LOGD(ERROR, domid, "Failed to set VMX CPUID flag");
+ rc = ERROR_FAIL;
+ goto out;
+ }
+
+ rc = libxl_cpuid_parse_config(&info->cpuid, GCSPRINTF("svm=%d",
+ nested_virt));
+ if (rc) {
+ LOGD(ERROR, domid, "Failed to set SVM CPUID flag");
+ rc = ERROR_FAIL;
+ goto out;
+ }
+ }
+
+ /* Apply the bits from info->cpuid if any. */
+ r = xc_cpu_policy_apply_cpuid(ctx->xch, policy, info->cpuid, hvm);
+ if (r) {
+ LOGEVD(ERROR, domid, -r, "Failed to apply CPUID changes");
+ rc = ERROR_FAIL;
+ goto out;
+ }
+
+ r = xc_cpu_policy_set_domain(ctx->xch, domid, policy);
+ if (r) {
+ LOGED(ERROR, domid, "Failed to set domain CPUID policy");
+ rc = ERROR_FAIL;
+ }
+ out:
+ xc_cpu_policy_destroy(policy);
GC_FREE;
- return r ? ERROR_FAIL : 0;
+ return rc;
}
static const char *input_names[2] = { "leaf", "subleaf" };
--
2.34.1
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v6 11/12] libs/guest: (re)move xc_cpu_policy_apply_cpuid
2022-01-17 9:48 [PATCH v6 00/12] libs/guest: new CPUID/MSR interface Roger Pau Monne
` (9 preceding siblings ...)
2022-01-17 9:48 ` [PATCH v6 10/12] libs/{light,guest}: implement xc_cpuid_apply_policy in libxl Roger Pau Monne
@ 2022-01-17 9:48 ` Roger Pau Monne
2022-01-17 9:48 ` [PATCH v6 12/12] x86/CPUID: shrink max_{,sub}leaf fields according to actual leaf contents Roger Pau Monne
11 siblings, 0 replies; 24+ messages in thread
From: Roger Pau Monne @ 2022-01-17 9:48 UTC (permalink / raw)
To: xen-devel; +Cc: Roger Pau Monne, Wei Liu, Anthony PERARD, Juergen Gross
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>
Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>
---
Changes since v2:
- Use LOG*D.
- Pass a gc to apply_policy.
- Use 'r' for libxc return values.
---
tools/include/libxl.h | 6 +-
tools/include/xenctrl.h | 26 ------
tools/include/xenguest.h | 4 -
tools/libs/guest/xg_cpuid_x86.c | 125 --------------------------
tools/libs/light/libxl_cpuid.c | 142 ++++++++++++++++++++++++++++--
tools/libs/light/libxl_internal.h | 26 ++++++
6 files changed, 165 insertions(+), 164 deletions(-)
diff --git a/tools/include/libxl.h b/tools/include/libxl.h
index 2bbbd21f0b..8a8032ba25 100644
--- a/tools/include/libxl.h
+++ b/tools/include/libxl.h
@@ -1420,10 +1420,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 745d67c970..79169f8ace 100644
--- a/tools/include/xenctrl.h
+++ b/tools/include/xenctrl.h
@@ -1803,32 +1803,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);
diff --git a/tools/include/xenguest.h b/tools/include/xenguest.h
index 8f05d8aa66..3462d27516 100644
--- a/tools/include/xenguest.h
+++ b/tools/include/xenguest.h
@@ -825,10 +825,6 @@ int xc_cpu_policy_make_compat_4_12(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 974549c0db..8bc7bd7224 100644
--- a/tools/libs/guest/xg_cpuid_x86.c
+++ b/tools/libs/guest/xg_cpuid_x86.c
@@ -254,131 +254,6 @@ int xc_set_domain_cpu_policy(xc_interface *xch, uint32_t domid,
return ret;
}
-int xc_cpu_policy_apply_cpuid(xc_interface *xch, xc_cpu_policy_t *policy,
- const struct xc_xend_cpuid *cpuid, bool hvm)
-{
- int rc;
- xc_cpu_policy_t *host = NULL, *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 7dcdb35a4c..556e8f41a7 100644
--- a/tools/libs/light/libxl_cpuid.c
+++ b/tools/libs/light/libxl_cpuid.c
@@ -309,7 +309,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;
@@ -387,7 +387,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);
@@ -437,6 +437,137 @@ int libxl_cpuid_parse_config_xend(libxl_cpuid_policy_list *cpuid,
return 0;
}
+static int apply_cpuid(libxl__gc *gc, xc_cpu_policy_t *policy,
+ libxl_cpuid_policy_list cpuid, bool hvm, domid_t domid)
+{
+ int r, rc = 0;
+ xc_cpu_policy_t *host = NULL, *def = NULL;
+
+ host = xc_cpu_policy_init();
+ def = xc_cpu_policy_init();
+ if (!host || !def) {
+ LOGD(ERROR, domid, "Failed to init policies");
+ rc = ERROR_FAIL;
+ goto out;
+ }
+
+ /* Get the domain type's default policy. */
+ r = xc_cpu_policy_get_system(CTX->xch,
+ hvm ? XEN_SYSCTL_cpu_policy_hvm_default
+ : XEN_SYSCTL_cpu_policy_pv_default,
+ def);
+ if (r) {
+ LOGED(ERROR, domid, "Failed to obtain %s def policy",
+ hvm ? "hvm" : "pv");
+ rc = ERROR_FAIL;
+ goto out;
+ }
+
+ /* Get the host policy. */
+ r = xc_cpu_policy_get_system(CTX->xch, XEN_SYSCTL_cpu_policy_host, host);
+ if (r) {
+ LOGED(ERROR, domid, "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;
+
+ r = xc_cpu_policy_get_cpuid(CTX->xch, policy, cpuid->leaf,
+ cpuid->subleaf, &cur_leaf);
+ if (r) {
+ LOGED(ERROR, domid,
+ "Failed to get current policy leaf %#x subleaf %#x",
+ cpuid->leaf, cpuid->subleaf);
+ r = ERROR_FAIL;
+ goto out;
+ }
+ r = xc_cpu_policy_get_cpuid(CTX->xch, def, cpuid->leaf, cpuid->subleaf,
+ &def_leaf);
+ if (r) {
+ LOGED(ERROR, domid,
+ "Failed to get def policy leaf %#x subleaf %#x",
+ cpuid->leaf, cpuid->subleaf);
+ rc = ERROR_FAIL;
+ goto out;
+ }
+ r = xc_cpu_policy_get_cpuid(CTX->xch, host, cpuid->leaf,
+ cpuid->subleaf, &host_leaf);
+ if (r) {
+ LOGED(ERROR, domid,
+ "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:
+ LOGD(ERROR, domid,
+ "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
+ }
+
+ r = xc_cpu_policy_update_cpuid(CTX->xch, policy, &cur_leaf, 1);
+ if (r) {
+ LOGED(ERROR, domid, "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);
+ return rc;
+}
+
int libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid, bool restore,
libxl_domain_build_info *info)
{
@@ -552,10 +683,9 @@ int libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid, bool restore,
}
/* Apply the bits from info->cpuid if any. */
- r = xc_cpu_policy_apply_cpuid(ctx->xch, policy, info->cpuid, hvm);
- if (r) {
- LOGEVD(ERROR, domid, -r, "Failed to apply CPUID changes");
- rc = ERROR_FAIL;
+ rc = apply_cpuid(gc, policy, info->cpuid, hvm, domid);
+ if (rc) {
+ LOGD(ERROR, domid, "Failed to apply CPUID changes");
goto out;
}
diff --git a/tools/libs/light/libxl_internal.h b/tools/libs/light/libxl_internal.h
index 37d5c27756..c99b271f9c 100644
--- a/tools/libs/light/libxl_internal.h
+++ b/tools/libs/light/libxl_internal.h
@@ -2059,6 +2059,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.34.1
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v6 12/12] x86/CPUID: shrink max_{,sub}leaf fields according to actual leaf contents
2022-01-17 9:48 [PATCH v6 00/12] libs/guest: new CPUID/MSR interface Roger Pau Monne
` (10 preceding siblings ...)
2022-01-17 9:48 ` [PATCH v6 11/12] libs/guest: (re)move xc_cpu_policy_apply_cpuid Roger Pau Monne
@ 2022-01-17 9:48 ` Roger Pau Monne
11 siblings, 0 replies; 24+ messages in thread
From: Roger Pau Monne @ 2022-01-17 9:48 UTC (permalink / raw)
To: xen-devel
Cc: Jan Beulich, Wei Liu, Anthony PERARD, Juergen Gross,
Andrew Cooper, Roger Pau Monné
From: Jan Beulich <jbeulich@suse.com>
Zapping leaf data for out of range leaves is just one half of it: To
avoid guests (bogusly or worse) inferring information from mere leaf
presence, also shrink maximum indicators such that the respective
trailing entry is not all blank (unless of course it's the initial
subleaf of a leaf that's not the final one).
This is also in preparation of bumping the maximum basic leaf we
support, to ensure guests not getting exposed related features won't
observe a change in behavior.
Note that such shrinking is only done when creating a policy for a
domain from scratch. Migrated in domains keep their previous policy if
present untouched, and for migrated in domains not having CPUID data
the crafted Xen pre-4.13 policy is not trimmed to keep a behavior
compatible with those older Xen versions.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v4:
- New in this version, picked up from 540d911c2813.
- Only shrink policies for newly created domains.
---
tools/include/xenguest.h | 3 +
tools/libs/guest/xg_cpuid_x86.c | 5 ++
tools/libs/light/libxl_cpuid.c | 7 ++
tools/tests/cpu-policy/test-cpu-policy.c | 101 +++++++++++++++++++++++
xen/include/xen/lib/x86/cpuid.h | 7 ++
xen/lib/x86/cpuid.c | 39 +++++++++
6 files changed, 162 insertions(+)
diff --git a/tools/include/xenguest.h b/tools/include/xenguest.h
index 3462d27516..e8b0d3ff16 100644
--- a/tools/include/xenguest.h
+++ b/tools/include/xenguest.h
@@ -830,6 +830,9 @@ int xc_cpu_policy_apply_featureset(xc_interface *xch, xc_cpu_policy_t *policy,
const uint32_t *featureset,
unsigned int nr_features);
+/* Sanitize a policy: can change the contents of the passed policy. */
+void xc_cpu_policy_sanitize(xc_interface *xch, 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 8bc7bd7224..c630dd8a73 100644
--- a/tools/libs/guest/xg_cpuid_x86.c
+++ b/tools/libs/guest/xg_cpuid_x86.c
@@ -688,3 +688,8 @@ int xc_cpu_policy_apply_featureset(xc_interface *xch, xc_cpu_policy_t *policy,
return 0;
}
+
+void xc_cpu_policy_sanitize(xc_interface *xch, xc_cpu_policy_t *policy)
+{
+ x86_cpuid_policy_shrink_max_leaves(&policy->cpuid);
+}
diff --git a/tools/libs/light/libxl_cpuid.c b/tools/libs/light/libxl_cpuid.c
index 556e8f41a7..c7294680d4 100644
--- a/tools/libs/light/libxl_cpuid.c
+++ b/tools/libs/light/libxl_cpuid.c
@@ -689,6 +689,13 @@ int libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid, bool restore,
goto out;
}
+ /*
+ * Do not attempt any modifications if creating a policy that aims to be
+ * compatible with pre-4.13 Xen versions.
+ */
+ if (!restore)
+ xc_cpu_policy_sanitize(ctx->xch, policy);
+
r = xc_cpu_policy_set_domain(ctx->xch, domid, policy);
if (r) {
LOGED(ERROR, domid, "Failed to set domain CPUID policy");
diff --git a/tools/tests/cpu-policy/test-cpu-policy.c b/tools/tests/cpu-policy/test-cpu-policy.c
index 686d7a886c..20419a6108 100644
--- a/tools/tests/cpu-policy/test-cpu-policy.c
+++ b/tools/tests/cpu-policy/test-cpu-policy.c
@@ -8,10 +8,13 @@
#include <err.h>
#include <xen-tools/libs.h>
+#include <xen/asm/x86-defns.h>
#include <xen/asm/x86-vendors.h>
#include <xen/lib/x86/cpu-policy.h>
#include <xen/domctl.h>
+#define XSTATE_FP_SSE (X86_XCR0_FP | X86_XCR0_SSE)
+
static unsigned int nr_failures;
#define fail(fmt, ...) \
({ \
@@ -671,6 +674,103 @@ static void test_msr_get_entry(void)
}
}
+static void test_cpuid_maximum_leaf_shrinking(void)
+{
+ static const struct test {
+ const char *name;
+ struct cpuid_policy p;
+ } tests[] = {
+ {
+ .name = "basic",
+ .p = {
+ /* Very basic information only. */
+ .basic.max_leaf = 1,
+ .basic.raw_fms = 0xc2,
+ },
+ },
+ {
+ .name = "cache",
+ .p = {
+ /* Cache subleaves present. */
+ .basic.max_leaf = 4,
+ .cache.subleaf[0].type = 1,
+ },
+ },
+ {
+ .name = "feat#0",
+ .p = {
+ /* Subleaf 0 only with some valid bit. */
+ .basic.max_leaf = 7,
+ .feat.max_subleaf = 0,
+ .feat.fsgsbase = 1,
+ },
+ },
+ {
+ .name = "feat#1",
+ .p = {
+ /* Subleaf 1 only with some valid bit. */
+ .basic.max_leaf = 7,
+ .feat.max_subleaf = 1,
+ .feat.avx_vnni = 1,
+ },
+ },
+ {
+ .name = "topo",
+ .p = {
+ /* Topology subleaves present. */
+ .basic.max_leaf = 0xb,
+ .topo.subleaf[0].type = 1,
+ },
+ },
+ {
+ .name = "xstate",
+ .p = {
+ /* First subleaf always valid (and then non-zero). */
+ .basic.max_leaf = 0xd,
+ .xstate.xcr0_low = XSTATE_FP_SSE,
+ },
+ },
+ {
+ .name = "extd",
+ .p = {
+ /* Commonly available information only. */
+ .extd.max_leaf = 0x80000008,
+ .extd.maxphysaddr = 0x28,
+ .extd.maxlinaddr = 0x30,
+ },
+ },
+ };
+
+ printf("Testing CPUID maximum leaf shrinking:\n");
+
+ for ( size_t i = 0; i < ARRAY_SIZE(tests); ++i )
+ {
+ const struct test *t = &tests[i];
+ struct cpuid_policy *p = memdup(&t->p);
+
+ p->basic.max_leaf = ARRAY_SIZE(p->basic.raw) - 1;
+ p->feat.max_subleaf = ARRAY_SIZE(p->feat.raw) - 1;
+ p->extd.max_leaf = 0x80000000 | (ARRAY_SIZE(p->extd.raw) - 1);
+
+ x86_cpuid_policy_shrink_max_leaves(p);
+
+ /* Check the the resulting max (sub)leaf values against expecations. */
+ if ( p->basic.max_leaf != t->p.basic.max_leaf )
+ fail(" Test %s basic fail - expected %#x, got %#x\n",
+ t->name, t->p.basic.max_leaf, p->basic.max_leaf);
+
+ if ( p->extd.max_leaf != t->p.extd.max_leaf )
+ fail(" Test %s extd fail - expected %#x, got %#x\n",
+ t->name, t->p.extd.max_leaf, p->extd.max_leaf);
+
+ if ( p->feat.max_subleaf != t->p.feat.max_subleaf )
+ fail(" Test %s feat fail - expected %#x, got %#x\n",
+ t->name, t->p.feat.max_subleaf, p->feat.max_subleaf);
+
+ free(p);
+ }
+}
+
static void test_is_compatible_success(void)
{
static struct test {
@@ -787,6 +887,7 @@ int main(int argc, char **argv)
test_cpuid_deserialise_failure();
test_cpuid_out_of_range_clearing();
test_cpuid_get_leaf_failure();
+ test_cpuid_maximum_leaf_shrinking();
test_msr_serialise_success();
test_msr_deserialise_failure();
diff --git a/xen/include/xen/lib/x86/cpuid.h b/xen/include/xen/lib/x86/cpuid.h
index 050cd4f9d1..86cda38986 100644
--- a/xen/include/xen/lib/x86/cpuid.h
+++ b/xen/include/xen/lib/x86/cpuid.h
@@ -386,6 +386,13 @@ void x86_cpuid_policy_fill_native(struct cpuid_policy *p);
*/
void x86_cpuid_policy_clear_out_of_range_leaves(struct cpuid_policy *p);
+/**
+ * Shrink max leaf/subleaf values such that the last respective valid entry
+ * isn't all blank. While permitted by the spec, such extraneous leaves may
+ * provide undue "hints" to guests.
+ */
+void x86_cpuid_policy_shrink_max_leaves(struct cpuid_policy *p);
+
#ifdef __XEN__
#include <public/arch-x86/xen.h>
typedef XEN_GUEST_HANDLE_64(xen_cpuid_leaf_t) cpuid_leaf_buffer_t;
diff --git a/xen/lib/x86/cpuid.c b/xen/lib/x86/cpuid.c
index 924f882fc4..6a943cd91b 100644
--- a/xen/lib/x86/cpuid.c
+++ b/xen/lib/x86/cpuid.c
@@ -236,6 +236,45 @@ void x86_cpuid_policy_clear_out_of_range_leaves(struct cpuid_policy *p)
ARRAY_SIZE(p->extd.raw) - 1);
}
+void x86_cpuid_policy_shrink_max_leaves(struct cpuid_policy *p)
+{
+ unsigned int i;
+
+ p->basic.raw[0x4] = p->cache.raw[0];
+
+ for ( i = p->feat.max_subleaf; i; --i )
+ if ( p->feat.raw[i].a | p->feat.raw[i].b |
+ p->feat.raw[i].c | p->feat.raw[i].d )
+ break;
+ p->feat.max_subleaf = i;
+ p->basic.raw[0x7] = p->feat.raw[i];
+
+ p->basic.raw[0xb] = p->topo.raw[0];
+
+ /*
+ * Due to the way xstate gets handled in the hypervisor (see
+ * recalculate_xstate()) there is (for now at least) no need to fiddle
+ * with the xstate subleaves (IOW we assume they're already in consistent
+ * shape, for coming from either hardware or recalculate_xstate()).
+ */
+ p->basic.raw[0xd] = p->xstate.raw[0];
+
+ for ( i = p->basic.max_leaf; i; --i )
+ if ( p->basic.raw[i].a | p->basic.raw[i].b |
+ p->basic.raw[i].c | p->basic.raw[i].d )
+ break;
+ p->basic.max_leaf = i;
+
+ for ( i = p->extd.max_leaf & 0xffff; i; --i )
+ if ( p->extd.raw[i].a | p->extd.raw[i].b |
+ p->extd.raw[i].c | p->extd.raw[i].d )
+ break;
+ if ( i | p->extd.raw[0].b | p->extd.raw[0].c | p->extd.raw[0].d )
+ p->extd.max_leaf = 0x80000000 | i;
+ else
+ p->extd.max_leaf = 0;
+}
+
const uint32_t *x86_cpuid_lookup_deep_deps(uint32_t feature)
{
static const uint32_t deep_features[] = INIT_DEEP_FEATURES;
--
2.34.1
^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH v6 03/12] libs/guest: allow fetching a specific CPUID leaf from a cpu policy
2022-01-17 9:48 ` [PATCH v6 03/12] libs/guest: allow fetching a specific CPUID leaf from a cpu policy Roger Pau Monne
@ 2022-01-17 12:28 ` Jan Beulich
0 siblings, 0 replies; 24+ messages in thread
From: Jan Beulich @ 2022-01-17 12:28 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: Wei Liu, Anthony PERARD, Juergen Gross, xen-devel
On 17.01.2022 10:48, Roger Pau Monne wrote:
> Introduce an interface that returns a specific leaf/subleaf from a cpu
> policy in xen_cpuid_leaf_t format.
>
> This is useful to callers can peek data from the opaque
> xc_cpu_policy_t type.
>
> No caller of the interface introduced on this patch.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
with a nit:
> --- a/tools/libs/guest/xg_cpuid_x86.c
> +++ b/tools/libs/guest/xg_cpuid_x86.c
> @@ -855,6 +855,31 @@ int xc_cpu_policy_update_msrs(xc_interface *xch, xc_cpu_policy_t *policy,
> return rc;
> }
>
> +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)
> +{
> + const struct cpuid_leaf *tmp;
> +
> + *out = (xen_cpuid_leaf_t){};
> +
> + tmp = x86_cpuid_get_leaf(&policy->cpuid, leaf, subleaf);
> + if ( !tmp )
> + {
> + /* Unable to find a matching leaf. */
> + errno = ENOENT;
> + return -1;
> + }
> +
> + out->leaf = leaf;
> + out->subleaf = subleaf;
> + out->a = tmp->a;
> + out->b = tmp->b;
> + out->c = tmp->c;
> + out->d = tmp->d;
> + return 0;
> +}
Hypervisor style, which aiui also applies to libxc, would prefer a blank
line before the main "return" of a function.
Jan
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v6 05/12] libs/guest: allow fetching a specific MSR entry from a cpu policy
2022-01-17 9:48 ` [PATCH v6 05/12] libs/guest: allow fetching a specific MSR entry from a cpu policy Roger Pau Monne
@ 2022-01-17 12:29 ` Jan Beulich
0 siblings, 0 replies; 24+ messages in thread
From: Jan Beulich @ 2022-01-17 12:29 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: Wei Liu, Anthony PERARD, Juergen Gross, xen-devel
On 17.01.2022 10:48, Roger Pau Monne wrote:
> Introduce an interface that returns a specific MSR entry from a cpu
> policy in xen_msr_entry_t format.
>
> This is useful to callers can peek data from the opaque
> xc_cpu_policy_t type.
>
> No caller of the interface introduced on this patch.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v6 01/12] libs/guest: move cpu policy related prototypes to xenguest.h
2022-01-17 9:48 ` [PATCH v6 01/12] libs/guest: move cpu policy related prototypes to xenguest.h Roger Pau Monne
@ 2022-01-18 9:55 ` Anthony PERARD
2022-01-18 10:50 ` Roger Pau Monné
0 siblings, 1 reply; 24+ messages in thread
From: Anthony PERARD @ 2022-01-18 9:55 UTC (permalink / raw)
To: Roger Pau Monne
Cc: xen-devel, Wei Liu, Juergen Gross, Jan Beulich, Andrew Cooper
On Mon, Jan 17, 2022 at 10:48:16AM +0100, Roger Pau Monne wrote:
> Do this before adding any more stuff to xg_cpuid_x86.c.
>
> The placement in xenctrl.h is wrong, as they are implemented by the
> xenguest library. Note that xg_cpuid_x86.c needs to include
> xg_private.h, and in turn also fix xg_private.h to include
> xc_bitops.h. The bitops definition of BITS_PER_LONG needs to be
> changed to not be an expression, so that xxhash.h can use it in a
> preprocessor if directive.
>
> As a result also modify xen-cpuid and the ocaml stubs to include
> xenguest.h.
Adding xenguest.h to ocaml stub has been done so it isn't part of this
patch anymore ;-).
> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Anthony PERARD <anthony.perard@citrix.com>
Thanks,
--
Anthony PERARD
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v6 01/12] libs/guest: move cpu policy related prototypes to xenguest.h
2022-01-18 9:55 ` Anthony PERARD
@ 2022-01-18 10:50 ` Roger Pau Monné
0 siblings, 0 replies; 24+ messages in thread
From: Roger Pau Monné @ 2022-01-18 10:50 UTC (permalink / raw)
To: Anthony PERARD
Cc: xen-devel, Wei Liu, Juergen Gross, Jan Beulich, Andrew Cooper
On Tue, Jan 18, 2022 at 09:55:45AM +0000, Anthony PERARD wrote:
> On Mon, Jan 17, 2022 at 10:48:16AM +0100, Roger Pau Monne wrote:
> > Do this before adding any more stuff to xg_cpuid_x86.c.
> >
> > The placement in xenctrl.h is wrong, as they are implemented by the
> > xenguest library. Note that xg_cpuid_x86.c needs to include
> > xg_private.h, and in turn also fix xg_private.h to include
> > xc_bitops.h. The bitops definition of BITS_PER_LONG needs to be
> > changed to not be an expression, so that xxhash.h can use it in a
> > preprocessor if directive.
> >
> > As a result also modify xen-cpuid and the ocaml stubs to include
> > xenguest.h.
>
> Adding xenguest.h to ocaml stub has been done so it isn't part of this
> patch anymore ;-).
I assume it was removed on rebase and I didn't even realize.
> > Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
>
> Acked-by: Anthony PERARD <anthony.perard@citrix.com>
Thanks.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v6 02/12] libx86: introduce helper to fetch cpuid leaf
2022-01-17 9:48 ` [PATCH v6 02/12] libx86: introduce helper to fetch cpuid leaf Roger Pau Monne
@ 2022-01-18 12:26 ` Andrew Cooper
2022-01-18 13:18 ` Jan Beulich
2022-01-18 16:17 ` Roger Pau Monné
0 siblings, 2 replies; 24+ messages in thread
From: Andrew Cooper @ 2022-01-18 12:26 UTC (permalink / raw)
To: Roger Pau Monne, xen-devel
Cc: Jan Beulich, Andrew Cooper, Wei Liu, Anthony PERARD
On 17/01/2022 09:48, Roger Pau Monne wrote:
> Introduce a helper based on the current Xen guest_cpuid code in order
> to fetch a cpuid leaf from a policy. The newly introduced function in
> cpuid.c should not be directly called and instead the provided
> x86_cpuid_get_leaf macro should be used that will properly deal with
> const and non-const inputs.
>
> Also add a test to check that the introduced helper doesn't go over
> the bounds of the policy.
>
> Note the code in x86_cpuid_copy_from_buffer is not switched to use the
> new function because of the boundary checks against the max fields of
> the policy, which might not be properly set at the point where
> x86_cpuid_copy_from_buffer get called, for example when filling an
> empty policy from scratch.
Filling an empty policy from scratch will be fine, because we always
ascend through leaves. This also matches the chronology of how CPUID
developed.
The most likely case to go wrong is enabling an optional feature above
max_leaf, and getting the bump to max_leaf out of order. That said, I
suspect such logic would be working on an object, rather than a list.
The important point is that x86_cpuid_copy_from_buffer() is deliberately
invariant to the order of entries for compatibility reasons, even if we
don't expect it to matter in practice.
>
> Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> ---
> Changes since v4:
> - Rename _x86_cpuid_get_leaf to x86_cpuid_get_leaf_const.
>
> Changes since v3:
> - New in this version.
> ---
> Regarding safety of the usage of array_access_nospec to obtain a
> pointer to an element of an array, there are already other instances
> of this usage, for example in viridian_time_wrmsr, so I would assume
> this is fine.
It's a bit of a weird construct, and both GCC and Clang could generate
better code, but it does look to be safe.
> ---
> tools/tests/cpu-policy/test-cpu-policy.c | 75 ++++++++++++++++++++++++
> xen/arch/x86/cpuid.c | 55 +++--------------
> xen/include/xen/lib/x86/cpuid.h | 19 ++++++
> xen/lib/x86/cpuid.c | 52 ++++++++++++++++
> 4 files changed, 153 insertions(+), 48 deletions(-)
>
> diff --git a/tools/tests/cpu-policy/test-cpu-policy.c b/tools/tests/cpu-policy/test-cpu-policy.c
> index ed450a0997..3f777fc1fc 100644
> --- a/tools/tests/cpu-policy/test-cpu-policy.c
> +++ b/tools/tests/cpu-policy/test-cpu-policy.c
> @@ -570,6 +570,80 @@ static void test_cpuid_out_of_range_clearing(void)
> }
> }
>
> +static void test_cpuid_get_leaf_failure(void)
> +{
> + static const struct test {
> + struct cpuid_policy p;
> + const char *name;
> + uint32_t leaf, subleaf;
> + } tests[] = {
> + /* Bound checking logic. */
> + {
> + .name = "Basic max leaf >= array size",
> + .p = {
> + .basic.max_leaf = CPUID_GUEST_NR_BASIC,
> + },
> + },
> + {
> + .name = "Feature max leaf >= array size",
> + .p = {
> + .basic.max_leaf = CPUID_GUEST_NR_BASIC - 1,
> + .feat.max_subleaf = CPUID_GUEST_NR_FEAT,
> + },
> + .leaf = 0x00000007,
> + },
> + {
> + .name = "Extended max leaf >= array size",
> + .p = {
> + .extd.max_leaf = 0x80000000 + CPUID_GUEST_NR_EXTD,
> + },
> + .leaf = 0x80000000,
> + },
> +
> + {
> + .name = "Basic leaf >= max leaf",
> + .p = {
> + .basic.max_leaf = CPUID_GUEST_NR_BASIC - 1,
> + },
> + .leaf = CPUID_GUEST_NR_BASIC,
> + },
> + {
> + .name = "Feature leaf >= max leaf",
> + .p = {
> + .basic.max_leaf = CPUID_GUEST_NR_BASIC - 1,
> + .feat.max_subleaf = CPUID_GUEST_NR_FEAT - 1,
> + },
> + .leaf = 0x00000007,
> + .subleaf = CPUID_GUEST_NR_FEAT,
> + },
> + {
> + .name = "Extended leaf >= max leaf",
> + .p = {
> + .extd.max_leaf = 0x80000000 + CPUID_GUEST_NR_EXTD - 1,
> + },
> + .leaf = 0x80000000 + CPUID_GUEST_NR_EXTD,
> + },
> + };
> + const struct cpuid_policy pc;
> + const struct cpuid_leaf *lc;
> + struct cpuid_policy p;
> + struct cpuid_leaf *l;
> +
> + /* Constness build test. */
> + lc = x86_cpuid_get_leaf(&pc, 0, 0);
> + l = x86_cpuid_get_leaf(&p, 0, 0);
> +
> + printf("Testing CPUID get leaf bound checking:\n");
> +
> + for ( size_t i = 0; i < ARRAY_SIZE(tests); ++i )
> + {
> + const struct test *t = &tests[i];
memdup(). It is important for tests which potentially reach out of
bounds, so ASAN can work.
That said, you're only testing half of the boundary cases. Perhaps more
important important is the case where max_leaf is really out of legal
bounds. Further, it is also important to check the non-NULL cases too.
It would probably be better to have a single cpuid_policy object, and a
list of pointers (perhaps offsets) to interesting max_leaf fields, along
with their relevant compile time bounds. That way, you can try all the
interesting max_leaf values (0, limit-1, limit, ~0) and check
NULL/non-NULLness of the answer with a simple min() calculation.
> diff --git a/xen/include/xen/lib/x86/cpuid.h b/xen/include/xen/lib/x86/cpuid.h
> index a4d254ea96..050cd4f9d1 100644
> --- a/xen/include/xen/lib/x86/cpuid.h
> +++ b/xen/include/xen/lib/x86/cpuid.h
> @@ -431,6 +431,25 @@ int x86_cpuid_copy_from_buffer(struct cpuid_policy *policy,
> uint32_t nr_entries, uint32_t *err_leaf,
> uint32_t *err_subleaf);
>
> +/**
> + * Get a cpuid leaf from a policy object.
> + *
> + * @param policy The cpuid_policy object.
> + * @param leaf The leaf index.
> + * @param subleaf The subleaf index.
> + * @returns a pointer to the requested leaf or NULL in case of error.
> + *
> + * The function will perform out of bound checks. Do not call this function
> + * directly and instead use x86_cpuid_get_leaf that will deal with both const
> + * and non-const policies returning a pointer with constness matching that of
> + * the input.
> + */
> +const struct cpuid_leaf *x86_cpuid_get_leaf_const(const struct cpuid_policy *p,
> + uint32_t leaf,
> + uint32_t subleaf);
Examples like this demonstrate obviously why
const struct cpuid_leaf *x86_cpuid_get_leaf_const(
const struct cpuid_policy *p, uint32_t leaf, uint32_t subleaf);
is a better layout.
> +#define x86_cpuid_get_leaf(p, l, s) \
> + ((__typeof__(&(p)->basic.raw[0]))x86_cpuid_get_leaf_const(p, l, s))
You can drop the outermost brackets.
~Andrew
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v6 02/12] libx86: introduce helper to fetch cpuid leaf
2022-01-18 12:26 ` Andrew Cooper
@ 2022-01-18 13:18 ` Jan Beulich
2022-01-18 16:17 ` Roger Pau Monné
1 sibling, 0 replies; 24+ messages in thread
From: Jan Beulich @ 2022-01-18 13:18 UTC (permalink / raw)
To: Andrew Cooper
Cc: Andrew Cooper, Wei Liu, Anthony PERARD, xen-devel, Roger Pau Monne
On 18.01.2022 13:26, Andrew Cooper wrote:
> On 17/01/2022 09:48, Roger Pau Monne wrote:
>> +#define x86_cpuid_get_leaf(p, l, s) \
>> + ((__typeof__(&(p)->basic.raw[0]))x86_cpuid_get_leaf_const(p, l, s))
>
> You can drop the outermost brackets.
How that? While people aren't expected to write
x86_cpuid_get_leaf(p, l, s)[0], such a case shouldn't result in misleading
compiler diagnostics. Afaict omitting outermost parentheses is only ever
safe on postfix expressions.
Jan
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v6 10/12] libs/{light,guest}: implement xc_cpuid_apply_policy in libxl
2022-01-17 9:48 ` [PATCH v6 10/12] libs/{light,guest}: implement xc_cpuid_apply_policy in libxl Roger Pau Monne
@ 2022-01-18 13:37 ` Andrew Cooper
2022-01-19 13:37 ` Roger Pau Monné
0 siblings, 1 reply; 24+ messages in thread
From: Andrew Cooper @ 2022-01-18 13:37 UTC (permalink / raw)
To: Roger Pau Monne, xen-devel; +Cc: Wei Liu, Anthony PERARD, Juergen Gross
On 17/01/2022 09:48, Roger Pau Monne wrote:
> diff --git a/tools/libs/light/libxl_cpuid.c b/tools/libs/light/libxl_cpuid.c
> index e1acf6648d..7dcdb35a4c 100644
> --- a/tools/libs/light/libxl_cpuid.c
> +++ b/tools/libs/light/libxl_cpuid.c
> @@ -454,6 +456,41 @@ 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) {
> + LOGED(ERROR, domid, "Failed to init CPU policy");
> + rc = ERROR_FAIL;
> + goto out;
> + }
> +
> + r = xc_cpu_policy_get_domain(ctx->xch, domid, policy);
> + if (r) {
> + LOGED(ERROR, domid, "Failed to fetch domain CPU policy");
> + rc = ERROR_FAIL;
> + goto out;
> + }
> +
> + if (restore) {
> + /*
> + * Make sure the policy is compatible with pre Xen 4.13. Note that
> + * newer Xen versions will pass policy data on the restore stream, so
> + * any adjustments done here will be superseded.
> + */
> + r = xc_cpu_policy_make_compat_4_12(ctx->xch, policy, hvm);
One hidden host policy acquisition, and ...
> + if (r) {
> + LOGED(ERROR, domid, "Failed to setup compatible CPU policy");
> + rc = ERROR_FAIL;
> + goto out;
> + }
> + }
> +
> + r = xc_cpu_policy_legacy_topology(ctx->xch, policy, hvm);
... one host featureset and ... (final comment)
> + if (r) {
> + LOGED(ERROR, domid, "Failed to setup CPU policy topology");
> + 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
> @@ -466,6 +503,13 @@ int libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid, bool restore,
> */
> if (info->type == LIBXL_DOMAIN_TYPE_HVM)
> pae = libxl_defbool_val(info->u.hvm.pae);
> + rc = libxl_cpuid_parse_config(&info->cpuid, GCSPRINTF("pae=%d", pae));
> + if (rc) {
> + LOGD(ERROR, domid, "Failed to set PAE CPUID flag");
> + rc = ERROR_FAIL;
> + goto out;
> + }
> +
>
> /*
> * Advertising Invariant TSC to a guest means that the TSC frequency won't
> @@ -481,14 +525,50 @@ 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) {
> + LOGD(ERROR, domid, "Failed to set Invariant TSC CPUID flag");
> + rc = ERROR_FAIL;
> + goto out;
> + }
>
> - r = xc_cpuid_apply_policy(ctx->xch, domid, restore, NULL, 0,
> - pae, itsc, nested_virt, info->cpuid);
> - if (r)
> - LOGEVD(ERROR, -r, domid, "Failed to apply CPUID policy");
> + /* Set Nested virt CPUID bits for HVM. */
> + if (hvm) {
> + rc = libxl_cpuid_parse_config(&info->cpuid, GCSPRINTF("vmx=%d",
> + nested_virt));
> + if (rc) {
> + LOGD(ERROR, domid, "Failed to set VMX CPUID flag");
> + rc = ERROR_FAIL;
> + goto out;
> + }
> +
> + rc = libxl_cpuid_parse_config(&info->cpuid, GCSPRINTF("svm=%d",
> + nested_virt));
> + if (rc) {
> + LOGD(ERROR, domid, "Failed to set SVM CPUID flag");
> + rc = ERROR_FAIL;
> + goto out;
> + }
> + }
libxl_cpuid_parse_config() is a large overhead, and cannot suffer errors
because libxl crashes rather than failing memory allocations. The
invasiveness would be reduced substantially by having:
str = GCSPRINTF("pae=%d,invtsc=%d", pae, itsc);
if ( hvm )
append GCSPRINTF("vmx=%d,svm=%d", nested_virt, nested_virt);
and a single call to libxl_cpuid_parse_config().
Depending on how much we care, these need inserting at the head of the
info->cpuid list so they get processed in the same order as before.
> +
> + /* Apply the bits from info->cpuid if any. */
'if any' is stale now.
> + r = xc_cpu_policy_apply_cpuid(ctx->xch, policy, info->cpuid, hvm);
... and both a host and default policy.
That is a lot of overhead added behind the scenes. It would be rather
better to have this function obtain the host policy and pass it to all 3
helpers. Default policy is harder to judge, but it would avoid needing
to pass an `hvm` boolean down into this helper.
~Andrew
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v6 08/12] libs/guest: rework xc_cpuid_xend_policy
2022-01-17 9:48 ` [PATCH v6 08/12] libs/guest: rework xc_cpuid_xend_policy Roger Pau Monne
@ 2022-01-18 14:06 ` Andrew Cooper
2022-01-19 10:45 ` Roger Pau Monné
0 siblings, 1 reply; 24+ messages in thread
From: Andrew Cooper @ 2022-01-18 14:06 UTC (permalink / raw)
To: Roger Pau Monne, xen-devel; +Cc: Wei Liu, Anthony PERARD, Juergen Gross
On 17/01/2022 09:48, Roger Pau Monne wrote:
> Rename xc_cpuid_xend_policy to xc_cpu_policy_apply_cpuid
I'm not convinced by this name. 'xend' is important because it's the
format of the data passed, which 'cpuid' is not.
It is a horribly inefficient format, and really doesn't want to survive
cleanup to the internals of libxl.
> diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
> index e7ae133d8d..9060a2f763 100644
> --- a/tools/libs/guest/xg_cpuid_x86.c
> +++ b/tools/libs/guest/xg_cpuid_x86.c
> @@ -254,144 +254,107 @@ int xc_set_domain_cpu_policy(xc_interface *xch, uint32_t domid,
> return ret;
> }
>
> -static int compare_leaves(const void *l, const void *r)
> -{
> - const xen_cpuid_leaf_t *lhs = l;
> - const xen_cpuid_leaf_t *rhs = r;
> -
> - if ( lhs->leaf != rhs->leaf )
> - return lhs->leaf < rhs->leaf ? -1 : 1;
> -
> - if ( lhs->subleaf != rhs->subleaf )
> - return lhs->subleaf < rhs->subleaf ? -1 : 1;
> -
> - return 0;
> -}
> -
> -static xen_cpuid_leaf_t *find_leaf(
> - xen_cpuid_leaf_t *leaves, unsigned int nr_leaves,
> - const struct xc_xend_cpuid *xend)
> -{
> - const xen_cpuid_leaf_t key = { xend->leaf, xend->subleaf };
> -
> - return bsearch(&key, leaves, nr_leaves, sizeof(*leaves), compare_leaves);
> -}
> -
> -static int xc_cpuid_xend_policy(
> - xc_interface *xch, uint32_t domid, const struct xc_xend_cpuid *xend)
> +int xc_cpu_policy_apply_cpuid(xc_interface *xch, xc_cpu_policy_t *policy,
> + const struct xc_xend_cpuid *cpuid, bool hvm)
> {
> int rc;
> - xc_dominfo_t di;
> - unsigned int nr_leaves, nr_msrs;
> - uint32_t err_leaf = -1, err_subleaf = -1, err_msr = -1;
> - /*
> - * Three full policies. The host, 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;
> + xc_cpu_policy_t *host = NULL, *def = NULL;
>
> - if ( xc_domain_getinfo(xch, domid, 1, &di) != 1 ||
> - di.domid != domid )
> - {
> - ERROR("Failed to obtain d%d info", domid);
> - rc = -ESRCH;
> - goto fail;
> - }
> -
> - rc = xc_cpu_policy_get_size(xch, &nr_leaves, &nr_msrs);
> - if ( rc )
> - {
> - PERROR("Failed to obtain policy info size");
> - rc = -errno;
> - goto fail;
> - }
> -
> - rc = -ENOMEM;
> - if ( (host = calloc(nr_leaves, sizeof(*host))) == NULL ||
> - (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);
> - 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;
> + xen_cpuid_leaf_t def_leaf;
> + xen_cpuid_leaf_t host_leaf;
>
> - if ( cur_leaf == NULL || def_leaf == NULL || host_leaf == NULL )
> + rc = xc_cpu_policy_get_cpuid(xch, policy, cpuid->leaf, cpuid->subleaf,
> + &cur_leaf);
This is a crazy increase in data shuffling.
Use x86_cpuid_get_leaf() from patch 2, which *is* find_leaf() for
objects rather than lists, and removes the need to further-shuffle the
data later now that you're not modifying cur in place.
It also removes the churn introduced by changing the indirection of
these variables.
It also removes all callers of xc_cpu_policy_get_cpuid(), which don't
have an obvious purpose to begin with. Relatedly,
xc_cpu_policy_get_msr() has no users at all, that I can see.
~Andrew
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v6 02/12] libx86: introduce helper to fetch cpuid leaf
2022-01-18 12:26 ` Andrew Cooper
2022-01-18 13:18 ` Jan Beulich
@ 2022-01-18 16:17 ` Roger Pau Monné
1 sibling, 0 replies; 24+ messages in thread
From: Roger Pau Monné @ 2022-01-18 16:17 UTC (permalink / raw)
To: Andrew Cooper
Cc: xen-devel, Jan Beulich, Andrew Cooper, Wei Liu, Anthony PERARD
On Tue, Jan 18, 2022 at 12:26:18PM +0000, Andrew Cooper wrote:
> On 17/01/2022 09:48, Roger Pau Monne wrote:
> > Introduce a helper based on the current Xen guest_cpuid code in order
> > to fetch a cpuid leaf from a policy. The newly introduced function in
> > cpuid.c should not be directly called and instead the provided
> > x86_cpuid_get_leaf macro should be used that will properly deal with
> > const and non-const inputs.
> >
> > Also add a test to check that the introduced helper doesn't go over
> > the bounds of the policy.
> >
> > Note the code in x86_cpuid_copy_from_buffer is not switched to use the
> > new function because of the boundary checks against the max fields of
> > the policy, which might not be properly set at the point where
> > x86_cpuid_copy_from_buffer get called, for example when filling an
> > empty policy from scratch.
>
> Filling an empty policy from scratch will be fine, because we always
> ascend through leaves. This also matches the chronology of how CPUID
> developed.
I'm slightly confused by this. The main point I wanted to make here is
that x86_cpuid_copy_from_buffer cannot be switched to use
x86_cpuid_get_leaf because the later relies on accessing the different
maximum leaf/subleaf fields in the policy object: basic.max_leaf,
feat.max_subleaf and extd.max_leaf.
That would for example make the existing
test_cpuid_deserialise_failure hit a page fault, since it passes NULL
as a policy object.
I don't really felt like changing test_cpuid_deserialise_failure to
cope with the new behavior of x86_cpuid_copy_from_buffer if it was
switched to use x86_cpuid_get_leaf.
Let me know if that's OK, or if you would like
x86_cpuid_copy_from_buffer switched to use x86_cpuid_get_leaf and
consequently have the callers also adjusted.
> The most likely case to go wrong is enabling an optional feature above
> max_leaf, and getting the bump to max_leaf out of order. That said, I
> suspect such logic would be working on an object, rather than a list.
>
> The important point is that x86_cpuid_copy_from_buffer() is deliberately
> invariant to the order of entries for compatibility reasons, even if we
> don't expect it to matter in practice.
>
> >
> > Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > Reviewed-by: Jan Beulich <jbeulich@suse.com>
> > ---
> > Changes since v4:
> > - Rename _x86_cpuid_get_leaf to x86_cpuid_get_leaf_const.
> >
> > Changes since v3:
> > - New in this version.
> > ---
> > Regarding safety of the usage of array_access_nospec to obtain a
> > pointer to an element of an array, there are already other instances
> > of this usage, for example in viridian_time_wrmsr, so I would assume
> > this is fine.
>
> It's a bit of a weird construct, and both GCC and Clang could generate
> better code, but it does look to be safe.
>
> > ---
> > tools/tests/cpu-policy/test-cpu-policy.c | 75 ++++++++++++++++++++++++
> > xen/arch/x86/cpuid.c | 55 +++--------------
> > xen/include/xen/lib/x86/cpuid.h | 19 ++++++
> > xen/lib/x86/cpuid.c | 52 ++++++++++++++++
> > 4 files changed, 153 insertions(+), 48 deletions(-)
> >
> > diff --git a/tools/tests/cpu-policy/test-cpu-policy.c b/tools/tests/cpu-policy/test-cpu-policy.c
> > index ed450a0997..3f777fc1fc 100644
> > --- a/tools/tests/cpu-policy/test-cpu-policy.c
> > +++ b/tools/tests/cpu-policy/test-cpu-policy.c
> > @@ -570,6 +570,80 @@ static void test_cpuid_out_of_range_clearing(void)
> > }
> > }
> >
> > +static void test_cpuid_get_leaf_failure(void)
> > +{
> > + static const struct test {
> > + struct cpuid_policy p;
> > + const char *name;
> > + uint32_t leaf, subleaf;
> > + } tests[] = {
> > + /* Bound checking logic. */
> > + {
> > + .name = "Basic max leaf >= array size",
> > + .p = {
> > + .basic.max_leaf = CPUID_GUEST_NR_BASIC,
> > + },
> > + },
> > + {
> > + .name = "Feature max leaf >= array size",
> > + .p = {
> > + .basic.max_leaf = CPUID_GUEST_NR_BASIC - 1,
> > + .feat.max_subleaf = CPUID_GUEST_NR_FEAT,
> > + },
> > + .leaf = 0x00000007,
> > + },
> > + {
> > + .name = "Extended max leaf >= array size",
> > + .p = {
> > + .extd.max_leaf = 0x80000000 + CPUID_GUEST_NR_EXTD,
> > + },
> > + .leaf = 0x80000000,
> > + },
> > +
> > + {
> > + .name = "Basic leaf >= max leaf",
> > + .p = {
> > + .basic.max_leaf = CPUID_GUEST_NR_BASIC - 1,
> > + },
> > + .leaf = CPUID_GUEST_NR_BASIC,
> > + },
> > + {
> > + .name = "Feature leaf >= max leaf",
> > + .p = {
> > + .basic.max_leaf = CPUID_GUEST_NR_BASIC - 1,
> > + .feat.max_subleaf = CPUID_GUEST_NR_FEAT - 1,
> > + },
> > + .leaf = 0x00000007,
> > + .subleaf = CPUID_GUEST_NR_FEAT,
> > + },
> > + {
> > + .name = "Extended leaf >= max leaf",
> > + .p = {
> > + .extd.max_leaf = 0x80000000 + CPUID_GUEST_NR_EXTD - 1,
> > + },
> > + .leaf = 0x80000000 + CPUID_GUEST_NR_EXTD,
> > + },
> > + };
> > + const struct cpuid_policy pc;
> > + const struct cpuid_leaf *lc;
> > + struct cpuid_policy p;
> > + struct cpuid_leaf *l;
> > +
> > + /* Constness build test. */
> > + lc = x86_cpuid_get_leaf(&pc, 0, 0);
> > + l = x86_cpuid_get_leaf(&p, 0, 0);
> > +
> > + printf("Testing CPUID get leaf bound checking:\n");
> > +
> > + for ( size_t i = 0; i < ARRAY_SIZE(tests); ++i )
> > + {
> > + const struct test *t = &tests[i];
>
> memdup(). It is important for tests which potentially reach out of
> bounds, so ASAN can work.
Done.
>
> That said, you're only testing half of the boundary cases. Perhaps more
> important important is the case where max_leaf is really out of legal
> bounds.
I was attempting to test this in the first two tests, by setting
max_leaf = CPUID_GUEST_NR_BASIC, max_subleaf = CPUID_GUEST_NR_FEAT and
extd.max_leaf also to it's max value (ie: out of legal bounds). I
could set them to ~0 if that's clearer.
> Further, it is also important to check the non-NULL cases too.
>
> It would probably be better to have a single cpuid_policy object, and a
> list of pointers (perhaps offsets) to interesting max_leaf fields, along
> with their relevant compile time bounds. That way, you can try all the
> interesting max_leaf values (0, limit-1, limit, ~0) and check
> NULL/non-NULLness of the answer with a simple min() calculation.
Then it would make sense to rename to test_cpuid_get_leaf_boundary.
I'm not sure I get the part of having a single cpuid_policy object, as
then we would have to go changing it's contents in order to do the
different tests rather than just having a const one for each single
test that's already setup as expected. Also I'm confused at the usage
of min(), but that's likely because I don't really get the part of
having a single cpuid_policy object.
I assume you would rather prefer your suggested testing to be
implemented here rather than in a follow up patch?
> > diff --git a/xen/include/xen/lib/x86/cpuid.h b/xen/include/xen/lib/x86/cpuid.h
> > index a4d254ea96..050cd4f9d1 100644
> > --- a/xen/include/xen/lib/x86/cpuid.h
> > +++ b/xen/include/xen/lib/x86/cpuid.h
> > @@ -431,6 +431,25 @@ int x86_cpuid_copy_from_buffer(struct cpuid_policy *policy,
> > uint32_t nr_entries, uint32_t *err_leaf,
> > uint32_t *err_subleaf);
> >
> > +/**
> > + * Get a cpuid leaf from a policy object.
> > + *
> > + * @param policy The cpuid_policy object.
> > + * @param leaf The leaf index.
> > + * @param subleaf The subleaf index.
> > + * @returns a pointer to the requested leaf or NULL in case of error.
> > + *
> > + * The function will perform out of bound checks. Do not call this function
> > + * directly and instead use x86_cpuid_get_leaf that will deal with both const
> > + * and non-const policies returning a pointer with constness matching that of
> > + * the input.
> > + */
> > +const struct cpuid_leaf *x86_cpuid_get_leaf_const(const struct cpuid_policy *p,
> > + uint32_t leaf,
> > + uint32_t subleaf);
>
> Examples like this demonstrate obviously why
>
> const struct cpuid_leaf *x86_cpuid_get_leaf_const(
> const struct cpuid_policy *p, uint32_t leaf, uint32_t subleaf);
>
> is a better layout.
I don't care that much in this case TBH. One way or another this is
the style used currently on the file, so it's likely better to keep it
that way.
Thanks, Roger.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v6 08/12] libs/guest: rework xc_cpuid_xend_policy
2022-01-18 14:06 ` Andrew Cooper
@ 2022-01-19 10:45 ` Roger Pau Monné
0 siblings, 0 replies; 24+ messages in thread
From: Roger Pau Monné @ 2022-01-19 10:45 UTC (permalink / raw)
To: Andrew Cooper; +Cc: xen-devel, Wei Liu, Anthony PERARD, Juergen Gross
On Tue, Jan 18, 2022 at 02:06:25PM +0000, Andrew Cooper wrote:
> On 17/01/2022 09:48, Roger Pau Monne wrote:
> > Rename xc_cpuid_xend_policy to xc_cpu_policy_apply_cpuid
>
> I'm not convinced by this name. 'xend' is important because it's the
> format of the data passed, which 'cpuid' is not.
The format would be the libxc format now. Having xend here is a
layering violation IMO.
Note this function goes away in patch 11, so I'm unsure there's a lot
of value in discussing over the name of a function that's about to
disappear.
> It is a horribly inefficient format, and really doesn't want to survive
> cleanup to the internals of libxl.
Even when moved to the internals of libxl the format is not exposed to
users of libxl (it's a libxl__cpuid_policy in libxl_internal.h), so we
are free to modify it at our own will. I would defer the changes to
the format to a separate series, there's enough churn here already. My
aim was to provide a new interface while keeping the functional
changes to a minimum.
> > diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
> > index e7ae133d8d..9060a2f763 100644
> > --- a/tools/libs/guest/xg_cpuid_x86.c
> > +++ b/tools/libs/guest/xg_cpuid_x86.c
> > @@ -254,144 +254,107 @@ int xc_set_domain_cpu_policy(xc_interface *xch, uint32_t domid,
> > return ret;
> > }
> >
> > -static int compare_leaves(const void *l, const void *r)
> > -{
> > - const xen_cpuid_leaf_t *lhs = l;
> > - const xen_cpuid_leaf_t *rhs = r;
> > -
> > - if ( lhs->leaf != rhs->leaf )
> > - return lhs->leaf < rhs->leaf ? -1 : 1;
> > -
> > - if ( lhs->subleaf != rhs->subleaf )
> > - return lhs->subleaf < rhs->subleaf ? -1 : 1;
> > -
> > - return 0;
> > -}
> > -
> > -static xen_cpuid_leaf_t *find_leaf(
> > - xen_cpuid_leaf_t *leaves, unsigned int nr_leaves,
> > - const struct xc_xend_cpuid *xend)
> > -{
> > - const xen_cpuid_leaf_t key = { xend->leaf, xend->subleaf };
> > -
> > - return bsearch(&key, leaves, nr_leaves, sizeof(*leaves), compare_leaves);
> > -}
> > -
> > -static int xc_cpuid_xend_policy(
> > - xc_interface *xch, uint32_t domid, const struct xc_xend_cpuid *xend)
> > +int xc_cpu_policy_apply_cpuid(xc_interface *xch, xc_cpu_policy_t *policy,
> > + const struct xc_xend_cpuid *cpuid, bool hvm)
> > {
> > int rc;
> > - xc_dominfo_t di;
> > - unsigned int nr_leaves, nr_msrs;
> > - uint32_t err_leaf = -1, err_subleaf = -1, err_msr = -1;
> > - /*
> > - * Three full policies. The host, 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;
> > + xc_cpu_policy_t *host = NULL, *def = NULL;
> >
> > - if ( xc_domain_getinfo(xch, domid, 1, &di) != 1 ||
> > - di.domid != domid )
> > - {
> > - ERROR("Failed to obtain d%d info", domid);
> > - rc = -ESRCH;
> > - goto fail;
> > - }
> > -
> > - rc = xc_cpu_policy_get_size(xch, &nr_leaves, &nr_msrs);
> > - if ( rc )
> > - {
> > - PERROR("Failed to obtain policy info size");
> > - rc = -errno;
> > - goto fail;
> > - }
> > -
> > - rc = -ENOMEM;
> > - if ( (host = calloc(nr_leaves, sizeof(*host))) == NULL ||
> > - (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);
> > - 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;
> > + xen_cpuid_leaf_t def_leaf;
> > + xen_cpuid_leaf_t host_leaf;
> >
> > - if ( cur_leaf == NULL || def_leaf == NULL || host_leaf == NULL )
> > + rc = xc_cpu_policy_get_cpuid(xch, policy, cpuid->leaf, cpuid->subleaf,
> > + &cur_leaf);
>
> This is a crazy increase in data shuffling.
>
> Use x86_cpuid_get_leaf() from patch 2, which *is* find_leaf() for
> objects rather than lists, and removes the need to further-shuffle the
> data later now that you're not modifying cur in place.
This function will get moved into libxl in patch 11, and then it would
be a layering violation to call x86_cpuid_get_leaf, so it needs to use
the xc wrapper.
> It also removes the churn introduced by changing the indirection of
> these variables.
>
> It also removes all callers of xc_cpu_policy_get_cpuid(), which don't
> have an obvious purpose to begin with. Relatedly,
> xc_cpu_policy_get_msr() has no users at all, that I can see.
This was introduced in order to provide a sensible interface. It
doesn't make sense to allow getting cpuid leafs but not MSRs from a
policy IMO. I would expect other toolstacks, or libxl in the future to
make use of it.
If you don't think that's enough justification we can leave that patch
aside.
Thanks, Roger.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v6 10/12] libs/{light,guest}: implement xc_cpuid_apply_policy in libxl
2022-01-18 13:37 ` Andrew Cooper
@ 2022-01-19 13:37 ` Roger Pau Monné
0 siblings, 0 replies; 24+ messages in thread
From: Roger Pau Monné @ 2022-01-19 13:37 UTC (permalink / raw)
To: Andrew Cooper; +Cc: xen-devel, Wei Liu, Anthony PERARD, Juergen Gross
On Tue, Jan 18, 2022 at 01:37:18PM +0000, Andrew Cooper wrote:
> On 17/01/2022 09:48, Roger Pau Monne wrote:
> > diff --git a/tools/libs/light/libxl_cpuid.c b/tools/libs/light/libxl_cpuid.c
> > index e1acf6648d..7dcdb35a4c 100644
> > --- a/tools/libs/light/libxl_cpuid.c
> > +++ b/tools/libs/light/libxl_cpuid.c
> > @@ -454,6 +456,41 @@ 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) {
> > + LOGED(ERROR, domid, "Failed to init CPU policy");
> > + rc = ERROR_FAIL;
> > + goto out;
> > + }
> > +
> > + r = xc_cpu_policy_get_domain(ctx->xch, domid, policy);
> > + if (r) {
> > + LOGED(ERROR, domid, "Failed to fetch domain CPU policy");
> > + rc = ERROR_FAIL;
> > + goto out;
> > + }
> > +
> > + if (restore) {
> > + /*
> > + * Make sure the policy is compatible with pre Xen 4.13. Note that
> > + * newer Xen versions will pass policy data on the restore stream, so
> > + * any adjustments done here will be superseded.
> > + */
> > + r = xc_cpu_policy_make_compat_4_12(ctx->xch, policy, hvm);
>
> One hidden host policy acquisition, and ...
>
> > + if (r) {
> > + LOGED(ERROR, domid, "Failed to setup compatible CPU policy");
> > + rc = ERROR_FAIL;
> > + goto out;
> > + }
> > + }
> > +
> > + r = xc_cpu_policy_legacy_topology(ctx->xch, policy, hvm);
>
> ... one host featureset and ... (final comment)
>
> > + if (r) {
> > + LOGED(ERROR, domid, "Failed to setup CPU policy topology");
> > + 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
> > @@ -466,6 +503,13 @@ int libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid, bool restore,
> > */
> > if (info->type == LIBXL_DOMAIN_TYPE_HVM)
> > pae = libxl_defbool_val(info->u.hvm.pae);
> > + rc = libxl_cpuid_parse_config(&info->cpuid, GCSPRINTF("pae=%d", pae));
> > + if (rc) {
> > + LOGD(ERROR, domid, "Failed to set PAE CPUID flag");
> > + rc = ERROR_FAIL;
> > + goto out;
> > + }
> > +
> >
> > /*
> > * Advertising Invariant TSC to a guest means that the TSC frequency won't
> > @@ -481,14 +525,50 @@ 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) {
> > + LOGD(ERROR, domid, "Failed to set Invariant TSC CPUID flag");
> > + rc = ERROR_FAIL;
> > + goto out;
> > + }
> >
> > - r = xc_cpuid_apply_policy(ctx->xch, domid, restore, NULL, 0,
> > - pae, itsc, nested_virt, info->cpuid);
> > - if (r)
> > - LOGEVD(ERROR, -r, domid, "Failed to apply CPUID policy");
> > + /* Set Nested virt CPUID bits for HVM. */
> > + if (hvm) {
> > + rc = libxl_cpuid_parse_config(&info->cpuid, GCSPRINTF("vmx=%d",
> > + nested_virt));
> > + if (rc) {
> > + LOGD(ERROR, domid, "Failed to set VMX CPUID flag");
> > + rc = ERROR_FAIL;
> > + goto out;
> > + }
> > +
> > + rc = libxl_cpuid_parse_config(&info->cpuid, GCSPRINTF("svm=%d",
> > + nested_virt));
> > + if (rc) {
> > + LOGD(ERROR, domid, "Failed to set SVM CPUID flag");
> > + rc = ERROR_FAIL;
> > + goto out;
> > + }
> > + }
>
> libxl_cpuid_parse_config() is a large overhead, and cannot suffer errors
> because libxl crashes rather than failing memory allocations. The
> invasiveness would be reduced substantially by having:
>
> str = GCSPRINTF("pae=%d,invtsc=%d", pae, itsc);
> if ( hvm )
> append GCSPRINTF("vmx=%d,svm=%d", nested_virt, nested_virt);
>
> and a single call to libxl_cpuid_parse_config().
>
>
> Depending on how much we care, these need inserting at the head of the
> info->cpuid list so they get processed in the same order as before.
>
> > +
> > + /* Apply the bits from info->cpuid if any. */
>
> 'if any' is stale now.
>
> > + r = xc_cpu_policy_apply_cpuid(ctx->xch, policy, info->cpuid, hvm);
>
> ... and both a host and default policy.
>
> That is a lot of overhead added behind the scenes. It would be rather
> better to have this function obtain the host policy and pass it to all 3
> helpers. Default policy is harder to judge, but it would avoid needing
> to pass an `hvm` boolean down into this helper.
I've fixed all the above, but haven't added a default policy parameter
to xc_cpu_policy_apply_cpuid. Let me know how strongly you feel about
that.
Thanks, Roger.
^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2022-01-19 13:38 UTC | newest]
Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-17 9:48 [PATCH v6 00/12] libs/guest: new CPUID/MSR interface Roger Pau Monne
2022-01-17 9:48 ` [PATCH v6 01/12] libs/guest: move cpu policy related prototypes to xenguest.h Roger Pau Monne
2022-01-18 9:55 ` Anthony PERARD
2022-01-18 10:50 ` Roger Pau Monné
2022-01-17 9:48 ` [PATCH v6 02/12] libx86: introduce helper to fetch cpuid leaf Roger Pau Monne
2022-01-18 12:26 ` Andrew Cooper
2022-01-18 13:18 ` Jan Beulich
2022-01-18 16:17 ` Roger Pau Monné
2022-01-17 9:48 ` [PATCH v6 03/12] libs/guest: allow fetching a specific CPUID leaf from a cpu policy Roger Pau Monne
2022-01-17 12:28 ` Jan Beulich
2022-01-17 9:48 ` [PATCH v6 04/12] libx86: introduce helper to fetch msr entry Roger Pau Monne
2022-01-17 9:48 ` [PATCH v6 05/12] libs/guest: allow fetching a specific MSR entry from a cpu policy Roger Pau Monne
2022-01-17 12:29 ` Jan Beulich
2022-01-17 9:48 ` [PATCH v6 06/12] libs/guest: make a cpu policy compatible with older Xen versions Roger Pau Monne
2022-01-17 9:48 ` [PATCH v6 07/12] libs/guest: introduce helper set cpu topology in cpu policy Roger Pau Monne
2022-01-17 9:48 ` [PATCH v6 08/12] libs/guest: rework xc_cpuid_xend_policy Roger Pau Monne
2022-01-18 14:06 ` Andrew Cooper
2022-01-19 10:45 ` Roger Pau Monné
2022-01-17 9:48 ` [PATCH v6 09/12] libs/guest: apply a featureset into a cpu policy Roger Pau Monne
2022-01-17 9:48 ` [PATCH v6 10/12] libs/{light,guest}: implement xc_cpuid_apply_policy in libxl Roger Pau Monne
2022-01-18 13:37 ` Andrew Cooper
2022-01-19 13:37 ` Roger Pau Monné
2022-01-17 9:48 ` [PATCH v6 11/12] libs/guest: (re)move xc_cpu_policy_apply_cpuid Roger Pau Monne
2022-01-17 9:48 ` [PATCH v6 12/12] x86/CPUID: shrink max_{,sub}leaf fields according to actual leaf contents Roger Pau Monne
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.