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

[Andrew Jones]

On Thu, Apr 06, 2017 at 10:02:15PM +0100, James Hogan wrote:
> 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))

Hi James,

I'm actually thinking we should do away with kvm_arch_vcpu_should_kick(),
putting the x86 implementation of it directly in the common code.  The
reason is that, while there are currently two implementations for the
function, the x86 one and 'return true', I don't think 'return true'
is correct.  'return true' doesn't consider whether or not the target VCPU
has interrupts disabled, and I don't see how sending the IPI when the
vcpu is not in guest mode, or has disabled interrupts prior to entering
guest mode, makes sense.  To consider whether the target vcpu has
interrupts disabled we need to require it to tell us.  Requiring the
setting of IN_GUEST_MODE after calling local_irq_disable() is how x86
does it, and seems like it should work for all architectures. So, can you
help me better understand the concerns you have below?

> 
> 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).

But as long as the kicks were due to vcpu requests, then, since the VCPU
should check requests again before reentering guest mode, it'll still
handle them.  I see the comment under the setting of
READING_SHADOW_PAGE_TABLES in arch/mips/kvm/trap_emul.c refers to TLB
flush requests, so that one should be OK.  Are there other kicks that
are request-less to be concerned with?

> - 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).

Can you elaborate on this one?  My understanding is that there should
be no harm in coalescing these IPIs.

> 
> 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.

I can't think of a scenario that requires sending an IPI to a vcpu
outside guest mode, particularly because if it's the only runnable
task on the cpu then nothing will happen anyway.

Thanks,
drew