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.
