On Wed, Jan 4, 2023 at 5:13 PM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
>
> 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()?
this_cpu_supported_xcr0 works for me.
>
> > +{
> > + 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.
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 assert information printed out on the host has to
be purposefully vague because you get one fmt for the entire test. 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. 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 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. E.g.
if you call:
check_xfeature_dependencies(supported_xcr0, XFEATURE_MASK_AVX512, 0)
it's the same as calling:
check_all_or_none_xfeature(supported_xcr0, XFEATURE_MASK_AVX512)
Maybe we should call them: check_xfeature() and
check_xfeature_dependencies(), or with_dependencies... something to
show they are related to each other.
>
> > + 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);
> }
I like the idea of this additional test. I'll add it.
