Re: [Qemu-devel] [PATCH 09/10] hw/sd/pxa2xx_mmci: Convert to VMStateDescription

[Peter Crosthwaite <crosthwaitepeter@gmail.com>]

On Fri, Dec 11, 2015 at 04:37:10PM +0000, Peter Maydell wrote:
> Convert the pxa2xx_mmci device from manual save/load
> functions to a VMStateDescription structure.
> 
> This is a migration compatibility break.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/sd/pxa2xx_mmci.c | 148 ++++++++++++++++++++--------------------------------
>  1 file changed, 56 insertions(+), 92 deletions(-)
> 
> diff --git a/hw/sd/pxa2xx_mmci.c b/hw/sd/pxa2xx_mmci.c
> index a30be2b..81fec4d 100644
> --- a/hw/sd/pxa2xx_mmci.c
> +++ b/hw/sd/pxa2xx_mmci.c
> @@ -43,27 +43,72 @@ typedef struct PXA2xxMMCIState {
>      uint32_t cmdat;
>      uint32_t resp_tout;
>      uint32_t read_tout;
> -    int blklen;
> -    int numblk;
> +    int32_t blklen;
> +    int32_t numblk;
>      uint32_t intmask;
>      uint32_t intreq;
> -    int cmd;
> +    int32_t cmd;
>      uint32_t arg;
>  
> -    int active;
> -    int bytesleft;
> +    int32_t active;
> +    int32_t bytesleft;
>      uint8_t tx_fifo[64];
> -    int tx_start;
> -    int tx_len;
> +    uint32_t tx_start;
> +    uint32_t tx_len;
>      uint8_t rx_fifo[32];
> -    int rx_start;
> -    int rx_len;
> +    uint32_t rx_start;
> +    uint32_t rx_len;
>      uint16_t resp_fifo[9];
> -    int resp_len;
> +    uint32_t resp_len;
>  
> -    int cmdreq;
> +    int32_t cmdreq;
>  } PXA2xxMMCIState;
>  
> +static bool pxa2xx_mmci_vmstate_validate(void *opaque, int version_id)
> +{
> +    PXA2xxMMCIState *s = opaque;
> +
> +    return s->tx_start < sizeof(s->tx_fifo)
> +        && s->rx_start < sizeof(s->rx_fifo)
> +        && s->tx_len <= sizeof(s->tx_fifo)
> +        && s->rx_len <= sizeof(s->rx_fifo)
> +        && s->resp_len <= sizeof(s->resp_fifo);


A nit, but I wonder if ARRAY_SIZE should be generally used in this case, as
it is a little more self documenting and would make code identical to
non-byte FIFOs.

> +}
> +

Extra blank.

> +
> +static const VMStateDescription vmstate_pxa2xx_mmci = {

Is the registration in class_init missing?

Otherwise,

Reviewed-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>

Regards,
Peter

> +    .name = "pxa2xx-mmci",
> +    .version_id = 2,
> +    .minimum_version_id = 2,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT32(status, PXA2xxMMCIState),
> +        VMSTATE_UINT32(clkrt, PXA2xxMMCIState),
> +        VMSTATE_UINT32(spi, PXA2xxMMCIState),
> +        VMSTATE_UINT32(cmdat, PXA2xxMMCIState),
> +        VMSTATE_UINT32(resp_tout, PXA2xxMMCIState),
> +        VMSTATE_UINT32(read_tout, PXA2xxMMCIState),
> +        VMSTATE_INT32(blklen, PXA2xxMMCIState),
> +        VMSTATE_INT32(numblk, PXA2xxMMCIState),
> +        VMSTATE_UINT32(intmask, PXA2xxMMCIState),
> +        VMSTATE_UINT32(intreq, PXA2xxMMCIState),
> +        VMSTATE_INT32(cmd, PXA2xxMMCIState),
> +        VMSTATE_UINT32(arg, PXA2xxMMCIState),
> +        VMSTATE_INT32(cmdreq, PXA2xxMMCIState),
> +        VMSTATE_INT32(active, PXA2xxMMCIState),
> +        VMSTATE_INT32(bytesleft, PXA2xxMMCIState),
> +        VMSTATE_UINT32(tx_start, PXA2xxMMCIState),
> +        VMSTATE_UINT32(tx_len, PXA2xxMMCIState),
> +        VMSTATE_UINT32(rx_start, PXA2xxMMCIState),
> +        VMSTATE_UINT32(rx_len, PXA2xxMMCIState),
> +        VMSTATE_UINT32(resp_len, PXA2xxMMCIState),
> +        VMSTATE_VALIDATE("fifo size incorrect", pxa2xx_mmci_vmstate_validate),
> +        VMSTATE_UINT8_ARRAY(tx_fifo, PXA2xxMMCIState, 64),
> +        VMSTATE_UINT8_ARRAY(rx_fifo, PXA2xxMMCIState, 32),
> +        VMSTATE_UINT16_ARRAY(resp_fifo, PXA2xxMMCIState, 9),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  #define MMC_STRPCL	0x00	/* MMC Clock Start/Stop register */
>  #define MMC_STAT	0x04	/* MMC Status register */
>  #define MMC_CLKRT	0x08	/* MMC Clock Rate register */
> @@ -405,84 +450,6 @@ static const MemoryRegionOps pxa2xx_mmci_ops = {
>      .endianness = DEVICE_NATIVE_ENDIAN,
>  };
>  
> -static void pxa2xx_mmci_save(QEMUFile *f, void *opaque)
> -{
> -    PXA2xxMMCIState *s = (PXA2xxMMCIState *) opaque;
> -    int i;
> -
> -    qemu_put_be32s(f, &s->status);
> -    qemu_put_be32s(f, &s->clkrt);
> -    qemu_put_be32s(f, &s->spi);
> -    qemu_put_be32s(f, &s->cmdat);
> -    qemu_put_be32s(f, &s->resp_tout);
> -    qemu_put_be32s(f, &s->read_tout);
> -    qemu_put_be32(f, s->blklen);
> -    qemu_put_be32(f, s->numblk);
> -    qemu_put_be32s(f, &s->intmask);
> -    qemu_put_be32s(f, &s->intreq);
> -    qemu_put_be32(f, s->cmd);
> -    qemu_put_be32s(f, &s->arg);
> -    qemu_put_be32(f, s->cmdreq);
> -    qemu_put_be32(f, s->active);
> -    qemu_put_be32(f, s->bytesleft);
> -
> -    qemu_put_byte(f, s->tx_len);
> -    for (i = 0; i < s->tx_len; i ++)
> -        qemu_put_byte(f, s->tx_fifo[(s->tx_start + i) & 63]);
> -
> -    qemu_put_byte(f, s->rx_len);
> -    for (i = 0; i < s->rx_len; i ++)
> -        qemu_put_byte(f, s->rx_fifo[(s->rx_start + i) & 31]);
> -
> -    qemu_put_byte(f, s->resp_len);
> -    for (i = s->resp_len; i < 9; i ++)
> -        qemu_put_be16s(f, &s->resp_fifo[i]);
> -}
> -
> -static int pxa2xx_mmci_load(QEMUFile *f, void *opaque, int version_id)
> -{
> -    PXA2xxMMCIState *s = (PXA2xxMMCIState *) opaque;
> -    int i;
> -
> -    qemu_get_be32s(f, &s->status);
> -    qemu_get_be32s(f, &s->clkrt);
> -    qemu_get_be32s(f, &s->spi);
> -    qemu_get_be32s(f, &s->cmdat);
> -    qemu_get_be32s(f, &s->resp_tout);
> -    qemu_get_be32s(f, &s->read_tout);
> -    s->blklen = qemu_get_be32(f);
> -    s->numblk = qemu_get_be32(f);
> -    qemu_get_be32s(f, &s->intmask);
> -    qemu_get_be32s(f, &s->intreq);
> -    s->cmd = qemu_get_be32(f);
> -    qemu_get_be32s(f, &s->arg);
> -    s->cmdreq = qemu_get_be32(f);
> -    s->active = qemu_get_be32(f);
> -    s->bytesleft = qemu_get_be32(f);
> -
> -    s->tx_len = qemu_get_byte(f);
> -    s->tx_start = 0;
> -    if (s->tx_len >= sizeof(s->tx_fifo) || s->tx_len < 0)
> -        return -EINVAL;
> -    for (i = 0; i < s->tx_len; i ++)
> -        s->tx_fifo[i] = qemu_get_byte(f);
> -
> -    s->rx_len = qemu_get_byte(f);
> -    s->rx_start = 0;
> -    if (s->rx_len >= sizeof(s->rx_fifo) || s->rx_len < 0)
> -        return -EINVAL;
> -    for (i = 0; i < s->rx_len; i ++)
> -        s->rx_fifo[i] = qemu_get_byte(f);
> -
> -    s->resp_len = qemu_get_byte(f);
> -    if (s->resp_len > 9 || s->resp_len < 0)
> -        return -EINVAL;
> -    for (i = s->resp_len; i < 9; i ++)
> -         qemu_get_be16s(f, &s->resp_fifo[i]);
> -
> -    return 0;
> -}
> -
>  PXA2xxMMCIState *pxa2xx_mmci_init(MemoryRegion *sysmem,
>                  hwaddr base,
>                  BlockBackend *blk, qemu_irq irq,
> @@ -555,9 +522,6 @@ static void pxa2xx_mmci_instance_init(Object *obj)
>      sysbus_init_irq(sbd, &s->rx_dma);
>      sysbus_init_irq(sbd, &s->tx_dma);
>  
> -    register_savevm(NULL, "pxa2xx_mmci", 0, 0,
> -                    pxa2xx_mmci_save, pxa2xx_mmci_load, s);
> -
>      qbus_create_inplace(&s->sdbus, sizeof(s->sdbus),
>                          TYPE_PXA2XX_MMCI_BUS, DEVICE(obj), "sd-bus");
>  }
> -- 
> 1.9.1
>