From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36296) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZpGV6-0005Z2-QV for qemu-devel@nongnu.org; Thu, 22 Oct 2015 10:05:57 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZpGV5-0000oj-8Y for qemu-devel@nongnu.org; Thu, 22 Oct 2015 10:05:52 -0400 Received: from mail-lf0-x236.google.com ([2a00:1450:4010:c07::236]:35531) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZpGV4-0000oR-Pe for qemu-devel@nongnu.org; Thu, 22 Oct 2015 10:05:51 -0400 Received: by lfbn126 with SMTP id n126so14915870lfb.2 for ; Thu, 22 Oct 2015 07:05:49 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <56288E11.6020102@redhat.com> 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> <56288E11.6020102@redhat.com> Date: Thu, 22 Oct 2015 17:05:49 +0300 Message-ID: From: Leonid Bloch Content-Type: text/plain; charset=UTF-8 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: Jason Wang Cc: Dmitry Fleytman , Leonid Bloch , qemu-devel@nongnu.org, Shmulik Ladkani On Thu, Oct 22, 2015 at 10:19 AM, Jason Wang wrote: > > > > 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. What can be tested exactly? That the higher 16 bits read as 0 with a real card? All the specs say is that these bits should be written with 0 (which the Linux driver, at least, indeed complies to). There is no requirement anywhere for these bits to be read as 0, regardless of their actual values. > > >> 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. Yes, only the lower 4 bits are used. But why the others need to be read as 0? > > [...]