From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:46072) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QbGE3-0006kr-Il for qemu-devel@nongnu.org; Mon, 27 Jun 2011 14:08:00 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QbGE1-0000iG-R7 for qemu-devel@nongnu.org; Mon, 27 Jun 2011 14:07:59 -0400 Received: from mail-pv0-f173.google.com ([74.125.83.173]:52486) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QbGE1-0000i5-AI for qemu-devel@nongnu.org; Mon, 27 Jun 2011 14:07:57 -0400 Received: by pvg3 with SMTP id 3so3147667pvg.4 for ; Mon, 27 Jun 2011 11:07:55 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <1309192483-32468-1-git-send-email-anthony.perard@citrix.com> References: <1309192483-32468-1-git-send-email-anthony.perard@citrix.com> Date: Mon, 27 Jun 2011 19:07:55 +0100 Message-ID: From: Peter Maydell Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH] e1000: Handle IO Port. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Anthony PERARD Cc: QEMU-devel On 27 June 2011 17:34, Anthony PERARD wrote: > @@ -83,6 +86,8 @@ typedef struct E1000State_st { > =C2=A0 =C2=A0 NICState *nic; > =C2=A0 =C2=A0 NICConf conf; > =C2=A0 =C2=A0 int mmio_index; > + =C2=A0 =C2=A0int ioport_base; > + =C2=A0 =C2=A0uint32_t ioport_reg[2]; I think ioport_reg[] needs to go in the VMStateDescription as well. I don't know enough about the PCI subsystem to know whether that's also true of ioport_base or whether the the map function is called again on a vmload. > @@ -202,6 +201,11 @@ rxbufsize(uint32_t v) > =C2=A0static void > =C2=A0set_ctrl(E1000State *s, int index, uint32_t val) > =C2=A0{ > + =C2=A0 =C2=A0DBGOUT(IO, "set ctrl =3D %08x\n", val); > + =C2=A0 =C2=A0if (val & E1000_CTRL_RST) { > + =C2=A0 =C2=A0 =C2=A0 =C2=A0s->mac_reg[CTRL] =3D val; > + =C2=A0 =C2=A0 =C2=A0 =C2=A0e1000_reset(s); > + =C2=A0 =C2=A0} There doesn't seem to be much point in setting mac_reg[CTRL] when e1000_reset() is going to put it to its reset value anyway; and you almost certainly don't want to fall through to this: > =C2=A0 =C2=A0 /* RST is self clearing */ > =C2=A0 =C2=A0 s->mac_reg[CTRL] =3D val & ~E1000_CTRL_RST; ...which means there's no need for that bit to special case RST. > =C2=A0static void > +e1000_ioport_writel(void *opaque, uint32_t addr, uint32_t val) > +{ > + =C2=A0 =C2=A0E1000State *s =3D opaque; > + > + =C2=A0 =C2=A0if (addr =3D=3D s->ioport_base + REG_IOADDR) { > + =C2=A0 =C2=A0 =C2=A0 =C2=A0DBGOUT(IO, "e1000_ioport_writel write base: = 0x%04x\n", val); > + =C2=A0 =C2=A0 =C2=A0 =C2=A0s->ioport_reg[REG_IOADDR] =3D val & 0xfffff; > + =C2=A0 =C2=A0} else if (addr =3D=3D (s->ioport_base + REG_IODATA)) { > + =C2=A0 =C2=A0 =C2=A0 =C2=A0unsigned int index =3D (s->ioport_reg[REG_IO= ADDR] & 0x1ffff) >> 2; > + > + =C2=A0 =C2=A0 =C2=A0 =C2=A0DBGOUT(IO, "e1000_ioport_writel %x: 0x%04x\n= ", index, val); > + > + =C2=A0 =C2=A0 =C2=A0 =C2=A0if (index < NWRITEOPS && macreg_writeops[ind= ex]) { > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0macreg_writeops[index](s, inde= x, val); > + =C2=A0 =C2=A0 =C2=A0 =C2=A0} else if (index < NREADOPS && macreg_readop= s[index]) { > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0DBGOUT(IO, "e1000_ioport_write= l RO %x: 0x%04x\n", index << 2, val); > + =C2=A0 =C2=A0 =C2=A0 =C2=A0} else { > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0DBGOUT(UNKNOWN, "IO unknown wr= ite index=3D0x%08x,val=3D0x%08x\n", > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 index, v= al); > + =C2=A0 =C2=A0 =C2=A0 =C2=A0} This part of this function seems to be duplicating the code in e1000_mmio_writel: wouldn't it be cleaner just to call that function? Ditto readl. -- PMM