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

[Anthony PERARD <anthony.perard@citrix.com>]

On Mon, Jun 27, 2011 at 19:07, Peter Maydell <peter.maydell@linaro.org> wrote:
> 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.

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)
>>  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:

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.

>>     /* 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.

Yes, I  will do that.

Thanks for the review,

-- 
Anthony PERARD