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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Tue, Jan 18, 2022 at 02:06:25PM +0000, Andrew Cooper wrote:
> 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.

The format would be the libxc format now. Having xend here is a
layering violation IMO.

Note this function goes away in patch 11, so I'm unsure there's a lot
of value in discussing over the name of a function that's about to
disappear.

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

Even when moved to the internals of libxl the format is not exposed to
users of libxl (it's a libxl__cpuid_policy in libxl_internal.h), so we
are free to modify it at our own will. I would defer the changes to
the format to a separate series, there's enough churn here already. My
aim was to provide a new interface while keeping the functional
changes to a minimum.

> > 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.

This function will get moved into libxl in patch 11, and then it would
be a layering violation to call x86_cpuid_get_leaf, so it needs to use
the xc wrapper.

> 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.

This was introduced in order to provide a sensible interface. It
doesn't make sense to allow getting cpuid leafs but not MSRs from a
policy IMO. I would expect other toolstacks, or libxl in the future to
make use of it.

If you don't think that's enough justification we can leave that patch
aside.

Thanks, Roger.