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

[James Hogan]

Hi Drew,

Note, MIPS doesn't directly use kicks as far as I can tell, only the TLB
flush request, so what I say below is in the context of requests where
the IPI is waited for.

On Mon, Apr 10, 2017 at 05:59:42PM +0200, Andrew Jones wrote:
> 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,

Generally agreed, except for the READING_SHADOW_PAGE_TABLES case.

> or has disabled interrupts prior to entering guest mode, makes sense.

OTOH:
1) disable interrups
2) set mode to IN_GUEST_MODE
3) check requests
4) enter guest mode

Before (3) an IPI is redundant since the requests will be checked prior
to entering guest mode anyway, before any guest mappings are accessed.

After (3) the IPI is important as its too late to handle the request
before entering guest mode, so the IPI is needed to inform
kvm_flush_remote_tlbs() when it is safe to continue, and to trigger a
prompt exit from guest mode so it isn't waiting too long.

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

At least for MIPS reading shadow page tables is treated a bit like being
in guest mode, in that guest mappings are accessed (including
potentially stale ones before kvm_flush_remote_tlbs() has returned), and
has to be done with IRQs disabled before also checking requests (to
handle requests sent prior to reading shadow page tables). The only
difference is it doesn't happen in guest mode and IRQs are properly
disabled so the IPI is delayed rather than interupting the activity.

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

Not that I'm aware of for MIPS.

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

My concern was e.g.:

CPU1			CPU2			CPU3 (in guest mode)
----------------------- ----------------------- ------------------------
kvm_flush_remote_tlbs()
			kvm_flush_remote_tlbs()
IN_GUEST_MODE->EXITING_GUEST_MODE
			EXITING_GUEST_MODE
			return without IPI
						*continue accessing*
						*guest mappings*

send IPI to CPU3 & wait ----------------------> Exit guest mode
						irqs enable
						take IPI
<-----------------------------------------------
wake and return

(i.e. kvm_flush_remote_tlbs() on CPU2 returned while stale mappings
still in use).

However at least for MIPS I think kvm->mmu_lock should protect against
that by serialising the second kvm_flush_remote_tlbs() after the first
is complete. If anything else can switch mode to EXITING_GUEST_MODE (a
kick?) without locking, then perhaps it could still be a problem?

Cheers
James

Attachment: signature.asc
Description: Digital signature