qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Anthony Liguori
Subject: Re: [Qemu-devel] [PATCH] e1000: make ICS write-only
Date: Wed, 09 Jan 2013 13:50:39 -0600
User-agent: Notmuch/0.13.2+93~ged93d79 (http://notmuchmail.org) Emacs/23.3.1 (x86_64-pc-linux-gnu)

Bill Paul <address@hidden> writes:

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

I figured this would be the case but this is where a comment in the code
or commit message would have helped a lot in avoiding future confusion.

Regards,

Anthony Liguori

>
> -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
> =============================================================================




reply via email to

[Prev in Thread] Current Thread [Next in Thread]