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

[Peter Crosthwaite]

On Tue, Apr 29, 2014 at 2:57 AM, Peter Maydell <address@hidden> wrote:
> 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.
>

Yep. My bad.

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

So I am starting to think there a better way. Ultimately I want this
API to work for random structs too, not just integer types (E.G. PL330
would be a nice client). So I'm thinking this problem is outsourced to
the client who is responsible from bringing a VMStateInfo to the table
(input to fifo_create). We then have some some wrappers for the simple
integer types that trivally take the global vmstate_info_uintxx
structs as appropriate:

 void fifo_create8(Fifo *fifo, uint32_t capacity)
 {
     fifo_create(fifo, capacity, sizeof(uint8_t), vmstate_info_uint8);
 }

Most users will just just user createxx for an int fifo. But if you
want a migratable struct Fifo you can do that too. From device land:

fifo_create(&s->fifo, s->fifo_capacity, sizeof(MySpecialStruct),
vmstate_my_special_struct);

Work for you?

Regards,
Peter

> thanks
> -- PMM
>