On Wed, Mar 31, 2021 at 09:10:13PM +0100, Andrew Cooper wrote:
> On 23/03/2021 09:58, Roger Pau Monne wrote:
> > Introduce an opaque type that is used to store the CPUID and MSRs
> > policies of a domain. Such type uses the existing cpu_policy structure
> > to store the data, but doesn't expose the type to the users of the
> > xenguest library.
> >
> > Introduce an allocation (init) and freeing function (destroy) to
> > manage the type.
> >
> > Note the type is not yet used anywhere.
> >
> > Signed-off-by: Roger Pau Monné <roger....@citrix.com>
> > ---
> > tools/include/xenctrl.h | 6 ++++++
> > tools/libs/guest/xg_cpuid_x86.c | 28 ++++++++++++++++++++++++++++
> > 2 files changed, 34 insertions(+)
> >
> > diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
> > index e91ff92b9b1..ffb3024bfeb 100644
> > --- a/tools/include/xenctrl.h
> > +++ b/tools/include/xenctrl.h
> > @@ -2590,6 +2590,12 @@ int xc_psr_get_domain_data(xc_interface *xch,
> > uint32_t domid,
> > int xc_psr_get_hw_info(xc_interface *xch, uint32_t socket,
> > xc_psr_feat_type type, xc_psr_hw_info *hw_info);
> >
> > +typedef struct cpu_policy *xc_cpu_policy_t;
> > +
> > +/* Create and free a xc_cpu_policy object. */
> > +xc_cpu_policy_t xc_cpu_policy_init(void);
> > +void xc_cpu_policy_destroy(xc_cpu_policy_t policy);
> > +
> > int xc_get_cpu_levelling_caps(xc_interface *xch, uint32_t *caps);
> > int xc_get_cpu_featureset(xc_interface *xch, uint32_t index,
> > uint32_t *nr_features, uint32_t *featureset);
> > diff --git a/tools/libs/guest/xg_cpuid_x86.c
> > b/tools/libs/guest/xg_cpuid_x86.c
> > index 9846f81e1f1..ade5281c178 100644
> > --- a/tools/libs/guest/xg_cpuid_x86.c
> > +++ b/tools/libs/guest/xg_cpuid_x86.c
> > @@ -659,3 +659,31 @@ out:
> >
> > return rc;
> > }
> > +
> > +xc_cpu_policy_t xc_cpu_policy_init(void)
> > +{
> > + xc_cpu_policy_t policy = calloc(1, sizeof(*policy));
> > +
> > + if ( !policy )
> > + return NULL;
> > +
> > + policy->cpuid = calloc(1, sizeof(*policy->cpuid));
> > + policy->msr = calloc(1, sizeof(*policy->msr));
> > + if ( !policy->cpuid || !policy->msr )
> > + {
> > + xc_cpu_policy_destroy(policy);
> > + return NULL;
> > + }
> > +
> > + return policy;
> > +}
> > +
> > +void xc_cpu_policy_destroy(xc_cpu_policy_t policy)
> > +{
> > + if ( !policy )
> > + return;
> > +
> > + free(policy->cpuid);
> > + free(policy->msr);
> > + free(policy);
> > +}
>
> Looking at the series as a whole, we have a fair quantity of complexity
> from short-lived dynamic allocations.
>
> I suspect that the code would be rather better if we had
>
> struct xc_cpu_policy {
> struct cpuid_policy cpuid;
> struct msr_policy msr;
> xen_cpuid_leaf_t leaves[CPUID_MAX_SERIALISED_LEAVES];
> xen_msr_entry_t msrs[MSR_MAX_SERIALISED_ENTRIES];
> /* Names perhaps subject to improvement */
> };
>
> and just made one memory allocation.
>
> This is userspace after all, and we're taking about <4k at the moment.
>
> All operations with Xen need to bounce through the leaves/msrs encoding
> (so we're using the space a minimum of twice for any logical operation
> at the higher level), and several userspace-only operations use them too.
We would still need to do some allocations for the system policies,
but yes, it would prevent some of the short-lived allocations. I
didn't care much because it's all user-space, but removing them will
likely make the code simpler.
Thanks, Roger.
