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

[Jason Wang <jasowang@redhat.com>]

On 10/29/2015 05:33 PM, Leonid Bloch wrote:
> On Thu, Oct 29, 2015 at 5:04 AM, Jason Wang <jasowang@redhat.com> wrote:
>>
>> On 10/28/2015 11:31 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 <leonid.bloch@ravellosystems.com>
>>> Signed-off-by: Dmitry Fleytman <dmitry.fleytman@ravellosystems.com>
>>> ---
>>>  hw/net/e1000.c      | 154 +++++++++++++++++++++++++++++++++++++++++++++++++++-
>>>  hw/net/e1000_regs.h |   6 ++
>>>  2 files changed, 157 insertions(+), 3 deletions(-)

[...]

>>>
>>> +static bool e1000_extra_trivial_regs_needed(void *opaque)
>>> +{
>>> +    return true;
>>> +}
>> This reminds me  that we need care the migration compatibility to older
>> version here. Probably we need another property for e1000, and only
>> migrate and implement the new mac registers for version >= 2.5. An
>> example is mit implementation. (see
>> e9845f0985f088dd01790f4821026df0afba5795). And need to do the same for
>> patch 6.
>>
>>> +
>>> +static const VMStateDescription vmstate_e1000_extra_trivial_regs = {
>>> +    .name = "e1000/extra_trivial_regs",
>>> +    .version_id = 1,
>>> +    .minimum_version_id = 1,
>>> +    .needed = e1000_extra_trivial_regs_needed,
>>> +    .fields = (VMStateField[]) {
>>> +        VMSTATE_UINT32(mac_reg[AIT], E1000State),
>>> +        VMSTATE_UINT32(mac_reg[SCC], E1000State),
>>> +        VMSTATE_UINT32(mac_reg[ECOL], E1000State),
>>> +        VMSTATE_UINT32(mac_reg[MCC], E1000State),
>>> +        VMSTATE_UINT32(mac_reg[LATECOL], E1000State),
>>> +        VMSTATE_UINT32(mac_reg[COLC], E1000State),
>>> +        VMSTATE_UINT32(mac_reg[DC], E1000State),
>>> +        VMSTATE_UINT32(mac_reg[TNCRS], E1000State),
>>> +        VMSTATE_UINT32(mac_reg[SEC], E1000State),
>>> +        VMSTATE_UINT32(mac_reg[CEXTERR], E1000State),
>>> +        VMSTATE_UINT32(mac_reg[RLEC], E1000State),
>>> +        VMSTATE_UINT32(mac_reg[XONRXC], E1000State),
>>> +        VMSTATE_UINT32(mac_reg[XONTXC], E1000State),
>>> +        VMSTATE_UINT32(mac_reg[XOFFRXC], E1000State),
>>> +        VMSTATE_UINT32(mac_reg[XOFFTXC], E1000State),
>>> +        VMSTATE_UINT32(mac_reg[FCRUC], E1000State),
>>> +        VMSTATE_UINT32(mac_reg[RNBC], E1000State),
>>> +        VMSTATE_UINT32(mac_reg[RFC], E1000State),
>>> +        VMSTATE_UINT32(mac_reg[RJC], E1000State),
>>> +        VMSTATE_UINT32(mac_reg[MGTPRC], E1000State),
>>> +        VMSTATE_UINT32(mac_reg[MGTPDC], E1000State),
>>> +        VMSTATE_UINT32(mac_reg[MGTPTC], E1000State),
>>> +        VMSTATE_UINT32(mac_reg[TSCTFC], E1000State),
>>> +        VMSTATE_UINT32(mac_reg[WUC], E1000State),
>>> +        VMSTATE_UINT32(mac_reg[WUS], E1000State),
>>> +        VMSTATE_UINT32(mac_reg[IPAV], E1000State),
>>> +        VMSTATE_UINT32_SUB_ARRAY(mac_reg, E1000State, IP4AT, 7),
>>> +        VMSTATE_UINT32_SUB_ARRAY(mac_reg, E1000State, IP6AT, 4),
>>> +        VMSTATE_UINT32_SUB_ARRAY(mac_reg, E1000State, WUPM, 32),
>>> +        VMSTATE_UINT32_SUB_ARRAY(mac_reg, E1000State, FFLT, 7),
>>> +        VMSTATE_UINT32_SUB_ARRAY(mac_reg, E1000State, FFMT, 255),
>>> +        VMSTATE_UINT32_SUB_ARRAY(mac_reg, E1000State, FFVT, 255),
>>> +        VMSTATE_UINT32(mac_reg[RDFH], E1000State),
>>> +        VMSTATE_UINT32(mac_reg[RDFT], E1000State),
>>> +        VMSTATE_UINT32(mac_reg[RDFHS], E1000State),
>>> +        VMSTATE_UINT32(mac_reg[RDFTS], E1000State),
>>> +        VMSTATE_UINT32(mac_reg[RDFPC], E1000State),
>>> +        VMSTATE_UINT32(mac_reg[TDFH], E1000State),
>>> +        VMSTATE_UINT32(mac_reg[TDFT], E1000State),
>>> +        VMSTATE_UINT32(mac_reg[TDFHS], E1000State),
>>> +        VMSTATE_UINT32(mac_reg[TDFTS], E1000State),
>>> +        VMSTATE_UINT32(mac_reg[TDFPC], E1000State),
>>> +        VMSTATE_UINT32_SUB_ARRAY(mac_reg, E1000State, PBM, 16384),
>>> +        VMSTATE_END_OF_LIST()
>>> +    }
>>> +};
>> I was considering a better approach here. Since we may want to add more
>> mac register implementation in the future, so we probably don't want to
>> add another subsections like this. How about just migrate mac_reg array
>> instead of specific registers here?
>>
>> Thanks
>>
>>> +
>>>  static const VMStateDescription vmstate_e1000 = {
>>>      .name = "e1000",
>>>      .version_id = 2,
>>> @@ -1469,6 +1616,7 @@ static const VMStateDescription vmstate_e1000 = {
>>>      },
>>>      .subsections = (const VMStateDescription*[]) {
>>>          &vmstate_e1000_mit_state,
>>> +        &vmstate_e1000_extra_trivial_regs,
>>>          NULL
>>>      }
>>>  };
>>> 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 */
> So you mean adding another boolean parameter, "full_mac_migration",
> for instance, and if it is set - migrate the full mac_reg array,
> otherwise - migrate just the registers that were previously
> implemented?
>
> Leonid.
> __________
>

Something like this, but this parameter may also be used to decide
whether or not we should emulate the new mac registers, so maybe a
better name is needed and we it will imply to migrate full mac_reg
array. (Since patch 6 adds even more mac registers, maybe it's better to
squash that one into this?)