Re: [PATCH v2 6/6] KVM: selftests: Add XCR0 Test

[Sean Christopherson <seanjc@xxxxxxxxxx>]

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);
	}