From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:51770) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TQaQK-0004wR-Je for qemu-devel@nongnu.org; Tue, 23 Oct 2012 05:05:30 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TQaQA-0006fx-QU for qemu-devel@nongnu.org; Tue, 23 Oct 2012 05:05:20 -0400 Received: from thoth.sbs.de ([192.35.17.2]:20934) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TQaQA-0006ej-GI for qemu-devel@nongnu.org; Tue, 23 Oct 2012 05:05:10 -0400 Message-ID: <50865DB9.1060106@siemens.com> Date: Tue, 23 Oct 2012 11:04:57 +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> 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" , Avi Kivity , Anthony Liguori , Paolo Bonzini 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). And third: too much boilerplate code... :-/ > } > > static void > @@ -268,6 +283,7 @@ static void e1000_reset(void *opaque) > E1000State *d = opaque; > > qemu_del_timer(d->autoneg_timer); > + > memset(d->phy_reg, 0, sizeof d->phy_reg); > memmove(d->phy_reg, phy_reg_init, sizeof phy_reg_init); > memset(d->mac_reg, 0, sizeof d->mac_reg); > @@ -448,7 +464,11 @@ e1000_send_packet(E1000State *s, const uint8_t *buf, int size) > if (s->phy_reg[PHY_CTRL] & MII_CR_LOOPBACK) { > s->nic->nc.info->receive(&s->nic->nc, buf, size); > } else { > + qemu_mutex_unlock(&s->e1000_lock); > + qemu_mutex_lock_iothread(); > qemu_send_packet(&s->nic->nc, buf, size); > + qemu_mutex_unlock_iothread(); > + qemu_mutex_lock(&s->e1000_lock); And that is the also a problem to be discussed next: How to handle locking of backends? Do we want separate locks for backend and frontend? Although they are typically in a 1:1 relationship? Oh, I'm revealing the content of my talk... ;) > } > } > > @@ -1221,6 +1241,8 @@ static int pci_e1000_init(PCIDevice *pci_dev) > int i; > uint8_t *macaddr; > > + qemu_mutex_init(&d->e1000_lock); > + > pci_conf = d->dev.config; > > /* TODO: RST# value should be 0, PCI spec 6.2.4 */ > Jan -- Siemens AG, Corporate Technology, CT RTC ITP SDP-DE Corporate Competence Center Embedded Linux