Re: [PATCH 13/21] libs/guest: switch users of xc_set_domain_cpu_policy

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 23/03/2021 09:58, Roger Pau Monne wrote:
> To use the new cpu policy interface xc_cpu_policy_set_domain. Note
> that xc_set_domain_cpu_policy is removed from the interface and the
> function is made static to xg_cpuid_x86.c for it's internal callers.
>
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> ---
>  tools/include/xenctrl.h             |  5 -----
>  tools/libs/guest/xg_cpuid_x86.c     | 22 +++++++++++-----------
>  tools/libs/guest/xg_sr_common_x86.c | 28 +++++++++++++++++++++-------
>  3 files changed, 32 insertions(+), 23 deletions(-)
>
> diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
> index 46f5026081c..164a287b367 100644
> --- a/tools/include/xenctrl.h
> +++ b/tools/include/xenctrl.h
> @@ -2625,11 +2625,6 @@ int xc_get_cpu_featureset(xc_interface *xch, uint32_t 
> index,
>  
>  int xc_cpu_policy_get_size(xc_interface *xch, uint32_t *nr_leaves,
>                             uint32_t *nr_msrs);
> -int xc_set_domain_cpu_policy(xc_interface *xch, uint32_t domid,
> -                             uint32_t nr_leaves, xen_cpuid_leaf_t *leaves,
> -                             uint32_t nr_msrs, xen_msr_entry_t *msrs,
> -                             uint32_t *err_leaf_p, uint32_t *err_subleaf_p,
> -                             uint32_t *err_msr_p);
>  
>  uint32_t xc_get_cpu_featureset_size(void);
>  
> diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
> index 07756743e76..f7b662f31aa 100644
> --- a/tools/libs/guest/xg_cpuid_x86.c
> +++ b/tools/libs/guest/xg_cpuid_x86.c
> @@ -204,11 +204,11 @@ static int get_domain_cpu_policy(xc_interface *xch, 
> uint32_t domid,
>      return ret;
>  }
>  
> -int xc_set_domain_cpu_policy(xc_interface *xch, uint32_t domid,
> -                             uint32_t nr_leaves, xen_cpuid_leaf_t *leaves,
> -                             uint32_t nr_msrs, xen_msr_entry_t *msrs,
> -                             uint32_t *err_leaf_p, uint32_t *err_subleaf_p,
> -                             uint32_t *err_msr_p)
> +static int set_domain_cpu_policy(xc_interface *xch, uint32_t domid,
> +                                 uint32_t nr_leaves, xen_cpuid_leaf_t 
> *leaves,
> +                                 uint32_t nr_msrs, xen_msr_entry_t *msrs,
> +                                 uint32_t *err_leaf_p, uint32_t 
> *err_subleaf_p,
> +                                 uint32_t *err_msr_p)
>  {
>      DECLARE_DOMCTL;
>      DECLARE_HYPERCALL_BOUNCE(leaves,
> @@ -405,8 +405,8 @@ 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);
> +    rc = 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)",
> @@ -638,8 +638,8 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t 
> domid, bool restore,
>          goto out;
>      }
>  
> -    rc = xc_set_domain_cpu_policy(xch, domid, nr_leaves, leaves, 0, NULL,
> -                                  &err_leaf, &err_subleaf, &err_msr);
> +    rc = set_domain_cpu_policy(xch, domid, nr_leaves, leaves, 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)",
> @@ -833,8 +833,8 @@ int xc_cpu_policy_set_domain(xc_interface *xch, uint32_t 
> domid,
>      if ( rc )
>          goto out;
>  
> -    rc = xc_set_domain_cpu_policy(xch, domid, nr_leaves, leaves, nr_msrs, 
> msrs,
> -                                  &err_leaf, &err_subleaf, &err_msr);
> +    rc = set_domain_cpu_policy(xch, domid, nr_leaves, leaves, nr_msrs, msrs,
> +                               &err_leaf, &err_subleaf, &err_msr);
>      if ( rc )
>      {
>          ERROR("Failed to set domain %u policy (%d = %s)", domid, -rc,
> diff --git a/tools/libs/guest/xg_sr_common_x86.c 
> b/tools/libs/guest/xg_sr_common_x86.c
> index 15265e7a331..02f0c3ae9ed 100644
> --- a/tools/libs/guest/xg_sr_common_x86.c
> +++ b/tools/libs/guest/xg_sr_common_x86.c
> @@ -151,7 +151,10 @@ int x86_static_data_complete(struct xc_sr_context *ctx, 
> unsigned int *missing)
>  {
>      xc_interface *xch = ctx->xch;
>      uint32_t nr_leaves = 0, nr_msrs = 0;
> -    uint32_t err_l = ~0, err_s = ~0, err_m = ~0;
> +    xc_cpu_policy_t policy = xc_cpu_policy_init();
> +
> +    if ( !policy )
> +        return -1;
>  
>      if ( ctx->x86.restore.cpuid.ptr )
>          nr_leaves = ctx->x86.restore.cpuid.size / sizeof(xen_cpuid_leaf_t);
> @@ -163,14 +166,25 @@ int x86_static_data_complete(struct xc_sr_context *ctx, 
> unsigned int *missing)
>      else
>          *missing |= XGR_SDD_MISSING_MSR;
>  
> +    if ( nr_leaves &&
> +         xc_cpu_policy_update_cpuid(xch, policy,
> +                                    ctx->x86.restore.cpuid.ptr, nr_leaves) )
> +    {
> +        PERROR("Failed to update CPUID policy");
> +        return -1;
> +    }
> +    if ( nr_msrs &&
> +         xc_cpu_policy_update_msrs(xch, policy,
> +                                   ctx->x86.restore.msr.ptr, nr_msrs) )
> +    {
> +        PERROR("Failed to update MSR policy");
> +        return -1;
> +    }
> +
>      if ( (nr_leaves || nr_msrs) &&
> -         xc_set_domain_cpu_policy(xch, ctx->domid,
> -                                  nr_leaves, ctx->x86.restore.cpuid.ptr,
> -                                  nr_msrs,   ctx->x86.restore.msr.ptr,
> -                                  &err_l, &err_s, &err_m) )
> +         xc_cpu_policy_set_domain(xch, ctx->domid, policy) )
>      {
> -        PERROR("Failed to set CPUID policy: leaf %08x, subleaf %08x, msr 
> %08x",
> -               err_l, err_s, err_m);
> +        PERROR("Failed to set CPUID policy");
>          return -1;
>      }

I don't think this is a good move.

All it does is waste time shuffling data in userspace during the VM
downtime window.  The format of CPUID/MSR data in the migration stream
is (deliberately) already in the form to hand it straight back to Xen,
and there will never be additional policy changes here (because that
would break the VM).

I'd just drop the patch entirely.  I'm not even certain if we want to
remove the thin hypercall wrappers - I'm pretty sure some of my low
level unit testing plans will want the raw accessors without being
forced through the xc_cpuid_policy_t interface.

~Andrew