[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v6 08/12] libs/guest: rework xc_cpuid_xend_policy
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.
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |