From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:45469) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TQFOv-0006vr-L4 for qemu-devel@nongnu.org; Mon, 22 Oct 2012 06:38:33 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TQFOu-0002Pv-Fu for qemu-devel@nongnu.org; Mon, 22 Oct 2012 06:38:29 -0400 Received: from mx1.redhat.com ([209.132.183.28]:64327) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TQFOu-0002Pn-5k for qemu-devel@nongnu.org; Mon, 22 Oct 2012 06:38:28 -0400 Message-ID: <50852204.2060004@redhat.com> Date: Mon, 22 Oct 2012 12:37:56 +0200 From: Avi Kivity 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> In-Reply-To: <1350897839-29593-13-git-send-email-pingfank@linux.vnet.ibm.com> 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: Stefan Hajnoczi , Marcelo Tosatti , qemu-devel@nongnu.org, Anthony Liguori , Jan Kiszka , Paolo Bonzini On 10/22/2012 11:23 AM, 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; Can call it '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 way too complicated for device model authors. There's no way to get it correct. If mmio dispatch needs to call a non-thread-safe subsystem, it must acquire the big lock: Something like e1000_mmio_read() { if (index < NREADOPS && macreg_readops[index]){ macreg_lockops[index].lock(s); ret = macreg_readops[index](s, index); macreg_lockops[index].unlock(s); } DBGOUT(UNKNOWN, "MMIO unknown read addr=0x%08x\n", index<<2); } Where .lock() either locks just the local lock, or both locks. As subsystems are converted to be thread safe, we can remove this. -- error compiling committee.c: too many arguments to function