From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:48668) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TTSLQ-00057U-SU for qemu-devel@nongnu.org; Wed, 31 Oct 2012 03:04:09 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TTSLK-0000qo-Qg for qemu-devel@nongnu.org; Wed, 31 Oct 2012 03:04:08 -0400 Received: from thoth.sbs.de ([192.35.17.2]:29717) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TTSLK-0000qV-Gq for qemu-devel@nongnu.org; Wed, 31 Oct 2012 03:04:02 -0400 Message-ID: <5090CD5A.9050806@siemens.com> Date: Wed, 31 Oct 2012 08:03:54 +0100 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> <50893FD4.3080102@siemens.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit 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-29 06:24, liu ping fan wrote: > On Thu, Oct 25, 2012 at 9:34 PM, Jan Kiszka wrote: >> 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. >> > I think, the major point here is to bypass the delivery process among > the irq chipset during runtime. Right? Right. Jan -- Siemens AG, Corporate Technology, CT RTC ITP SDP-DE Corporate Competence Center Embedded Linux