Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [GIT PULL] KVM changes for Linux 5.6-rc2
To:     Linus Torvalds <torvalds@linux-foundation.org>
Cc:     Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        KVM list <kvm@vger.kernel.org>,
        Oliver Upton <oupton@google.com>
References: <20200212164714.7733-1-pbonzini@redhat.com>
 <CAHk-=wh6KEgPz_7TFqSgg3T29SrCBU+h64t=BWyCKwJOrk3RLQ@mail.gmail.com>
From:   Paolo Bonzini <pbonzini@redhat.com>
Message-ID: <b90a886e-b320-43d3-b2b6-7032aac57abe@redhat.com>
Date:   Wed, 12 Feb 2020 20:19:25 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.1.1
MIME-Version: 1.0
In-Reply-To: <CAHk-=wh6KEgPz_7TFqSgg3T29SrCBU+h64t=BWyCKwJOrk3RLQ@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 12/02/20 19:53, Linus Torvalds wrote:
> It doesn't even compile. Just in the patch itself - so this is not a
> merge issue, I see this:
> 
>           int (*skip_emulated_instruction)(struct kvm_vcpu *vcpu);
>   ..
>   @@ -1599,6 +1599,40 @@ static int skip_emulated_instruction(struct
> kvm_vcpu *vcpu)
>   ..
>   +static void vmx_skip_emulated_instruction(struct kvm_vcpu *vcpu)
>   +       return skip_emulated_instruction(vcpu);
>   ..
>   -       .skip_emulated_instruction = skip_emulated_instruction,
>   +       .skip_emulated_instruction = vmx_skip_emulated_instruction,
> 
> ie note how that vmx_skip_emulated_instruction() is a void function,
> and then you have
> 
>          return skip_emulated_instruction(vcpu);
> 
> in it, and you assign that garbage to ".skip_emulated_instruction"
> which is supposed to be returning 'int'.

Indeed I missed the warning.  Of course the return value is in %rax so,
despite the patch being shitty (it is), it is also true that it
*happens* to pass the corresponding unit test.

Not a particularly high bar to clear I admit, but enough to explain the
mistake and ensure it doesn't happen again; I have now added "ccflags-y
+= -Werror" to the KVM makefile.

> So this clearly never even got a _whiff_ of build-testing.

Oh come on.

> You're now on my shit-list, which means that I want to see only (a)
> pure fixes and (b) well-tested such. Nothing else will be pulled.

Fair enough, I removed the following patches from the pull request and
will resend:

 KVM: nVMX: Emulate MTF when performing instruction emulation
 KVM: nVMX: Rename nested_ept_get_cr3() to nested_ept_get_eptp()
 KVM: nVMX: Rename EPTP validity helper and associated variables
 KVM: nVMX: Drop unnecessary check on ept caps for execute-only
 KVM: Provide kvm_flush_remote_tlbs_common()
 KVM: MIPS: Drop flush_shadow_memslot() callback
 KVM: MIPS: Replace all the kvm_flush_remote_tlbs() references
 KVM: MIPS: Define arch-specific kvm_flush_remote_tlbs()

The first one is a bug fix, but since it's the one that caused all the
mess I guess it's not really a good idea to argue about it.

Paolo