Re: [PATCH] libx86: Introduce x86_cpu_policy_calculate_compatible() with MSR_ARCH_CAPS handling

[Jan Beulich <jbeulich@xxxxxxxx>]

On 04.05.2021 23:31, Andrew Cooper wrote:
> --- a/tools/include/xen-tools/libs.h
> +++ b/tools/include/xen-tools/libs.h
> @@ -63,4 +63,9 @@
>  #define ROUNDUP(_x,_w) (((unsigned long)(_x)+(1UL<<(_w))-1) & 
> ~((1UL<<(_w))-1))
>  #endif
>  
> +#ifndef _AC
> +#define __AC(X, Y)   (X ## Y)
> +#define _AC(X, Y)    __AC(X, Y)
> +#endif

Somewhere in Roger's recent / pending work I recall he moved these
from somewhere, instead of adding new instances.

> --- a/tools/tests/cpu-policy/test-cpu-policy.c
> +++ b/tools/tests/cpu-policy/test-cpu-policy.c
> @@ -775,6 +775,154 @@ static void test_is_compatible_failure(void)
>      }
>  }
>  
> +static void test_calculate_compatible_success(void)
> +{
> +    static struct test {
> +        const char *name;
> +        struct {
> +            struct cpuid_policy p;
> +            struct msr_policy m;
> +        } a, b, out;
> +    } tests[] = {
> +        {
> +            "arch_caps, b short max_leaf",
> +            .a = {
> +                .p.basic.max_leaf = 7,
> +                .p.feat.arch_caps = true,
> +                .m.arch_caps.rdcl_no = true,
> +            },
> +            .b = {
> +                .p.basic.max_leaf = 6,
> +                .p.feat.arch_caps = true,
> +                .m.arch_caps.rdcl_no = true,

Is this legitimate input in the first place?

> --- a/xen/lib/x86/policy.c
> +++ b/xen/lib/x86/policy.c
> @@ -29,6 +29,9 @@ int x86_cpu_policies_are_compatible(const struct cpu_policy 
> *host,
>      if ( ~host->msr->platform_info.raw & guest->msr->platform_info.raw )
>          FAIL_MSR(MSR_INTEL_PLATFORM_INFO);
>  
> +    if ( ~host->msr->arch_caps.raw & guest->msr->arch_caps.raw )
> +        FAIL_MSR(MSR_ARCH_CAPABILITIES);

Doesn't this need special treatment of RSBA, just like it needs specially
treating below?

> @@ -43,6 +46,50 @@ int x86_cpu_policies_are_compatible(const struct 
> cpu_policy *host,
>      return ret;
>  }
>  
> +int x86_cpu_policy_calculate_compatible(const struct cpu_policy *a,
> +                                        const struct cpu_policy *b,
> +                                        struct cpu_policy *out,
> +                                        struct cpu_policy_errors *err)
> +{
> +    const struct cpuid_policy *ap = a->cpuid, *bp = b->cpuid;
> +    const struct msr_policy *am = a->msr, *bm = b->msr;
> +    struct cpuid_policy *cp = out->cpuid;
> +    struct msr_policy *mp = out->msr;

Hmm, okay - this would not work with my proposal in reply to your
other patch. The output would instead need to have pointers
allocated here then.

> +    memset(cp, 0, sizeof(*cp));
> +    memset(mp, 0, sizeof(*mp));
> +
> +    cp->basic.max_leaf = min(ap->basic.max_leaf, bp->basic.max_leaf);

Any reason you don't do the same right away for the max extended
leaf?

Jan