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
