Re: [Xen-devel] [PATCH 1/8] libx86: Introduce x86_cpu_policies_are_compatible()

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 12/09/2019 09:22, Jan Beulich wrote:
> On 12.09.2019 09:59, Andrew Cooper wrote:
>> On 12/09/2019 08:43, Jan Beulich wrote:
>>> On 11.09.2019 22:04, Andrew Cooper wrote:
>>>> This helper will eventually be the core "can a guest confiured like this run
>>>> on the CPU?" logic.  For now, it is just enough of a stub to allow us to
>>>> replace the hypercall interface while retaining the previous behaviour.
>>>>
>>>> It will be expanded as various other bits of CPUID handling get cleaned up.
>>>>
>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> Fundamentally
>>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>> but a couple of remarks:
>>>
>>> For one, despite being just testing code, I think the two test[]
>>> arrays could do with constification.
>> Sadly they can't.  It is a consequence of struct cpu_policy using
>> non-const pointers.
>>
>> I tried introducing struct const_cpu_policy but that is even worse
>> because it prohibits operating on the system policy objects in Xen.
>>
>> Overall, dropping a const in the unit tests turned out to be the least
>> bad option, unless you have a radically different suggestion to try.
>>
>>>> --- a/xen/include/xen/lib/x86/cpu-policy.h
>>>> +++ b/xen/include/xen/lib/x86/cpu-policy.h
>>>> @@ -11,6 +11,25 @@ struct cpu_policy
>>>>      struct msr_policy *msr;
>>>>  };
>>>>  
>>>> +struct cpu_policy_errors
>>>> +{
>>>> +    uint32_t leaf, subleaf;
>>>> +    uint32_t msr;
>>>> +};
>>>> +
>>>> +#define INIT_CPU_POLICY_ERRORS { ~0u, ~0u, ~0u }
>>> Instead of this (and using it in every caller), couldn't the function
>>> fill this first thing? (The initializer isn't strictly needed anyway,
>>> as consumers are supposed to look at the structure only when having
>>> got back an error from the function, but since error paths fill just
>>> a subset of the fields I can see how pre-filling the whole structure
>>> is easier.)
>> At the moment, error pointers are conditionally written on error, which
>> means that all callers passing non-NULL need to initialise.
>>
>> This could be altered to have xc_set_domain_cpu_policy() and
>> x86_cpu_policies_are_compatible() pro-actively set "no error" to begin
>> with, but that doesn't remove the need for INIT_CPU_POLICY_ERRORS in the
>> first place.
> Right, I did notice this in a later patch. But yes, I do think
> having the functions proactively fill the structures would be
> better overall (and remove the need to use the initializer in at
> least some cases, i.e. where there are no other early error paths).

Ok.  I'll switch to doing this, and have another prep patch which
adjusts the behaviour of the already existing functions.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel