* [PATCH] libx86: Introduce x86_cpu_policy_calculate_compatible() with MSR_ARCH_CAPS handling @ 2021-05-04 21:31 Andrew Cooper 2021-05-05 6:39 ` Jan Beulich 2021-05-05 10:04 ` Roger Pau Monné 0 siblings, 2 replies; 12+ messages in thread From: Andrew Cooper @ 2021-05-04 21:31 UTC (permalink / raw) To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich, Roger Pau Monné, Wei Liu Just as with x86_cpu_policies_are_compatible(), make a start on this function with some token handling. Add levelling support for MSR_ARCH_CAPS, because RSBA has interesting properties, and introduce test_calculate_compatible_success() to the unit tests, covering various cases where the arch_caps CPUID bit falls out, and with RSBA accumulating rather than intersecting across the two. Extend x86_cpu_policies_are_compatible() with a check for MSR_ARCH_CAPS, which was arguably missing from c/s e32605b07ef "x86: Begin to introduce support for MSR_ARCH_CAPS". Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Wei Liu <wl@xen.org> --- tools/include/xen-tools/libs.h | 5 ++ tools/tests/cpu-policy/test-cpu-policy.c | 150 +++++++++++++++++++++++++++++++ xen/include/xen/lib/x86/cpu-policy.h | 22 +++++ xen/lib/x86/policy.c | 47 ++++++++++ 4 files changed, 224 insertions(+) diff --git a/tools/include/xen-tools/libs.h b/tools/include/xen-tools/libs.h index a16e0c3807..4de10efdea 100644 --- 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 + #endif /* __XEN_TOOLS_LIBS__ */ diff --git a/tools/tests/cpu-policy/test-cpu-policy.c b/tools/tests/cpu-policy/test-cpu-policy.c index 75973298df..455b4fe3c0 100644 --- 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, + }, + .out = { + .p.basic.max_leaf = 6, + }, + }, + { + "arch_caps, b feat missing", + .a = { + .p.basic.max_leaf = 7, + .p.feat.arch_caps = true, + .m.arch_caps.rdcl_no = true, + }, + .b = { + .p.basic.max_leaf = 7, + .m.arch_caps.rdcl_no = true, + }, + .out = { + .p.basic.max_leaf = 7, + }, + }, + { + "arch_caps, b rdcl_no missing", + .a = { + .p.basic.max_leaf = 7, + .p.feat.arch_caps = true, + .m.arch_caps.rdcl_no = true, + }, + .b = { + .p.basic.max_leaf = 7, + .p.feat.arch_caps = true, + }, + .out = { + .p.basic.max_leaf = 7, + .p.feat.arch_caps = true, + }, + }, + { + "arch_caps, rdcl_no ok", + .a = { + .p.basic.max_leaf = 7, + .p.feat.arch_caps = true, + .m.arch_caps.rdcl_no = true, + }, + .b = { + .p.basic.max_leaf = 7, + .p.feat.arch_caps = true, + .m.arch_caps.rdcl_no = true, + }, + .out = { + .p.basic.max_leaf = 7, + .p.feat.arch_caps = true, + .m.arch_caps.rdcl_no = true, + }, + }, + { + "arch_caps, rsba accum", + .a = { + .p.basic.max_leaf = 7, + .p.feat.arch_caps = true, + .m.arch_caps.rsba = true, + }, + .b = { + .p.basic.max_leaf = 7, + .p.feat.arch_caps = true, + }, + .out = { + .p.basic.max_leaf = 7, + .p.feat.arch_caps = true, + .m.arch_caps.rsba = true, + }, + }, + }; + struct cpu_policy_errors no_errors = INIT_CPU_POLICY_ERRORS; + + printf("Testing calculate compatibility success:\n"); + + for ( size_t i = 0; i < ARRAY_SIZE(tests); ++i ) + { + struct test *t = &tests[i]; + struct cpuid_policy *p = malloc(sizeof(struct cpuid_policy)); + struct msr_policy *m = malloc(sizeof(struct msr_policy)); + struct cpu_policy a = { + &t->a.p, + &t->a.m, + }, b = { + &t->b.p, + &t->b.m, + }, out = { + p, + m, + }; + struct cpu_policy_errors e; + int res; + + if ( !p || !m ) + err(1, "%s() malloc failure", __func__); + + res = x86_cpu_policy_calculate_compatible(&a, &b, &out, &e); + + /* Check the expected error output. */ + if ( res != 0 || memcmp(&no_errors, &e, sizeof(no_errors)) ) + { + fail(" Test '%s' expected no errors\n" + " got res %d { leaf %08x, subleaf %08x, msr %08x }\n", + t->name, res, e.leaf, e.subleaf, e.msr); + goto test_done; + } + + if ( memcmp(&t->out.p, p, sizeof(*p)) ) + { + fail(" Test '%s' resulting CPUID policy not as expected\n", + t->name); + goto test_done; + } + + if ( memcmp(&t->out.m, m, sizeof(*m)) ) + { + fail(" Test '%s' resulting MSR policy not as expected\n", + t->name); + goto test_done; + } + + test_done: + free(p); + free(m); + } +} + int main(int argc, char **argv) { printf("CPU Policy unit tests\n"); @@ -793,6 +941,8 @@ int main(int argc, char **argv) test_is_compatible_success(); test_is_compatible_failure(); + test_calculate_compatible_success(); + if ( nr_failures ) printf("Done: %u failures\n", nr_failures); else diff --git a/xen/include/xen/lib/x86/cpu-policy.h b/xen/include/xen/lib/x86/cpu-policy.h index 5a2c4c7b2d..0422a15557 100644 --- a/xen/include/xen/lib/x86/cpu-policy.h +++ b/xen/include/xen/lib/x86/cpu-policy.h @@ -37,6 +37,28 @@ int x86_cpu_policies_are_compatible(const struct cpu_policy *host, const struct cpu_policy *guest, struct cpu_policy_errors *err); +/* + * Given two policies, calculate one which is compatible with each. + * + * i.e. Given host @a and host @b, calculate what to give a VM so it can live + * migrate between the two. + * + * @param a A cpu_policy. + * @param b Another cpu_policy. + * @param out A policy compatible with @a and @b. + * @param err Optional hint for error diagnostics. + * @returns -errno + * + * For typical usage, @a and @b should be system policies of the same type + * (i.e. PV/HVM default/max) from different hosts. In the case that an + * incompatibility is detected, the optional err pointer may identify the + * problematic leaf/subleaf and/or MSR. + */ +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); + #endif /* !XEN_LIB_X86_POLICIES_H */ /* diff --git a/xen/lib/x86/policy.c b/xen/lib/x86/policy.c index f6cea4e2f9..06039e8aa8 100644 --- 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); + #undef FAIL_MSR #undef FAIL_CPUID #undef NA @@ -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; + + memset(cp, 0, sizeof(*cp)); + memset(mp, 0, sizeof(*mp)); + + cp->basic.max_leaf = min(ap->basic.max_leaf, bp->basic.max_leaf); + + if ( cp->basic.max_leaf >= 7 ) + { + cp->feat.max_subleaf = min(ap->feat.max_subleaf, bp->feat.max_subleaf); + + cp->feat.raw[0].b = ap->feat.raw[0].b & bp->feat.raw[0].b; + cp->feat.raw[0].c = ap->feat.raw[0].c & bp->feat.raw[0].c; + cp->feat.raw[0].d = ap->feat.raw[0].d & bp->feat.raw[0].d; + } + + /* TODO: Far more. */ + + mp->platform_info.raw = am->platform_info.raw & bm->platform_info.raw; + + if ( cp->feat.arch_caps ) + { + /* + * RSBA means "RSB Alternative", i.e. RSB stuffing not necesserily + * safe. It needs to accumulate rather than intersect across a + * resource pool. + */ +#define POL_MASK ARCH_CAPS_RSBA + mp->arch_caps.raw = ((am->arch_caps.raw ^ POL_MASK) & + (bm->arch_caps.raw ^ POL_MASK)) ^ POL_MASK; +#undef POL_MASK + } + + return 0; +} + /* * Local variables: * mode: C -- 2.11.0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] libx86: Introduce x86_cpu_policy_calculate_compatible() with MSR_ARCH_CAPS handling 2021-05-04 21:31 [PATCH] libx86: Introduce x86_cpu_policy_calculate_compatible() with MSR_ARCH_CAPS handling Andrew Cooper @ 2021-05-05 6:39 ` Jan Beulich 2021-05-05 12:15 ` Andrew Cooper 2021-05-05 10:04 ` Roger Pau Monné 1 sibling, 1 reply; 12+ messages in thread From: Jan Beulich @ 2021-05-05 6:39 UTC (permalink / raw) To: Andrew Cooper; +Cc: Roger Pau Monné, Wei Liu, Xen-devel 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] libx86: Introduce x86_cpu_policy_calculate_compatible() with MSR_ARCH_CAPS handling 2021-05-05 6:39 ` Jan Beulich @ 2021-05-05 12:15 ` Andrew Cooper 2021-05-05 12:32 ` Jan Beulich 0 siblings, 1 reply; 12+ messages in thread From: Andrew Cooper @ 2021-05-05 12:15 UTC (permalink / raw) To: Jan Beulich; +Cc: Roger Pau Monné, Wei Liu, Xen-devel On 05/05/2021 07:39, Jan Beulich wrote: > On 04.05.2021 23:31, Andrew Cooper wrote: >> --- 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? I've been debating this for a long time, and have decided "yes". We have the xend format, and libxl format, and cpuid=["host:max_leaf=6"] ought not to be rejected simply because it hasn't also gone and zeroed every higher piece of information as well. In production, this function will be used once per host in the pool, and once more if any custom configuration is specified. Requiring both inputs to be of the normal form isn't necessary for this to function correctly (and indeed, would only add extra overhead), but the eventual result will be cleaned/shrunk/etc as appropriate. >> --- 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? No. If RSBA is clear in 'host', then Xen doesn't know about it, and it cannot be set in the policy, and cannot be offered to guests. At the moment, our ARCH_CAPS in the policy is special for dom0 alone to see, which is why RSBA isn't unilaterally set, but it will just as soon as the toolstack logic for handling MSRs properly is in place. >> @@ -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? I did the minimum for RSBA testing. The line needs to be drawn somewhere. ~Andrew ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] libx86: Introduce x86_cpu_policy_calculate_compatible() with MSR_ARCH_CAPS handling 2021-05-05 12:15 ` Andrew Cooper @ 2021-05-05 12:32 ` Jan Beulich 0 siblings, 0 replies; 12+ messages in thread From: Jan Beulich @ 2021-05-05 12:32 UTC (permalink / raw) To: Andrew Cooper; +Cc: Roger Pau Monné, Wei Liu, Xen-devel On 05.05.2021 14:15, Andrew Cooper wrote: > On 05/05/2021 07:39, Jan Beulich wrote: >> On 04.05.2021 23:31, Andrew Cooper wrote: >>> --- 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? > > I've been debating this for a long time, and have decided "yes". > > We have the xend format, and libxl format, and > > cpuid=["host:max_leaf=6"] > > ought not to be rejected simply because it hasn't also gone and zeroed > every higher piece of information as well. > > In production, this function will be used once per host in the pool, and > once more if any custom configuration is specified. > > Requiring both inputs to be of the normal form isn't necessary for this > to function correctly (and indeed, would only add extra overhead), but > the eventual result will be cleaned/shrunk/etc as appropriate. Makes sense to me. >>> --- 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? > > No. If RSBA is clear in 'host', then Xen doesn't know about it, and it > cannot be set in the policy, and cannot be offered to guests. How does this play together with the comment in x86_cpu_policy_calculate_compatible() saying "accumulate"? If it's clear in one policy because it has to be clear in the host's, how can it be valid for it to get set there in the result, just because it was set in the other input? >>> @@ -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? > > I did the minimum for RSBA testing. The line needs to be drawn somewhere. I see. I was thinking that it would be nice if the various related pieces could remain in sync (or at least new code not staying behind what we already handle elsewhere), i.e. this one doing at least the equivalent of what x86_cpu_policies_are_compatible() is currently capable of dealing with. This, again, would make it easier to spot all the places that need adjusting when something new is to be added. But I'm not going to insist - this is largely your realm anyway. Jan ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] libx86: Introduce x86_cpu_policy_calculate_compatible() with MSR_ARCH_CAPS handling 2021-05-04 21:31 [PATCH] libx86: Introduce x86_cpu_policy_calculate_compatible() with MSR_ARCH_CAPS handling Andrew Cooper 2021-05-05 6:39 ` Jan Beulich @ 2021-05-05 10:04 ` Roger Pau Monné 2021-05-05 12:37 ` Andrew Cooper 1 sibling, 1 reply; 12+ messages in thread From: Roger Pau Monné @ 2021-05-05 10:04 UTC (permalink / raw) To: Andrew Cooper; +Cc: Xen-devel, Jan Beulich, Wei Liu On Tue, May 04, 2021 at 10:31:20PM +0100, Andrew Cooper wrote: > Just as with x86_cpu_policies_are_compatible(), make a start on this function > with some token handling. > > Add levelling support for MSR_ARCH_CAPS, because RSBA has interesting > properties, and introduce test_calculate_compatible_success() to the unit > tests, covering various cases where the arch_caps CPUID bit falls out, and > with RSBA accumulating rather than intersecting across the two. > > Extend x86_cpu_policies_are_compatible() with a check for MSR_ARCH_CAPS, which > was arguably missing from c/s e32605b07ef "x86: Begin to introduce support for > MSR_ARCH_CAPS". > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > CC: Jan Beulich <JBeulich@suse.com> > CC: Roger Pau Monné <roger.pau@citrix.com> > CC: Wei Liu <wl@xen.org> > --- > tools/include/xen-tools/libs.h | 5 ++ > tools/tests/cpu-policy/test-cpu-policy.c | 150 +++++++++++++++++++++++++++++++ > xen/include/xen/lib/x86/cpu-policy.h | 22 +++++ > xen/lib/x86/policy.c | 47 ++++++++++ > 4 files changed, 224 insertions(+) > > diff --git a/tools/include/xen-tools/libs.h b/tools/include/xen-tools/libs.h > index a16e0c3807..4de10efdea 100644 > --- 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 You need to remove those definitions from libxl_internal.h. > #endif /* __XEN_TOOLS_LIBS__ */ > diff --git a/tools/tests/cpu-policy/test-cpu-policy.c b/tools/tests/cpu-policy/test-cpu-policy.c > index 75973298df..455b4fe3c0 100644 > --- 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, > + }, > + .out = { > + .p.basic.max_leaf = 6, > + }, > + }, > + { > + "arch_caps, b feat missing", > + .a = { > + .p.basic.max_leaf = 7, > + .p.feat.arch_caps = true, > + .m.arch_caps.rdcl_no = true, > + }, > + .b = { > + .p.basic.max_leaf = 7, > + .m.arch_caps.rdcl_no = true, > + }, > + .out = { > + .p.basic.max_leaf = 7, > + }, > + }, > + { > + "arch_caps, b rdcl_no missing", > + .a = { > + .p.basic.max_leaf = 7, > + .p.feat.arch_caps = true, > + .m.arch_caps.rdcl_no = true, > + }, > + .b = { > + .p.basic.max_leaf = 7, > + .p.feat.arch_caps = true, > + }, > + .out = { > + .p.basic.max_leaf = 7, > + .p.feat.arch_caps = true, > + }, > + }, > + { > + "arch_caps, rdcl_no ok", > + .a = { > + .p.basic.max_leaf = 7, > + .p.feat.arch_caps = true, > + .m.arch_caps.rdcl_no = true, > + }, > + .b = { > + .p.basic.max_leaf = 7, > + .p.feat.arch_caps = true, > + .m.arch_caps.rdcl_no = true, > + }, > + .out = { > + .p.basic.max_leaf = 7, > + .p.feat.arch_caps = true, > + .m.arch_caps.rdcl_no = true, > + }, > + }, > + { > + "arch_caps, rsba accum", > + .a = { > + .p.basic.max_leaf = 7, > + .p.feat.arch_caps = true, > + .m.arch_caps.rsba = true, > + }, > + .b = { > + .p.basic.max_leaf = 7, > + .p.feat.arch_caps = true, > + }, > + .out = { > + .p.basic.max_leaf = 7, > + .p.feat.arch_caps = true, > + .m.arch_caps.rsba = true, > + }, > + }, > + }; > + struct cpu_policy_errors no_errors = INIT_CPU_POLICY_ERRORS; > + > + printf("Testing calculate compatibility success:\n"); > + > + for ( size_t i = 0; i < ARRAY_SIZE(tests); ++i ) > + { > + struct test *t = &tests[i]; > + struct cpuid_policy *p = malloc(sizeof(struct cpuid_policy)); > + struct msr_policy *m = malloc(sizeof(struct msr_policy)); > + struct cpu_policy a = { > + &t->a.p, > + &t->a.m, > + }, b = { > + &t->b.p, > + &t->b.m, > + }, out = { > + p, > + m, > + }; > + struct cpu_policy_errors e; > + int res; > + > + if ( !p || !m ) > + err(1, "%s() malloc failure", __func__); > + > + res = x86_cpu_policy_calculate_compatible(&a, &b, &out, &e); > + > + /* Check the expected error output. */ > + if ( res != 0 || memcmp(&no_errors, &e, sizeof(no_errors)) ) > + { > + fail(" Test '%s' expected no errors\n" > + " got res %d { leaf %08x, subleaf %08x, msr %08x }\n", > + t->name, res, e.leaf, e.subleaf, e.msr); > + goto test_done; > + } > + > + if ( memcmp(&t->out.p, p, sizeof(*p)) ) > + { > + fail(" Test '%s' resulting CPUID policy not as expected\n", > + t->name); > + goto test_done; > + } > + > + if ( memcmp(&t->out.m, m, sizeof(*m)) ) > + { > + fail(" Test '%s' resulting MSR policy not as expected\n", > + t->name); > + goto test_done; > + } In order to assert that we don't mess things up, I would also add a x86_cpu_policies_are_compatible check here: res = x86_cpu_policies_are_compatible(&a, &out, &e); if ( res ) fail(" Test '%s' created incompatible policy\n" " got res %d { leaf %08x, subleaf %08x, msr %08x }\n", t->name, res, e.leaf, e.subleaf, e.msr); res = x86_cpu_policies_are_compatible(&b, &out, &e); if ( res ) fail(" Test '%s' created incompatible policy\n" " got res %d { leaf %08x, subleaf %08x, msr %08x }\n", t->name, res, e.leaf, e.subleaf, e.msr); > + > + test_done: > + free(p); > + free(m); > + } > +} > + > int main(int argc, char **argv) > { > printf("CPU Policy unit tests\n"); > @@ -793,6 +941,8 @@ int main(int argc, char **argv) > test_is_compatible_success(); > test_is_compatible_failure(); > > + test_calculate_compatible_success(); > + > if ( nr_failures ) > printf("Done: %u failures\n", nr_failures); > else > diff --git a/xen/include/xen/lib/x86/cpu-policy.h b/xen/include/xen/lib/x86/cpu-policy.h > index 5a2c4c7b2d..0422a15557 100644 > --- a/xen/include/xen/lib/x86/cpu-policy.h > +++ b/xen/include/xen/lib/x86/cpu-policy.h > @@ -37,6 +37,28 @@ int x86_cpu_policies_are_compatible(const struct cpu_policy *host, > const struct cpu_policy *guest, > struct cpu_policy_errors *err); > > +/* > + * Given two policies, calculate one which is compatible with each. > + * > + * i.e. Given host @a and host @b, calculate what to give a VM so it can live > + * migrate between the two. > + * > + * @param a A cpu_policy. > + * @param b Another cpu_policy. > + * @param out A policy compatible with @a and @b. > + * @param err Optional hint for error diagnostics. > + * @returns -errno > + * > + * For typical usage, @a and @b should be system policies of the same type > + * (i.e. PV/HVM default/max) from different hosts. In the case that an > + * incompatibility is detected, the optional err pointer may identify the > + * problematic leaf/subleaf and/or MSR. > + */ > +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); > + > #endif /* !XEN_LIB_X86_POLICIES_H */ > > /* > diff --git a/xen/lib/x86/policy.c b/xen/lib/x86/policy.c > index f6cea4e2f9..06039e8aa8 100644 > --- 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); It might be nice to expand test_is_compatible_{success,failure} to account for arch_caps being checked now. Shouldn't this check also take into account that host might not have RSBA set, but it's legit for a guest policy to have it? if ( ~host->msr->arch_caps.raw & guest->msr->arch_caps.raw & ~POL_MASK ) FAIL_MSR(MSR_ARCH_CAPABILITIES); Maybe POL_MASK should be renamed and defined in a header so it's widely available? > + > #undef FAIL_MSR > #undef FAIL_CPUID > #undef NA > @@ -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) I think this should be in an #ifndef __XEN__ protected region? There's no need to expose this to the hypervisor, as I would expect it will never have to do compatible policy generation? (ie: it will always be done by the toolstack?) > +{ > + 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; > + > + memset(cp, 0, sizeof(*cp)); > + memset(mp, 0, sizeof(*mp)); > + > + cp->basic.max_leaf = min(ap->basic.max_leaf, bp->basic.max_leaf); > + > + if ( cp->basic.max_leaf >= 7 ) > + { > + cp->feat.max_subleaf = min(ap->feat.max_subleaf, bp->feat.max_subleaf); > + > + cp->feat.raw[0].b = ap->feat.raw[0].b & bp->feat.raw[0].b; > + cp->feat.raw[0].c = ap->feat.raw[0].c & bp->feat.raw[0].c; > + cp->feat.raw[0].d = ap->feat.raw[0].d & bp->feat.raw[0].d; > + } > + > + /* TODO: Far more. */ Right, my proposed patch (07/13) went a bit further and also leveled 1c, 1d, Da1, e1c, e1d, e7d, e8b and e21a, and we also need to level a couple of max_leaf fields. I'm happy for this to go in first, and I can rebase the extra logic I have on top of this one. Thanks, Roger. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] libx86: Introduce x86_cpu_policy_calculate_compatible() with MSR_ARCH_CAPS handling 2021-05-05 10:04 ` Roger Pau Monné @ 2021-05-05 12:37 ` Andrew Cooper 2021-05-05 13:02 ` Roger Pau Monné 0 siblings, 1 reply; 12+ messages in thread From: Andrew Cooper @ 2021-05-05 12:37 UTC (permalink / raw) To: Roger Pau Monné; +Cc: Xen-devel, Jan Beulich, Wei Liu On 05/05/2021 11:04, Roger Pau Monné wrote: > On Tue, May 04, 2021 at 10:31:20PM +0100, Andrew Cooper wrote: >> Just as with x86_cpu_policies_are_compatible(), make a start on this function >> with some token handling. >> >> Add levelling support for MSR_ARCH_CAPS, because RSBA has interesting >> properties, and introduce test_calculate_compatible_success() to the unit >> tests, covering various cases where the arch_caps CPUID bit falls out, and >> with RSBA accumulating rather than intersecting across the two. >> >> Extend x86_cpu_policies_are_compatible() with a check for MSR_ARCH_CAPS, which >> was arguably missing from c/s e32605b07ef "x86: Begin to introduce support for >> MSR_ARCH_CAPS". >> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >> --- >> CC: Jan Beulich <JBeulich@suse.com> >> CC: Roger Pau Monné <roger.pau@citrix.com> >> CC: Wei Liu <wl@xen.org> >> --- >> tools/include/xen-tools/libs.h | 5 ++ >> tools/tests/cpu-policy/test-cpu-policy.c | 150 +++++++++++++++++++++++++++++++ >> xen/include/xen/lib/x86/cpu-policy.h | 22 +++++ >> xen/lib/x86/policy.c | 47 ++++++++++ >> 4 files changed, 224 insertions(+) >> >> diff --git a/tools/include/xen-tools/libs.h b/tools/include/xen-tools/libs.h >> index a16e0c3807..4de10efdea 100644 >> --- 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 > You need to remove those definitions from libxl_internal.h. Ok. > >> #endif /* __XEN_TOOLS_LIBS__ */ >> diff --git a/tools/tests/cpu-policy/test-cpu-policy.c b/tools/tests/cpu-policy/test-cpu-policy.c >> index 75973298df..455b4fe3c0 100644 >> --- 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, >> + }, >> + .out = { >> + .p.basic.max_leaf = 6, >> + }, >> + }, >> + { >> + "arch_caps, b feat missing", >> + .a = { >> + .p.basic.max_leaf = 7, >> + .p.feat.arch_caps = true, >> + .m.arch_caps.rdcl_no = true, >> + }, >> + .b = { >> + .p.basic.max_leaf = 7, >> + .m.arch_caps.rdcl_no = true, >> + }, >> + .out = { >> + .p.basic.max_leaf = 7, >> + }, >> + }, >> + { >> + "arch_caps, b rdcl_no missing", >> + .a = { >> + .p.basic.max_leaf = 7, >> + .p.feat.arch_caps = true, >> + .m.arch_caps.rdcl_no = true, >> + }, >> + .b = { >> + .p.basic.max_leaf = 7, >> + .p.feat.arch_caps = true, >> + }, >> + .out = { >> + .p.basic.max_leaf = 7, >> + .p.feat.arch_caps = true, >> + }, >> + }, >> + { >> + "arch_caps, rdcl_no ok", >> + .a = { >> + .p.basic.max_leaf = 7, >> + .p.feat.arch_caps = true, >> + .m.arch_caps.rdcl_no = true, >> + }, >> + .b = { >> + .p.basic.max_leaf = 7, >> + .p.feat.arch_caps = true, >> + .m.arch_caps.rdcl_no = true, >> + }, >> + .out = { >> + .p.basic.max_leaf = 7, >> + .p.feat.arch_caps = true, >> + .m.arch_caps.rdcl_no = true, >> + }, >> + }, >> + { >> + "arch_caps, rsba accum", >> + .a = { >> + .p.basic.max_leaf = 7, >> + .p.feat.arch_caps = true, >> + .m.arch_caps.rsba = true, >> + }, >> + .b = { >> + .p.basic.max_leaf = 7, >> + .p.feat.arch_caps = true, >> + }, >> + .out = { >> + .p.basic.max_leaf = 7, >> + .p.feat.arch_caps = true, >> + .m.arch_caps.rsba = true, >> + }, >> + }, >> + }; >> + struct cpu_policy_errors no_errors = INIT_CPU_POLICY_ERRORS; >> + >> + printf("Testing calculate compatibility success:\n"); >> + >> + for ( size_t i = 0; i < ARRAY_SIZE(tests); ++i ) >> + { >> + struct test *t = &tests[i]; >> + struct cpuid_policy *p = malloc(sizeof(struct cpuid_policy)); >> + struct msr_policy *m = malloc(sizeof(struct msr_policy)); >> + struct cpu_policy a = { >> + &t->a.p, >> + &t->a.m, >> + }, b = { >> + &t->b.p, >> + &t->b.m, >> + }, out = { >> + p, >> + m, >> + }; >> + struct cpu_policy_errors e; >> + int res; >> + >> + if ( !p || !m ) >> + err(1, "%s() malloc failure", __func__); >> + >> + res = x86_cpu_policy_calculate_compatible(&a, &b, &out, &e); >> + >> + /* Check the expected error output. */ >> + if ( res != 0 || memcmp(&no_errors, &e, sizeof(no_errors)) ) >> + { >> + fail(" Test '%s' expected no errors\n" >> + " got res %d { leaf %08x, subleaf %08x, msr %08x }\n", >> + t->name, res, e.leaf, e.subleaf, e.msr); >> + goto test_done; >> + } >> + >> + if ( memcmp(&t->out.p, p, sizeof(*p)) ) >> + { >> + fail(" Test '%s' resulting CPUID policy not as expected\n", >> + t->name); >> + goto test_done; >> + } >> + >> + if ( memcmp(&t->out.m, m, sizeof(*m)) ) >> + { >> + fail(" Test '%s' resulting MSR policy not as expected\n", >> + t->name); >> + goto test_done; >> + } > In order to assert that we don't mess things up, I would also add a > x86_cpu_policies_are_compatible check here: > > res = x86_cpu_policies_are_compatible(&a, &out, &e); > if ( res ) > fail(" Test '%s' created incompatible policy\n" > " got res %d { leaf %08x, subleaf %08x, msr %08x }\n", > t->name, res, e.leaf, e.subleaf, e.msr); > res = x86_cpu_policies_are_compatible(&b, &out, &e); > if ( res ) > fail(" Test '%s' created incompatible policy\n" > " got res %d { leaf %08x, subleaf %08x, msr %08x }\n", > t->name, res, e.leaf, e.subleaf, e.msr); That's potentially problematic, hence not including it the first time around. See the discussion below. >> + >> + test_done: >> + free(p); >> + free(m); >> + } >> +} >> + >> int main(int argc, char **argv) >> { >> printf("CPU Policy unit tests\n"); >> @@ -793,6 +941,8 @@ int main(int argc, char **argv) >> test_is_compatible_success(); >> test_is_compatible_failure(); >> >> + test_calculate_compatible_success(); >> + >> if ( nr_failures ) >> printf("Done: %u failures\n", nr_failures); >> else >> diff --git a/xen/include/xen/lib/x86/cpu-policy.h b/xen/include/xen/lib/x86/cpu-policy.h >> index 5a2c4c7b2d..0422a15557 100644 >> --- a/xen/include/xen/lib/x86/cpu-policy.h >> +++ b/xen/include/xen/lib/x86/cpu-policy.h >> @@ -37,6 +37,28 @@ int x86_cpu_policies_are_compatible(const struct cpu_policy *host, >> const struct cpu_policy *guest, >> struct cpu_policy_errors *err); >> >> +/* >> + * Given two policies, calculate one which is compatible with each. >> + * >> + * i.e. Given host @a and host @b, calculate what to give a VM so it can live >> + * migrate between the two. >> + * >> + * @param a A cpu_policy. >> + * @param b Another cpu_policy. >> + * @param out A policy compatible with @a and @b. >> + * @param err Optional hint for error diagnostics. >> + * @returns -errno >> + * >> + * For typical usage, @a and @b should be system policies of the same type >> + * (i.e. PV/HVM default/max) from different hosts. In the case that an >> + * incompatibility is detected, the optional err pointer may identify the >> + * problematic leaf/subleaf and/or MSR. >> + */ >> +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); >> + >> #endif /* !XEN_LIB_X86_POLICIES_H */ >> >> /* >> diff --git a/xen/lib/x86/policy.c b/xen/lib/x86/policy.c >> index f6cea4e2f9..06039e8aa8 100644 >> --- 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); > It might be nice to expand test_is_compatible_{success,failure} to > account for arch_caps being checked now. At some point we're going to need to stop unit testing "does the AND operator work", and limit testing to the interesting corner cases. > Shouldn't this check also take into account that host might not have > RSBA set, but it's legit for a guest policy to have it? When we expose this properly to guests, the max policies will have RSBA set, and the default policies will have RSBA forwarded from hardware and/or the model table. Therefore, we can accept any VM RSBA configuration, irrespective of the particulars of this host, but if you e.g. have a pool of haswell's, the default policy will have RSBA clear across the board, and the VM won't see it. > if ( ~host->msr->arch_caps.raw & guest->msr->arch_caps.raw & ~POL_MASK ) > FAIL_MSR(MSR_ARCH_CAPABILITIES); > > Maybe POL_MASK should be renamed and defined in a header so it's > widely available? No - this would be incorrect. The polarity of certain bits only matters for levelling calculations, not for "can this VM run on this host" calculations. If the VM has seen RSBA, and Xen doesn't know about it, the VM cannot run. > >> + >> #undef FAIL_MSR >> #undef FAIL_CPUID >> #undef NA >> @@ -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) > I think this should be in an #ifndef __XEN__ protected region? > > There's no need to expose this to the hypervisor, as I would expect it > will never have to do compatible policy generation? (ie: it will > always be done by the toolstack?) As indicated previously, I still think we want this in Xen for the boot paths, but I suppose the guard was my suggestion to you, so is only fair at this point. > >> +{ >> + 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; >> + >> + memset(cp, 0, sizeof(*cp)); >> + memset(mp, 0, sizeof(*mp)); >> + >> + cp->basic.max_leaf = min(ap->basic.max_leaf, bp->basic.max_leaf); >> + >> + if ( cp->basic.max_leaf >= 7 ) >> + { >> + cp->feat.max_subleaf = min(ap->feat.max_subleaf, bp->feat.max_subleaf); >> + >> + cp->feat.raw[0].b = ap->feat.raw[0].b & bp->feat.raw[0].b; >> + cp->feat.raw[0].c = ap->feat.raw[0].c & bp->feat.raw[0].c; >> + cp->feat.raw[0].d = ap->feat.raw[0].d & bp->feat.raw[0].d; >> + } >> + >> + /* TODO: Far more. */ > Right, my proposed patch (07/13) went a bit further and also leveled > 1c, 1d, Da1, e1c, e1d, e7d, e8b and e21a, and we also need to level > a couple of max_leaf fields. > > I'm happy for this to go in first, and I can rebase the extra logic I > have on top of this one. There is a lot of work to do. One thing I haven't addressed yet is the fact is things which don't level, e.g. vendor. You've got to pick one, and there isn't a mathematical relationship to use between a and b. I think for that, we ought to document that we strictly take from a. This makes the operation not commutative, and in particular, I don't think we want to waste too much time/effort trying to make cross-vendor cases work - it was a stunt a decade ago, with a huge number of sharp corners, as well as creating a number of XSAs due to poor implementation. For v1, I suggest we firmly stick to the same-vendor case. It's not as if there is a lack of things to do to make this work. ~Andrew ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] libx86: Introduce x86_cpu_policy_calculate_compatible() with MSR_ARCH_CAPS handling 2021-05-05 12:37 ` Andrew Cooper @ 2021-05-05 13:02 ` Roger Pau Monné 2021-05-05 14:29 ` Andrew Cooper 0 siblings, 1 reply; 12+ messages in thread From: Roger Pau Monné @ 2021-05-05 13:02 UTC (permalink / raw) To: Andrew Cooper; +Cc: Xen-devel, Jan Beulich, Wei Liu On Wed, May 05, 2021 at 01:37:48PM +0100, Andrew Cooper wrote: > On 05/05/2021 11:04, Roger Pau Monné wrote: > > On Tue, May 04, 2021 at 10:31:20PM +0100, Andrew Cooper wrote: > >> diff --git a/tools/tests/cpu-policy/test-cpu-policy.c b/tools/tests/cpu-policy/test-cpu-policy.c > >> index 75973298df..455b4fe3c0 100644 > >> --- 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, > >> + }, > >> + .out = { > >> + .p.basic.max_leaf = 6, > >> + }, > >> + }, > >> + { > >> + "arch_caps, b feat missing", > >> + .a = { > >> + .p.basic.max_leaf = 7, > >> + .p.feat.arch_caps = true, > >> + .m.arch_caps.rdcl_no = true, > >> + }, > >> + .b = { > >> + .p.basic.max_leaf = 7, > >> + .m.arch_caps.rdcl_no = true, > >> + }, > >> + .out = { > >> + .p.basic.max_leaf = 7, > >> + }, > >> + }, > >> + { > >> + "arch_caps, b rdcl_no missing", > >> + .a = { > >> + .p.basic.max_leaf = 7, > >> + .p.feat.arch_caps = true, > >> + .m.arch_caps.rdcl_no = true, > >> + }, > >> + .b = { > >> + .p.basic.max_leaf = 7, > >> + .p.feat.arch_caps = true, > >> + }, > >> + .out = { > >> + .p.basic.max_leaf = 7, > >> + .p.feat.arch_caps = true, > >> + }, > >> + }, > >> + { > >> + "arch_caps, rdcl_no ok", > >> + .a = { > >> + .p.basic.max_leaf = 7, > >> + .p.feat.arch_caps = true, > >> + .m.arch_caps.rdcl_no = true, > >> + }, > >> + .b = { > >> + .p.basic.max_leaf = 7, > >> + .p.feat.arch_caps = true, > >> + .m.arch_caps.rdcl_no = true, > >> + }, > >> + .out = { > >> + .p.basic.max_leaf = 7, > >> + .p.feat.arch_caps = true, > >> + .m.arch_caps.rdcl_no = true, > >> + }, > >> + }, > >> + { > >> + "arch_caps, rsba accum", > >> + .a = { > >> + .p.basic.max_leaf = 7, > >> + .p.feat.arch_caps = true, > >> + .m.arch_caps.rsba = true, > >> + }, > >> + .b = { > >> + .p.basic.max_leaf = 7, > >> + .p.feat.arch_caps = true, > >> + }, > >> + .out = { > >> + .p.basic.max_leaf = 7, > >> + .p.feat.arch_caps = true, > >> + .m.arch_caps.rsba = true, > >> + }, > >> + }, > >> + }; > >> + struct cpu_policy_errors no_errors = INIT_CPU_POLICY_ERRORS; > >> + > >> + printf("Testing calculate compatibility success:\n"); > >> + > >> + for ( size_t i = 0; i < ARRAY_SIZE(tests); ++i ) > >> + { > >> + struct test *t = &tests[i]; > >> + struct cpuid_policy *p = malloc(sizeof(struct cpuid_policy)); > >> + struct msr_policy *m = malloc(sizeof(struct msr_policy)); > >> + struct cpu_policy a = { > >> + &t->a.p, > >> + &t->a.m, > >> + }, b = { > >> + &t->b.p, > >> + &t->b.m, > >> + }, out = { > >> + p, > >> + m, > >> + }; > >> + struct cpu_policy_errors e; > >> + int res; > >> + > >> + if ( !p || !m ) > >> + err(1, "%s() malloc failure", __func__); > >> + > >> + res = x86_cpu_policy_calculate_compatible(&a, &b, &out, &e); > >> + > >> + /* Check the expected error output. */ > >> + if ( res != 0 || memcmp(&no_errors, &e, sizeof(no_errors)) ) > >> + { > >> + fail(" Test '%s' expected no errors\n" > >> + " got res %d { leaf %08x, subleaf %08x, msr %08x }\n", > >> + t->name, res, e.leaf, e.subleaf, e.msr); > >> + goto test_done; > >> + } > >> + > >> + if ( memcmp(&t->out.p, p, sizeof(*p)) ) > >> + { > >> + fail(" Test '%s' resulting CPUID policy not as expected\n", > >> + t->name); > >> + goto test_done; > >> + } > >> + > >> + if ( memcmp(&t->out.m, m, sizeof(*m)) ) > >> + { > >> + fail(" Test '%s' resulting MSR policy not as expected\n", > >> + t->name); > >> + goto test_done; > >> + } > > In order to assert that we don't mess things up, I would also add a > > x86_cpu_policies_are_compatible check here: > > > > res = x86_cpu_policies_are_compatible(&a, &out, &e); > > if ( res ) > > fail(" Test '%s' created incompatible policy\n" > > " got res %d { leaf %08x, subleaf %08x, msr %08x }\n", > > t->name, res, e.leaf, e.subleaf, e.msr); > > res = x86_cpu_policies_are_compatible(&b, &out, &e); > > if ( res ) > > fail(" Test '%s' created incompatible policy\n" > > " got res %d { leaf %08x, subleaf %08x, msr %08x }\n", > > t->name, res, e.leaf, e.subleaf, e.msr); > > That's potentially problematic, hence not including it the first time > around. See the discussion below. Right, I think being able to do what I propose would also allow to detect missing bits between x86_cpu_policy_calculate_compatible and x86_cpu_policies_are_compatible. > >> + > >> + test_done: > >> + free(p); > >> + free(m); > >> + } > >> +} > >> + > >> int main(int argc, char **argv) > >> { > >> printf("CPU Policy unit tests\n"); > >> @@ -793,6 +941,8 @@ int main(int argc, char **argv) > >> test_is_compatible_success(); > >> test_is_compatible_failure(); > >> > >> + test_calculate_compatible_success(); > >> + > >> if ( nr_failures ) > >> printf("Done: %u failures\n", nr_failures); > >> else > >> diff --git a/xen/include/xen/lib/x86/cpu-policy.h b/xen/include/xen/lib/x86/cpu-policy.h > >> index 5a2c4c7b2d..0422a15557 100644 > >> --- a/xen/include/xen/lib/x86/cpu-policy.h > >> +++ b/xen/include/xen/lib/x86/cpu-policy.h > >> @@ -37,6 +37,28 @@ int x86_cpu_policies_are_compatible(const struct cpu_policy *host, > >> const struct cpu_policy *guest, > >> struct cpu_policy_errors *err); > >> > >> +/* > >> + * Given two policies, calculate one which is compatible with each. > >> + * > >> + * i.e. Given host @a and host @b, calculate what to give a VM so it can live > >> + * migrate between the two. > >> + * > >> + * @param a A cpu_policy. > >> + * @param b Another cpu_policy. > >> + * @param out A policy compatible with @a and @b. > >> + * @param err Optional hint for error diagnostics. > >> + * @returns -errno > >> + * > >> + * For typical usage, @a and @b should be system policies of the same type > >> + * (i.e. PV/HVM default/max) from different hosts. In the case that an > >> + * incompatibility is detected, the optional err pointer may identify the > >> + * problematic leaf/subleaf and/or MSR. > >> + */ > >> +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); > >> + > >> #endif /* !XEN_LIB_X86_POLICIES_H */ > >> > >> /* > >> diff --git a/xen/lib/x86/policy.c b/xen/lib/x86/policy.c > >> index f6cea4e2f9..06039e8aa8 100644 > >> --- 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); > > It might be nice to expand test_is_compatible_{success,failure} to > > account for arch_caps being checked now. > > At some point we're going to need to stop unit testing "does the AND > operator work", and limit testing to the interesting corner cases. > > > Shouldn't this check also take into account that host might not have > > RSBA set, but it's legit for a guest policy to have it? > > When we expose this properly to guests, the max policies will have RSBA > set, and the default policies will have RSBA forwarded from hardware > and/or the model table. > > Therefore, we can accept any VM RSBA configuration, irrespective of the > particulars of this host, but if you e.g. have a pool of haswell's, the > default policy will have RSBA clear across the board, and the VM won't > see it. > > > if ( ~host->msr->arch_caps.raw & guest->msr->arch_caps.raw & ~POL_MASK ) > > FAIL_MSR(MSR_ARCH_CAPABILITIES); > > > > Maybe POL_MASK should be renamed and defined in a header so it's > > widely available? > > No - this would be incorrect. The polarity of certain bits only matters > for levelling calculations, not for "can this VM run on this host" > calculations. > > If the VM has seen RSBA, and Xen doesn't know about it, the VM cannot run. But then the logic relation between x86_cpu_policy_calculate_compatible and x86_cpu_policies_are_compatible is broken AFAICT. If you give x86_cpu_policy_calculate_compatible one policy with RSBA set and one without it will generate a compatible policy, yet that output will be regarded as not compatible if feed into x86_cpu_policies_are_compatible against the policy that doesn't have RSBA set. I think the output from x86_cpu_policy_calculate_compatible should strictly return true when checked against any of the inputs using x86_cpu_policies_are_compatible, or else we need to note it somewhere because I think it's not the expected behavior. > > > >> + > >> #undef FAIL_MSR > >> #undef FAIL_CPUID > >> #undef NA > >> @@ -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) > > I think this should be in an #ifndef __XEN__ protected region? > > > > There's no need to expose this to the hypervisor, as I would expect it > > will never have to do compatible policy generation? (ie: it will > > always be done by the toolstack?) > > As indicated previously, I still think we want this in Xen for the boot > paths, but I suppose the guard was my suggestion to you, so is only fair > at this point. TBH I replied before seeing your email that also had this suggestion. If it's indeed going to be used by Xen itself then that's fine, but I couldn't figure out why the hypervisor would need to generate compatible policies itself. Maybe it will be used to generate the initial policies? > > > >> +{ > >> + 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; > >> + > >> + memset(cp, 0, sizeof(*cp)); > >> + memset(mp, 0, sizeof(*mp)); > >> + > >> + cp->basic.max_leaf = min(ap->basic.max_leaf, bp->basic.max_leaf); > >> + > >> + if ( cp->basic.max_leaf >= 7 ) > >> + { > >> + cp->feat.max_subleaf = min(ap->feat.max_subleaf, bp->feat.max_subleaf); > >> + > >> + cp->feat.raw[0].b = ap->feat.raw[0].b & bp->feat.raw[0].b; > >> + cp->feat.raw[0].c = ap->feat.raw[0].c & bp->feat.raw[0].c; > >> + cp->feat.raw[0].d = ap->feat.raw[0].d & bp->feat.raw[0].d; > >> + } > >> + > >> + /* TODO: Far more. */ > > Right, my proposed patch (07/13) went a bit further and also leveled > > 1c, 1d, Da1, e1c, e1d, e7d, e8b and e21a, and we also need to level > > a couple of max_leaf fields. > > > > I'm happy for this to go in first, and I can rebase the extra logic I > > have on top of this one. > > There is a lot of work to do. > > One thing I haven't addressed yet is the fact is things which don't > level, e.g. vendor. You've got to pick one, and there isn't a > mathematical relationship to use between a and b. > > I think for that, we ought to document that we strictly take from a. > This makes the operation not commutative, and in particular, I don't > think we want to waste too much time/effort trying to make cross-vendor > cases work - it was a stunt a decade ago, with a huge number of sharp > corners, as well as creating a number of XSAs due to poor implementation. > > For v1, I suggest we firmly stick to the same-vendor case. It's not as > if there is a lack of things to do to make this work. OK, so level all the feature fields and pick the non feature parts of cpuid strictly from one of the inputs. Thanks, Roger. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] libx86: Introduce x86_cpu_policy_calculate_compatible() with MSR_ARCH_CAPS handling 2021-05-05 13:02 ` Roger Pau Monné @ 2021-05-05 14:29 ` Andrew Cooper 2021-05-05 14:48 ` Jan Beulich 0 siblings, 1 reply; 12+ messages in thread From: Andrew Cooper @ 2021-05-05 14:29 UTC (permalink / raw) To: Roger Pau Monné; +Cc: Xen-devel, Jan Beulich, Wei Liu On 05/05/2021 14:02, Roger Pau Monné wrote: > On Wed, May 05, 2021 at 01:37:48PM +0100, Andrew Cooper wrote: >> On 05/05/2021 11:04, Roger Pau Monné wrote: >>> On Tue, May 04, 2021 at 10:31:20PM +0100, Andrew Cooper wrote: >>>> diff --git a/xen/lib/x86/policy.c b/xen/lib/x86/policy.c >>>> index f6cea4e2f9..06039e8aa8 100644 >>>> --- 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); >>> It might be nice to expand test_is_compatible_{success,failure} to >>> account for arch_caps being checked now. >> At some point we're going to need to stop unit testing "does the AND >> operator work", and limit testing to the interesting corner cases. >> >>> Shouldn't this check also take into account that host might not have >>> RSBA set, but it's legit for a guest policy to have it? >> When we expose this properly to guests, the max policies will have RSBA >> set, and the default policies will have RSBA forwarded from hardware >> and/or the model table. >> >> Therefore, we can accept any VM RSBA configuration, irrespective of the >> particulars of this host, but if you e.g. have a pool of haswell's, the >> default policy will have RSBA clear across the board, and the VM won't >> see it. >> >>> if ( ~host->msr->arch_caps.raw & guest->msr->arch_caps.raw & ~POL_MASK ) >>> FAIL_MSR(MSR_ARCH_CAPABILITIES); >>> >>> Maybe POL_MASK should be renamed and defined in a header so it's >>> widely available? >> No - this would be incorrect. The polarity of certain bits only matters >> for levelling calculations, not for "can this VM run on this host" >> calculations. >> >> If the VM has seen RSBA, and Xen doesn't know about it, the VM cannot run. > But then the logic relation between > x86_cpu_policy_calculate_compatible and > x86_cpu_policies_are_compatible is broken AFAICT. > > If you give x86_cpu_policy_calculate_compatible one policy with RSBA set > and one without it will generate a compatible policy, yet that output > will be regarded as not compatible if feed into > x86_cpu_policies_are_compatible against the policy that doesn't have > RSBA set. > > I think the output from x86_cpu_policy_calculate_compatible should > strictly return true when checked against any of the inputs using > x86_cpu_policies_are_compatible, or else we need to note it somewhere > because I think it's not the expected behavior. Welcome to the monumental complexity, and the reason why this isn't 5 minutes of work. This is just the tip of the iceberg. "Please create me a policy for a VM" is conducted across PV/HVM default policies, and/or user settings, while "Can this VM run on this host" is checked against the max policy. This split is necessary to cope with the corner cases. So no - levelling max policies isn't expected to result in anything useful, and calling is_compatible with a default (rather than max) host setting also isn't going result in a useful answer. And yes - for some changes, RSBA included, you're going to need to update all your Xen's across the pool before migration is going to work, but that's already the case now. Tangentially, we haven't started yet on struct irritating_corner_cases { bool vm_not_using_fcs_fds; bool vm_not_using_lbr; ... }; which will require explicit user opt-in to override the "No - you can't migration across the IvyBridge/Haswell, or pre-Zen/Zen boundary". Technically, MCXSR_MASK is also a hard blocker to migration, but we don't even have that data in a consumable form, and we just might be extremely lucky and discover that it is restricted to non-64-bit CPUs. Migration with a VM having turned on LBR is still a disaster. For now, we drop everything on the floor, and let the VM explode if the LBR_FORMAT has changed, or if the number of stack entries changes (which does change with Hyperthreading enabled/disabled in firmware). >>>> + >>>> #undef FAIL_MSR >>>> #undef FAIL_CPUID >>>> #undef NA >>>> @@ -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) >>> I think this should be in an #ifndef __XEN__ protected region? >>> >>> There's no need to expose this to the hypervisor, as I would expect it >>> will never have to do compatible policy generation? (ie: it will >>> always be done by the toolstack?) >> As indicated previously, I still think we want this in Xen for the boot >> paths, but I suppose the guard was my suggestion to you, so is only fair >> at this point. > TBH I replied before seeing your email that also had this suggestion. > If it's indeed going to be used by Xen itself then that's fine, but I > couldn't figure out why the hypervisor would need to generate > compatible policies itself. > > Maybe it will be used to generate the initial policies? Yes. > >>>> +{ >>>> + 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; >>>> + >>>> + memset(cp, 0, sizeof(*cp)); >>>> + memset(mp, 0, sizeof(*mp)); >>>> + >>>> + cp->basic.max_leaf = min(ap->basic.max_leaf, bp->basic.max_leaf); >>>> + >>>> + if ( cp->basic.max_leaf >= 7 ) >>>> + { >>>> + cp->feat.max_subleaf = min(ap->feat.max_subleaf, bp->feat.max_subleaf); >>>> + >>>> + cp->feat.raw[0].b = ap->feat.raw[0].b & bp->feat.raw[0].b; >>>> + cp->feat.raw[0].c = ap->feat.raw[0].c & bp->feat.raw[0].c; >>>> + cp->feat.raw[0].d = ap->feat.raw[0].d & bp->feat.raw[0].d; >>>> + } >>>> + >>>> + /* TODO: Far more. */ >>> Right, my proposed patch (07/13) went a bit further and also leveled >>> 1c, 1d, Da1, e1c, e1d, e7d, e8b and e21a, and we also need to level >>> a couple of max_leaf fields. >>> >>> I'm happy for this to go in first, and I can rebase the extra logic I >>> have on top of this one. >> There is a lot of work to do. >> >> One thing I haven't addressed yet is the fact is things which don't >> level, e.g. vendor. You've got to pick one, and there isn't a >> mathematical relationship to use between a and b. >> >> I think for that, we ought to document that we strictly take from a. >> This makes the operation not commutative, and in particular, I don't >> think we want to waste too much time/effort trying to make cross-vendor >> cases work - it was a stunt a decade ago, with a huge number of sharp >> corners, as well as creating a number of XSAs due to poor implementation. >> >> For v1, I suggest we firmly stick to the same-vendor case. It's not as >> if there is a lack of things to do to make this work. > OK, so level all the feature fields and pick the non feature parts of > cpuid strictly from one of the inputs. The awkward part to address is that we've still got simultaneous equations with feature handling. I'm fairly certain that the simple and's which both you and I did won't be sufficient in due course. ~Andrew ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] libx86: Introduce x86_cpu_policy_calculate_compatible() with MSR_ARCH_CAPS handling 2021-05-05 14:29 ` Andrew Cooper @ 2021-05-05 14:48 ` Jan Beulich 2021-05-05 14:50 ` Andrew Cooper 0 siblings, 1 reply; 12+ messages in thread From: Jan Beulich @ 2021-05-05 14:48 UTC (permalink / raw) To: Andrew Cooper; +Cc: Xen-devel, Wei Liu, Roger Pau Monné On 05.05.2021 16:29, Andrew Cooper wrote: > Technically, MCXSR_MASK is also a hard blocker to migration, but we > don't even have that data in a consumable form, and we just might be > extremely lucky and discover that it is restricted to non-64-bit CPUs. "it" being what here? The value's presence / absence in an {F,}XSAVE image? Or the precise value of it? Jan ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] libx86: Introduce x86_cpu_policy_calculate_compatible() with MSR_ARCH_CAPS handling 2021-05-05 14:48 ` Jan Beulich @ 2021-05-05 14:50 ` Andrew Cooper 2021-05-05 15:00 ` Jan Beulich 0 siblings, 1 reply; 12+ messages in thread From: Andrew Cooper @ 2021-05-05 14:50 UTC (permalink / raw) To: Jan Beulich; +Cc: Xen-devel, Wei Liu, Roger Pau Monné On 05/05/2021 15:48, Jan Beulich wrote: > On 05.05.2021 16:29, Andrew Cooper wrote: >> Technically, MCXSR_MASK is also a hard blocker to migration, but we >> don't even have that data in a consumable form, and we just might be >> extremely lucky and discover that it is restricted to non-64-bit CPUs. > "it" being what here? The value's presence / absence in an {F,}XSAVE > image? Or the precise value of it? The precise value of it. Migrating across the boundary where the default changed will cause {F,}RSTOR instructions to #GP. ~Andrew ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] libx86: Introduce x86_cpu_policy_calculate_compatible() with MSR_ARCH_CAPS handling 2021-05-05 14:50 ` Andrew Cooper @ 2021-05-05 15:00 ` Jan Beulich 2021-05-05 15:18 ` Andrew Cooper 0 siblings, 1 reply; 12+ messages in thread From: Jan Beulich @ 2021-05-05 15:00 UTC (permalink / raw) To: Andrew Cooper; +Cc: Xen-devel, Wei Liu, Roger Pau Monné On 05.05.2021 16:50, Andrew Cooper wrote: > On 05/05/2021 15:48, Jan Beulich wrote: >> On 05.05.2021 16:29, Andrew Cooper wrote: >>> Technically, MCXSR_MASK is also a hard blocker to migration, but we >>> don't even have that data in a consumable form, and we just might be >>> extremely lucky and discover that it is restricted to non-64-bit CPUs. >> "it" being what here? The value's presence / absence in an {F,}XSAVE >> image? Or the precise value of it? > > The precise value of it. Not sure whether DAZ is new enough, but relatively sure MM isn't. > Migrating across the boundary where the > default changed will cause {F,}RSTOR instructions to #GP. That's understood. Is there actually anything standing in the way of treating MXCSR_MASK like yet another feature flag group? Jan ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] libx86: Introduce x86_cpu_policy_calculate_compatible() with MSR_ARCH_CAPS handling 2021-05-05 15:00 ` Jan Beulich @ 2021-05-05 15:18 ` Andrew Cooper 0 siblings, 0 replies; 12+ messages in thread From: Andrew Cooper @ 2021-05-05 15:18 UTC (permalink / raw) To: Jan Beulich; +Cc: Xen-devel, Wei Liu, Roger Pau Monné On 05/05/2021 16:00, Jan Beulich wrote: > On 05.05.2021 16:50, Andrew Cooper wrote: >> On 05/05/2021 15:48, Jan Beulich wrote: >>> On 05.05.2021 16:29, Andrew Cooper wrote: >>>> Technically, MCXSR_MASK is also a hard blocker to migration, but we >>>> don't even have that data in a consumable form, and we just might be >>>> extremely lucky and discover that it is restricted to non-64-bit CPUs. >>> "it" being what here? The value's presence / absence in an {F,}XSAVE >>> image? Or the precise value of it? >> The precise value of it. > Not sure whether DAZ is new enough, but relatively sure MM isn't. > >> Migrating across the boundary where the >> default changed will cause {F,}RSTOR instructions to #GP. > That's understood. Is there actually anything standing in the way > of treating MXCSR_MASK like yet another feature flag group? Well - we'd need to find somewhere to put it. It doesn't fit in architectural CPUID or MSR information. We could add a 3rd category of information in "a cpu policy", and that is always an option. However, this issue hasn't been a problem so far, is only for very legacy CPUs at this point, and isn't high on my list of worries. ~Andrew ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2021-05-05 15:18 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-05-04 21:31 [PATCH] libx86: Introduce x86_cpu_policy_calculate_compatible() with MSR_ARCH_CAPS handling Andrew Cooper 2021-05-05 6:39 ` Jan Beulich 2021-05-05 12:15 ` Andrew Cooper 2021-05-05 12:32 ` Jan Beulich 2021-05-05 10:04 ` Roger Pau Monné 2021-05-05 12:37 ` Andrew Cooper 2021-05-05 13:02 ` Roger Pau Monné 2021-05-05 14:29 ` Andrew Cooper 2021-05-05 14:48 ` Jan Beulich 2021-05-05 14:50 ` Andrew Cooper 2021-05-05 15:00 ` Jan Beulich 2021-05-05 15:18 ` Andrew Cooper
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).