[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] e1000: make ICS write-only
From: |
Michael S. Tsirkin |
Subject: |
Re: [Qemu-devel] [PATCH] e1000: make ICS write-only |
Date: |
Wed, 9 Jan 2013 17:48:57 +0200 |
On Wed, Jan 09, 2013 at 11:36:50PM +0800, Jason Wang wrote:
> 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.
What Bill wrote in the commit log is that he didn't see any guest
reading this.
--
MST
Re: [Qemu-devel] [PATCH] e1000: make ICS write-only, Michael S. Tsirkin, 2013/01/09