Re: [Qemu-devel] [PATCH v2 1/4] FIFO: Add a FIFO32 implementation

[Peter Maydell]

On 15 February 2016 at 11:17, Jean-Christophe Dubois
<address@hidden> wrote:
> This one is build on top of the existing FIFO8
>
> Signed-off-by: Jean-Christophe Dubois <address@hidden>

> --- /dev/null
> +++ b/include/qemu/fifo32.h
> @@ -0,0 +1,206 @@

Can we have a copyright/license comment at the top of new files, please?
(I know fifo8.h doesn't have one, but ideally it should.)
Use gpl-2-or-later, since that's what util/fifo.c does and thus
by extension what Peter C intended for the header too.

> +#ifndef FIFO32_H
> +#define FIFO32_H
> +
> +#include "qemu/fifo8.h"
> +
> +typedef Fifo8 Fifo32;

This means the compiler won't notice if you accidentally pass
a Fifo8 to a fifo32 function, which seems a shame. Could we do

typedef struct {
    Fifo8 bytefifo;
} Fifo32;

(and then have all these wrapper functions pass &fifo->bytefifo to
the underlying fifo8 functions) ?

> +
> +/**
> + * fifo32_create:
> + * @fifo: struct Fifo32 to initialise with new FIFO
> + * @capacity: capacity of the newly created FIFO

You should specifically say "(number of 32-bit words)" here to avoid
possible confusion.

> + *
> + * Create a FIFO of the specified size. Clients should call fifo32_destroy()
> + * when finished using the fifo. The FIFO is initially empty.
> + */
> +
> +static inline void fifo32_create(Fifo32 *fifo, uint32_t capacity)
> +{
> +    fifo8_create(fifo, capacity * sizeof(uint32_t));
> +}
> +
> +/**
> + * fifo32_destroy:
> + * @fifo: FIFO to cleanup
> + *
> + * Cleanup a FIFO created with fifo32_create(). Frees memory created for FIFO
> +  *storage. The FIFO is no longer usable after this has been called.

Space has got into the wrong place at start of this line.

> + */
> +
> +static inline void fifo32_destroy(Fifo32 *fifo)
> +{
> +    fifo8_destroy(fifo);
> +}
> +
> +/**
> + * fifo32_num_free:
> + * @fifo: FIFO to check
> + *
> + * Return the number of free uint32_t slots in the FIFO.
> + *
> + * Returns: Number of free bytes.
> + */
> +
> +static inline uint32_t fifo32_num_free(Fifo32 *fifo)
> +{
> +    return (fifo8_num_free(fifo) + sizeof(uint32_t) - 1) / sizeof(uint32_t);

Why the round-up code? Isn't fifo8_num_free() always going to
return a multiple of 4 ? (If it is necessary, this is
DIV_ROUND_UP(fifo8_num_free(fifo), sizeof(uint32_t)) .)

> +}
> +
> +/**
> + * fifo32_num_used:
> + * @fifo: FIFO to check
> + *
> + * Return the number of used uint32_t slots in the FIFO.
> + *
> + * Returns: Number of used bytes.
> + */
> +
> +static inline uint32_t fifo32_num_used(Fifo32 *fifo)
> +{
> +    return (fifo8_num_used(fifo) + sizeof(uint32_t) - 1) / sizeof(uint32_t);

Similarly here.

> +}
> +
> +/**
> + * fifo32_push:
> + * @fifo: FIFO to push to
> + * @data: data byte to push

Not a byte...

> + *
> + * Push a data byte to the FIFO. Behaviour is undefined if the FIFO is full.

Ditto.

> + * Clients are responsible for checking for fullness using fifo32_is_full().
> + */
> +
> +static inline void fifo32_push(Fifo32 *fifo, uint32_t data)
> +{
> +    uint8_t *ptr = (uint8_t *)&data;

This is a bad idea. It means the representation of the data in the
underlying buffer depends on the endianness of the host system, which
means that migration between big and little endian hosts won't work.

Unfortunately fifo32_pop_buf() as an API makes this awkward,
because it implicitly asusmes that the buffer has data as
a series of host 32-bit words.

I think we have two choices:
(1) drop fifo32_pop_buf(), and store data in the fifo in
a fixed order, like LE:
    uint8_t le_data[4];
    stl_le_p(le_data, data);
    for (i = 0; i < sizeof(le_data); i++) {
        fifo8_push(fifo, le_data[i]);
    }
(and equivalently ldl_le_p() on fifo pop)

(2) store data in the fifo in host-word order and fix the
migration handling to cope with saving and restoring buffers
that need a 'swap from BE to host order' pass.

(2) would be cleaner but is more effort grubbing around in
the migration code. Unfortunately we can't start with the easy
choice and switch later if we need to because it would be a
migration compatibility break, so I think we need to do the
difficult but right thing now.

> +    int i;
> +
> +    if (fifo32_num_free(fifo) == 0) {
> +        abort();
> +    }

You don't need this, you can let fifo8_push() handle it for you.

> +
> +    for (i=0; i < sizeof(data); i++) {
> +        fifo8_push(fifo, ptr[i]);
> +    }
> +}
> +
> +/**
> + * fifo32_push_all:
> + * @fifo: FIFO to push to
> + * @data: data to push
> + * @size: number of bytes to push
> + *
> + * Push a byte array to the FIFO.

More things saying "byte" that don't mean it.

> Behaviour is undefined if the FIFO is full.
> + * Clients are responsible for checking the space left in the FIFO using
> + * fifo32_num_free().
> + */
> +
> +static inline void fifo32_push_all(Fifo32 *fifo, const uint32_t *data,
> +                                   uint32_t num)
> +{
> +    fifo8_push_all(fifo, (const uint8_t *)data, num * sizeof(uint32_t));

This will bite us if 'num' is guest-controlled information because then
the guest could make the multiply overflow.

> +}
> +
> +/**
> + * fifo32_pop:
> + * @fifo: fifo to pop from
> + *
> + * Pop a data byte from the FIFO. Behaviour is undefined if the FIFO is 
> empty.

not "byte"

> + * Clients are responsible for checking for emptyness using 
> fifo32_is_empty().

"emptiness"

> + *
> + * Returns: The popped data byte.
> + */
> +
> +static inline uint32_t fifo32_pop(Fifo32 *fifo)
> +{
> +    uint32_t ret = 0;
> +    uint8_t *ptr = (uint8_t *)&ret;
> +    int i;
> +
> +    for (i=0; i < sizeof(uint32_t); i++) {
> +        if (fifo8_is_empty(fifo)) {
> +            break;

This makes the behaviour on empty fifo different from fifo8_pop(),
which just abort()s. You can drop this check and let fifo8_pop()
handle the empty fifo case.

> +        }
> +        ptr[i] = fifo8_pop(fifo);
> +    }
> +
> +    return ret;
> +}
> +
> +/**
> + * fifo32_pop_buf:
> + * @fifo: FIFO to pop from
> + * @max: maximum number of bytes to pop
> + * @num: actual number of returned bytes

not "bytes"

> + *
> + * Pop a number of elements from the FIFO up to a maximum of max. The buffer
> + * containing the popped data is returned. This buffer points directly into
> + * the FIFO backing store and data is invalidated once any of the fifo32_* 
> APIs
> + * are called on the FIFO.
> + *
> + * The function may return fewer bytes than requested when the data wraps
> + * around in the ring buffer; in this case only a contiguous part of the data
> + * is returned.
> + *
> + * The number of valid bytes returned is populated in *num; will always 
> return
> + * at least 1 byte. max must not be 0 or greater than the number of bytes in
> + * the FIFO.
> + *
> + * Clients are responsible for checking the availability of requested data
> + * using fifo32_num_used().
> + *
> + * Returns: A pointer to popped data.
> + */
> +
> +static inline const uint32_t *fifo32_pop_buf(Fifo32 *fifo, uint32_t max,
> +                                             uint32_t *num)
> +{
> +    const uint8_t *ptr = fifo8_pop_buf(fifo, max * sizeof(uint32_t), num);
> +
> +    *num /= sizeof(uint32_t);
> +
> +    return (const uint32_t *)ptr;

You can't just cast this because you have no guarantee that the
value you get back from fifo8_pop_buf() meets the alignment
restrictions for a uint32_t*.

> +}
> +
> +/**
> + * fifo32_reset:
> + * @fifo: FIFO to reset
> + *
> + * Reset a FIFO. All data is discarded and the FIFO is emptied.
> + */
> +
> +static inline void fifo32_reset(Fifo32 *fifo)
> +{
> +    fifo8_reset(fifo);
> +}
> +
> +/**
> + * fifo32_is_empty:
> + * @fifo: FIFO to check
> + *
> + * Check if a FIFO is empty.
> + *
> + * Returns: True if the fifo is empty, false otherwise.
> + */
> +
> +static inline bool fifo32_is_empty(Fifo32 *fifo)
> +{
> +    return fifo8_is_empty(fifo);
> +}
> +
> +/**
> + * fifo32_is_full:
> + * @fifo: FIFO to check
> + *
> + * Check if a FIFO is full.
> + *
> + * Returns: True if the fifo is full, false otherwise.
> + */
> +
> +static inline bool fifo32_is_full(Fifo32 *fifo)
> +{
> +    return fifo8_num_free(fifo) < sizeof(uint32_t) ? true : false;

 "X ? true : false" is a longwinded way to say "X".

> +}
> +
> +#define VMSTATE_FIFO32(_field, _state) VMSTATE_FIFO8(_field, _state)
> +
> +#endif /* FIFO32_H */

thanks
-- PMM