From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39541) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZpA9j-0000g2-4U for qemu-devel@nongnu.org; Thu, 22 Oct 2015 03:19:24 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZpA9f-000683-VF for qemu-devel@nongnu.org; Thu, 22 Oct 2015 03:19:23 -0400 Received: from mx1.redhat.com ([209.132.183.28]:45971) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZpA9f-00067q-Ni for qemu-devel@nongnu.org; Thu, 22 Oct 2015 03:19:19 -0400 References: <1445154799-31083-1-git-send-email-leonid.bloch@ravellosystems.com> <1445154799-31083-3-git-send-email-leonid.bloch@ravellosystems.com> <5625D3C1.7010607@redhat.com> From: Jason Wang Message-ID: <56288E11.6020102@redhat.com> Date: Thu, 22 Oct 2015 15:19:45 +0800 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 2/6] e1000: Trivial implementation of various MAC registers 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 10/21/2015 05:13 PM, Leonid Bloch wrote: > Hi Jason, thanks for the review! > > On Tue, Oct 20, 2015 at 8:40 AM, Jason Wang wrote: >> >> >> On 10/18/2015 03:53 PM, Leonid Bloch wrote: >>> These registers appear in Intel's specs, but were not implemented. >>> These registers are now implemented trivially, i.e. they are initiated >>> with zero values, and if they are RW, they can be written or read by the >>> driver, or read only if they are R (essentially retaining their zero >>> values). For these registers no other procedures are performed. >>> >>> The registers implemented here are: >>> >>> Transmit: >>> RW: AIT >>> >>> Management: >>> RW: WUC WUS IPAV* IP6AT* IP4AT* FFLT* WUPM* FFMT* FFVT* >> My version of DSM (Revision) said WUS is read only. > This seems to be a typo in the specs. We also have the specs where it > is said that WUS's read only, but exactly two lines below it - writing > to it is mentioned. Additionally, in the specs for newer Intel's > devices, where the offset and the functionality of WUS are exactly the > same, it is written that WUS is RW. Ok. >>> Diagnostic: >>> RW: RDFH RDFT RDFHS RDFTS RDFPC PBM* >> For those diagnostic register, isn't it better to warn the incomplete >> emulation instead of trying to give all zero values silently? I suspect >> this can make diagnostic software think the device is malfunction? > That's a good point. What do you think about keeping the zero values, > but printing out a warning (via DBGOUT) on each read/write attempt? This is fine for me. >>> Statistic: >>> RW: FCRUC TDFH TDFT TDFHS TDFTS TDFPC >> TDFH TDFT TDFHS TDFTS TDFPC should be Diagnostic? > Yes, they are. Thanks, I'll reword. >>> R: RNBC TSCTFC MGTPRC MGTPDC MGTPTC RFC RJC SCC ECOL >>> LATECOL MCC COLC DC TNCRS SEC CEXTERR RLEC XONRXC >>> XONTXC XOFFRXC XOFFTXC >>> >>> Signed-off-by: Leonid Bloch >>> Signed-off-by: Dmitry Fleytman >>> --- >>> hw/net/e1000.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++++--- >>> hw/net/e1000_regs.h | 6 ++++++ >>> 2 files changed, 55 insertions(+), 3 deletions(-) >>> >>> diff --git a/hw/net/e1000.c b/hw/net/e1000.c >>> index 6d57663..6f754ac 100644 >>> --- a/hw/net/e1000.c >>> +++ b/hw/net/e1000.c >>> @@ -168,7 +168,17 @@ enum { >>> defreg(TPR), defreg(TPT), defreg(TXDCTL), defreg(WUFC), >>> defreg(RA), defreg(MTA), defreg(CRCERRS), defreg(VFTA), >>> defreg(VET), defreg(RDTR), defreg(RADV), defreg(TADV), >>> - defreg(ITR), >>> + defreg(ITR), defreg(FCRUC), defreg(TDFH), defreg(TDFT), >>> + defreg(TDFHS), defreg(TDFTS), defreg(TDFPC), defreg(RDFH), >>> + defreg(RDFT), defreg(RDFHS), defreg(RDFTS), defreg(RDFPC), >>> + defreg(IPAV), defreg(WUC), defreg(WUS), defreg(AIT), >>> + defreg(IP6AT), defreg(IP4AT), defreg(FFLT), defreg(FFMT), >>> + defreg(FFVT), defreg(WUPM), defreg(PBM), defreg(SCC), >>> + defreg(ECOL), defreg(MCC), defreg(LATECOL), defreg(COLC), >>> + defreg(DC), defreg(TNCRS), defreg(SEC), defreg(CEXTERR), >>> + defreg(RLEC), defreg(XONRXC), defreg(XONTXC), defreg(XOFFRXC), >>> + defreg(XOFFTXC), defreg(RFC), defreg(RJC), defreg(RNBC), >>> + defreg(TSCTFC), defreg(MGTPRC), defreg(MGTPDC), defreg(MGTPTC) >>> }; >>> >>> static void >>> @@ -1114,6 +1124,18 @@ mac_readreg(E1000State *s, int index) >>> } >>> >>> static uint32_t >>> +mac_low11_read(E1000State *s, int index) >>> +{ >>> + return s->mac_reg[index] & 0x7ff; >>> +} >>> + >>> +static uint32_t >>> +mac_low13_read(E1000State *s, int index) >>> +{ >>> + return s->mac_reg[index] & 0x1fff; >>> +} >>> + >>> +static uint32_t >>> mac_icr_read(E1000State *s, int index) >>> { >>> uint32_t ret = s->mac_reg[ICR]; >>> @@ -1215,16 +1237,31 @@ static uint32_t (*macreg_readops[])(E1000State *, int) = { >>> getreg(RDH), getreg(RDT), getreg(VET), getreg(ICS), >>> getreg(TDBAL), getreg(TDBAH), getreg(RDBAH), getreg(RDBAL), >>> getreg(TDLEN), getreg(RDLEN), getreg(RDTR), getreg(RADV), >>> - getreg(TADV), getreg(ITR), >>> + getreg(TADV), getreg(ITR), getreg(FCRUC), getreg(IPAV), >>> + getreg(WUC), getreg(WUS), getreg(AIT), getreg(SCC), >> For AIT should we use low16_read() ? > Contrary to registers where lowXX_read() is used, for AIT the specs > say that the higher bits should be written with 0b, and not "read as > 0b". That's my reasoning for that. What do you think? I think it's better to test this behavior on real card. >> And low4_read() for FFMT? > Why? The specs say nothing about the reserved bits there... If I read the spec (Revision 3.7 13.6.10) correctly, only low 4 bits were used for byte mask. [...]