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

[Jason Wang]

On 01/09/2013 11:34 PM, Michael S. Tsirkin wrote:
> On Wed, Jan 09, 2013 at 11:28:29PM +0800, Jason Wang wrote:
>> On 01/09/2013 06:51 PM, Michael S. Tsirkin wrote:
>>> 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.
>>>
>>> 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);
>>>  }
>>>  
>> If my memory is correct, though ICS is marked as read only in the spec,
>> we do can read it when I'm examining a real e1000 card.
> Interesting, this was not Bill's motivation.
> I haven't seen any reads with linux or windows guests -
> which guest did trigger them for you?
> Also, what's the value one would expect?
>

I also find this violation of spec in the past, so I just hack the linux
driver to see the result. I'm not sure if there are some driver depends
on this value. I forget the detail of what value it returns, may worth
to try again to see.