From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:38424) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Tt1fp-0007HT-0v for qemu-devel@nongnu.org; Wed, 09 Jan 2013 14:50:55 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Tt1fn-0006CC-4f for qemu-devel@nongnu.org; Wed, 09 Jan 2013 14:50:52 -0500 Received: from e8.ny.us.ibm.com ([32.97.182.138]:40945) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Tt1fn-0006C6-0k for qemu-devel@nongnu.org; Wed, 09 Jan 2013 14:50:51 -0500 Received: from /spool/local by e8.ny.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 9 Jan 2013 14:50:50 -0500 Received: from d01relay03.pok.ibm.com (d01relay03.pok.ibm.com [9.56.227.235]) by d01dlp03.pok.ibm.com (Postfix) with ESMTP id 1A224C90045 for ; Wed, 9 Jan 2013 14:50:44 -0500 (EST) Received: from d01av03.pok.ibm.com (d01av03.pok.ibm.com [9.56.224.217]) by d01relay03.pok.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id r09JohNb306466 for ; Wed, 9 Jan 2013 14:50:43 -0500 Received: from d01av03.pok.ibm.com (loopback [127.0.0.1]) by d01av03.pok.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id r09JohYf003712 for ; Wed, 9 Jan 2013 17:50:43 -0200 From: Anthony Liguori In-Reply-To: <201301090930.44035.wpaul@windriver.com> References: <20130109105100.GA17137@redhat.com> <201301090930.44035.wpaul@windriver.com> Date: Wed, 09 Jan 2013 13:50:39 -0600 Message-ID: <8762366sq8.fsf@codemonkey.ws> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Subject: Re: [Qemu-devel] [PATCH] e1000: make ICS write-only List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Bill Paul , "Michael S. Tsirkin" Cc: yan@daynix.com, Paolo Bonzini , Jason Wang , qemu-devel@nongnu.org, Stefan Hajnoczi Bill Paul writes: > Of all the gin joints in all the towns in all the world, Michael S. Tsirkin > had to walk into mine at 02:51:00 on Wednesday 09 January 2013 and say: > >> 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. > > You are wrong. > > I know what the spec says. The spec lies (or at the very least, it doesn't > agree with reality). With actual PRO/1000 hardware, the ICS register is > readable. It provides a way to obtain the currently pending interrupt bits > without the auto-clear behavior of the ICR register (which in some cases is > actually detrimental). > > The Intel 10GbE NICs (82598, 82599, x540) are the same way (they're very > similar in design to the PRO/1000s, particularly the 82575, 82576, 82580 and > later devices). The spes for the 10GbE devices _also_ say that ICS is read > only. But if you look at the Intel drivers for those chips, you'll see they > have actually implemented a workaround for a device errata (I think the for > the 82598) that actually requires reading the ICS register. If you had > implemented a PRO/10GbE virtual device based on the same chip and obeyed the > spec the same way, I suspect Intel's driver would break. > > I actually have in my possession real PRO/1000 silicon going all the way back > to the 82543 and have tested every single one of them, and they all work like > this. I've also asked the Intel LAN access division people about it and they > said that in spite of it not being documented as readable, there's nothing > particularly wrong with doing this. > > The problem with using ICR is that the auto-clear behavior can sometimes > result in some awkward interrupt handling code. You need to test ICR in > interrupt context to see if there are events pending, and then schedule some > other thread to handle them. But since reading ICR clears the bits, you need > to save the events somewhere so that the other thread knows what events need > servicing. Keeping the saved copy of pending events properly synchronized so > that you don't lose any is actually big challenge. The VxWorks PRO/1000 driver > used to have some very ugly code in it to deal with it, all of which became > much simpler when using the ICS register instead. > > I know what the spec says. But this is a case where the spec only says that > because Intel decided not to disclose what the hardware actually does. They > also don't tell you about all the hidden debug registers in the hardware > either, but that doesn't mean they don't exist. > > So pretty please, with sugar on top, leave this code alone. I figured this would be the case but this is where a comment in the code or commit message would have helped a lot in avoiding future confusion. Regards, Anthony Liguori > > -Bill > >> 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); >> } > > -- > ============================================================================= > -Bill Paul (510) 749-2329 | Member of Technical Staff, > wpaul@windriver.com | Master of Unix-Fu - Wind River Systems > ============================================================================= > "I put a dollar in a change machine. Nothing changed." - George Carlin > =============================================================================