From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:34722) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TRNZt-0002yP-Nf for qemu-devel@nongnu.org; Thu, 25 Oct 2012 09:34:30 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TRNZs-0002Hb-5o for qemu-devel@nongnu.org; Thu, 25 Oct 2012 09:34:29 -0400 Received: from goliath.siemens.de ([192.35.17.28]:17195) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TRNZr-0002HK-Rw for qemu-devel@nongnu.org; Thu, 25 Oct 2012 09:34:28 -0400 Message-ID: <50893FD4.3080102@siemens.com> Date: Thu, 25 Oct 2012 15:34:12 +0200 From: Jan Kiszka MIME-Version: 1.0 References: <1350897839-29593-1-git-send-email-pingfank@linux.vnet.ibm.com> <1350897839-29593-13-git-send-email-pingfank@linux.vnet.ibm.com> <50865DB9.1060106@siemens.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [patch v4 12/16] e1000: apply fine lock on e1000 List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: liu ping fan Cc: Liu Ping Fan , Stefan Hajnoczi , Marcelo Tosatti , "qemu-devel@nongnu.org" , Avi Kivity , Anthony Liguori , Paolo Bonzini On 2012-10-24 09:29, liu ping fan wrote: > On Tue, Oct 23, 2012 at 5:04 PM, Jan Kiszka wrote: >> On 2012-10-22 11:23, Liu Ping Fan wrote: >>> Use local lock to protect e1000. When calling the system function, >>> dropping the fine lock before acquiring the big lock. This will >>> introduce broken device state, which need extra effort to fix. >>> >>> Signed-off-by: Liu Ping Fan >>> --- >>> hw/e1000.c | 24 +++++++++++++++++++++++- >>> 1 files changed, 23 insertions(+), 1 deletions(-) >>> >>> diff --git a/hw/e1000.c b/hw/e1000.c >>> index ae8a6c5..5eddab5 100644 >>> --- a/hw/e1000.c >>> +++ b/hw/e1000.c >>> @@ -85,6 +85,7 @@ typedef struct E1000State_st { >>> NICConf conf; >>> MemoryRegion mmio; >>> MemoryRegion io; >>> + QemuMutex e1000_lock; >>> >>> uint32_t mac_reg[0x8000]; >>> uint16_t phy_reg[0x20]; >>> @@ -223,13 +224,27 @@ static const uint32_t mac_reg_init[] = { >>> static void >>> set_interrupt_cause(E1000State *s, int index, uint32_t val) >>> { >>> + QemuThread *t; >>> + >>> if (val && (E1000_DEVID >= E1000_DEV_ID_82547EI_MOBILE)) { >>> /* Only for 8257x */ >>> val |= E1000_ICR_INT_ASSERTED; >>> } >>> s->mac_reg[ICR] = val; >>> s->mac_reg[ICS] = val; >>> - qemu_set_irq(s->dev.irq[0], (s->mac_reg[IMS] & s->mac_reg[ICR]) != 0); >>> + >>> + t = pthread_getspecific(qemu_thread_key); >>> + if (t->context_type == 1) { >>> + qemu_mutex_unlock(&s->e1000_lock); >>> + qemu_mutex_lock_iothread(); >>> + } >>> + if (DEVICE(s)->state < DEV_STATE_STOPPING) { >>> + qemu_set_irq(s->dev.irq[0], (s->mac_reg[IMS] & s->mac_reg[ICR]) != 0); >>> + } >>> + if (t->context_type == 1) { >>> + qemu_mutex_unlock_iothread(); >>> + qemu_mutex_lock(&s->e1000_lock); >>> + } >> >> This is ugly for many reasons. First of all, it is racy as the register >> content may change while dropping the device lock, no? Then you would >> raise or clear an IRQ spuriously. >> >> Second, it clearly shows that we need to address lock-less IRQ delivery. >> Almost nothing is won if we have to take the global lock again to push >> an IRQ event to the guest. I'm repeating myself, but the problem to be >> solved here is almost identical to fast IRQ delivery for assigned >> devices (which we only address pretty ad-hoc for PCI so far). >> > Interesting, could you show me more detail about it, so I can google... No need to look that far, just grep for pci_device_route_intx_to_irq, pci_device_set_intx_routing_notifier and related functions in the code. Jan -- Siemens AG, Corporate Technology, CT RTC ITP SDP-DE Corporate Competence Center Embedded Linux