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

[Sean Christopherson <seanjc@google.com>]

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.