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

[Juan Quintela]

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?
- is very rare that we are in the middle of an io cycle?

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.

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.

> +    /* 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:-)

> +
> +    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).

Later, Juan.