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

[Jan Beulich <jbeulich@xxxxxxxx>]

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?

Jan