From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:60992) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TsxqG-0002CW-5M for qemu-devel@nongnu.org; Wed, 09 Jan 2013 10:45:33 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TsxqC-0004TO-Nj for qemu-devel@nongnu.org; Wed, 09 Jan 2013 10:45:24 -0500 Received: from mx1.redhat.com ([209.132.183.28]:18361) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TsxqC-0004T9-F7 for qemu-devel@nongnu.org; Wed, 09 Jan 2013 10:45:20 -0500 Date: Wed, 9 Jan 2013 17:48:57 +0200 From: "Michael S. Tsirkin" Message-ID: <20130109154857.GA7968@redhat.com> References: <20130109105100.GA17137@redhat.com> <50ED8C9D.1060109@redhat.com> <20130109153407.GB7726@redhat.com> <50ED8E92.3080501@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <50ED8E92.3080501@redhat.com> Subject: Re: [Qemu-devel] [PATCH] e1000: make ICS write-only List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jason Wang Cc: Anthony Liguori , wpaul@windriver.com, qemu-devel@nongnu.org, Stefan Hajnoczi , yan@daynix.com, Paolo Bonzini 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 > >>> Reported-by: Yan Vugenfirer > >>> Signed-off-by: Michael S. Tsirkin > >>> --- > >>> 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