Re: [Qemu-devel] [PATCH v4 2/4] util/fifo: Generalise for common integer widths

[Peter Maydell]

On 15 April 2014 04:18, Peter Crosthwaite <address@hidden> wrote:
> Add support for 16, 32 and 64 bit width FIFOs. The push and pop
> functions are replicated to accept all four different integer types.
> The element width of the FIFO is set at creation time.
>
> The backing storage for all element types is still uint8_t regardless of
> element width so some save-load logic is needed to handle endianness
> issue WRT VMSD.

> +/* Always store buffer data in little (arbitrarily chosen) endian format to
> + * allow for migration to/from BE <-> LE hosts.
> + */
> +
> +static inline void fifo_save_load_swap(Fifo *fifo)
> +{
> +#ifdef HOST_WORDS_BIGENDIAN
> +    int i;
> +    uint16_t *d16 = (uint16_t *)fifo->data;
> +    uint32_t *d32 = (uint32_t *)fifo->data;
> +    uint64_t *d64 = (uint64_t *)fifo->data;
> +
> +    for (i = 0; i < fifo->capacity; ++i) {
> +        switch (fifo->width) {
> +        case 1:
> +            return;
> +        case 2:
> +            d16[i] = bswap16(d16[i]);
> +            break;
> +        case 4:
> +            d32[i] = bswap32(d32[i]);
> +            break;
> +        case 8:
> +            d64[i] = bswap64(d64[i]);
> +            break;
> +        default:
> +            abort();
> +        }
> +    }
> +#endif
> +}
> +
> +static void fifo_pre_save(void *opaque)
> +{
> +    fifo_save_load_swap((Fifo *)opaque);
> +}
> +
> +static int fifo_post_load(void *opaque, int version_id)
> +{
> +    fifo_save_load_swap((Fifo *)opaque);
> +    return 0;
> +}
> +
>  const VMStateDescription vmstate_fifo = {
>      .name = "Fifo8",
>      .version_id = 1,
>      .minimum_version_id = 1,
>      .minimum_version_id_old = 1,
> +    .pre_save = fifo_pre_save,
> +    .post_load = fifo_post_load,
>      .fields      = (VMStateField[]) {
> -        VMSTATE_VBUFFER_UINT32(data, Fifo, 1, NULL, 0, capacity),
> +        VMSTATE_VBUFFER_UINT32(data, Fifo, 1, NULL, 0, buffer_size),
>          VMSTATE_UINT32(head, Fifo),
>          VMSTATE_UINT32(num, Fifo),
>          VMSTATE_END_OF_LIST()

This doesn't look right -- when the VM continues after
a save on a bigendian system all its FIFO data will
have been byteswapped and it'll fall over.

I think you need to implement this by adding get/put
functions which byteswap as they put the data onto
or take it off the wire. Check the use and definition
of vmstate_info_bitmap for an example of handling a
data structure where the on-the-wire and in-memory
formats differ.

thanks
-- PMM