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

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

[Evgeny Iakovlev]

v2:
* Moved FIFO depth refactoring part of FIFO flags change into its own
  commit.
* Added a reset method for PL011

Evgeny Iakovlev (4):
  hw/char/pl011: refactor FIFO depth handling code
  hw/char/pl011: implement a reset method
  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         | 78 +++++++++++++++++++++++++++++++----------
 include/hw/char/pl011.h |  5 ++-
 2 files changed, 64 insertions(+), 19 deletions(-)

-- 
2.34.1

[PATCH v2 1/4] hw/char/pl011: refactor FIFO depth handling code

[Evgeny Iakovlev]

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

[PATCH v2 2/4] hw/char/pl011: implement a reset method

[Evgeny Iakovlev]

PL011 currently lacks a reset method. Implement it.

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

diff --git a/hw/char/pl011.c b/hw/char/pl011.c
index 329cc6926d..404d52a3b8 100644
--- a/hw/char/pl011.c
+++ b/hw/char/pl011.c
@@ -397,11 +397,6 @@ static void pl011_init(Object *obj)
     s->clk = qdev_init_clock_in(DEVICE(obj), "clk", pl011_clock_update, s,
                                 ClockUpdate);
 
-    s->read_trigger = 1;
-    s->ifl = 0x12;
-    s->cr = 0x300;
-    s->flags = 0x90;
-
     s->id = pl011_id_arm;
 }
 
@@ -413,11 +408,37 @@ static void pl011_realize(DeviceState *dev, Error **errp)
                              pl011_event, NULL, s, NULL, true);
 }
 
+static void pl011_reset(DeviceState *dev)
+{
+    PL011State *s = PL011(dev);
+    int i;
+
+    s->lcr = 0;
+    s->rsr = 0;
+    s->dmacr = 0;
+    s->int_enabled = 0;
+    s->int_level = 0;
+    s->ilpr = 0;
+    s->ibrd = 0;
+    s->fbrd = 0;
+    s->read_pos = 0;
+    s->read_count = 0;
+    s->read_trigger = 1;
+    s->ifl = 0x12;
+    s->cr = 0x300;
+    s->flags = 0x90;
+
+    for (i = 0; i < ARRAY_SIZE(s->irq); i++) {
+        qemu_irq_lower(s->irq[i]);
+    }
+}
+
 static void pl011_class_init(ObjectClass *oc, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(oc);
 
     dc->realize = pl011_realize;
+    dc->reset = pl011_reset;
     dc->vmsd = &vmstate_pl011;
     device_class_set_props(dc, pl011_properties);
 }
-- 
2.34.1

[PATCH v2 3/4] 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 guest-visible flag update will happen
only on next data read or write attempt. As a result of that any guest
that expects RXFE flag to be set (and RXFF to be cleared) after resetting
FIFO will never see that happen.

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

diff --git a/hw/char/pl011.c b/hw/char/pl011.c
index 404d52a3b8..3184949d69 100644
--- a/hw/char/pl011.c
+++ b/hw/char/pl011.c
@@ -87,6 +87,13 @@ 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)
 {
@@ -234,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;
@@ -421,12 +427,10 @@ static void pl011_reset(DeviceState *dev)
     s->ilpr = 0;
     s->ibrd = 0;
     s->fbrd = 0;
-    s->read_pos = 0;
-    s->read_count = 0;
     s->read_trigger = 1;
     s->ifl = 0x12;
     s->cr = 0x300;
-    s->flags = 0x90;
+    pl011_reset_pipe(s);
 
     for (i = 0; i < ARRAY_SIZE(s->irq); i++) {
         qemu_irq_lower(s->irq[i]);
-- 
2.34.1

[PATCH v2 4/4] 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 3184949d69..522f36e4f3 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;
 }
@@ -429,7 +445,7 @@ static void pl011_reset(DeviceState *dev)
     s->fbrd = 0;
     s->read_trigger = 1;
     s->ifl = 0x12;
-    s->cr = 0x300;
+    s->cr = PL011_CR_RXE | PL011_CR_TXE;
     pl011_reset_pipe(s);
 
     for (i = 0; i < ARRAY_SIZE(s->irq); i++) {
-- 
2.34.1

Re: [PATCH v2 2/4] hw/char/pl011: implement a reset method

[Peter Maydell]

On Tue, 17 Jan 2023 at 22:05, Evgeny Iakovlev
<eiakovlev@linux.microsoft.com> wrote:
>
> PL011 currently lacks a reset method. Implement it.
>
> Signed-off-by: Evgeny Iakovlev <eiakovlev@linux.microsoft.com>
> ---
>  hw/char/pl011.c | 31 ++++++++++++++++++++++++++-----
>  1 file changed, 26 insertions(+), 5 deletions(-)
>
> diff --git a/hw/char/pl011.c b/hw/char/pl011.c
> index 329cc6926d..404d52a3b8 100644
> --- a/hw/char/pl011.c
> +++ b/hw/char/pl011.c
> @@ -397,11 +397,6 @@ static void pl011_init(Object *obj)
>      s->clk = qdev_init_clock_in(DEVICE(obj), "clk", pl011_clock_update, s,
>                                  ClockUpdate);
>
> -    s->read_trigger = 1;
> -    s->ifl = 0x12;
> -    s->cr = 0x300;
> -    s->flags = 0x90;
> -
>      s->id = pl011_id_arm;
>  }
>
> @@ -413,11 +408,37 @@ static void pl011_realize(DeviceState *dev, Error **errp)
>                               pl011_event, NULL, s, NULL, true);
>  }
>
> +static void pl011_reset(DeviceState *dev)
> +{
> +    PL011State *s = PL011(dev);
> +    int i;
> +
> +    s->lcr = 0;
> +    s->rsr = 0;
> +    s->dmacr = 0;
> +    s->int_enabled = 0;
> +    s->int_level = 0;
> +    s->ilpr = 0;
> +    s->ibrd = 0;
> +    s->fbrd = 0;
> +    s->read_pos = 0;
> +    s->read_count = 0;
> +    s->read_trigger = 1;
> +    s->ifl = 0x12;
> +    s->cr = 0x300;
> +    s->flags = 0x90;
> +
> +    for (i = 0; i < ARRAY_SIZE(s->irq); i++) {
> +        qemu_irq_lower(s->irq[i]);
> +    }

Reset should never touch outbound qemu_irq lines.
(The other end of the line will also reset and will end
up in the correct "as if the input is 0" state.)

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

thanks
-- PMM

Re: [PATCH v2 3/4] hw/char/pl011: better handling of FIFO flags on LCR reset

[Peter Maydell]

On Tue, 17 Jan 2023 at 22:05, 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 guest-visible flag update will happen
> only on next data read or write attempt. As a result of that any guest
> that expects RXFE flag to be set (and RXFF to be cleared) after resetting
> FIFO will never see that happen.
>
> Signed-off-by: Evgeny Iakovlev <eiakovlev@linux.microsoft.com>
> ---
>  hw/char/pl011.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/hw/char/pl011.c b/hw/char/pl011.c
> index 404d52a3b8..3184949d69 100644
> --- a/hw/char/pl011.c
> +++ b/hw/char/pl011.c
> @@ -87,6 +87,13 @@ 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;

Should this really be resetting all the other flags to 0 ?
I think we should set/clear only the FIFO related flags, and
leave the others alone. We don't yet implement the
modem-status signals, but if/when we ever do, clearing them
would be the wrong thing here.

(Reset still needs to reset all the flag register bits.)

thanks
-- PMM

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

[Peter Maydell]

On Tue, 17 Jan 2023 at 22:05, 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.
>
> Signed-off-by: Evgeny Iakovlev <eiakovlev@linux.microsoft.com>
> ---
>  hw/char/pl011.c | 22 +++++++++++++++++++---
>  1 file changed, 19 insertions(+), 3 deletions(-)

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

thanks
-- PMM

Re: [PATCH v2 1/4] hw/char/pl011: refactor FIFO depth handling code

[Peter Maydell]

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

Re: [PATCH v2 2/4] hw/char/pl011: implement a reset method

[Evgeny Iakovlev]

On 1/19/2023 14:27, Peter Maydell wrote:
> On Tue, 17 Jan 2023 at 22:05, Evgeny Iakovlev
> <eiakovlev@linux.microsoft.com> wrote:
>> PL011 currently lacks a reset method. Implement it.
>>
>> Signed-off-by: Evgeny Iakovlev <eiakovlev@linux.microsoft.com>
>> ---
>>   hw/char/pl011.c | 31 ++++++++++++++++++++++++++-----
>>   1 file changed, 26 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/char/pl011.c b/hw/char/pl011.c
>> index 329cc6926d..404d52a3b8 100644
>> --- a/hw/char/pl011.c
>> +++ b/hw/char/pl011.c
>> @@ -397,11 +397,6 @@ static void pl011_init(Object *obj)
>>       s->clk = qdev_init_clock_in(DEVICE(obj), "clk", pl011_clock_update, s,
>>                                   ClockUpdate);
>>
>> -    s->read_trigger = 1;
>> -    s->ifl = 0x12;
>> -    s->cr = 0x300;
>> -    s->flags = 0x90;
>> -
>>       s->id = pl011_id_arm;
>>   }
>>
>> @@ -413,11 +408,37 @@ static void pl011_realize(DeviceState *dev, Error **errp)
>>                                pl011_event, NULL, s, NULL, true);
>>   }
>>
>> +static void pl011_reset(DeviceState *dev)
>> +{
>> +    PL011State *s = PL011(dev);
>> +    int i;
>> +
>> +    s->lcr = 0;
>> +    s->rsr = 0;
>> +    s->dmacr = 0;
>> +    s->int_enabled = 0;
>> +    s->int_level = 0;
>> +    s->ilpr = 0;
>> +    s->ibrd = 0;
>> +    s->fbrd = 0;
>> +    s->read_pos = 0;
>> +    s->read_count = 0;
>> +    s->read_trigger = 1;
>> +    s->ifl = 0x12;
>> +    s->cr = 0x300;
>> +    s->flags = 0x90;
>> +
>> +    for (i = 0; i < ARRAY_SIZE(s->irq); i++) {
>> +        qemu_irq_lower(s->irq[i]);
>> +    }
> Reset should never touch outbound qemu_irq lines.
> (The other end of the line will also reset and will end
> up in the correct "as if the input is 0" state.)


Really? I saw this reset happening on other devices in hw/char (don't 
remember which ones specifically), so i though it is needed.


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

Re: [PATCH v2 3/4] hw/char/pl011: better handling of FIFO flags on LCR reset

[Evgeny Iakovlev]

On 1/19/2023 14:30, Peter Maydell wrote:
> On Tue, 17 Jan 2023 at 22:05, 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 guest-visible flag update will happen
>> only on next data read or write attempt. As a result of that any guest
>> that expects RXFE flag to be set (and RXFF to be cleared) after resetting
>> FIFO will never see that happen.
>>
>> Signed-off-by: Evgeny Iakovlev <eiakovlev@linux.microsoft.com>
>> ---
>>   hw/char/pl011.c | 14 +++++++++-----
>>   1 file changed, 9 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/char/pl011.c b/hw/char/pl011.c
>> index 404d52a3b8..3184949d69 100644
>> --- a/hw/char/pl011.c
>> +++ b/hw/char/pl011.c
>> @@ -87,6 +87,13 @@ 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;
> Should this really be resetting all the other flags to 0 ?
> I think we should set/clear only the FIFO related flags, and
> leave the others alone. We don't yet implement the
> modem-status signals, but if/when we ever do, clearing them
> would be the wrong thing here.
>
> (Reset still needs to reset all the flag register bits.)


Right, i thought about it, but as you mention we only use FIFO flags 
currently. Still, your suggestion about future changes makes sense.


>
> thanks
> -- PMM

Re: [PATCH v2 1/4] hw/char/pl011: refactor FIFO depth handling code

[Evgeny Iakovlev]

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

Re: [PATCH v2 2/4] hw/char/pl011: implement a reset method

[Peter Maydell]

On Thu, 19 Jan 2023 at 21:57, Evgeny Iakovlev
<eiakovlev@linux.microsoft.com> wrote:
>
>
> On 1/19/2023 14:27, Peter Maydell wrote:
> > On Tue, 17 Jan 2023 at 22:05, Evgeny Iakovlev
> > <eiakovlev@linux.microsoft.com> wrote:
> >> PL011 currently lacks a reset method. Implement it.
> >>
> >> Signed-off-by: Evgeny Iakovlev <eiakovlev@linux.microsoft.com>
> >> ---
> >>   hw/char/pl011.c | 31 ++++++++++++++++++++++++++-----
> >>   1 file changed, 26 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/hw/char/pl011.c b/hw/char/pl011.c
> >> index 329cc6926d..404d52a3b8 100644
> >> --- a/hw/char/pl011.c
> >> +++ b/hw/char/pl011.c
> >> @@ -397,11 +397,6 @@ static void pl011_init(Object *obj)
> >>       s->clk = qdev_init_clock_in(DEVICE(obj), "clk", pl011_clock_update, s,
> >>                                   ClockUpdate);
> >>
> >> -    s->read_trigger = 1;
> >> -    s->ifl = 0x12;
> >> -    s->cr = 0x300;
> >> -    s->flags = 0x90;
> >> -
> >>       s->id = pl011_id_arm;
> >>   }
> >>
> >> @@ -413,11 +408,37 @@ static void pl011_realize(DeviceState *dev, Error **errp)
> >>                                pl011_event, NULL, s, NULL, true);
> >>   }
> >>
> >> +static void pl011_reset(DeviceState *dev)
> >> +{
> >> +    PL011State *s = PL011(dev);
> >> +    int i;
> >> +
> >> +    s->lcr = 0;
> >> +    s->rsr = 0;
> >> +    s->dmacr = 0;
> >> +    s->int_enabled = 0;
> >> +    s->int_level = 0;
> >> +    s->ilpr = 0;
> >> +    s->ibrd = 0;
> >> +    s->fbrd = 0;
> >> +    s->read_pos = 0;
> >> +    s->read_count = 0;
> >> +    s->read_trigger = 1;
> >> +    s->ifl = 0x12;
> >> +    s->cr = 0x300;
> >> +    s->flags = 0x90;
> >> +
> >> +    for (i = 0; i < ARRAY_SIZE(s->irq); i++) {
> >> +        qemu_irq_lower(s->irq[i]);
> >> +    }
> > Reset should never touch outbound qemu_irq lines.
> > (The other end of the line will also reset and will end
> > up in the correct "as if the input is 0" state.)
>
>
> Really? I saw this reset happening on other devices in hw/char (don't
> remember which ones specifically), so i though it is needed.

A few devices mess with their IRQ line in a reset handler;
this is a bug but usually one you can get away with. Some
devices use the newer "three phase reset" approach which
does allow you to change IRQ line state in the 'hold' phase.
But generally the standard is not to touch the IRQ line if
its reset state would be 'low'. You only need to do odd
things for the rare case where an IRQ line is supposed to
be taken high on reset.

(The reason for the "no touching IRQs in reset" rule is that
there's no ordering on device reset, so if you raise an
IRQ line in your reset handler, you don't know if the
device on the other end has already reset and thus will
correctly deal with the 0->1 transition it sees, or if
it has not yet reset and is about to undo the effects of
that 0->1 transition. 3-phase reset lets devices split
their reset handling up, so you know that if you do something
with an IRQ line in the 'hold' phase that the device you're
talking to has definitely already done its 'enter' phase.
Though 3-phase reset is pretty new, so a lot of devices
don't use it yet, and I have a fairly strong suspicion
that there are still some issues with the design that we
will need to iron out as we make more use of it.)

thanks
-- PMM

Re: [PATCH v2 2/4] hw/char/pl011: implement a reset method

[eiakovlev]

On 1/20/23 12:45 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On Thu, 19 Jan 2023 at 21:57, Evgeny Iakovlev
> <eiakovlev@linux.microsoft.com> wrote:
> >
> >
> > On 1/19/2023 14:27, Peter Maydell wrote:
> >> On Tue, 17 Jan 2023 at 22:05, Evgeny Iakovlev
> >> <eiakovlev@linux.microsoft.com> wrote:
> >>> PL011 currently lacks a reset method. Implement it.
> >>>
> >>> Signed-off-by: Evgeny Iakovlev <eiakovlev@linux.microsoft.com>
> >>> ---
> >>>    hw/char/pl011.c | 31 ++++++++++++++++++++++++++-----
> >>>    1 file changed, 26 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/hw/char/pl011.c b/hw/char/pl011.c
> >>> index 329cc6926d..404d52a3b8 100644
> >>> --- a/hw/char/pl011.c
> >>> +++ b/hw/char/pl011.c
> >>> @@ -397,11 +397,6 @@ static void pl011_init(Object *obj)
> >>>        s->clk = qdev_init_clock_in(DEVICE(obj), "clk", pl011_clock_update, s,
> >>>                                    ClockUpdate);
> >>>
> >>> -    s->read_trigger = 1;
> >>> -    s->ifl = 0x12;
> >>> -    s->cr = 0x300;
> >>> -    s->flags = 0x90;
> >>> -
> >>>        s->id = pl011_id_arm;
> >>>    }
> >>>
> >>> @@ -413,11 +408,37 @@ static void pl011_realize(DeviceState *dev, Error **errp)
> >>>                                 pl011_event, NULL, s, NULL, true);
> >>>    }
> >>>
> >>> +static void pl011_reset(DeviceState *dev)
> >>> +{
> >>> +    PL011State *s = PL011(dev);
> >>> +    int i;
> >>> +
> >>> +    s->lcr = 0;
> >>> +    s->rsr = 0;
> >>> +    s->dmacr = 0;
> >>> +    s->int_enabled = 0;
> >>> +    s->int_level = 0;
> >>> +    s->ilpr = 0;
> >>> +    s->ibrd = 0;
> >>> +    s->fbrd = 0;
> >>> +    s->read_pos = 0;
> >>> +    s->read_count = 0;
> >>> +    s->read_trigger = 1;
> >>> +    s->ifl = 0x12;
> >>> +    s->cr = 0x300;
> >>> +    s->flags = 0x90;
> >>> +
> >>> +    for (i = 0; i < ARRAY_SIZE(s->irq); i++) {
> >>> +        qemu_irq_lower(s->irq[i]);
> >>> +    }
> >> Reset should never touch outbound qemu_irq lines.
> >> (The other end of the line will also reset and will end
> >> up in the correct "as if the input is 0" state.)
> >
> >
> > Really? I saw this reset happening on other devices in hw/char (don't
> > remember which ones specifically), so i though it is needed.
> 
> A few devices mess with their IRQ line in a reset handler;
> this is a bug but usually one you can get away with. Some
> devices use the newer "three phase reset" approach which
> does allow you to change IRQ line state in the 'hold' phase.
> But generally the standard is not to touch the IRQ line if
> its reset state would be 'low'. You only need to do odd
> things for the rare case where an IRQ line is supposed to
> be taken high on reset.
> 
> (The reason for the "no touching IRQs in reset" rule is that
> there's no ordering on device reset, so if you raise an
> IRQ line in your reset handler, you don't know if the
> device on the other end has already reset and thus will
> correctly deal with the 0->1 transition it sees, or if
> it has not yet reset and is about to undo the effects of
> that 0->1 transition. 3-phase reset lets devices split
> their reset handling up, so you know that if you do something
> with an IRQ line in the 'hold' phase that the device you're
> talking to has definitely already done its 'enter' phase.
> Though 3-phase reset is pretty new, so a lot of devices
> don't use it yet, and I have a fairly strong suspicion
> that there are still some issues with the design that we
> will need to iron out as we make more use of it.)

I see. Thanks a lot for explaining.

> 
> thanks
> -- PMM
>