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

[Peter Crosthwaite]

On Tue, May 6, 2014 at 11:41 AM, Peter Crosthwaite
<address@hidden> wrote:
> 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).

Ok so this is much trickier than I thought (+Juan). Opaquifying the
VMState info (and I use the term loosely there) is rather tricky as
sometimes the info is described via a VMStateInfo (usually for the
simpler types such sm_state_info_uint8), but for structs you use a
VMStateDescription. You therefore cant wrap these oqauely as you dont
know which of VMStateInfo and VMStateDescription you want to use until
you have a specific case.

I guess you could overcome this by merging VMStateInfo and
VMStateDescription. I think this may be a good idea. AFAICT, the
difference between the two, is that VMStateInfo (VMSI) is code driven
and forms the leaves of the VMSD tree, while VMStateDescription is
data driven and describes the tree branches (and trunk). What's
irregular here is that the "branch" (VMSD) and "leaf" (VMSI) types are
different and the branches need to be aware of that. E.G. VMSD needs
to know whether its children (i.e. the .fields) are VMSD's or VMSI's,
leading to significant API replication. E.G. VMSTATE_VARRAY_UINT32 and
VMSTATE_STRUCT_VARRAY_UINT32 are the same thing, except they describe
element types with VMSI and VMSD resp. Only diff is "my children are
leaves" or "my children are branches" which generally something you
want to abstract away from a recursion mechanism.

So this whole system could be made simpler, if what little
functionality VMSI implements (just some code driven hooks) was rolled
into VMSD and everyone used VMSD. Then all these duped APIs could be
consilidated.

And I could solve my opaque VMSD problem :)

Thoughts?

Regards,
Peter

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