From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from [140.186.70.92] (port=47347 helo=eggs.gnu.org)
	by lists.gnu.org with esmtp (Exim 4.43) id 1Osa6W-00080q-Te
	for qemu-devel@nongnu.org; Mon, 06 Sep 2010 07:43:17 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69)
	(envelope-from <mst@redhat.com>) id 1Osa6V-0003PN-LT
	for qemu-devel@nongnu.org; Mon, 06 Sep 2010 07:43:16 -0400
Received: from mx1.redhat.com ([209.132.183.28]:51122)
	by eggs.gnu.org with esmtp (Exim 4.69)
	(envelope-from <mst@redhat.com>) id 1Osa6V-0003PH-F6
	for qemu-devel@nongnu.org; Mon, 06 Sep 2010 07:43:15 -0400
Date: Mon, 6 Sep 2010 14:37:15 +0300
From: "Michael S. Tsirkin" <mst@redhat.com>
Message-ID: <20100906113715.GB16143@redhat.com>
References: <20100905163014.GA23307@redhat.com>
 <m362yjyuso.fsf@trasno.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <m362yjyuso.fsf@trasno.org>
Subject: [Qemu-devel] Re: [PATCH] qemu: e1000 fix TOR math
List-Id: qemu-devel.nongnu.org
List-Unsubscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Juan Quintela <quintela@redhat.com>
Cc: qemu-devel@nongnu.org

On Mon, Sep 06, 2010 at 01:29:27PM +0200, Juan Quintela wrote:
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > Patch b0b900070c7cb29bbefb732ec00397abe5de6d73 made
> > TOR valuer incorrect: the spec says it should always
> > include the CRC field, while size does not include CRC now.
> > No one seems to use TOR field (which is likely why
> > current code works fine), but better to stick to spec.
> >
> > Lightly tested with a linux guest.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  hw/e1000.c |    8 ++++++--
> >  1 files changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/e1000.c b/hw/e1000.c
> > index 80b78bc..eb9faf2 100644
> > --- a/hw/e1000.c
> > +++ b/hw/e1000.c
> > @@ -345,7 +345,7 @@ is_vlan_txd(uint32_t txd_lower)
> >  
> >  /* FCS aka Ethernet CRC-32. We don't get it from backends and can't
> >   * fill it in, just pad descriptor length by 4 bytes unless guest
> > - * told us to trip it off the packet. */
> > + * told us to strip it off the packet. */
> >  static inline int
> >  fcs_len(E1000State *s)
> >  {
> > @@ -690,8 +690,12 @@ e1000_receive(VLANClientState *nc, const uint8_t *buf, size_t size)
> >  
> >      s->mac_reg[GPRC]++;
> >      s->mac_reg[TPR]++;
> > +    /* TOR - Total Octets Received:
> > +     * This register includes bytes received in a packet from the <Destination
> > +     * Address> field through the <CRC> field, inclusively.
> > +     */
> >      n = s->mac_reg[TORL];
> > -    if ((s->mac_reg[TORL] += size) < n)
> > +    if ((s->mac_reg[TORL] += size + 4 /* Always include FCS
> >      length. */) < n)
> 
> once changing this, can we move the assignation out of the if?
> It was already complex enough, but adding a comment inside an if
> condition looks "too much" to me.
> 
> Later, Juan.

Sure, I'll do that.

> >          s->mac_reg[TORH]++;
> >  
> >      n = E1000_ICS_RXT0;