* [PATCH 0/3] Clean the policy manipulation path in domain creation
@ 2024-02-07 17:39 Alejandro Vallejo
2024-02-07 17:39 ` [PATCH 1/3] tools/xg: Move xc_cpu_policy_t to xenguest.h Alejandro Vallejo
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Alejandro Vallejo @ 2024-02-07 17:39 UTC (permalink / raw
To: Xen-devel
Cc: Alejandro Vallejo, Wei Liu, Anthony PERARD, Juergen Gross,
Jan Beulich, Andrew Cooper, Roger Pau Monné
In the context of creating a domain, we currently issue a lot of hypercalls
redundantly while populating its CPU policy; likely a side effect of
organic growth more than anything else.
However, the worst part is not the overhead (this is a glacially cold
path), but the insane amounts of boilerplate that make it really hard to
pick apart what's going on. One major contributor to this situation is the
fact that what's effectively "setup" and "teardown" phases in policy
manipulation are not factored out from the functions that perform said
manipulations, leading to the same getters and setter being invoked many
times, when once each would do.
Another big contributor is the code being unaware of when a policy is
serialised and when it's not.
This patch attempts to alleviate this situation, yielding over 200 LoC
reduction.
Patch 1: Mechanical change. Makes xc_cpu_policy_t public so it's usable
from clients of libxc/libxg.
Patch 2: Changes the (de)serialization wrappers in xenguest so they always
serialise to/from the internal buffers of xc_cpu_policy_t. The
struct is suitably expanded to hold extra information required.
Patch 3: Performs the refactor of the policy manipulation code so that it
follows a strict: PULL_POLICIES, MUTATE_POLICY (n times), PUSH_POLICY.
Alejandro Vallejo (3):
tools/xg: Move xc_cpu_policy_t to xenguest.h
tools/xg: Streamline cpu policy serialise/deserialise calls
tools/xg: Clean up xend-style overrides for CPU policies
tools/include/xenguest.h | 14 +-
tools/libs/guest/xg_cpuid_x86.c | 500 +++++++++------------------
tools/libs/guest/xg_private.h | 10 -
tools/libs/guest/xg_sr_common_x86.c | 54 +--
tools/misc/xen-cpuid.c | 40 +--
xen/include/xen/lib/x86/cpu-policy.h | 6 +-
6 files changed, 197 insertions(+), 427 deletions(-)
--
2.34.1
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/3] tools/xg: Move xc_cpu_policy_t to xenguest.h
2024-02-07 17:39 [PATCH 0/3] Clean the policy manipulation path in domain creation Alejandro Vallejo
@ 2024-02-07 17:39 ` Alejandro Vallejo
2024-04-30 14:13 ` Anthony PERARD
2024-02-07 17:39 ` [PATCH 2/3] tools/xg: Streamline cpu policy serialise/deserialise calls Alejandro Vallejo
2024-02-07 17:39 ` [PATCH 3/3] tools/xg: Clean up xend-style overrides for CPU policies Alejandro Vallejo
2 siblings, 1 reply; 10+ messages in thread
From: Alejandro Vallejo @ 2024-02-07 17:39 UTC (permalink / raw
To: Xen-devel
Cc: Alejandro Vallejo, Wei Liu, Anthony PERARD, Juergen Gross,
Jan Beulich, Andrew Cooper, Roger Pau Monné
This enables a set of follow-up simplifications in the toolstack.
No functional change.
Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
---
tools/include/xenguest.h | 8 +++++++-
tools/libs/guest/xg_private.h | 10 ----------
xen/include/xen/lib/x86/cpu-policy.h | 6 ++++--
3 files changed, 11 insertions(+), 13 deletions(-)
diff --git a/tools/include/xenguest.h b/tools/include/xenguest.h
index e01f494b772a..4e9078fdee4d 100644
--- a/tools/include/xenguest.h
+++ b/tools/include/xenguest.h
@@ -784,7 +784,13 @@ xen_pfn_t *xc_map_m2p(xc_interface *xch,
unsigned long *mfn0);
#if defined(__i386__) || defined(__x86_64__)
-typedef struct xc_cpu_policy xc_cpu_policy_t;
+#include <xen/lib/x86/cpu-policy.h>
+
+typedef struct xc_cpu_policy {
+ struct cpu_policy policy;
+ xen_cpuid_leaf_t leaves[CPUID_MAX_SERIALISED_LEAVES];
+ xen_msr_entry_t msrs[MSR_MAX_SERIALISED_ENTRIES];
+} xc_cpu_policy_t;
/* Create and free a xc_cpu_policy object. */
xc_cpu_policy_t *xc_cpu_policy_init(void);
diff --git a/tools/libs/guest/xg_private.h b/tools/libs/guest/xg_private.h
index d73947094f2e..d1940f1ea482 100644
--- a/tools/libs/guest/xg_private.h
+++ b/tools/libs/guest/xg_private.h
@@ -170,14 +170,4 @@ int pin_table(xc_interface *xch, unsigned int type, unsigned long mfn,
#define M2P_SIZE(_m) ROUNDUP(((_m) * sizeof(xen_pfn_t)), M2P_SHIFT)
#define M2P_CHUNKS(_m) (M2P_SIZE((_m)) >> M2P_SHIFT)
-#if defined(__x86_64__) || defined(__i386__)
-#include <xen/lib/x86/cpu-policy.h>
-
-struct xc_cpu_policy {
- struct cpu_policy policy;
- xen_cpuid_leaf_t leaves[CPUID_MAX_SERIALISED_LEAVES];
- xen_msr_entry_t msrs[MSR_MAX_SERIALISED_ENTRIES];
-};
-#endif /* x86 */
-
#endif /* XG_PRIVATE_H */
diff --git a/xen/include/xen/lib/x86/cpu-policy.h b/xen/include/xen/lib/x86/cpu-policy.h
index d5e447e9dc06..0cd454dfc0b6 100644
--- a/xen/include/xen/lib/x86/cpu-policy.h
+++ b/xen/include/xen/lib/x86/cpu-policy.h
@@ -85,6 +85,8 @@ unsigned int x86_cpuid_lookup_vendor(uint32_t ebx, uint32_t ecx, uint32_t edx);
*/
const char *x86_cpuid_vendor_to_str(unsigned int vendor);
+#define XENLIB_MAX(x, y) ((x) < (y) ? (y) : (x))
+
#define CPUID_GUEST_NR_BASIC (0xdu + 1)
#define CPUID_GUEST_NR_CACHE (5u + 1)
#define CPUID_GUEST_NR_FEAT (2u + 1)
@@ -92,8 +94,8 @@ const char *x86_cpuid_vendor_to_str(unsigned int vendor);
#define CPUID_GUEST_NR_XSTATE (62u + 1)
#define CPUID_GUEST_NR_EXTD_INTEL (0x8u + 1)
#define CPUID_GUEST_NR_EXTD_AMD (0x21u + 1)
-#define CPUID_GUEST_NR_EXTD MAX(CPUID_GUEST_NR_EXTD_INTEL, \
- CPUID_GUEST_NR_EXTD_AMD)
+#define CPUID_GUEST_NR_EXTD XENLIB_MAX(CPUID_GUEST_NR_EXTD_INTEL, \
+ CPUID_GUEST_NR_EXTD_AMD)
/*
* Maximum number of leaves a struct cpu_policy turns into when serialised for
--
2.34.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/3] tools/xg: Streamline cpu policy serialise/deserialise calls
2024-02-07 17:39 [PATCH 0/3] Clean the policy manipulation path in domain creation Alejandro Vallejo
2024-02-07 17:39 ` [PATCH 1/3] tools/xg: Move xc_cpu_policy_t to xenguest.h Alejandro Vallejo
@ 2024-02-07 17:39 ` Alejandro Vallejo
2024-04-30 14:20 ` Anthony PERARD
2024-02-07 17:39 ` [PATCH 3/3] tools/xg: Clean up xend-style overrides for CPU policies Alejandro Vallejo
2 siblings, 1 reply; 10+ messages in thread
From: Alejandro Vallejo @ 2024-02-07 17:39 UTC (permalink / raw
To: Xen-devel
Cc: Alejandro Vallejo, Wei Liu, Anthony PERARD, Juergen Gross,
Jan Beulich, Andrew Cooper, Roger Pau Monné
The idea is to use xc_cpu_policy_t as a single object containing both the
serialised and deserialised forms of the policy. Note that we need lengths
for the arrays, as the serialised policies may be shorter than the array
capacities.
* Add the serialised lengths to the struct so we can distinguish
between length and capacity of the serialisation buffers.
* Remove explicit buffer+lengths in serialise/deserialise calls
and use the internal buffer inside xc_cpu_policy_t instead.
* Refactor everything to use the new serialisation functions.
* Remove redundant serialization calls and avoid allocating dynamic
memory aside from the policy objects in xen-cpuid. Also minor cleanup
in the policy print call sites.
No functional change intended.
Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
---
Splitting this patch in several is quite annoying, as it implies
artificially creating duplicates of the (de)serialization functions, and
applying them little by little. The resulting diffs are not much better
than this.
---
tools/include/xenguest.h | 6 +--
tools/libs/guest/xg_cpuid_x86.c | 62 ++++++++++++++---------------
tools/libs/guest/xg_sr_common_x86.c | 54 +++++++++----------------
tools/misc/xen-cpuid.c | 40 +++++--------------
4 files changed, 61 insertions(+), 101 deletions(-)
diff --git a/tools/include/xenguest.h b/tools/include/xenguest.h
index 4e9078fdee4d..5c287b4a210c 100644
--- a/tools/include/xenguest.h
+++ b/tools/include/xenguest.h
@@ -789,7 +789,9 @@ xen_pfn_t *xc_map_m2p(xc_interface *xch,
typedef struct xc_cpu_policy {
struct cpu_policy policy;
xen_cpuid_leaf_t leaves[CPUID_MAX_SERIALISED_LEAVES];
+ uint32_t nr_leaves;
xen_msr_entry_t msrs[MSR_MAX_SERIALISED_ENTRIES];
+ uint32_t nr_msrs;
} xc_cpu_policy_t;
/* Create and free a xc_cpu_policy object. */
@@ -805,9 +807,7 @@ 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_serialise(xc_interface *xch, xc_cpu_policy_t *policy);
int xc_cpu_policy_update_cpuid(xc_interface *xch, xc_cpu_policy_t *policy,
const xen_cpuid_leaf_t *leaves,
uint32_t nr);
diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
index 4453178100ad..5699a26b946e 100644
--- a/tools/libs/guest/xg_cpuid_x86.c
+++ b/tools/libs/guest/xg_cpuid_x86.c
@@ -834,14 +834,13 @@ void xc_cpu_policy_destroy(xc_cpu_policy_t *policy)
}
}
-static int deserialize_policy(xc_interface *xch, xc_cpu_policy_t *policy,
- unsigned int nr_leaves, unsigned int nr_entries)
+static int deserialize_policy(xc_interface *xch, xc_cpu_policy_t *policy)
{
uint32_t err_leaf = -1, err_subleaf = -1, err_msr = -1;
int rc;
rc = x86_cpuid_copy_from_buffer(&policy->policy, policy->leaves,
- nr_leaves, &err_leaf, &err_subleaf);
+ policy->nr_leaves, &err_leaf, &err_subleaf);
if ( rc )
{
if ( err_leaf != -1 )
@@ -851,7 +850,7 @@ static int deserialize_policy(xc_interface *xch, xc_cpu_policy_t *policy,
}
rc = x86_msr_copy_from_buffer(&policy->policy, policy->msrs,
- nr_entries, &err_msr);
+ policy->nr_msrs, &err_msr);
if ( rc )
{
if ( err_msr != -1 )
@@ -878,7 +877,10 @@ int xc_cpu_policy_get_system(xc_interface *xch, unsigned int policy_idx,
return rc;
}
- rc = deserialize_policy(xch, policy, nr_leaves, nr_msrs);
+ policy->nr_leaves = nr_leaves;
+ policy->nr_msrs = nr_msrs;
+
+ rc = deserialize_policy(xch, policy);
if ( rc )
{
errno = -rc;
@@ -903,7 +905,10 @@ int xc_cpu_policy_get_domain(xc_interface *xch, uint32_t domid,
return rc;
}
- rc = deserialize_policy(xch, policy, nr_leaves, nr_msrs);
+ policy->nr_leaves = nr_leaves;
+ policy->nr_msrs = nr_msrs;
+
+ rc = deserialize_policy(xch, policy);
if ( rc )
{
errno = -rc;
@@ -917,17 +922,14 @@ int xc_cpu_policy_set_domain(xc_interface *xch, uint32_t domid,
xc_cpu_policy_t *policy)
{
uint32_t err_leaf = -1, err_subleaf = -1, err_msr = -1;
- unsigned int nr_leaves = ARRAY_SIZE(policy->leaves);
- unsigned int nr_msrs = ARRAY_SIZE(policy->msrs);
int rc;
- rc = xc_cpu_policy_serialise(xch, policy, policy->leaves, &nr_leaves,
- policy->msrs, &nr_msrs);
+ rc = xc_cpu_policy_serialise(xch, policy);
if ( rc )
return rc;
- rc = xc_set_domain_cpu_policy(xch, domid, nr_leaves, policy->leaves,
- nr_msrs, policy->msrs,
+ rc = xc_set_domain_cpu_policy(xch, domid, policy->nr_leaves, policy->leaves,
+ policy->nr_msrs, policy->msrs,
&err_leaf, &err_subleaf, &err_msr);
if ( rc )
{
@@ -942,34 +944,32 @@ int xc_cpu_policy_set_domain(xc_interface *xch, uint32_t domid,
return rc;
}
-int xc_cpu_policy_serialise(xc_interface *xch, const xc_cpu_policy_t *p,
- xen_cpuid_leaf_t *leaves, uint32_t *nr_leaves,
- xen_msr_entry_t *msrs, uint32_t *nr_msrs)
+int xc_cpu_policy_serialise(xc_interface *xch, xc_cpu_policy_t *p)
{
+ unsigned int nr_leaves = ARRAY_SIZE(p->leaves);
+ unsigned int nr_msrs = ARRAY_SIZE(p->msrs);
int rc;
- if ( leaves )
+ rc = x86_cpuid_copy_to_buffer(&p->policy, p->leaves, &nr_leaves);
+ if ( rc )
{
- rc = x86_cpuid_copy_to_buffer(&p->policy, leaves, nr_leaves);
- if ( rc )
- {
- ERROR("Failed to serialize CPUID policy");
- errno = -rc;
- return -1;
- }
+ ERROR("Failed to serialize CPUID policy");
+ errno = -rc;
+ return -1;
}
- if ( msrs )
+ p->nr_leaves = nr_leaves;
+
+ rc = x86_msr_copy_to_buffer(&p->policy, p->msrs, &nr_msrs);
+ if ( rc )
{
- rc = x86_msr_copy_to_buffer(&p->policy, msrs, nr_msrs);
- if ( rc )
- {
- ERROR("Failed to serialize MSR policy");
- errno = -rc;
- return -1;
- }
+ ERROR("Failed to serialize MSR policy");
+ errno = -rc;
+ return -1;
}
+ p->nr_msrs = nr_msrs;
+
errno = 0;
return 0;
}
diff --git a/tools/libs/guest/xg_sr_common_x86.c b/tools/libs/guest/xg_sr_common_x86.c
index 563b4f016877..832047756e58 100644
--- a/tools/libs/guest/xg_sr_common_x86.c
+++ b/tools/libs/guest/xg_sr_common_x86.c
@@ -1,4 +1,5 @@
#include "xg_sr_common_x86.h"
+#include "xg_sr_stream_format.h"
int write_x86_tsc_info(struct xc_sr_context *ctx)
{
@@ -45,54 +46,37 @@ int handle_x86_tsc_info(struct xc_sr_context *ctx, struct xc_sr_record *rec)
int write_x86_cpu_policy_records(struct xc_sr_context *ctx)
{
xc_interface *xch = ctx->xch;
- struct xc_sr_record cpuid = { .type = REC_TYPE_X86_CPUID_POLICY, };
- struct xc_sr_record msrs = { .type = REC_TYPE_X86_MSR_POLICY, };
- uint32_t nr_leaves = 0, nr_msrs = 0;
- xc_cpu_policy_t *policy = NULL;
+ struct xc_sr_record record;
+ xc_cpu_policy_t *policy = xc_cpu_policy_init();
int rc;
- if ( xc_cpu_policy_get_size(xch, &nr_leaves, &nr_msrs) < 0 )
- {
- PERROR("Unable to get CPU Policy size");
- return -1;
- }
-
- cpuid.data = malloc(nr_leaves * sizeof(xen_cpuid_leaf_t));
- msrs.data = malloc(nr_msrs * sizeof(xen_msr_entry_t));
- policy = xc_cpu_policy_init();
- if ( !cpuid.data || !msrs.data || !policy )
- {
- ERROR("Cannot allocate memory for CPU Policy");
- rc = -1;
- goto out;
- }
-
- if ( xc_cpu_policy_get_domain(xch, ctx->domid, policy) )
+ if ( !policy || xc_cpu_policy_get_domain(xch, ctx->domid, policy) )
{
PERROR("Unable to get d%d CPU Policy", ctx->domid);
rc = -1;
goto out;
}
- if ( xc_cpu_policy_serialise(xch, policy, cpuid.data, &nr_leaves,
- msrs.data, &nr_msrs) )
- {
- PERROR("Unable to serialize d%d CPU Policy", ctx->domid);
- rc = -1;
- goto out;
- }
- cpuid.length = nr_leaves * sizeof(xen_cpuid_leaf_t);
- if ( cpuid.length )
+ record = (struct xc_sr_record) {
+ .type = REC_TYPE_X86_CPUID_POLICY,
+ .data = policy->leaves,
+ .length = policy->nr_leaves * sizeof(*policy->leaves),
+ };
+ if ( record.length )
{
- rc = write_record(ctx, &cpuid);
+ rc = write_record(ctx, &record);
if ( rc )
goto out;
}
- msrs.length = nr_msrs * sizeof(xen_msr_entry_t);
- if ( msrs.length )
+ record = (struct xc_sr_record) {
+ .type = REC_TYPE_X86_MSR_POLICY,
+ .data = policy->msrs,
+ .length = policy->nr_msrs * sizeof(*policy->msrs),
+ };
+ if ( record.length )
{
- rc = write_record(ctx, &msrs);
+ rc = write_record(ctx, &record);
if ( rc )
goto out;
}
@@ -100,8 +84,6 @@ int write_x86_cpu_policy_records(struct xc_sr_context *ctx)
rc = 0;
out:
- free(cpuid.data);
- free(msrs.data);
xc_cpu_policy_destroy(policy);
return rc;
diff --git a/tools/misc/xen-cpuid.c b/tools/misc/xen-cpuid.c
index 46abdb914092..1d4761d9ad9f 100644
--- a/tools/misc/xen-cpuid.c
+++ b/tools/misc/xen-cpuid.c
@@ -403,17 +403,18 @@ static void dump_info(xc_interface *xch, bool detail)
free(fs);
}
-static void print_policy(const char *name,
- xen_cpuid_leaf_t *leaves, uint32_t nr_leaves,
- xen_msr_entry_t *msrs, uint32_t nr_msrs)
+static void print_policy(const char *name, xc_cpu_policy_t *policy)
{
- unsigned int l;
+ const xen_cpuid_leaf_t *leaves = policy->leaves;
+ const xen_msr_entry_t *msrs = policy->msrs;
+ uint32_t nr_leaves = policy->nr_leaves;
+ uint32_t nr_msrs = policy->nr_msrs;
printf("%s policy: %u leaves, %u MSRs\n", name, nr_leaves, nr_msrs);
printf(" CPUID:\n");
printf(" %-8s %-8s -> %-8s %-8s %-8s %-8s\n",
"leaf", "subleaf", "eax", "ebx", "ecx", "edx");
- for ( l = 0; l < nr_leaves; ++l )
+ for ( uint32_t l = 0; l < nr_leaves; ++l )
{
/* Skip empty leaves. */
if ( !leaves[l].a && !leaves[l].b && !leaves[l].c && !leaves[l].d )
@@ -426,7 +427,7 @@ static void print_policy(const char *name,
printf(" MSRs:\n");
printf(" %-8s -> %-16s\n", "index", "value");
- for ( l = 0; l < nr_msrs; ++l )
+ for ( uint32_t l = 0; l < nr_msrs; ++l )
printf(" %08x -> %016"PRIx64"\n",
msrs[l].idx, msrs[l].val);
}
@@ -533,8 +534,6 @@ int main(int argc, char **argv)
[ XEN_SYSCTL_cpu_policy_pv_default ] = "PV Default",
[ XEN_SYSCTL_cpu_policy_hvm_default ] = "HVM Default",
};
- xen_cpuid_leaf_t *leaves;
- xen_msr_entry_t *msrs;
uint32_t i, max_leaves, max_msrs;
xc_interface *xch = xc_interface_open(0, 0, 0);
@@ -551,36 +550,21 @@ int main(int argc, char **argv)
printf("Xen reports there are maximum %u leaves and %u MSRs\n",
max_leaves, max_msrs);
- leaves = calloc(max_leaves, sizeof(xen_cpuid_leaf_t));
- if ( !leaves )
- err(1, "calloc(max_leaves)");
- msrs = calloc(max_msrs, sizeof(xen_msr_entry_t));
- if ( !msrs )
- err(1, "calloc(max_msrs)");
-
if ( domid != -1 )
{
char name[20];
- uint32_t nr_leaves = max_leaves;
- uint32_t nr_msrs = max_msrs;
if ( xc_cpu_policy_get_domain(xch, domid, policy) )
err(1, "xc_cpu_policy_get_domain(, %d, )", domid);
- if ( xc_cpu_policy_serialise(xch, policy, leaves, &nr_leaves,
- msrs, &nr_msrs) )
- err(1, "xc_cpu_policy_serialise");
snprintf(name, sizeof(name), "Domain %d", domid);
- print_policy(name, leaves, nr_leaves, msrs, nr_msrs);
+ print_policy(name, policy);
}
else
{
/* Get system policies */
for ( i = 0; i < ARRAY_SIZE(sys_policies); ++i )
{
- uint32_t nr_leaves = max_leaves;
- uint32_t nr_msrs = max_msrs;
-
if ( xc_cpu_policy_get_system(xch, i, policy) )
{
if ( errno == EOPNOTSUPP )
@@ -592,18 +576,12 @@ int main(int argc, char **argv)
err(1, "xc_cpu_policy_get_system(, %s, )", sys_policies[i]);
}
- if ( xc_cpu_policy_serialise(xch, policy, leaves, &nr_leaves,
- msrs, &nr_msrs) )
- err(1, "xc_cpu_policy_serialise");
- print_policy(sys_policies[i], leaves, nr_leaves,
- msrs, nr_msrs);
+ print_policy(sys_policies[i], policy);
}
}
xc_cpu_policy_destroy(policy);
- free(leaves);
- free(msrs);
xc_interface_close(xch);
}
else if ( mode == MODE_INFO || mode == MODE_DETAIL )
--
2.34.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 3/3] tools/xg: Clean up xend-style overrides for CPU policies
2024-02-07 17:39 [PATCH 0/3] Clean the policy manipulation path in domain creation Alejandro Vallejo
2024-02-07 17:39 ` [PATCH 1/3] tools/xg: Move xc_cpu_policy_t to xenguest.h Alejandro Vallejo
2024-02-07 17:39 ` [PATCH 2/3] tools/xg: Streamline cpu policy serialise/deserialise calls Alejandro Vallejo
@ 2024-02-07 17:39 ` Alejandro Vallejo
2024-04-30 14:42 ` Anthony PERARD
2 siblings, 1 reply; 10+ messages in thread
From: Alejandro Vallejo @ 2024-02-07 17:39 UTC (permalink / raw
To: Xen-devel; +Cc: Alejandro Vallejo, Wei Liu, Anthony PERARD, Juergen Gross
Factor out policy getters/setters from both (CPUID and MSR) policy override
functions. Additionally, use host policy rather than featureset when
preparing the cur policy, saving one hypercall and several lines of
boilerplate.
No functional change intended.
Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
---
tools/libs/guest/xg_cpuid_x86.c | 444 +++++++++-----------------------
1 file changed, 128 insertions(+), 316 deletions(-)
diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
index 5699a26b946e..cee0be80ba5b 100644
--- a/tools/libs/guest/xg_cpuid_x86.c
+++ b/tools/libs/guest/xg_cpuid_x86.c
@@ -36,6 +36,34 @@ enum {
#define bitmaskof(idx) (1u << ((idx) & 31))
#define featureword_of(idx) ((idx) >> 5)
+static int deserialize_policy(xc_interface *xch, xc_cpu_policy_t *policy)
+{
+ uint32_t err_leaf = -1, err_subleaf = -1, err_msr = -1;
+ int rc;
+
+ rc = x86_cpuid_copy_from_buffer(&policy->policy, policy->leaves,
+ policy->nr_leaves, &err_leaf, &err_subleaf);
+ if ( rc )
+ {
+ if ( err_leaf != -1 )
+ ERROR("Failed to deserialise CPUID (err leaf %#x, subleaf %#x) (%d = %s)",
+ err_leaf, err_subleaf, -rc, strerror(-rc));
+ return rc;
+ }
+
+ rc = x86_msr_copy_from_buffer(&policy->policy, policy->msrs,
+ policy->nr_msrs, &err_msr);
+ if ( rc )
+ {
+ if ( err_msr != -1 )
+ ERROR("Failed to deserialise MSR (err MSR %#x) (%d = %s)",
+ err_msr, -rc, strerror(-rc));
+ return rc;
+ }
+
+ return 0;
+}
+
int xc_get_cpu_levelling_caps(xc_interface *xch, uint32_t *caps)
{
struct xen_sysctl sysctl = {};
@@ -260,102 +288,34 @@ static int compare_leaves(const void *l, const void *r)
return 0;
}
-static xen_cpuid_leaf_t *find_leaf(
- xen_cpuid_leaf_t *leaves, unsigned int nr_leaves,
- const struct xc_xend_cpuid *xend)
+static xen_cpuid_leaf_t *find_leaf(xc_cpu_policy_t *p,
+ 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);
+ return bsearch(&key, p->leaves, ARRAY_SIZE(p->leaves),
+ sizeof(*p->leaves), compare_leaves);
}
-static int xc_cpuid_xend_policy(
- xc_interface *xch, uint32_t domid, const struct xc_xend_cpuid *xend)
+static int xc_cpuid_xend_policy(xc_interface *xch, uint32_t domid,
+ const struct xc_xend_cpuid *xend,
+ xc_cpu_policy_t *host,
+ xc_cpu_policy_t *def,
+ xc_cpu_policy_t *cur)
{
- 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_cpuid_leaf_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_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 )
- {
- PERROR("Failed to obtain d%d current policy", domid);
- rc = -errno;
- goto fail;
- }
-
- /* Get the domain type's default policy. */
- nr_msrs = 0;
- nr_def = nr_leaves;
- rc = get_system_cpu_policy(xch, hvm ? XEN_SYSCTL_cpu_policy_hvm_default
- : XEN_SYSCTL_cpu_policy_pv_default,
- &nr_def, def, &nr_msrs, NULL);
- if ( rc )
- {
- PERROR("Failed to obtain %s def policy", hvm ? "hvm" : "pv");
- rc = -errno;
- goto fail;
- }
-
- /* 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);
- if ( rc )
- {
- PERROR("Failed to obtain host policy");
- rc = -errno;
- goto fail;
- }
+ if ( !xend )
+ return 0;
- rc = -EINVAL;
for ( ; xend->leaf != XEN_CPUID_INPUT_UNUSED; ++xend )
{
- xen_cpuid_leaf_t *cur_leaf = find_leaf(cur, nr_cur, xend);
- const xen_cpuid_leaf_t *def_leaf = find_leaf(def, nr_def, xend);
- const xen_cpuid_leaf_t *host_leaf = find_leaf(host, nr_host, xend);
+ xen_cpuid_leaf_t *cur_leaf = find_leaf(cur, xend);
+ const xen_cpuid_leaf_t *def_leaf = find_leaf(def, xend);
+ const xen_cpuid_leaf_t *host_leaf = find_leaf(host, xend);
if ( cur_leaf == NULL || def_leaf == NULL || host_leaf == NULL )
{
ERROR("Missing leaf %#x, subleaf %#x", xend->leaf, xend->subleaf);
- goto fail;
+ return -EINVAL;
}
for ( unsigned int i = 0; i < ARRAY_SIZE(xend->policy); i++ )
@@ -384,7 +344,7 @@ static int xc_cpuid_xend_policy(
{
ERROR("Bad character '%c' in policy[%d] string '%s'",
xend->policy[i][j], i, xend->policy[i]);
- goto fail;
+ return -EINVAL;
}
clear_bit(31 - j, cur_reg);
@@ -394,25 +354,7 @@ static int xc_cpuid_xend_policy(
}
}
- /* Feed the transformed currrent policy back up to Xen. */
- rc = xc_set_domain_cpu_policy(xch, domid, nr_cur, cur, 0, NULL,
- &err_leaf, &err_subleaf, &err_msr);
- 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;
+ return 0;
}
static int compare_msr(const void *l, const void *r)
@@ -426,107 +368,34 @@ static int compare_msr(const void *l, const void *r)
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)
+static xen_msr_entry_t *find_msr(xc_cpu_policy_t *p,
+ uint32_t index)
{
const xen_msr_entry_t key = { .idx = index };
- return bsearch(&key, msrs, nr_msrs, sizeof(*msrs), compare_msr);
+ return bsearch(&key, p->msrs, ARRAY_SIZE(p->msrs),
+ sizeof(*p->msrs), compare_msr);
}
-
static int xc_msr_policy(xc_interface *xch, domid_t domid,
- const struct xc_msr *msr)
+ const struct xc_msr *msr,
+ xc_cpu_policy_t *host,
+ xc_cpu_policy_t *def,
+ xc_cpu_policy_t *cur)
{
- 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 out;
- }
- 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 out;
- }
-
- 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);
- rc = -ENOMEM;
- goto out;
- }
-
- /* 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 out;
- }
-
- /* 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 out;
- }
-
- /* 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 out;
- }
-
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;
+ xen_msr_entry_t *cur_msr = find_msr(cur, msr->index);
+ const xen_msr_entry_t *def_msr = find_msr(def, msr->index);
+ const xen_msr_entry_t *host_msr = find_msr(host, msr->index);
if ( cur_msr == NULL || def_msr == NULL || host_msr == NULL )
{
ERROR("Missing MSR %#x", msr->index);
- rc = -ENOENT;
- goto out;
+ return -ENOENT;
}
- for ( i = 0; i < ARRAY_SIZE(msr->policy) - 1; i++ )
+ for ( size_t i = 0; i < ARRAY_SIZE(msr->policy) - 1; i++ )
{
bool val;
@@ -542,8 +411,7 @@ static int xc_msr_policy(xc_interface *xch, domid_t domid,
{
ERROR("MSR index %#x: bad character '%c' in policy string '%s'",
msr->index, msr->policy[i], msr->policy);
- rc = -EINVAL;
- goto out;
+ return -EINVAL;
}
if ( val )
@@ -553,25 +421,7 @@ static int xc_msr_policy(xc_interface *xch, domid_t domid,
}
}
- /* 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 out;
- }
-
- /* Success! */
-
- out:
- free(cur);
- free(def);
- free(host);
-
- return rc;
+ return 0;
}
int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
@@ -583,14 +433,16 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
int rc;
bool hvm;
xc_domaininfo_t di;
- struct xc_cpu_policy *p = xc_cpu_policy_init();
- unsigned int i, nr_leaves = ARRAY_SIZE(p->leaves), nr_msrs = 0;
- 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 ( !p )
- return -ENOMEM;
+ struct xc_cpu_policy *host = xc_cpu_policy_init();
+ struct xc_cpu_policy *def = xc_cpu_policy_init();
+ struct xc_cpu_policy *cur = xc_cpu_policy_init();
+
+ if ( !host || !def || !cur )
+ {
+ rc = -ENOMEM;
+ goto out;
+ }
if ( (rc = xc_domain_getinfo_single(xch, domid, &di)) < 0 )
{
@@ -600,21 +452,19 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
}
hvm = di.flags & XEN_DOMINF_hvm_guest;
- /* Get the host policy. */
- rc = xc_get_cpu_featureset(xch, XEN_SYSCTL_cpu_featureset_host,
- &len, host_featureset);
- /* Tolerate "buffer too small", as we've got the bits we need. */
- if ( rc && errno != ENOBUFS )
+ /* Get the raw host policy */
+ rc = xc_cpu_policy_get_system(xch, XEN_SYSCTL_cpu_policy_host, host);
+ if ( rc )
{
- PERROR("Failed to obtain host featureset");
+ PERROR("Failed to obtain host policy");
rc = -errno;
goto out;
}
/* Get the domain's default policy. */
- rc = get_system_cpu_policy(xch, hvm ? XEN_SYSCTL_cpu_policy_hvm_default
- : XEN_SYSCTL_cpu_policy_pv_default,
- &nr_leaves, p->leaves, &nr_msrs, NULL);
+ 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 default policy", hvm ? "hvm" : "pv");
@@ -622,14 +472,8 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
goto out;
}
- rc = x86_cpuid_copy_from_buffer(&p->policy, 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;
- }
+ /* Copy the deserialised default policy to modify it */
+ memcpy(cur, def, sizeof(*cur));
if ( restore )
{
@@ -647,18 +491,18 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
* - Re-enable features which have become (possibly) off by default.
*/
- p->policy.basic.rdrand = test_bit(X86_FEATURE_RDRAND, host_featureset);
- p->policy.feat.hle = test_bit(X86_FEATURE_HLE, host_featureset);
- p->policy.feat.rtm = test_bit(X86_FEATURE_RTM, host_featureset);
+ cur->policy.basic.rdrand = host->policy.basic.rdrand;
+ cur->policy.feat.hle = host->policy.feat.hle;
+ cur->policy.feat.rtm = host->policy.feat.rtm;
if ( hvm )
{
- p->policy.feat.mpx = test_bit(X86_FEATURE_MPX, host_featureset);
+ cur->policy.feat.mpx = host->policy.feat.mpx;
}
- p->policy.basic.max_leaf = min(p->policy.basic.max_leaf, 0xdu);
- p->policy.feat.max_subleaf = 0;
- p->policy.extd.max_leaf = min(p->policy.extd.max_leaf, 0x8000001c);
+ cur->policy.basic.max_leaf = min(cur->policy.basic.max_leaf, 0xdu);
+ cur->policy.feat.max_subleaf = 0;
+ cur->policy.extd.max_leaf = min(cur->policy.extd.max_leaf, 0x8000001c);
}
if ( featureset )
@@ -666,7 +510,7 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
uint32_t disabled_features[FEATURESET_NR_ENTRIES],
feat[FEATURESET_NR_ENTRIES] = {};
static const uint32_t deep_features[] = INIT_DEEP_FEATURES;
- unsigned int i, b;
+ unsigned int b;
/*
* The user supplied featureset may be shorter or longer than
@@ -677,14 +521,14 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
/* Check for truncated set bits. */
rc = -EOPNOTSUPP;
- for ( i = user_len; i < nr_features; ++i )
+ for ( size_t 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 )
+ for ( size_t 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 )
@@ -695,24 +539,24 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
!(dfs = x86_cpu_policy_lookup_deep_deps(b)) )
continue;
- for ( i = 0; i < ARRAY_SIZE(disabled_features); ++i )
+ for ( size_t i = 0; i < ARRAY_SIZE(disabled_features); ++i )
{
feat[i] &= ~dfs[i];
disabled_features[i] &= ~dfs[i];
}
}
- x86_cpu_featureset_to_policy(feat, &p->policy);
+ x86_cpu_featureset_to_policy(feat, &cur->policy);
}
else
{
- p->policy.extd.itsc = itsc;
+ cur->policy.extd.itsc = itsc;
if ( hvm )
{
- p->policy.basic.pae = pae;
- p->policy.basic.vmx = nested_virt;
- p->policy.extd.svm = nested_virt;
+ cur->policy.basic.pae = pae;
+ cur->policy.basic.vmx = nested_virt;
+ cur->policy.extd.svm = nested_virt;
}
}
@@ -722,8 +566,8 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
* On hardware without CPUID Faulting, PV guests see real topology.
* As a consequence, they also need to see the host htt/cmp fields.
*/
- p->policy.basic.htt = test_bit(X86_FEATURE_HTT, host_featureset);
- p->policy.extd.cmp_legacy = test_bit(X86_FEATURE_CMP_LEGACY, host_featureset);
+ cur->policy.basic.htt = host->policy.basic.htt;
+ cur->policy.extd.cmp_legacy = host->policy.extd.cmp_legacy;
}
else
{
@@ -731,28 +575,28 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
* 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->policy.basic.htt = true;
- p->policy.extd.cmp_legacy = false;
+ cur->policy.basic.htt = true;
+ cur->policy.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->policy.basic.lppp )
- p->policy.basic.lppp = 2;
- else if ( !(p->policy.basic.lppp & 0x80) )
- p->policy.basic.lppp *= 2;
+ if ( !cur->policy.basic.lppp )
+ cur->policy.basic.lppp = 2;
+ else if ( !(cur->policy.basic.lppp & 0x80) )
+ cur->policy.basic.lppp *= 2;
- switch ( p->policy.x86_vendor )
+ switch ( cur->policy.x86_vendor )
{
case X86_VENDOR_INTEL:
- for ( i = 0; (p->policy.cache.subleaf[i].type &&
- i < ARRAY_SIZE(p->policy.cache.raw)); ++i )
+ for ( size_t i = 0; (cur->policy.cache.subleaf[i].type &&
+ i < ARRAY_SIZE(cur->policy.cache.raw)); ++i )
{
- p->policy.cache.subleaf[i].cores_per_package =
- (p->policy.cache.subleaf[i].cores_per_package << 1) | 1;
- p->policy.cache.subleaf[i].threads_per_cache = 0;
+ cur->policy.cache.subleaf[i].cores_per_package =
+ (cur->policy.cache.subleaf[i].cores_per_package << 1) | 1;
+ cur->policy.cache.subleaf[i].threads_per_cache = 0;
}
break;
@@ -772,49 +616,45 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
* apic_id_size values greater than 7. Limit the value to
* 7 for now.
*/
- if ( p->policy.extd.nc < 0x7f )
+ if ( cur->policy.extd.nc < 0x7f )
{
- if ( p->policy.extd.apic_id_size != 0 && p->policy.extd.apic_id_size < 0x7 )
- p->policy.extd.apic_id_size++;
+ if ( cur->policy.extd.apic_id_size != 0 && cur->policy.extd.apic_id_size < 0x7 )
+ cur->policy.extd.apic_id_size++;
- p->policy.extd.nc = (p->policy.extd.nc << 1) | 1;
+ cur->policy.extd.nc = (cur->policy.extd.nc << 1) | 1;
}
break;
}
}
- nr_leaves = ARRAY_SIZE(p->leaves);
- rc = x86_cpuid_copy_to_buffer(&p->policy, p->leaves, &nr_leaves);
- if ( rc )
+ if ( xend || msr )
{
- ERROR("Failed to serialise CPUID (%d = %s)", -rc, strerror(-rc));
- goto out;
+ // The overrides are over the serialised form of the policy
+ if ( (rc = xc_cpu_policy_serialise(xch, cur)) )
+ goto out;
+
+ if ( (rc = xc_cpuid_xend_policy(xch, domid, xend, host, def, cur)) )
+ goto out;
+ if ( (rc = xc_msr_policy(xch, domid, msr, host, def, cur)) )
+ goto out;
+
+ if ( (rc = deserialize_policy(xch, cur)) )
+ goto out;
}
- rc = xc_set_domain_cpu_policy(xch, domid, nr_leaves, p->leaves, 0, NULL,
- &err_leaf, &err_subleaf, &err_msr);
+ rc = xc_cpu_policy_set_domain(xch, domid, cur);
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;
}
- 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:
- xc_cpu_policy_destroy(p);
+ xc_cpu_policy_destroy(def);
+ xc_cpu_policy_destroy(host);
+ xc_cpu_policy_destroy(cur);
return rc;
}
@@ -834,34 +674,6 @@ void xc_cpu_policy_destroy(xc_cpu_policy_t *policy)
}
}
-static int deserialize_policy(xc_interface *xch, xc_cpu_policy_t *policy)
-{
- uint32_t err_leaf = -1, err_subleaf = -1, err_msr = -1;
- int rc;
-
- rc = x86_cpuid_copy_from_buffer(&policy->policy, policy->leaves,
- policy->nr_leaves, &err_leaf, &err_subleaf);
- if ( rc )
- {
- if ( err_leaf != -1 )
- ERROR("Failed to deserialise CPUID (err leaf %#x, subleaf %#x) (%d = %s)",
- err_leaf, err_subleaf, -rc, strerror(-rc));
- return rc;
- }
-
- rc = x86_msr_copy_from_buffer(&policy->policy, policy->msrs,
- policy->nr_msrs, &err_msr);
- if ( rc )
- {
- if ( err_msr != -1 )
- ERROR("Failed to deserialise MSR (err MSR %#x) (%d = %s)",
- err_msr, -rc, strerror(-rc));
- return rc;
- }
-
- return 0;
-}
-
int xc_cpu_policy_get_system(xc_interface *xch, unsigned int policy_idx,
xc_cpu_policy_t *policy)
{
--
2.34.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] tools/xg: Move xc_cpu_policy_t to xenguest.h
2024-02-07 17:39 ` [PATCH 1/3] tools/xg: Move xc_cpu_policy_t to xenguest.h Alejandro Vallejo
@ 2024-04-30 14:13 ` Anthony PERARD
2024-05-16 16:28 ` Alejandro Vallejo
0 siblings, 1 reply; 10+ messages in thread
From: Anthony PERARD @ 2024-04-30 14:13 UTC (permalink / raw
To: Alejandro Vallejo
Cc: Xen-devel, Wei Liu, Juergen Gross, Jan Beulich, Andrew Cooper,
Roger Pau Monné
On Wed, Feb 07, 2024 at 05:39:55PM +0000, Alejandro Vallejo wrote:
> diff --git a/tools/include/xenguest.h b/tools/include/xenguest.h
> index e01f494b772a..4e9078fdee4d 100644
> --- a/tools/include/xenguest.h
> +++ b/tools/include/xenguest.h
> @@ -784,7 +784,13 @@ xen_pfn_t *xc_map_m2p(xc_interface *xch,
> unsigned long *mfn0);
>
> #if defined(__i386__) || defined(__x86_64__)
> -typedef struct xc_cpu_policy xc_cpu_policy_t;
> +#include <xen/lib/x86/cpu-policy.h>
I don't think it's a good idea to expose "cpu-policy.h" to "xenguest.h",
and it's going to break the build of every tools outside of xen
repository that are using xenguest.h. xenguest.h is installed on a
system, but cpu-policy.h isn't.
> +typedef struct xc_cpu_policy {
> + struct cpu_policy policy;
> + xen_cpuid_leaf_t leaves[CPUID_MAX_SERIALISED_LEAVES];
> + xen_msr_entry_t msrs[MSR_MAX_SERIALISED_ENTRIES];
> +} xc_cpu_policy_t;
Would using an accessor be an option to access `leaves` and `msrs` for
"xen-cpuid" tool? That would avoid the need to expose the "cpu-policy.h"
headers.
With accessors, we might not need to expose xc_cpu_policy_serialise() to
the world anymore, and it could be internal to libxenguest, probably, I
haven't check.
Thanks,
--
Anthony PERARD
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/3] tools/xg: Streamline cpu policy serialise/deserialise calls
2024-02-07 17:39 ` [PATCH 2/3] tools/xg: Streamline cpu policy serialise/deserialise calls Alejandro Vallejo
@ 2024-04-30 14:20 ` Anthony PERARD
0 siblings, 0 replies; 10+ messages in thread
From: Anthony PERARD @ 2024-04-30 14:20 UTC (permalink / raw
To: Alejandro Vallejo
Cc: Xen-devel, Wei Liu, Juergen Gross, Jan Beulich, Andrew Cooper,
Roger Pau Monné
On Wed, Feb 07, 2024 at 05:39:56PM +0000, Alejandro Vallejo wrote:
> The idea is to use xc_cpu_policy_t as a single object containing both the
> serialised and deserialised forms of the policy. Note that we need lengths
> for the arrays, as the serialised policies may be shorter than the array
> capacities.
>
> * Add the serialised lengths to the struct so we can distinguish
> between length and capacity of the serialisation buffers.
> * Remove explicit buffer+lengths in serialise/deserialise calls
> and use the internal buffer inside xc_cpu_policy_t instead.
> * Refactor everything to use the new serialisation functions.
> * Remove redundant serialization calls and avoid allocating dynamic
> memory aside from the policy objects in xen-cpuid. Also minor cleanup
> in the policy print call sites.
>
> No functional change intended.
>
> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
The patch looks fine beside xen-cpuid.c which might need to tweaked due
to change needed in the first patch.
Thanks,
--
Anthony PERARD
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] tools/xg: Clean up xend-style overrides for CPU policies
2024-02-07 17:39 ` [PATCH 3/3] tools/xg: Clean up xend-style overrides for CPU policies Alejandro Vallejo
@ 2024-04-30 14:42 ` Anthony PERARD
2024-05-16 15:37 ` Alejandro Vallejo
0 siblings, 1 reply; 10+ messages in thread
From: Anthony PERARD @ 2024-04-30 14:42 UTC (permalink / raw
To: Alejandro Vallejo; +Cc: Xen-devel, Wei Liu, Juergen Gross
On Wed, Feb 07, 2024 at 05:39:57PM +0000, Alejandro Vallejo wrote:
> diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
> index 5699a26b946e..cee0be80ba5b 100644
> --- a/tools/libs/guest/xg_cpuid_x86.c
> +++ b/tools/libs/guest/xg_cpuid_x86.c
> @@ -772,49 +616,45 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
> * apic_id_size values greater than 7. Limit the value to
> * 7 for now.
> */
> - if ( p->policy.extd.nc < 0x7f )
> + if ( cur->policy.extd.nc < 0x7f )
> {
> - if ( p->policy.extd.apic_id_size != 0 && p->policy.extd.apic_id_size < 0x7 )
> - p->policy.extd.apic_id_size++;
> + if ( cur->policy.extd.apic_id_size != 0 && cur->policy.extd.apic_id_size < 0x7 )
> + cur->policy.extd.apic_id_size++;
>
> - p->policy.extd.nc = (p->policy.extd.nc << 1) | 1;
> + cur->policy.extd.nc = (cur->policy.extd.nc << 1) | 1;
> }
> break;
> }
> }
>
> - nr_leaves = ARRAY_SIZE(p->leaves);
> - rc = x86_cpuid_copy_to_buffer(&p->policy, p->leaves, &nr_leaves);
> - if ( rc )
> + if ( xend || msr )
> {
> - ERROR("Failed to serialise CPUID (%d = %s)", -rc, strerror(-rc));
> - goto out;
> + // The overrides are over the serialised form of the policy
Comments should use /* */
> + if ( (rc = xc_cpu_policy_serialise(xch, cur)) )
> + goto out;
> +
> + if ( (rc = xc_cpuid_xend_policy(xch, domid, xend, host, def, cur)) )
> + goto out;
> + if ( (rc = xc_msr_policy(xch, domid, msr, host, def, cur)) )
> + goto out;
What if `xend` is set, but `msr` isn't? Looks like there's going to be a
segv in xc_msr_policy() because it doesn't check that `msr` is actually
set.
Thanks,
--
Anthony PERARD
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] tools/xg: Clean up xend-style overrides for CPU policies
2024-04-30 14:42 ` Anthony PERARD
@ 2024-05-16 15:37 ` Alejandro Vallejo
2024-05-16 15:58 ` Alejandro Vallejo
0 siblings, 1 reply; 10+ messages in thread
From: Alejandro Vallejo @ 2024-05-16 15:37 UTC (permalink / raw
To: Anthony PERARD; +Cc: Xen-devel, Wei Liu, Juergen Gross
On 30/04/2024 15:42, Anthony PERARD wrote:
> On Wed, Feb 07, 2024 at 05:39:57PM +0000, Alejandro Vallejo wrote:
>> diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
>> index 5699a26b946e..cee0be80ba5b 100644
>> --- a/tools/libs/guest/xg_cpuid_x86.c
>> +++ b/tools/libs/guest/xg_cpuid_x86.c
>> @@ -772,49 +616,45 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
>> * apic_id_size values greater than 7. Limit the value to
>> * 7 for now.
>> */
>> - if ( p->policy.extd.nc < 0x7f )
>> + if ( cur->policy.extd.nc < 0x7f )
>> {
>> - if ( p->policy.extd.apic_id_size != 0 && p->policy.extd.apic_id_size < 0x7 )
>> - p->policy.extd.apic_id_size++;
>> + if ( cur->policy.extd.apic_id_size != 0 && cur->policy.extd.apic_id_size < 0x7 )
>> + cur->policy.extd.apic_id_size++;
>>
>> - p->policy.extd.nc = (p->policy.extd.nc << 1) | 1;
>> + cur->policy.extd.nc = (cur->policy.extd.nc << 1) | 1;
>> }
>> break;
>> }
>> }
>>
>> - nr_leaves = ARRAY_SIZE(p->leaves);
>> - rc = x86_cpuid_copy_to_buffer(&p->policy, p->leaves, &nr_leaves);
>> - if ( rc )
>> + if ( xend || msr )
>> {
>> - ERROR("Failed to serialise CPUID (%d = %s)", -rc, strerror(-rc));
>> - goto out;
>> + // The overrides are over the serialised form of the policy
>
> Comments should use /* */
Ugh, yes.
>
>> + if ( (rc = xc_cpu_policy_serialise(xch, cur)) )
>> + goto out;
>> +
>> + if ( (rc = xc_cpuid_xend_policy(xch, domid, xend, host, def, cur)) )
>> + goto out;
>> + if ( (rc = xc_msr_policy(xch, domid, msr, host, def, cur)) )
>> + goto out;
>
> What if `xend` is set, but `msr` isn't? Looks like there's going to be a
> segv in xc_msr_policy() because it doesn't check that `msr` is actually
> set.
>
>
> Thanks,
>
OOPS! Yes, msrs was meant to have the same check I added for
xc_cpuid_xend_policy. Will do.
Cheers,
Alejandro
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] tools/xg: Clean up xend-style overrides for CPU policies
2024-05-16 15:37 ` Alejandro Vallejo
@ 2024-05-16 15:58 ` Alejandro Vallejo
0 siblings, 0 replies; 10+ messages in thread
From: Alejandro Vallejo @ 2024-05-16 15:58 UTC (permalink / raw
To: anthony; +Cc: Xen-devel
On 16/05/2024 16:37, Alejandro Vallejo wrote:
> On 30/04/2024 15:42, Anthony PERARD wrote:
>> On Wed, Feb 07, 2024 at 05:39:57PM +0000, Alejandro Vallejo wrote:
>>> diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
>>> index 5699a26b946e..cee0be80ba5b 100644
>>> --- a/tools/libs/guest/xg_cpuid_x86.c
>>> +++ b/tools/libs/guest/xg_cpuid_x86.c
>>> @@ -772,49 +616,45 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
>>> * apic_id_size values greater than 7. Limit the value to
>>> * 7 for now.
>>> */
>>> - if ( p->policy.extd.nc < 0x7f )
>>> + if ( cur->policy.extd.nc < 0x7f )
>>> {
>>> - if ( p->policy.extd.apic_id_size != 0 && p->policy.extd.apic_id_size < 0x7 )
>>> - p->policy.extd.apic_id_size++;
>>> + if ( cur->policy.extd.apic_id_size != 0 && cur->policy.extd.apic_id_size < 0x7 )
>>> + cur->policy.extd.apic_id_size++;
>>>
>>> - p->policy.extd.nc = (p->policy.extd.nc << 1) | 1;
>>> + cur->policy.extd.nc = (cur->policy.extd.nc << 1) | 1;
>>> }
>>> break;
>>> }
>>> }
>>>
>>> - nr_leaves = ARRAY_SIZE(p->leaves);
>>> - rc = x86_cpuid_copy_to_buffer(&p->policy, p->leaves, &nr_leaves);
>>> - if ( rc )
>>> + if ( xend || msr )
>>> {
>>> - ERROR("Failed to serialise CPUID (%d = %s)", -rc, strerror(-rc));
>>> - goto out;
>>> + // The overrides are over the serialised form of the policy
>>
>> Comments should use /* */
>
> Ugh, yes.
>
>>
>>> + if ( (rc = xc_cpu_policy_serialise(xch, cur)) )
>>> + goto out;
>>> +
>>> + if ( (rc = xc_cpuid_xend_policy(xch, domid, xend, host, def, cur)) )
>>> + goto out;
>>> + if ( (rc = xc_msr_policy(xch, domid, msr, host, def, cur)) )
>>> + goto out;
>>
>> What if `xend` is set, but `msr` isn't? Looks like there's going to be a
>> segv in xc_msr_policy() because it doesn't check that `msr` is actually
>> set.
>>
>>
>> Thanks,
>>
>
> OOPS! Yes, msrs was meant to have the same check I added for
> xc_cpuid_xend_policy. Will do.
>
> Cheers,
> Alejandro
Ugh answered to the old email. pinging to the xenproject one now.
Cheers,
Alejandro
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] tools/xg: Move xc_cpu_policy_t to xenguest.h
2024-04-30 14:13 ` Anthony PERARD
@ 2024-05-16 16:28 ` Alejandro Vallejo
0 siblings, 0 replies; 10+ messages in thread
From: Alejandro Vallejo @ 2024-05-16 16:28 UTC (permalink / raw
To: Anthony PERARD
Cc: Xen-devel, Wei Liu, Juergen Gross, Jan Beulich, Andrew Cooper,
Roger Pau Monné
On 30/04/2024 15:13, Anthony PERARD wrote:
> On Wed, Feb 07, 2024 at 05:39:55PM +0000, Alejandro Vallejo wrote:
>> diff --git a/tools/include/xenguest.h b/tools/include/xenguest.h
>> index e01f494b772a..4e9078fdee4d 100644
>> --- a/tools/include/xenguest.h
>> +++ b/tools/include/xenguest.h
>> @@ -784,7 +784,13 @@ xen_pfn_t *xc_map_m2p(xc_interface *xch,
>> unsigned long *mfn0);
>>
>> #if defined(__i386__) || defined(__x86_64__)
>> -typedef struct xc_cpu_policy xc_cpu_policy_t;
>> +#include <xen/lib/x86/cpu-policy.h>
>
> I don't think it's a good idea to expose "cpu-policy.h" to "xenguest.h",
> and it's going to break the build of every tools outside of xen
> repository that are using xenguest.h. xenguest.h is installed on a
> system, but cpu-policy.h isn't.
>
>> +typedef struct xc_cpu_policy {
>> + struct cpu_policy policy;
>> + xen_cpuid_leaf_t leaves[CPUID_MAX_SERIALISED_LEAVES];
>> + xen_msr_entry_t msrs[MSR_MAX_SERIALISED_ENTRIES];
>> +} xc_cpu_policy_t;
>
> Would using an accessor be an option to access `leaves` and `msrs` for
> "xen-cpuid" tool? That would avoid the need to expose the "cpu-policy.h"
> headers.
>
> With accessors, we might not need to expose xc_cpu_policy_serialise() to
> the world anymore, and it could be internal to libxenguest, probably, I
> haven't check.
>
> Thanks,
>
That would work out for xen-cpuid.
I'm on the fence in the general case because I think it'd be
advantageous to let clients (i.e: xl) manipulate directly the
deserialized form of the policy. That would allow flipping features on
or off a heck of lot easier. We could create per-feature accessors, but
then we'll end up with _a lot_ of them.
I guess we'll get there when we get there. The xen-cpuid case definitely
doesn't warrant it. I'll give this a go.
Cheers,
Alejandro
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2024-05-16 16:29 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-02-07 17:39 [PATCH 0/3] Clean the policy manipulation path in domain creation Alejandro Vallejo
2024-02-07 17:39 ` [PATCH 1/3] tools/xg: Move xc_cpu_policy_t to xenguest.h Alejandro Vallejo
2024-04-30 14:13 ` Anthony PERARD
2024-05-16 16:28 ` Alejandro Vallejo
2024-02-07 17:39 ` [PATCH 2/3] tools/xg: Streamline cpu policy serialise/deserialise calls Alejandro Vallejo
2024-04-30 14:20 ` Anthony PERARD
2024-02-07 17:39 ` [PATCH 3/3] tools/xg: Clean up xend-style overrides for CPU policies Alejandro Vallejo
2024-04-30 14:42 ` Anthony PERARD
2024-05-16 15:37 ` Alejandro Vallejo
2024-05-16 15:58 ` Alejandro Vallejo
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).