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

[Jan Beulich <jbeulich@suse.com>]

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.

> --- 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.)

> --- /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)?

Jan

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