From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42144) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YAvHK-0007Mg-Hi for qemu-devel@nongnu.org; Tue, 13 Jan 2015 01:48:40 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YAvHG-0001h4-EW for qemu-devel@nongnu.org; Tue, 13 Jan 2015 01:48:38 -0500 Received: from mx1.redhat.com ([209.132.183.28]:38880) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YAvHG-0001go-76 for qemu-devel@nongnu.org; Tue, 13 Jan 2015 01:48:34 -0500 Date: Tue, 13 Jan 2015 06:56:13 +0008 From: Jason Wang Message-Id: <1421131693.4843.1@smtp.corp.redhat.com> In-Reply-To: <87fvbfx179.fsf@weregild.i-did-not-set--mail-host-address--so-tickle-me> References: <1418275427-18921-1-git-send-email-rich.tollerton@ni.com> <1418275427-18921-3-git-send-email-rich.tollerton@ni.com> <1334031842.234534.1418878908279.JavaMail.zimbra@redhat.com> <20150112102401.GB28420@redhat.com> <87fvbfx179.fsf@weregild.i-did-not-set--mail-host-address--so-tickle-me> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Subject: Re: [Qemu-devel] [PATCH v2 2/2] e1000: decrement RDT if equal to RDH List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Richard Tollerton Cc: kraxel@redhat.com, qemu-devel@nongnu.org, Jeff Westfahl , "Michael S. Tsirkin" On Tue, Jan 13, 2015 at 3:12 AM, Richard Tollerton wrote: > "Michael S. Tsirkin" writes: > >> Richard, can you respond please? >> I'd like to see this clarified in code comment or >> commit message before applying this patchset. > > Apologies, and thanks for reminding me. > > On Thu, Dec 18, 2014 at 12:01:48AM -0500, Jason Wang wrote: > >> > Some drivers set RDT=RDH. Oddly, this works on real hardware. To >> work >> > around this, autodecrement RDT when this happens. >> >> Please describe more details on the issue. The spec 3.2.6 said: >> "When >> the head pointer is equal to the tail pointer, the ring is empty." >> So >> RDT=RDH in fact empty the ring. No? > > That is incorrect; the spec explicitly states that RDT=RDH means the > ring is full. The linux e1000 driver more or less implies the same > thing. > > You forgot to include the sentence after that in section 3.2.6 :) > > "When the head pointer is equal to the tail pointer, the ring is > empty. > Hardware stops storing packets in system memory until software > advances > the tail pointer, making more receive buffers available." > > Yeah, this seems really poorly worded to me too. :( You appear to be > interpreting "ring is empty" in the usual sense, i.e. "all N elements > of > the ring buffer are available for use by hardware". In fact, the > correct > interpretation [1] is the exact opposite, "none of the elements are > available for use by hardware". Yes, I do think 'empty' means no available buffer for device to receive :) > The last sentence in the quote makes > this explicit. See also linux e1000 driver sources at [2] [3] [4]. Btw, [2],[3],[4] are all codes that deal with driver's internal variable, not the one that the hardware use. > > > See also [5] which implies that hardware DMA is kicked off by setting > tail != head at initialization. Yes, and we trigger receiving in set_rdt(). > I'm *guessing* (?) that the DMA engine > isn't correspondingly stopped when software sets RDT=RDH, so that once > packets start getting received, Do you mean in qemu? I/O are single threaded, so looks like we are safe. > the hardware can more or less ignore it. > In this context, my patch makes sense. > > (Yes, this is totally an ex-post-facto justification for the patch; it > arrived to me secondhand, and I had not been familiar with the driver > source before now.) True, we've found many undocumented behavior in the past (some even conflicts with spec). I don't have a 82540EM in my hand, but I think the best thing is to check this behavior in real hardware to prevent this patch from breaking many existing drivers. > > [1] http://sourceforge.net/p/e1000/mailman/message/29280078/ This issue mentioned in the thread seems solved. Current e1000_has_rxbufs() will return false if RDT==RDH. > > [2] > http://lxr.free-electrons.com/source/drivers/net/ethernet/intel/e1000/e1000_main.c#L398 > [3] > http://lxr.free-electrons.com/source/drivers/net/ethernet/intel/e1000/e1000.h#L215 > [4] > http://lxr.free-electrons.com/source/drivers/net/ethernet/intel/e1000/e1000_main.c#L4302 > [5] http://sourceforge.net/p/e1000/mailman/message/29969887/ Looks like what mentioned in this thread was also solved. We check both RCTL and e1000_has_rxbufs() in e1000_can_receive(). And flush queued packets in set_rx_control(). > >>> > diff --git a/hw/net/e1000.c b/hw/net/e1000.c >>> > index 44ae3a8..b8cbfc1 100644 >>> > --- a/hw/net/e1000.c >>> > +++ b/hw/net/e1000.c >>> > @@ -1152,6 +1152,12 @@ mac_writereg(E1000State *s, int index, >>> uint32_t val) >>> > static void >>> > set_rdt(E1000State *s, int index, uint32_t val) >>> > { >>> > + if (val == s->mac_reg[RDH]) { /* Decrement RDT if it's >>> too big */ >>> > + if (val == 0) { >>> > + val = s->mac_reg[RDLEN] / sizeof(struct >>> e1000_rx_desc); >>> > + } >>> > + val--; >>> > + } >>> > s->mac_reg[index] = val & 0xffff; >>> > if (e1000_has_rxbufs(s, 1)) { >>> > qemu_flush_queued_packets(qemu_get_queue(s->nic)); >>> > -- >>> > 2.1.3 >>> > >>> > >>> > > > -- > Richard Tollerton >