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

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

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.

>
>> --- /dev/null
>> +++ b/xen/lib/x86/policy.c
>> @@ -0,0 +1,53 @@
>> +#include "private.h"
>> +
>> +#include <xen/lib/x86/cpu-policy.h>
>> +
>> +int x86_cpu_policies_are_compatible(const struct cpu_policy *host,
>> +                                    const struct cpu_policy *guest,
>> +                                    struct cpu_policy_errors *e)
>> +{
>> +    uint32_t leaf = -1, subleaf = -1, msr = -1;
>> +    int ret = -EINVAL;
>> +
>> +#define NA XEN_CPUID_NO_SUBLEAF
>> +#define FAIL_CPUID(l, s) do { leaf = (l); subleaf = (s); goto out; } while ( 0 )
>> +#define FAIL_MSR(m) do { msr = (m); goto out; } while ( 0 )
>> +
>> +    if ( guest->cpuid->basic.max_leaf > host->cpuid->basic.max_leaf )
>> +        FAIL_CPUID(0, NA);
>> +
>> +    if ( guest->cpuid->extd.max_leaf > host->cpuid->extd.max_leaf )
>> +        FAIL_CPUID(0x80000008, NA);
>> +
>> +    /* TODO: Audit more CPUID data. */
>> +
>> +    if ( ~host->msr->plaform_info.raw & guest->msr->plaform_info.raw )
> I've noticed this only here, but there are numerous instances elsewhere:
> Could I talk you into fixing the spelling mistake (missing 't' in
> "platform_info") here or in a prereq patch (feel free to add my ack there
> without even posting)?

I'd not even noticed the mistake.  I'll get a fixup sorted as you suggest.

~Andrew

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