Re: [PATCH v3 8/8] KVM: selftests: Add XCR0 Test

[Sean Christopherson <seanjc@google.com>]

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().