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

[Jason Wang <jasowang@redhat.com>]

On 10/21/2015 08:20 PM, Leonid Bloch wrote:
> On Tue, Oct 20, 2015 at 9:16 AM, Jason Wang <jasowang@redhat.com> 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 <leonid.bloch@ravellosystems.com>
>>> >> Signed-off-by: Dmitry Fleytman <dmitry.fleytman@ravellosystems.com>
>>> >> ---
>>> >>  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;
>
>> >

Looks good to me.