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

[Anthony PERARD]

On Tue, Jul 19, 2011 at 14:41, Juan Quintela <address@hidden> wrote:
> Anthony PERARD <address@hidden> wrote:
>> This patch introduces the two IOPorts on e1000, IOADDR and IODATA. The
>> IOADDR is used to specify which register we want to access when we read
>> or write on IODATA.
>>
>> This patch fixes some weird behavior that I see when I use e1000 with
>> QEMU/Xen, the guest memory can be corrupted by this NIC because it will
>> write on memory that it doesn't own anymore after a reset. It's because
>> the kernel Linux use the IOPort to reset the network card instead of the
>> MMIO.
>>
>> Signed-off-by: Anthony PERARD <address@hidden>
>
> This "used" to work, so the question is:
> - do ioport_addr normally has a value of 0, and then migration works?

0 is the reset value. Otherwise, it's the last value write to the register.

> - is very rare that we are in the middle of an io cycle?

I'm not sure this will answer to your question, but I have only see
the use of these IO port in the kernel module once. And it's used to
reset the device. (So just 2 write, together).

> To be able to use a subsection, we have to had a way to decide that the
> old default value is going to go.  My understanding is that testing for
> ->ioport_addr == 0 should be the test for a subsection, but the code
> never looks to put ioport_addr back to zero.
>
> I am missing anything obvious?  Or is there any easy way to now if we
> are in the middle of a couple of io operations?  For my reading of
> e100_ioport_read/writel() it looks like it should be used as:
>
> write(base+IOADDR)
> write(base+IODATA)
>
> but, should this always be paired, and we can reset ioport_addr after
> the second?  Then just setting ioport_addr to zero after the second
> would made the subsection work in the normal case.

The value in ioport_addr is "retained until the next write or reset",
like said the documentation. So I suppose we do not have to pair the
two IO writes, and we can not reset the ioport_addr after a read/write
to ioport_data.

> Any other clue about _when_ we should send ioport_addr?
>
> Thanks, Juan.
>> @@ -202,8 +201,12 @@ rxbufsize(uint32_t v)
>>  static void
>>  set_ctrl(E1000State *s, int index, uint32_t val)
>>  {
>> -    /* RST is self clearing */
>> -    s->mac_reg[CTRL] = val & ~E1000_CTRL_RST;
>> +    DBGOUT(IO, "set ctrl = %08x\n", val);
>> +    if (val & E1000_CTRL_RST) {
>> +        e1000_reset(s);
>> +        return;
>> +    }
>> +    s->mac_reg[CTRL] = val;
>>  }
>
>
> This looks to me as a different fix that can go in a different patch.

OK, I will create another patch with this fix.

>> +    /* Writes that are less than 32 bits are ignored on IOADDR.
>> +     * For the Flash access, a write can be less than 32 bits for
>> +     * IODATA register, but is not handled.
>> +     */
>
> Code to implement it is almost the same lenght that this O:-)

:), fine, will add the code.

>> +
>> +    register_ioport_read(addr, size, 1, e1000_ioport_readl, d);
>> +
>> +    register_ioport_read(addr, size, 2, e1000_ioport_readl, d);
>> +
>> +    register_ioport_write(addr, size, 4, e1000_ioport_writel, d);
>> +    register_ioport_read(addr, size, 4, e1000_ioport_readl, d);
>
> This is curiosity on my part.  Are we returinng 32bits reads for 1,2 and
> 4 bytes reads, or there is code at some other level that drops the bits
> that we are not interested into?  My understanding of iport.c is that
> this is not checked done (it is more, but I don't claim to fully
> understand it, or if it mattres at all).

Well, the e1000 doc said to return a DWORD for a read of all size, and
the CPU/chipset will return only a subset of that dword. So I'm not
sure if I have to handle it here or not. But as the ioport code in
qemu will not be aware of what it will be returned by the function,
maybe I have to actually do the "convertion" here and return only the
subset of the register.

Thanks for the review,
Regards,

-- 
Anthony PERARD