From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:34491) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SiE8V-0003p9-5L for qemu-devel@nongnu.org; Fri, 22 Jun 2012 20:23:36 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SiE8T-0005qa-Bt for qemu-devel@nongnu.org; Fri, 22 Jun 2012 20:23:34 -0400 Received: from mx1.redhat.com ([209.132.183.28]:11214) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SiE8T-0005qK-3a for qemu-devel@nongnu.org; Fri, 22 Jun 2012 20:23:33 -0400 Date: Fri, 22 Jun 2012 21:22:59 -0300 From: Marcelo Tosatti Message-ID: <20120623002259.GA13440@amt.cnet> References: <4FE4F56D.1020201@web.de> <4FE4F7F5.7030400@web.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4FE4F7F5.7030400@web.de> Subject: Re: [Qemu-devel] [PATCH] kvm: First step to push iothread lock out of inner run loop List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jan Kiszka Cc: Liu Ping Fan , kvm , qemu-devel , Alexander Graf , Avi Kivity , Anthony Liguori On Sat, Jun 23, 2012 at 12:55:49AM +0200, Jan Kiszka wrote: > Should have declared this [RFC] in the subject and CC'ed kvm... > > On 2012-06-23 00:45, Jan Kiszka wrote: > > This sketches a possible path to get rid of the iothread lock on vmexits > > in KVM mode. On x86, the the in-kernel irqchips has to be used because > > we otherwise need to synchronize APIC and other per-cpu state accesses > > that could be changed concurrently. Not yet fully analyzed is the NMI > > injection path in the absence of an APIC. > > > > s390x should be fine without specific locking as their pre/post-run > > callbacks are empty. Power requires locking for the pre-run callback. > > > > This patch is untested, but a similar version was successfully used in > > a x86 setup with a network I/O path that needed no central iothread > > locking anymore (required special MMIO exit handling). > > --- > > kvm-all.c | 18 ++++++++++++++++-- > > target-i386/kvm.c | 7 +++++++ > > target-ppc/kvm.c | 4 ++++ > > 3 files changed, 27 insertions(+), 2 deletions(-) > > > > diff --git a/kvm-all.c b/kvm-all.c > > index f8e4328..9c3e26f 100644 > > --- a/kvm-all.c > > +++ b/kvm-all.c > > @@ -1460,6 +1460,8 @@ int kvm_cpu_exec(CPUArchState *env) > > return EXCP_HLT; > > } > > > > + qemu_mutex_unlock_iothread(); > > + > > do { > > if (env->kvm_vcpu_dirty) { > > kvm_arch_put_registers(env, KVM_PUT_RUNTIME_STATE); > > @@ -1476,14 +1478,16 @@ int kvm_cpu_exec(CPUArchState *env) > > */ > > qemu_cpu_kick_self(); > > } > > - qemu_mutex_unlock_iothread(); > > > > run_ret = kvm_vcpu_ioctl(env, KVM_RUN, 0); > > > > - qemu_mutex_lock_iothread(); > > kvm_arch_post_run(env, run); target-i386/kvm.c void kvm_arch_post_run(CPUX86State *env, struct kvm_run *run) { if (run->if_flag) { env->eflags |= IF_MASK; } else { env->eflags &= ~IF_MASK; } cpu_set_apic_tpr(env->apic_state, run->cr8); cpu_set_apic_base(env->apic_state, run->apic_base); } Clearly there is no structure to any of the writes around the writes in x86's kvm_arch_post_run, so it is unsafe. In kvm_flush_coalesced_mmio_buffer, however, the first and last pointers can be read safely without the global lock (so you could move the lock after reading run->exit_reason, in that case). > > + /* TODO: push coalesced mmio flushing to the point where we access > > + * devices that are using it (currently VGA and E1000). */ > > + qemu_mutex_lock_iothread(); > > kvm_flush_coalesced_mmio_buffer(); > > + qemu_mutex_unlock_iothread(); But you have to flush first to then figure out which device the coalesced mmio belongs to (don't get that comment). It would be good to move the global lock acquision to as late as possible, but acquiring/reacquiring is not very useful (can result in a slowdown, actually).