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

[Jason Wang]

On 11/04/2015 11:44 PM, Leonid Bloch wrote:
> 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.

I agree, but this is the price of compatibility. (And which can make our
user happy). For performance, I doubt a single condition will cause
noticeable difference.
> 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.

Same as patch 3, reading to those registers will have a zero value.

For the issue of bulky, how about something like this?

- introduce another array mac_regcap[] (which is like phy_regcap).
- store the compat flags required for the function of the registers in
this array, zero means no requirement
- check the enabled compat flag again this in e1000_mmio_write() and
e1000_mmio_read()

And this method would be even useful for future extension for e1000.