PL011 can be in either of 2 modes depending guest config: FIFO and
single register. The last mode could be viewed as a 1-element-deep FIFO.

Current code open-codes a bunch of depth-dependent logic. Refactor FIFO
depth handling code to isolate calculating current FIFO depth.

One functional (albeit guest-invisible) side-effect of this change is
that previously we would always increment s->read_pos in UARTDR read
handler even if FIFO was disabled, now we are limiting read_pos to not
exceed FIFO depth (read_pos itself is reset to 0 if user disables FIFO).

Signed-off-by: Evgeny Iakovlev <eiakovlev@linux.microsoft.com>
---
 hw/char/pl011.c         | 25 +++++++++++++------------
 include/hw/char/pl011.h |  5 ++++-
 2 files changed, 17 insertions(+), 13 deletions(-)

diff --git a/hw/char/pl011.c b/hw/char/pl011.c
index c076813423..329cc6926d 100644
--- a/hw/char/pl011.c
+++ b/hw/char/pl011.c
@@ -81,6 +81,12 @@ static void pl011_update(PL011State *s)
     }
 }
 
+static inline unsigned pl011_get_fifo_depth(PL011State *s)
+{
+    /* Note: FIFO depth is expected to be power-of-2 */
+    return s->lcr & 0x10 ? PL011_FIFO_DEPTH : 1;
+}
+
 static uint64_t pl011_read(void *opaque, hwaddr offset,
                            unsigned size)
 {
@@ -94,8 +100,7 @@ static uint64_t pl011_read(void *opaque, hwaddr offset,
         c = s->read_fifo[s->read_pos];
         if (s->read_count > 0) {
             s->read_count--;
-            if (++s->read_pos == 16)
-                s->read_pos = 0;
+            s->read_pos = (s->read_pos + 1) & (pl011_get_fifo_depth(s) - 1);
         }
         if (s->read_count == 0) {
             s->flags |= PL011_FLAG_RXFE;
@@ -273,11 +278,7 @@ static int pl011_can_receive(void *opaque)
     PL011State *s = (PL011State *)opaque;
     int r;
 
-    if (s->lcr & 0x10) {
-        r = s->read_count < 16;
-    } else {
-        r = s->read_count < 1;
-    }
+    r = s->read_count < pl011_get_fifo_depth(s);
     trace_pl011_can_receive(s->lcr, s->read_count, r);
     return r;
 }
@@ -286,15 +287,15 @@ static void pl011_put_fifo(void *opaque, uint32_t value)
 {
     PL011State *s = (PL011State *)opaque;
     int slot;
+    unsigned pipe_depth;
 
-    slot = s->read_pos + s->read_count;
-    if (slot >= 16)
-        slot -= 16;
+    pipe_depth = pl011_get_fifo_depth(s);
+    slot = (s->read_pos + s->read_count) & (pipe_depth - 1);
     s->read_fifo[slot] = value;
     s->read_count++;
     s->flags &= ~PL011_FLAG_RXFE;
     trace_pl011_put_fifo(value, s->read_count);
-    if (!(s->lcr & 0x10) || s->read_count == 16) {
+    if (s->read_count == pipe_depth) {
         trace_pl011_put_fifo_full();
         s->flags |= PL011_FLAG_RXFF;
     }
@@ -359,7 +360,7 @@ static const VMStateDescription vmstate_pl011 = {
         VMSTATE_UINT32(dmacr, PL011State),
         VMSTATE_UINT32(int_enabled, PL011State),
         VMSTATE_UINT32(int_level, PL011State),
-        VMSTATE_UINT32_ARRAY(read_fifo, PL011State, 16),
+        VMSTATE_UINT32_ARRAY(read_fifo, PL011State, PL011_FIFO_DEPTH),
         VMSTATE_UINT32(ilpr, PL011State),
         VMSTATE_UINT32(ibrd, PL011State),
         VMSTATE_UINT32(fbrd, PL011State),
diff --git a/include/hw/char/pl011.h b/include/hw/char/pl011.h
index dc2c90eedc..926322e242 100644
--- a/include/hw/char/pl011.h
+++ b/include/hw/char/pl011.h
@@ -27,6 +27,9 @@ OBJECT_DECLARE_SIMPLE_TYPE(PL011State, PL011)
 /* This shares the same struct (and cast macro) as the base pl011 device */
 #define TYPE_PL011_LUMINARY "pl011_luminary"
 
+/* Depth of UART FIFO in bytes, when FIFO mode is enabled (else depth == 1) */
+#define PL011_FIFO_DEPTH 16
+
 struct PL011State {
     SysBusDevice parent_obj;
 
@@ -39,7 +42,7 @@ struct PL011State {
     uint32_t dmacr;
     uint32_t int_enabled;
     uint32_t int_level;
-    uint32_t read_fifo[16];
+    uint32_t read_fifo[PL011_FIFO_DEPTH];
     uint32_t ilpr;
     uint32_t ibrd;
     uint32_t fbrd;
-- 
2.34.1

On Tue, 17 Jan 2023 at 22:05, Evgeny Iakovlev
<eiakovlev@linux.microsoft.com> wrote:
>
> PL011 can be in either of 2 modes depending guest config: FIFO and
> single register. The last mode could be viewed as a 1-element-deep FIFO.
>
> Current code open-codes a bunch of depth-dependent logic. Refactor FIFO
> depth handling code to isolate calculating current FIFO depth.
>
> One functional (albeit guest-invisible) side-effect of this change is
> that previously we would always increment s->read_pos in UARTDR read
> handler even if FIFO was disabled, now we are limiting read_pos to not
> exceed FIFO depth (read_pos itself is reset to 0 if user disables FIFO).
>
> Signed-off-by: Evgeny Iakovlev <eiakovlev@linux.microsoft.com>
> ---
>  hw/char/pl011.c         | 25 +++++++++++++------------
>  include/hw/char/pl011.h |  5 ++++-
>  2 files changed, 17 insertions(+), 13 deletions(-)

Looking at this again, I realised that there's a subtle point
here about migration compatibility. If we do a VM migration
from an older version of QEMU without this change to a newer
version that does have this change, the incoming migration state
might indicate that we have FIFOs disabled, and there's a character
in read_fifo[] that isn't in array element 0 (because the old
code doesn't put it there). I think this works out OK because
the codepath in the UARTDR read-from-FIFO will first read the
character from read_fifo[read_pos], which will be the non-zero
read_pos as set by the old QEMU, before constraining it to be
0 when it does the advance of read_pos; and the pl011_put_fifo
code doesn't care about the actual value of read_pos.

But this is kind of tricky to reason about, and fragile to
future changes in the code, so I feel like it would be better
to have a migration post_load function that sanitizes the
incoming state to enforce the invariant assumed by the new code, i.e.

  if (pl011_fifo_depth(s) == 1 && s->read_count > 0 && s->read_pos > 0) {
      /*
       * Older versions of QEMU didn't ensure that the single
       * character in the FIFO in FIFO-disabled mode is in
       * element 0 of the array; convert to follow the current
       * code's assumptions.
       */
      s->read_fifo[0] = s->read_fifo[s->read_pos];
      s->read_pos = 0;
  }

If we're putting in a post-load function we can also sanitize
the incoming migration stream to fail the migration on bogus
(possibly malicious) data like read_pos > ARRAY_SIZE(read_fifo)
or read_count > fifo depth.

> diff --git a/hw/char/pl011.c b/hw/char/pl011.c
> index c076813423..329cc6926d 100644
> --- a/hw/char/pl011.c
> +++ b/hw/char/pl011.c
> @@ -81,6 +81,12 @@ static void pl011_update(PL011State *s)
>      }
>  }
>
> +static inline unsigned pl011_get_fifo_depth(PL011State *s)
> +{
> +    /* Note: FIFO depth is expected to be power-of-2 */
> +    return s->lcr & 0x10 ? PL011_FIFO_DEPTH : 1;
> +}
> +
>  static uint64_t pl011_read(void *opaque, hwaddr offset,
>                             unsigned size)
>  {
> @@ -94,8 +100,7 @@ static uint64_t pl011_read(void *opaque, hwaddr offset,
>          c = s->read_fifo[s->read_pos];
>          if (s->read_count > 0) {
>              s->read_count--;
> -            if (++s->read_pos == 16)
> -                s->read_pos = 0;
> +            s->read_pos = (s->read_pos + 1) & (pl011_get_fifo_depth(s) - 1);
>          }
>          if (s->read_count == 0) {
>              s->flags |= PL011_FLAG_RXFE;
> @@ -273,11 +278,7 @@ static int pl011_can_receive(void *opaque)
>      PL011State *s = (PL011State *)opaque;
>      int r;
>
> -    if (s->lcr & 0x10) {
> -        r = s->read_count < 16;
> -    } else {
> -        r = s->read_count < 1;
> -    }
> +    r = s->read_count < pl011_get_fifo_depth(s);
>      trace_pl011_can_receive(s->lcr, s->read_count, r);
>      return r;
>  }
> @@ -286,15 +287,15 @@ static void pl011_put_fifo(void *opaque, uint32_t value)
>  {
>      PL011State *s = (PL011State *)opaque;
>      int slot;
> +    unsigned pipe_depth;
>
> -    slot = s->read_pos + s->read_count;
> -    if (slot >= 16)
> -        slot -= 16;
> +    pipe_depth = pl011_get_fifo_depth(s);
> +    slot = (s->read_pos + s->read_count) & (pipe_depth - 1);
>      s->read_fifo[slot] = value;
>      s->read_count++;
>      s->flags &= ~PL011_FLAG_RXFE;
>      trace_pl011_put_fifo(value, s->read_count);
> -    if (!(s->lcr & 0x10) || s->read_count == 16) {
> +    if (s->read_count == pipe_depth) {
>          trace_pl011_put_fifo_full();
>          s->flags |= PL011_FLAG_RXFF;
>      }

thanks
-- PMM

On 1/19/2023 14:45, Peter Maydell wrote:
> On Tue, 17 Jan 2023 at 22:05, Evgeny Iakovlev
> <eiakovlev@linux.microsoft.com> wrote:
>> PL011 can be in either of 2 modes depending guest config: FIFO and
>> single register. The last mode could be viewed as a 1-element-deep FIFO.
>>
>> Current code open-codes a bunch of depth-dependent logic. Refactor FIFO
>> depth handling code to isolate calculating current FIFO depth.
>>
>> One functional (albeit guest-invisible) side-effect of this change is
>> that previously we would always increment s->read_pos in UARTDR read
>> handler even if FIFO was disabled, now we are limiting read_pos to not
>> exceed FIFO depth (read_pos itself is reset to 0 if user disables FIFO).
>>
>> Signed-off-by: Evgeny Iakovlev <eiakovlev@linux.microsoft.com>
>> ---
>>   hw/char/pl011.c         | 25 +++++++++++++------------
>>   include/hw/char/pl011.h |  5 ++++-
>>   2 files changed, 17 insertions(+), 13 deletions(-)
> Looking at this again, I realised that there's a subtle point
> here about migration compatibility. If we do a VM migration
> from an older version of QEMU without this change to a newer
> version that does have this change, the incoming migration state
> might indicate that we have FIFOs disabled, and there's a character
> in read_fifo[] that isn't in array element 0 (because the old
> code doesn't put it there). I think this works out OK because
> the codepath in the UARTDR read-from-FIFO will first read the
> character from read_fifo[read_pos], which will be the non-zero
> read_pos as set by the old QEMU, before constraining it to be
> 0 when it does the advance of read_pos; and the pl011_put_fifo
> code doesn't care about the actual value of read_pos.
>
> But this is kind of tricky to reason about, and fragile to
> future changes in the code, so I feel like it would be better
> to have a migration post_load function that sanitizes the
> incoming state to enforce the invariant assumed by the new code, i.e.
>
>    if (pl011_fifo_depth(s) == 1 && s->read_count > 0 && s->read_pos > 0) {
>        /*
>         * Older versions of QEMU didn't ensure that the single
>         * character in the FIFO in FIFO-disabled mode is in
>         * element 0 of the array; convert to follow the current
>         * code's assumptions.
>         */
>        s->read_fifo[0] = s->read_fifo[s->read_pos];
>        s->read_pos = 0;
>    }
>
> If we're putting in a post-load function we can also sanitize
> the incoming migration stream to fail the migration on bogus
> (possibly malicious) data like read_pos > ARRAY_SIZE(read_fifo)
> or read_count > fifo depth.


Yeah, i also saw this issue with migration and how it was not really a 
problem. I do agree with your point about making it more obviously fixed 
though.


>
>> diff --git a/hw/char/pl011.c b/hw/char/pl011.c
>> index c076813423..329cc6926d 100644
>> --- a/hw/char/pl011.c
>> +++ b/hw/char/pl011.c
>> @@ -81,6 +81,12 @@ static void pl011_update(PL011State *s)
>>       }
>>   }
>>
>> +static inline unsigned pl011_get_fifo_depth(PL011State *s)
>> +{
>> +    /* Note: FIFO depth is expected to be power-of-2 */
>> +    return s->lcr & 0x10 ? PL011_FIFO_DEPTH : 1;
>> +}
>> +
>>   static uint64_t pl011_read(void *opaque, hwaddr offset,
>>                              unsigned size)
>>   {
>> @@ -94,8 +100,7 @@ static uint64_t pl011_read(void *opaque, hwaddr offset,
>>           c = s->read_fifo[s->read_pos];
>>           if (s->read_count > 0) {
>>               s->read_count--;
>> -            if (++s->read_pos == 16)
>> -                s->read_pos = 0;
>> +            s->read_pos = (s->read_pos + 1) & (pl011_get_fifo_depth(s) - 1);
>>           }
>>           if (s->read_count == 0) {
>>               s->flags |= PL011_FLAG_RXFE;
>> @@ -273,11 +278,7 @@ static int pl011_can_receive(void *opaque)
>>       PL011State *s = (PL011State *)opaque;
>>       int r;
>>
>> -    if (s->lcr & 0x10) {
>> -        r = s->read_count < 16;
>> -    } else {
>> -        r = s->read_count < 1;
>> -    }
>> +    r = s->read_count < pl011_get_fifo_depth(s);
>>       trace_pl011_can_receive(s->lcr, s->read_count, r);
>>       return r;
>>   }
>> @@ -286,15 +287,15 @@ static void pl011_put_fifo(void *opaque, uint32_t value)
>>   {
>>       PL011State *s = (PL011State *)opaque;
>>       int slot;
>> +    unsigned pipe_depth;
>>
>> -    slot = s->read_pos + s->read_count;
>> -    if (slot >= 16)
>> -        slot -= 16;
>> +    pipe_depth = pl011_get_fifo_depth(s);
>> +    slot = (s->read_pos + s->read_count) & (pipe_depth - 1);
>>       s->read_fifo[slot] = value;
>>       s->read_count++;
>>       s->flags &= ~PL011_FLAG_RXFE;
>>       trace_pl011_put_fifo(value, s->read_count);
>> -    if (!(s->lcr & 0x10) || s->read_count == 16) {
>> +    if (s->read_count == pipe_depth) {
>>           trace_pl011_put_fifo_full();
>>           s->flags |= PL011_FLAG_RXFF;
>>       }
> thanks
> -- PMM