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 <[email protected]>
Suggested-by: Sean Christopherson <[email protected]>
Reviewed-by: Jim Mattson <[email protected]>
---
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/[email protected]/
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
Vipin Sharma <[email protected]> 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 <[email protected]>
> Suggested-by: Sean Christopherson <[email protected]>
> Reviewed-by: Jim Mattson <[email protected]>
>
> ---
>
> 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/[email protected]/
>
> 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 <[email protected]>
--
Vitaly
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.
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 <[email protected]> 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 <[email protected]>
> > Suggested-by: Sean Christopherson <[email protected]>
> > Reviewed-by: Jim Mattson <[email protected]>
> >
> > ---
> >
> > 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 <[email protected]>
...
> 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