On Wed, Sep 21, 2022 at 4:46 PM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
>
> On Wed, Sep 21, 2022, Jim Mattson wrote:
> > On Wed, Sep 21, 2022 at 4:11 PM Vipin Sharma <vipinsh@xxxxxxxxxx> 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@xxxxxxxxxx>
> > > Suggested-by: Sean Christopherson <seanjc@xxxxxxxxxx>
> > >
> > > ---
> > > 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.
