[PATCH v2] KVM: selftests: Check result in hyperv_features.c test only for successful hypercalls

[PATCH v2] KVM: selftests: Check result in hyperv_features.c test only for successful hypercalls

[Vipin Sharma]

Commit cc5851c6be86 ("KVM: selftests: Use exception fixup for #UD/#GP
Hyper-V MSR/hcall tests") introduced a wrong guest assert in guest_hcall().
It is not checking the successful hypercall results and only checks the result
when a fault happens.

  GUEST_ASSERT_2(!hcall->ud_expected || res == hcall->expect,
                 hcall->expect, res);

Correct the assertion by only checking results of the successful
hypercalls.

This issue was observed when this test started failing after building it
in Clang. Above guest assert statement fails because "res" is not equal
to "hcall->expect" when "hcall->ud_expected" is true. "res" gets some
garbage value in Clang from the RAX register. In GCC, RAX is 0 because
it using RAX for @output_address in the asm statement and resetting it
to 0 before using it as output operand in the same asm statement. Clang
is not using RAX for @output_address.

Load RAX with some default input value so that the compiler cannot
modify it or use it for anything else. This makes sure that KVM is
correctly clearing up return value on successful hypercall and compiler cannot
generate any false positive.

Fixes: cc5851c6be86 ("KVM: selftests: Use exception fixup for #UD/#GP Hyper-V MSR/hcall tests")
Signed-off-by: Vipin Sharma <vipinsh@google.com>
Suggested-by: Sean Christopherson <seanjc@google.com>
Reviewed-by: Jim Mattson <jmattson@google.com>

---

Jim's Reviewed-by is only for the code change and not shortlog message
of v1. Also, there is one change in asm which was not present in v1 and
not reviewed by Jim. But I am writing his name here so that it is not missed
when patch is merged.

v2:
- Updated the shortlog message.
- Using RAX register in hypercall asm as input operand also and
  initializing it with -EFAULT

v1:
https://lore.kernel.org/lkml/20220921231151.2321058-1-vipinsh@google.com/

 tools/testing/selftests/kvm/x86_64/hyperv_features.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/tools/testing/selftests/kvm/x86_64/hyperv_features.c b/tools/testing/selftests/kvm/x86_64/hyperv_features.c
index 79ab0152d281..4d55e038c2d7 100644
--- a/tools/testing/selftests/kvm/x86_64/hyperv_features.c
+++ b/tools/testing/selftests/kvm/x86_64/hyperv_features.c
@@ -26,7 +26,8 @@ static inline uint8_t hypercall(u64 control, vm_vaddr_t input_address,
 		     : "=a" (*hv_status),
 		       "+c" (control), "+d" (input_address),
 		       KVM_ASM_SAFE_OUTPUTS(vector)
-		     : [output_address] "r"(output_address)
+		     : [output_address] "r"(output_address),
+		       "a" (-EFAULT)
 		     : "cc", "memory", "r8", KVM_ASM_SAFE_CLOBBERS);
 	return vector;
 }
@@ -81,13 +82,13 @@ static void guest_hcall(vm_vaddr_t pgs_gpa, struct hcall_data *hcall)
 	}
 
 	vector = hypercall(hcall->control, input, output, &res);
-	if (hcall->ud_expected)
+	if (hcall->ud_expected) {
 		GUEST_ASSERT_2(vector == UD_VECTOR, hcall->control, vector);
-	else
+	} else {
 		GUEST_ASSERT_2(!vector, hcall->control, vector);
+		GUEST_ASSERT_2(res == hcall->expect, hcall->expect, res);
+	}
 
-	GUEST_ASSERT_2(!hcall->ud_expected || res == hcall->expect,
-			hcall->expect, res);
 	GUEST_DONE();
 }
 
-- 
2.37.3.968.ga6b4b080e4-goog

Re: [PATCH v2] KVM: selftests: Check result in hyperv_features.c test only for successful hypercalls

[Vitaly Kuznetsov]

[-- Attachment #1: Type: text/plain, Size: 4431 bytes --]

Vipin Sharma <vipinsh@google.com> writes:

> Commit cc5851c6be86 ("KVM: selftests: Use exception fixup for #UD/#GP
> Hyper-V MSR/hcall tests") introduced a wrong guest assert in guest_hcall().
> It is not checking the successful hypercall results and only checks the result
> when a fault happens.
>
>   GUEST_ASSERT_2(!hcall->ud_expected || res == hcall->expect,
>                  hcall->expect, res);
>
> Correct the assertion by only checking results of the successful
> hypercalls.
>
> This issue was observed when this test started failing after building it
> in Clang. Above guest assert statement fails because "res" is not equal
> to "hcall->expect" when "hcall->ud_expected" is true. "res" gets some
> garbage value in Clang from the RAX register. In GCC, RAX is 0 because
> it using RAX for @output_address in the asm statement and resetting it
> to 0 before using it as output operand in the same asm statement. Clang
> is not using RAX for @output_address.
>
> Load RAX with some default input value so that the compiler cannot
> modify it or use it for anything else. This makes sure that KVM is
> correctly clearing up return value on successful hypercall and compiler cannot
> generate any false positive.
>
> Fixes: cc5851c6be86 ("KVM: selftests: Use exception fixup for #UD/#GP Hyper-V MSR/hcall tests")
> Signed-off-by: Vipin Sharma <vipinsh@google.com>
> Suggested-by: Sean Christopherson <seanjc@google.com>
> Reviewed-by: Jim Mattson <jmattson@google.com>
>
> ---
>
> Jim's Reviewed-by is only for the code change and not shortlog message
> of v1. Also, there is one change in asm which was not present in v1 and
> not reviewed by Jim. But I am writing his name here so that it is not missed
> when patch is merged.
>
> v2:
> - Updated the shortlog message.
> - Using RAX register in hypercall asm as input operand also and
>   initializing it with -EFAULT
>
> v1:
> https://lore.kernel.org/lkml/20220921231151.2321058-1-vipinsh@google.com/
>
>  tools/testing/selftests/kvm/x86_64/hyperv_features.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/tools/testing/selftests/kvm/x86_64/hyperv_features.c b/tools/testing/selftests/kvm/x86_64/hyperv_features.c
> index 79ab0152d281..4d55e038c2d7 100644
> --- a/tools/testing/selftests/kvm/x86_64/hyperv_features.c
> +++ b/tools/testing/selftests/kvm/x86_64/hyperv_features.c
> @@ -26,7 +26,8 @@ static inline uint8_t hypercall(u64 control, vm_vaddr_t input_address,
>  		     : "=a" (*hv_status),
>  		       "+c" (control), "+d" (input_address),
>  		       KVM_ASM_SAFE_OUTPUTS(vector)
> -		     : [output_address] "r"(output_address)
> +		     : [output_address] "r"(output_address),
> +		       "a" (-EFAULT)
>  		     : "cc", "memory", "r8", KVM_ASM_SAFE_CLOBBERS);
>  	return vector;
>  }
> @@ -81,13 +82,13 @@ static void guest_hcall(vm_vaddr_t pgs_gpa, struct hcall_data *hcall)
>  	}
>  
>  	vector = hypercall(hcall->control, input, output, &res);
> -	if (hcall->ud_expected)
> +	if (hcall->ud_expected) {
>  		GUEST_ASSERT_2(vector == UD_VECTOR, hcall->control, vector);
> -	else
> +	} else {
>  		GUEST_ASSERT_2(!vector, hcall->control, vector);
> +		GUEST_ASSERT_2(res == hcall->expect, hcall->expect, res);
> +	}
>  
> -	GUEST_ASSERT_2(!hcall->ud_expected || res == hcall->expect,
> -			hcall->expect, res);
>  	GUEST_DONE();
>  }

And this immediately discovers a problem in the test!

$ ./x86_64/hyperv_features 
Testing access to Hyper-V specific MSRs
Testing access to Hyper-V hypercalls
==== Test Assertion Failure ====
  x86_64/hyperv_features.c:622: false
  pid=3683520 tid=3683520 errno=4 - Interrupted system call
     1	0x0000000000402832: guest_test_hcalls_access at hyperv_features.c:622
     2	 (inlined by) main at hyperv_features.c:642
     3	0x00007f546503feaf: ?? ??:0
     4	0x00007f546503ff5f: ?? ??:0
     5	0x0000000000402eb4: _start at ??:?
  Failed guest assert: res == hcall->expect at x86_64/hyperv_features.c:89
arg1 = 2, arg2 = 3

The root cause is: we're trying to test an invalid hypercall code but we
set 'control' wrong, i.e.:

	hcall->control = 0xdeadbeef;
	hcall->expect = HV_STATUS_INVALID_HYPERCALL_CODE;

as '0xdeadbeef' contains reserved bits 27 through 31 and we're getting
HV_STATUS_INVALID_HYPERCALL_INPUT instead.

Could you please include the attached patch to your series? Thanks a bunch!

For your patch:
Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>

-- 
Vitaly


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-KVM-selftests-Do-not-set-reserved-control-bits-when-.patch --]
[-- Type: text/x-patch, Size: 1408 bytes --]

From d0670e7d7ed4c4a00f46c1f0b69e1e06eae06c8f Mon Sep 17 00:00:00 2001
From: Vitaly Kuznetsov <vkuznets@redhat.com>
Date: Thu, 22 Sep 2022 10:39:41 +0200
Subject: [PATCH] KVM: selftests: Do not set reserved control bits when testing
 invalid Hyper-V hypercall number
Content-Type: text/plain

Bits 27 through 31 in Hyper-V hypercall 'control' are reserved (see
HV_HYPERCALL_RSVD0_MASK) but '0xdeadbeef' includes them. This causes
KVM to return HV_STATUS_INVALID_HYPERCALL_INPUT instead of the expected
HV_STATUS_INVALID_HYPERCALL_CODE.

The test doesn't currently fail as the problem is masked by the wrong check
of the hypercall return code, this is going to be fixed separately.

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

diff --git a/tools/testing/selftests/kvm/x86_64/hyperv_features.c b/tools/testing/selftests/kvm/x86_64/hyperv_features.c
index 79ab0152d281..d71b5cd4b74b 100644
--- a/tools/testing/selftests/kvm/x86_64/hyperv_features.c
+++ b/tools/testing/selftests/kvm/x86_64/hyperv_features.c
@@ -507,7 +507,7 @@ static void guest_test_hcalls_access(void)
 		switch (stage) {
 		case 0:
 			feat->eax |= HV_MSR_HYPERCALL_AVAILABLE;
-			hcall->control = 0xdeadbeef;
+			hcall->control = 0xbeef;
 			hcall->expect = HV_STATUS_INVALID_HYPERCALL_CODE;
 			break;
 
-- 
2.37.3

Re: [PATCH v2] KVM: selftests: Check result in hyperv_features.c test only for successful hypercalls

[Sean Christopherson]

Tweaked the shortlog to make it a wee bit shorter:

  KVM: selftests: Check result in hyperv_features for successful hypercalls

On Thu, Sep 22, 2022, Vitaly Kuznetsov wrote:
> Vipin Sharma <vipinsh@google.com> writes:
> 
> > Commit cc5851c6be86 ("KVM: selftests: Use exception fixup for #UD/#GP
> > Hyper-V MSR/hcall tests") introduced a wrong guest assert in guest_hcall().
> > It is not checking the successful hypercall results and only checks the result

Wrap changelogs at ~75 chars.  It's ok to go over for things like stack traces
and Fixes:, where the format of the text is more important than the line length.
But for "just words", stay under 75 chars.

> > when a fault happens.
> >
> >   GUEST_ASSERT_2(!hcall->ud_expected || res == hcall->expect,
> >                  hcall->expect, res);
> >
> > Correct the assertion by only checking results of the successful
> > hypercalls.
> >
> > This issue was observed when this test started failing after building it
> > in Clang. Above guest assert statement fails because "res" is not equal
> > to "hcall->expect" when "hcall->ud_expected" is true. "res" gets some
> > garbage value in Clang from the RAX register. In GCC, RAX is 0 because
> > it using RAX for @output_address in the asm statement and resetting it
> > to 0 before using it as output operand in the same asm statement. Clang
> > is not using RAX for @output_address.
> >
> > Load RAX with some default input value so that the compiler cannot
> > modify it or use it for anything else. This makes sure that KVM is

Try to avoid pronouns as they are often ambiguous, and even when they're not, using
pronouns can sometimes require more effort from the readers, e.g. might require the
reader to "jump back" in the sentence to understand what "it" means.

And for cases like this, RAX is one more char, so just type RAX.

> > correctly clearing up return value on successful hypercall and compiler cannot
> > generate any false positive.
> >
> > Fixes: cc5851c6be86 ("KVM: selftests: Use exception fixup for #UD/#GP Hyper-V MSR/hcall tests")
> > Signed-off-by: Vipin Sharma <vipinsh@google.com>
> > Suggested-by: Sean Christopherson <seanjc@google.com>
> > Reviewed-by: Jim Mattson <jmattson@google.com>
> >
> > ---
> >
> > Jim's Reviewed-by is only for the code change and not shortlog message
> > of v1.

This is "working as intended".  When someone gives a conditional Reviewed-by,
the intent is very much that _if_ you make the requested changes in good faith,
then their review will be carried.  In other words, not adding Jim's review
would be "wrong" from a certain perspective.

> > Also, there is one change in asm which was not present in v1 and
> > not reviewed by Jim. But I am writing his name here so that it is not missed
> > when patch is merged.

Heh, the fact that you felt compelled to write this is a very, very good indication
that this should be two patches.

The bug Vitaly encountered is exactly why it's pre

> Could you please include the attached patch to your series? Thanks a bunch!

Pushed both (as three patches) to branch `for_paolo/6.1` at:

    https://github.com/sean-jc/linux.git

> For your patch:
> Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>

...

> Subject: [PATCH] KVM: selftests: Do not set reserved control bits when testing
>  invalid Hyper-V hypercall number

I shortened this one too, 94 chars is a bit much :-)

  KVM: selftests: Don't set reserved bits for invalid Hyper-V hypercall number

Re: [PATCH v2] KVM: selftests: Check result in hyperv_features.c test only for successful hypercalls

[Sean Christopherson]

On Thu, Sep 22, 2022, Sean Christopherson wrote:
> The bug Vitaly encountered is exactly why it's pre

Premature send :-)

What I was going to say...

The bug Vitaly encountered is exactly why upstream process _strongly_ prefers
splitting patches by logical changes, even when the changes are related or tiny.
Bundling the fix for the egregious bug with the enhancement makes it unnecessarily
difficult to grab _just_ the fix.  In this case, Vitaly was on top of things and
there was minimal fallout, but imagine if the fix was for KVM proper and needed to
be backported.  Some unsuspecting user would grab the "fix", apply it to their
kernel, and suddenly be presented with previously unseen failures.