Re: [Qemu-devel] [PATCH v3 07/30] imx_fec: Add support for multiple Tx DMA rings

[Peter Maydell]

On 6 November 2017 at 15:47, Andrey Smirnov <address@hidden> 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 <address@hidden>
> Cc: Jason Wang <address@hidden>
> Cc: Philippe Mathieu-Daudé <address@hidden>
> Cc: address@hidden
> Cc: address@hidden
> Cc: address@hidden
> Signed-off-by: Andrey Smirnov <address@hidden>

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

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

Otherwise

Reviewed-by: Peter Maydell <address@hidden>

thanks
-- PMM