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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Apr 14, 2021 at 03:49:02PM +0200, Jan Beulich wrote:
> 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.

I've added: "MSRs passed to level_msr are expected to be bitmaps of
features"

> > +                       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?

Yes, the intent is to only level the features bitfield found in
subleaf 1.

I was planning for level_leaf so far in this series to deal with the
feature leaves part of the featureset only. I guess you would also
like to leverage other parts of the xstate leaf, like the max_size or
the supported bits in xss_{low,high}?

> > +    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

I think you meant subleaf 0 EAX (the max subleaf value)?

subleaf 1 EAX is just a normal bitfield AFAIK.

> EAX of course)? This would avoid having to remember to make yet
> another mechanical change when enabling a new subleaf.

Sure, seems like a fine approach since leaf 7 will only contain
feature bitmaps.

> > +    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?

Indeed.

Thanks, Roger.