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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 22.04.2021 14:07, Roger Pau Monné wrote:
> On Thu, Apr 22, 2021 at 01:42:45PM +0200, Jan Beulich wrote:
>> 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.
> 
> Maybe level_msr should return an error for MSRs not explicitly
> handled, that's propagated to the caller of
> xc_cpu_policy_calc_compatible.
> 
> That way addition of new MSRs are not likely to miss adding the
> required handling in level_msr?

Perhaps, yes.

Jan