From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58204) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZuB2x-0003JY-3g for qemu-devel@nongnu.org; Wed, 04 Nov 2015 22:17:08 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZuB2t-0001bT-Rq for qemu-devel@nongnu.org; Wed, 04 Nov 2015 22:17:07 -0500 Received: from mx1.redhat.com ([209.132.183.28]:56731) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZuB2t-0001bL-K2 for qemu-devel@nongnu.org; Wed, 04 Nov 2015 22:17:03 -0500 References: <1446549255-26172-1-git-send-email-leonid.bloch@ravellosystems.com> <1446549255-26172-8-git-send-email-leonid.bloch@ravellosystems.com> <56397193.3020004@redhat.com> From: Jason Wang Message-ID: <563ACA2B.4060704@redhat.com> Date: Thu, 5 Nov 2015 11:16:59 +0800 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v4 7/7] e1000: Implementing various counters List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Leonid Bloch Cc: Dmitry Fleytman , Leonid Bloch , qemu-devel@nongnu.org, Shmulik Ladkani On 11/04/2015 11:44 PM, Leonid Bloch wrote: > On Wed, Nov 4, 2015 at 4:46 AM, Jason Wang 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 >>> Signed-off-by: Dmitry Fleytman >>> --- >>> 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.