[Qemu-devel] [PATCH] qemu: e1000 fix TOR math

[Qemu-devel] [PATCH] qemu: e1000 fix TOR math

[Michael S. Tsirkin]

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)
         s->mac_reg[TORH]++;
 
     n = E1000_ICS_RXT0;
-- 
1.7.2.rc0.14.g41c1c

[Qemu-devel] Re: [PATCH] qemu: e1000 fix TOR math

[Juan Quintela]

"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.

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

[Qemu-devel] Re: [PATCH] qemu: e1000 fix TOR math

[Michael S. Tsirkin]

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;