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

[Beniamino Galvani]

On Wed, Apr 09, 2014 at 11:42:31PM -0700, Peter Crosthwaite 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.
> 
> Signed-off-by: Peter Crosthwaite <address@hidden>
> ---
> changed since v2:
> replicated (and glueified) the push/pop functions (Don Slutz review).
> Fix "each each" typo (Beniamino review).
> Done use "Case(n):" (Beniamino review).
> 
>  hw/char/serial.c        |   4 +-
>  hw/net/allwinner_emac.c |   6 +--
>  hw/ssi/xilinx_spi.c     |   4 +-
>  hw/ssi/xilinx_spips.c   |   4 +-
>  include/qemu/fifo.h     |  33 ++++++++++---
>  util/fifo.c             | 120 
> +++++++++++++++++++++++++++++++++++++-----------
>  6 files changed, 127 insertions(+), 44 deletions(-)
> 
> diff --git a/hw/char/serial.c b/hw/char/serial.c
> index c7a1841..c53db13 100644
> --- a/hw/char/serial.c
> +++ b/hw/char/serial.c
> @@ -659,8 +659,8 @@ void serial_realize_core(SerialState *s, Error **errp)
>  
>      qemu_chr_add_handlers(s->chr, serial_can_receive1, serial_receive1,
>                            serial_event, s);
> -    fifo_create(&s->recv_fifo, UART_FIFO_LENGTH);
> -    fifo_create(&s->xmit_fifo, UART_FIFO_LENGTH);
> +    fifo_create(&s->recv_fifo, UART_FIFO_LENGTH, 8);
> +    fifo_create(&s->xmit_fifo, UART_FIFO_LENGTH, 8);
>  }
>  
>  void serial_exit_core(SerialState *s)
> diff --git a/hw/net/allwinner_emac.c b/hw/net/allwinner_emac.c
> index 860cb5e..669b54a 100644
> --- a/hw/net/allwinner_emac.c
> +++ b/hw/net/allwinner_emac.c
> @@ -455,9 +455,9 @@ static void aw_emac_realize(DeviceState *dev, Error 
> **errp)
>                            object_get_typename(OBJECT(dev)), dev->id, s);
>      qemu_format_nic_info_str(qemu_get_queue(s->nic), s->conf.macaddr.a);
>  
> -    fifo_create(&s->rx_fifo, RX_FIFO_SIZE);
> -    fifo_create(&s->tx_fifo[0], TX_FIFO_SIZE);
> -    fifo_create(&s->tx_fifo[1], TX_FIFO_SIZE);
> +    fifo_create(&s->rx_fifo, RX_FIFO_SIZE, 8);
> +    fifo_create(&s->tx_fifo[0], TX_FIFO_SIZE, 8);
> +    fifo_create(&s->tx_fifo[1], TX_FIFO_SIZE, 8);
>  }
>  
>  static Property aw_emac_properties[] = {
> diff --git a/hw/ssi/xilinx_spi.c b/hw/ssi/xilinx_spi.c
> index ca19e47..017331d 100644
> --- a/hw/ssi/xilinx_spi.c
> +++ b/hw/ssi/xilinx_spi.c
> @@ -341,8 +341,8 @@ static int xilinx_spi_init(SysBusDevice *sbd)
>  
>      s->irqline = -1;
>  
> -    fifo_create(&s->tx_fifo, FIFO_CAPACITY);
> -    fifo_create(&s->rx_fifo, FIFO_CAPACITY);
> +    fifo_create(&s->tx_fifo, FIFO_CAPACITY, 8);
> +    fifo_create(&s->rx_fifo, FIFO_CAPACITY, 8);
>  
>      return 0;
>  }
> diff --git a/hw/ssi/xilinx_spips.c b/hw/ssi/xilinx_spips.c
> index 5633209..0777f5c 100644
> --- a/hw/ssi/xilinx_spips.c
> +++ b/hw/ssi/xilinx_spips.c
> @@ -669,8 +669,8 @@ static void xilinx_spips_realize(DeviceState *dev, Error 
> **errp)
>  
>      s->irqline = -1;
>  
> -    fifo_create(&s->rx_fifo, xsc->rx_fifo_size);
> -    fifo_create(&s->tx_fifo, xsc->tx_fifo_size);
> +    fifo_create(&s->rx_fifo, xsc->rx_fifo_size, 8);
> +    fifo_create(&s->tx_fifo, xsc->tx_fifo_size, 8);
>  }
>  
>  static void xilinx_qspips_realize(DeviceState *dev, Error **errp)
> diff --git a/include/qemu/fifo.h b/include/qemu/fifo.h
> index a704bd4..d15964d 100644
> --- a/include/qemu/fifo.h
> +++ b/include/qemu/fifo.h
> @@ -5,8 +5,12 @@
>  
>  typedef struct {
>      /* All fields are private */
> +    int width; /* byte width of each element */
> +    uint32_t capacity; /* number of elements */
> +
>      uint8_t *data;
> -    uint32_t capacity;
> +    uint32_t buffer_size;
> +
>      uint32_t head;
>      uint32_t num;
>  } Fifo;
> @@ -14,13 +18,14 @@ typedef struct {
>  /**
>   * fifo_create:
>   * @fifo: struct Fifo to initialise with new FIFO
> - * @capacity: capacity of the newly created FIFO
> + * @capacity: capacity (number of elements) of the newly created FIFO
> + * @width: integer width of each element. Must be 8, 16, 32 or 64.
>   *
>   * Create a FIFO of the specified size. Clients should call fifo_destroy()
>   * when finished using the fifo. The FIFO is initially empty.
>   */
>  
> -void fifo_create(Fifo *fifo, uint32_t capacity);
> +void fifo_create(Fifo *fifo, uint32_t capacity, int width);
>  
>  /**
>   * fifo_destroy:
> @@ -33,15 +38,22 @@ void fifo_create(Fifo *fifo, uint32_t capacity);
>  void fifo_destroy(Fifo *fifo);
>  
>  /**
> - * fifo_push:
> + * fifo_pushX:
>   * @fifo: FIFO to push to
>   * @data: data value to push
>   *
>   * Push a data value to the FIFO. Behaviour is undefined if the FIFO is full.
>   * Clients are responsible for checking for fullness using fifo_is_full().
> + *
> + * 8, 16, 32 and 64 bit variants are available. Behaviour is undefined if a
> + * variant mismatched to the FIFO width used (e.g. you cannot use fifo_push8
> + * with a FIFO created with width == 16).
>   */
>  
>  void fifo_push8(Fifo *fifo, uint8_t data);
> +void fifo_push16(Fifo *fifo, uint16_t data);
> +void fifo_push32(Fifo *fifo, uint32_t data);
> +void fifo_push64(Fifo *fifo, uint64_t data);
>  
>  /**
>   * fifo_push_all:
> @@ -54,19 +66,26 @@ void fifo_push8(Fifo *fifo, uint8_t data);
>   * fifo_num_free().
>   */
>  
> -void fifo_push_all(Fifo *fifo, const uint8_t *data, uint32_t num);
> +void fifo_push_all(Fifo *fifo, const void *data, uint32_t num);
>  
>  /**
> - * fifo_pop:
> + * fifo_popX:
>   * @fifo: fifo to pop from
>   *
>   * Pop a data value from the FIFO. Behaviour is undefined if the FIFO is 
> empty.
>   * Clients are responsible for checking for emptyness using fifo_is_empty().
>   *
> + * 8, 16, 32 and 64 bit variants are available. Behaviour is undefined if a
> + * variant mismatched to the FIFO width is used (e.g. you cannot use 
> fifo_pop8
> + * with a FIFO created with width == 16).
> + *
>   * Returns: The popped data value.
>   */
>  
>  uint8_t fifo_pop8(Fifo *fifo);
> +uint16_t fifo_pop16(Fifo *fifo);
> +uint32_t fifo_pop32(Fifo *fifo);
> +uint64_t fifo_pop64(Fifo *fifo);
>  
>  /**
>   * fifo_pop_buf:
> @@ -93,7 +112,7 @@ uint8_t fifo_pop8(Fifo *fifo);
>   * Returns: A pointer to popped data.
>   */
>  
> -const uint8_t *fifo_pop_buf(Fifo *fifo, uint32_t max, uint32_t *num);
> +const void *fifo_pop_buf(Fifo *fifo, uint32_t max, uint32_t *num);
>  
>  /**
>   * fifo_reset:
> diff --git a/util/fifo.c b/util/fifo.c
> index 4ee6c85..a52e920 100644
> --- a/util/fifo.c
> +++ b/util/fifo.c
> @@ -15,9 +15,11 @@
>  #include "qemu-common.h"
>  #include "qemu/fifo.h"
>  
> -void fifo_create(Fifo *fifo, uint32_t capacity)
> +void fifo_create(Fifo *fifo, uint32_t capacity, int width)
>  {
> -    fifo->data = g_new(uint8_t, capacity);
> +    assert(width == 8 || width == 16 || width == 32 || width == 64);
> +    fifo->width = width / 8;
> +    fifo->data = g_new(uint8_t, capacity * fifo->width);
>      fifo->capacity = capacity;

Maybe buffer_size should be initialized here as well.

Beniamino

>      fifo->head = 0;
>      fifo->num = 0;
> @@ -28,16 +30,25 @@ void fifo_destroy(Fifo *fifo)
>      g_free(fifo->data);
>  }
>  
> -void fifo_push8(Fifo *fifo, uint8_t data)
> -{
> -    if (fifo->num == fifo->capacity) {
> -        abort();
> -    }
> -    fifo->data[(fifo->head + fifo->num) % fifo->capacity] = data;
> -    fifo->num++;
> +#define FIFO_PUSH_FN(n)                                                     \
> +void fifo_push ## n(Fifo *fifo, uint ## n ## _t data)                       \
> +{                                                                           \
> +    uint32_t next_idx = (fifo->head + fifo->num) % fifo->capacity;          \
> +                                                                            \
> +    assert(n == fifo->width * 8);                                           \
> +    if (fifo->num == fifo->capacity) {                                      \
> +        abort();                                                            \
> +    }                                                                       \
> +    ((uint ## n ## _t *)fifo->data)[next_idx] = data;                       \
> +    fifo->num++;                                                            \
>  }
>  
> -void fifo_push_all(Fifo *fifo, const uint8_t *data, uint32_t num)
> +FIFO_PUSH_FN(8)
> +FIFO_PUSH_FN(16)
> +FIFO_PUSH_FN(32)
> +FIFO_PUSH_FN(64)
> +
> +void fifo_push_all(Fifo *fifo, const void *data, uint32_t num)
>  {
>      uint32_t start, avail;
>  
> @@ -48,38 +59,46 @@ void fifo_push_all(Fifo *fifo, const uint8_t *data, 
> uint32_t num)
>      start = (fifo->head + fifo->num) % fifo->capacity;
>  
>      if (start + num <= fifo->capacity) {
> -        memcpy(&fifo->data[start], data, num);
> +        memcpy(&fifo->data[start * fifo->width], data, num * fifo->width);
>      } else {
>          avail = fifo->capacity - start;
> -        memcpy(&fifo->data[start], data, avail);
> -        memcpy(&fifo->data[0], &data[avail], num - avail);
> +        memcpy(&fifo->data[start * fifo->width], data, avail * fifo->width);
> +        memcpy(&fifo->data[0], data + avail * fifo->width,
> +               (num - avail) * fifo->width);
>      }
>  
>      fifo->num += num;
>  }
>  
> -uint8_t fifo_pop8(Fifo *fifo)
> -{
> -    uint8_t ret;
> -
> -    if (fifo->num == 0) {
> -        abort();
> -    }
> -    ret = fifo->data[fifo->head++];
> -    fifo->head %= fifo->capacity;
> -    fifo->num--;
> -    return ret;
> +#define FIFO_POP_FN(n)                                                      \
> +uint ## n ## _t fifo_pop ## n(Fifo *fifo)                                   \
> +{                                                                           \
> +    uint32_t next_idx;                                                      \
> +                                                                            \
> +    assert(n == fifo->width * 8);                                           \
> +    if (fifo->num == 0) {                                                   \
> +        abort();                                                            \
> +    }                                                                       \
> +    next_idx = fifo->head++;                                                \
> +    fifo->head %= fifo->capacity;                                           \
> +    fifo->num--;                                                            \
> +    return ((uint ## n ## _t *)fifo->data)[next_idx];                       \
>  }
>  
> -const uint8_t *fifo_pop_buf(Fifo *fifo, uint32_t max, uint32_t *num)
> +FIFO_POP_FN(8)
> +FIFO_POP_FN(16)
> +FIFO_POP_FN(32)
> +FIFO_POP_FN(64)
> +
> +const void *fifo_pop_buf(Fifo *fifo, uint32_t max, uint32_t *num)
>  {
> -    uint8_t *ret;
> +    void *ret;
>  
>      if (max == 0 || max > fifo->num) {
>          abort();
>      }
>      *num = MIN(fifo->capacity - fifo->head, max);
> -    ret = &fifo->data[fifo->head];
> +    ret = &fifo->data[fifo->head * fifo->width];
>      fifo->head += *num;
>      fifo->head %= fifo->capacity;
>      fifo->num -= *num;
> @@ -112,13 +131,58 @@ uint32_t fifo_num_used(Fifo *fifo)
>      return fifo->num;
>  }
>  
> +/* 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()
> -- 
> 1.9.1.1.gbb9f595
>