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