Re: [PATCH v2 14/21] libs/guest: introduce helper to check cpu policy compatibility

[Jan Beulich <jbeulich@suse.com>]

On 22.04.2021 10:22, Roger Pau Monné wrote:
> On Wed, Apr 14, 2021 at 03:36:54PM +0200, Jan Beulich wrote:
>> On 13.04.2021 16:01, Roger Pau Monne wrote:
>>> --- a/tools/libs/guest/xg_cpuid_x86.c
>>> +++ b/tools/libs/guest/xg_cpuid_x86.c
>>> @@ -925,3 +925,22 @@ int xc_cpu_policy_update_msrs(xc_interface *xch, xc_cpu_policy_t policy,
>>>  
>>>      return rc;
>>>  }
>>> +
>>> +bool xc_cpu_policy_is_compatible(xc_interface *xch, const xc_cpu_policy_t host,
>>> +                                 const xc_cpu_policy_t guest)
>>> +{
>>> +    struct cpu_policy_errors err = INIT_CPU_POLICY_ERRORS;
>>> +    struct cpu_policy h = { &host->cpuid, &host->msr };
>>> +    struct cpu_policy g = { &guest->cpuid, &guest->msr };
>>> +    int rc = x86_cpu_policies_are_compatible(&h, &g, &err);
>>> +
>>> +    if ( !rc )
>>> +        return true;
>>> +
>>> +    if ( err.leaf != -1 )
>>> +        ERROR("Leaf %#x subleaf %#x is not compatible", err.leaf, err.subleaf);
>>> +    if ( err.msr != -1 )
>>> +        ERROR("MSR index %#x is not compatible", err.msr);
>>
>> Personally I'm against making assumptions like these ones about what
>> (in this case) INIT_CPU_POLICY_ERRORS actually expands to (i.e. three
>> times -1). I can see how alternatives to this are quickly going to
>> get ugly, so I'll leave it to others to judge.
> 
> Would you like me to define a separate POLICY_ERROR? ie:
> 
> #define INIT_CPU_POLICY_ERROR -1
> #define INIT_CPU_POLICY_ERRORS { INIT_CPU_POLICY_ERROR, \
>                                  INIT_CPU_POLICY_ERROR, \
>                                  INIT_CPU_POLICY_ERROR }

I would prefer this; I'm not sure (nit: properly parenthesized)
-1 is  the value to use though, considering the fields are unsigned.
I wonder what Andrew thinks, as he did introduce all of this.

> We already have a bunch of open coded -1 checks anyway, but might
> prevent new ones from appearing.

Indeed.

Jan