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

[Juan Quintela]

"Michael S. Tsirkin" <address@hidden> 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 <address@hidden>
> ---
>  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;