Re: [Qemu-devel] [PATCH 2/6] e1000: Trivial implementation of various MAC registers

[Leonid Bloch <leonid.bloch@ravellosystems.com>]

On Thu, Oct 22, 2015 at 10:19 AM, Jason Wang <jasowang@redhat.com> 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 <jasowang@redhat.com> 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 <leonid.bloch@ravellosystems.com>
> >>> Signed-off-by: Dmitry Fleytman <dmitry.fleytman@ravellosystems.com>
> >>> ---
> >>>  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?
>
> [...]