| Message ID | 20221230013648.2850519-2-aaronlewis@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix check in amx_test | expand |
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? > } > > static void check_xtile_info(void) > -- > 2.39.0.314.g84b9a713c41-goog >
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); }
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) { - return __xgetbv(0) & XFEATURE_MASK_XTILE; + GUEST_ASSERT(__xgetbv(0) & XFEATURE_MASK_XTILE); } static void check_xtile_info(void)
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") 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(-)