| Message ID | 20221227183713.29140-4-aaronlewis@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Clean up AMX cpuid bits XTILE_CFG and XTILE_DATA | expand |
On Tue, Dec 27, 2022 at 10:38 AM Aaron Lewis <aaronlewis@google.com> wrote: > > Test that the user xfeature bits, EDX:EAX of CPUID.(EAX=0DH,ECX=0), > don't set up userspace for failure. > > Though it isn't architectural, test that the user xfeature bits aren't > set in a half baked state that will cause a #GP if used when setting > XCR0. > > 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 xfeature bits too. > > Signed-off-by: Aaron Lewis <aaronlewis@google.com> > --- > tools/testing/selftests/kvm/Makefile | 1 + > .../selftests/kvm/x86_64/xcr0_cpuid_test.c | 134 ++++++++++++++++++ > 2 files changed, 135 insertions(+) > create mode 100644 tools/testing/selftests/kvm/x86_64/xcr0_cpuid_test.c > > 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..644791ff5c48b > --- /dev/null > +++ b/tools/testing/selftests/kvm/x86_64/xcr0_cpuid_test.c > @@ -0,0 +1,134 @@ > +// 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" > + > +#define XFEATURE_MASK_SSE (1ul << 1) > +#define XFEATURE_MASK_YMM (1ul << 2) > +#define XFEATURE_MASK_BNDREGS (1ul << 3) > +#define XFEATURE_MASK_BNDCSR (1ul << 4) > +#define XFEATURE_MASK_OPMASK (1ul << 5) > +#define XFEATURE_MASK_ZMM_Hi256 (1ul << 6) > +#define XFEATURE_MASK_Hi16_ZMM (1ul << 7) > +#define XFEATURE_MASK_XTILECFG (1ul << 17) > +#define XFEATURE_MASK_XTILEDATA (1ul << 18) > +#define XFEATURE_MASK_XTILE (XFEATURE_MASK_XTILECFG | \ > + XFEATURE_MASK_XTILEDATA) With XSETBV hoisted into processor.h, shouldn't these macros be more widely available as well? > +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_OPMASK | > + XFEATURE_MASK_ZMM_Hi256 | > + XFEATURE_MASK_Hi16_ZMM; > + supported_state = supported_xcr0 & xfeature_mask; > + GUEST_ASSERT((supported_state == xfeature_mask) || > + (supported_state == 0ul)); > + > + /* Check AMX */ > + xfeature_mask = XFEATURE_MASK_XTILE; > + supported_state = supported_xcr0 & xfeature_mask; > + GUEST_ASSERT((supported_state == xfeature_mask) || > + (supported_state == 0ul)); In this series, you've added code to __do_cpuid_func() to ensure that XFEATURE_MASK_XTILECFG and XFEATURE_MASK_XTILEDATA can't be set unless the other is set. Do we need to do something similar for AVX-512 and MPX? > + 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; > +} > -- > 2.39.0.314.g84b9a713c41-goog >
> > +#include "kvm_util.h" > > +#include "processor.h" > > + > > +#define XFEATURE_MASK_SSE (1ul << 1) > > +#define XFEATURE_MASK_YMM (1ul << 2) > > +#define XFEATURE_MASK_BNDREGS (1ul << 3) > > +#define XFEATURE_MASK_BNDCSR (1ul << 4) > > +#define XFEATURE_MASK_OPMASK (1ul << 5) > > +#define XFEATURE_MASK_ZMM_Hi256 (1ul << 6) > > +#define XFEATURE_MASK_Hi16_ZMM (1ul << 7) > > +#define XFEATURE_MASK_XTILECFG (1ul << 17) > > +#define XFEATURE_MASK_XTILEDATA (1ul << 18) > > +#define XFEATURE_MASK_XTILE (XFEATURE_MASK_XTILECFG | \ > > + XFEATURE_MASK_XTILEDATA) > > With XSETBV hoisted into processor.h, shouldn't these macros be more > widely available as well? Sure, I'll hoist them up there too. > > > +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); > > +} > > + > > + 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_OPMASK | > > + XFEATURE_MASK_ZMM_Hi256 | > > + XFEATURE_MASK_Hi16_ZMM; > > + supported_state = supported_xcr0 & xfeature_mask; > > + GUEST_ASSERT((supported_state == xfeature_mask) || > > + (supported_state == 0ul)); > > + > > + /* Check AMX */ > > + xfeature_mask = XFEATURE_MASK_XTILE; > > + supported_state = supported_xcr0 & xfeature_mask; > > + GUEST_ASSERT((supported_state == xfeature_mask) || > > + (supported_state == 0ul)); > > In this series, you've added code to __do_cpuid_func() to ensure that > XFEATURE_MASK_XTILECFG and XFEATURE_MASK_XTILEDATA can't be set unless > the other is set. Do we need to do something similar for AVX-512 and > MPX? That does sound like a natural extension of this. I can add support for that in v2. > > > + 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."); > > +} > > +
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..644791ff5c48b --- /dev/null +++ b/tools/testing/selftests/kvm/x86_64/xcr0_cpuid_test.c @@ -0,0 +1,134 @@ +// 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" + +#define XFEATURE_MASK_SSE (1ul << 1) +#define XFEATURE_MASK_YMM (1ul << 2) +#define XFEATURE_MASK_BNDREGS (1ul << 3) +#define XFEATURE_MASK_BNDCSR (1ul << 4) +#define XFEATURE_MASK_OPMASK (1ul << 5) +#define XFEATURE_MASK_ZMM_Hi256 (1ul << 6) +#define XFEATURE_MASK_Hi16_ZMM (1ul << 7) +#define XFEATURE_MASK_XTILECFG (1ul << 17) +#define XFEATURE_MASK_XTILEDATA (1ul << 18) +#define XFEATURE_MASK_XTILE (XFEATURE_MASK_XTILECFG | \ + XFEATURE_MASK_XTILEDATA) + +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_OPMASK | + XFEATURE_MASK_ZMM_Hi256 | + XFEATURE_MASK_Hi16_ZMM; + supported_state = supported_xcr0 & xfeature_mask; + GUEST_ASSERT((supported_state == xfeature_mask) || + (supported_state == 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 user xfeature bits, EDX:EAX of CPUID.(EAX=0DH,ECX=0), don't set up userspace for failure. Though it isn't architectural, test that the user xfeature bits aren't set in a half baked state that will cause a #GP if used when setting XCR0. 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 xfeature bits too. Signed-off-by: Aaron Lewis <aaronlewis@google.com> --- tools/testing/selftests/kvm/Makefile | 1 + .../selftests/kvm/x86_64/xcr0_cpuid_test.c | 134 ++++++++++++++++++ 2 files changed, 135 insertions(+) create mode 100644 tools/testing/selftests/kvm/x86_64/xcr0_cpuid_test.c +}