On Tue, Nov 21, 2017 at 9:44 AM, Peter Maydell <peter.mayd...@linaro.org> wrote: > On 6 November 2017 at 15:47, Andrey Smirnov <andrew.smir...@gmail.com> wrote: >> More recent version of the IP block support more than one Tx DMA ring, >> so add the code implementing that feature. >> >> Cc: Peter Maydell <peter.mayd...@linaro.org> >> Cc: Jason Wang <jasow...@redhat.com> >> Cc: Philippe Mathieu-Daudé <f4...@amsat.org> >> Cc: qemu-devel@nongnu.org >> Cc: qemu-...@nongnu.org >> Cc: yurov...@gmail.com >> Signed-off-by: Andrey Smirnov <andrew.smir...@gmail.com> > >> static const VMStateDescription vmstate_imx_eth = { >> .name = TYPE_IMX_FEC, >> - .version_id = 2, >> - .minimum_version_id = 2, >> + .version_id = 3, >> + .minimum_version_id = 3, >> .fields = (VMStateField[]) { >> VMSTATE_UINT32_ARRAY(regs, IMXFECState, ENET_MAX), >> VMSTATE_UINT32(rx_descriptor, IMXFECState), >> - VMSTATE_UINT32(tx_descriptor, IMXFECState), >> - >> + VMSTATE_UINT32_ARRAY(tx_descriptor, IMXFECState, ENET_TX_RING_NUM), >> + VMSTATE_UINT32(tx_ring_num, IMXFECState), >> VMSTATE_UINT32(phy_status, IMXFECState), >> VMSTATE_UINT32(phy_control, IMXFECState), >> VMSTATE_UINT32(phy_advertise, IMXFECState), > > tx_ring_num is constant for any particular instantiation of the device, > so you don't need to put it in the vmstate. > > It's pretty trivial to make this vmstate compatible with the old > ones for the existing single-tx-descriptor devices, so we might as well: > > /* Versions of this device with more than one TX descriptor > * save the 2nd and 3rd descriptors in a subsection, to maintain > * migration compatibility with previous versions of the device > * that only supported a single descriptor. > */ > static bool txdescs_needed(void *opaque) { > IMXFECState *s = opaque; > > return s->tx_ring_num > 1; > } > > static const VMStateDescription vmstate_imx_eth_txdescs = { > .name = "imx.fec/txdescs", > .version_id = 1, > .minimum_version_id = 1, > .needed = txdescs_needed, > .fields = (VMStateField[]) { > VMSTATE_UINT32(tx_descriptor[1], IMXFECState), > VMSTATE_UINT32(tx_descriptor[2], IMXFECState), > VMSTATE_END_OF_LIST() > } > }; > > and then add this to the vmx_state_eth at the end: > .subsections = (const VMStateDescription*[]) { > &vmstate_imx_eth_txdescs, > NULL > } > > Then you don't need to bump version_id/minimum_version_id on the > vmstate_imx_eth struct. >
Cool, sounds good. Will add that to the patch in v4. >> @@ -791,6 +821,7 @@ static void imx_eth_write(void *opaque, hwaddr offset, >> uint64_t value, >> unsigned size) >> { >> IMXFECState *s = IMX_FEC(opaque); >> + const bool single_tx_ring = s->tx_ring_num != 3; > > This looks odd -- I would have expected "single_tx_ring = > s->tx_ring_num == 1" ... AFAIK the HW that's out there will have either 3 or 1 Tx ring, so that's why I wrote it this way. I'll change the logic to the way your suggest to avoid surprising the reader. Thanks, Andrey Smirnov