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.coop...@citrix.com>
> Fundamentally
> Reviewed-by: Jan Beulich <jbeul...@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
