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 <wp...@windriver.com> >>> Reported-by: Yan Vugenfirer <y...@daynix.com> >>> Signed-off-by: Michael S. Tsirkin <m...@redhat.com> >>> --- >>> 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.
