On Tue, Jul 19, 2011 at 14:41, Juan Quintela <quint...@redhat.com> wrote:
> Anthony PERARD <anthony.per...@citrix.com> 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 <anthony.per...@citrix.com>
>
> 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
