Re: [PATCH v6 08/12] libs/guest: rework xc_cpuid_xend_policy

[Andrew Cooper <amc96@xxxxxxxx>]

On 17/01/2022 09:48, Roger Pau Monne wrote:
> Rename xc_cpuid_xend_policy to xc_cpu_policy_apply_cpuid

I'm not convinced by this name.  'xend' is important because it's the
format of the data passed, which 'cpuid' is not.

It is a horribly inefficient format, and really doesn't want to survive
cleanup to the internals of libxl.

> diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
> index e7ae133d8d..9060a2f763 100644
> --- a/tools/libs/guest/xg_cpuid_x86.c
> +++ b/tools/libs/guest/xg_cpuid_x86.c
> @@ -254,144 +254,107 @@ int xc_set_domain_cpu_policy(xc_interface *xch, 
> uint32_t domid,
>      return ret;
>  }
>  
> -static int compare_leaves(const void *l, const void *r)
> -{
> -    const xen_cpuid_leaf_t *lhs = l;
> -    const xen_cpuid_leaf_t *rhs = r;
> -
> -    if ( lhs->leaf != rhs->leaf )
> -        return lhs->leaf < rhs->leaf ? -1 : 1;
> -
> -    if ( lhs->subleaf != rhs->subleaf )
> -        return lhs->subleaf < rhs->subleaf ? -1 : 1;
> -
> -    return 0;
> -}
> -
> -static xen_cpuid_leaf_t *find_leaf(
> -    xen_cpuid_leaf_t *leaves, unsigned int nr_leaves,
> -    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);
> -}
> -
> -static int xc_cpuid_xend_policy(
> -    xc_interface *xch, uint32_t domid, const struct xc_xend_cpuid *xend)
> +int xc_cpu_policy_apply_cpuid(xc_interface *xch, xc_cpu_policy_t *policy,
> +                              const struct xc_xend_cpuid *cpuid, bool hvm)
>  {
>      int rc;
> -    xc_dominfo_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;
> +    xc_cpu_policy_t *host = NULL, *def = NULL;
>  
> -    if ( xc_domain_getinfo(xch, domid, 1, &di) != 1 ||
> -         di.domid != domid )
> -    {
> -        ERROR("Failed to obtain d%d info", domid);
> -        rc = -ESRCH;
> -        goto fail;
> -    }
> -
> -    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 )
> +    host = xc_cpu_policy_init();
> +    def = xc_cpu_policy_init();
> +    if ( !host || !def )
>      {
> -        PERROR("Failed to obtain d%d current policy", domid);
> -        rc = -errno;
> -        goto fail;
> +        PERROR("Failed to init policies");
> +        rc = -ENOMEM;
> +        goto out;
>      }
>  
>      /* Get the domain type's default policy. */
> -    nr_msrs = 0;
> -    nr_def = nr_leaves;
> -    rc = get_system_cpu_policy(xch, di.hvm ? 
> XEN_SYSCTL_cpu_policy_hvm_default
> +    rc = xc_cpu_policy_get_system(xch, hvm ? 
> XEN_SYSCTL_cpu_policy_hvm_default
>                                             : 
> XEN_SYSCTL_cpu_policy_pv_default,
> -                               &nr_def, def, &nr_msrs, NULL);
> +                                  def);
>      if ( rc )
>      {
> -        PERROR("Failed to obtain %s def policy", di.hvm ? "hvm" : "pv");
> -        rc = -errno;
> -        goto fail;
> +        PERROR("Failed to obtain %s def policy", hvm ? "hvm" : "pv");
> +        goto out;
>      }
>  
>      /* 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);
> +    rc = xc_cpu_policy_get_system(xch, XEN_SYSCTL_cpu_policy_host, host);
>      if ( rc )
>      {
>          PERROR("Failed to obtain host policy");
> -        rc = -errno;
> -        goto fail;
> +        goto out;
>      }
>  
>      rc = -EINVAL;
> -    for ( ; xend->leaf != XEN_CPUID_INPUT_UNUSED; ++xend )
> +    for ( ; cpuid->leaf != XEN_CPUID_INPUT_UNUSED; ++cpuid )
>      {
> -        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;
> +        xen_cpuid_leaf_t def_leaf;
> +        xen_cpuid_leaf_t host_leaf;
>  
> -        if ( cur_leaf == NULL || def_leaf == NULL || host_leaf == NULL )
> +        rc = xc_cpu_policy_get_cpuid(xch, policy, cpuid->leaf, 
> cpuid->subleaf,
> +                                     &cur_leaf);

This is a crazy increase in data shuffling.

Use x86_cpuid_get_leaf() from patch 2, which *is* find_leaf() for
objects rather than lists, and removes the need to further-shuffle the
data later now that you're not modifying cur in place.

It also removes the churn introduced by changing the indirection of
these variables.

It also removes all callers of xc_cpu_policy_get_cpuid(), which don't
have an obvious purpose to begin with.  Relatedly,
xc_cpu_policy_get_msr() has no users at all, that I can see.

~Andrew