Re: [PATCH v4 1/2] tools/xg: Streamline cpu policy serialise/deserialise calls

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 29/05/2024 3:30 pm, Alejandro Vallejo wrote:
> diff --git a/tools/include/xenguest.h b/tools/include/xenguest.h
> index e01f494b772a..85d56f26537b 100644
> --- a/tools/include/xenguest.h
> +++ b/tools/include/xenguest.h
> @@ -799,15 +799,23 @@ 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);
>  
> +/*
> + * Accessors for the serialised forms of the policy. The outputs are pointers
> + * into the policy object and not fresh allocations, so their lifetimes are 
> tied
> + * to the policy object itself.

This is far more complicated.  See below.

> + */
> +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,
>                                   xc_cpu_policy_t *guest);
> diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
> index 4453178100ad..6cab5c60bb41 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 )

Urgh - this is a mess.  (Not your fault, but we really need to think
twice before continuing.)

xc_cpu_policy_serialise() is an exported function and, prior to this
series, used by external entities to get at the content inside the
opaque object.

deserialize_policy()  (Clearly not written by me - Roger?) is a local
helper.  Also it looks wonky in the next patch, although I think that's
just code movement to avoid a forward declaration?

By the end of the series, xc_cpu_policy_serialise() isn't used
externally, but it's still exported.

But, besides the visibility, there's a second difference...


> @@ -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);

... they're asymmetric as to whether the caller or the callee preloads
policy->nr_*.

Both of these need rationalising, one way or another.


But, there's a related problem.

Previously there was only one canonical form (the deserialised form),
and anything operating on state was responsible for getting it back to
being the deserialised form.

Now, there are two forms which are coexist side by side.  The buffer
exposed by get_{cpuid,msr}() is only good until the next operation which
uses what were (previously) the internal staging buffer(s).

And that makes it a fragile and error prone interface.

>      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,32 +944,26 @@ 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)
>  {
>      int rc;
> +    p->nr_leaves = ARRAY_SIZE(p->leaves);
> +    p->nr_msrs = ARRAY_SIZE(p->msrs);
>  
> -    if ( leaves )
> +    rc = x86_cpuid_copy_to_buffer(&p->policy, p->leaves, &p->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 )
> +    rc = x86_msr_copy_to_buffer(&p->policy, p->msrs, &p->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;
>      }
>  
>      errno = 0;
> @@ -1012,6 +1008,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;
> +    }

This check isn't useful, and it's making the interface inconsistent. 
There's no case ever where a NULL policy is meaningful, except for the
very initial failure to allocate, and there it's the return value not an
input parameter.

More importantly however, the error message is misleading as a consequence.

> +
> +    *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;

These need a comment explaining how they're used, and sadly they have no
relationship to the lengths of the array.  There's a corner case where
they can end up larger.

>  };
>  #endif /* x86 */
>  
> diff --git a/tools/libs/guest/xg_sr_common_x86.c 
> b/tools/libs/guest/xg_sr_common_x86.c
> index 563b4f016877..a0d67c3211c6 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"

I'm pretty sure this shouldn't be necessary.  Is it?

>  
>  int write_x86_tsc_info(struct xc_sr_context *ctx)
>  {
> @@ -45,54 +46,39 @@ 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;
> +    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;
> -    }

Wow, the old code here was especially daft.

We're having Xen serialise the policy, copying (double buffering) into
the policy object then desensitising.  And vs the old copy, we've got
rid of the re-serialise into yet another buffer.

But we should still be using a plain XEN_DOMCTL_get_cpu_policy here. 
Literally all we want to do is take the array(s) Xen gave us and feed
them straight into the fd.

deserialising is already a reasonably expensive operation (every
individual leaf coordinate needs re-range checking), and is only ever
going to get worse.

It will probably help to split the changes to
write_x86_cpu_policy_records() out into a separate patch.  It's more
clear cut and also addresses one of the local vs external issues
discussed above.


>  
> -    cpuid.length = nr_leaves * sizeof(xen_cpuid_leaf_t);
> -    if ( cpuid.length )
> +
> +    if ( policy->nr_leaves )
>      {
> -        rc = write_record(ctx, &cpuid);
> +        struct xc_sr_record record = {
> +            .type = REC_TYPE_X86_CPUID_POLICY,
> +            .data = policy->leaves,
> +            .length = policy->nr_leaves * sizeof(*policy->leaves),
> +        };
> +
> +        rc = write_record(ctx, &record);

Please keep this name being cpuid.  It's more helpful when grepping, and
it also shrinks the diff.

>          if ( rc )
>              goto out;
>      }
>  
> -    msrs.length = nr_msrs * sizeof(xen_msr_entry_t);
> -    if ( msrs.length )
> +    if ( policy->nr_msrs )
>      {
> -        rc = write_record(ctx, &msrs);
> +        struct xc_sr_record record = {
> +            .type = REC_TYPE_X86_MSR_POLICY,
> +            .data = policy->msrs,
> +            .length = policy->nr_msrs * sizeof(*policy->msrs),
> +        };
> +
> +        rc = write_record(ctx, &record);
>          if ( rc )
>              goto out;
>      }
> @@ -100,8 +86,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 4c4593528dfe..488f43378406 100644
> --- a/tools/misc/xen-cpuid.c
> +++ b/tools/misc/xen-cpuid.c
> @@ -156,12 +156,18 @@ static void dump_info(xc_interface *xch, bool detail)
>  
>      free(fs);
>  }
> -

Stray (deleted) whitespace.

> -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) )
> +        err(1, "xc_cpu_policy_get_leaves()");
> +    if ( xc_cpu_policy_get_msrs(xch, policy, &msrs, &nr_msrs) )
> +        err(1, "xc_cpu_policy_get_msrs()");

Not an issue with here per say, but to drive home the main problem.

This doesn't return the current leaves/msrs.  It gives you whatever's
stale in the staging buffer, which happens to be ok in xen-cpuid because
it only ever reads a policy...

~Andrew