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

[James Hogan]

On Thu, Apr 06, 2017 at 10:20:51PM +0200, 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.

Whoops. I hadn't spotted kvm_arch_vcpu_should_kick() when I added
vcpu->mode stuff in 4.11... I'm guessing I need to implement that
similar to ARM / x86... though MIPS doesn't use kvm_vcpu_kick() yet.

> 
> 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.
> 
> Signed-off-by: Radim KrÄmÃÅ <rkrcmar@xxxxxxxxxx>
> ---
>  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))

This presumably changes the behaviour on x86, from != OUTSIDE_GUEST_MODE
to == IN_GUEST_MODE. so:
- you'll no longer get IPIs if its in READING_SHADOW_PAGE_TABLES (which
  MIPS also now uses when accessing mappings outside of guest mode and
  depends upon to wait until the old mappings are no longer in use).
- you'll no longer get IPIs if its in EXITING_GUEST_MODE (i.e. if you
  get two of these in quick succession only the first will wait for the
  IPI, which might work as long as they're already serialised but it
  still feels wrong).

But it doesn't seem right to change the kvm_arch_vcpu_should_kick()
implementations to check kvm_vcpu_exiting_guest_mode(vcpu) !=
OUTSIDE_GUEST_MODE to match condition either, since kvm_vcpu_kick()
doesn't seem to need synchronisation, only to know that it won't be
delayed in reaching hypervisor code.

Cheers
James


>  			cpumask_set_cpu(cpu, cpus);
>  	}
>  	if (unlikely(cpus == NULL))
> -- 
> 2.12.0
>

Attachment: signature.asc
Description: Digital signature