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

From: Jan Beulich <jbeulich@suse.com>
To: "Roger Pau Monné" <roger.pau@citrix.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>,
	Ian Jackson <iwj@xenproject.org>, Wei Liu <wl@xen.org>,
	Anthony PERARD <anthony.perard@citrix.com>,
	xen-devel@lists.xenproject.org
Subject: Re: [PATCH v2 15/21] libs/guest: obtain a compatible cpu policy from two input ones
Date: Thu, 22 Apr 2021 11:58:45 +0200	[thread overview]
Message-ID: <c692e77f-1a9a-ea1d-e029-2a8f62ee0e35@suse.com> (raw)
In-Reply-To: <YIFFBEhPYSYQhycf@Air-de-Roger>

On 22.04.2021 11:42, Roger Pau Monné wrote:
> 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"

How does such a comment help? I.e. how does the caller tell which MSRs
to pass here and which to deal with anyother way?

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

The latter is clearly one of the things to consider, yes (alongside
the respective bits in sub-leaf 0 for XCR0). Sub-leaves > 1 may also
need dealing with ECX. Yet then again some or all of this may need
handling elsewhere, not the least because of the unusual handling of
leaf 0xd in the hypervisor. What gets checked and/or adjusted where
needs to be settled upon, and then the different parts of code would
imo better cross-reference each other.

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

Indeed I did.

Jan

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