Re: [PATCH 15/21] libs/guest: obtain a compatible cpu policy from two input ones

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.03.2021 10:58, Roger Pau Monne wrote:
> Introduce a helper to obtain a compatible cpu policy based on two
> input cpu policies. Currently this is done by and'ing all CPUID leaves
> and MSR entries, except for MSR_ARCH_CAPABILITIES which has the RSBA
> bit or'ed.

I'm afraid this is too simplistic. It might do as an initial
approximation if limited to the feature flag leaves, but you
can't reasonably AND together e.g. leaf 0 output.

> @@ -1115,3 +1116,117 @@ bool xc_cpu_policy_is_compatible(xc_interface *xch, 
> const xc_cpu_policy_t p1,
>  
>      return false;
>  }
> +
> +static uint64_t level_msr(unsigned int index, uint64_t val1, uint64_t val2)
> +{
> +    uint64_t val;
> +
> +    switch( index )
> +    {
> +    case MSR_ARCH_CAPABILITIES:
> +        val = val1 & val2;

Considering you need this even here, how about making this the
initializer of the variable, allowing to drop "default:"
altogether?

Also, nit: Missing blank after "switch".

> +    index = 0;
> +    for ( i = 0; i < p1_nr_leaves; i++ )
> +        for ( j = 0; j < p2_nr_leaves; j++ )
> +            if ( p1_leaves[i].leaf == p2_leaves[j].leaf &&
> +                 p1_leaves[i].subleaf == p2_leaves[j].subleaf )
> +            {
> +                leaves[index].leaf = p1_leaves[i].leaf;
> +                leaves[index].subleaf = p1_leaves[i].subleaf;
> +                leaves[index].a = p1_leaves[i].a & p2_leaves[j].a;
> +                leaves[index].b = p1_leaves[i].b & p2_leaves[j].b;
> +                leaves[index].c = p1_leaves[i].c & p2_leaves[j].c;
> +                leaves[index].d = p1_leaves[i].d & p2_leaves[j].d;
> +                index++;
> +            }
> +    nr_leaves = index;
> +
> +    index = 0;
> +    for ( i = 0; i < p1_nr_msrs; i++ )
> +        for ( j = 0; j < p2_nr_msrs; j++ )
> +            if ( p1_msrs[i].idx == p2_msrs[j].idx )
> +            {
> +                msrs[index].idx = p1_msrs[i].idx;
> +                msrs[index].val = level_msr(p1_msrs[i].idx,
> +                                            p1_msrs[i].val, p2_msrs[j].val);
> +                index++;
> +            }
> +    nr_msrs = index;

Mid- to long-term I'd be afraid of this going to take too long for
at least the MSRs. Can't we build on some similarity in the ordering
of both arrays, to avoid the double for()s?

Jan