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 12:48:36 +0200	[thread overview]
Message-ID: <d877502d-9c18-6ff3-6e01-ee997c9f196a@suse.com> (raw)
In-Reply-To: <YIFRQu/CLEId2uMi@Air-de-Roger>

On 22.04.2021 12:34, Roger Pau Monné wrote:
> On Thu, Apr 22, 2021 at 11:58:45AM +0200, Jan Beulich wrote:
>> 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?
> 
> All MSRs should be passed to level_msr, but it's handling logic would
> need to be expanded to support MSRs that are not feature bitmaps.
> 
> It might be best to restore the previous switch and handle each MSR
> specifically?

I think so, yes. We need to be very careful with what a possible
default case does there, though.

>>>>> +                       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.
> 
> There's a comment in recalculate_xstate that mentions that Da1 leaf is
> the only piece of information preserved, and that everything else is
> derived from feature state. I don't think it makes sense to try to
> level anything apart from Da1 if it's going to be discarded by
> recalculate_xstate anyway?
> 
> I can add a comment here regarding why only Da1 is taken into account
> for leveling so far.

Yes, this would help. Thanks.

Jan