On 27 June 2011 17:34, Anthony PERARD <anthony.per...@citrix.com> wrote: > @@ -83,6 +86,8 @@ typedef struct E1000State_st { > NICState *nic; > NICConf conf; > int mmio_index; > + int ioport_base; > + uint32_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) > static void > set_ctrl(E1000State *s, int index, uint32_t val) > { > + DBGOUT(IO, "set ctrl = %08x\n", val); > + if (val & E1000_CTRL_RST) { > + s->mac_reg[CTRL] = val; > + e1000_reset(s); > + } 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: > /* RST is self clearing */ > s->mac_reg[CTRL] = val & ~E1000_CTRL_RST; ...which means there's no need for that bit to special case RST. > static void > +e1000_ioport_writel(void *opaque, uint32_t addr, uint32_t val) > +{ > + E1000State *s = opaque; > + > + if (addr == s->ioport_base + REG_IOADDR) { > + DBGOUT(IO, "e1000_ioport_writel write base: 0x%04x\n", val); > + s->ioport_reg[REG_IOADDR] = val & 0xfffff; > + } else if (addr == (s->ioport_base + REG_IODATA)) { > + unsigned int index = (s->ioport_reg[REG_IOADDR] & 0x1ffff) >> 2; > + > + DBGOUT(IO, "e1000_ioport_writel %x: 0x%04x\n", index, val); > + > + if (index < NWRITEOPS && macreg_writeops[index]) { > + macreg_writeops[index](s, index, val); > + } else if (index < NREADOPS && macreg_readops[index]) { > + DBGOUT(IO, "e1000_ioport_writel RO %x: 0x%04x\n", index << 2, > val); > + } else { > + DBGOUT(UNKNOWN, "IO unknown write index=0x%08x,val=0x%08x\n", > + index, val); > + } 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