Re: [Qemu-devel] [PATCH 4/6] e1000: Fixing the received/transmitted octets' counters
On 10/21/2015 08:20 PM, Leonid Bloch wrote:
> On Tue, Oct 20, 2015 at 9:16 AM, Jason Wang 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
>>> >> Signed-off-by: Dmitry Fleytman
>>> >> ---
>>> >> 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 == 0x) {
>>> >> +if ((hi += size) > s->mac_reg[index+1]) {
>>> >> +s->mac_reg[index+1] = hi;
>>> >> +} else if (s->mac_reg[index+1] != 0x) {
>>> >> +s->mac_reg[index+1] = 0x;
>>> >> +}
>>> >> +} else {
>>> >> +if (((lo += size) < s->mac_reg[index])
>>> >> +&& (s->mac_reg[index] = 0x)) { /* 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 = 0x;
>> > } 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.
Re: [Qemu-devel] [PATCH 4/6] e1000: Fixing the received/transmitted octets' counters
On Tue, Oct 20, 2015 at 9:16 AM, Jason Wang 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
>> Signed-off-by: Dmitry Fleytman
>> ---
>> 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 == 0x) {
>> +if ((hi += size) > s->mac_reg[index+1]) {
>> +s->mac_reg[index+1] = hi;
>> +} else if (s->mac_reg[index+1] != 0x) {
>> +s->mac_reg[index+1] = 0x;
>> +}
>> +} else {
>> +if (((lo += size) < s->mac_reg[index])
>> +&& (s->mac_reg[index] = 0x)) { /* 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 = 0x;
> } 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
>> > * Address> field through the 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])
>
Re: [Qemu-devel] [PATCH 4/6] e1000: Fixing the received/transmitted octets' counters
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
> Signed-off-by: Dmitry Fleytman
> ---
> 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 == 0x) {
> +if ((hi += size) > s->mac_reg[index+1]) {
> +s->mac_reg[index+1] = hi;
> +} else if (s->mac_reg[index+1] != 0x) {
> +s->mac_reg[index+1] = 0x;
> +}
> +} else {
> +if (((lo += size) < s->mac_reg[index])
> +&& (s->mac_reg[index] = 0x)) { /* 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 = 0x;
} else {
sum += size;
}
s->max_reg[index] = sum;
s->max_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
>* Address> field through the 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])
[Qemu-devel] [PATCH 4/6] e1000: Fixing the received/transmitted octets' counters
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
Signed-off-by: Dmitry Fleytman
---
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 == 0x) {
+if ((hi += size) > s->mac_reg[index+1]) {
+s->mac_reg[index+1] = hi;
+} else if (s->mac_reg[index+1] != 0x) {
+s->mac_reg[index+1] = 0x;
+}
+} else {
+if (((lo += size) < s->mac_reg[index])
+&& (s->mac_reg[index] = 0x)) { /* setting low to full */
+s->mac_reg[index+1] += ++lo;
+} else {
+s->mac_reg[index] = lo;
+}
+}
+}
+
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 field through the 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])
--
2.4.3
