Re: [Qemu-devel] [PATCH v4 7/7] e1000: Implementing various counters

[Leonid Bloch]

On Wed, Nov 4, 2015 at 4:46 AM, Jason Wang <address@hidden> wrote:
>
>
> On 11/03/2015 07:14 PM, Leonid Bloch wrote:
>> This implements the following Statistic registers (various counters)
>> according to Intel's specs:
>>
>> TSCTC  GOTCL  GOTCH  GORCL  GORCH  MPRC   BPRC   RUC    ROC
>> BPTC   MPTC   PTC... PRC...
>>
>> Signed-off-by: Leonid Bloch <address@hidden>
>> Signed-off-by: Dmitry Fleytman <address@hidden>
>> ---
>>  hw/net/e1000.c | 78 
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++----
>>  1 file changed, 73 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/net/e1000.c b/hw/net/e1000.c
>> index af97e8a..fbda0d1 100644
>> --- a/hw/net/e1000.c
>> +++ b/hw/net/e1000.c
>> @@ -37,6 +37,8 @@
>>
>>  #include "e1000_regs.h"
>>
>
> [...]
>
>>
>> @@ -1111,6 +1164,7 @@ e1000_receive_iov(NetClientState *nc, const struct 
>> iovec *iov, int iovcnt)
>>          }
>>      } while (desc_offset < total_size);
>>
>> +    increase_size_stats(s, PRCregs, total_size);
>>      inc_reg_if_not_full(s, TPR);
>>      s->mac_reg[GPRC] = s->mac_reg[TPR];
>>      /* TOR - Total Octets Received:
>> @@ -1119,6 +1173,8 @@ e1000_receive_iov(NetClientState *nc, const struct 
>> iovec *iov, int iovcnt)
>>       * Always include FCS length (4) in size.
>>       */
>>      grow_8reg_if_not_full(s, TORL, size+4);
>> +    s->mac_reg[GORCL] = s->mac_reg[TORL];
>> +    s->mac_reg[GORCH] = s->mac_reg[TORH];
>>
>>      n = E1000_ICS_RXT0;
>>      if ((rdt = s->mac_reg[RDT]) < s->mac_reg[RDH])
>> @@ -1307,11 +1363,23 @@ static uint32_t (*macreg_readops[])(E1000State *, 
>> int) = {
>>      getreg(TNCRS),    getreg(SEC),      getreg(CEXTERR),  getreg(RLEC),
>>      getreg(XONRXC),   getreg(XONTXC),   getreg(XOFFRXC),  getreg(XOFFTXC),
>>      getreg(RFC),      getreg(RJC),      getreg(RNBC),     getreg(TSCTFC),
>> -    getreg(MGTPRC),   getreg(MGTPDC),   getreg(MGTPTC),
>> +    getreg(MGTPRC),   getreg(MGTPDC),   getreg(MGTPTC),   getreg(GORCL),
>> +    getreg(GOTCL),
>>
>>      [TOTH]    = mac_read_clr8,      [TORH]    = mac_read_clr8,
>> +    [GOTCH]   = mac_read_clr8,      [GORCH]   = mac_read_clr8,
>> +    [PRC64]   = mac_read_clr4,      [PRC127]  = mac_read_clr4,
>> +    [PRC255]  = mac_read_clr4,      [PRC511]  = mac_read_clr4,
>> +    [PRC1023] = mac_read_clr4,      [PRC1522] = mac_read_clr4,
>> +    [PTC64]   = mac_read_clr4,      [PTC127]  = mac_read_clr4,
>> +    [PTC255]  = mac_read_clr4,      [PTC511]  = mac_read_clr4,
>> +    [PTC1023] = mac_read_clr4,      [PTC1522] = mac_read_clr4,
>>      [GPRC]    = mac_read_clr4,      [GPTC]    = mac_read_clr4,
>>      [TPT]     = mac_read_clr4,      [TPR]     = mac_read_clr4,
>> +    [RUC]     = mac_read_clr4,      [ROC]     = mac_read_clr4,
>> +    [BPRC]    = mac_read_clr4,      [MPRC]    = mac_read_clr4,
>> +    [TSCTC]   = mac_read_clr4,      [BPTC]    = mac_read_clr4,
>> +    [MPTC]    = mac_read_clr4,
>>      [ICR]     = mac_icr_read,       [EECD]    = get_eecd,
>>      [EERD]    = flash_eerd_read,
>>      [RDFH]    = mac_low13_read_prt, [RDFT]    = mac_low13_read_prt,
>
> Same issue with patch 3. Need limit the function of those registers
> works only for 2.5 and post 2.5.

Contrary to the registers in patch 3, these registers do have a
functionality in the device - they are counters. But they have a
simple logic and they are distributed throughout the code.
Contrary to that, for example, the registers that were implemented in
the patch that added interrupt mitigation support (e9845f098), are
concentrated in a single location, and a single "if" statement checks
for a flag: if it is set, some non-trivial logic begins (which is
skipped if these registers are not needed anyway).
In the current case, some 10 "if"s are needed to enable a simple logic
in many places. That may influence performance, and will certainly
make the code more bulky.
On the other hand, if these registers will function always, absolutely
no harm will be done if migrating to an older version: these registers
will simply be inaccessible, as they were so far.