Re: [PATCH v2 1/2] KVM: selftests: Assert that XSAVE supports XTILE in amx_test

Sean Christopherson
2023-01-03T18:24:41Z

On Tue, Jan 03, 2023, Sean Christopherson wrote:
> On Fri, Dec 30, 2022, Aaron Lewis wrote:
> > The check in amx_test that ensures that XSAVE supports XTILE, doesn't
> > actually check anything. It simply returns a bool which the test does
> > nothing with.
> >
> > Assert that XSAVE supports XTILE.
> >
> > Fixes: 5dc19f1c7dd3 ("KVM: selftests: Convert AMX test to use X86_PROPRETY_XXX")
>
> Doh.
>
> > Signed-off-by: Aaron Lewis <aaronlewis@google.com>
> > ---
> > tools/testing/selftests/kvm/x86_64/amx_test.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/testing/selftests/kvm/x86_64/amx_test.c b/tools/testing/selftests/kvm/x86_64/amx_test.c
> > index bd72c6eb3b670..2f555f5c93e99 100644
> > --- a/tools/testing/selftests/kvm/x86_64/amx_test.c
> > +++ b/tools/testing/selftests/kvm/x86_64/amx_test.c
> > @@ -119,9 +119,9 @@ static inline void check_cpuid_xsave(void)
> > GUEST_ASSERT(this_cpu_has(X86_FEATURE_OSXSAVE));
> > }
> >
> > -static bool check_xsave_supports_xtile(void)
> > +static inline void check_xsave_supports_xtile(void)
>
> Don't explicitly tag local static functions as inline (ignore the existing code
> that sets a bad precedent), modern compilers don't need the hint to generate
> optimal code,
>
> > {
> > - return __xgetbv(0) & XFEATURE_MASK_XTILE;
> > + GUEST_ASSERT(__xgetbv(0) & XFEATURE_MASK_XTILE);
>
> Any objection to moving the assertion into check_xtile_info() and dropping this
> one-line helper?

Actually, this code is silly, and arguably unnecessary. init_regs() explicitly
sets XCR0 to XFEATURE_MASK_XTILE. If something goes awry, XSETBV should #GP.
If we want to be really paranoid and assert that KVM didn't silently fail XSETBV,
then the more logical place for the assertion is immediately after the XSETBV.

i.e.

static void init_regs(void)
{
uint64_t cr4, xcr0;

/* turn on CR4.OSXSAVE */
cr4 = get_cr4();
cr4 |= X86_CR4_OSXSAVE;
set_cr4(cr4);

xcr0 = __xgetbv(0);
xcr0 |= XFEATURE_MASK_XTILE;
__xsetbv(0x0, xcr0);

GUEST_ASSERT((__xgetbv(0) & XFEATURE_MASK_XTILE) == XFEATURE_MASK_XTILE);
}