Re: [Qemu-devel] [PATCH] e1000: Handle IO Port.

[Peter Maydell <peter.maydell@linaro.org>]

On 27 June 2011 17:34, Anthony PERARD <anthony.perard@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