From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:40774) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TQvDn-0002y1-M1 for qemu-devel@nongnu.org; Wed, 24 Oct 2012 03:17:52 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TQvDg-0001fV-Sc for qemu-devel@nongnu.org; Wed, 24 Oct 2012 03:17:47 -0400 Received: from mout.web.de ([212.227.17.11]:50319) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TQvDg-0001f4-Ir for qemu-devel@nongnu.org; Wed, 24 Oct 2012 03:17:40 -0400 Message-ID: <50879606.3070004@web.de> Date: Wed, 24 Oct 2012 09:17:26 +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: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="------------enig6F201237BD5E796F6D99C58C" 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 This is an OpenPGP/MIME signed message (RFC 2440 and 3156) --------------enig6F201237BD5E796F6D99C58C Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable On 2012-10-24 08:31, liu ping fan wrote: > On Tue, Oct 23, 2012 at 5:04 PM, Jan Kiszka wr= ote: >> 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[] =3D { >>> static void >>> set_interrupt_cause(E1000State *s, int index, uint32_t val) >>> { >>> + QemuThread *t; >>> + >>> if (val && (E1000_DEVID >=3D E1000_DEV_ID_82547EI_MOBILE)) { >>> /* Only for 8257x */ >>> val |=3D E1000_ICR_INT_ASSERTED; >>> } >>> s->mac_reg[ICR] =3D val; >>> s->mac_reg[ICS] =3D val; >>> - qemu_set_irq(s->dev.irq[0], (s->mac_reg[IMS] & s->mac_reg[ICR]) = !=3D 0); >>> + >>> + t =3D pthread_getspecific(qemu_thread_key); >>> + if (t->context_type =3D=3D 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[IC= R]) !=3D 0); >>> + } >>> + if (t->context_type =3D=3D 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 registe= r >> content may change while dropping the device lock, no? Then you would >> raise or clear an IRQ spuriously. >> > Device state's intact is protected by busy flag, and will not broken Except that the busy flag concept is broken in itself. I see that we have a all-or-nothing problem here: to address this properly, we need to convert the IRQ path to lock-less (or at least compatible with holding per-device locks) as well. Jan --------------enig6F201237BD5E796F6D99C58C Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.19 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://www.enigmail.net/ iEYEARECAAYFAlCHlgcACgkQitSsb3rl5xQD8gCgmoX2L4dRWeSPExucQHU0xbXJ 8QcAoMGcfUVfyIWRRK64DcPTfTc/4ebI =NQp5 -----END PGP SIGNATURE----- --------------enig6F201237BD5E796F6D99C58C--