Add fifo32_peek() that returns the first element from the queue
without popping it.

Signed-off-by: Octavian Purdila <tavip@google.com>
---
 include/qemu/fifo32.h | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/include/qemu/fifo32.h b/include/qemu/fifo32.h
index 4e9fd1b5ef..9de1807375 100644
--- a/include/qemu/fifo32.h
+++ b/include/qemu/fifo32.h
@@ -140,6 +140,34 @@ static inline uint32_t fifo32_pop(Fifo32 *fifo)
     return ret;
 }
 
+/**
+ * fifo32_peek:
+ * @fifo: fifo to peek at
+ *
+ * Returns the value from the FIFO's head without poping it. Behaviour
+ * is undefined if the FIFO is empty. Clients are responsible for
+ * checking for emptiness using fifo32_is_empty().
+ *
+ * Returns: the value from the FIFO's head
+ */
+
+static inline uint32_t fifo32_peek(Fifo32 *fifo)
+{
+    uint32_t ret = 0, num;
+    const uint8_t *buf;
+
+    buf = fifo8_peek_bufptr(&fifo->fifo, 4, &num);
+    if (num != 4) {
+        return ret;
+    }
+
+    for (int i = 0; i < sizeof(uint32_t); i++) {
+        ret |= buf[i] << (i * 8);
+    }
+
+    return ret;
+}
+
 /**
  * There is no fifo32_pop_buf() because the data is not stored in the buffer
  * as a set of native-order words.
-- 
2.47.0.rc0.187.ge670bccf7e-goog

On 08/10/2024 02:18, Octavian Purdila wrote:

> Add fifo32_peek() that returns the first element from the queue
> without popping it.
> 
> Signed-off-by: Octavian Purdila <tavip@google.com>
> ---
>   include/qemu/fifo32.h | 28 ++++++++++++++++++++++++++++
>   1 file changed, 28 insertions(+)
> 
> diff --git a/include/qemu/fifo32.h b/include/qemu/fifo32.h
> index 4e9fd1b5ef..9de1807375 100644
> --- a/include/qemu/fifo32.h
> +++ b/include/qemu/fifo32.h
> @@ -140,6 +140,34 @@ static inline uint32_t fifo32_pop(Fifo32 *fifo)
>       return ret;
>   }
>   
> +/**
> + * fifo32_peek:
> + * @fifo: fifo to peek at
> + *
> + * Returns the value from the FIFO's head without poping it. Behaviour
> + * is undefined if the FIFO is empty. Clients are responsible for
> + * checking for emptiness using fifo32_is_empty().
> + *
> + * Returns: the value from the FIFO's head
> + */
> +
> +static inline uint32_t fifo32_peek(Fifo32 *fifo)
> +{
> +    uint32_t ret = 0, num;
> +    const uint8_t *buf;
> +
> +    buf = fifo8_peek_bufptr(&fifo->fifo, 4, &num);

Are you sure that you want to use fifo8_peek_bufptr() as opposed to fifo8_peek_buf() 
here? The reason for using the latter function (and why fifo8_*_bufptr() functions 
are not generally recommended) is that they will correctly handle the FIFO wraparound 
caused by the drifting head pointer which can occur if you don't empty the entire 
FIFO contents in a single *_pop() or *_pop_buf() operation.

> +    if (num != 4) {
> +        return ret;
> +    }
> +
> +    for (int i = 0; i < sizeof(uint32_t); i++) {
> +        ret |= buf[i] << (i * 8);
> +    }
> +
> +    return ret;
> +}
> +
>   /**
>    * There is no fifo32_pop_buf() because the data is not stored in the buffer
>    * as a set of native-order words.


ATB,

Mark.

On Tue, Oct 8, 2024 at 4:27 AM Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> On 08/10/2024 02:18, Octavian Purdila wrote:
>
> > Add fifo32_peek() that returns the first element from the queue
> > without popping it.
> >
> > Signed-off-by: Octavian Purdila <tavip@google.com>
> > ---
> >   include/qemu/fifo32.h | 28 ++++++++++++++++++++++++++++
> >   1 file changed, 28 insertions(+)
> >
> > diff --git a/include/qemu/fifo32.h b/include/qemu/fifo32.h
> > index 4e9fd1b5ef..9de1807375 100644
> > --- a/include/qemu/fifo32.h
> > +++ b/include/qemu/fifo32.h
> > @@ -140,6 +140,34 @@ static inline uint32_t fifo32_pop(Fifo32 *fifo)
> >       return ret;
> >   }
> >
> > +/**
> > + * fifo32_peek:
> > + * @fifo: fifo to peek at
> > + *
> > + * Returns the value from the FIFO's head without poping it. Behaviour
> > + * is undefined if the FIFO is empty. Clients are responsible for
> > + * checking for emptiness using fifo32_is_empty().
> > + *
> > + * Returns: the value from the FIFO's head
> > + */
> > +
> > +static inline uint32_t fifo32_peek(Fifo32 *fifo)
> > +{
> > +    uint32_t ret = 0, num;
> > +    const uint8_t *buf;
> > +
> > +    buf = fifo8_peek_bufptr(&fifo->fifo, 4, &num);
>
> Are you sure that you want to use fifo8_peek_bufptr() as opposed to fifo8_peek_buf()
> here? The reason for using the latter function (and why fifo8_*_bufptr() functions
> are not generally recommended) is that they will correctly handle the FIFO wraparound
> caused by the drifting head pointer which can occur if you don't empty the entire
> FIFO contents in a single *_pop() or *_pop_buf() operation.
>

I don't think that it matters in this case because the size of the
FIFO is always going to be a multiple of 4 and all push and pop
operations happen with 4 bytes as well. Am I missing something?

In any case, if it makes things more clear / consistent I can switch
to fifo8_peek_buf.

> > +    if (num != 4) {
> > +        return ret;
> > +    }
> > +
> > +    for (int i = 0; i < sizeof(uint32_t); i++) {
> > +        ret |= buf[i] << (i * 8);
> > +    }
> > +
> > +    return ret;
> > +}
> > +
> >   /**
> >    * There is no fifo32_pop_buf() because the data is not stored in the buffer
> >    * as a set of native-order words.
>
>
> ATB,
>
> Mark.
>

On 08/10/2024 18:25, Octavian Purdila wrote:

> On Tue, Oct 8, 2024 at 4:27 AM Mark Cave-Ayland
> <mark.cave-ayland@ilande.co.uk> wrote:
>>
>> On 08/10/2024 02:18, Octavian Purdila wrote:
>>
>>> Add fifo32_peek() that returns the first element from the queue
>>> without popping it.
>>>
>>> Signed-off-by: Octavian Purdila <tavip@google.com>
>>> ---
>>>    include/qemu/fifo32.h | 28 ++++++++++++++++++++++++++++
>>>    1 file changed, 28 insertions(+)
>>>
>>> diff --git a/include/qemu/fifo32.h b/include/qemu/fifo32.h
>>> index 4e9fd1b5ef..9de1807375 100644
>>> --- a/include/qemu/fifo32.h
>>> +++ b/include/qemu/fifo32.h
>>> @@ -140,6 +140,34 @@ static inline uint32_t fifo32_pop(Fifo32 *fifo)
>>>        return ret;
>>>    }
>>>
>>> +/**
>>> + * fifo32_peek:
>>> + * @fifo: fifo to peek at
>>> + *
>>> + * Returns the value from the FIFO's head without poping it. Behaviour
>>> + * is undefined if the FIFO is empty. Clients are responsible for
>>> + * checking for emptiness using fifo32_is_empty().
>>> + *
>>> + * Returns: the value from the FIFO's head
>>> + */
>>> +
>>> +static inline uint32_t fifo32_peek(Fifo32 *fifo)
>>> +{
>>> +    uint32_t ret = 0, num;
>>> +    const uint8_t *buf;
>>> +
>>> +    buf = fifo8_peek_bufptr(&fifo->fifo, 4, &num);
>>
>> Are you sure that you want to use fifo8_peek_bufptr() as opposed to fifo8_peek_buf()
>> here? The reason for using the latter function (and why fifo8_*_bufptr() functions
>> are not generally recommended) is that they will correctly handle the FIFO wraparound
>> caused by the drifting head pointer which can occur if you don't empty the entire
>> FIFO contents in a single *_pop() or *_pop_buf() operation.
>>
> 
> I don't think that it matters in this case because the size of the
> FIFO is always going to be a multiple of 4 and all push and pop
> operations happen with 4 bytes as well. Am I missing something?
> 
> In any case, if it makes things more clear / consistent I can switch
> to fifo8_peek_buf.

I'm guess I'm just a little bit wary of the Fifo32 API since it appears that 
fifo32_num_used(), fifo32_num_free() and fifo32_is_full() are written in a way that 
suggests unaligned accesses can occur.

Given that fifo8_push() and fifo8_pop() should assert() upon failure I don't think 
that's possible for Fifo32, but then all my test cases use Fifo8.

If you're confident from your tests that this can't happen then we can leave it as-is.


ATB,

Mark.