Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754337AbdDKKoo (ORCPT ); Tue, 11 Apr 2017 06:44:44 -0400 Received: from mailapp01.imgtec.com ([195.59.15.196]:15641 "EHLO imgpgp01.kl.imgtec.org" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1752506AbdDKKnc (ORCPT ); Tue, 11 Apr 2017 06:43:32 -0400 X-PGP-Universal: processed; by imgpgp01.kl.imgtec.org on Tue, 11 Apr 2017 12:50:00 +0100 Date: Tue, 11 Apr 2017 11:43:29 +0100 From: James Hogan To: Andrew Jones CC: Radim =?utf-8?B?S3LEjW3DocWZ?= , , , Christoffer Dall , Marc Zyngier , Paolo Bonzini , Christian Borntraeger , Cornelia Huck , Paul Mackerras Subject: Re: [PATCH RFC 1/6] KVM: fix guest_mode optimization in kvm_make_all_cpus_request() Message-ID: <20170411104329.GB31606@jhogan-linux.le.imgtec.org> References: <20170406202056.18379-1-rkrcmar@redhat.com> <20170406202056.18379-2-rkrcmar@redhat.com> <20170406210215.GV31606@jhogan-linux.le.imgtec.org> <20170410155942.7blxepebuhv2mppf@kamzik.brq.redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="NP+fiIeNjDlB/E6h" Content-Disposition: inline In-Reply-To: <20170410155942.7blxepebuhv2mppf@kamzik.brq.redhat.com> User-Agent: Mutt/1.5.24 (2015-08-30) X-Originating-IP: [192.168.154.110] X-ESG-ENCRYPT-TAG: 1b7d744b Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5223 Lines: 131 --NP+fiIeNjDlB/E6h Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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? >=20 > >=20 > > This presumably changes the behaviour on x86, from !=3D OUTSIDE_GUEST_M= ODE > > to =3D=3D 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). >=20 > 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. >=20 > > - 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). >=20 > 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 --NP+fiIeNjDlB/E6h Content-Type: application/pgp-signature; name="signature.asc" Content-Description: Digital signature -----BEGIN PGP SIGNATURE----- iQIcBAEBCAAGBQJY7LM/AAoJEGwLaZPeOHZ6kJQQAJZ+DOpPwBSNJyO4gz4hOjgN CxQvYEaz6wRHe/CcYWIJZKhkXacGevULsgVGo3phvL4G0vXloFPn0EDQXn5k8Th7 X5M6uieYQmrL+pvc2PWKbdwwCGjZmvBnCEVP4uEbJHufYzZUp9uPuxEfdoqPjnr7 XbnCEai0E2eG6DHpqgTlz0Kd8HOy1gAZwEkafK3xFq1bi+MxZYv8m5XcaWEb9KLC ezhEaeg9bFIeKGP57eYd0dUbWy1GWAzX5UlF3Ca3mPEi2JI5sB5slhakvDZSLVaL 0kd7JrLXObayJ8Q7ahhfQrTRXp3CoBs5ALh/mPeMplYpfjEcURVyZyZUZqmzbriX ipxNLth3tDx2fFYxRZdoPkNkPVtZy+TIEkyjp8wu8pRN5kovlXxQDk4ohZzWG8fY cH3cgZwDlZUfkH6tmItRWnmM5CpJvtjb6lGTKKBexUR5rrKf16zr0ldW0ht59CMk Kx5LTUO3U/12WvnrvcqVZ87U8tmynQToCZShluqJ8jlSF6B5joYNs0S6txOpC9yv vZCg2KR+VJpFVNarCu/CLkIUjt8qHhVHfaX7ahP3UVSiGk4gEP54qro5G4FQ/8Gb YgBqUQ4E1NGiEV6u/0HpCFJnR5z8TbqVGzmxjY1J0ngsw6vKOejNaM2K+2Ss5NhE piLMoGORV1RIn8My8UVR =A8Y3 -----END PGP SIGNATURE----- --NP+fiIeNjDlB/E6h--