Re: [Qemu-devel] [PATCH] e1000: make ICS write-only

[Bill Paul]

Of all the gin joints in all the towns in all the world, Michael S. Tsirkin 
had to walk into mine at 02:51:00 on Wednesday 09 January 2013 and say:

> Since commit b1332393cdd7d023de8f1f8aa136ee7866a18968,
> qemu started updating ICS register when interrupt
> is sent, with the intent to match spec better
> (guests do not actually read this register).
> However, the function set_interrupt_cause where ICS
> is updated is often called internally by
> device emulation so reading it does not produce the last value
> written by driver.  Looking closer at the spec,
> it documents ICS as write-only, so there's no need
> to update it at all. I conclude that while harmless this line is useless
> code so removing it is a bit cleaner than keeping it in.

You are wrong.

I know what the spec says. The spec lies (or at the very least, it doesn't 
agree with reality). With actual PRO/1000 hardware, the ICS register is 
readable. It provides a way to obtain the currently pending interrupt bits 
without the auto-clear behavior of the ICR register (which in some cases is 
actually detrimental).

The Intel 10GbE NICs (82598, 82599, x540) are the same way (they're very 
similar in design to the PRO/1000s, particularly the 82575, 82576, 82580 and 
later devices). The spes for the 10GbE devices _also_ say that ICS is read 
only. But if you look at the Intel drivers for those chips, you'll see they 
have actually implemented a workaround for a device errata (I think the for 
the 82598) that actually requires reading the ICS register. If you had 
implemented a PRO/10GbE virtual device based on the same chip and obeyed the 
spec the same way, I suspect Intel's driver would break.

I actually have in my possession real PRO/1000 silicon going all the way back 
to the 82543 and have tested every single one of them, and they all work like 
this. I've also asked the Intel LAN access division people about it and they 
said that in spite of it not being documented as readable, there's nothing 
particularly wrong with doing this.

The problem with using ICR is that the auto-clear behavior can sometimes 
result in some awkward interrupt handling code. You need to test ICR in 
interrupt context to see if there are events pending, and then schedule some 
other thread to handle them. But since reading ICR clears the bits, you need 
to save the events somewhere so that the other thread knows what events need 
servicing. Keeping the saved copy of pending events properly synchronized so 
that you don't lose any is actually big challenge. The VxWorks PRO/1000 driver 
used to have some very ugly code in it to deal with it, all of which became 
much simpler when using the ICS register instead.

I know what the spec says. But this is a case where the spec only says that 
because Intel decided not to disclose what the hardware actually does. They 
also don't tell you about all the hidden debug registers in the hardware 
either, but that doesn't mean they don't exist.

So pretty please, with sugar on top, leave this code alone.

-Bill

> Tested with windows and linux guests.
> 
> Cc: Bill Paul <address@hidden>
> Reported-by: Yan Vugenfirer <address@hidden>
> Signed-off-by: Michael S. Tsirkin <address@hidden>
> ---
>  hw/e1000.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/hw/e1000.c b/hw/e1000.c
> index 92fb00a..928d804 100644
> --- a/hw/e1000.c
> +++ b/hw/e1000.c
> @@ -230,7 +230,6 @@ set_interrupt_cause(E1000State *s, int index, uint32_t
> val) val |= E1000_ICR_INT_ASSERTED;
>      }
>      s->mac_reg[ICR] = val;
> -    s->mac_reg[ICS] = val;
>      qemu_set_irq(s->dev.irq[0], (s->mac_reg[IMS] & s->mac_reg[ICR]) != 0);
>  }

-- 
=============================================================================
-Bill Paul            (510) 749-2329 | Member of Technical Staff,
                 address@hidden | Master of Unix-Fu - Wind River Systems
=============================================================================
   "I put a dollar in a change machine. Nothing changed." - George Carlin
=============================================================================