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

[Leonid Bloch]

Hi Jason, thanks for the review!

On Tue, Oct 20, 2015 at 8:40 AM, Jason Wang <address@hidden> 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.
>
> >
> > 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?
>
> >
> > 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 <address@hidden>
> > Signed-off-by: Dmitry Fleytman <address@hidden>
> > ---
> >  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?
>
> And low4_read() for FFMT?

Why? The specs say nothing about the reserved bits there...
>
> > +    getreg(ECOL),     getreg(MCC),      getreg(LATECOL),  getreg(COLC),
> > +    getreg(DC),       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),
> >
> >      [TOTH] = mac_read_clr8,   [TORH] = mac_read_clr8,
> >      [GPRC] = mac_read_clr4,   [GPTC] = mac_read_clr4,   [TPR] = 
> > mac_read_clr4,
> >      [TPT] = mac_read_clr4,
> >      [ICR] = mac_icr_read,     [EECD] = get_eecd,        [EERD] = 
> > flash_eerd_read,
> > +    [RDFH] = mac_low13_read,  [RDFT] = mac_low13_read,
> > +    [RDFHS] = mac_low13_read, [RDFTS] = mac_low13_read, [RDFPC] = 
> > mac_low13_read,
> > +    [TDFH] = mac_low11_read,  [TDFT] = mac_low11_read,
> > +    [TDFHS] = mac_low13_read, [TDFTS] = mac_low13_read, [TDFPC] = 
> > mac_low13_read,
> >      [CRCERRS ... MPC] = &mac_readreg,
> > +    [IP6AT ... IP6AT+3] = &mac_readreg, [IP4AT ... IP4AT+6] = &mac_readreg,
> > +    [FFLT ... FFLT+6] = &mac_low11_read,
> >      [RA ... RA+31] = &mac_readreg,
> > +    [WUPM ... WUPM+31] = &mac_readreg,
> >      [MTA ... MTA+127] = &mac_readreg,
> >      [VFTA ... VFTA+127] = &mac_readreg,
> > +    [FFMT ... FFMT+254] = &mac_readreg, [FFVT ... FFVT+254] = &mac_readreg,
> > +    [PBM ... PBM+16383] = &mac_readreg,
> >  };
> >  enum { NREADOPS = ARRAY_SIZE(macreg_readops) };
> >
> > @@ -1232,7 +1269,11 @@ enum { NREADOPS = ARRAY_SIZE(macreg_readops) };
> >  static void (*macreg_writeops[])(E1000State *, int, uint32_t) = {
> >      putreg(PBA),      putreg(EERD),     putreg(SWSM),     putreg(WUFC),
> >      putreg(TDBAL),    putreg(TDBAH),    putreg(TXDCTL),   putreg(RDBAH),
> > -    putreg(RDBAL),    putreg(LEDCTL),   putreg(VET),
> > +    putreg(RDBAL),    putreg(LEDCTL),   putreg(VET),      putreg(FCRUC),
> > +    putreg(TDFH),     putreg(TDFT),     putreg(TDFHS),    putreg(TDFTS),
> > +    putreg(TDFPC),    putreg(RDFH),     putreg(RDFT),     putreg(RDFHS),
> > +    putreg(RDFTS),    putreg(RDFPC),    putreg(IPAV),     putreg(WUC),
> > +    putreg(WUS),      putreg(AIT),
> >
> >      [TDLEN] = set_dlen, [RDLEN] = set_dlen,      [TCTL] = set_tctl,
> >      [TDT] = set_tctl,   [MDIC] = set_mdic,       [ICS] = set_ics,
> > @@ -1241,9 +1282,14 @@ static void (*macreg_writeops[])(E1000State *, int, 
> > uint32_t) = {
> >      [EECD] = set_eecd,  [RCTL] = set_rx_control, [CTRL] = set_ctrl,
> >      [RDTR] = set_16bit, [RADV] = set_16bit,      [TADV] = set_16bit,
> >      [ITR] = set_16bit,
> > +    [IP6AT ... IP6AT+3] = &mac_writereg, [IP4AT ... IP4AT+6] = 
> > &mac_writereg,
> > +    [FFLT ... FFLT+6] = &mac_writereg,
> >      [RA ... RA+31] = &mac_writereg,
> > +    [WUPM ... WUPM+31] = &mac_writereg,
> >      [MTA ... MTA+127] = &mac_writereg,
> >      [VFTA ... VFTA+127] = &mac_writereg,
> > +    [FFMT ... FFMT+254] = &mac_writereg, [FFVT ... FFVT+254] = 
> > &mac_writereg,
> > +    [PBM ... PBM+16383] = &mac_writereg,
> >  };
> >
> >  enum { NWRITEOPS = ARRAY_SIZE(macreg_writeops) };
> > diff --git a/hw/net/e1000_regs.h b/hw/net/e1000_regs.h
> > index afd81cc..1c40244 100644
> > --- a/hw/net/e1000_regs.h
> > +++ b/hw/net/e1000_regs.h
> > @@ -158,6 +158,7 @@
> >  #define E1000_PHY_CTRL     0x00F10  /* PHY Control Register in CSR */
> >  #define FEXTNVM_SW_CONFIG  0x0001
> >  #define E1000_PBA      0x01000  /* Packet Buffer Allocation - RW */
> > +#define E1000_PBM      0x10000  /* Packet Buffer Memory - RW */
> >  #define E1000_PBS      0x01008  /* Packet Buffer Size - RW */
> >  #define E1000_EEMNGCTL 0x01010  /* MNG EEprom Control */
> >  #define E1000_FLASH_UPDATES 1000
> > @@ -191,6 +192,11 @@
> >  #define E1000_RAID     0x02C08  /* Receive Ack Interrupt Delay - RW */
> >  #define E1000_TXDMAC   0x03000  /* TX DMA Control - RW */
> >  #define E1000_KABGTXD  0x03004  /* AFE Band Gap Transmit Ref Data */
> > +#define E1000_RDFH     0x02410  /* Receive Data FIFO Head Register - RW */
> > +#define E1000_RDFT     0x02418  /* Receive Data FIFO Tail Register - RW */
> > +#define E1000_RDFHS    0x02420  /* Receive Data FIFO Head Saved Register - 
> > RW */
> > +#define E1000_RDFTS    0x02428  /* Receive Data FIFO Tail Saved Register - 
> > RW */
> > +#define E1000_RDFPC    0x02430  /* Receive Data FIFO Packet Count - RW */
> >  #define E1000_TDFH     0x03410  /* TX Data FIFO Head - RW */
> >  #define E1000_TDFT     0x03418  /* TX Data FIFO Tail - RW */
> >  #define E1000_TDFHS    0x03420  /* TX Data FIFO Head Saved - RW */
>