| Message ID | 20190911200504.5693-2-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/8] libx86: Introduce x86_cpu_policies_are_compatible() | expand |
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
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
On 12.09.2019 09:59, Andrew Cooper wrote: > 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. Right, I did notice this in a later patch. But yes, I do think having the functions proactively fill the structures would be better overall (and remove the need to use the initializer in at least some cases, i.e. where there are no other early error paths). Jan
On 12/09/2019 09:22, Jan Beulich wrote: > On 12.09.2019 09:59, Andrew Cooper wrote: >> 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. > Right, I did notice this in a later patch. But yes, I do think > having the functions proactively fill the structures would be > better overall (and remove the need to use the initializer in at > least some cases, i.e. where there are no other early error paths). Ok. I'll switch to doing this, and have another prep patch which adjusts the behaviour of the already existing functions. ~Andrew
diff --git a/tools/tests/cpu-policy/Makefile b/tools/tests/cpu-policy/Makefile index fb548c9b9a..70ff154da6 100644 --- a/tools/tests/cpu-policy/Makefile +++ b/tools/tests/cpu-policy/Makefile @@ -39,7 +39,7 @@ CFLAGS += $(APPEND_CFLAGS) vpath %.c ../../../xen/lib/x86 -test-cpu-policy: test-cpu-policy.o msr.o cpuid.o +test-cpu-policy: test-cpu-policy.o msr.o cpuid.o policy.o $(CC) $(CFLAGS) $^ -o $@ -include $(DEPS_INCLUDE) diff --git a/tools/tests/cpu-policy/test-cpu-policy.c b/tools/tests/cpu-policy/test-cpu-policy.c index fe00cd4276..10cfa7cd97 100644 --- a/tools/tests/cpu-policy/test-cpu-policy.c +++ b/tools/tests/cpu-policy/test-cpu-policy.c @@ -9,8 +9,7 @@ #include <xen-tools/libs.h> #include <xen/asm/x86-vendors.h> -#include <xen/lib/x86/cpuid.h> -#include <xen/lib/x86/msr.h> +#include <xen/lib/x86/cpu-policy.h> #include <xen/domctl.h> static unsigned int nr_failures; @@ -503,6 +502,111 @@ static void test_cpuid_out_of_range_clearing(void) } } +static void test_is_compatible_success(void) +{ + static struct test { + const char *name; + struct cpuid_policy host_cpuid; + struct cpuid_policy guest_cpuid; + struct msr_policy host_msr; + struct msr_policy guest_msr; + } tests[] = { + { + .name = "Host CPUID faulting, Guest not", + .host_msr = { + .plaform_info.cpuid_faulting = true, + }, + }, + { + .name = "Host CPUID faulting, Guest wanted", + .host_msr = { + .plaform_info.cpuid_faulting = true, + }, + .guest_msr = { + .plaform_info.cpuid_faulting = true, + }, + }, + }; + struct cpu_policy_errors no_errors = INIT_CPU_POLICY_ERRORS; + + printf("Testing policy compatibility success:\n"); + + for ( size_t i = 0; i < ARRAY_SIZE(tests); ++i ) + { + struct test *t = &tests[i]; + struct cpu_policy sys = { + &t->host_cpuid, + &t->host_msr, + }, new = { + &t->guest_cpuid, + &t->guest_msr, + }; + struct cpu_policy_errors e = INIT_CPU_POLICY_ERRORS; + int res = x86_cpu_policies_are_compatible(&sys, &new, &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); + } +} + +static void test_is_compatible_failure(void) +{ + static struct test { + const char *name; + struct cpuid_policy host_cpuid; + struct cpuid_policy guest_cpuid; + struct msr_policy host_msr; + struct msr_policy guest_msr; + struct cpu_policy_errors e; + } tests[] = { + { + .name = "Host basic.max_leaf out of range", + .guest_cpuid.basic.max_leaf = 1, + .e = { 0, -1, -1 }, + }, + { + .name = "Host extd.max_leaf out of range", + .guest_cpuid.extd.max_leaf = 1, + .e = { 0x80000008, -1, -1 }, + }, + { + .name = "Host no CPUID faulting, Guest wanted", + .guest_msr = { + .plaform_info.cpuid_faulting = true, + }, + .e = { -1, -1, 0xce }, + }, + }; + + printf("Testing policy compatibility failure:\n"); + + for ( size_t i = 0; i < ARRAY_SIZE(tests); ++i ) + { + struct test *t = &tests[i]; + struct cpu_policy sys = { + &t->host_cpuid, + &t->host_msr, + }, new = { + &t->guest_cpuid, + &t->guest_msr, + }; + struct cpu_policy_errors e = INIT_CPU_POLICY_ERRORS; + int res = x86_cpu_policies_are_compatible(&sys, &new, &e); + + /* Check the expected error output. */ + if ( res == 0 || memcmp(&t->e, &e, sizeof(t->e)) ) + fail(" Test '%s' res %d\n" + " expected { leaf %08x, subleaf %08x, msr %08x }\n" + " got { leaf %08x, subleaf %08x, msr %08x }\n", + t->name, res, + t->e.leaf, t->e.subleaf, t->e.msr, + e.leaf, e.subleaf, e.msr); + } +} + int main(int argc, char **argv) { printf("CPU Policy unit tests\n"); @@ -516,6 +620,9 @@ int main(int argc, char **argv) test_msr_serialise_success(); test_msr_deserialise_failure(); + test_is_compatible_success(); + test_is_compatible_failure(); + 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 6f07c4b493..65ec71835b 100644 --- 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 } + +/* + * Calculate whether two policies are compatible. + * + * i.e. Can a VM configured with @guest run on a CPU supporting @host. + * + * For typical usage, @host should be a system policy. + */ +int x86_cpu_policies_are_compatible(const struct cpu_policy *host, + const struct cpu_policy *guest, + struct cpu_policy_errors *e); + #endif /* !XEN_LIB_X86_POLICIES_H */ /* diff --git a/xen/lib/x86/Makefile b/xen/lib/x86/Makefile index 2f9691e964..780ea05db1 100644 --- a/xen/lib/x86/Makefile +++ b/xen/lib/x86/Makefile @@ -1,2 +1,3 @@ obj-y += cpuid.o obj-y += msr.o +obj-y += policy.o diff --git a/xen/lib/x86/policy.c b/xen/lib/x86/policy.c new file mode 100644 index 0000000000..3155e07a7c --- /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 ) + FAIL_MSR(MSR_INTEL_PLATFORM_INFO); + +#undef FAIL_MSR +#undef FAIL_CPUID +#undef NA + + /* Success. */ + ret = 0; + + out: + if ( ret && e ) + { + e->leaf = leaf; + e->subleaf = subleaf; + e->msr = msr; + } + + return ret; +} + +/* + * Local variables: + * mode: C + * c-file-style: "BSD" + * c-basic-offset: 4 + * tab-width: 4 + * indent-tabs-mode: nil + * End: + */
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> --- CC: Jan Beulich <JBeulich@suse.com> CC: Wei Liu <wl@xen.org> CC: Roger Pau Monné <roger.pau@citrix.com> --- tools/tests/cpu-policy/Makefile | 2 +- tools/tests/cpu-policy/test-cpu-policy.c | 111 ++++++++++++++++++++++++++++++- xen/include/xen/lib/x86/cpu-policy.h | 19 ++++++ xen/lib/x86/Makefile | 1 + xen/lib/x86/policy.c | 53 +++++++++++++++ 5 files changed, 183 insertions(+), 3 deletions(-) create mode 100644 xen/lib/x86/policy.c