* [PATCH v2 0/6] lib{xc,xl}: support for guest MSR features
@ 2023-07-11 9:22 Roger Pau Monne
2023-07-11 9:22 ` [PATCH v2 1/6] libs/guest: introduce support for setting guest MSRs Roger Pau Monne
` (5 more replies)
0 siblings, 6 replies; 21+ messages in thread
From: Roger Pau Monne @ 2023-07-11 9:22 UTC (permalink / raw)
To: xen-devel; +Cc: Roger Pau Monne, Wei Liu, Anthony PERARD, Juergen Gross
Hello,
The following series adds support for handling guest MSR features as
defined in arch-x86/cpufeatureset.h.
The end result is the user being able to use such features with the
xl.cfg(5) cpuid option. This also involves adding support to all the
underlying layers, so both libxl and libxc also get new functionality in
order to properly parse those.
Thanks, Roger.
Roger Pau Monne (6):
libs/guest: introduce support for setting guest MSRs
libxl: change the type of libxl_cpuid_policy_list
libxl: introduce MSR data in libxl_cpuid_policy
libxl: split logic to parse user provided CPUID features
libxl: use the cpuid feature names from cpufeatureset.h
libxl: add support for parsing MSR features
docs/man/xl.cfg.5.pod.in | 24 +-
tools/include/libxl.h | 8 +-
tools/include/xenctrl.h | 21 +-
tools/libs/guest/xg_cpuid_x86.c | 168 +++++++++-
tools/libs/light/libxl_cpuid.c | 527 ++++++++++++++++--------------
tools/libs/light/libxl_internal.h | 5 +
tools/xl/xl_parse.c | 3 +
7 files changed, 479 insertions(+), 277 deletions(-)
--
2.41.0
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v2 1/6] libs/guest: introduce support for setting guest MSRs
2023-07-11 9:22 [PATCH v2 0/6] lib{xc,xl}: support for guest MSR features Roger Pau Monne
@ 2023-07-11 9:22 ` Roger Pau Monne
2023-07-12 14:34 ` Anthony PERARD
2023-07-13 15:14 ` Andrew Cooper
2023-07-11 9:22 ` [PATCH v2 2/6] libxl: change the type of libxl_cpuid_policy_list Roger Pau Monne
` (4 subsequent siblings)
5 siblings, 2 replies; 21+ messages in thread
From: Roger Pau Monne @ 2023-07-11 9:22 UTC (permalink / raw)
To: xen-devel; +Cc: Roger Pau Monne, Wei Liu, Anthony PERARD, Juergen Gross
Like it's done with CPUID, introduce support for passing MSR values to
xc_cpuid_apply_policy(). The chosen format for expressing MSR policy
data matches the current one used for CPUID. Note that existing
callers of xc_cpuid_apply_policy() can pass NULL as the value for the
newly introduced 'msr' parameter in order to preserve the same
functionality, and in fact that's done in libxl on this patch.
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
tools/include/xenctrl.h | 21 +++-
tools/libs/guest/xg_cpuid_x86.c | 168 +++++++++++++++++++++++++++++++-
tools/libs/light/libxl_cpuid.c | 2 +-
3 files changed, 187 insertions(+), 4 deletions(-)
diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index dba33d5d0f39..faec1dd82453 100644
--- a/tools/include/xenctrl.h
+++ b/tools/include/xenctrl.h
@@ -1822,6 +1822,21 @@ struct xc_xend_cpuid {
char *policy[4];
};
+/*
+ * MSR policy data.
+ *
+ * The format of the policy string is the following:
+ * '1' -> force to 1
+ * '0' -> force to 0
+ * 'x' -> we don't care (use default)
+ * 'k' -> pass through host value
+ */
+struct xc_msr {
+ uint32_t index;
+ char policy[65];
+};
+#define XC_MSR_INPUT_UNUSED 0xffffffffu
+
/*
* Make adjustments to the CPUID settings for a domain.
*
@@ -1833,13 +1848,15 @@ struct xc_xend_cpuid {
* 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.
+ * Then (optionally) apply legacy XEND CPUID overrides (@xend) or MSR (@msr)
+ * 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);
+ bool nested_virt, const struct xc_xend_cpuid *xend,
+ const struct xc_msr *msr);
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 5b035223f4f5..5e5c8124dd74 100644
--- a/tools/libs/guest/xg_cpuid_x86.c
+++ b/tools/libs/guest/xg_cpuid_x86.c
@@ -423,10 +423,169 @@ static int xc_cpuid_xend_policy(
return rc;
}
+static int compare_msr(const void *l, const void *r)
+{
+ const xen_msr_entry_t *lhs = l;
+ const xen_msr_entry_t *rhs = r;
+
+ if ( lhs->idx == rhs->idx )
+ return 0;
+
+ return lhs->idx < rhs->idx ? -1 : 1;
+}
+
+static xen_msr_entry_t *find_msr(
+ xen_msr_entry_t *msrs, unsigned int nr_msrs,
+ uint32_t index)
+{
+ const xen_msr_entry_t key = { .idx = index };
+
+ return bsearch(&key, msrs, nr_msrs, sizeof(*msrs), compare_msr);
+}
+
+
+static int xc_msr_policy(xc_interface *xch, domid_t domid,
+ const struct xc_msr *msr)
+{
+ int rc;
+ bool hvm;
+ xc_domaininfo_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_msr_entry_t *host = NULL, *def = NULL, *cur = NULL;
+ unsigned int nr_host, nr_def, nr_cur;
+
+ if ( (rc = xc_domain_getinfo_single(xch, domid, &di)) < 0 )
+ {
+ PERROR("Failed to obtain d%d info", domid);
+ rc = -errno;
+ goto fail;
+ }
+ hvm = di.flags & XEN_DOMINF_hvm_guest;
+
+ 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_msrs, sizeof(*host))) == NULL ||
+ (def = calloc(nr_msrs, sizeof(*def))) == NULL ||
+ (cur = calloc(nr_msrs, sizeof(*cur))) == NULL )
+ {
+ ERROR("Unable to allocate memory for %u CPUID leaves", nr_leaves);
+ goto fail;
+ }
+
+ /* Get the domain's current policy. */
+ nr_leaves = 0;
+ nr_cur = nr_msrs;
+ rc = get_domain_cpu_policy(xch, domid, &nr_leaves, NULL, &nr_cur, cur);
+ if ( rc )
+ {
+ PERROR("Failed to obtain d%d current policy", domid);
+ rc = -errno;
+ goto fail;
+ }
+
+ /* Get the domain type's default policy. */
+ nr_leaves = 0;
+ nr_def = nr_msrs;
+ rc = get_system_cpu_policy(xch, hvm ? XEN_SYSCTL_cpu_policy_hvm_default
+ : XEN_SYSCTL_cpu_policy_pv_default,
+ &nr_leaves, NULL, &nr_def, def);
+ if ( rc )
+ {
+ PERROR("Failed to obtain %s def policy", hvm ? "hvm" : "pv");
+ rc = -errno;
+ goto fail;
+ }
+
+ /* Get the host policy. */
+ nr_leaves = 0;
+ nr_host = nr_msrs;
+ rc = get_system_cpu_policy(xch, XEN_SYSCTL_cpu_policy_host,
+ &nr_leaves, NULL, &nr_host, host);
+ if ( rc )
+ {
+ PERROR("Failed to obtain host policy");
+ rc = -errno;
+ goto fail;
+ }
+
+ for ( ; msr->index != XC_MSR_INPUT_UNUSED; ++msr )
+ {
+ xen_msr_entry_t *cur_msr = find_msr(cur, nr_cur, msr->index);
+ const xen_msr_entry_t *def_msr = find_msr(def, nr_def, msr->index);
+ const xen_msr_entry_t *host_msr = find_msr(host, nr_host, msr->index);
+ unsigned int i;
+
+ if ( cur_msr == NULL || def_msr == NULL || host_msr == NULL )
+ {
+ ERROR("Missing MSR %#x", msr->index);
+ rc = -ENOENT;
+ goto fail;
+ }
+
+ for ( i = 0; i < ARRAY_SIZE(msr->policy) - 1; i++ )
+ {
+ bool val;
+
+ if ( msr->policy[i] == '1' )
+ val = true;
+ else if ( msr->policy[i] == '0' )
+ val = false;
+ else if ( msr->policy[i] == 'x' )
+ val = test_bit(63 - i, &def_msr->val);
+ else if ( msr->policy[i] == 'k' )
+ val = test_bit(63 - i, &host_msr->val);
+ else
+ {
+ ERROR("Bad character '%c' in policy string '%s'",
+ msr->policy[i], msr->policy);
+ rc = -EINVAL;
+ goto fail;
+ }
+
+ clear_bit(63 - i, &cur_msr->val);
+ if ( val )
+ set_bit(63 - i, &cur_msr->val);
+ }
+ }
+
+ /* Feed the transformed policy back up to Xen. */
+ rc = xc_set_domain_cpu_policy(xch, domid, 0, NULL, nr_cur, cur,
+ &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;
+ }
+
+ /* Success! */
+
+ fail:
+ free(cur);
+ free(def);
+ free(host);
+
+ 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 *xend)
+ const struct xc_xend_cpuid *xend,
+ const struct xc_msr *msr)
{
int rc;
bool hvm;
@@ -663,6 +822,13 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
if ( xend && (rc = xc_cpuid_xend_policy(xch, domid, xend)) )
goto out;
+ if ( msr )
+ {
+ rc = xc_msr_policy(xch, domid, msr);
+ if ( rc )
+ goto out;
+ }
+
rc = 0;
out:
diff --git a/tools/libs/light/libxl_cpuid.c b/tools/libs/light/libxl_cpuid.c
index f5ce9f97959c..c96aeb3bce46 100644
--- a/tools/libs/light/libxl_cpuid.c
+++ b/tools/libs/light/libxl_cpuid.c
@@ -502,7 +502,7 @@ int libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid, bool restore,
info->tsc_mode == LIBXL_TSC_MODE_ALWAYS_EMULATE);
r = xc_cpuid_apply_policy(ctx->xch, domid, restore, NULL, 0,
- pae, itsc, nested_virt, info->cpuid);
+ pae, itsc, nested_virt, info->cpuid, NULL);
if (r)
LOGEVD(ERROR, -r, domid, "Failed to apply CPUID policy");
--
2.41.0
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v2 2/6] libxl: change the type of libxl_cpuid_policy_list
2023-07-11 9:22 [PATCH v2 0/6] lib{xc,xl}: support for guest MSR features Roger Pau Monne
2023-07-11 9:22 ` [PATCH v2 1/6] libs/guest: introduce support for setting guest MSRs Roger Pau Monne
@ 2023-07-11 9:22 ` Roger Pau Monne
2023-07-12 16:02 ` Anthony PERARD
2023-07-11 9:22 ` [PATCH v2 3/6] libxl: introduce MSR data in libxl_cpuid_policy Roger Pau Monne
` (3 subsequent siblings)
5 siblings, 1 reply; 21+ messages in thread
From: Roger Pau Monne @ 2023-07-11 9:22 UTC (permalink / raw)
To: xen-devel; +Cc: Roger Pau Monne, Wei Liu, Anthony PERARD, Juergen Gross
Currently libxl_cpuid_policy_list is an opaque type to the users of
libxl, and internally it's an array of xc_xend_cpuid objects.
Change the type to instead be a structure that contains one array for
CPUID policies, in preparation for it also holding another array for
MSR policies.
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
tools/include/libxl.h | 8 +--
tools/libs/light/libxl_cpuid.c | 89 ++++++++++++++++++++-----------
tools/libs/light/libxl_internal.h | 4 ++
3 files changed, 65 insertions(+), 36 deletions(-)
diff --git a/tools/include/libxl.h b/tools/include/libxl.h
index cac641a7eba2..f3975ecc021f 100644
--- a/tools/include/libxl.h
+++ b/tools/include/libxl.h
@@ -1455,12 +1455,8 @@ typedef struct {
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.
- */
-typedef struct xc_xend_cpuid libxl_cpuid_policy;
-typedef libxl_cpuid_policy * libxl_cpuid_policy_list;
+struct libxl__cpu_policy;
+typedef struct libxl__cpu_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);
void libxl_cpuid_policy_list_copy(libxl_ctx *ctx,
diff --git a/tools/libs/light/libxl_cpuid.c b/tools/libs/light/libxl_cpuid.c
index c96aeb3bce46..724cb4f182d4 100644
--- a/tools/libs/light/libxl_cpuid.c
+++ b/tools/libs/light/libxl_cpuid.c
@@ -19,22 +19,29 @@ int libxl__cpuid_policy_is_empty(libxl_cpuid_policy_list *pl)
return !libxl_cpuid_policy_list_length(pl);
}
-void libxl_cpuid_dispose(libxl_cpuid_policy_list *p_cpuid_list)
+void libxl_cpuid_dispose(libxl_cpuid_policy_list *pl)
{
- int i, j;
- libxl_cpuid_policy_list cpuid_list = *p_cpuid_list;
+ libxl_cpuid_policy_list policy = *pl;
- if (cpuid_list == NULL)
+ if (policy == NULL)
return;
- for (i = 0; cpuid_list[i].input[0] != XEN_CPUID_INPUT_UNUSED; i++) {
- for (j = 0; j < 4; j++)
- if (cpuid_list[i].policy[j] != NULL) {
- free(cpuid_list[i].policy[j]);
- cpuid_list[i].policy[j] = NULL;
- }
+
+ if (policy->cpuid) {
+ unsigned int i, j;
+ struct xc_xend_cpuid *cpuid_list = policy->cpuid;
+
+ for (i = 0; cpuid_list[i].input[0] != XEN_CPUID_INPUT_UNUSED; i++) {
+ for (j = 0; j < 4; j++)
+ if (cpuid_list[i].policy[j] != NULL) {
+ free(cpuid_list[i].policy[j]);
+ cpuid_list[i].policy[j] = NULL;
+ }
+ }
+ free(policy->cpuid);
}
- free(cpuid_list);
- *p_cpuid_list = NULL;
+
+ free(policy);
+ *pl = NULL;
return;
}
@@ -62,11 +69,17 @@ struct cpuid_flags {
/* go through the dynamic array finding the entry for a specified leaf.
* if no entry exists, allocate one and return that.
*/
-static libxl_cpuid_policy_list cpuid_find_match(libxl_cpuid_policy_list *list,
- uint32_t leaf, uint32_t subleaf)
+static struct xc_xend_cpuid *cpuid_find_match(libxl_cpuid_policy_list *pl,
+ uint32_t leaf, uint32_t subleaf)
{
+ libxl_cpuid_policy_list policy = *pl;
+ struct xc_xend_cpuid **list;
int i = 0;
+ if (policy == NULL)
+ policy = *pl = calloc(1, sizeof(*policy));
+
+ list = &policy->cpuid;
if (*list != NULL) {
for (i = 0; (*list)[i].input[0] != XEN_CPUID_INPUT_UNUSED; i++) {
if ((*list)[i].input[0] == leaf && (*list)[i].input[1] == subleaf)
@@ -86,7 +99,7 @@ static libxl_cpuid_policy_list cpuid_find_match(libxl_cpuid_policy_list *list,
* Will overwrite earlier entries and thus can be called multiple
* times.
*/
-int libxl_cpuid_parse_config(libxl_cpuid_policy_list *cpuid, const char* str)
+int libxl_cpuid_parse_config(libxl_cpuid_policy_list *policy, const char* str)
{
#define NA XEN_CPUID_INPUT_UNUSED
static const struct cpuid_flags cpuid_flags[] = {
@@ -345,7 +358,7 @@ int libxl_cpuid_parse_config(libxl_cpuid_policy_list *cpuid, const char* str)
if (flag->name == NULL) {
return 2;
}
- entry = cpuid_find_match(cpuid, flag->leaf, flag->subleaf);
+ entry = cpuid_find_match(policy, flag->leaf, flag->subleaf);
resstr = entry->policy[flag->reg - 1];
num = strtoull(val, &endptr, 0);
flags[flag->length] = 0;
@@ -400,7 +413,7 @@ int libxl_cpuid_parse_config(libxl_cpuid_policy_list *cpuid, const char* str)
* the strings for each register were directly exposed to the user.
* Used for maintaining compatibility with older config files
*/
-int libxl_cpuid_parse_config_xend(libxl_cpuid_policy_list *cpuid,
+int libxl_cpuid_parse_config_xend(libxl_cpuid_policy_list *policy,
const char* str)
{
char *endptr;
@@ -427,7 +440,7 @@ int libxl_cpuid_parse_config_xend(libxl_cpuid_policy_list *cpuid,
return 3;
}
str = endptr + 1;
- entry = cpuid_find_match(cpuid, leaf, subleaf);
+ entry = cpuid_find_match(policy, leaf, subleaf);
for (str = endptr + 1; *str != 0;) {
if (str[0] != 'e' || str[2] != 'x') {
return 4;
@@ -502,7 +515,8 @@ int libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid, bool restore,
info->tsc_mode == LIBXL_TSC_MODE_ALWAYS_EMULATE);
r = xc_cpuid_apply_policy(ctx->xch, domid, restore, NULL, 0,
- pae, itsc, nested_virt, info->cpuid, NULL);
+ pae, itsc, nested_virt,
+ info->cpuid ? info->cpuid->cpuid : NULL, NULL);
if (r)
LOGEVD(ERROR, -r, domid, "Failed to apply CPUID policy");
@@ -527,15 +541,19 @@ static const char *policy_names[4] = { "eax", "ebx", "ecx", "edx" };
*/
yajl_gen_status libxl_cpuid_policy_list_gen_json(yajl_gen hand,
- libxl_cpuid_policy_list *pcpuid)
+ libxl_cpuid_policy_list *pl)
{
- libxl_cpuid_policy_list cpuid = *pcpuid;
+ libxl_cpuid_policy_list policy = *pl;
+ struct xc_xend_cpuid *cpuid;
yajl_gen_status s;
int i, j;
+ if (policy == NULL) goto empty;
+
s = yajl_gen_array_open(hand);
if (s != yajl_gen_status_ok) goto out;
+ cpuid = policy->cpuid;
if (cpuid == NULL) goto empty;
for (i = 0; cpuid[i].input[0] != XEN_CPUID_INPUT_UNUSED; i++) {
@@ -575,7 +593,7 @@ int libxl__cpuid_policy_list_parse_json(libxl__gc *gc,
libxl_cpuid_policy_list *p)
{
int i, size;
- libxl_cpuid_policy_list l;
+ struct xc_xend_cpuid *l;
flexarray_t *array;
if (!libxl__json_object_is_array(o))
@@ -586,8 +604,10 @@ int libxl__cpuid_policy_list_parse_json(libxl__gc *gc,
return 0;
size = array->count;
+ *p = libxl__calloc(NOGC, 1, sizeof(*p));
/* need one extra slot as sentinel */
- l = *p = libxl__calloc(NOGC, size + 1, sizeof(libxl_cpuid_policy));
+ l = (*p)->cpuid = libxl__calloc(NOGC, size + 1,
+ sizeof(struct xc_xend_cpuid));
l[size].input[0] = XEN_CPUID_INPUT_UNUSED;
l[size].input[1] = XEN_CPUID_INPUT_UNUSED;
@@ -630,8 +650,12 @@ int libxl__cpuid_policy_list_parse_json(libxl__gc *gc,
int libxl_cpuid_policy_list_length(const libxl_cpuid_policy_list *pl)
{
int i = 0;
- libxl_cpuid_policy_list l = *pl;
+ const struct xc_xend_cpuid *l;
+
+ if (*pl == NULL)
+ return 0;
+ l = (*pl)->cpuid;
if (l) {
while (l[i].input[0] != XEN_CPUID_INPUT_UNUSED)
i++;
@@ -641,20 +665,25 @@ int libxl_cpuid_policy_list_length(const libxl_cpuid_policy_list *pl)
}
void libxl_cpuid_policy_list_copy(libxl_ctx *ctx,
- libxl_cpuid_policy_list *dst,
- const libxl_cpuid_policy_list *src)
+ libxl_cpuid_policy_list *pdst,
+ const libxl_cpuid_policy_list *psrc)
{
+ struct xc_xend_cpuid **dst;
+ struct xc_xend_cpuid *const *src;
GC_INIT(ctx);
int i, j, len;
- if (*src == NULL) {
- *dst = NULL;
+ if (*psrc == NULL || (*psrc)->cpuid == NULL) {
+ *pdst = NULL;
goto out;
}
- len = libxl_cpuid_policy_list_length(src);
+ *pdst = libxl__calloc(NOGC, 1, sizeof(**pdst));
+ dst = &(*pdst)->cpuid;
+ src = &(*psrc)->cpuid;
+ len = libxl_cpuid_policy_list_length(psrc);
/* one extra slot for sentinel */
- *dst = libxl__calloc(NOGC, len + 1, sizeof(libxl_cpuid_policy));
+ *dst = libxl__calloc(NOGC, len + 1, sizeof(struct xc_xend_cpuid));
(*dst)[len].input[0] = XEN_CPUID_INPUT_UNUSED;
(*dst)[len].input[1] = XEN_CPUID_INPUT_UNUSED;
diff --git a/tools/libs/light/libxl_internal.h b/tools/libs/light/libxl_internal.h
index 1cf3d400bfce..ef882cff3912 100644
--- a/tools/libs/light/libxl_internal.h
+++ b/tools/libs/light/libxl_internal.h
@@ -4869,6 +4869,10 @@ int libxl__setresuid(uid_t ruid, uid_t euid, uid_t suid);
_hidden int libxl__domain_set_paging_mempool_size(
libxl__gc *gc, libxl_domain_config *d_config, uint32_t domid);
+struct libxl__cpu_policy {
+ struct xc_xend_cpuid *cpuid;
+};
+
#endif
/*
--
2.41.0
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v2 3/6] libxl: introduce MSR data in libxl_cpuid_policy
2023-07-11 9:22 [PATCH v2 0/6] lib{xc,xl}: support for guest MSR features Roger Pau Monne
2023-07-11 9:22 ` [PATCH v2 1/6] libs/guest: introduce support for setting guest MSRs Roger Pau Monne
2023-07-11 9:22 ` [PATCH v2 2/6] libxl: change the type of libxl_cpuid_policy_list Roger Pau Monne
@ 2023-07-11 9:22 ` Roger Pau Monne
2023-07-12 16:39 ` Anthony PERARD
2023-07-11 9:22 ` [PATCH v2 4/6] libxl: split logic to parse user provided CPUID features Roger Pau Monne
` (2 subsequent siblings)
5 siblings, 1 reply; 21+ messages in thread
From: Roger Pau Monne @ 2023-07-11 9:22 UTC (permalink / raw)
To: xen-devel; +Cc: Roger Pau Monne, Wei Liu, Anthony PERARD, Juergen Gross
Add a new array field to libxl_cpuid_policy in order to store the MSR
policies.
Note that libxl_cpuid_policy_list_{copy,length,parse_json,gen_json}
are not adjusted to deal with the new MSR array now part of
libxl_cpuid_policy_list.
Adding the MSR data in the libxl_cpuid_policy_list type is done so
that existing users can seamlessly pass MSR features as part of the
CPUID data, without requiring the introduction of a separate
domain_build_info field, and a new set of handlers functions.
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
tools/libs/light/libxl_cpuid.c | 6 +++++-
tools/libs/light/libxl_internal.h | 1 +
2 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/tools/libs/light/libxl_cpuid.c b/tools/libs/light/libxl_cpuid.c
index 724cb4f182d4..65cad28c3ef0 100644
--- a/tools/libs/light/libxl_cpuid.c
+++ b/tools/libs/light/libxl_cpuid.c
@@ -40,6 +40,9 @@ void libxl_cpuid_dispose(libxl_cpuid_policy_list *pl)
free(policy->cpuid);
}
+ if (policy->msr)
+ free(policy->msr);
+
free(policy);
*pl = NULL;
return;
@@ -516,7 +519,8 @@ int libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid, bool restore,
r = xc_cpuid_apply_policy(ctx->xch, domid, restore, NULL, 0,
pae, itsc, nested_virt,
- info->cpuid ? info->cpuid->cpuid : NULL, NULL);
+ info->cpuid ? info->cpuid->cpuid : NULL,
+ info->cpuid ? info->cpuid->msr : NULL);
if (r)
LOGEVD(ERROR, -r, domid, "Failed to apply CPUID policy");
diff --git a/tools/libs/light/libxl_internal.h b/tools/libs/light/libxl_internal.h
index ef882cff3912..b1a7cd9f615b 100644
--- a/tools/libs/light/libxl_internal.h
+++ b/tools/libs/light/libxl_internal.h
@@ -4871,6 +4871,7 @@ _hidden int libxl__domain_set_paging_mempool_size(
struct libxl__cpu_policy {
struct xc_xend_cpuid *cpuid;
+ struct xc_msr *msr;
};
#endif
--
2.41.0
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v2 4/6] libxl: split logic to parse user provided CPUID features
2023-07-11 9:22 [PATCH v2 0/6] lib{xc,xl}: support for guest MSR features Roger Pau Monne
` (2 preceding siblings ...)
2023-07-11 9:22 ` [PATCH v2 3/6] libxl: introduce MSR data in libxl_cpuid_policy Roger Pau Monne
@ 2023-07-11 9:22 ` Roger Pau Monne
2023-07-12 16:57 ` Anthony PERARD
2023-07-11 9:22 ` [PATCH v2 5/6] libxl: use the cpuid feature names from cpufeatureset.h Roger Pau Monne
2023-07-11 9:22 ` [PATCH v2 6/6] libxl: add support for parsing MSR features Roger Pau Monne
5 siblings, 1 reply; 21+ messages in thread
From: Roger Pau Monne @ 2023-07-11 9:22 UTC (permalink / raw)
To: xen-devel; +Cc: Roger Pau Monne, Wei Liu, Anthony PERARD, Juergen Gross
Move the CPUID value parsers out of libxl_cpuid_parse_config() into a
newly created cpuid_add() local helper. This is in preparation for
also adding MSR feature parsing support.
No functional change intended.
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
tools/libs/light/libxl_cpuid.c | 120 +++++++++++++++++----------------
1 file changed, 63 insertions(+), 57 deletions(-)
diff --git a/tools/libs/light/libxl_cpuid.c b/tools/libs/light/libxl_cpuid.c
index 65cad28c3ef0..52e21de81fc7 100644
--- a/tools/libs/light/libxl_cpuid.c
+++ b/tools/libs/light/libxl_cpuid.c
@@ -97,6 +97,66 @@ static struct xc_xend_cpuid *cpuid_find_match(libxl_cpuid_policy_list *pl,
return *list + i;
}
+static int cpuid_add(libxl_cpuid_policy_list *policy,
+ const struct cpuid_flags *flag, const char *val)
+{
+ struct xc_xend_cpuid *entry = cpuid_find_match(policy, flag->leaf,
+ flag->subleaf);
+ unsigned long num;
+ char flags[33], *resstr, *endptr;
+ unsigned int i;
+
+ resstr = entry->policy[flag->reg - 1];
+ num = strtoull(val, &endptr, 0);
+ flags[flag->length] = 0;
+ if (endptr != val) {
+ /* if this was a valid number, write the binary form into the string */
+ for (i = 0; i < flag->length; i++) {
+ flags[flag->length - 1 - i] = "01"[!!(num & (1 << i))];
+ }
+ } else {
+ switch(val[0]) {
+ case 'x': case 'k': case 's':
+ memset(flags, val[0], flag->length);
+ break;
+ default:
+ return 3;
+ }
+ }
+
+ if (resstr == NULL) {
+ resstr = strdup("xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx");
+ }
+
+ /* the family and model entry is potentially split up across
+ * two fields in Fn0000_0001_EAX, so handle them here separately.
+ */
+ if (!strcmp(flag->name, "family")) {
+ if (num < 16) {
+ memcpy(resstr + (32 - 4) - flag->bit, flags + 4, 4);
+ memcpy(resstr + (32 - 8) - 20, "00000000", 8);
+ } else {
+ num -= 15;
+ memcpy(resstr + (32 - 4) - flag->bit, "1111", 4);
+ for (i = 0; i < 7; i++) {
+ flags[7 - i] = "01"[num & 1];
+ num >>= 1;
+ }
+ memcpy(resstr + (32 - 8) - 20, flags, 8);
+ }
+ } else if (!strcmp(flag->name, "model")) {
+ memcpy(resstr + (32 - 4) - 16, flags, 4);
+ memcpy(resstr + (32 - 4) - flag->bit, flags + 4, 4);
+ } else {
+ memcpy(resstr + (32 - flag->length) - flag->bit, flags,
+ flag->length);
+ }
+ entry->policy[flag->reg - 1] = resstr;
+
+ return 0;
+
+}
+
/* parse a single key=value pair and translate it into the libxc
* used interface using 32-characters strings for each register.
* Will overwrite earlier entries and thus can be called multiple
@@ -341,12 +401,8 @@ int libxl_cpuid_parse_config(libxl_cpuid_policy_list *policy, const char* str)
{NULL, 0, NA, CPUID_REG_INV, 0, 0}
};
#undef NA
- char *sep, *val, *endptr;
- int i;
+ const char *sep, *val;
const struct cpuid_flags *flag;
- struct xc_xend_cpuid *entry;
- unsigned long num;
- char flags[33], *resstr;
sep = strchr(str, '=');
if (sep == NULL) {
@@ -356,60 +412,10 @@ int libxl_cpuid_parse_config(libxl_cpuid_policy_list *policy, const char* str)
}
for (flag = cpuid_flags; flag->name != NULL; flag++) {
if(!strncmp(str, flag->name, sep - str) && flag->name[sep - str] == 0)
- break;
- }
- if (flag->name == NULL) {
- return 2;
- }
- entry = cpuid_find_match(policy, flag->leaf, flag->subleaf);
- resstr = entry->policy[flag->reg - 1];
- num = strtoull(val, &endptr, 0);
- flags[flag->length] = 0;
- if (endptr != val) {
- /* if this was a valid number, write the binary form into the string */
- for (i = 0; i < flag->length; i++) {
- flags[flag->length - 1 - i] = "01"[!!(num & (1 << i))];
- }
- } else {
- switch(val[0]) {
- case 'x': case 'k': case 's':
- memset(flags, val[0], flag->length);
- break;
- default:
- return 3;
- }
- }
-
- if (resstr == NULL) {
- resstr = strdup("xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx");
+ return cpuid_add(policy, flag, val);
}
- /* the family and model entry is potentially split up across
- * two fields in Fn0000_0001_EAX, so handle them here separately.
- */
- if (!strncmp(str, "family", sep - str)) {
- if (num < 16) {
- memcpy(resstr + (32 - 4) - flag->bit, flags + 4, 4);
- memcpy(resstr + (32 - 8) - 20, "00000000", 8);
- } else {
- num -= 15;
- memcpy(resstr + (32 - 4) - flag->bit, "1111", 4);
- for (i = 0; i < 7; i++) {
- flags[7 - i] = "01"[num & 1];
- num >>= 1;
- }
- memcpy(resstr + (32 - 8) - 20, flags, 8);
- }
- } else if (!strncmp(str, "model", sep - str)) {
- memcpy(resstr + (32 - 4) - 16, flags, 4);
- memcpy(resstr + (32 - 4) - flag->bit, flags + 4, 4);
- } else {
- memcpy(resstr + (32 - flag->length) - flag->bit, flags,
- flag->length);
- }
- entry->policy[flag->reg - 1] = resstr;
-
- return 0;
+ return 2;
}
/* parse a single list item from the legacy Python xend syntax, where
--
2.41.0
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v2 5/6] libxl: use the cpuid feature names from cpufeatureset.h
2023-07-11 9:22 [PATCH v2 0/6] lib{xc,xl}: support for guest MSR features Roger Pau Monne
` (3 preceding siblings ...)
2023-07-11 9:22 ` [PATCH v2 4/6] libxl: split logic to parse user provided CPUID features Roger Pau Monne
@ 2023-07-11 9:22 ` Roger Pau Monne
2023-07-12 17:23 ` Anthony PERARD
2023-07-11 9:22 ` [PATCH v2 6/6] libxl: add support for parsing MSR features Roger Pau Monne
5 siblings, 1 reply; 21+ messages in thread
From: Roger Pau Monne @ 2023-07-11 9:22 UTC (permalink / raw)
To: xen-devel; +Cc: Roger Pau Monne, Wei Liu, Anthony PERARD, Juergen Gross
The current implementation in libxl_cpuid_parse_config() requires
keeping a list of cpuid feature bits that should be mostly in sync
with the contents of cpufeatureset.h.
Avoid such duplication by using the automatically generated list of
cpuid features in INIT_FEATURE_NAMES in order to map feature names to
featureset bits, and then translate from featureset bits into cpuid
leaf, subleaf, register tuple.
Note that the full contents of the previous cpuid translation table
can't be removed. That's because some feature names allowed by libxl
are not described in the featuresets, or because naming has diverged
and the previous nomenclature is preserved for compatibility reasons.
Should result in no functional change observed by callers, albeit some
new cpuid features will be available as a result of the change.
While there constify cpuid_flags name field.
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v1:
- const unnamed structure cast.
- Declare struct feature_name outside the function.
- Use strcmp.
- Fix indentation.
- Add back missing feature name options.
- Return ERROR_NOMEM if allocation fails.
- Improve xl.cfg documentation about how to reference the features
described in the public header.
---
docs/man/xl.cfg.5.pod.in | 24 +--
tools/libs/light/libxl_cpuid.c | 267 ++++++++++++---------------------
tools/xl/xl_parse.c | 3 +
3 files changed, 107 insertions(+), 187 deletions(-)
diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
index 3979be2a590a..55161856f4c7 100644
--- a/docs/man/xl.cfg.5.pod.in
+++ b/docs/man/xl.cfg.5.pod.in
@@ -2010,24 +2010,16 @@ proccount procpkg stepping
=back
-List of keys taking a character:
+List of keys taking a character can be found in the public header file
+L<arch-x86/cpufeatureset.h|https://xenbits.xen.org/docs/unstable/hypercall/x86_64/include,public,arch-x86,cpufeatureset.h.html>
-=over 4
-
-3dnow 3dnowext 3dnowprefetch abm acpi adx aes altmovcr8 apic arat avx avx2
-avx512-4fmaps avx512-4vnniw avx512bw avx512cd avx512dq avx512er avx512f
-avx512ifma avx512pf avx512vbmi avx512vl bmi1 bmi2 clflushopt clfsh clwb cmov
-cmplegacy cmpxchg16 cmpxchg8 cmt cntxid dca de ds dscpl dtes64 erms est extapic
-f16c ffxsr fma fma4 fpu fsgsbase fxsr hle htt hypervisor ia64 ibs invpcid
-invtsc lahfsahf lm lwp mca mce misalignsse mmx mmxext monitor movbe mpx msr
-mtrr nodeid nx ospke osvw osxsave pae page1gb pat pbe pcid pclmulqdq pdcm
-perfctr_core perfctr_nb pge pku popcnt pse pse36 psn rdrand rdseed rdtscp rtm
-sha skinit smap smep smx ss sse sse2 sse3 sse4.1 sse4.2 sse4_1 sse4_2 sse4a
-ssse3 svm svm_decode svm_lbrv svm_npt svm_nrips svm_pausefilt svm_tscrate
-svm_vmcbclean syscall sysenter tbm tm tm2 topoext tsc tsc-deadline tsc_adjust
-umip vme vmx wdt x2apic xop xsave xtpr
+The feature names described in C<cpufeatureset.h> should be specified in all
+lowercase letters, and with underscores converted to hyphens. For example in
+order to reference feature C<LAHF_LM> the string C<lahf-lm> should be used.
-=back
+Note that C<clflush> is described as an option that takes a value, and that
+takes precedence over the C<clflush> flag in C<cpufeatureset.h>. The feature
+flag must be referenced as C<clfsh>.
=back
diff --git a/tools/libs/light/libxl_cpuid.c b/tools/libs/light/libxl_cpuid.c
index 52e21de81fc7..b1c4f8f2f45b 100644
--- a/tools/libs/light/libxl_cpuid.c
+++ b/tools/libs/light/libxl_cpuid.c
@@ -14,6 +14,8 @@
#include "libxl_internal.h"
+#include <xen/lib/x86/cpu-policy.h>
+
int libxl__cpuid_policy_is_empty(libxl_cpuid_policy_list *pl)
{
return !libxl_cpuid_policy_list_length(pl);
@@ -61,7 +63,7 @@ void libxl_cpuid_dispose(libxl_cpuid_policy_list *pl)
* Used for the static structure describing all features.
*/
struct cpuid_flags {
- char* name;
+ const char *name;
uint32_t leaf;
uint32_t subleaf;
int reg;
@@ -154,7 +156,19 @@ static int cpuid_add(libxl_cpuid_policy_list *policy,
entry->policy[flag->reg - 1] = resstr;
return 0;
+}
+
+struct feature_name {
+ const char *name;
+ unsigned int bit;
+};
+
+static int search_feature(const void *a, const void *b)
+{
+ const char *key = a;
+ const char *feat = ((const struct feature_name *)b)->name;
+ return strcmp(key, feat);
}
/* parse a single key=value pair and translate it into the libxc
@@ -177,208 +191,42 @@ int libxl_cpuid_parse_config(libxl_cpuid_policy_list *policy, const char* str)
{"proccount", 0x00000001, NA, CPUID_REG_EBX, 16, 8},
{"localapicid", 0x00000001, NA, CPUID_REG_EBX, 24, 8},
- {"sse3", 0x00000001, NA, CPUID_REG_ECX, 0, 1},
- {"pclmulqdq", 0x00000001, NA, CPUID_REG_ECX, 1, 1},
- {"dtes64", 0x00000001, NA, CPUID_REG_ECX, 2, 1},
- {"monitor", 0x00000001, NA, CPUID_REG_ECX, 3, 1},
- {"dscpl", 0x00000001, NA, CPUID_REG_ECX, 4, 1},
- {"vmx", 0x00000001, NA, CPUID_REG_ECX, 5, 1},
- {"smx", 0x00000001, NA, CPUID_REG_ECX, 6, 1},
{"est", 0x00000001, NA, CPUID_REG_ECX, 7, 1},
- {"tm2", 0x00000001, NA, CPUID_REG_ECX, 8, 1},
- {"ssse3", 0x00000001, NA, CPUID_REG_ECX, 9, 1},
{"cntxid", 0x00000001, NA, CPUID_REG_ECX, 10, 1},
- {"fma", 0x00000001, NA, CPUID_REG_ECX, 12, 1},
{"cmpxchg16", 0x00000001, NA, CPUID_REG_ECX, 13, 1},
- {"xtpr", 0x00000001, NA, CPUID_REG_ECX, 14, 1},
- {"pdcm", 0x00000001, NA, CPUID_REG_ECX, 15, 1},
- {"pcid", 0x00000001, NA, CPUID_REG_ECX, 17, 1},
- {"dca", 0x00000001, NA, CPUID_REG_ECX, 18, 1},
/* Linux uses sse4_{1,2}. Keep sse4.{1,2} for compatibility */
{"sse4_1", 0x00000001, NA, CPUID_REG_ECX, 19, 1},
{"sse4.1", 0x00000001, NA, CPUID_REG_ECX, 19, 1},
{"sse4_2", 0x00000001, NA, CPUID_REG_ECX, 20, 1},
{"sse4.2", 0x00000001, NA, CPUID_REG_ECX, 20, 1},
- {"x2apic", 0x00000001, NA, CPUID_REG_ECX, 21, 1},
- {"movbe", 0x00000001, NA, CPUID_REG_ECX, 22, 1},
- {"popcnt", 0x00000001, NA, CPUID_REG_ECX, 23, 1},
- {"tsc-deadline", 0x00000001, NA, CPUID_REG_ECX, 24, 1},
{"aes", 0x00000001, NA, CPUID_REG_ECX, 25, 1},
- {"xsave", 0x00000001, NA, CPUID_REG_ECX, 26, 1},
- {"osxsave", 0x00000001, NA, CPUID_REG_ECX, 27, 1},
- {"avx", 0x00000001, NA, CPUID_REG_ECX, 28, 1},
- {"f16c", 0x00000001, NA, CPUID_REG_ECX, 29, 1},
- {"rdrand", 0x00000001, NA, CPUID_REG_ECX, 30, 1},
- {"hypervisor", 0x00000001, NA, CPUID_REG_ECX, 31, 1},
-
- {"fpu", 0x00000001, NA, CPUID_REG_EDX, 0, 1},
- {"vme", 0x00000001, NA, CPUID_REG_EDX, 1, 1},
- {"de", 0x00000001, NA, CPUID_REG_EDX, 2, 1},
- {"pse", 0x00000001, NA, CPUID_REG_EDX, 3, 1},
- {"tsc", 0x00000001, NA, CPUID_REG_EDX, 4, 1},
- {"msr", 0x00000001, NA, CPUID_REG_EDX, 5, 1},
- {"pae", 0x00000001, NA, CPUID_REG_EDX, 6, 1},
- {"mce", 0x00000001, NA, CPUID_REG_EDX, 7, 1},
+
{"cmpxchg8", 0x00000001, NA, CPUID_REG_EDX, 8, 1},
- {"apic", 0x00000001, NA, CPUID_REG_EDX, 9, 1},
{"sysenter", 0x00000001, NA, CPUID_REG_EDX, 11, 1},
- {"mtrr", 0x00000001, NA, CPUID_REG_EDX, 12, 1},
- {"pge", 0x00000001, NA, CPUID_REG_EDX, 13, 1},
- {"mca", 0x00000001, NA, CPUID_REG_EDX, 14, 1},
- {"cmov", 0x00000001, NA, CPUID_REG_EDX, 15, 1},
- {"pat", 0x00000001, NA, CPUID_REG_EDX, 16, 1},
- {"pse36", 0x00000001, NA, CPUID_REG_EDX, 17, 1},
{"psn", 0x00000001, NA, CPUID_REG_EDX, 18, 1},
{"clfsh", 0x00000001, NA, CPUID_REG_EDX, 19, 1},
- {"ds", 0x00000001, NA, CPUID_REG_EDX, 21, 1},
- {"acpi", 0x00000001, NA, CPUID_REG_EDX, 22, 1},
- {"mmx", 0x00000001, NA, CPUID_REG_EDX, 23, 1},
- {"fxsr", 0x00000001, NA, CPUID_REG_EDX, 24, 1},
- {"sse", 0x00000001, NA, CPUID_REG_EDX, 25, 1},
- {"sse2", 0x00000001, NA, CPUID_REG_EDX, 26, 1},
- {"ss", 0x00000001, NA, CPUID_REG_EDX, 27, 1},
- {"htt", 0x00000001, NA, CPUID_REG_EDX, 28, 1},
{"tm", 0x00000001, NA, CPUID_REG_EDX, 29, 1},
{"ia64", 0x00000001, NA, CPUID_REG_EDX, 30, 1},
{"pbe", 0x00000001, NA, CPUID_REG_EDX, 31, 1},
{"arat", 0x00000006, NA, CPUID_REG_EAX, 2, 1},
- {"fsgsbase", 0x00000007, 0, CPUID_REG_EBX, 0, 1},
{"tsc_adjust", 0x00000007, 0, CPUID_REG_EBX, 1, 1},
- {"bmi1", 0x00000007, 0, CPUID_REG_EBX, 3, 1},
- {"hle", 0x00000007, 0, CPUID_REG_EBX, 4, 1},
- {"avx2", 0x00000007, 0, CPUID_REG_EBX, 5, 1},
- {"smep", 0x00000007, 0, CPUID_REG_EBX, 7, 1},
- {"bmi2", 0x00000007, 0, CPUID_REG_EBX, 8, 1},
- {"erms", 0x00000007, 0, CPUID_REG_EBX, 9, 1},
- {"invpcid", 0x00000007, 0, CPUID_REG_EBX, 10, 1},
- {"rtm", 0x00000007, 0, CPUID_REG_EBX, 11, 1},
{"cmt", 0x00000007, 0, CPUID_REG_EBX, 12, 1},
- {"mpx", 0x00000007, 0, CPUID_REG_EBX, 14, 1},
- {"avx512f", 0x00000007, 0, CPUID_REG_EBX, 16, 1},
- {"avx512dq", 0x00000007, 0, CPUID_REG_EBX, 17, 1},
- {"rdseed", 0x00000007, 0, CPUID_REG_EBX, 18, 1},
- {"adx", 0x00000007, 0, CPUID_REG_EBX, 19, 1},
- {"smap", 0x00000007, 0, CPUID_REG_EBX, 20, 1},
- {"avx512-ifma", 0x00000007, 0, CPUID_REG_EBX, 21, 1},
- {"clflushopt", 0x00000007, 0, CPUID_REG_EBX, 23, 1},
- {"clwb", 0x00000007, 0, CPUID_REG_EBX, 24, 1},
- {"proc-trace", 0x00000007, 0, CPUID_REG_EBX, 25, 1},
- {"avx512pf", 0x00000007, 0, CPUID_REG_EBX, 26, 1},
- {"avx512er", 0x00000007, 0, CPUID_REG_EBX, 27, 1},
- {"avx512cd", 0x00000007, 0, CPUID_REG_EBX, 28, 1},
- {"sha", 0x00000007, 0, CPUID_REG_EBX, 29, 1},
- {"avx512bw", 0x00000007, 0, CPUID_REG_EBX, 30, 1},
- {"avx512vl", 0x00000007, 0, CPUID_REG_EBX, 31, 1},
-
- {"prefetchwt1", 0x00000007, 0, CPUID_REG_ECX, 0, 1},
- {"avx512-vbmi", 0x00000007, 0, CPUID_REG_ECX, 1, 1},
- {"umip", 0x00000007, 0, CPUID_REG_ECX, 2, 1},
- {"pku", 0x00000007, 0, CPUID_REG_ECX, 3, 1},
- {"ospke", 0x00000007, 0, CPUID_REG_ECX, 4, 1},
- {"avx512-vbmi2", 0x00000007, 0, CPUID_REG_ECX, 6, 1},
- {"cet-ss", 0x00000007, 0, CPUID_REG_ECX, 7, 1},
- {"gfni", 0x00000007, 0, CPUID_REG_ECX, 8, 1},
- {"vaes", 0x00000007, 0, CPUID_REG_ECX, 9, 1},
- {"vpclmulqdq", 0x00000007, 0, CPUID_REG_ECX, 10, 1},
- {"avx512-vnni", 0x00000007, 0, CPUID_REG_ECX, 11, 1},
- {"avx512-bitalg",0x00000007, 0, CPUID_REG_ECX, 12, 1},
- {"avx512-vpopcntdq",0x00000007,0,CPUID_REG_ECX, 14, 1},
- {"rdpid", 0x00000007, 0, CPUID_REG_ECX, 22, 1},
- {"cldemote", 0x00000007, 0, CPUID_REG_ECX, 25, 1},
- {"pks", 0x00000007, 0, CPUID_REG_ECX, 31, 1},
-
- {"avx512-4vnniw",0x00000007, 0, CPUID_REG_EDX, 2, 1},
- {"avx512-4fmaps",0x00000007, 0, CPUID_REG_EDX, 3, 1},
- {"fsrm", 0x00000007, 0, CPUID_REG_EDX, 4, 1},
- {"avx512-vp2intersect",0x00000007,0,CPUID_REG_EDX,8, 1},
- {"srbds-ctrl", 0x00000007, 0, CPUID_REG_EDX, 9, 1},
- {"md-clear", 0x00000007, 0, CPUID_REG_EDX, 10, 1},
- {"serialize", 0x00000007, 0, CPUID_REG_EDX, 14, 1},
- {"tsxldtrk", 0x00000007, 0, CPUID_REG_EDX, 16, 1},
- {"cet-ibt", 0x00000007, 0, CPUID_REG_EDX, 20, 1},
- {"avx512-fp16", 0x00000007, 0, CPUID_REG_EDX, 23, 1},
- {"ibrsb", 0x00000007, 0, CPUID_REG_EDX, 26, 1},
- {"stibp", 0x00000007, 0, CPUID_REG_EDX, 27, 1},
- {"l1d-flush", 0x00000007, 0, CPUID_REG_EDX, 28, 1},
- {"arch-caps", 0x00000007, 0, CPUID_REG_EDX, 29, 1},
- {"core-caps", 0x00000007, 0, CPUID_REG_EDX, 30, 1},
- {"ssbd", 0x00000007, 0, CPUID_REG_EDX, 31, 1},
-
- {"avx-vnni", 0x00000007, 1, CPUID_REG_EAX, 4, 1},
- {"avx512-bf16", 0x00000007, 1, CPUID_REG_EAX, 5, 1},
- {"fzrm", 0x00000007, 1, CPUID_REG_EAX, 10, 1},
- {"fsrs", 0x00000007, 1, CPUID_REG_EAX, 11, 1},
- {"fsrcs", 0x00000007, 1, CPUID_REG_EAX, 12, 1},
- {"wrmsrns", 0x00000007, 1, CPUID_REG_EAX, 19, 1},
- {"avx-ifma", 0x00000007, 1, CPUID_REG_EAX, 23, 1},
-
- {"avx-vnni-int8",0x00000007, 1, CPUID_REG_EDX, 4, 1},
- {"avx-ne-convert",0x00000007, 1, CPUID_REG_EDX, 5, 1},
- {"cet-sss", 0x00000007, 1, CPUID_REG_EDX, 18, 1},
-
- {"intel-psfd", 0x00000007, 2, CPUID_REG_EDX, 0, 1},
- {"ipred-ctrl", 0x00000007, 2, CPUID_REG_EDX, 1, 1},
- {"rrsba-ctrl", 0x00000007, 2, CPUID_REG_EDX, 2, 1},
- {"ddp-ctrl", 0x00000007, 2, CPUID_REG_EDX, 3, 1},
- {"bhi-ctrl", 0x00000007, 2, CPUID_REG_EDX, 4, 1},
- {"mcdt-no", 0x00000007, 2, CPUID_REG_EDX, 5, 1},
{"lahfsahf", 0x80000001, NA, CPUID_REG_ECX, 0, 1},
{"cmplegacy", 0x80000001, NA, CPUID_REG_ECX, 1, 1},
- {"svm", 0x80000001, NA, CPUID_REG_ECX, 2, 1},
- {"extapic", 0x80000001, NA, CPUID_REG_ECX, 3, 1},
{"altmovcr8", 0x80000001, NA, CPUID_REG_ECX, 4, 1},
- {"abm", 0x80000001, NA, CPUID_REG_ECX, 5, 1},
- {"sse4a", 0x80000001, NA, CPUID_REG_ECX, 6, 1},
- {"misalignsse", 0x80000001, NA, CPUID_REG_ECX, 7, 1},
- {"3dnowprefetch",0x80000001, NA, CPUID_REG_ECX, 8, 1},
- {"osvw", 0x80000001, NA, CPUID_REG_ECX, 9, 1},
- {"ibs", 0x80000001, NA, CPUID_REG_ECX, 10, 1},
- {"xop", 0x80000001, NA, CPUID_REG_ECX, 11, 1},
- {"skinit", 0x80000001, NA, CPUID_REG_ECX, 12, 1},
- {"wdt", 0x80000001, NA, CPUID_REG_ECX, 13, 1},
- {"lwp", 0x80000001, NA, CPUID_REG_ECX, 15, 1},
- {"fma4", 0x80000001, NA, CPUID_REG_ECX, 16, 1},
{"nodeid", 0x80000001, NA, CPUID_REG_ECX, 19, 1},
- {"tbm", 0x80000001, NA, CPUID_REG_ECX, 21, 1},
- {"topoext", 0x80000001, NA, CPUID_REG_ECX, 22, 1},
{"perfctr_core", 0x80000001, NA, CPUID_REG_ECX, 23, 1},
{"perfctr_nb", 0x80000001, NA, CPUID_REG_ECX, 24, 1},
- {"syscall", 0x80000001, NA, CPUID_REG_EDX, 11, 1},
- {"nx", 0x80000001, NA, CPUID_REG_EDX, 20, 1},
- {"mmxext", 0x80000001, NA, CPUID_REG_EDX, 22, 1},
- {"ffxsr", 0x80000001, NA, CPUID_REG_EDX, 25, 1},
- {"page1gb", 0x80000001, NA, CPUID_REG_EDX, 26, 1},
- {"rdtscp", 0x80000001, NA, CPUID_REG_EDX, 27, 1},
- {"lm", 0x80000001, NA, CPUID_REG_EDX, 29, 1},
- {"3dnowext", 0x80000001, NA, CPUID_REG_EDX, 30, 1},
- {"3dnow", 0x80000001, NA, CPUID_REG_EDX, 31, 1},
-
{"procpkg", 0x00000004, 0, CPUID_REG_EAX, 26, 6},
{"invtsc", 0x80000007, NA, CPUID_REG_EDX, 8, 1},
- {"clzero", 0x80000008, NA, CPUID_REG_EBX, 0, 1},
- {"rstr-fp-err-ptrs", 0x80000008, NA, CPUID_REG_EBX, 2, 1},
- {"wbnoinvd", 0x80000008, NA, CPUID_REG_EBX, 9, 1},
- {"ibpb", 0x80000008, NA, CPUID_REG_EBX, 12, 1},
- {"ibrs", 0x80000008, NA, CPUID_REG_EBX, 14, 1},
- {"amd-stibp", 0x80000008, NA, CPUID_REG_EBX, 15, 1},
- {"ibrs-always", 0x80000008, NA, CPUID_REG_EBX, 16, 1},
- {"stibp-always", 0x80000008, NA, CPUID_REG_EBX, 17, 1},
- {"ibrs-fast", 0x80000008, NA, CPUID_REG_EBX, 18, 1},
- {"ibrs-same-mode", 0x80000008, NA, CPUID_REG_EBX, 19, 1},
- {"no-lmsl", 0x80000008, NA, CPUID_REG_EBX, 20, 1},
{"ppin", 0x80000008, NA, CPUID_REG_EBX, 23, 1},
- {"amd-ssbd", 0x80000008, NA, CPUID_REG_EBX, 24, 1},
- {"virt-ssbd", 0x80000008, NA, CPUID_REG_EBX, 25, 1},
- {"ssb-no", 0x80000008, NA, CPUID_REG_EBX, 26, 1},
- {"psfd", 0x80000008, NA, CPUID_REG_EBX, 28, 1},
{"btc-no", 0x80000008, NA, CPUID_REG_EBX, 29, 1},
- {"ibpb-ret", 0x80000008, NA, CPUID_REG_EBX, 30, 1},
{"nc", 0x80000008, NA, CPUID_REG_ECX, 0, 8},
{"apicidsize", 0x80000008, NA, CPUID_REG_ECX, 12, 4},
@@ -392,17 +240,63 @@ int libxl_cpuid_parse_config(libxl_cpuid_policy_list *policy, const char* str)
{"svm_pausefilt",0x8000000a, NA, CPUID_REG_EDX, 10, 1},
{"lfence+", 0x80000021, NA, CPUID_REG_EAX, 2, 1},
- {"nscb", 0x80000021, NA, CPUID_REG_EAX, 6, 1},
- {"auto-ibrs", 0x80000021, NA, CPUID_REG_EAX, 8, 1},
- {"cpuid-user-dis", 0x80000021, NA, CPUID_REG_EAX, 17, 1},
{"maxhvleaf", 0x40000000, NA, CPUID_REG_EAX, 0, 8},
{NULL, 0, NA, CPUID_REG_INV, 0, 0}
};
+ static const struct feature_name features[] = INIT_FEATURE_NAMES;
+ /*
+ * NB: if we switch to using a cpu_policy derived object instead of a
+ * libxl_cpuid_policy_list we could get rid of the featureset -> cpuid leaf
+ * conversion table and use a featureset directly as we have conversions
+ * to/from featureset and cpu_policy.
+ */
+ static const struct {
+ enum { FEAT_CPUID, FEAT_MSR } type;
+ union {
+ struct {
+ uint32_t leaf, subleaf;
+ unsigned int reg;
+ } cpuid;
+ struct {
+ uint32_t index;
+ unsigned int reg;
+ } msr;
+ };
+ } feature_to_policy[] = {
+#define CPUID_ENTRY(l, s, r) \
+ { .type = FEAT_CPUID, .cpuid.leaf = l, .cpuid.subleaf = s, .cpuid.reg = r }
+#define MSR_ENTRY(i, r) \
+ { .type = FEAT_MSR, .msr.index = i, .msr.reg = r }
+ CPUID_ENTRY(0x00000001, NA, CPUID_REG_EDX),
+ CPUID_ENTRY(0x00000001, NA, CPUID_REG_ECX),
+ CPUID_ENTRY(0x80000001, NA, CPUID_REG_EDX),
+ CPUID_ENTRY(0x80000001, NA, CPUID_REG_ECX),
+ CPUID_ENTRY(0x0000000D, 1, CPUID_REG_EAX),
+ CPUID_ENTRY(0x00000007, 0, CPUID_REG_EBX),
+ CPUID_ENTRY(0x00000007, 0, CPUID_REG_ECX),
+ CPUID_ENTRY(0x80000007, NA, CPUID_REG_EDX),
+ CPUID_ENTRY(0x80000008, NA, CPUID_REG_EBX),
+ CPUID_ENTRY(0x00000007, 0, CPUID_REG_EDX),
+ CPUID_ENTRY(0x00000007, 1, CPUID_REG_EAX),
+ CPUID_ENTRY(0x80000021, NA, CPUID_REG_EAX),
+ CPUID_ENTRY(0x00000007, 1, CPUID_REG_EBX),
+ CPUID_ENTRY(0x00000007, 2, CPUID_REG_EDX),
+ CPUID_ENTRY(0x00000007, 1, CPUID_REG_ECX),
+ CPUID_ENTRY(0x00000007, 1, CPUID_REG_EDX),
+ MSR_ENTRY(0x10a, CPUID_REG_EAX),
+ MSR_ENTRY(0x10a, CPUID_REG_EDX),
+#undef MSR_ENTRY
+#undef CPUID_ENTRY
+ };
#undef NA
const char *sep, *val;
+ char *name;
const struct cpuid_flags *flag;
+ const struct feature_name *feat;
+
+ BUILD_BUG_ON(ARRAY_SIZE(feature_to_policy) != FEATURESET_NR_ENTRIES);
sep = strchr(str, '=');
if (sep == NULL) {
@@ -415,6 +309,37 @@ int libxl_cpuid_parse_config(libxl_cpuid_policy_list *policy, const char* str)
return cpuid_add(policy, flag, val);
}
+ /* Provide a NUL terminated feature name to the search helper. */
+ name = strndup(str, sep - str);
+ if (name == NULL)
+ return ERROR_NOMEM;
+
+ feat = bsearch(name, features, ARRAY_SIZE(features), sizeof(features[0]),
+ search_feature);
+ free(name);
+
+ if (feat == NULL)
+ return 2;
+
+ switch (feature_to_policy[feat->bit / 32].type) {
+ case FEAT_CPUID:
+ {
+ struct cpuid_flags f;
+
+ f.name = feat->name;
+ f.leaf = feature_to_policy[feat->bit / 32].cpuid.leaf;
+ f.subleaf = feature_to_policy[feat->bit / 32].cpuid.subleaf;
+ f.reg = feature_to_policy[feat->bit / 32].cpuid.reg;
+ f.bit = feat->bit % 32;
+ f.length = 1;
+
+ return cpuid_add(policy, &f, val);
+ }
+
+ case FEAT_MSR:
+ return 2;
+ }
+
return 2;
}
diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
index f036e56fc239..7bf587455d08 100644
--- a/tools/xl/xl_parse.c
+++ b/tools/xl/xl_parse.c
@@ -2620,6 +2620,9 @@ skip_usbdev:
case 3:
errstr = "illegal CPUID value (must be: [0|1|x|k|s])";
break;
+ case ERROR_NOMEM:
+ errstr = "out of memory";
+ break;
default:
errstr = "unknown error";
break;
--
2.41.0
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v2 6/6] libxl: add support for parsing MSR features
2023-07-11 9:22 [PATCH v2 0/6] lib{xc,xl}: support for guest MSR features Roger Pau Monne
` (4 preceding siblings ...)
2023-07-11 9:22 ` [PATCH v2 5/6] libxl: use the cpuid feature names from cpufeatureset.h Roger Pau Monne
@ 2023-07-11 9:22 ` Roger Pau Monne
2023-07-13 10:39 ` Anthony PERARD
5 siblings, 1 reply; 21+ messages in thread
From: Roger Pau Monne @ 2023-07-11 9:22 UTC (permalink / raw)
To: xen-devel; +Cc: Roger Pau Monne, Wei Liu, Anthony PERARD, Juergen Gross
Introduce support for handling MSR features in
libxl_cpuid_parse_config(). The MSR policies are added to the
libxl_cpuid_policy like the CPUID one, which gets passed to
xc_cpuid_apply_policy().
This allows existing users of libxl to provide MSR related features as
key=value pairs to libxl_cpuid_parse_config() without requiring the
usage of a different API.
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
tools/libs/light/libxl_cpuid.c | 61 +++++++++++++++++++++++++++++++++-
1 file changed, 60 insertions(+), 1 deletion(-)
diff --git a/tools/libs/light/libxl_cpuid.c b/tools/libs/light/libxl_cpuid.c
index b1c4f8f2f45b..86a08f29a19c 100644
--- a/tools/libs/light/libxl_cpuid.c
+++ b/tools/libs/light/libxl_cpuid.c
@@ -158,6 +158,57 @@ static int cpuid_add(libxl_cpuid_policy_list *policy,
return 0;
}
+static struct xc_msr *msr_find_match(libxl_cpuid_policy_list *pl, uint32_t index)
+{
+ unsigned int i = 0;
+ libxl_cpuid_policy_list policy = *pl;
+
+ if (policy == NULL)
+ policy = *pl = calloc(1, sizeof(*policy));
+
+ if (policy->msr != NULL)
+ for (i = 0; policy->msr[i].index != XC_MSR_INPUT_UNUSED; i++)
+ if (policy->msr[i].index == index)
+ return &policy->msr[i];
+
+ policy->msr = realloc(policy->msr, sizeof(struct xc_msr) * (i + 2));
+ policy->msr[i].index = index;
+ memset(policy->msr[i].policy, 'x', ARRAY_SIZE(policy->msr[0].policy) - 1);
+ policy->msr[i].policy[ARRAY_SIZE(policy->msr[0].policy) - 1] = '\0';
+ policy->msr[i + 1].index = XC_MSR_INPUT_UNUSED;
+
+ return &policy->msr[i];
+}
+
+static int msr_add(libxl_cpuid_policy_list *policy, uint32_t index, unsigned int bit,
+ const char *val)
+{
+ struct xc_msr *entry = msr_find_match(policy, index);
+
+ /* Only allow options taking a character for MSRs, no values allowed. */
+ if (strlen(val) != 1)
+ return 3;
+
+ switch (val[0]) {
+ case '0':
+ case '1':
+ case 'x':
+ case 'k':
+ entry->policy[63 - bit] = val[0];
+ break;
+
+ case 's':
+ /* Translate s -> k as xc_msr doesn't support the deprecated 's'. */
+ entry->policy[63 - bit] = 'k';
+ break;
+
+ default:
+ return 3;
+ }
+
+ return 0;
+}
+
struct feature_name {
const char *name;
unsigned int bit;
@@ -337,7 +388,15 @@ int libxl_cpuid_parse_config(libxl_cpuid_policy_list *policy, const char* str)
}
case FEAT_MSR:
- return 2;
+ {
+ unsigned int bit = feat->bit % 32;
+
+ if (feature_to_policy[feat->bit / 32].msr.reg == CPUID_REG_EDX)
+ bit += 32;
+
+ return msr_add(policy, feature_to_policy[feat->bit / 32].msr.index,
+ bit, val);
+ }
}
return 2;
--
2.41.0
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v2 1/6] libs/guest: introduce support for setting guest MSRs
2023-07-11 9:22 ` [PATCH v2 1/6] libs/guest: introduce support for setting guest MSRs Roger Pau Monne
@ 2023-07-12 14:34 ` Anthony PERARD
2023-07-17 14:20 ` Roger Pau Monné
2023-07-13 15:14 ` Andrew Cooper
1 sibling, 1 reply; 21+ messages in thread
From: Anthony PERARD @ 2023-07-12 14:34 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: xen-devel, Wei Liu, Juergen Gross
On Tue, Jul 11, 2023 at 11:22:25AM +0200, Roger Pau Monne wrote:
> diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
> index 5b035223f4f5..5e5c8124dd74 100644
> --- a/tools/libs/guest/xg_cpuid_x86.c
> +++ b/tools/libs/guest/xg_cpuid_x86.c
> +static int xc_msr_policy(xc_interface *xch, domid_t domid,
> + const struct xc_msr *msr)
> +{
[...]
> +
> + rc = -ENOMEM;
This `rc' value looks unused, should it be moved to the if true block?
> + if ( (host = calloc(nr_msrs, sizeof(*host))) == NULL ||
> + (def = calloc(nr_msrs, sizeof(*def))) == NULL ||
> + (cur = calloc(nr_msrs, sizeof(*cur))) == NULL )
> + {
> + ERROR("Unable to allocate memory for %u CPUID leaves", nr_leaves);
> + goto fail;
> + }
> +
> + /* Get the domain's current policy. */
> + nr_leaves = 0;
> + nr_cur = nr_msrs;
> + rc = get_domain_cpu_policy(xch, domid, &nr_leaves, NULL, &nr_cur, cur);
[...]
> + for ( ; msr->index != XC_MSR_INPUT_UNUSED; ++msr )
> + {
> + xen_msr_entry_t *cur_msr = find_msr(cur, nr_cur, msr->index);
> + const xen_msr_entry_t *def_msr = find_msr(def, nr_def, msr->index);
> + const xen_msr_entry_t *host_msr = find_msr(host, nr_host, msr->index);
> + unsigned int i;
> +
> + if ( cur_msr == NULL || def_msr == NULL || host_msr == NULL )
> + {
> + ERROR("Missing MSR %#x", msr->index);
> + rc = -ENOENT;
> + goto fail;
> + }
> +
> + for ( i = 0; i < ARRAY_SIZE(msr->policy) - 1; i++ )
> + {
> + bool val;
> +
> + if ( msr->policy[i] == '1' )
> + val = true;
> + else if ( msr->policy[i] == '0' )
> + val = false;
> + else if ( msr->policy[i] == 'x' )
> + val = test_bit(63 - i, &def_msr->val);
> + else if ( msr->policy[i] == 'k' )
> + val = test_bit(63 - i, &host_msr->val);
> + else
> + {
> + ERROR("Bad character '%c' in policy string '%s'",
> + msr->policy[i], msr->policy);
Would it be useful to also display msr->index?
> + rc = -EINVAL;
> + goto fail;
> + }
> +
> + clear_bit(63 - i, &cur_msr->val);
> + if ( val )
> + set_bit(63 - i, &cur_msr->val);
Does this need to be first clear then set? A opposed to just do one or
the other.
> + }
> + }
> +
> + /* Feed the transformed policy back up to Xen. */
> + rc = xc_set_domain_cpu_policy(xch, domid, 0, NULL, nr_cur, cur,
> + &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;
> + }
> +
> + /* Success! */
> +
> + fail:
Even if this label is only used on "fail", the code path is also use on
success. So a label named "out" might be more appropriate.
> + free(cur);
> + free(def);
> + free(host);
> +
> + return rc;
> +}
> +
In any case, patch looks fine to me:
Acked-by: Anthony PERARD <anthony.perard@citrix.com>
Thanks,
--
Anthony PERARD
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 2/6] libxl: change the type of libxl_cpuid_policy_list
2023-07-11 9:22 ` [PATCH v2 2/6] libxl: change the type of libxl_cpuid_policy_list Roger Pau Monne
@ 2023-07-12 16:02 ` Anthony PERARD
2023-07-17 14:28 ` Roger Pau Monné
0 siblings, 1 reply; 21+ messages in thread
From: Anthony PERARD @ 2023-07-12 16:02 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: xen-devel, Wei Liu, Juergen Gross
On Tue, Jul 11, 2023 at 11:22:26AM +0200, Roger Pau Monne wrote:
> -void libxl_cpuid_dispose(libxl_cpuid_policy_list *p_cpuid_list)
> +void libxl_cpuid_dispose(libxl_cpuid_policy_list *pl)
> {
> - int i, j;
> - libxl_cpuid_policy_list cpuid_list = *p_cpuid_list;
> + libxl_cpuid_policy_list policy = *pl;
>
> - if (cpuid_list == NULL)
> + if (policy == NULL)
> return;
> - for (i = 0; cpuid_list[i].input[0] != XEN_CPUID_INPUT_UNUSED; i++) {
> - for (j = 0; j < 4; j++)
> - if (cpuid_list[i].policy[j] != NULL) {
> - free(cpuid_list[i].policy[j]);
> - cpuid_list[i].policy[j] = NULL;
> - }
> +
> + if (policy->cpuid) {
> + unsigned int i, j;
> + struct xc_xend_cpuid *cpuid_list = policy->cpuid;
> +
> + for (i = 0; cpuid_list[i].input[0] != XEN_CPUID_INPUT_UNUSED; i++) {
> + for (j = 0; j < 4; j++)
> + if (cpuid_list[i].policy[j] != NULL) {
> + free(cpuid_list[i].policy[j]);
> + cpuid_list[i].policy[j] = NULL;
> + }
This looks like a lot of work. We could just call
free(cpuid_list[i].policy[j]) and that's all, as cpuid_list will be gone
right after the loop.
Also, please add {} for the second "for ()" loop.
> + }
> + free(policy->cpuid);
> }
Beside some the coding style pointing out, the patch looks fine:
Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>
Thanks,
--
Anthony PERARD
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 3/6] libxl: introduce MSR data in libxl_cpuid_policy
2023-07-11 9:22 ` [PATCH v2 3/6] libxl: introduce MSR data in libxl_cpuid_policy Roger Pau Monne
@ 2023-07-12 16:39 ` Anthony PERARD
2023-07-17 14:36 ` Roger Pau Monné
0 siblings, 1 reply; 21+ messages in thread
From: Anthony PERARD @ 2023-07-12 16:39 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: xen-devel, Wei Liu, Juergen Gross
On Tue, Jul 11, 2023 at 11:22:27AM +0200, Roger Pau Monne wrote:
> Add a new array field to libxl_cpuid_policy in order to store the MSR
> policies.
>
> Note that libxl_cpuid_policy_list_{copy,length,parse_json,gen_json}
> are not adjusted to deal with the new MSR array now part of
> libxl_cpuid_policy_list.
Why? Isn't this going to be an issue? Or maybe that going to be dealt
with in a future patch?
>
> Adding the MSR data in the libxl_cpuid_policy_list type is done so
> that existing users can seamlessly pass MSR features as part of the
> CPUID data, without requiring the introduction of a separate
> domain_build_info field, and a new set of handlers functions.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> tools/libs/light/libxl_cpuid.c | 6 +++++-
> tools/libs/light/libxl_internal.h | 1 +
> 2 files changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/tools/libs/light/libxl_cpuid.c b/tools/libs/light/libxl_cpuid.c
> index 724cb4f182d4..65cad28c3ef0 100644
> --- a/tools/libs/light/libxl_cpuid.c
> +++ b/tools/libs/light/libxl_cpuid.c
> @@ -40,6 +40,9 @@ void libxl_cpuid_dispose(libxl_cpuid_policy_list *pl)
> free(policy->cpuid);
> }
>
> + if (policy->msr)
You don't need to test for NULL, you can call free() in this case as
well.
> + free(policy->msr);
> +
> free(policy);
> *pl = NULL;
> return;
--
Anthony PERARD
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 4/6] libxl: split logic to parse user provided CPUID features
2023-07-11 9:22 ` [PATCH v2 4/6] libxl: split logic to parse user provided CPUID features Roger Pau Monne
@ 2023-07-12 16:57 ` Anthony PERARD
0 siblings, 0 replies; 21+ messages in thread
From: Anthony PERARD @ 2023-07-12 16:57 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: xen-devel, Wei Liu, Juergen Gross
On Tue, Jul 11, 2023 at 11:22:28AM +0200, Roger Pau Monne wrote:
> Move the CPUID value parsers out of libxl_cpuid_parse_config() into a
> newly created cpuid_add() local helper. This is in preparation for
> also adding MSR feature parsing support.
>
> No functional change intended.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>
Thanks,
--
Anthony PERARD
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 5/6] libxl: use the cpuid feature names from cpufeatureset.h
2023-07-11 9:22 ` [PATCH v2 5/6] libxl: use the cpuid feature names from cpufeatureset.h Roger Pau Monne
@ 2023-07-12 17:23 ` Anthony PERARD
0 siblings, 0 replies; 21+ messages in thread
From: Anthony PERARD @ 2023-07-12 17:23 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: xen-devel, Wei Liu, Juergen Gross
On Tue, Jul 11, 2023 at 11:22:29AM +0200, Roger Pau Monne wrote:
> The current implementation in libxl_cpuid_parse_config() requires
> keeping a list of cpuid feature bits that should be mostly in sync
> with the contents of cpufeatureset.h.
>
> Avoid such duplication by using the automatically generated list of
> cpuid features in INIT_FEATURE_NAMES in order to map feature names to
> featureset bits, and then translate from featureset bits into cpuid
> leaf, subleaf, register tuple.
>
> Note that the full contents of the previous cpuid translation table
> can't be removed. That's because some feature names allowed by libxl
> are not described in the featuresets, or because naming has diverged
> and the previous nomenclature is preserved for compatibility reasons.
>
> Should result in no functional change observed by callers, albeit some
> new cpuid features will be available as a result of the change.
>
> While there constify cpuid_flags name field.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>
Thanks,
--
Anthony PERARD
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 6/6] libxl: add support for parsing MSR features
2023-07-11 9:22 ` [PATCH v2 6/6] libxl: add support for parsing MSR features Roger Pau Monne
@ 2023-07-13 10:39 ` Anthony PERARD
2023-07-17 14:46 ` Roger Pau Monné
0 siblings, 1 reply; 21+ messages in thread
From: Anthony PERARD @ 2023-07-13 10:39 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: xen-devel, Wei Liu, Juergen Gross
On Tue, Jul 11, 2023 at 11:22:30AM +0200, Roger Pau Monne wrote:
> diff --git a/tools/libs/light/libxl_cpuid.c b/tools/libs/light/libxl_cpuid.c
> index b1c4f8f2f45b..86a08f29a19c 100644
> --- a/tools/libs/light/libxl_cpuid.c
> +++ b/tools/libs/light/libxl_cpuid.c
> @@ -158,6 +158,57 @@ static int cpuid_add(libxl_cpuid_policy_list *policy,
> return 0;
> }
>
> +static struct xc_msr *msr_find_match(libxl_cpuid_policy_list *pl, uint32_t index)
> +{
> + unsigned int i = 0;
> + libxl_cpuid_policy_list policy = *pl;
> +
> + if (policy == NULL)
> + policy = *pl = calloc(1, sizeof(*policy));
> +
> + if (policy->msr != NULL)
> + for (i = 0; policy->msr[i].index != XC_MSR_INPUT_UNUSED; i++)
Could you add { } for this two blocks? One line after a if() without { }
is ok, but not more.
> + if (policy->msr[i].index == index)
> + return &policy->msr[i];
> +
> + policy->msr = realloc(policy->msr, sizeof(struct xc_msr) * (i + 2));
> + policy->msr[i].index = index;
> + memset(policy->msr[i].policy, 'x', ARRAY_SIZE(policy->msr[0].policy) - 1);
Is this "array_size() - 1" correct? The -1 need to go, right?
> + policy->msr[i].policy[ARRAY_SIZE(policy->msr[0].policy) - 1] = '\0';
Is it for convenience? Maybe for easier debugging (printf)? Also, I
guess having a NUL at the end mean the -1 on the previous statement kind
of useful.
> + policy->msr[i + 1].index = XC_MSR_INPUT_UNUSED;
> +
> + return &policy->msr[i];
> +}
Thanks,
--
Anthony PERARD
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 1/6] libs/guest: introduce support for setting guest MSRs
2023-07-11 9:22 ` [PATCH v2 1/6] libs/guest: introduce support for setting guest MSRs Roger Pau Monne
2023-07-12 14:34 ` Anthony PERARD
@ 2023-07-13 15:14 ` Andrew Cooper
2023-07-17 13:48 ` Roger Pau Monné
1 sibling, 1 reply; 21+ messages in thread
From: Andrew Cooper @ 2023-07-13 15:14 UTC (permalink / raw)
To: Roger Pau Monne, xen-devel; +Cc: Wei Liu, Anthony PERARD, Juergen Gross
On 11/07/2023 10:22 am, Roger Pau Monne wrote:
> diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
> index 5b035223f4f5..5e5c8124dd74 100644
> --- a/tools/libs/guest/xg_cpuid_x86.c
> +++ b/tools/libs/guest/xg_cpuid_x86.c
> @@ -423,10 +423,169 @@ static int xc_cpuid_xend_policy(
> return rc;
> }
>
> +static int compare_msr(const void *l, const void *r)
> +{
> + const xen_msr_entry_t *lhs = l;
> + const xen_msr_entry_t *rhs = r;
> +
> + if ( lhs->idx == rhs->idx )
> + return 0;
> +
> + return lhs->idx < rhs->idx ? -1 : 1;
The sum total of logic here is just
return lhs->idx - rhs->idx;
(I think. Double check which way around the subtraction works.)
You don't need to return -1, 0 or 1. You only need negative, 0 or positive.
~Andrew
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 1/6] libs/guest: introduce support for setting guest MSRs
2023-07-13 15:14 ` Andrew Cooper
@ 2023-07-17 13:48 ` Roger Pau Monné
0 siblings, 0 replies; 21+ messages in thread
From: Roger Pau Monné @ 2023-07-17 13:48 UTC (permalink / raw)
To: Andrew Cooper; +Cc: xen-devel, Wei Liu, Anthony PERARD, Juergen Gross
On Thu, Jul 13, 2023 at 04:14:30PM +0100, Andrew Cooper wrote:
> On 11/07/2023 10:22 am, Roger Pau Monne wrote:
> > diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
> > index 5b035223f4f5..5e5c8124dd74 100644
> > --- a/tools/libs/guest/xg_cpuid_x86.c
> > +++ b/tools/libs/guest/xg_cpuid_x86.c
> > @@ -423,10 +423,169 @@ static int xc_cpuid_xend_policy(
> > return rc;
> > }
> >
> > +static int compare_msr(const void *l, const void *r)
> > +{
> > + const xen_msr_entry_t *lhs = l;
> > + const xen_msr_entry_t *rhs = r;
> > +
> > + if ( lhs->idx == rhs->idx )
> > + return 0;
> > +
> > + return lhs->idx < rhs->idx ? -1 : 1;
>
> The sum total of logic here is just
>
> return lhs->idx - rhs->idx;
>
> (I think. Double check which way around the subtraction works.)
Since MSR index is a 32bit value, what about one index being ~0u and
the other 0u: the result would then wrongly be -1 ((int)(~0u - 0u)),
when it should instead be a positive value to denote the left hand
side is greater than the right hand side.
Thanks, Roger.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 1/6] libs/guest: introduce support for setting guest MSRs
2023-07-12 14:34 ` Anthony PERARD
@ 2023-07-17 14:20 ` Roger Pau Monné
0 siblings, 0 replies; 21+ messages in thread
From: Roger Pau Monné @ 2023-07-17 14:20 UTC (permalink / raw)
To: Anthony PERARD; +Cc: xen-devel, Wei Liu, Juergen Gross
On Wed, Jul 12, 2023 at 03:34:23PM +0100, Anthony PERARD wrote:
> On Tue, Jul 11, 2023 at 11:22:25AM +0200, Roger Pau Monne wrote:
> > diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
> > index 5b035223f4f5..5e5c8124dd74 100644
> > --- a/tools/libs/guest/xg_cpuid_x86.c
> > +++ b/tools/libs/guest/xg_cpuid_x86.c
> > +static int xc_msr_policy(xc_interface *xch, domid_t domid,
> > + const struct xc_msr *msr)
> > +{
> [...]
> > +
> > + rc = -ENOMEM;
>
> This `rc' value looks unused, should it be moved to the if true block?
This is mostly copy&paste from the cpuid counterpart, some parts of
the code tend to use this kind of error assignation, but it only makes
sense when the if is the code block is a single line in order to avoid
the braces. Since here the code block already requires braces, doing
the assign outside is pointless.
> > + if ( (host = calloc(nr_msrs, sizeof(*host))) == NULL ||
> > + (def = calloc(nr_msrs, sizeof(*def))) == NULL ||
> > + (cur = calloc(nr_msrs, sizeof(*cur))) == NULL )
> > + {
> > + ERROR("Unable to allocate memory for %u CPUID leaves", nr_leaves);
> > + goto fail;
> > + }
> > +
> > + /* Get the domain's current policy. */
> > + nr_leaves = 0;
> > + nr_cur = nr_msrs;
> > + rc = get_domain_cpu_policy(xch, domid, &nr_leaves, NULL, &nr_cur, cur);
> [...]
> > + for ( ; msr->index != XC_MSR_INPUT_UNUSED; ++msr )
> > + {
> > + xen_msr_entry_t *cur_msr = find_msr(cur, nr_cur, msr->index);
> > + const xen_msr_entry_t *def_msr = find_msr(def, nr_def, msr->index);
> > + const xen_msr_entry_t *host_msr = find_msr(host, nr_host, msr->index);
> > + unsigned int i;
> > +
> > + if ( cur_msr == NULL || def_msr == NULL || host_msr == NULL )
> > + {
> > + ERROR("Missing MSR %#x", msr->index);
> > + rc = -ENOENT;
> > + goto fail;
> > + }
> > +
> > + for ( i = 0; i < ARRAY_SIZE(msr->policy) - 1; i++ )
> > + {
> > + bool val;
> > +
> > + if ( msr->policy[i] == '1' )
> > + val = true;
> > + else if ( msr->policy[i] == '0' )
> > + val = false;
> > + else if ( msr->policy[i] == 'x' )
> > + val = test_bit(63 - i, &def_msr->val);
> > + else if ( msr->policy[i] == 'k' )
> > + val = test_bit(63 - i, &host_msr->val);
> > + else
> > + {
> > + ERROR("Bad character '%c' in policy string '%s'",
> > + msr->policy[i], msr->policy);
>
> Would it be useful to also display msr->index?
Why not, will add it.
> > + rc = -EINVAL;
> > + goto fail;
> > + }
> > +
> > + clear_bit(63 - i, &cur_msr->val);
> > + if ( val )
> > + set_bit(63 - i, &cur_msr->val);
>
> Does this need to be first clear then set? A opposed to just do one or
> the other.
Will adjust, I assume some people prefer 3 lines instead of 4 if you
use and if ... else ...?
> > + }
> > + }
> > +
> > + /* Feed the transformed policy back up to Xen. */
> > + rc = xc_set_domain_cpu_policy(xch, domid, 0, NULL, nr_cur, cur,
> > + &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;
> > + }
> > +
> > + /* Success! */
> > +
> > + fail:
>
> Even if this label is only used on "fail", the code path is also use on
> success. So a label named "out" might be more appropriate.
>
> > + free(cur);
> > + free(def);
> > + free(host);
> > +
> > + return rc;
> > +}
> > +
>
> In any case, patch looks fine to me:
> Acked-by: Anthony PERARD <anthony.perard@citrix.com>
Will make the adjustments requested.
Thanks, Roger.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 2/6] libxl: change the type of libxl_cpuid_policy_list
2023-07-12 16:02 ` Anthony PERARD
@ 2023-07-17 14:28 ` Roger Pau Monné
0 siblings, 0 replies; 21+ messages in thread
From: Roger Pau Monné @ 2023-07-17 14:28 UTC (permalink / raw)
To: Anthony PERARD; +Cc: xen-devel, Wei Liu, Juergen Gross
On Wed, Jul 12, 2023 at 05:02:03PM +0100, Anthony PERARD wrote:
> On Tue, Jul 11, 2023 at 11:22:26AM +0200, Roger Pau Monne wrote:
> > -void libxl_cpuid_dispose(libxl_cpuid_policy_list *p_cpuid_list)
> > +void libxl_cpuid_dispose(libxl_cpuid_policy_list *pl)
> > {
> > - int i, j;
> > - libxl_cpuid_policy_list cpuid_list = *p_cpuid_list;
> > + libxl_cpuid_policy_list policy = *pl;
> >
> > - if (cpuid_list == NULL)
> > + if (policy == NULL)
> > return;
> > - for (i = 0; cpuid_list[i].input[0] != XEN_CPUID_INPUT_UNUSED; i++) {
> > - for (j = 0; j < 4; j++)
> > - if (cpuid_list[i].policy[j] != NULL) {
> > - free(cpuid_list[i].policy[j]);
> > - cpuid_list[i].policy[j] = NULL;
> > - }
> > +
> > + if (policy->cpuid) {
> > + unsigned int i, j;
> > + struct xc_xend_cpuid *cpuid_list = policy->cpuid;
> > +
> > + for (i = 0; cpuid_list[i].input[0] != XEN_CPUID_INPUT_UNUSED; i++) {
> > + for (j = 0; j < 4; j++)
> > + if (cpuid_list[i].policy[j] != NULL) {
> > + free(cpuid_list[i].policy[j]);
> > + cpuid_list[i].policy[j] = NULL;
> > + }
>
> This looks like a lot of work. We could just call
> free(cpuid_list[i].policy[j]) and that's all, as cpuid_list will be gone
> right after the loop.
I wasn't planning on changing the code, as this is just indentation
movement, but will do.
> Also, please add {} for the second "for ()" loop.
>
> > + }
> > + free(policy->cpuid);
> > }
>
> Beside some the coding style pointing out, the patch looks fine:
> Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>
Thanks, Roger.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 3/6] libxl: introduce MSR data in libxl_cpuid_policy
2023-07-12 16:39 ` Anthony PERARD
@ 2023-07-17 14:36 ` Roger Pau Monné
2023-07-18 11:14 ` Anthony PERARD
0 siblings, 1 reply; 21+ messages in thread
From: Roger Pau Monné @ 2023-07-17 14:36 UTC (permalink / raw)
To: Anthony PERARD; +Cc: xen-devel, Wei Liu, Juergen Gross
On Wed, Jul 12, 2023 at 05:39:01PM +0100, Anthony PERARD wrote:
> On Tue, Jul 11, 2023 at 11:22:27AM +0200, Roger Pau Monne wrote:
> > Add a new array field to libxl_cpuid_policy in order to store the MSR
> > policies.
> >
> > Note that libxl_cpuid_policy_list_{copy,length,parse_json,gen_json}
> > are not adjusted to deal with the new MSR array now part of
> > libxl_cpuid_policy_list.
>
> Why? Isn't this going to be an issue? Or maybe that going to be dealt
> with in a future patch?
I'm unsure what's the point of those? The CPUID/MSR data is passed as
a migration stream record from the hypervisor, so I don't see much
point into converting it into json. It also seems quite complex, and
can't likely we done without breaking (or adjusting) the current
format.
My plan was to leave this unimplemented and if someone is in
interested in having the data in json they can as well implement it.
Would you be OK if I add a note to the commit message that
implementing json marshalling is left to implement for interested
parties?
> >
> > Adding the MSR data in the libxl_cpuid_policy_list type is done so
> > that existing users can seamlessly pass MSR features as part of the
> > CPUID data, without requiring the introduction of a separate
> > domain_build_info field, and a new set of handlers functions.
> >
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > ---
> > tools/libs/light/libxl_cpuid.c | 6 +++++-
> > tools/libs/light/libxl_internal.h | 1 +
> > 2 files changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/libs/light/libxl_cpuid.c b/tools/libs/light/libxl_cpuid.c
> > index 724cb4f182d4..65cad28c3ef0 100644
> > --- a/tools/libs/light/libxl_cpuid.c
> > +++ b/tools/libs/light/libxl_cpuid.c
> > @@ -40,6 +40,9 @@ void libxl_cpuid_dispose(libxl_cpuid_policy_list *pl)
> > free(policy->cpuid);
> > }
> >
> > + if (policy->msr)
>
> You don't need to test for NULL, you can call free() in this case as
> well.
Right, thanks, will adjust.
Roger.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 6/6] libxl: add support for parsing MSR features
2023-07-13 10:39 ` Anthony PERARD
@ 2023-07-17 14:46 ` Roger Pau Monné
2023-07-18 11:26 ` Anthony PERARD
0 siblings, 1 reply; 21+ messages in thread
From: Roger Pau Monné @ 2023-07-17 14:46 UTC (permalink / raw)
To: Anthony PERARD; +Cc: xen-devel, Wei Liu, Juergen Gross
On Thu, Jul 13, 2023 at 11:39:53AM +0100, Anthony PERARD wrote:
> On Tue, Jul 11, 2023 at 11:22:30AM +0200, Roger Pau Monne wrote:
> > diff --git a/tools/libs/light/libxl_cpuid.c b/tools/libs/light/libxl_cpuid.c
> > index b1c4f8f2f45b..86a08f29a19c 100644
> > --- a/tools/libs/light/libxl_cpuid.c
> > +++ b/tools/libs/light/libxl_cpuid.c
> > @@ -158,6 +158,57 @@ static int cpuid_add(libxl_cpuid_policy_list *policy,
> > return 0;
> > }
> >
> > +static struct xc_msr *msr_find_match(libxl_cpuid_policy_list *pl, uint32_t index)
> > +{
> > + unsigned int i = 0;
> > + libxl_cpuid_policy_list policy = *pl;
> > +
> > + if (policy == NULL)
> > + policy = *pl = calloc(1, sizeof(*policy));
> > +
> > + if (policy->msr != NULL)
> > + for (i = 0; policy->msr[i].index != XC_MSR_INPUT_UNUSED; i++)
>
> Could you add { } for this two blocks? One line after a if() without { }
> is ok, but not more.
Sure.
> > + if (policy->msr[i].index == index)
> > + return &policy->msr[i];
> > +
> > + policy->msr = realloc(policy->msr, sizeof(struct xc_msr) * (i + 2));
> > + policy->msr[i].index = index;
> > + memset(policy->msr[i].policy, 'x', ARRAY_SIZE(policy->msr[0].policy) - 1);
>
> Is this "array_size() - 1" correct? The -1 need to go, right?
>
> > + policy->msr[i].policy[ARRAY_SIZE(policy->msr[0].policy) - 1] = '\0';
>
> Is it for convenience? Maybe for easier debugging (printf)? Also, I
> guess having a NUL at the end mean the -1 on the previous statement kind
> of useful.
Yes, it's also to match the format of the policy string used by
xc_xend_cpuid, which also has a terminating zero.
Are you OK with this?
Thanks, Roger.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 3/6] libxl: introduce MSR data in libxl_cpuid_policy
2023-07-17 14:36 ` Roger Pau Monné
@ 2023-07-18 11:14 ` Anthony PERARD
0 siblings, 0 replies; 21+ messages in thread
From: Anthony PERARD @ 2023-07-18 11:14 UTC (permalink / raw)
To: Roger Pau Monné; +Cc: xen-devel, Wei Liu, Juergen Gross
On Mon, Jul 17, 2023 at 04:36:11PM +0200, Roger Pau Monné wrote:
> On Wed, Jul 12, 2023 at 05:39:01PM +0100, Anthony PERARD wrote:
> > On Tue, Jul 11, 2023 at 11:22:27AM +0200, Roger Pau Monne wrote:
> > > Add a new array field to libxl_cpuid_policy in order to store the MSR
> > > policies.
> > >
> > > Note that libxl_cpuid_policy_list_{copy,length,parse_json,gen_json}
> > > are not adjusted to deal with the new MSR array now part of
> > > libxl_cpuid_policy_list.
> >
> > Why? Isn't this going to be an issue? Or maybe that going to be dealt
> > with in a future patch?
>
> I'm unsure what's the point of those? The CPUID/MSR data is passed as
> a migration stream record from the hypervisor, so I don't see much
> point into converting it into json. It also seems quite complex, and
> can't likely we done without breaking (or adjusting) the current
> format.
I think this data is used when the machine reboots. I've only try to
edit the file "/var/lib/xen/userdata-d.*" and it change which cpu flags
where available in the machine after a reboot. You could maybe confirm
that when you change an msr value, it is actually conserved across
reboot.
> My plan was to leave this unimplemented and if someone is in
> interested in having the data in json they can as well implement it.
>
> Would you be OK if I add a note to the commit message that
> implementing json marshalling is left to implement for interested
> parties?
My guess is, we will have to think of something and add the msr into
json, even if it's done in a separate patch.
Cheers,
--
Anthony PERARD
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 6/6] libxl: add support for parsing MSR features
2023-07-17 14:46 ` Roger Pau Monné
@ 2023-07-18 11:26 ` Anthony PERARD
0 siblings, 0 replies; 21+ messages in thread
From: Anthony PERARD @ 2023-07-18 11:26 UTC (permalink / raw)
To: Roger Pau Monné; +Cc: xen-devel, Wei Liu, Juergen Gross
On Mon, Jul 17, 2023 at 04:46:25PM +0200, Roger Pau Monné wrote:
> On Thu, Jul 13, 2023 at 11:39:53AM +0100, Anthony PERARD wrote:
> > On Tue, Jul 11, 2023 at 11:22:30AM +0200, Roger Pau Monne wrote:
> > > diff --git a/tools/libs/light/libxl_cpuid.c b/tools/libs/light/libxl_cpuid.c
> > > index b1c4f8f2f45b..86a08f29a19c 100644
> > > --- a/tools/libs/light/libxl_cpuid.c
> > > +++ b/tools/libs/light/libxl_cpuid.c
> > > @@ -158,6 +158,57 @@ static int cpuid_add(libxl_cpuid_policy_list *policy,
> > > return 0;
> > > }
> > >
> > > +static struct xc_msr *msr_find_match(libxl_cpuid_policy_list *pl, uint32_t index)
> > > +{
> > > + unsigned int i = 0;
> > > + libxl_cpuid_policy_list policy = *pl;
> > > +
> > > + if (policy == NULL)
> > > + policy = *pl = calloc(1, sizeof(*policy));
> > > +
> > > + if (policy->msr != NULL)
> > > + for (i = 0; policy->msr[i].index != XC_MSR_INPUT_UNUSED; i++)
> >
> > Could you add { } for this two blocks? One line after a if() without { }
> > is ok, but not more.
>
> Sure.
>
> > > + if (policy->msr[i].index == index)
> > > + return &policy->msr[i];
> > > +
> > > + policy->msr = realloc(policy->msr, sizeof(struct xc_msr) * (i + 2));
> > > + policy->msr[i].index = index;
> > > + memset(policy->msr[i].policy, 'x', ARRAY_SIZE(policy->msr[0].policy) - 1);
> >
> > Is this "array_size() - 1" correct? The -1 need to go, right?
> >
> > > + policy->msr[i].policy[ARRAY_SIZE(policy->msr[0].policy) - 1] = '\0';
> >
> > Is it for convenience? Maybe for easier debugging (printf)? Also, I
> > guess having a NUL at the end mean the -1 on the previous statement kind
> > of useful.
>
> Yes, it's also to match the format of the policy string used by
> xc_xend_cpuid, which also has a terminating zero.
>
> Are you OK with this?
Yes.
With the other style change done:
Acked-by: Anthony PERARD <anthony.perard@citrix.com>
Thanks,
--
Anthony PERARD
^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2023-07-18 11:26 UTC | newest]
Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-07-11 9:22 [PATCH v2 0/6] lib{xc,xl}: support for guest MSR features Roger Pau Monne
2023-07-11 9:22 ` [PATCH v2 1/6] libs/guest: introduce support for setting guest MSRs Roger Pau Monne
2023-07-12 14:34 ` Anthony PERARD
2023-07-17 14:20 ` Roger Pau Monné
2023-07-13 15:14 ` Andrew Cooper
2023-07-17 13:48 ` Roger Pau Monné
2023-07-11 9:22 ` [PATCH v2 2/6] libxl: change the type of libxl_cpuid_policy_list Roger Pau Monne
2023-07-12 16:02 ` Anthony PERARD
2023-07-17 14:28 ` Roger Pau Monné
2023-07-11 9:22 ` [PATCH v2 3/6] libxl: introduce MSR data in libxl_cpuid_policy Roger Pau Monne
2023-07-12 16:39 ` Anthony PERARD
2023-07-17 14:36 ` Roger Pau Monné
2023-07-18 11:14 ` Anthony PERARD
2023-07-11 9:22 ` [PATCH v2 4/6] libxl: split logic to parse user provided CPUID features Roger Pau Monne
2023-07-12 16:57 ` Anthony PERARD
2023-07-11 9:22 ` [PATCH v2 5/6] libxl: use the cpuid feature names from cpufeatureset.h Roger Pau Monne
2023-07-12 17:23 ` Anthony PERARD
2023-07-11 9:22 ` [PATCH v2 6/6] libxl: add support for parsing MSR features Roger Pau Monne
2023-07-13 10:39 ` Anthony PERARD
2023-07-17 14:46 ` Roger Pau Monné
2023-07-18 11:26 ` Anthony PERARD
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.