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

[Jason Wang]

On 10/22/2015 10:05 PM, Leonid Bloch wrote:
> On Thu, Oct 22, 2015 at 10:19 AM, Jason Wang <address@hidden> 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 <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.
> What can be tested exactly? That the higher 16 bits read as 0 with a
> real card? 

Yes.

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

We should emulate the real card behavior even if it was buggy or
conflicts with spec. We've met several undocumented e1000 behavior in
the past, so we need to be careful to prevent us from debugging such
issue in the future. Leaving a known issue is much better in this case
if we don't know what real card will give us.

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

I'm not sure, so this needs to be tested on real card also.

>> [...]