[PATCH v2 0/2] Fix check in amx_test

[PATCH v2 0/2] Fix check in amx_test

[Aaron Lewis]

Two fixes for the same check in amx_test.
  1. Add an assert that XSAVE supports XTILE, rather than doing nothing with it.
  2. Assert that XSAVE supports both XTILECFG and XTILEDATA.

Aaron Lewis (2):
  KVM: selftests: Assert that XSAVE supports XTILE in amx_test
  KVM: selftests: Assert that XSAVE supports both XTILE{CFG,DATA}

 tools/testing/selftests/kvm/x86_64/amx_test.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

-- 
2.39.0.314.g84b9a713c41-goog

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

[Aaron Lewis]

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(-)

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)
-- 
2.39.0.314.g84b9a713c41-goog

[PATCH v2 2/2] KVM: selftests: Assert that XSAVE supports both XTILE{CFG,DATA}

[Aaron Lewis]

The check in amx_test that ensures that XSAVE supports XTILE makes sure
at least one of the XTILE bits are set, XTILECFG or XTILEDATA, when it
really should be checking that both are set.

Assert that both XTILECFG and XTILEDATA a set.

Fixes: bf70636d9443 ("selftest: kvm: Add amx selftest")
Signed-off-by: Aaron Lewis <aaronlewis@google.com>
---
 tools/testing/selftests/kvm/x86_64/amx_test.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/kvm/x86_64/amx_test.c b/tools/testing/selftests/kvm/x86_64/amx_test.c
index 2f555f5c93e99..db1b38ca7c840 100644
--- a/tools/testing/selftests/kvm/x86_64/amx_test.c
+++ b/tools/testing/selftests/kvm/x86_64/amx_test.c
@@ -121,7 +121,7 @@ static inline void check_cpuid_xsave(void)
 
 static inline void check_xsave_supports_xtile(void)
 {
-	GUEST_ASSERT(__xgetbv(0) & XFEATURE_MASK_XTILE);
+	GUEST_ASSERT((__xgetbv(0) & XFEATURE_MASK_XTILE) == XFEATURE_MASK_XTILE);
 }
 
 static void check_xtile_info(void)
-- 
2.39.0.314.g84b9a713c41-goog

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

[Sean Christopherson]

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
> 

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

[Sean Christopherson]

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