[PATCH] KVM: selftests: Fix hyperv_features test failure when built on Clang

[PATCH] KVM: selftests: Fix hyperv_features test failure when built on Clang

[Vipin Sharma]

hyperv_features test fails when built on Clang. It throws error:

	 Failed guest assert: !hcall->ud_expected || res == hcall->expect at
	 x86_64/hyperv_features.c:90

On GCC, EAX is set to 0 before the hypercall whereas in Clang it is not,
this causes EAX to have garbage value when hypercall is returned in Clang
binary.

Fix by executing the guest assertion only when ud_expected is false.

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>

---
 tools/testing/selftests/kvm/x86_64/hyperv_features.c | 8 ++++----
 1 file changed, 4 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 79ab0152d281..ad01868548f9 100644
--- a/tools/testing/selftests/kvm/x86_64/hyperv_features.c
+++ b/tools/testing/selftests/kvm/x86_64/hyperv_features.c
@@ -81,13 +81,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] KVM: selftests: Fix hyperv_features test failure when built on Clang

[Jim Mattson]

On Wed, Sep 21, 2022 at 4:11 PM Vipin Sharma <vipinsh@google.com> wrote:
>
> hyperv_features test fails when built on Clang. It throws error:
>
>          Failed guest assert: !hcall->ud_expected || res == hcall->expect at
>          x86_64/hyperv_features.c:90
>
> On GCC, EAX is set to 0 before the hypercall whereas in Clang it is not,
> this causes EAX to have garbage value when hypercall is returned in Clang
> binary.
>
> Fix by executing the guest assertion only when ud_expected is false.
>
> 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>
>
> ---
>  tools/testing/selftests/kvm/x86_64/hyperv_features.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)

In case Sean doesn't point it out, be wary of starting a shortlog with
"Fix." You may later regret it.

Also, I think the "clang" part is a red herring. You are fixing a
latent bug in the code.

Reviewed-by: Jim Mattson <jmattson@google.com>

Re: [PATCH] KVM: selftests: Fix hyperv_features test failure when built on Clang

[Sean Christopherson]

On Wed, Sep 21, 2022, Jim Mattson wrote:
> On Wed, Sep 21, 2022 at 4:11 PM Vipin Sharma <vipinsh@google.com> wrote:
> >
> > hyperv_features test fails when built on Clang. It throws error:
> >
> >          Failed guest assert: !hcall->ud_expected || res == hcall->expect at
> >          x86_64/hyperv_features.c:90
> >
> > On GCC, EAX is set to 0 before the hypercall whereas in Clang it is not,
> > this causes EAX to have garbage value when hypercall is returned in Clang
> > binary.
> >
> > Fix by executing the guest assertion only when ud_expected is false.

TL;DR: please rewrite the changelog to explain the actual bug (checking the
result when the hypercall is expected to fault) and the fix, and only mention the
gcc vs. clang behavior as a footnote.

As Jim pointed out, the bug has nothing to do with clang.  Ha, figured out why
gcc passes; it uses RAX as the scratch reg that the asm blob loads into R8,
i.e. loads RAX with @output_address.  So ignore my earlier suggestion of:

	*hv_status = -EFAULT,

even better is to do:

diff --git a/tools/testing/selftests/kvm/x86_64/hyperv_features.c b/tools/testing/selftests/kvm/x86_64/hyperv_features.c
index 79ab0152d281..673085f6edfd 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;
 }

so that there is zero chance of getting a false positive due to the compiler
(but not KVM) modifying RAX.

Anyways, this bug is not clang's fault, with above patch it fails as "expected".

==== Test Assertion Failure ====
  x86_64/hyperv_features.c:622: false
  pid=283847 tid=283847 errno=4 - Interrupted system call
     1	0x0000000000402842: guest_test_hcalls_access at hyperv_features.c:622
     2	 (inlined by) main at hyperv_features.c:642
     3	0x00007f23fc513082: ?? ??:0
     4	0x0000000000402ebd: _start at ??:?
  Failed guest assert: !hcall->ud_expected || res == hcall->expect at x86_64/hyperv_features.c:90
arg1 = 0, arg2 = fffffff2


> > 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>
> >
> > ---
> >  tools/testing/selftests/kvm/x86_64/hyperv_features.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> In case Sean doesn't point it out, be wary of starting a shortlog with
> "Fix." You may later regret it.

Heh, I think our record is "Really, really fix xyz" for a shortlog.

> Also, I think the "clang" part is a red herring. You are fixing a
> latent bug in the code.

Ya, it's definitely a good idea to call out why a bug was missed, but clang is
not to blame here, at all.  

Re: [PATCH] KVM: selftests: Fix hyperv_features test failure when built on Clang

[Vipin Sharma]

On Wed, Sep 21, 2022 at 4:46 PM Sean Christopherson <seanjc@google.com> wrote:
>
> On Wed, Sep 21, 2022, Jim Mattson wrote:
> > On Wed, Sep 21, 2022 at 4:11 PM Vipin Sharma <vipinsh@google.com> wrote:
> > >
> > > hyperv_features test fails when built on Clang. It throws error:
> > >
> > >          Failed guest assert: !hcall->ud_expected || res == hcall->expect at
> > >          x86_64/hyperv_features.c:90
> > >
> > > On GCC, EAX is set to 0 before the hypercall whereas in Clang it is not,
> > > this causes EAX to have garbage value when hypercall is returned in Clang
> > > binary.
> > >
> > > Fix by executing the guest assertion only when ud_expected is false.
>
> TL;DR: please rewrite the changelog to explain the actual bug (checking the
> result when the hypercall is expected to fault) and the fix, and only mention the
> gcc vs. clang behavior as a footnote.

On it.
>
> As Jim pointed out, the bug has nothing to do with clang.  Ha, figured out why
> gcc passes; it uses RAX as the scratch reg that the asm blob loads into R8,
> i.e. loads RAX with @output_address.  So ignore my earlier suggestion of:
>
>         *hv_status = -EFAULT,
>
> even better is to do:
>
> diff --git a/tools/testing/selftests/kvm/x86_64/hyperv_features.c b/tools/testing/selftests/kvm/x86_64/hyperv_features.c
> index 79ab0152d281..673085f6edfd 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;
>  }
>
> so that there is zero chance of getting a false positive due to the compiler
> (but not KVM) modifying RAX.
>

Taking it.

> Anyways, this bug is not clang's fault, with above patch it fails as "expected".
>
> ==== Test Assertion Failure ====
>   x86_64/hyperv_features.c:622: false
>   pid=283847 tid=283847 errno=4 - Interrupted system call
>      1  0x0000000000402842: guest_test_hcalls_access at hyperv_features.c:622
>      2   (inlined by) main at hyperv_features.c:642
>      3  0x00007f23fc513082: ?? ??:0
>      4  0x0000000000402ebd: _start at ??:?
>   Failed guest assert: !hcall->ud_expected || res == hcall->expect at x86_64/hyperv_features.c:90
> arg1 = 0, arg2 = fffffff2
>
>
> > > 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>
> > >
> > > ---
> > >  tools/testing/selftests/kvm/x86_64/hyperv_features.c | 8 ++++----
> > >  1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > In case Sean doesn't point it out, be wary of starting a shortlog with
> > "Fix." You may later regret it.

I am doing it already! :D

>
> Heh, I think our record is "Really, really fix xyz" for a shortlog.
>
> > Also, I think the "clang" part is a red herring. You are fixing a
> > latent bug in the code.
>
> Ya, it's definitely a good idea to call out why a bug was missed, but clang is
> not to blame here, at all.

I agree, my understanding was not complete for this bug. I will send
out a v2 with an updated shortlog. Thanks, Jim and Sean, for pointing
it out.