[PATCH 1/2] KVM: selftests: Avoid infinite loop in hyperv_features when invtsc is missing

[PATCH 1/2] KVM: selftests: Avoid infinite loop in hyperv_features when invtsc is missing

[Vitaly Kuznetsov]

When X86_FEATURE_INVTSC is missing, guest_test_msrs_access() was supposed
to skip testing dependent Hyper-V invariant TSC feature. Unfortunately,
'continue' does not lead to that as stage is not incremented. Moreover,
'vm' allocated with vm_create_with_one_vcpu() is not freed and the test
runs out of available file descriptors very quickly.

Fixes: bd827bd77537 ("KVM: selftests: Test Hyper-V invariant TSC control")
Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 tools/testing/selftests/kvm/x86_64/hyperv_features.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/kvm/x86_64/hyperv_features.c b/tools/testing/selftests/kvm/x86_64/hyperv_features.c
index 4f4193fc74ff..b923a285e96f 100644
--- a/tools/testing/selftests/kvm/x86_64/hyperv_features.c
+++ b/tools/testing/selftests/kvm/x86_64/hyperv_features.c
@@ -454,7 +454,7 @@ static void guest_test_msrs_access(void)
 		case 44:
 			/* MSR is not available when CPUID feature bit is unset */
 			if (!has_invtsc)
-				continue;
+				goto next_stage;
 			msr->idx = HV_X64_MSR_TSC_INVARIANT_CONTROL;
 			msr->write = false;
 			msr->fault_expected = true;
@@ -462,7 +462,7 @@ static void guest_test_msrs_access(void)
 		case 45:
 			/* MSR is vailable when CPUID feature bit is set */
 			if (!has_invtsc)
-				continue;
+				goto next_stage;
 			vcpu_set_cpuid_feature(vcpu, HV_ACCESS_TSC_INVARIANT);
 			msr->idx = HV_X64_MSR_TSC_INVARIANT_CONTROL;
 			msr->write = false;
@@ -471,7 +471,7 @@ static void guest_test_msrs_access(void)
 		case 46:
 			/* Writing bits other than 0 is forbidden */
 			if (!has_invtsc)
-				continue;
+				goto next_stage;
 			msr->idx = HV_X64_MSR_TSC_INVARIANT_CONTROL;
 			msr->write = true;
 			msr->write_val = 0xdeadbeef;
@@ -480,7 +480,7 @@ static void guest_test_msrs_access(void)
 		case 47:
 			/* Setting bit 0 enables the feature */
 			if (!has_invtsc)
-				continue;
+				goto next_stage;
 			msr->idx = HV_X64_MSR_TSC_INVARIANT_CONTROL;
 			msr->write = true;
 			msr->write_val = 1;
@@ -513,6 +513,7 @@ static void guest_test_msrs_access(void)
 			return;
 		}
 
+next_stage:
 		stage++;
 		kvm_vm_free(vm);
 	}
-- 
2.43.0

[PATCH 2/2] KVM: selftests: Fail tests when open() fails with !ENOENT

[Vitaly Kuznetsov]

open_path_or_exit() is used for '/dev/kvm', '/dev/sev', and
'/sys/module/%s/parameters/%s' and skipping test when the entry is missing
is completely reasonable. Other errors, however, may indicate a real issue
which is easy to miss. E.g. when 'hyperv_features' test was entering an
infinite loop the output was:

./hyperv_features
Testing access to Hyper-V specific MSRs
1..0 # SKIP - /dev/kvm not available (errno: 24)

and this can easily get overlooked.

Keep ENOENT case 'special' for skipping tests and fail when open() results
in any other errno.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 tools/testing/selftests/kvm/lib/kvm_util.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
index e066d584c656..f3dfd0d38b7f 100644
--- a/tools/testing/selftests/kvm/lib/kvm_util.c
+++ b/tools/testing/selftests/kvm/lib/kvm_util.c
@@ -27,7 +27,8 @@ int open_path_or_exit(const char *path, int flags)
 	int fd;
 
 	fd = open(path, flags);
-	__TEST_REQUIRE(fd >= 0, "%s not available (errno: %d)", path, errno);
+	__TEST_REQUIRE(fd >= 0 || errno != ENOENT, "%s not present", path);
+	TEST_ASSERT(fd >= 0, "%s not available (errno: %d)", path, errno);
 
 	return fd;
 }
-- 
2.43.0

Re: [PATCH 2/2] KVM: selftests: Fail tests when open() fails with !ENOENT

[Sean Christopherson]

On Wed, Jan 24, 2024, Vitaly Kuznetsov wrote:
> open_path_or_exit() is used for '/dev/kvm', '/dev/sev', and
> '/sys/module/%s/parameters/%s' and skipping test when the entry is missing
> is completely reasonable. Other errors, however, may indicate a real issue
> which is easy to miss. E.g. when 'hyperv_features' test was entering an
> infinite loop the output was:
> 
> ./hyperv_features
> Testing access to Hyper-V specific MSRs
> 1..0 # SKIP - /dev/kvm not available (errno: 24)
> 
> and this can easily get overlooked.
> 
> Keep ENOENT case 'special' for skipping tests and fail when open() results
> in any other errno.
> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
>  tools/testing/selftests/kvm/lib/kvm_util.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
> index e066d584c656..f3dfd0d38b7f 100644
> --- a/tools/testing/selftests/kvm/lib/kvm_util.c
> +++ b/tools/testing/selftests/kvm/lib/kvm_util.c
> @@ -27,7 +27,8 @@ int open_path_or_exit(const char *path, int flags)
>  	int fd;
>  
>  	fd = open(path, flags);
> -	__TEST_REQUIRE(fd >= 0, "%s not available (errno: %d)", path, errno);
> +	__TEST_REQUIRE(fd >= 0 || errno != ENOENT, "%s not present", path);

Rather than make up our own error messages, can we use strerror()?

> +	TEST_ASSERT(fd >= 0, "%s not available (errno: %d)", path, errno);

And then here just say "Failed to open '%s'" and let test_assert() fill in the
strerror() information.