[PATCH 0/2] Series of fixes for PL011 char device

[PATCH 0/2] Series of fixes for PL011 char device

[Evgeny Iakovlev]

Evgeny Iakovlev (2):
  hw/char/pl011: better handling of FIFO flags on LCR reset
  hw/char/pl011: check if UART is enabled before RX or TX operation

 hw/char/pl011.c         | 51 ++++++++++++++++++++++++++++++-----------
 include/hw/char/pl011.h |  5 +++-
 2 files changed, 41 insertions(+), 15 deletions(-)

-- 
2.34.1

[PATCH 1/2] hw/char/pl011: better handling of FIFO flags on LCR reset

[Evgeny Iakovlev]

Current FIFO handling code does not reset RXFE/RXFF flags when guest
resets FIFO by writing to UARTLCR register, although internal FIFO state
is reset to 0 read count. Actual flag update will happen only on next
read or write to UART. As a result of that any guest that expects RXFE
flag to be set (and RXFF to be cleared) after resetting FIFO will just
hang.

Correctly reset FIFO flags on FIFO reset. Also, clean up some FIFO
depth handling code based on current FIFO mode.

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

diff --git a/hw/char/pl011.c b/hw/char/pl011.c
index c076813423..9108ed2be9 100644
--- a/hw/char/pl011.c
+++ b/hw/char/pl011.c
@@ -81,6 +81,18 @@ static void pl011_update(PL011State *s)
     }
 }
 
+static inline unsigned pl011_get_fifo_depth(PL011State *s)
+{
+    return s->lcr & 0x10 ? PL011_FIFO_DEPTH : 1;
+}
+
+static inline void pl011_reset_pipe(PL011State *s)
+{
+    s->read_count = 0;
+    s->read_pos = 0;
+    s->flags = PL011_FLAG_RXFE | PL011_FLAG_TXFE;
+}
+
 static uint64_t pl011_read(void *opaque, hwaddr offset,
                            unsigned size)
 {
@@ -94,7 +106,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)
+            if (++s->read_pos == PL011_FIFO_DEPTH)
                 s->read_pos = 0;
         }
         if (s->read_count == 0) {
@@ -229,8 +241,7 @@ static void pl011_write(void *opaque, hwaddr offset,
     case 11: /* UARTLCR_H */
         /* Reset the FIFO state on FIFO enable or disable */
         if ((s->lcr ^ value) & 0x10) {
-            s->read_count = 0;
-            s->read_pos = 0;
+            pl011_reset_pipe(s);
         }
         if ((s->lcr ^ value) & 0x1) {
             int break_enable = value & 0x1;
@@ -273,11 +284,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 +293,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;
     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 +366,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),
@@ -399,7 +406,7 @@ static void pl011_init(Object *obj)
     s->read_trigger = 1;
     s->ifl = 0x12;
     s->cr = 0x300;
-    s->flags = 0x90;
+    pl011_reset_pipe(s);
 
     s->id = pl011_id_arm;
 }
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

Re: [PATCH 1/2] hw/char/pl011: better handling of FIFO flags on LCR reset

[Peter Maydell]

On Fri, 6 Jan 2023 at 17:28, Evgeny Iakovlev
<eiakovlev@linux.microsoft.com> wrote:
>
> Current FIFO handling code does not reset RXFE/RXFF flags when guest
> resets FIFO by writing to UARTLCR register, although internal FIFO state
> is reset to 0 read count. Actual flag update will happen only on next
> read or write to UART. As a result of that any guest that expects RXFE
> flag to be set (and RXFF to be cleared) after resetting FIFO will just
> hang.
>
> Correctly reset FIFO flags on FIFO reset. Also, clean up some FIFO
> depth handling code based on current FIFO mode.

This patch is doing multiple things at once ("also" in a
commit message is often a sign of that) and I think it
would be helpful to split it. There are three things here:
 * refactorings which aren't making any real code change
   (eg calling pl011_get_fifo_depth() instead of doing the
   "if LCR bit set then 16 else 1" inline)
 * the fix to the actual bug
 * changes to the handling of the FIFO which should in theory
   not be visible to the guest (I think it now when the FIFO
   is disabled always puts the incoming character in read_fifo[0],
   whereas previously it would allow read_pos to increment all
   the way around the FIFO even though we only keep one character
   at a time).

Type 3 in particular is tricky and could use a commit message
explaining what it's doing.

I think the actual code changes are OK, but the FIFO handling
change is a bit complicated and at first I thought it had
introduced a bug.

thanks
-- PMM

Re: [PATCH 1/2] hw/char/pl011: better handling of FIFO flags on LCR reset

[Evgeny Iakovlev]

On 1/17/2023 16:24, Peter Maydell wrote:
> On Fri, 6 Jan 2023 at 17:28, Evgeny Iakovlev
> <eiakovlev@linux.microsoft.com> wrote:
>> Current FIFO handling code does not reset RXFE/RXFF flags when guest
>> resets FIFO by writing to UARTLCR register, although internal FIFO state
>> is reset to 0 read count. Actual flag update will happen only on next
>> read or write to UART. As a result of that any guest that expects RXFE
>> flag to be set (and RXFF to be cleared) after resetting FIFO will just
>> hang.
>>
>> Correctly reset FIFO flags on FIFO reset. Also, clean up some FIFO
>> depth handling code based on current FIFO mode.
> This patch is doing multiple things at once ("also" in a
> commit message is often a sign of that) and I think it
> would be helpful to split it. There are three things here:
>   * refactorings which aren't making any real code change
>     (eg calling pl011_get_fifo_depth() instead of doing the
>     "if LCR bit set then 16 else 1" inline)


Yeah, tbh i also though i should do it..


>   * the fix to the actual bug
>   * changes to the handling of the FIFO which should in theory
>     not be visible to the guest (I think it now when the FIFO
>     is disabled always puts the incoming character in read_fifo[0],
>     whereas previously it would allow read_pos to increment all
>     the way around the FIFO even though we only keep one character
>     at a time).


That last part i don't understand. If by changes to the FIFO you are 
referring to the flags handling, then it will be visible to the guest by 
design, since that's what I'm fixing. Could you maybe explain that one 
again? :)


>
> Type 3 in particular is tricky and could use a commit message
> explaining what it's doing.
>
> I think the actual code changes are OK, but the FIFO handling
> change is a bit complicated and at first I thought it had
> introduced a bug.
>
> thanks
> -- PMM

Re: [PATCH 1/2] hw/char/pl011: better handling of FIFO flags on LCR reset

[Peter Maydell]

On Tue, 17 Jan 2023 at 15:54, Evgeny Iakovlev
<eiakovlev@linux.microsoft.com> wrote:
>
>
> On 1/17/2023 16:24, Peter Maydell wrote:
> > On Fri, 6 Jan 2023 at 17:28, Evgeny Iakovlev
> > <eiakovlev@linux.microsoft.com> wrote:
> >> Current FIFO handling code does not reset RXFE/RXFF flags when guest
> >> resets FIFO by writing to UARTLCR register, although internal FIFO state
> >> is reset to 0 read count. Actual flag update will happen only on next
> >> read or write to UART. As a result of that any guest that expects RXFE
> >> flag to be set (and RXFF to be cleared) after resetting FIFO will just
> >> hang.
> >>
> >> Correctly reset FIFO flags on FIFO reset. Also, clean up some FIFO
> >> depth handling code based on current FIFO mode.
> > This patch is doing multiple things at once ("also" in a
> > commit message is often a sign of that) and I think it
> > would be helpful to split it. There are three things here:
> >   * refactorings which aren't making any real code change
> >     (eg calling pl011_get_fifo_depth() instead of doing the
> >     "if LCR bit set then 16 else 1" inline)
>
>
> Yeah, tbh i also though i should do it..
>
>
> >   * the fix to the actual bug
> >   * changes to the handling of the FIFO which should in theory
> >     not be visible to the guest (I think it now when the FIFO
> >     is disabled always puts the incoming character in read_fifo[0],
> >     whereas previously it would allow read_pos to increment all
> >     the way around the FIFO even though we only keep one character
> >     at a time).
>
>
> That last part i don't understand. If by changes to the FIFO you are
> referring to the flags handling, then it will be visible to the guest by
> design, since that's what I'm fixing. Could you maybe explain that one
> again? :)

I meant the bit where the existing code for the FIFO-disabled
case puts incoming characters in s->read_fifo[s->read_pos] and
reads from UARTDR always increment s->read_pos; whereas the
changed code always puts characters in s->read_fifo[0] and
avoids incrementing read_pos. I think your version is more
sensible (and also matches what the TRM claims the hardware
is doing), although the guest-visible behaviour doesn't change.

thanks
-- PMM

Re: [PATCH 1/2] hw/char/pl011: better handling of FIFO flags on LCR reset

[eiakovlev]

On 1/17/23 5:02 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On Tue, 17 Jan 2023 at 15:54, Evgeny Iakovlev
> <eiakovlev@linux.microsoft.com> wrote:
> >
> >
> > On 1/17/2023 16:24, Peter Maydell wrote:
> >> On Fri, 6 Jan 2023 at 17:28, Evgeny Iakovlev
> >> <eiakovlev@linux.microsoft.com> wrote:
> >>> Current FIFO handling code does not reset RXFE/RXFF flags when guest
> >>> resets FIFO by writing to UARTLCR register, although internal FIFO state
> >>> is reset to 0 read count. Actual flag update will happen only on next
> >>> read or write to UART. As a result of that any guest that expects RXFE
> >>> flag to be set (and RXFF to be cleared) after resetting FIFO will just
> >>> hang.
> >>>
> >>> Correctly reset FIFO flags on FIFO reset. Also, clean up some FIFO
> >>> depth handling code based on current FIFO mode.
> >> This patch is doing multiple things at once ("also" in a
> >> commit message is often a sign of that) and I think it
> >> would be helpful to split it. There are three things here:
> >>    * refactorings which aren't making any real code change
> >>      (eg calling pl011_get_fifo_depth() instead of doing the
> >>      "if LCR bit set then 16 else 1" inline)
> >
> >
> > Yeah, tbh i also though i should do it..
> >
> >
> >>    * the fix to the actual bug
> >>    * changes to the handling of the FIFO which should in theory
> >>      not be visible to the guest (I think it now when the FIFO
> >>      is disabled always puts the incoming character in read_fifo[0],
> >>      whereas previously it would allow read_pos to increment all
> >>      the way around the FIFO even though we only keep one character
> >>      at a time).
> >
> >
> > That last part i don't understand. If by changes to the FIFO you are
> > referring to the flags handling, then it will be visible to the guest by
> > design, since that's what I'm fixing. Could you maybe explain that one
> > again? :)
> 
> I meant the bit where the existing code for the FIFO-disabled
> case puts incoming characters in s->read_fifo[s->read_pos] and
> reads from UARTDR always increment s->read_pos; whereas the
> changed code always puts characters in s->read_fifo[0] and
> avoids incrementing read_pos. I think your version is more
> sensible (and also matches what the TRM claims the hardware
> is doing), although the guest-visible behaviour doesn't change.

I see, thanks. I tried separating this particular piece into its own commit, but i just feel like it makes the part that's left pointless, because pl011_get_fifo_depth replaces mostly just those 2 occurences anyway.. I've added a paragraph to a commit message to document a functional change. Let me know if that's not good enough. I've sent out a v2 with the rest of comments addressed + a reset method for PL011.

> 
> thanks
> -- PMM
> 

[PATCH 2/2] hw/char/pl011: check if UART is enabled before RX or TX operation

[Evgeny Iakovlev]

UART should be enabled in general and have RX enabled specifically to be
able to receive data from peripheral device. Same goes for transmitting
data to peripheral device and a TXE flag.

Check if UART CR register has EN and RXE or TXE bits enabled before
trying to receive or transmit data.

Signed-off-by: Evgeny Iakovlev <eiakovlev@linux.microsoft.com>
---
 hw/char/pl011.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/hw/char/pl011.c b/hw/char/pl011.c
index 9108ed2be9..fcc2600944 100644
--- a/hw/char/pl011.c
+++ b/hw/char/pl011.c
@@ -54,6 +54,11 @@
 #define INT_E (INT_OE | INT_BE | INT_PE | INT_FE)
 #define INT_MS (INT_RI | INT_DSR | INT_DCD | INT_CTS)
 
+/* UARTCR bits */
+#define PL011_CR_UARTEN (1 << 0)
+#define PL011_CR_TXE    (1 << 8)
+#define PL011_CR_RXE    (1 << 9)
+
 static const unsigned char pl011_id_arm[8] =
   { 0x11, 0x10, 0x14, 0x00, 0x0d, 0xf0, 0x05, 0xb1 };
 static const unsigned char pl011_id_luminary[8] =
@@ -203,6 +208,11 @@ static void pl011_trace_baudrate_change(const PL011State *s)
                                 s->ibrd, s->fbrd);
 }
 
+static inline bool pl011_can_transmit(PL011State *s)
+{
+    return s->cr & PL011_CR_UARTEN && s->cr & PL011_CR_TXE;
+}
+
 static void pl011_write(void *opaque, hwaddr offset,
                         uint64_t value, unsigned size)
 {
@@ -213,7 +223,9 @@ static void pl011_write(void *opaque, hwaddr offset,
 
     switch (offset >> 2) {
     case 0: /* UARTDR */
-        /* ??? Check if transmitter is enabled.  */
+        if (!pl011_can_transmit(s)) {
+            break;
+        }
         ch = value;
         /* XXX this blocks entire thread. Rewrite to use
          * qemu_chr_fe_write and background I/O callbacks */
@@ -284,7 +296,11 @@ static int pl011_can_receive(void *opaque)
     PL011State *s = (PL011State *)opaque;
     int r;
 
-    r = s->read_count < pl011_get_fifo_depth(s);
+    if (!(s->cr & PL011_CR_UARTEN) || !(s->cr & PL011_CR_RXE)) {
+        r = 0;
+    } else {
+        r = s->read_count < pl011_get_fifo_depth(s);
+    }
     trace_pl011_can_receive(s->lcr, s->read_count, r);
     return r;
 }
@@ -405,7 +421,7 @@ static void pl011_init(Object *obj)
 
     s->read_trigger = 1;
     s->ifl = 0x12;
-    s->cr = 0x300;
+    s->cr = PL011_CR_RXE | PL011_CR_TXE;
     pl011_reset_pipe(s);
 
     s->id = pl011_id_arm;
-- 
2.34.1

Re: [PATCH 2/2] hw/char/pl011: check if UART is enabled before RX or TX operation

[Peter Maydell]

On Fri, 6 Jan 2023 at 17:29, Evgeny Iakovlev
<eiakovlev@linux.microsoft.com> wrote:
>
> UART should be enabled in general and have RX enabled specifically to be
> able to receive data from peripheral device. Same goes for transmitting
> data to peripheral device and a TXE flag.
>
> Check if UART CR register has EN and RXE or TXE bits enabled before
> trying to receive or transmit data.

I wonder if the real hardware lets you fill the TX fifo
when TXE or EN are clear (and then transmits it when
you enable tx later)? That seems kind of an odd corner
case to implement, though, and the TRM doesn't specifically
say one way or the other, so it doesn't seem worth the bother.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

I notice this device doesn't have a reset method, incidentally,
which is probably something we should fix.

thanks
-- PMM