From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:32812) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QcM85-0003Ad-In for qemu-devel@nongnu.org; Thu, 30 Jun 2011 14:38:22 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QcM83-0000D7-FI for qemu-devel@nongnu.org; Thu, 30 Jun 2011 14:38:21 -0400 Received: from mail-pv0-f173.google.com ([74.125.83.173]:36907) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QcM83-0000Cv-57 for qemu-devel@nongnu.org; Thu, 30 Jun 2011 14:38:19 -0400 Received: by pvg3 with SMTP id 3so2277474pvg.4 for ; Thu, 30 Jun 2011 11:38:17 -0700 (PDT) MIME-Version: 1.0 Sender: anthony.perard@gmail.com In-Reply-To: References: <1309192483-32468-1-git-send-email-anthony.perard@citrix.com> From: Anthony PERARD Date: Thu, 30 Jun 2011 19:37:47 +0100 Message-ID: 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: Peter Maydell Cc: Anthony PERARD , QEMU-devel On Mon, Jun 27, 2011 at 19:07, Peter Maydell wro= te: > 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. Yes, I will save ioport_reg in the VMStateDescripption. I'm not sure, but I think e1000_ioport_map will be called on resume, so no need to save ioport_base as it will be regenereted. >> @@ -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: Well, it seams to be the beavior of the RST flag, after reading the e1000 doc. It is reset when the device has finished to reset. But if there is no other read possible during the reset, indeed that will not make much sens to set it. >> =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_I= OADDR] & 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[in= dex]) { >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0macreg_writeops[index](s, ind= ex, val); >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0} else if (index < NREADOPS && macreg_reado= ps[index]) { >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0DBGOUT(IO, "e1000_ioport_writ= el 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 w= rite 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, = val); >> + =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. Yes, I will do that. Thanks for the review, --=20 Anthony PERARD