Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 22 Sep 2022 20:20:41 +0000
From:   Sean Christopherson <seanjc@google.com>
To:     Vitaly Kuznetsov <vkuznets@redhat.com>
Cc:     Vipin Sharma <vipinsh@google.com>, pbonzini@redhat.com,
        jmattson@google.com, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] KVM: selftests: Check result in hyperv_features.c
 test only for successful hypercalls
Message-ID: <YyzDmYAhWkMRt6E4@google.com>
References: <20220922062451.2927010-1-vipinsh@google.com>
 <87fsgjol20.fsf@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <87fsgjol20.fsf@redhat.com>
Precedence: bulk

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