From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:40243) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TswQF-0001Nu-8f for qemu-devel@nongnu.org; Wed, 09 Jan 2013 09:14:29 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TswQD-0004hb-Ox for qemu-devel@nongnu.org; Wed, 09 Jan 2013 09:14:27 -0500 Received: from mail-bk0-f43.google.com ([209.85.214.43]:51674) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TswQD-0004hV-Hn for qemu-devel@nongnu.org; Wed, 09 Jan 2013 09:14:25 -0500 Received: by mail-bk0-f43.google.com with SMTP id jf20so935308bkc.2 for ; Wed, 09 Jan 2013 06:14:24 -0800 (PST) Date: Wed, 9 Jan 2013 15:14:21 +0100 From: Stefan Hajnoczi Message-ID: <20130109141421.GA1565@stefanha-thinkpad.redhat.com> References: <20130109105100.GA17137@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20130109105100.GA17137@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: "Michael S. Tsirkin" Cc: Anthony Liguori , wpaul@windriver.com, Jason Wang , qemu-devel@nongnu.org, Stefan Hajnoczi , yan@daynix.com, Paolo Bonzini On Wed, Jan 09, 2013 at 12:51:00PM +0200, 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); > } Looks good according to datasheet but let's give Bill a chance to review this patch before merging. Stefan