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

[Peter Crosthwaite]

On Wed, Apr 9, 2014 at 7:42 AM, Beniamino Galvani <address@hidden> wrote:
> On Mon, Apr 07, 2014 at 07:05:18PM -0700, Peter Crosthwaite wrote:
>> Add support for 16, 32 and 64 bit width FIFOs. The push and pop
>> functions are patched to accept uint64_t always to support up to 64bit
>> integer elements. The element width 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>
>> ---
>>
>>  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     |  19 +++++----
>>  util/fifo.c             | 109 
>> +++++++++++++++++++++++++++++++++++++++++-------
>>  6 files changed, 114 insertions(+), 32 deletions(-)
>>
>> diff --git a/hw/char/serial.c b/hw/char/serial.c
>> index a0d8024..3060769 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 e804411..2a0d2ac 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 8fe3072..cac666b 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 d7d4c41..a7a639f 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 44766b3..8738027 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 each each element */
>
> of each element
>

Will fix.

>> +    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:
>> @@ -41,7 +46,7 @@ void fifo_destroy(Fifo *fifo);
>>   * Clients are responsible for checking for fullness using fifo_is_full().
>>   */
>>
>> -void fifo_push(Fifo *fifo, uint8_t data);
>> +void fifo_push(Fifo *fifo, uint64_t data);
>>
>>  /**
>>   * fifo_push_all:
>> @@ -54,7 +59,7 @@ void fifo_push(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:
>> @@ -66,7 +71,7 @@ void fifo_push_all(Fifo *fifo, const uint8_t *data, 
>> uint32_t num);
>>   * Returns: The popped data value.
>>   */
>>
>> -uint8_t fifo_pop(Fifo *fifo);
>> +uint64_t fifo_pop(Fifo *fifo);
>>
>>  /**
>>   * fifo_pop_buf:
>> @@ -93,7 +98,7 @@ uint8_t fifo_pop(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 ffadf55..51318ad 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;
>>      fifo->head = 0;
>>      fifo->num = 0;
>> @@ -28,16 +30,33 @@ void fifo_destroy(Fifo *fifo)
>>      g_free(fifo->data);
>>  }
>>
>> -void fifo_push(Fifo *fifo, uint8_t data)
>> +void fifo_push(Fifo *fifo, uint64_t data)
>>  {
>> +    uint32_t next_idx = (fifo->head + fifo->num) % fifo->capacity;
>> +
>>      if (fifo->num == fifo->capacity) {
>>          abort();
>>      }
>> -    fifo->data[(fifo->head + fifo->num) % fifo->capacity] = data;
>> +    switch (fifo->width) {
>> +    case(1):
>
> To me 'case 1:' looks better, but it's only a personal taste :)
>

Ok, will fix.

Regards,
Peter