| Message ID | 20221227220518.965948-1-aaronlewis@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: selftests: Assert that XSAVE supports XTILE in amx_test | expand |
On Tue, Dec 27, 2022 at 2:05 PM Aaron Lewis <aaronlewis@google.com> 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. > Additionally, the check ensures that at least one of the XTILE bits are > set, XTILECFG or XTILEDATA, when it really should be checking that both > are set. > > Change both behaviors to: > 1) Asserting if the check fails. > 2) Fail if both XTILECFG and XTILEDATA aren't set. For (1), why not simply undo the damage caused by commit 5dc19f1c7dd3 ("KVM: selftests: Convert AMX test to use X86_PROPRETY_XXX"), and restore the GUEST_ASSERT() at the call site? Should this be two separate changes, since there are two separate bug fixes?
On Wed, Dec 28, 2022 at 6:36 PM Jim Mattson <jmattson@google.com> wrote: > > On Tue, Dec 27, 2022 at 2:05 PM Aaron Lewis <aaronlewis@google.com> 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. > > Additionally, the check ensures that at least one of the XTILE bits are > > set, XTILECFG or XTILEDATA, when it really should be checking that both > > are set. > > > > Change both behaviors to: > > 1) Asserting if the check fails. > > 2) Fail if both XTILECFG and XTILEDATA aren't set. > > For (1), why not simply undo the damage caused by commit 5dc19f1c7dd3 > ("KVM: selftests: Convert AMX test to use X86_PROPRETY_XXX"), and > restore the GUEST_ASSERT() at the call site? I opted to add the assert in the check call to be consistent with the others. I thought it would look odd to have 3 check calls being called one after the other, where 2 of them made the call and did nothing with the result and 1 was wrapped in an assert. > Should this be two separate changes, since there are two separate bug fixes? Sure, splitting this into two commits SGTM.
diff --git a/tools/testing/selftests/kvm/x86_64/amx_test.c b/tools/testing/selftests/kvm/x86_64/amx_test.c index bd72c6eb3b670..db1b38ca7c840 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) == 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. Additionally, the check ensures that at least one of the XTILE bits are set, XTILECFG or XTILEDATA, when it really should be checking that both are set. Change both behaviors to: 1) Asserting if the check fails. 2) Fail if both XTILECFG and XTILEDATA aren't set. Fixes: 5dc19f1c7dd3 ("KVM: selftests: Convert AMX test to use X86_PROPRETY_XXX") Fixes: bf70636d9443 ("selftest: kvm: Add amx selftest") 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(-)