Re: [PATCH RFC 1/6] KVM: fix guest_mode optimization in kvm_make_all_cpus_request()

From: Christian Borntraeger <borntraeger@de.ibm.com>
To: "Radim Krčmář" <rkrcmar@redhat.com>,
	linux-kernel@vger.kernel.org, kvm@vger.kernel.org
Cc: Christoffer Dall <cdall@linaro.org>,
	Andrew Jones <drjones@redhat.com>,
	Marc Zyngier <marc.zyngier@arm.com>,
	Paolo Bonzini <pbonzini@redhat.com>,
	Cornelia Huck <cornelia.huck@de.ibm.com>,
	James Hogan <james.hogan@imgtec.com>,
	Paul Mackerras <paulus@ozlabs.org>
Subject: Re: [PATCH RFC 1/6] KVM: fix guest_mode optimization in kvm_make_all_cpus_request()
Date: Fri, 7 Apr 2017 12:47:38 +0200	[thread overview]
Message-ID: <2edb5eb1-f67e-eeea-a478-008fcb92a812@de.ibm.com> (raw)
In-Reply-To: <20170406202056.18379-2-rkrcmar@redhat.com>

On 04/06/2017 10:20 PM, Radim Krčmář wrote:
> We have kvm_arch_vcpu_should_kick() to decide whether the target cpu
> needs to be kicked.  The previous condition was wrong, because
> architectures that don't use vcpu->mode would not get interrupts and
> also suboptimal, because it sent IPI in cases where none was necessary.
> 
> The situation is even more convoluted.  MIPS and POWERPC return 1 from
> kvm_arch_vcpu_should_kick(), but implement vcpu->mode for some reason,
> so now they might kick uselessly.  This is not a huge problem.
> 
> s390, on the other hand, never changed vcpu->mode, so it would always be
> OUTSIDE_GUEST_MODE before and therefore didn't send IPIs.
> I don't see a reason why s390 had kvm_make_all_cpus_request() that did
> nothing but set the bit in vcpu->request, so the new behavior seems
> better.

As on s390 nobody ever called kvm_make_all_cpus_request this patch should be fine
for s390. But even if somebody would start calling kvm_make_all_cpus_requests
this should do what we want (as long as we do not need the special "make sure
to be really out of guest" thing).

Not sure about the x86 things that James mentioned. 
> 
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> ---
>  arch/s390/kvm/kvm-s390.c | 4 +---
>  virt/kvm/kvm_main.c      | 2 +-
>  2 files changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index fd6cd05bb6a7..45b6d9ca5d24 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -2130,9 +2130,7 @@ static void kvm_gmap_notifier(struct gmap *gmap, unsigned long start,
> 
>  int kvm_arch_vcpu_should_kick(struct kvm_vcpu *vcpu)
>  {
> -	/* kvm common code refers to this, but never calls it */
> -	BUG();
> -	return 0;
> +	return 1;
>  }
> 
>  static int kvm_arch_vcpu_ioctl_get_one_reg(struct kvm_vcpu *vcpu,
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index f489167839c4..2389e9c41cd2 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -187,7 +187,7 @@ bool kvm_make_all_cpus_request(struct kvm *kvm, unsigned int req)
>  		smp_mb__after_atomic();
> 
>  		if (cpus != NULL && cpu != -1 && cpu != me &&
> -		      kvm_vcpu_exiting_guest_mode(vcpu) != OUTSIDE_GUEST_MODE)
> +				kvm_arch_vcpu_should_kick(vcpu))
>  			cpumask_set_cpu(cpu, cpus);
>  	}
>  	if (unlikely(cpus == NULL))
> 