Re: [GIT PULL] KVM changes for Linux 5.6-rc2

[Paolo Bonzini]

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