| Message ID | 20221230162442.3781098-7-aaronlewis@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Clean up the supported xfeatures | expand |
On Fri, Dec 30, 2022, Aaron Lewis wrote: > +static uint64_t get_supported_user_xfeatures(void) I would rather put this in processor.h too, with a "this_cpu" prefix. Maybe this_cpu_supported_xcr0() or this_cpu_supported_user_xfeatures()? > +{ > + uint32_t a, b, c, d; > + > + cpuid(0xd, &a, &b, &c, &d); > + > + return a | ((uint64_t)d << 32); > +} > + > +static void guest_code(void) > +{ > + uint64_t xcr0_rest; s/rest/reset ? > + uint64_t supported_xcr0; > + uint64_t xfeature_mask; > + uint64_t supported_state; > + > + set_cr4(get_cr4() | X86_CR4_OSXSAVE); > + > + xcr0_rest = xgetbv(0); > + supported_xcr0 = get_supported_user_xfeatures(); > + > + GUEST_ASSERT(xcr0_rest == 1ul); XFEATURE_MASK_FP instead of 1ul. > + > + /* Check AVX */ > + xfeature_mask = XFEATURE_MASK_SSE | XFEATURE_MASK_YMM; > + supported_state = supported_xcr0 & xfeature_mask; > + GUEST_ASSERT(supported_state != XFEATURE_MASK_YMM); Oof, this took me far too long to read correctly. What about? /* AVX can be supported if and only if SSE is supported. */ GUEST_ASSERT((supported_xcr0 & XFEATURE_MASK_SSE) || !(supported_xcr0 & XFEATURE_MASK_YMM)); Hmm or maybe add helpers? Printing the info on failure would also make it easier to debug. E.g. static void check_all_or_none_xfeature(uint64_t supported_xcr0, uint64_t mask) { supported_xcr0 &= mask; GUEST_ASSERT_2(!supported_xcr0 || supported_xcr0 == mask, supported_xcr0, mask); } static void check_xfeature_dependencies(uint64_t supported_xcr0, uint64_t mask, uint64_t dependencies) { supported_xcr0 &= (mask | dependencies); GUEST_ASSERT_3(!(supported_xcr0 & mask) || supported_xcr0 == (mask | dependencies), supported_xcr0, mask, dependencies); } would yield check_xfeature_dependencies(supported_xcr0, XFEATURE_MASK_YMM, XFEATURE_MASK_SSE); and then for AVX512: check_xfeature_dependencies(supported_xcr0, XFEATURE_MASK_AVX512, XFEATURE_MASK_SSE | XFEATURE_MASK_YMM); check_all_or_none_xfeature(supported_xcr0, XFEATURE_MASK_AVX512); That would more or less eliminate the need for comments, and IMO makes it more obvious what is being checked. > + xsetbv(0, supported_xcr0); > + > + GUEST_DONE(); > +} > + > +static void guest_gp_handler(struct ex_regs *regs) > +{ > + GUEST_ASSERT(!"Failed to set the supported xfeature bits in XCR0."); I'd rather add an xsetbv_safe() variant than install a #GP handler. That would also make it super easy to add negative testing. E.g. (completely untested) static inline uint8_t xsetbv_safe(uint32_t index, uint64_t value) { u32 eax = value; u32 edx = value >> 32; return kvm_asm_safe("xsetbv", "a" (eax), "d" (edx), "c" (index)); } and vector = xsetbv_safe(0, supported_xcr0); GUEST_ASSERT_2(!vector, supported_xcr0, vector); and rudimentary negative testing for (i = 0; i < 64; i++) { if (supported_xcr0 & BIT_ULL(i)) continue; vector = xsetbv_safe(0, supported_xcr0 | BIT_ULL(i)); GUEST_ASSERT_2(vector == GP_VECTOR, supported_xcr0, vector); }
On Wed, Jan 4, 2023 at 5:13 PM Sean Christopherson <seanjc@google.com> wrote: > > On Fri, Dec 30, 2022, Aaron Lewis wrote: > > +static uint64_t get_supported_user_xfeatures(void) > > I would rather put this in processor.h too, with a "this_cpu" prefix. Maybe > this_cpu_supported_xcr0() or this_cpu_supported_user_xfeatures()? this_cpu_supported_xcr0 works for me. > > > +{ > > + uint32_t a, b, c, d; > > + > > + cpuid(0xd, &a, &b, &c, &d); > > + > > + return a | ((uint64_t)d << 32); > > +} > > + > > +static void guest_code(void) > > +{ > > + uint64_t xcr0_rest; > > s/rest/reset ? > > > + uint64_t supported_xcr0; > > + uint64_t xfeature_mask; > > + uint64_t supported_state; > > + > > + set_cr4(get_cr4() | X86_CR4_OSXSAVE); > > + > > + xcr0_rest = xgetbv(0); > > + supported_xcr0 = get_supported_user_xfeatures(); > > + > > + GUEST_ASSERT(xcr0_rest == 1ul); > > XFEATURE_MASK_FP instead of 1ul. > > > + > > + /* Check AVX */ > > + xfeature_mask = XFEATURE_MASK_SSE | XFEATURE_MASK_YMM; > > + supported_state = supported_xcr0 & xfeature_mask; > > + GUEST_ASSERT(supported_state != XFEATURE_MASK_YMM); > > Oof, this took me far too long to read correctly. What about? > > /* AVX can be supported if and only if SSE is supported. */ > GUEST_ASSERT((supported_xcr0 & XFEATURE_MASK_SSE) || > !(supported_xcr0 & XFEATURE_MASK_YMM)); > > Hmm or maybe add helpers? Printing the info on failure would also make it easier > to debug. E.g. > > static void check_all_or_none_xfeature(uint64_t supported_xcr0, uint64_t mask) > { > supported_xcr0 &= mask; > > GUEST_ASSERT_2(!supported_xcr0 || supported_xcr0 == mask, > supported_xcr0, mask); > } > > static void check_xfeature_dependencies(uint64_t supported_xcr0, uint64_t mask, > uint64_t dependencies) > { > supported_xcr0 &= (mask | dependencies); > > GUEST_ASSERT_3(!(supported_xcr0 & mask) || > supported_xcr0 == (mask | dependencies), > supported_xcr0, mask, dependencies); > } > > would yield > > check_xfeature_dependencies(supported_xcr0, XFEATURE_MASK_YMM, > XFEATURE_MASK_SSE); > > and then for AVX512: > > check_xfeature_dependencies(supported_xcr0, XFEATURE_MASK_AVX512, > XFEATURE_MASK_SSE | XFEATURE_MASK_YMM); > check_all_or_none_xfeature(supported_xcr0, XFEATURE_MASK_AVX512); > > That would more or less eliminate the need for comments, and IMO makes it more > obvious what is being checked. The reason I chose not to use helpers here was it hides the line number the assert occured on. With helpers you have multiple places the problem came from and one place it's asserting. The way I wrote it the line number in the assert tells you exactly where the problem occured. I get you can deduce the line number with the values passed back in the assert, but the assert information printed out on the host has to be purposefully vague because you get one fmt for the entire test. If I was able to do a printf style asserts from the guest, that would allow me to provide more, clear context to tie it back. Having the line number where the assert fired I felt was useful to keep. I guess one way to get the best of both worlds would be to have the helpers return a bool rather than assert in the helper. I could also include the additional info you suggested in the asserts. That said, if we do end up going with helpers I don't think we have to call them both like in the AVX512 example. They both check the same thing, except check_xfeature_dependencies() additionally allows for dependencies to be used. E.g. if you call: check_xfeature_dependencies(supported_xcr0, XFEATURE_MASK_AVX512, 0) it's the same as calling: check_all_or_none_xfeature(supported_xcr0, XFEATURE_MASK_AVX512) Maybe we should call them: check_xfeature() and check_xfeature_dependencies(), or with_dependencies... something to show they are related to each other. > > > + xsetbv(0, supported_xcr0); > > + > > + GUEST_DONE(); > > +} > > + > > +static void guest_gp_handler(struct ex_regs *regs) > > +{ > > + GUEST_ASSERT(!"Failed to set the supported xfeature bits in XCR0."); > > I'd rather add an xsetbv_safe() variant than install a #GP handler. That would > also make it super easy to add negative testing. E.g. (completely untested) > > static inline uint8_t xsetbv_safe(uint32_t index, uint64_t value) > { > u32 eax = value; > u32 edx = value >> 32; > > return kvm_asm_safe("xsetbv", "a" (eax), "d" (edx), "c" (index)); > } > > and > vector = xsetbv_safe(0, supported_xcr0); > GUEST_ASSERT_2(!vector, supported_xcr0, vector); > > and rudimentary negative testing > > for (i = 0; i < 64; i++) { > if (supported_xcr0 & BIT_ULL(i)) > continue; > > vector = xsetbv_safe(0, supported_xcr0 | BIT_ULL(i)); > GUEST_ASSERT_2(vector == GP_VECTOR, supported_xcr0, vector); > } I like the idea of this additional test. I'll add it.
On Thu, Jan 05, 2023, Aaron Lewis wrote: > On Wed, Jan 4, 2023 at 5:13 PM Sean Christopherson <seanjc@google.com> wrote: > > Hmm or maybe add helpers? Printing the info on failure would also make it easier > > to debug. E.g. > > > > static void check_all_or_none_xfeature(uint64_t supported_xcr0, uint64_t mask) > > { > > supported_xcr0 &= mask; > > > > GUEST_ASSERT_2(!supported_xcr0 || supported_xcr0 == mask, > > supported_xcr0, mask); > > } > > > > static void check_xfeature_dependencies(uint64_t supported_xcr0, uint64_t mask, > > uint64_t dependencies) > > { > > supported_xcr0 &= (mask | dependencies); > > > > GUEST_ASSERT_3(!(supported_xcr0 & mask) || > > supported_xcr0 == (mask | dependencies), > > supported_xcr0, mask, dependencies); > > } > > > > would yield > > > > check_xfeature_dependencies(supported_xcr0, XFEATURE_MASK_YMM, > > XFEATURE_MASK_SSE); > > > > and then for AVX512: > > > > check_xfeature_dependencies(supported_xcr0, XFEATURE_MASK_AVX512, > > XFEATURE_MASK_SSE | XFEATURE_MASK_YMM); > > check_all_or_none_xfeature(supported_xcr0, XFEATURE_MASK_AVX512); > > > > That would more or less eliminate the need for comments, and IMO makes it more > > obvious what is being checked. > > The reason I chose not to use helpers here was it hides the line > number the assert occured on. With helpers you have multiple places > the problem came from and one place it's asserting. The way I wrote > it the line number in the assert tells you exactly where the problem > occured. > > I get you can deduce the line number with the values passed back in > the assert, But the line number ultimately doesn't matter, no? In the original code, the line number lets you know what feature bits failed, but in the proposed helpers above, that's explicitly provided. > but the assert information printed out on the host has to > be purposefully vague because you get one fmt for the entire test Right, but the line number of the helper disambiguates the data. E.g. knowing that the assert in check_xfeature_dependencies() fired lets the reader know what the args mean. > If I was able to do a printf style asserts from the guest, that would allow > me to provide more, clear context to tie it back. Yeah, we need to figure out a solution for that sooner than later. > Having the line number where the assert fired I felt was useful to keep. > > I guess one way to get the best of both worlds would be to have the > helpers return a bool rather than assert in the helper. I could also > include the additional info you suggested in the asserts. That seems like it will end up with hard to read code, and a lot of copy+paste. E.g. GUEST_ASSERT_3(is_valid_xfeature(supported_xcr0, XFEATURE_MASK_AVX512, XFEATURE_MASK_SSE | XFEATURE_MASK_YMM), supported_xcr0, XFEATURE_MASK_AVX512, XFEATURE_MASK_SSE | XFEATURE_MASK_YMM); isn't the friendliest. What about using macros? It's a little gory, but I don't think it'll be a maintenance issue, and the code is quite small. And on the plus side, it's more obvious that the "caller" is making an assertion. #define ASSERT_ALL_OR_NONE_XFEATURE(supported_xcr0, mask) \ do { \ uint64_t __supported = supported_xcr0 & (mask); \ \ GUEST_ASSERT_2(!supported || supported == (mask), \ supported, (mask)); \ while (0) > That said, if we do end up going with helpers I don't think we have to > call them both like in the AVX512 example. They both check the same > thing, except check_xfeature_dependencies() additionally allows for > dependencies to be used. My thought was to intentionally separate the checks by their semantics, even though it means more checks. Asserting that the dependencies are in place is backed by architectural rules, whereas asserting the "all or nothing" stuff is KVM's own software-defined rules.
diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile index 1750f91dd9362..e2e56c82b8a90 100644 --- a/tools/testing/selftests/kvm/Makefile +++ b/tools/testing/selftests/kvm/Makefile @@ -104,6 +104,7 @@ TEST_GEN_PROGS_x86_64 += x86_64/vmx_tsc_adjust_test TEST_GEN_PROGS_x86_64 += x86_64/vmx_nested_tsc_scaling_test TEST_GEN_PROGS_x86_64 += x86_64/xapic_ipi_test TEST_GEN_PROGS_x86_64 += x86_64/xapic_state_test +TEST_GEN_PROGS_x86_64 += x86_64/xcr0_cpuid_test TEST_GEN_PROGS_x86_64 += x86_64/xss_msr_test TEST_GEN_PROGS_x86_64 += x86_64/debug_regs TEST_GEN_PROGS_x86_64 += x86_64/tsc_msrs_test diff --git a/tools/testing/selftests/kvm/x86_64/xcr0_cpuid_test.c b/tools/testing/selftests/kvm/x86_64/xcr0_cpuid_test.c new file mode 100644 index 0000000000000..6bef362872154 --- /dev/null +++ b/tools/testing/selftests/kvm/x86_64/xcr0_cpuid_test.c @@ -0,0 +1,121 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * XCR0 cpuid test + * + * Copyright (C) 2022, Google LLC. + */ + +#include <fcntl.h> +#include <stdio.h> +#include <stdlib.h> +#include <string.h> +#include <sys/ioctl.h> + +#include "test_util.h" + +#include "kvm_util.h" +#include "processor.h" + +static uint64_t get_supported_user_xfeatures(void) +{ + uint32_t a, b, c, d; + + cpuid(0xd, &a, &b, &c, &d); + + return a | ((uint64_t)d << 32); +} + +static void guest_code(void) +{ + uint64_t xcr0_rest; + uint64_t supported_xcr0; + uint64_t xfeature_mask; + uint64_t supported_state; + + set_cr4(get_cr4() | X86_CR4_OSXSAVE); + + xcr0_rest = xgetbv(0); + supported_xcr0 = get_supported_user_xfeatures(); + + GUEST_ASSERT(xcr0_rest == 1ul); + + /* Check AVX */ + xfeature_mask = XFEATURE_MASK_SSE | XFEATURE_MASK_YMM; + supported_state = supported_xcr0 & xfeature_mask; + GUEST_ASSERT(supported_state != XFEATURE_MASK_YMM); + + /* Check MPX */ + xfeature_mask = XFEATURE_MASK_BNDREGS | XFEATURE_MASK_BNDCSR; + supported_state = supported_xcr0 & xfeature_mask; + GUEST_ASSERT((supported_state == xfeature_mask) || + (supported_state == 0ul)); + + /* Check AVX-512 */ + xfeature_mask = XFEATURE_MASK_SSE | XFEATURE_MASK_YMM | + XFEATURE_MASK_AVX512; + supported_state = supported_xcr0 & xfeature_mask; + GUEST_ASSERT((supported_state == xfeature_mask) || + ((supported_state & XFEATURE_MASK_AVX512) == 0ul)); + + /* Check AMX */ + xfeature_mask = XFEATURE_MASK_XTILE; + supported_state = supported_xcr0 & xfeature_mask; + GUEST_ASSERT((supported_state == xfeature_mask) || + (supported_state == 0ul)); + + GUEST_SYNC(0); + + xsetbv(0, supported_xcr0); + + GUEST_DONE(); +} + +static void guest_gp_handler(struct ex_regs *regs) +{ + GUEST_ASSERT(!"Failed to set the supported xfeature bits in XCR0."); +} + +int main(int argc, char *argv[]) +{ + struct kvm_vcpu *vcpu; + struct kvm_run *run; + struct kvm_vm *vm; + struct ucall uc; + + TEST_REQUIRE(kvm_cpu_has(X86_FEATURE_XSAVE)); + + /* Tell stdout not to buffer its content */ + setbuf(stdout, NULL); + + vm = vm_create_with_one_vcpu(&vcpu, guest_code); + run = vcpu->run; + + vm_init_descriptor_tables(vm); + vcpu_init_descriptor_tables(vcpu); + + while (1) { + vcpu_run(vcpu); + + TEST_ASSERT(run->exit_reason == KVM_EXIT_IO, + "Unexpected exit reason: %u (%s),\n", + run->exit_reason, + exit_reason_str(run->exit_reason)); + + switch (get_ucall(vcpu, &uc)) { + case UCALL_SYNC: + vm_install_exception_handler(vm, GP_VECTOR, + guest_gp_handler); + break; + case UCALL_ABORT: + REPORT_GUEST_ASSERT(uc); + break; + case UCALL_DONE: + goto done; + default: + TEST_FAIL("Unknown ucall %lu", uc.cmd); + } + } + +done: + kvm_vm_free(vm); + return 0;
Test that the supported xfeatures, i.e. EDX:EAX of CPUID.(EAX=0DH,ECX=0), don't set userspace up for failure. Though it isn't architectural, test that the supported xfeatures aren't set in a half baked state that will cause a #GP if used to execute XSETBV. Check that the rules for XCR0 described in the SDM vol 1, section 13.3 ENABLING THE XSAVE FEATURE SET AND XSAVE-ENABLED FEATURES, are followed for the supported xfeatures too. Signed-off-by: Aaron Lewis <aaronlewis@google.com> --- tools/testing/selftests/kvm/Makefile | 1 + .../selftests/kvm/x86_64/xcr0_cpuid_test.c | 121 ++++++++++++++++++ 2 files changed, 122 insertions(+) create mode 100644 tools/testing/selftests/kvm/x86_64/xcr0_cpuid_test.c +}