diff mbox series

[v3,8/8] KVM: selftests: Add XCR0 Test

Message ID 20230224223607.1580880-9-aaronlewis@google.com (mailing list archive)
State New, archived
Headers show
Series Clean up the supported xfeatures | expand

Commit Message

Aaron Lewis Feb. 24, 2023, 10:36 p.m. UTC 
Check both architectural rules and KVM's own software-defined rules to
ensure the supported xfeatures[1] don't violate any of them.

The architectural rules[2] and KVM's rules ensure for a given
feature, e.g. sse, avx, amx, etc... their associated xfeatures are
either all sets or none of them are set, and any dependencies
are enabled if needed.

[1] EDX:EAX of CPUID.(EAX=0DH,ECX=0)
[2] SDM vol 1, 13.3 ENABLING THE XSAVE FEATURE SET AND XSAVE-ENABLED
    FEATURES

Signed-off-by: Aaron Lewis <aaronlewis@google.com>
---
 tools/testing/selftests/kvm/Makefile          |   1 +
 .../selftests/kvm/include/x86_64/processor.h  |  17 +++
 .../selftests/kvm/x86_64/xcr0_cpuid_test.c    | 128 ++++++++++++++++++
 3 files changed, 146 insertions(+)
 create mode 100644 tools/testing/selftests/kvm/x86_64/xcr0_cpuid_test.c

Comments

Mingwei Zhang March 2, 2023, 8:34 p.m. UTC | #1 
On Fri, Feb 24, 2023, Aaron Lewis wrote:
> Check both architectural rules and KVM's own software-defined rules to
> ensure the supported xfeatures[1] don't violate any of them.
> 
> The architectural rules[2] and KVM's rules ensure for a given
> feature, e.g. sse, avx, amx, etc... their associated xfeatures are
> either all sets or none of them are set, and any dependencies
> are enabled if needed.
> 
> [1] EDX:EAX of CPUID.(EAX=0DH,ECX=0)
> [2] SDM vol 1, 13.3 ENABLING THE XSAVE FEATURE SET AND XSAVE-ENABLED
>     FEATURES
> 
> Signed-off-by: Aaron Lewis <aaronlewis@google.com>

Sorry, I did not get the point of this test? I run your test in an old
(unpatched) kernel on two machines: 1) one with AMX and 2) one without
it. (SPR and Skylake). Neither of them fails. Do you want to clarify a
little bit?


Thanks.
-Mingwei
Aaron Lewis March 2, 2023, 10:50 p.m. UTC | #2 
On Thu, Mar 2, 2023 at 8:34 PM Mingwei Zhang <mizhang@google.com> wrote:
>
> On Fri, Feb 24, 2023, Aaron Lewis wrote:
> > Check both architectural rules and KVM's own software-defined rules to
> > ensure the supported xfeatures[1] don't violate any of them.
> >
> > The architectural rules[2] and KVM's rules ensure for a given
> > feature, e.g. sse, avx, amx, etc... their associated xfeatures are
> > either all sets or none of them are set, and any dependencies
> > are enabled if needed.
> >
> > [1] EDX:EAX of CPUID.(EAX=0DH,ECX=0)
> > [2] SDM vol 1, 13.3 ENABLING THE XSAVE FEATURE SET AND XSAVE-ENABLED
> >     FEATURES
> >
> > Signed-off-by: Aaron Lewis <aaronlewis@google.com>
>
> Sorry, I did not get the point of this test? I run your test in an old
> (unpatched) kernel on two machines: 1) one with AMX and 2) one without
> it. (SPR and Skylake). Neither of them fails. Do you want to clarify a
> little bit?

The only known issue exists on newer versions of the kernel when run
on SPR.  It occurs after the syscall, prctl (to enable XTILEDATA), was
introduced.  If you run this test without the fix[1] you will get the
assert below, indicating the XTILECFG is supported by the guest, but
XTILEDATA isn't.

==== Test Assertion Failure ====
  x86_64/xcr0_cpuid_test.c:116: false
  pid=18124 tid=18124 errno=4 - Interrupted system call
     1 0x0000000000401894: main at xcr0_cpuid_test.c:116
     2 0x0000000000414263: __libc_start_call_main at libc-start.o:?
     3 0x00000000004158af: __libc_start_main_impl at ??:?
     4 0x0000000000401660: _start at ??:?
  Failed guest assert: !__supported || __supported == ((((((1ULL))) <<
(18)) | ((((1ULL))) << (17)))) at x86_64/xcr0_cpuid_test.c:72
0x20000 0x60000 0x0

[1] KVM: x86: Clear all supported AMX xfeatures if they are not all set
https://lore.kernel.org/kvm/20230224223607.1580880-6-aaronlewis@google.com/

>
>
> Thanks.
> -Mingwei
Mingwei Zhang March 3, 2023, 9:11 p.m. UTC | #3 
On Thu, Mar 02, 2023, Aaron Lewis wrote:
> On Thu, Mar 2, 2023 at 8:34 PM Mingwei Zhang <mizhang@google.com> wrote:
> > > > On Fri, Feb 24, 2023, Aaron Lewis wrote:
> > > Check both architectural rules and KVM's own software-defined rules to
> > > ensure the supported xfeatures[1] don't violate any of them.
> > >
> > > The architectural rules[2] and KVM's rules ensure for a given
> > > feature, e.g. sse, avx, amx, etc... their associated xfeatures are
> > > either all sets or none of them are set, and any dependencies
> > > are enabled if needed.
> > >
> > > [1] EDX:EAX of CPUID.(EAX=0DH,ECX=0)
> > > [2] SDM vol 1, 13.3 ENABLING THE XSAVE FEATURE SET AND XSAVE-ENABLED
> > >     FEATURES
> > >
> > > Signed-off-by: Aaron Lewis <aaronlewis@google.com>
> >
> > Sorry, I did not get the point of this test? I run your test in an old
> > (unpatched) kernel on two machines: 1) one with AMX and 2) one without
> > it. (SPR and Skylake). Neither of them fails. Do you want to clarify a
> > little bit?
> 
> The only known issue exists on newer versions of the kernel when run
> on SPR.  It occurs after the syscall, prctl (to enable XTILEDATA), was
> introduced.  If you run this test without the fix[1] you will get the
> assert below, indicating the XTILECFG is supported by the guest, but
> XTILEDATA isn't.
> 
> ==== Test Assertion Failure ====
>   x86_64/xcr0_cpuid_test.c:116: false
>   pid=18124 tid=18124 errno=4 - Interrupted system call
>      1 0x0000000000401894: main at xcr0_cpuid_test.c:116
>      2 0x0000000000414263: __libc_start_call_main at libc-start.o:?
>      3 0x00000000004158af: __libc_start_main_impl at ??:?
>      4 0x0000000000401660: _start at ??:?
>   Failed guest assert: !__supported || __supported == ((((((1ULL))) <<
> (18)) | ((((1ULL))) << (17)))) at x86_64/xcr0_cpuid_test.c:72
> 0x20000 0x60000 0x0
> 
> [1] KVM: x86: Clear all supported AMX xfeatures if they are not all set
> https://lore.kernel.org/kvm/20230224223607.1580880-6-aaronlewis@google.com/
> 

Understood. Thanks for reminding me. Yes, without the invocation of
vm_xsave_require_permission(XSTATE_XTILE_DATA_BIT); VM guest will see a
partially enabled feature (XTILECFG without XTILEDATA).

Reviewed-by: Mingwei Zhang <mizhang@google.com>
> >
> >
> > Thanks.
> > -Mingwei
Sean Christopherson March 23, 2023, 9:46 p.m. UTC | #4 
On Fri, Feb 24, 2023, Aaron Lewis wrote:
> diff --git a/tools/testing/selftests/kvm/include/x86_64/processor.h b/tools/testing/selftests/kvm/include/x86_64/processor.h
> index ebe83cfe521c..380daa82b023 100644
> --- a/tools/testing/selftests/kvm/include/x86_64/processor.h
> +++ b/tools/testing/selftests/kvm/include/x86_64/processor.h
> @@ -667,6 +667,15 @@ static inline bool this_pmu_has(struct kvm_x86_pmu_feature feature)
>  	       !this_cpu_has(feature.anti_feature);
>  }
>  
> +static __always_inline uint64_t this_cpu_supported_xcr0(void)
> +{
> +	uint32_t a, b, c, d;
> +
> +	cpuid(0xd, &a, &b, &c, &d);
> +
> +	return a | ((uint64_t)d << 32);

This can be done via X86_PROPERTY.  It's not that much prettier, but it at least
avoids open coding things.

diff --git a/tools/testing/selftests/kvm/include/x86_64/processor.h b/tools/testing/selftests/kvm/include/x86_64/processor.h
index fa49d51753e5..2bb0ec8dddf3 100644
--- a/tools/testing/selftests/kvm/include/x86_64/processor.h
+++ b/tools/testing/selftests/kvm/include/x86_64/processor.h
@@ -228,8 +228,11 @@ struct kvm_x86_cpu_property {
 #define X86_PROPERTY_PMU_NR_GP_COUNTERS                KVM_X86_CPU_PROPERTY(0xa, 0, EAX, 8, 15)
 #define X86_PROPERTY_PMU_EBX_BIT_VECTOR_LENGTH KVM_X86_CPU_PROPERTY(0xa, 0, EAX, 24, 31)
 
+#define X86_PROPERTY_SUPPORTED_XCR0_LO         KVM_X86_CPU_PROPERTY(0xd,  0, EAX,  0, 31)
 #define X86_PROPERTY_XSTATE_MAX_SIZE_XCR0      KVM_X86_CPU_PROPERTY(0xd,  0, EBX,  0, 31)
 #define X86_PROPERTY_XSTATE_MAX_SIZE           KVM_X86_CPU_PROPERTY(0xd,  0, ECX,  0, 31)
+#define X86_PROPERTY_SUPPORTED_XCR0_HI         KVM_X86_CPU_PROPERTY(0xd,  0, EDX,  0, 31)
+
 #define X86_PROPERTY_XSTATE_TILE_SIZE          KVM_X86_CPU_PROPERTY(0xd, 18, EAX,  0, 31)
 #define X86_PROPERTY_XSTATE_TILE_OFFSET                KVM_X86_CPU_PROPERTY(0xd, 18, EBX,  0, 31)
 #define X86_PROPERTY_AMX_TOTAL_TILE_BYTES      KVM_X86_CPU_PROPERTY(0x1d, 1, EAX,  0, 15)
@@ -669,11 +672,11 @@ static inline bool this_pmu_has(struct kvm_x86_pmu_feature feature)
 
 static __always_inline uint64_t this_cpu_supported_xcr0(void)
 {
-       uint32_t a, b, c, d;
+       if (!this_cpu_has_p(X86_PROPERTY_SUPPORTED_XCR0_LO))
+               return 0;
 
-       cpuid(0xd, &a, &b, &c, &d);
-
-       return a | ((uint64_t)d << 32);
+       return this_cpu_property(X86_PROPERTY_SUPPORTED_XCR0_LO) |
+              ((uint64_t)this_cpu_property(X86_PROPERTY_SUPPORTED_XCR0_HI) << 32);
 }
 
 typedef u32            __attribute__((vector_size(16))) sse128_t;

> +/* Architectural check. */

If you're going to bother with a comment, might as well actually explain the
architectural rule.

/*
 * Assert that architectural dependency rules are satisfied, e.g. that AVX is
 * supported if and only if SSE is supported.
 */

> +#define ASSERT_XFEATURE_DEPENDENCIES(supported_xcr0, xfeatures, dependencies)	  \
> +do {										  \
> +	uint64_t __supported = (supported_xcr0) & ((xfeatures) | (dependencies)); \
> +										  \
> +	GUEST_ASSERT_3((__supported & (xfeatures)) != (xfeatures) ||		  \
> +		       __supported == ((xfeatures) | (dependencies)),		  \
> +		       __supported, (xfeatures), (dependencies));		  \
> +} while (0)
> +

> +/*
> + * KVM's own software-defined rules.  While not architectural it really
> + * ought to be true.

This should call out what KVM's "rule" is.  But it's not really a rule, it's more
of a contract with userspace.

/*
 * Assert that KVM reports a sane, usable as-is XCR0.  Architecturally, a CPU
 * isn't strictly required to _support_ all XFeatures related to a feature, but
 * at the same time XSETBV will #GP if bundled XFeatures aren't enabled and
 * disabled coherently.  E.g. a CPU can technically enumerate supported for
 * XTILE_CFG but not XTILE_DATA, but attempt to enable XTILE_CFG without also
 * enabling XTILE_DATA will #GP.
 */

> +	/* Tell stdout not to buffer its content */
> +	setbuf(stdout, NULL);

This copy pasta is no longer necessary, see kvm_selftest_init().
diff mbox series

Patch

diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
index 84a627c43795..18cadc669798 100644
--- a/tools/testing/selftests/kvm/Makefile
+++ b/tools/testing/selftests/kvm/Makefile
@@ -105,6 +105,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/include/x86_64/processor.h b/tools/testing/selftests/kvm/include/x86_64/processor.h
index ebe83cfe521c..380daa82b023 100644
--- a/tools/testing/selftests/kvm/include/x86_64/processor.h
+++ b/tools/testing/selftests/kvm/include/x86_64/processor.h
@@ -667,6 +667,15 @@  static inline bool this_pmu_has(struct kvm_x86_pmu_feature feature)
 	       !this_cpu_has(feature.anti_feature);
 }
 
+static __always_inline uint64_t this_cpu_supported_xcr0(void)
+{
+	uint32_t a, b, c, d;
+
+	cpuid(0xd, &a, &b, &c, &d);
+
+	return a | ((uint64_t)d << 32);
+}
+
 typedef u32		__attribute__((vector_size(16))) sse128_t;
 #define __sse128_u	union { sse128_t vec; u64 as_u64[2]; u32 as_u32[4]; }
 #define sse128_lo(x)	({ __sse128_u t; t.vec = x; t.as_u64[0]; })
@@ -1090,6 +1099,14 @@  static inline uint8_t wrmsr_safe(uint32_t msr, uint64_t val)
 	return kvm_asm_safe("wrmsr", "a"(val & -1u), "d"(val >> 32), "c"(msr));
 }
 
+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));
+}
+
 bool kvm_is_tdp_enabled(void);
 
 uint64_t *__vm_get_page_table_entry(struct kvm_vm *vm, uint64_t vaddr,
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 000000000000..7ca0dea3d144
--- /dev/null
+++ b/tools/testing/selftests/kvm/x86_64/xcr0_cpuid_test.c
@@ -0,0 +1,128 @@ 
+// 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"
+
+/* Architectural check. */
+#define ASSERT_XFEATURE_DEPENDENCIES(supported_xcr0, xfeatures, dependencies)	  \
+do {										  \
+	uint64_t __supported = (supported_xcr0) & ((xfeatures) | (dependencies)); \
+										  \
+	GUEST_ASSERT_3((__supported & (xfeatures)) != (xfeatures) ||		  \
+		       __supported == ((xfeatures) | (dependencies)),		  \
+		       __supported, (xfeatures), (dependencies));		  \
+} while (0)
+
+/*
+ * KVM's own software-defined rules.  While not architectural it really
+ * ought to be true.
+ */
+#define ASSERT_ALL_OR_NONE_XFEATURE(supported_xcr0, xfeatures)		\
+do {									\
+	uint64_t __supported = (supported_xcr0) & (xfeatures);		\
+									\
+	GUEST_ASSERT_2(!__supported || __supported == (xfeatures),	\
+		       __supported, (xfeatures));			\
+} while (0)
+
+static void guest_code(void)
+{
+	uint64_t xcr0_reset;
+	uint64_t supported_xcr0;
+	int i, vector;
+
+	set_cr4(get_cr4() | X86_CR4_OSXSAVE);
+
+	xcr0_reset = xgetbv(0);
+	supported_xcr0 = this_cpu_supported_xcr0();
+
+	GUEST_ASSERT(xcr0_reset == XFEATURE_MASK_FP);
+
+	/* Check AVX */
+	ASSERT_XFEATURE_DEPENDENCIES(supported_xcr0,
+				     XFEATURE_MASK_YMM,
+				     XFEATURE_MASK_SSE);
+
+	/* Check MPX */
+	ASSERT_ALL_OR_NONE_XFEATURE(supported_xcr0,
+				    XFEATURE_MASK_BNDREGS | XFEATURE_MASK_BNDCSR);
+
+	/* Check AVX-512 */
+	ASSERT_XFEATURE_DEPENDENCIES(supported_xcr0,
+				     XFEATURE_MASK_AVX512,
+				     XFEATURE_MASK_SSE | XFEATURE_MASK_YMM);
+	ASSERT_ALL_OR_NONE_XFEATURE(supported_xcr0,
+				    XFEATURE_MASK_AVX512);
+
+	/* Check AMX */
+	ASSERT_ALL_OR_NONE_XFEATURE(supported_xcr0,
+				    XFEATURE_MASK_XTILE);
+
+	vector = xsetbv_safe(0, supported_xcr0);
+	GUEST_ASSERT_2(!vector, supported_xcr0, vector);
+
+	for (i = 0; i < 64; i++) {
+		if (supported_xcr0 & BIT_ULL(i))
+			continue;
+
+		vector = xsetbv_safe(0, supported_xcr0 | BIT_ULL(i));
+		GUEST_ASSERT_3(vector == GP_VECTOR, supported_xcr0, vector, BIT_ULL(i));
+	}
+
+	GUEST_DONE();
+}
+
+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_ABORT:
+			REPORT_GUEST_ASSERT_3(uc, "0x%lx 0x%lx 0x%lx");
+			break;
+		case UCALL_DONE:
+			goto done;
+		default:
+			TEST_FAIL("Unknown ucall %lu", uc.cmd);
+		}
+	}
+
+done:
+	kvm_vm_free(vm);
+	return 0;
+}