Re: [PATCH v4 24/26] tools/libxc: Wire a featureset through to cpuid policy logic

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Wed, Mar 23, 2016 at 04:36:27PM +0000, Andrew Cooper wrote:
> Later changes will cause the cpuid generation logic to seed their information

s/Later changes/Patch titled tools/libxc: Use featuresets rather than guesswork

> from a featureset.  This patch adds the infrastructure to specify a
> featureset, and will obtain the appropriate default from Xen if omitted.
s/default/defaults/

> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Acked-by: Wei Liu <wei.liu2@citrix.com>
> ---
> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> 
> v2:
>  * Modify existing call rather than introducing a new one.
>  * Fix up in-tree callsites.
> ---
>  tools/libxc/include/xenctrl.h       |  4 ++-
>  tools/libxc/xc_cpuid_x86.c          | 69 ++++++++++++++++++++++++++++++++-----
>  tools/libxl/libxl_cpuid.c           |  2 +-
>  tools/ocaml/libs/xc/xenctrl_stubs.c |  2 +-
>  tools/python/xen/lowlevel/xc/xc.c   |  2 +-
>  5 files changed, 66 insertions(+), 13 deletions(-)
> 
> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> index 66acbd1..872fd08 100644
> --- a/tools/libxc/include/xenctrl.h
> +++ b/tools/libxc/include/xenctrl.h
> @@ -1896,7 +1896,9 @@ int xc_cpuid_set(xc_interface *xch,
>                   const char **config,
>                   char **config_transformed);
>  int xc_cpuid_apply_policy(xc_interface *xch,
> -                          domid_t domid);
> +                          domid_t domid,
> +                          uint32_t *featureset,
> +                          unsigned int nr_features);
>  void xc_cpuid_to_str(const unsigned int *regs,
>                       char **strs); /* some strs[] may be NULL if ENOMEM */
>  int xc_mca_op(xc_interface *xch, struct xen_mc *mc);
> diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c
> index 0cffb36..a92f5e4 100644
> --- a/tools/libxc/xc_cpuid_x86.c
> +++ b/tools/libxc/xc_cpuid_x86.c
> @@ -166,6 +166,9 @@ struct cpuid_domain_info
>      bool pvh;
>      uint64_t xfeature_mask;
>  
> +    uint32_t *featureset;
> +    unsigned int nr_features;
> +
>      /* PV-only information. */
>      bool pv64;
>  
> @@ -197,11 +200,14 @@ static void cpuid(const unsigned int *input, unsigned int *regs)
>  }
>  
>  static int get_cpuid_domain_info(xc_interface *xch, domid_t domid,
> -                                 struct cpuid_domain_info *info)
> +                                 struct cpuid_domain_info *info,
> +                                 uint32_t *featureset,
> +                                 unsigned int nr_features)
>  {
>      struct xen_domctl domctl = {};
>      xc_dominfo_t di;
>      unsigned int in[2] = { 0, ~0U }, regs[4];
> +    unsigned int i, host_nr_features = xc_get_cpu_featureset_size();
>      int rc;
>  
>      cpuid(in, regs);
> @@ -223,6 +229,23 @@ static int get_cpuid_domain_info(xc_interface *xch, domid_t domid,
>      info->hvm = di.hvm;
>      info->pvh = di.pvh;
>  
> +    info->featureset = calloc(host_nr_features, sizeof(*info->featureset));
> +    if ( !info->featureset )
> +        return -ENOMEM;
> +
> +    info->nr_features = host_nr_features;
> +
> +    if ( featureset )
> +    {
> +        memcpy(info->featureset, featureset,
> +               min(host_nr_features, nr_features) * sizeof(*info->featureset));
> +
> +        /* Check for truncated set bits. */
> +        for ( i = nr_features; i < host_nr_features; ++i )

What if nr_features is greater then host_nr_features? Should we fail immediately?


> +            if ( featureset[i] != 0 )

Could you make this: if ( !featureset[i] )  - to complement the style?

Otherwise: Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel