On 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.vall...@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.
