From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44518) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zrk1O-00046y-3r for qemu-devel@nongnu.org; Thu, 29 Oct 2015 06:01:32 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zrk1F-0006Xu-UO for qemu-devel@nongnu.org; Thu, 29 Oct 2015 06:01:26 -0400 Received: from mx1.redhat.com ([209.132.183.28]:32929) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zrk1F-0006Xh-Kc for qemu-devel@nongnu.org; Thu, 29 Oct 2015 06:01:17 -0400 References: <1446046288-25650-1-git-send-email-leonid.bloch@ravellosystems.com> <1446046288-25650-3-git-send-email-leonid.bloch@ravellosystems.com> <56318CB7.1000507@redhat.com> From: Jason Wang Message-ID: <5631EE69.7010105@redhat.com> Date: Thu, 29 Oct 2015 18:01:13 +0800 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v3 2/6] e1000: Trivial implementation of various MAC registers List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Leonid Bloch Cc: Dmitry Fleytman , Leonid Bloch , qemu-devel@nongnu.org, Shmulik Ladkani On 10/29/2015 05:33 PM, Leonid Bloch wrote: > On Thu, Oct 29, 2015 at 5:04 AM, Jason Wang 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 >>> Signed-off-by: Dmitry Fleytman >>> --- >>> 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?)