From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:40891) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SnmFD-0002yc-If for qemu-devel@nongnu.org; Sun, 08 Jul 2012 03:49:28 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SnmFA-0002Zc-Kc for qemu-devel@nongnu.org; Sun, 08 Jul 2012 03:49:27 -0400 Received: from mx1.redhat.com ([209.132.183.28]:56908) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SnmFA-0002ZI-CV for qemu-devel@nongnu.org; Sun, 08 Jul 2012 03:49:24 -0400 Message-ID: <4FF93B76.90206@redhat.com> Date: Sun, 08 Jul 2012 10:49:10 +0300 From: Avi Kivity MIME-Version: 1.0 References: <4FE4F56D.1020201@web.de> <4FE4F7F5.7030400@web.de> <20120623002259.GA13440@amt.cnet> <20120623090646.GA21908@amt.cnet> <4FE5AC75.1020504@web.de> <4FE6D4A6.9080708@redhat.com> <4FE71F44.9020800@web.de> <4FF71D5E.4060409@siemens.com> <4FF72931.7020704@siemens.com> In-Reply-To: <4FF72931.7020704@siemens.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit 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 , Gleb Natapov , kvm , Marcelo Tosatti , qemu-devel , Alexander Graf , Anthony Liguori On 07/06/2012 09:06 PM, Jan Kiszka wrote: > On 2012-07-06 19:16, Jan Kiszka wrote: >> On 2012-06-24 16:08, Jan Kiszka wrote: >>> On 2012-06-24 10:49, Avi Kivity wrote: >>>> On 06/23/2012 02:45 PM, Jan Kiszka wrote: >>>>> >>>>> Hmm, we may need the iothread lock around cpu_set_apic_tpr for >>>>> !kvm_irqchip_in_kernel(). And as we are at it, apic_base manipulation >>>>> can be but there as well. >>>>> >>>>> With in-kernel irqchip, there is no such need. Also, no one accesses >>>>> eflags outside of the vcpu thread, independent of the irqchip mode. >>>> >>>> In fact !kvm_irqchip_in_kernel() is broken wrt the tpr. Interrupt >>>> injection needs to be done atomically, but currently we check the tpr >>>> from the injecting thread, which means the cpu thread can race with it. >>>> We need to move the check to the vcpu thread so that the guest vcpu is >>>> halted. >>> >>> So apic_set_irq basically needs to be deferred to vcpu context, right? >>> Will have a look. >> >> Tried to wrap my head around this, but only found different issues >> (patches under construction). >> >> First of all, a simple run_on_cpu doesn't work as it may drops the BQL >> at unexpected points inside device models. >> >> Then I thought about what could actually race here: The testing of the >> userspace TPR value under BQL vs. some modification by the CPU while in >> KVM mode. So we may either inject while the CPU is trying to prevent >> this - harmless as it happens on real hw as well - or not inject while > > Hmm, this could actually be a problem as the race window is extended > beyond the instruction that raises TPR. So we may generate unexpected > spurious interrupts for the guest. And that's because the userspace APIC > fails to update the CPU interrupt pin properly when the TPR is modified. > Here is the bug... Exactly. To avoid dropping the lock we can do something similar to kvm - queue a request to reevaluate IRR, then signal that vcpu to exit. No need to drop the lock. Possibly a check in apic_update_irq() whether we're running in the vcpu thread; if not, signal and return. That function's effects are asynchronous if run outside the vcpu, so it's a safe change. -- error compiling committee.c: too many arguments to function