Vipin Sharma <vipinsh@xxxxxxxxxx> 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@xxxxxxxxxx>
> Suggested-by: Sean Christopherson <seanjc@xxxxxxxxxx>
> Reviewed-by: Jim Mattson <jmattson@xxxxxxxxxx>
>
> ---
>
> 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@xxxxxxxxxx/
>
> 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@xxxxxxxxxx>
--
Vitaly
>From d0670e7d7ed4c4a00f46c1f0b69e1e06eae06c8f Mon Sep 17 00:00:00 2001
From: Vitaly Kuznetsov <vkuznets@xxxxxxxxxx>
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@xxxxxxxxxx>
---
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
