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

[Jason Wang]

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 <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.

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 <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?

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.

[...]