Re: [PATCH 03/21] libs/guest: introduce xc_cpu_policy_t

["Roger Pau Monné" <roger.pau@citrix.com>]

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