[PATCH 0/3] util/fifo8: Introduce fifo8_change_capacity()

[PATCH 0/3] util/fifo8: Introduce fifo8_change_capacity()

[Philippe Mathieu-Daudé]

Preliminary series to PL011 FIFO work:
- Clarify doc in qemu_chr_fe_*
- Introduce fifo8_change_capacity()

Philippe Mathieu-Daudé (3):
  chardev/char-fe: Document returned value on error
  util/fifo8: Use fifo8_reset() in fifo8_create()
  util/fifo8: Introduce fifo8_change_capacity()

 include/chardev/char-fe.h |  3 +++
 include/qemu/fifo8.h      | 10 ++++++++++
 util/fifo8.c              | 22 ++++++++++++++--------
 3 files changed, 27 insertions(+), 8 deletions(-)

-- 
2.41.0

[PATCH 1/3] chardev/char-fe: Document returned value on error

[Philippe Mathieu-Daudé]

qemu_chr_fe_add_watch() and qemu_chr_fe_write[_all]()
return -1 on error. Mention it in the documentation.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/chardev/char-fe.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/chardev/char-fe.h b/include/chardev/char-fe.h
index ecef182835..3310449eaf 100644
--- a/include/chardev/char-fe.h
+++ b/include/chardev/char-fe.h
@@ -228,6 +228,7 @@ guint qemu_chr_fe_add_watch(CharBackend *be, GIOCondition cond,
  * is thread-safe.
  *
  * Returns: the number of bytes consumed (0 if no associated Chardev)
+ *          or -1 on error.
  */
 int qemu_chr_fe_write(CharBackend *be, const uint8_t *buf, int len);
 
@@ -242,6 +243,7 @@ int qemu_chr_fe_write(CharBackend *be, const uint8_t *buf, int len);
  * attempted to be written.  This function is thread-safe.
  *
  * Returns: the number of bytes consumed (0 if no associated Chardev)
+ *          or -1 on error.
  */
 int qemu_chr_fe_write_all(CharBackend *be, const uint8_t *buf, int len);
 
@@ -253,6 +255,7 @@ int qemu_chr_fe_write_all(CharBackend *be, const uint8_t *buf, int len);
  * Read data to a buffer from the back end.
  *
  * Returns: the number of bytes read (0 if no associated Chardev)
+ *          or -1 on error.
  */
 int qemu_chr_fe_read_all(CharBackend *be, uint8_t *buf, int len);
 
-- 
2.41.0

Re: [PATCH 1/3] chardev/char-fe: Document returned value on error

[Mark Cave-Ayland]

On 19/07/2024 16:16, Philippe Mathieu-Daudé wrote:

> qemu_chr_fe_add_watch() and qemu_chr_fe_write[_all]()
> return -1 on error. Mention it in the documentation.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   include/chardev/char-fe.h | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/include/chardev/char-fe.h b/include/chardev/char-fe.h
> index ecef182835..3310449eaf 100644
> --- a/include/chardev/char-fe.h
> +++ b/include/chardev/char-fe.h
> @@ -228,6 +228,7 @@ guint qemu_chr_fe_add_watch(CharBackend *be, GIOCondition cond,
>    * is thread-safe.
>    *
>    * Returns: the number of bytes consumed (0 if no associated Chardev)
> + *          or -1 on error.
>    */
>   int qemu_chr_fe_write(CharBackend *be, const uint8_t *buf, int len);
>   
> @@ -242,6 +243,7 @@ int qemu_chr_fe_write(CharBackend *be, const uint8_t *buf, int len);
>    * attempted to be written.  This function is thread-safe.
>    *
>    * Returns: the number of bytes consumed (0 if no associated Chardev)
> + *          or -1 on error.
>    */
>   int qemu_chr_fe_write_all(CharBackend *be, const uint8_t *buf, int len);
>   
> @@ -253,6 +255,7 @@ int qemu_chr_fe_write_all(CharBackend *be, const uint8_t *buf, int len);
>    * Read data to a buffer from the back end.
>    *
>    * Returns: the number of bytes read (0 if no associated Chardev)
> + *          or -1 on error.
>    */
>   int qemu_chr_fe_read_all(CharBackend *be, uint8_t *buf, int len);

Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


ATB,

Mark.

[PATCH 2/3] util/fifo8: Use fifo8_reset() in fifo8_create()

[Philippe Mathieu-Daudé]

Avoid open-coding fifo8_reset() in fifo8_create().

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 util/fifo8.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/util/fifo8.c b/util/fifo8.c
index 4e01b532d9..2925fe5611 100644
--- a/util/fifo8.c
+++ b/util/fifo8.c
@@ -16,12 +16,17 @@
 #include "migration/vmstate.h"
 #include "qemu/fifo8.h"
 
+void fifo8_reset(Fifo8 *fifo)
+{
+    fifo->num = 0;
+    fifo->head = 0;
+}
+
 void fifo8_create(Fifo8 *fifo, uint32_t capacity)
 {
     fifo->data = g_new(uint8_t, capacity);
     fifo->capacity = capacity;
-    fifo->head = 0;
-    fifo->num = 0;
+    fifo8_reset(fifo);
 }
 
 void fifo8_destroy(Fifo8 *fifo)
@@ -97,12 +102,6 @@ const uint8_t *fifo8_pop_buf(Fifo8 *fifo, uint32_t max, uint32_t *numptr)
     return fifo8_peekpop_buf(fifo, max, numptr, true);
 }
 
-void fifo8_reset(Fifo8 *fifo)
-{
-    fifo->num = 0;
-    fifo->head = 0;
-}
-
 bool fifo8_is_empty(Fifo8 *fifo)
 {
     return (fifo->num == 0);
-- 
2.41.0

Re: [PATCH 2/3] util/fifo8: Use fifo8_reset() in fifo8_create()

[Mark Cave-Ayland]

On 19/07/2024 16:16, Philippe Mathieu-Daudé wrote:

> Avoid open-coding fifo8_reset() in fifo8_create().
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   util/fifo8.c | 15 +++++++--------
>   1 file changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/util/fifo8.c b/util/fifo8.c
> index 4e01b532d9..2925fe5611 100644
> --- a/util/fifo8.c
> +++ b/util/fifo8.c
> @@ -16,12 +16,17 @@
>   #include "migration/vmstate.h"
>   #include "qemu/fifo8.h"
>   
> +void fifo8_reset(Fifo8 *fifo)
> +{
> +    fifo->num = 0;
> +    fifo->head = 0;
> +}
> +
>   void fifo8_create(Fifo8 *fifo, uint32_t capacity)
>   {
>       fifo->data = g_new(uint8_t, capacity);
>       fifo->capacity = capacity;
> -    fifo->head = 0;
> -    fifo->num = 0;
> +    fifo8_reset(fifo);
>   }
>   
>   void fifo8_destroy(Fifo8 *fifo)
> @@ -97,12 +102,6 @@ const uint8_t *fifo8_pop_buf(Fifo8 *fifo, uint32_t max, uint32_t *numptr)
>       return fifo8_peekpop_buf(fifo, max, numptr, true);
>   }
>   
> -void fifo8_reset(Fifo8 *fifo)
> -{
> -    fifo->num = 0;
> -    fifo->head = 0;
> -}
> -
>   bool fifo8_is_empty(Fifo8 *fifo)
>   {
>       return (fifo->num == 0);

Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


ATB,

Mark.

[PATCH 3/3] util/fifo8: Introduce fifo8_change_capacity()

[Philippe Mathieu-Daudé]

FIFOs can be resized at runtime. Introduce the
fifo8_change_capacity() method to do that.
When capacity is changed, the FIFO must be reset.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/qemu/fifo8.h | 10 ++++++++++
 util/fifo8.c         |  7 +++++++
 2 files changed, 17 insertions(+)

diff --git a/include/qemu/fifo8.h b/include/qemu/fifo8.h
index c6295c6ff0..9fe0555a24 100644
--- a/include/qemu/fifo8.h
+++ b/include/qemu/fifo8.h
@@ -31,6 +31,16 @@ void fifo8_create(Fifo8 *fifo, uint32_t capacity);
 
 void fifo8_destroy(Fifo8 *fifo);
 
+/**
+ * fifo8_change_capacity:
+ * @fifo: struct Fifo8 to change the capacity
+ * @capacity: new capacity of the FIFO
+ *
+ * Change a FIFO capacity to the specified size. The FIFO is reset.
+ */
+
+void fifo8_change_capacity(Fifo8 *fifo, uint32_t capacity);
+
 /**
  * fifo8_push:
  * @fifo: FIFO to push to
diff --git a/util/fifo8.c b/util/fifo8.c
index 2925fe5611..c453afd774 100644
--- a/util/fifo8.c
+++ b/util/fifo8.c
@@ -34,6 +34,13 @@ void fifo8_destroy(Fifo8 *fifo)
     g_free(fifo->data);
 }
 
+void fifo8_change_capacity(Fifo8 *fifo, uint32_t capacity)
+{
+    fifo->data = g_renew(uint8_t, fifo->data, capacity);
+    fifo->capacity = capacity;
+    fifo8_reset(fifo);
+}
+
 void fifo8_push(Fifo8 *fifo, uint8_t data)
 {
     assert(fifo->num < fifo->capacity);
-- 
2.41.0

Re: [PATCH 3/3] util/fifo8: Introduce fifo8_change_capacity()

[Mark Cave-Ayland]

On 19/07/2024 16:16, Philippe Mathieu-Daudé wrote:

> FIFOs can be resized at runtime. Introduce the
> fifo8_change_capacity() method to do that.
> When capacity is changed, the FIFO must be reset.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   include/qemu/fifo8.h | 10 ++++++++++
>   util/fifo8.c         |  7 +++++++
>   2 files changed, 17 insertions(+)
> 
> diff --git a/include/qemu/fifo8.h b/include/qemu/fifo8.h
> index c6295c6ff0..9fe0555a24 100644
> --- a/include/qemu/fifo8.h
> +++ b/include/qemu/fifo8.h
> @@ -31,6 +31,16 @@ void fifo8_create(Fifo8 *fifo, uint32_t capacity);
>   
>   void fifo8_destroy(Fifo8 *fifo);
>   
> +/**
> + * fifo8_change_capacity:
> + * @fifo: struct Fifo8 to change the capacity
> + * @capacity: new capacity of the FIFO
> + *
> + * Change a FIFO capacity to the specified size. The FIFO is reset.
> + */
> +
> +void fifo8_change_capacity(Fifo8 *fifo, uint32_t capacity);
> +
>   /**
>    * fifo8_push:
>    * @fifo: FIFO to push to
> diff --git a/util/fifo8.c b/util/fifo8.c
> index 2925fe5611..c453afd774 100644
> --- a/util/fifo8.c
> +++ b/util/fifo8.c
> @@ -34,6 +34,13 @@ void fifo8_destroy(Fifo8 *fifo)
>       g_free(fifo->data);
>   }
>   
> +void fifo8_change_capacity(Fifo8 *fifo, uint32_t capacity)
> +{
> +    fifo->data = g_renew(uint8_t, fifo->data, capacity);
> +    fifo->capacity = capacity;
> +    fifo8_reset(fifo);
> +}
> +
>   void fifo8_push(Fifo8 *fifo, uint8_t data)
>   {
>       assert(fifo->num < fifo->capacity);

The changes look okay, however I'm a little confused as to why this is needed as 
generally hardware FIFOs are a fixed size? Presumably this is related to the PL011 
series?


ATB,

Mark.

Re: [PATCH 3/3] util/fifo8: Introduce fifo8_change_capacity()

[Philippe Mathieu-Daudé]

On 19/7/24 22:21, Mark Cave-Ayland wrote:
> On 19/07/2024 16:16, Philippe Mathieu-Daudé wrote:
> 
>> FIFOs can be resized at runtime. Introduce the
>> fifo8_change_capacity() method to do that.
>> When capacity is changed, the FIFO must be reset.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   include/qemu/fifo8.h | 10 ++++++++++
>>   util/fifo8.c         |  7 +++++++
>>   2 files changed, 17 insertions(+)
>>
>> diff --git a/include/qemu/fifo8.h b/include/qemu/fifo8.h
>> index c6295c6ff0..9fe0555a24 100644
>> --- a/include/qemu/fifo8.h
>> +++ b/include/qemu/fifo8.h
>> @@ -31,6 +31,16 @@ void fifo8_create(Fifo8 *fifo, uint32_t capacity);
>>   void fifo8_destroy(Fifo8 *fifo);
>> +/**
>> + * fifo8_change_capacity:
>> + * @fifo: struct Fifo8 to change the capacity
>> + * @capacity: new capacity of the FIFO
>> + *
>> + * Change a FIFO capacity to the specified size. The FIFO is reset.
>> + */
>> +
>> +void fifo8_change_capacity(Fifo8 *fifo, uint32_t capacity);
>> +
>>   /**
>>    * fifo8_push:
>>    * @fifo: FIFO to push to
>> diff --git a/util/fifo8.c b/util/fifo8.c
>> index 2925fe5611..c453afd774 100644
>> --- a/util/fifo8.c
>> +++ b/util/fifo8.c
>> @@ -34,6 +34,13 @@ void fifo8_destroy(Fifo8 *fifo)
>>       g_free(fifo->data);
>>   }
>> +void fifo8_change_capacity(Fifo8 *fifo, uint32_t capacity)
>> +{
>> +    fifo->data = g_renew(uint8_t, fifo->data, capacity);
>> +    fifo->capacity = capacity;
>> +    fifo8_reset(fifo);
>> +}
>> +
>>   void fifo8_push(Fifo8 *fifo, uint8_t data)
>>   {
>>       assert(fifo->num < fifo->capacity);
> 
> The changes look okay, however I'm a little confused as to why this is 
> needed as generally hardware FIFOs are a fixed size? Presumably this is 
> related to the PL011 series?

Indeed, this is to model trying to stay as close as possible to
the datasheet, which states:

   2.4.3 UART operation

     Disabling the FIFOs

       Additionally, you can disable the FIFOs. In this case,
       the transmit and receive sides of the UART have 1-byte
       holding registers (the bottom entry of the FIFOs). The
       overrun bit is set when a word has been received, and
       the previous one was not yet read. In this implementation,
       the FIFOs are not physically disabled, but the flags are
       manipulated to give the illusion of a 1-byte register.

Re: [PATCH 3/3] util/fifo8: Introduce fifo8_change_capacity()

[Peter Maydell]

On Mon, 22 Jul 2024 at 11:56, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> On 19/7/24 22:21, Mark Cave-Ayland wrote:
> > On 19/07/2024 16:16, Philippe Mathieu-Daudé wrote:
> >
> >> FIFOs can be resized at runtime. Introduce the
> >> fifo8_change_capacity() method to do that.
> >> When capacity is changed, the FIFO must be reset.
> >>
> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> >> ---
> >>   include/qemu/fifo8.h | 10 ++++++++++
> >>   util/fifo8.c         |  7 +++++++
> >>   2 files changed, 17 insertions(+)
> >>
> >> diff --git a/include/qemu/fifo8.h b/include/qemu/fifo8.h
> >> index c6295c6ff0..9fe0555a24 100644
> >> --- a/include/qemu/fifo8.h
> >> +++ b/include/qemu/fifo8.h
> >> @@ -31,6 +31,16 @@ void fifo8_create(Fifo8 *fifo, uint32_t capacity);
> >>   void fifo8_destroy(Fifo8 *fifo);
> >> +/**
> >> + * fifo8_change_capacity:
> >> + * @fifo: struct Fifo8 to change the capacity
> >> + * @capacity: new capacity of the FIFO
> >> + *
> >> + * Change a FIFO capacity to the specified size. The FIFO is reset.
> >> + */
> >> +
> >> +void fifo8_change_capacity(Fifo8 *fifo, uint32_t capacity);
> >> +
> >>   /**
> >>    * fifo8_push:
> >>    * @fifo: FIFO to push to
> >> diff --git a/util/fifo8.c b/util/fifo8.c
> >> index 2925fe5611..c453afd774 100644
> >> --- a/util/fifo8.c
> >> +++ b/util/fifo8.c
> >> @@ -34,6 +34,13 @@ void fifo8_destroy(Fifo8 *fifo)
> >>       g_free(fifo->data);
> >>   }
> >> +void fifo8_change_capacity(Fifo8 *fifo, uint32_t capacity)
> >> +{
> >> +    fifo->data = g_renew(uint8_t, fifo->data, capacity);
> >> +    fifo->capacity = capacity;
> >> +    fifo8_reset(fifo);
> >> +}
> >> +
> >>   void fifo8_push(Fifo8 *fifo, uint8_t data)
> >>   {
> >>       assert(fifo->num < fifo->capacity);
> >
> > The changes look okay, however I'm a little confused as to why this is
> > needed as generally hardware FIFOs are a fixed size? Presumably this is
> > related to the PL011 series?
>
> Indeed, this is to model trying to stay as close as possible to
> the datasheet, which states:
>
>    2.4.3 UART operation
>
>      Disabling the FIFOs
>
>        Additionally, you can disable the FIFOs. In this case,
>        the transmit and receive sides of the UART have 1-byte
>        holding registers (the bottom entry of the FIFOs). The
>        overrun bit is set when a word has been received, and
>        the previous one was not yet read. In this implementation,
>        the FIFOs are not physically disabled, but the flags are
>        manipulated to give the illusion of a 1-byte register.

Notice that in the hardware we don't actually resize the FIFO,
though -- we just only ever keep one element in it...

thanks
-- PMM

Re: [PATCH 3/3] util/fifo8: Introduce fifo8_change_capacity()

[Philippe Mathieu-Daudé]

On 22/7/24 13:52, Peter Maydell wrote:
> On Mon, 22 Jul 2024 at 11:56, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> On 19/7/24 22:21, Mark Cave-Ayland wrote:
>>> On 19/07/2024 16:16, Philippe Mathieu-Daudé wrote:
>>>
>>>> FIFOs can be resized at runtime. Introduce the
>>>> fifo8_change_capacity() method to do that.
>>>> When capacity is changed, the FIFO must be reset.
>>>>
>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>> ---
>>>>    include/qemu/fifo8.h | 10 ++++++++++
>>>>    util/fifo8.c         |  7 +++++++
>>>>    2 files changed, 17 insertions(+)
>>>>
>>>> diff --git a/include/qemu/fifo8.h b/include/qemu/fifo8.h
>>>> index c6295c6ff0..9fe0555a24 100644
>>>> --- a/include/qemu/fifo8.h
>>>> +++ b/include/qemu/fifo8.h
>>>> @@ -31,6 +31,16 @@ void fifo8_create(Fifo8 *fifo, uint32_t capacity);
>>>>    void fifo8_destroy(Fifo8 *fifo);
>>>> +/**
>>>> + * fifo8_change_capacity:
>>>> + * @fifo: struct Fifo8 to change the capacity
>>>> + * @capacity: new capacity of the FIFO
>>>> + *
>>>> + * Change a FIFO capacity to the specified size. The FIFO is reset.
>>>> + */
>>>> +
>>>> +void fifo8_change_capacity(Fifo8 *fifo, uint32_t capacity);
>>>> +
>>>>    /**
>>>>     * fifo8_push:
>>>>     * @fifo: FIFO to push to
>>>> diff --git a/util/fifo8.c b/util/fifo8.c
>>>> index 2925fe5611..c453afd774 100644
>>>> --- a/util/fifo8.c
>>>> +++ b/util/fifo8.c
>>>> @@ -34,6 +34,13 @@ void fifo8_destroy(Fifo8 *fifo)
>>>>        g_free(fifo->data);
>>>>    }
>>>> +void fifo8_change_capacity(Fifo8 *fifo, uint32_t capacity)
>>>> +{
>>>> +    fifo->data = g_renew(uint8_t, fifo->data, capacity);
>>>> +    fifo->capacity = capacity;
>>>> +    fifo8_reset(fifo);
>>>> +}
>>>> +
>>>>    void fifo8_push(Fifo8 *fifo, uint8_t data)
>>>>    {
>>>>        assert(fifo->num < fifo->capacity);
>>>
>>> The changes look okay, however I'm a little confused as to why this is
>>> needed as generally hardware FIFOs are a fixed size? Presumably this is
>>> related to the PL011 series?
>>
>> Indeed, this is to model trying to stay as close as possible to
>> the datasheet, which states:
>>
>>     2.4.3 UART operation
>>
>>       Disabling the FIFOs
>>
>>         Additionally, you can disable the FIFOs. In this case,
>>         the transmit and receive sides of the UART have 1-byte
>>         holding registers (the bottom entry of the FIFOs). The
>>         overrun bit is set when a word has been received, and
>>         the previous one was not yet read. In this implementation,
>>         the FIFOs are not physically disabled, but the flags are
>>         manipulated to give the illusion of a 1-byte register.
> 
> Notice that in the hardware we don't actually resize the FIFO,
> though -- we just only ever keep one element in it...

Yes. IIUC Mark's comment in the PL011 series, we don't need
this (confusing) method at all:
https://lore.kernel.org/qemu-devel/4e6b616f-3acc-4130-9b92-1af7fed540da@ilande.co.uk/