Re: [Qemu-devel] [PATCH v4 3/7] e1000: Trivial implementation of various MAC registers

[Jason Wang]

On 11/04/2015 11:21 PM, Leonid Bloch wrote:
> On Wed, Nov 4, 2015 at 4:44 AM, Jason Wang <address@hidden> wrote:
>>
>> On 11/03/2015 07:14 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.
>>>
>>> For the trivially implemented Diagnostic registers, a debug warning is
>>> produced on read/write attempts.
>>>
>>> The registers implemented here are:
>>>
>>> Transmit:
>>> RW: AIT
>>>
>>> Management:
>>> RW: WUC     WUS     IPAV    IP6AT*  IP4AT*  FFLT*   WUPM*   FFMT*   FFVT*
>>>
>>> Diagnostic:
>>> RW: RDFH    RDFT    RDFHS   RDFTS   RDFPC   PBM*    TDFH    TDFT    TDFHS
>>>     TDFTS   TDFPC
>>>
>>> Statistic:
>>> RW: FCRUC
>>> 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      | 95 
>>> +++++++++++++++++++++++++++++++++++++++++++++++++++--
>>>  hw/net/e1000_regs.h |  6 ++++
>>>  2 files changed, 98 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/hw/net/e1000.c b/hw/net/e1000.c
>>> index 1190bbe..7db6614 100644
>>> --- a/hw/net/e1000.c
>>> +++ b/hw/net/e1000.c
>>> @@ -170,7 +170,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
>>> @@ -1118,6 +1128,48 @@ mac_readreg(E1000State *s, int index)
>>>  }
>>>
>>>  static uint32_t
>>> +mac_readreg_prt(E1000State *s, int index)
>>> +{
>>> +    DBGOUT(GENERAL, "Reading register at offset: 0x%08x. "
>>> +           "It is not fully implemented.\n", index<<2);
>>> +    return s->mac_reg[index];
>>> +}
>>> +
>>> +static uint32_t
>>> +mac_low4_read(E1000State *s, int index)
>>> +{
>>> +    return s->mac_reg[index] & 0xf;
>>> +}
>>> +
>>> +static uint32_t
>>> +mac_low11_read(E1000State *s, int index)
>>> +{
>>> +    return s->mac_reg[index] & 0x7ff;
>>> +}
>>> +
>>> +static uint32_t
>>> +mac_low11_read_prt(E1000State *s, int index)
>>> +{
>>> +    DBGOUT(GENERAL, "Reading register at offset: 0x%08x. "
>>> +           "It is not fully implemented.\n", index<<2);
>>> +    return s->mac_reg[index] & 0x7ff;
>>> +}
>>> +
>>> +static uint32_t
>>> +mac_low13_read_prt(E1000State *s, int index)
>>> +{
>>> +    DBGOUT(GENERAL, "Reading register at offset: 0x%08x. "
>>> +           "It is not fully implemented.\n", index<<2);
>>> +    return s->mac_reg[index] & 0x1fff;
>>> +}
>>> +
>>> +static uint32_t
>>> +mac_low16_read(E1000State *s, int index)
>>> +{
>>> +    return s->mac_reg[index] & 0xffff;
>>> +}
>>> +
>>> +static uint32_t
>>>  mac_icr_read(E1000State *s, int index)
>>>  {
>>>      uint32_t ret = s->mac_reg[ICR];
>>> @@ -1161,6 +1213,14 @@ mac_writereg(E1000State *s, int index, uint32_t val)
>>>  }
>>>
>>>  static void
>>> +mac_writereg_prt(E1000State *s, int index, uint32_t val)
>>> +{
>>> +    DBGOUT(GENERAL, "Writing to register at offset: 0x%08x. "
>>> +           "It is not fully implemented.\n", index<<2);
>>> +    s->mac_reg[index] = val;
>>> +}
>>> +
>>> +static void
>>>  set_rdt(E1000State *s, int index, uint32_t val)
>>>  {
>>>      s->mac_reg[index] = val & 0xffff;
>>> @@ -1219,26 +1279,50 @@ 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(SCC),      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,
>>>      [TPT]     = mac_read_clr4,      [TPR]     = mac_read_clr4,
>>>      [ICR]     = mac_icr_read,       [EECD]    = get_eecd,
>>>      [EERD]    = flash_eerd_read,
>>> +    [RDFH]    = mac_low13_read_prt, [RDFT]    = mac_low13_read_prt,
>>> +    [RDFHS]   = mac_low13_read_prt, [RDFTS]   = mac_low13_read_prt,
>>> +    [RDFPC]   = mac_low13_read_prt,
>>> +    [TDFH]    = mac_low11_read_prt, [TDFT]    = mac_low11_read_prt,
>>> +    [TDFHS]   = mac_low13_read_prt, [TDFTS]   = mac_low13_read_prt,
>>> +    [TDFPC]   = mac_low13_read_prt,
>>> +    [AIT]     = mac_low16_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_low4_read,
>>> +    [FFVT ... FFVT+254] = &mac_readreg,
>>> +    [PBM ... PBM+16383] = &mac_readreg_prt,
>>>  };
>> Need to limit the function of those MAC registers to 2.5 and above
>> (probably another flag and turn it off by default for pre 2.5) only.
>> Otherwise, the values of those registers will be zero after migrating to
>> old qemu.
> The registers in this patch are just for reading/writing, there is no
> functionality for them in the emulated device itself.

Yes, but this causes guest noticeable changes, e.g reading can have non
zero result after writing to them. For migration, we need to make sure
device behave same before and after migration. Otherwise, this may lead
unexpected result in guest which may be very hard to debug.

>  In case of
> migration to an older version, there will be no access to them anyhow
> - an attempt to access them will probably raise an error on an older
> version (in fact, this is the reason why they are implemented now) so
> zero value will not be a concern.
>

For older version (at least 2.4), writing will be ignored and reading
will result zero. (no errors but just some warnings).