Re: [Qemu-devel] [PATCH 4/6] e1000: Fixing the received/transmitted octets' counters

[Leonid Bloch]

On Tue, Oct 20, 2015 at 9:16 AM, Jason Wang <address@hidden> wrote:
>
>
> On 10/18/2015 03:53 PM, Leonid Bloch wrote:
>> Previously, the lower parts of these counters (TORL, TOTL) were
>> resetting after reaching their maximal values, and since the continuation
>> of counting in the higher parts (TORH, TOTH) was triggered by an
>> overflow event of the lower parts, the count was not correct.
>>
>> Additionally, TORH and TOTH were counting the corresponding frames, and
>> not the octets, as they supposed to do.
>>
>> Additionally, these 64-bit registers did not stick at their maximal
>> values when (and if) they reached them.
>>
>> This fix resolves all the issues mentioned above, and makes the octet
>> counters behave according to Intel's specs.
>>
>> Signed-off-by: Leonid Bloch <address@hidden>
>> Signed-off-by: Dmitry Fleytman <address@hidden>
>> ---
>>  hw/net/e1000.c | 34 ++++++++++++++++++++++++++--------
>>  1 file changed, 26 insertions(+), 8 deletions(-)
>>
>> diff --git a/hw/net/e1000.c b/hw/net/e1000.c
>> index 5530285..7f977b6 100644
>> --- a/hw/net/e1000.c
>> +++ b/hw/net/e1000.c
>> @@ -583,6 +583,28 @@ inc_reg_if_not_full(E1000State *s, int index)
>>      }
>>  }
>>
>> +static void
>> +grow_8reg_if_not_full(E1000State *s, int index, int size)
>> +{
>> +    uint32_t lo = s->mac_reg[index];
>> +    uint32_t hi = s->mac_reg[index+1];
>> +
>> +    if (lo == 0xffffffff) {
>> +        if ((hi += size) > s->mac_reg[index+1]) {
>> +            s->mac_reg[index+1] = hi;
>> +        } else if (s->mac_reg[index+1] != 0xffffffff) {
>> +            s->mac_reg[index+1] = 0xffffffff;
>> +        }
>> +    } else {
>> +        if (((lo += size) < s->mac_reg[index])
>> +            && (s->mac_reg[index] = 0xffffffff)) {  /* setting low to full 
>> */
>> +            s->mac_reg[index+1] += ++lo;
>> +        } else {
>> +            s->mac_reg[index] = lo;
>> +        }
>> +    }
>> +}
>
> How about something easier:
>
> uint64_t sum = s->mac_reg[index] | (uint64_t)s->mac_reg[index+1] <<32;
> if (sum + size < sum) {
>     sum = 0xffffffffffffffff;
> } else {
>     sum += size;
> }
> s->max_reg[index] = sum;
> s->max_reg[index+1] = sum >> 32;

Yes, that is better! Few small changes:

uint64_t sum = s->mac_reg[index] | (uint64_t)s->mac_reg[index+1] << 32;

if (sum + size < sum) {
    sum = ~0;
} else {
    sum += size;
}
s->mac_reg[index] = sum;
s->mac_reg[index+1] = sum >> 32;

>
>> +
>>  static inline int
>>  vlan_enabled(E1000State *s)
>>  {
>> @@ -632,7 +654,7 @@ static void
>>  xmit_seg(E1000State *s)
>>  {
>>      uint16_t len, *sp;
>> -    unsigned int frames = s->tx.tso_frames, css, sofar, n;
>> +    unsigned int frames = s->tx.tso_frames, css, sofar;
>>      struct e1000_tx *tp = &s->tx;
>>
>>      if (tp->tse && tp->cptse) {
>> @@ -678,10 +700,8 @@ xmit_seg(E1000State *s)
>>      } else
>>          e1000_send_packet(s, tp->data, tp->size);
>>      inc_reg_if_not_full(s, TPT);
>> +    grow_8reg_if_not_full(s, TOTL, s->tx.size);
>>      s->mac_reg[GPTC] = s->mac_reg[TPT];
>> -    n = s->mac_reg[TOTL];
>> -    if ((s->mac_reg[TOTL] += s->tx.size) < n)
>> -        s->mac_reg[TOTH]++;
>>  }
>>
>>  static void
>> @@ -1096,11 +1116,9 @@ e1000_receive_iov(NetClientState *nc, const struct 
>> iovec *iov, int iovcnt)
>>      /* TOR - Total Octets Received:
>>       * This register includes bytes received in a packet from the 
>> <Destination
>>       * Address> field through the <CRC> field, inclusively.
>> +     * Always include FCS length (4) in size.
>>       */
>> -    n = s->mac_reg[TORL] + size + /* Always include FCS length. */ 4;
>> -    if (n < s->mac_reg[TORL])
>> -        s->mac_reg[TORH]++;
>> -    s->mac_reg[TORL] = n;
>> +    grow_8reg_if_not_full(s, TORL, size+4);
>>
>>      n = E1000_ICS_RXT0;
>>      if ((rdt = s->mac_reg[RDT]) < s->mac_reg[RDH])
>