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

[Jan Beulich <jbeulich@suse.com>]

On 13.04.2021 16:01, Roger Pau Monne wrote:
> @@ -944,3 +945,130 @@ bool xc_cpu_policy_is_compatible(xc_interface *xch, const xc_cpu_policy_t host,
>  
>      return false;
>  }
> +
> +static uint64_t level_msr(unsigned int index, uint64_t val1, uint64_t val2)
> +{
> +    uint64_t val = val1 & val2;;

For arbitrary MSRs this isn't going to do any good. If only very
specific MSRs are assumed to make it here, I think this wants
commenting on.

Also, nit: stray semicolon.

> +    switch ( index )
> +    {
> +    case MSR_ARCH_CAPABILITIES:
> +        /*
> +         * Set RSBA if present on any of the input values to notice the guest
> +         * might run on vulnerable hardware at some point.
> +         */
> +        val |= (val1 | val2) & ARCH_CAPS_RSBA;
> +        break;
> +    }
> +
> +    return val;
> +}
> +
> +static bool level_leaf(xen_cpuid_leaf_t *l1, xen_cpuid_leaf_t *l2,

const (twice)?

> +                       xen_cpuid_leaf_t *out)
> +{
> +    *out = (xen_cpuid_leaf_t){ };
> +
> +    switch ( l1->leaf )
> +    {
> +    case 0x1:
> +    case 0x80000001:
> +        out->c = l1->c & l2->c;
> +        out->d = l1->d & l2->d;
> +        return true;
> +
> +    case 0xd:
> +        if ( l1->subleaf != 1 )
> +            break;
> +        out->a = l1->a & l2->a;
> +        return true;

Could you explain your thinking behind this (a code comment would
likely help)? You effectively discard everything except subleaf 1
by returning false in that case, don't you?

> +    case 0x7:
> +        switch ( l1->subleaf )
> +        {
> +        case 0:
> +            out->b = l1->b & l2->b;
> +            out->c = l1->c & l2->c;
> +            out->d = l1->d & l2->d;
> +            return true;
> +
> +        case 1:
> +            out->a = l1->a & l2->a;
> +            return true;
> +        }
> +        break;

Can we perhaps assume all subleaves here are going to hold flags,
and hence and both sides together without regard to what subleaf
we're actually dealing with (subleaf 1 remaining special as to
EAX of course)? This would avoid having to remember to make yet
another mechanical change when enabling a new subleaf.

> +    case 0x80000007:
> +        out->d = l1->d & l2->d;
> +        return true;
> +
> +    case 0x80000008:
> +        out->b = l1->b & l2->b;
> +        return true;
> +    }
> +
> +    return false;
> +}

Considering your LFENCE-always-serializing patch, I assume
whichever ends up going in last will take care of adding handling
of that leaf here?

Jan