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

From: Andrew Cooper <andrew.cooper3@citrix.com>
To: Roger Pau Monne <roger.pau@citrix.com>, <xen-devel@lists.xenproject.org>
Cc: Ian Jackson <iwj@xenproject.org>, Wei Liu <wl@xen.org>
Subject: Re: [PATCH 03/21] libs/guest: introduce xc_cpu_policy_t
Date: Wed, 31 Mar 2021 21:10:13 +0100	[thread overview]
Message-ID: <7f52bd98-24fd-95ba-f8c6-05a331941b49@citrix.com> (raw)
In-Reply-To: <20210323095849.37858-4-roger.pau@citrix.com>

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.

~Andrew