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>
---
v2:
* Removed v1/patch1.
* Added the accessors suggested in feedback.
---
tools/include/xenguest.h | 8 ++-
tools/libs/guest/xg_cpuid_x86.c | 98 ++++++++++++++++++++---------
tools/libs/guest/xg_private.h | 2 +
tools/libs/guest/xg_sr_common_x86.c | 54 ++++++----------
tools/misc/xen-cpuid.c | 43 ++++---------
5 files changed, 104 insertions(+), 101 deletions(-)
diff --git a/tools/include/xenguest.h b/tools/include/xenguest.h
index e01f494b772a..563811cd8dde 100644
--- a/tools/include/xenguest.h
+++ b/tools/include/xenguest.h
@@ -799,14 +799,16 @@ 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);
int xc_cpu_policy_update_msrs(xc_interface *xch, xc_cpu_policy_t *policy,
const xen_msr_entry_t *msrs, uint32_t nr);
+int xc_cpu_policy_get_leaves(xc_interface *xch, const xc_cpu_policy_t *policy,
+ const xen_cpuid_leaf_t **leaves, uint32_t *nr);
+int xc_cpu_policy_get_msrs(xc_interface *xch, const xc_cpu_policy_t *policy,
+ const xen_msr_entry_t **msrs, uint32_t *nr);
/* Compatibility calculations. */
bool xc_cpu_policy_is_compatible(xc_interface *xch, xc_cpu_policy_t *host,
diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
index 4453178100ad..4f4b86b59470 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;
}
@@ -1012,6 +1012,42 @@ int xc_cpu_policy_update_msrs(xc_interface *xch, xc_cpu_policy_t *policy,
return rc;
}
+int xc_cpu_policy_get_leaves(xc_interface *xch,
+ const xc_cpu_policy_t *policy,
+ const xen_cpuid_leaf_t **leaves,
+ uint32_t *nr)
+{
+ if ( !policy )
+ {
+ ERROR("Failed to fetch CPUID leaves from policy object");
+ errno = -EINVAL;
+ return -1;
+ }
+
+ *leaves = policy->leaves;
+ *nr = policy->nr_leaves;
+
+ return 0;
+}
+
+int xc_cpu_policy_get_msrs(xc_interface *xch,
+ const xc_cpu_policy_t *policy,
+ const xen_msr_entry_t **msrs,
+ uint32_t *nr)
+{
+ if ( !policy )
+ {
+ ERROR("Failed to fetch MSRs from policy object");
+ errno = -EINVAL;
+ return -1;
+ }
+
+ *msrs = policy->msrs;
+ *nr = policy->nr_msrs;
+
+ return 0;
+}
+
bool xc_cpu_policy_is_compatible(xc_interface *xch, xc_cpu_policy_t *host,
xc_cpu_policy_t *guest)
{
diff --git a/tools/libs/guest/xg_private.h b/tools/libs/guest/xg_private.h
index d73947094f2e..a65dae818f3d 100644
--- a/tools/libs/guest/xg_private.h
+++ b/tools/libs/guest/xg_private.h
@@ -177,6 +177,8 @@ 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];
+ uint32_t nr_leaves;
+ uint32_t nr_msrs;
};
#endif /* x86 */
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 8893547bebce..1c9ba6d32060 100644
--- a/tools/misc/xen-cpuid.c
+++ b/tools/misc/xen-cpuid.c
@@ -409,17 +409,21 @@ 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(xc_interface *xch, const char *name, const xc_cpu_policy_t *policy)
{
- unsigned int l;
+ const xen_cpuid_leaf_t *leaves;
+ const xen_msr_entry_t *msrs;
+ uint32_t nr_leaves, nr_msrs;
+
+ if ( xc_cpu_policy_get_leaves(xch, policy, &leaves, &nr_leaves) ||
+ xc_cpu_policy_get_msrs(xch, policy, &msrs, &nr_msrs) )
+ err(1, "print_policy()");
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 )
@@ -432,7 +436,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);
}
@@ -539,8 +543,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);
@@ -557,36 +559,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(xch, 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 )
@@ -598,18 +585,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(xch, 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.1On Fri, May 17, 2024 at 05:08:34PM +0100, 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>
> ---
> v2:
> * Removed v1/patch1.
> * Added the accessors suggested in feedback.
> ---
> tools/include/xenguest.h | 8 ++-
> tools/libs/guest/xg_cpuid_x86.c | 98 ++++++++++++++++++++---------
> tools/libs/guest/xg_private.h | 2 +
> tools/libs/guest/xg_sr_common_x86.c | 54 ++++++----------
> tools/misc/xen-cpuid.c | 43 ++++---------
> 5 files changed, 104 insertions(+), 101 deletions(-)
>
> diff --git a/tools/include/xenguest.h b/tools/include/xenguest.h
> index e01f494b772a..563811cd8dde 100644
> --- a/tools/include/xenguest.h
> +++ b/tools/include/xenguest.h
> @@ -799,14 +799,16 @@ 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);
> int xc_cpu_policy_update_msrs(xc_interface *xch, xc_cpu_policy_t *policy,
> const xen_msr_entry_t *msrs, uint32_t nr);
> +int xc_cpu_policy_get_leaves(xc_interface *xch, const xc_cpu_policy_t *policy,
> + const xen_cpuid_leaf_t **leaves, uint32_t *nr);
> +int xc_cpu_policy_get_msrs(xc_interface *xch, const xc_cpu_policy_t *policy,
> + const xen_msr_entry_t **msrs, uint32_t *nr);
>
> /* Compatibility calculations. */
> bool xc_cpu_policy_is_compatible(xc_interface *xch, xc_cpu_policy_t *host,
> diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
> index 4453178100ad..4f4b86b59470 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,
I would be tempted to just pass the policy to
xc_set_domain_cpu_policy() and get rid of the separate cpuid and msrs
serialized arrays, but that hides (or makes it less obvious) that the
policy needs to be serialized before providing to
xc_set_domain_cpu_policy(). Just a rant, no need to change it here.
> &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;
> }
> @@ -1012,6 +1012,42 @@ int xc_cpu_policy_update_msrs(xc_interface *xch, xc_cpu_policy_t *policy,
> return rc;
> }
>
> +int xc_cpu_policy_get_leaves(xc_interface *xch,
> + const xc_cpu_policy_t *policy,
> + const xen_cpuid_leaf_t **leaves,
> + uint32_t *nr)
> +{
> + if ( !policy )
> + {
> + ERROR("Failed to fetch CPUID leaves from policy object");
> + errno = -EINVAL;
> + return -1;
> + }
> +
> + *leaves = policy->leaves;
> + *nr = policy->nr_leaves;
> +
> + return 0;
> +}
> +
> +int xc_cpu_policy_get_msrs(xc_interface *xch,
> + const xc_cpu_policy_t *policy,
> + const xen_msr_entry_t **msrs,
> + uint32_t *nr)
> +{
> + if ( !policy )
> + {
> + ERROR("Failed to fetch MSRs from policy object");
> + errno = -EINVAL;
> + return -1;
> + }
> +
> + *msrs = policy->msrs;
> + *nr = policy->nr_msrs;
> +
> + return 0;
> +}
My preference would probably be to return NULL or
xen_{leaf,msr}_entry_t * from those, as we can then avoid an extra
leaves/msrs parameter. Again I'm fine with leaving it like this.
> +
> bool xc_cpu_policy_is_compatible(xc_interface *xch, xc_cpu_policy_t *host,
> xc_cpu_policy_t *guest)
> {
> diff --git a/tools/libs/guest/xg_private.h b/tools/libs/guest/xg_private.h
> index d73947094f2e..a65dae818f3d 100644
> --- a/tools/libs/guest/xg_private.h
> +++ b/tools/libs/guest/xg_private.h
> @@ -177,6 +177,8 @@ 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];
> + uint32_t nr_leaves;
> + uint32_t nr_msrs;
> };
> #endif /* x86 */
>
> 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;
> }
You could maybe write this as:
if ( policy->nr_leaves )
{
const struct xc_sr_record r = {
.type = REC_TYPE_X86_CPUID_POLICY,
.data = policy->leaves,
.length = policy->nr_leaves * sizeof(*policy->leaves),
};
rc = write_record(ctx, &record);
}
(same for the msr record)
>
> - 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 8893547bebce..1c9ba6d32060 100644
> --- a/tools/misc/xen-cpuid.c
> +++ b/tools/misc/xen-cpuid.c
> @@ -409,17 +409,21 @@ 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(xc_interface *xch, const char *name, const xc_cpu_policy_t *policy)
Line length.
> {
> - unsigned int l;
> + const xen_cpuid_leaf_t *leaves;
> + const xen_msr_entry_t *msrs;
> + uint32_t nr_leaves, nr_msrs;
> +
> + if ( xc_cpu_policy_get_leaves(xch, policy, &leaves, &nr_leaves) ||
> + xc_cpu_policy_get_msrs(xch, policy, &msrs, &nr_msrs) )
> + err(1, "print_policy()");
Shouldn't the error message be "xc_cpu_policy_get_{leaves,msrs}()"
instead, as one of those is the cause of the error?
Other err() usages do print the function triggering the error, not the
function context name.
>
> 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 )
> @@ -432,7 +436,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 )
I would be tempted to leave `l` as-is, seeing as there's no real need
to modify it in the patch context, and the patch is already fairly
long.
Thanks, Roger.
On 20/05/2024 14:47, Roger Pau Monné wrote:
>> @@ -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,
>
> I would be tempted to just pass the policy to
> xc_set_domain_cpu_policy() and get rid of the separate cpuid and msrs
> serialized arrays, but that hides (or makes it less obvious) that the
> policy needs to be serialized before providing to
> xc_set_domain_cpu_policy(). Just a rant, no need to change it here.
I'm still pondering what to do about that. I'd like to refactor all that
faff away as well, but I'm not sure how to do it cleanly yet. The
biggest danger I see is modifying one side of the policy and then wiping
those changes by mistake reserializing or deserializing at the wrong time.
Not for this series, I reckon.
>> +int xc_cpu_policy_get_msrs(xc_interface *xch,
>> + const xc_cpu_policy_t *policy,
>> + const xen_msr_entry_t **msrs,
>> + uint32_t *nr)
>> +{
>> + if ( !policy )
>> + {
>> + ERROR("Failed to fetch MSRs from policy object");
>> + errno = -EINVAL;
>> + return -1;
>> + }
>> +
>> + *msrs = policy->msrs;
>> + *nr = policy->nr_msrs;
>> +
>> + return 0;
>> +}
>
> My preference would probably be to return NULL or
> xen_{leaf,msr}_entry_t * from those, as we can then avoid an extra
> leaves/msrs parameter. Again I'm fine with leaving it like this.
>
It didn't feel right to have an output parameter as the return value
doubling as status code when another output is in the parameter list. I
can perfectly imagine someone grabbing "nr" and ignoring "msrs" because
"msrs" doesn't happen to be needed for them.
I think there's extra safety in making it harder to ignore the error.
>> - 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;
>> }
>
>
> You could maybe write this as:
>
> if ( policy->nr_leaves )
> {
> const struct xc_sr_record r = {
> .type = REC_TYPE_X86_CPUID_POLICY,
> .data = policy->leaves,
> .length = policy->nr_leaves * sizeof(*policy->leaves),
> };
>
> rc = write_record(ctx, &record);
> }
>
> (same for the msr record)
>
Ack. Looks nicer that way.
>>
>> - 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 8893547bebce..1c9ba6d32060 100644
>> --- a/tools/misc/xen-cpuid.c
>> +++ b/tools/misc/xen-cpuid.c
>> @@ -409,17 +409,21 @@ 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(xc_interface *xch, const char *name, const xc_cpu_policy_t *policy)
>
> Line length.
Ack
>
>> {
>> - unsigned int l;
>> + const xen_cpuid_leaf_t *leaves;
>> + const xen_msr_entry_t *msrs;
>> + uint32_t nr_leaves, nr_msrs;
>> +
>> + if ( xc_cpu_policy_get_leaves(xch, policy, &leaves, &nr_leaves) ||
>> + xc_cpu_policy_get_msrs(xch, policy, &msrs, &nr_msrs) )
>> + err(1, "print_policy()");
>
> Shouldn't the error message be "xc_cpu_policy_get_{leaves,msrs}()"
> instead, as one of those is the cause of the error?
>
> Other err() usages do print the function triggering the error, not the
> function context name.
>
Sure. I've demultiplexed them into separate conditionals as well now.
>>
>> 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 )
>> @@ -432,7 +436,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 )
>
> I would be tempted to leave `l` as-is, seeing as there's no real need
> to modify it in the patch context, and the patch is already fairly
> long.
done
Cheers,
Alejandro
© 2016 - 2024 Red Hat, Inc.