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

[Jan Beulich <jbeulich@suse.com>]

On 22.04.2021 13:37, Roger Pau Monné wrote:
> On Thu, Apr 22, 2021 at 01:05:23PM +0200, Jan Beulich wrote:
>> On 22.04.2021 12:56, Roger Pau Monné wrote:
>>> On Thu, Apr 22, 2021 at 12:48:36PM +0200, Jan Beulich wrote:
>>>> 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.
>>>
>>> Maybe it would be better to handle level_msr in a way similar to
>>> level_leaf: return true/false to notice whether the MSR should be
>>> added to the resulting compatible policy?
>>>
>>> And then make the default case in level_msr just return false in order
>>> to prevent adding MSRs not properly leveled to the policy?
>>
>> I'm afraid I'm not clear about the implications. What again is the
>> (planned?) final effect of an MSR not getting added there?
> 
> Adding the MSR with a 0 value will zero out any previous value on the
> 'out' policy, while not adding it would leave the previous value
> there given the current code in xc_cpu_policy_calc_compatible added by
> this patch.
> 
> I would expect callers of xc_cpu_policy_calc_compatible to pass a
> zeroed 'out' policy, so I think the end result should be the same.

But we're not talking about actual MSR values here, as this is all
about policy. So in the end we'll have to see how things need to
be once we have the first non-feature-flag-like entries there. It
feels as if simply zeroing can't be generally the right thing in
such a case. It may e.g. be that min() is wanted instead.

Jan