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

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

[Evgeny Iakovlev]

v3:
* Introduced a post_load hook for PL011State migration for
  backwards-compatibility due to some input state fragility.
* No longer touching irq lines in reset method
* Minor changes based on review feedback.

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

Evgeny Iakovlev (5):
  hw/char/pl011: refactor FIFO depth handling code
  hw/char/pl011: add post_load hook for backwards-compatibility
  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         | 110 +++++++++++++++++++++++++++++++++-------
 include/hw/char/pl011.h |   5 +-
 2 files changed, 95 insertions(+), 20 deletions(-)

-- 
2.34.1

[PATCH v3 1/5] 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         | 30 ++++++++++++++++++------------
 include/hw/char/pl011.h |  5 ++++-
 2 files changed, 22 insertions(+), 13 deletions(-)

diff --git a/hw/char/pl011.c b/hw/char/pl011.c
index c076813423..3fa3b75d04 100644
--- a/hw/char/pl011.c
+++ b/hw/char/pl011.c
@@ -81,6 +81,17 @@ static void pl011_update(PL011State *s)
     }
 }
 
+static bool pl011_is_fifo_enabled(PL011State *s)
+{
+    return (s->lcr & 0x10) != 0;
+}
+
+static inline unsigned pl011_get_fifo_depth(PL011State *s)
+{
+    /* Note: FIFO depth is expected to be power-of-2 */
+    return pl011_is_fifo_enabled(s) ? PL011_FIFO_DEPTH : 1;
+}
+
 static uint64_t pl011_read(void *opaque, hwaddr offset,
                            unsigned size)
 {
@@ -94,8 +105,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 +283,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 +292,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 +365,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 v3 2/5] hw/char/pl011: add post_load hook for backwards-compatibility

[Evgeny Iakovlev]

Previous change slightly modified the way we handle data writes when
FIFO is disabled. Previously we kept incrementing read_pos and were
storing data at that position, although we only have a
single-register-deep FIFO now. Then we changed it to always store data
at pos 0.

If guest disables FIFO and the proceeds to read data, it will work out
fine, because we read from current read_pos before setting it to 0.

However, to make code less fragile, introduce a post_load hook for
PL011State and move fixup read FIFO state when FIFO is disabled. Since
we are introducing a post_load hook, also do some sanity checking on
untrusted incoming input state.

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

diff --git a/hw/char/pl011.c b/hw/char/pl011.c
index 3fa3b75d04..4df649a064 100644
--- a/hw/char/pl011.c
+++ b/hw/char/pl011.c
@@ -352,10 +352,35 @@ static const VMStateDescription vmstate_pl011_clock = {
     }
 };
 
+static int pl011_post_load(void *opaque, int version_id)
+{
+    PL011State* s = opaque;
+
+    /* Sanity-check input state */
+    if (s->read_pos >= ARRAY_SIZE(s->read_fifo) ||
+        s->read_count > ARRAY_SIZE(s->read_fifo)) {
+        return -1;
+    }
+
+    if (version_id < 3 && !pl011_is_fifo_enabled(s)) {
+        /*
+         * Older versions of PL011 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;
+    }
+
+    return 0;
+}
+
 static const VMStateDescription vmstate_pl011 = {
     .name = "pl011",
-    .version_id = 2,
+    .version_id = 3,
     .minimum_version_id = 2,
+    .post_load = pl011_post_load,
     .fields = (VMStateField[]) {
         VMSTATE_UINT32(readbuff, PL011State),
         VMSTATE_UINT32(flags, PL011State),
-- 
2.34.1

[PATCH v3 3/5] 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 | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

diff --git a/hw/char/pl011.c b/hw/char/pl011.c
index 4df649a064..f9413f3703 100644
--- a/hw/char/pl011.c
+++ b/hw/char/pl011.c
@@ -427,11 +427,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;
 }
 
@@ -443,11 +438,32 @@ static void pl011_realize(DeviceState *dev, Error **errp)
                              pl011_event, NULL, s, NULL, true);
 }
 
+static void pl011_reset(DeviceState *dev)
+{
+    PL011State *s = PL011(dev);
+
+    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;
+}
+
 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 v3 4/5] 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 | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/hw/char/pl011.c b/hw/char/pl011.c
index f9413f3703..c72fbb7d50 100644
--- a/hw/char/pl011.c
+++ b/hw/char/pl011.c
@@ -92,6 +92,16 @@ static inline unsigned pl011_get_fifo_depth(PL011State *s)
     return pl011_is_fifo_enabled(s) ? PL011_FIFO_DEPTH : 1;
 }
 
+static inline void pl011_reset_fifo(PL011State *s)
+{
+    s->read_count = 0;
+    s->read_pos = 0;
+
+    /* Reset FIFO flags */
+    s->flags &= ~(PL011_FLAG_RXFF | PL011_FLAG_TXFF);
+    s->flags |= PL011_FLAG_RXFE | PL011_FLAG_TXFE;
+}
+
 static uint64_t pl011_read(void *opaque, hwaddr offset,
                            unsigned size)
 {
@@ -239,8 +249,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_fifo(s);
         }
         if ((s->lcr ^ value) & 0x1) {
             int break_enable = value & 0x1;
@@ -450,12 +459,11 @@ 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;
+    s->flags = 0;
+    pl011_reset_fifo(s);
 }
 
 static void pl011_class_init(ObjectClass *oc, void *data)
-- 
2.34.1

[PATCH v3 5/5] 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>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 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 c72fbb7d50..dd20b76609 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] =
@@ -211,6 +216,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)
 {
@@ -221,7 +231,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 */
@@ -292,7 +304,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;
 }
@@ -461,7 +477,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;
     s->flags = 0;
     pl011_reset_fifo(s);
 }
-- 
2.34.1

Re: [PATCH v3 2/5] hw/char/pl011: add post_load hook for backwards-compatibility

[Peter Maydell]

On Fri, 20 Jan 2023 at 15:54, Evgeny Iakovlev
<eiakovlev@linux.microsoft.com> wrote:
>
> Previous change slightly modified the way we handle data writes when
> FIFO is disabled. Previously we kept incrementing read_pos and were
> storing data at that position, although we only have a
> single-register-deep FIFO now. Then we changed it to always store data
> at pos 0.
>
> If guest disables FIFO and the proceeds to read data, it will work out
> fine, because we read from current read_pos before setting it to 0.
>
> However, to make code less fragile, introduce a post_load hook for
> PL011State and move fixup read FIFO state when FIFO is disabled. Since
> we are introducing a post_load hook, also do some sanity checking on
> untrusted incoming input state.
>
> Signed-off-by: Evgeny Iakovlev <eiakovlev@linux.microsoft.com>
> ---
>  hw/char/pl011.c | 27 ++++++++++++++++++++++++++-
>  1 file changed, 26 insertions(+), 1 deletion(-)
>
> diff --git a/hw/char/pl011.c b/hw/char/pl011.c
> index 3fa3b75d04..4df649a064 100644
> --- a/hw/char/pl011.c
> +++ b/hw/char/pl011.c
> @@ -352,10 +352,35 @@ static const VMStateDescription vmstate_pl011_clock = {
>      }
>  };
>
> +static int pl011_post_load(void *opaque, int version_id)
> +{
> +    PL011State* s = opaque;
> +
> +    /* Sanity-check input state */
> +    if (s->read_pos >= ARRAY_SIZE(s->read_fifo) ||
> +        s->read_count > ARRAY_SIZE(s->read_fifo)) {
> +        return -1;
> +    }
> +
> +    if (version_id < 3 && !pl011_is_fifo_enabled(s)) {
> +        /*
> +         * Older versions of PL011 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;
> +    }

You don't need to bump the version id and do this
check based on version ID. You can just check whether
the old state indicates that the data isn't in slot 0
of the array, the way I suggested in my comment on the
previous version of the patchset.

(New->old migration will work fine.)

thanks
-- PMM

Re: [PATCH v3 1/5] hw/char/pl011: refactor FIFO depth handling code

[Peter Maydell]

On Fri, 20 Jan 2023 at 15:54, 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>
> ---

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

thanks
-- PMM

Re: [PATCH v3 3/5] hw/char/pl011: implement a reset method

[Peter Maydell]

On Fri, 20 Jan 2023 at 15:54, 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 | 26 +++++++++++++++++++++-----

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

thanks
-- PMM

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

[Peter Maydell]

On Fri, 20 Jan 2023 at 15:54, 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>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH v3 3/5] hw/char/pl011: implement a reset method

[Philippe Mathieu-Daudé]

On 20/1/23 16:54, Evgeny Iakovlev wrote:
> PL011 currently lacks a reset method. Implement it.
> 
> Signed-off-by: Evgeny Iakovlev <eiakovlev@linux.microsoft.com>
> ---
>   hw/char/pl011.c | 26 +++++++++++++++++++++-----
>   1 file changed, 21 insertions(+), 5 deletions(-)

>   static void pl011_class_init(ObjectClass *oc, void *data)
>   {
>       DeviceClass *dc = DEVICE_CLASS(oc);
>   
>       dc->realize = pl011_realize;
> +    dc->reset = pl011_reset;
Maybe directly implement as ResettableHoldPhase from "hw/resettable.h".

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH v3 1/5] hw/char/pl011: refactor FIFO depth handling code

[Philippe Mathieu-Daudé]

On 20/1/23 16:54, Evgeny Iakovlev 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         | 30 ++++++++++++++++++------------
>   include/hw/char/pl011.h |  5 ++++-
>   2 files changed, 22 insertions(+), 13 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

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

[Philippe Mathieu-Daudé]

On 20/1/23 16:54, Evgeny Iakovlev 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>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   hw/char/pl011.c | 22 +++++++++++++++++++---
>   1 file changed, 19 insertions(+), 3 deletions(-)

> +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)
>   {
> @@ -221,7 +231,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 */
> @@ -292,7 +304,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)) {

Maybe add pl011_can_receive() similarly to pl011_can_transmit().

Otherwise:
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

> +        r = 0;
> +    } else {
> +        r = s->read_count < pl011_get_fifo_depth(s);
> +    }
>       trace_pl011_can_receive(s->lcr, s->read_count, r);
>       return r;
>   }

Re: [PATCH v3 2/5] hw/char/pl011: add post_load hook for backwards-compatibility

[Evgeny Iakovlev]

On 1/20/2023 19:22, Peter Maydell wrote:
> On Fri, 20 Jan 2023 at 15:54, Evgeny Iakovlev
> <eiakovlev@linux.microsoft.com> wrote:
>> Previous change slightly modified the way we handle data writes when
>> FIFO is disabled. Previously we kept incrementing read_pos and were
>> storing data at that position, although we only have a
>> single-register-deep FIFO now. Then we changed it to always store data
>> at pos 0.
>>
>> If guest disables FIFO and the proceeds to read data, it will work out
>> fine, because we read from current read_pos before setting it to 0.
>>
>> However, to make code less fragile, introduce a post_load hook for
>> PL011State and move fixup read FIFO state when FIFO is disabled. Since
>> we are introducing a post_load hook, also do some sanity checking on
>> untrusted incoming input state.
>>
>> Signed-off-by: Evgeny Iakovlev <eiakovlev@linux.microsoft.com>
>> ---
>>   hw/char/pl011.c | 27 ++++++++++++++++++++++++++-
>>   1 file changed, 26 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/char/pl011.c b/hw/char/pl011.c
>> index 3fa3b75d04..4df649a064 100644
>> --- a/hw/char/pl011.c
>> +++ b/hw/char/pl011.c
>> @@ -352,10 +352,35 @@ static const VMStateDescription vmstate_pl011_clock = {
>>       }
>>   };
>>
>> +static int pl011_post_load(void *opaque, int version_id)
>> +{
>> +    PL011State* s = opaque;
>> +
>> +    /* Sanity-check input state */
>> +    if (s->read_pos >= ARRAY_SIZE(s->read_fifo) ||
>> +        s->read_count > ARRAY_SIZE(s->read_fifo)) {
>> +        return -1;
>> +    }
>> +
>> +    if (version_id < 3 && !pl011_is_fifo_enabled(s)) {
>> +        /*
>> +         * Older versions of PL011 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;
>> +    }
> You don't need to bump the version id and do this
> check based on version ID. You can just check whether
> the old state indicates that the data isn't in slot 0
> of the array, the way I suggested in my comment on the
> previous version of the patchset.
>
> (New->old migration will work fine.)


Right, i thought this was a cleaner check, because it doesn't rely as 
much on internal state. Breaking backwards migration is not as great 
though..


>
> thanks
> -- PMM

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

[Evgeny Iakovlev]

On 1/23/2023 09:14, Philippe Mathieu-Daudé wrote:
> On 20/1/23 16:54, Evgeny Iakovlev 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>
>> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>>   hw/char/pl011.c | 22 +++++++++++++++++++---
>>   1 file changed, 19 insertions(+), 3 deletions(-)
>
>> +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)
>>   {
>> @@ -221,7 +231,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 */
>> @@ -292,7 +304,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)) {
>
> Maybe add pl011_can_receive() similarly to pl011_can_transmit().
>
> Otherwise:
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>


Thanks! There's already a pl011_can_receive though, its the 
pl011_can_transmit that's new :)


>
>> +        r = 0;
>> +    } else {
>> +        r = s->read_count < pl011_get_fifo_depth(s);
>> +    }
>>       trace_pl011_can_receive(s->lcr, s->read_count, r);
>>       return r;
>>   }

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

[Philippe Mathieu-Daudé]

On 23/1/23 15:43, Evgeny Iakovlev wrote:
> 
> On 1/23/2023 09:14, Philippe Mathieu-Daudé wrote:
>> On 20/1/23 16:54, Evgeny Iakovlev 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>
>>> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>>> ---
>>>   hw/char/pl011.c | 22 +++++++++++++++++++---
>>>   1 file changed, 19 insertions(+), 3 deletions(-)
>>
>>> +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)
>>>   {
>>> @@ -221,7 +231,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 */
>>> @@ -292,7 +304,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)) {
>>
>> Maybe add pl011_can_receive() similarly to pl011_can_transmit().
>>
>> Otherwise:
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> 
> 
> Thanks! There's already a pl011_can_receive though, its the 
> pl011_can_transmit that's new :)

pl011_can_receive() returns the number of bytes that pl011_receive() can 
accept, pl011_can_transmit() returns a boolean.

I was thinking of:

-- >8 --
diff --git a/hw/char/pl011.c b/hw/char/pl011.c
index dd20b76609..ea5769a31c 100644
--- a/hw/char/pl011.c
+++ b/hw/char/pl011.c
@@ -221,6 +221,11 @@ static inline bool pl011_can_transmit(PL011State *s)
      return s->cr & PL011_CR_UARTEN && s->cr & PL011_CR_TXE;
  }

+static inline bool pl011_can_receive(PL011State *s)
+{
+    return s->cr & PL011_CR_UARTEN && s->cr & PL011_CR_RXE;
+}
+
  static void pl011_write(void *opaque, hwaddr offset,
                          uint64_t value, unsigned size)
  {
@@ -299,12 +304,12 @@ static void pl011_write(void *opaque, hwaddr offset,
      }
  }

-static int pl011_can_receive(void *opaque)
+static int pl011_receivable_bytes(void *opaque)
  {
      PL011State *s = (PL011State *)opaque;
      int r;

-    if (!(s->cr & PL011_CR_UARTEN) || !(s->cr & PL011_CR_RXE)) {
+    if (!pl011_can_receive(s)) {
          r = 0;
      } else {
          r = s->read_count < pl011_get_fifo_depth(s);
@@ -459,7 +464,7 @@ static void pl011_realize(DeviceState *dev, Error 
**errp)
  {
      PL011State *s = PL011(dev);

-    qemu_chr_fe_set_handlers(&s->chr, pl011_can_receive, pl011_receive,
+    qemu_chr_fe_set_handlers(&s->chr, pl011_receivable_bytes, 
pl011_receive,
                               pl011_event, NULL, s, NULL, true);
  }

---

with maybe a better name for pl011_receivable_bytes().

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

[Evgeny Iakovlev]

On 1/23/2023 16:21, Philippe Mathieu-Daudé wrote:
> On 23/1/23 15:43, Evgeny Iakovlev wrote:
>>
>> On 1/23/2023 09:14, Philippe Mathieu-Daudé wrote:
>>> On 20/1/23 16:54, Evgeny Iakovlev 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>
>>>> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>>>> ---
>>>>   hw/char/pl011.c | 22 +++++++++++++++++++---
>>>>   1 file changed, 19 insertions(+), 3 deletions(-)
>>>
>>>> +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)
>>>>   {
>>>> @@ -221,7 +231,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 */
>>>> @@ -292,7 +304,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)) {
>>>
>>> Maybe add pl011_can_receive() similarly to pl011_can_transmit().
>>>
>>> Otherwise:
>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>
>>
>> Thanks! There's already a pl011_can_receive though, its the 
>> pl011_can_transmit that's new :)
>
> pl011_can_receive() returns the number of bytes that pl011_receive() 
> can accept, pl011_can_transmit() returns a boolean.


Hm, no, pl011_can_receive never actually returned the number of bytes. 
It's a boolean value as an int. Which is a bug, because 
qemu_chr_fe_set_handlers expects it to return the byte count, not a 0/1 
value.

I'll fix it then.


>
> I was thinking of:
>
> -- >8 --
> diff --git a/hw/char/pl011.c b/hw/char/pl011.c
> index dd20b76609..ea5769a31c 100644
> --- a/hw/char/pl011.c
> +++ b/hw/char/pl011.c
> @@ -221,6 +221,11 @@ static inline bool pl011_can_transmit(PL011State *s)
>      return s->cr & PL011_CR_UARTEN && s->cr & PL011_CR_TXE;
>  }
>
> +static inline bool pl011_can_receive(PL011State *s)
> +{
> +    return s->cr & PL011_CR_UARTEN && s->cr & PL011_CR_RXE;
> +}
> +
>  static void pl011_write(void *opaque, hwaddr offset,
>                          uint64_t value, unsigned size)
>  {
> @@ -299,12 +304,12 @@ static void pl011_write(void *opaque, hwaddr 
> offset,
>      }
>  }
>
> -static int pl011_can_receive(void *opaque)
> +static int pl011_receivable_bytes(void *opaque)
>  {
>      PL011State *s = (PL011State *)opaque;
>      int r;
>
> -    if (!(s->cr & PL011_CR_UARTEN) || !(s->cr & PL011_CR_RXE)) {
> +    if (!pl011_can_receive(s)) {
>          r = 0;
>      } else {
>          r = s->read_count < pl011_get_fifo_depth(s);
> @@ -459,7 +464,7 @@ static void pl011_realize(DeviceState *dev, Error 
> **errp)
>  {
>      PL011State *s = PL011(dev);
>
> -    qemu_chr_fe_set_handlers(&s->chr, pl011_can_receive, pl011_receive,
> +    qemu_chr_fe_set_handlers(&s->chr, pl011_receivable_bytes, 
> pl011_receive,
>                               pl011_event, NULL, s, NULL, true);
>  }
>
> ---
>
> with maybe a better name for pl011_receivable_bytes().
>

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

[Evgeny Iakovlev]

On 1/23/2023 16:59, Evgeny Iakovlev wrote:
>
> On 1/23/2023 16:21, Philippe Mathieu-Daudé wrote:
>> On 23/1/23 15:43, Evgeny Iakovlev wrote:
>>>
>>> On 1/23/2023 09:14, Philippe Mathieu-Daudé wrote:
>>>> On 20/1/23 16:54, Evgeny Iakovlev 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>
>>>>> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>>>>> ---
>>>>>   hw/char/pl011.c | 22 +++++++++++++++++++---
>>>>>   1 file changed, 19 insertions(+), 3 deletions(-)
>>>>
>>>>> +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)
>>>>>   {
>>>>> @@ -221,7 +231,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 */
>>>>> @@ -292,7 +304,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)) {
>>>>
>>>> Maybe add pl011_can_receive() similarly to pl011_can_transmit().
>>>>
>>>> Otherwise:
>>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>
>>>
>>> Thanks! There's already a pl011_can_receive though, its the 
>>> pl011_can_transmit that's new :)
>>
>> pl011_can_receive() returns the number of bytes that pl011_receive() 
>> can accept, pl011_can_transmit() returns a boolean.
>
>
> Hm, no, pl011_can_receive never actually returned the number of bytes. 
> It's a boolean value as an int. Which is a bug, because 
> qemu_chr_fe_set_handlers expects it to return the byte count, not a 
> 0/1 value.
>
> I'll fix it then.


Actually, no, i spoke too soon. qemu_chr_fe_set_handlers indeed expects 
a number of bytes from pl011_can_receive, but pl011_can_receive also 
deliberately gives it 1 and not how many bytes are there in queue 
really, because everything else on the receive code path is written with 
an assumption to only accept 1 element at a time (see pl011_put_fifo).

If we want to optimize this part, we would need to change that 
assumption. That should better be done as a separate change though, 
which i can send later.


>
>
>>
>> I was thinking of:
>>
>> -- >8 --
>> diff --git a/hw/char/pl011.c b/hw/char/pl011.c
>> index dd20b76609..ea5769a31c 100644
>> --- a/hw/char/pl011.c
>> +++ b/hw/char/pl011.c
>> @@ -221,6 +221,11 @@ static inline bool pl011_can_transmit(PL011State 
>> *s)
>>      return s->cr & PL011_CR_UARTEN && s->cr & PL011_CR_TXE;
>>  }
>>
>> +static inline bool pl011_can_receive(PL011State *s)
>> +{
>> +    return s->cr & PL011_CR_UARTEN && s->cr & PL011_CR_RXE;
>> +}
>> +
>>  static void pl011_write(void *opaque, hwaddr offset,
>>                          uint64_t value, unsigned size)
>>  {
>> @@ -299,12 +304,12 @@ static void pl011_write(void *opaque, hwaddr 
>> offset,
>>      }
>>  }
>>
>> -static int pl011_can_receive(void *opaque)
>> +static int pl011_receivable_bytes(void *opaque)
>>  {
>>      PL011State *s = (PL011State *)opaque;
>>      int r;
>>
>> -    if (!(s->cr & PL011_CR_UARTEN) || !(s->cr & PL011_CR_RXE)) {
>> +    if (!pl011_can_receive(s)) {
>>          r = 0;
>>      } else {
>>          r = s->read_count < pl011_get_fifo_depth(s);
>> @@ -459,7 +464,7 @@ static void pl011_realize(DeviceState *dev, Error 
>> **errp)
>>  {
>>      PL011State *s = PL011(dev);
>>
>> -    qemu_chr_fe_set_handlers(&s->chr, pl011_can_receive, pl011_receive,
>> +    qemu_chr_fe_set_handlers(&s->chr, pl011_receivable_bytes, 
>> pl011_receive,
>>                               pl011_event, NULL, s, NULL, true);
>>  }
>>
>> ---
>>
>> with maybe a better name for pl011_receivable_bytes().
>>

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

[Peter Maydell]

On Mon, 23 Jan 2023 at 15:21, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> pl011_can_receive() returns the number of bytes that pl011_receive() can
> accept, pl011_can_transmit() returns a boolean.
>
> I was thinking of:
>
> -- >8 --
> diff --git a/hw/char/pl011.c b/hw/char/pl011.c
> index dd20b76609..ea5769a31c 100644
> --- a/hw/char/pl011.c
> +++ b/hw/char/pl011.c
> @@ -221,6 +221,11 @@ static inline bool pl011_can_transmit(PL011State *s)
>       return s->cr & PL011_CR_UARTEN && s->cr & PL011_CR_TXE;
>   }
>
> +static inline bool pl011_can_receive(PL011State *s)
> +{
> +    return s->cr & PL011_CR_UARTEN && s->cr & PL011_CR_RXE;
> +}
> +
>   static void pl011_write(void *opaque, hwaddr offset,
>                           uint64_t value, unsigned size)
>   {
> @@ -299,12 +304,12 @@ static void pl011_write(void *opaque, hwaddr offset,
>       }
>   }
>
> -static int pl011_can_receive(void *opaque)
> +static int pl011_receivable_bytes(void *opaque)
>   {
>       PL011State *s = (PL011State *)opaque;
>       int r;
>
> -    if (!(s->cr & PL011_CR_UARTEN) || !(s->cr & PL011_CR_RXE)) {
> +    if (!pl011_can_receive(s)) {
>           r = 0;
>       } else {
>           r = s->read_count < pl011_get_fifo_depth(s);
> @@ -459,7 +464,7 @@ static void pl011_realize(DeviceState *dev, Error
> **errp)
>   {
>       PL011State *s = PL011(dev);
>
> -    qemu_chr_fe_set_handlers(&s->chr, pl011_can_receive, pl011_receive,
> +    qemu_chr_fe_set_handlers(&s->chr, pl011_receivable_bytes,
> pl011_receive,
>                                pl011_event, NULL, s, NULL, true);
>   }
>
> ---
>
> with maybe a better name for pl011_receivable_bytes().

Our standard-ish name for the function you pass to
qemu_chr_fe_set_handlers() is either foo_can_receive
or foo_can_read, though. That is followed through in
the name of the function argument (fd_can_read),
its type (IOCanReadHandler), and the field it gets stored
in (CharBackend::chr_can_read). It's not a great convention
but at least it is a convention...

thanks
-- PMM

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

[Philippe Mathieu-Daudé]

On 23/1/23 17:23, Peter Maydell wrote:
> On Mon, 23 Jan 2023 at 15:21, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>> pl011_can_receive() returns the number of bytes that pl011_receive() can
>> accept, pl011_can_transmit() returns a boolean.
>>
>> I was thinking of:
>>
>> -- >8 --
>> diff --git a/hw/char/pl011.c b/hw/char/pl011.c
>> index dd20b76609..ea5769a31c 100644
>> --- a/hw/char/pl011.c
>> +++ b/hw/char/pl011.c
>> @@ -221,6 +221,11 @@ static inline bool pl011_can_transmit(PL011State *s)
>>        return s->cr & PL011_CR_UARTEN && s->cr & PL011_CR_TXE;
>>    }
>>
>> +static inline bool pl011_can_receive(PL011State *s)
>> +{
>> +    return s->cr & PL011_CR_UARTEN && s->cr & PL011_CR_RXE;
>> +}
>> +
>>    static void pl011_write(void *opaque, hwaddr offset,
>>                            uint64_t value, unsigned size)
>>    {
>> @@ -299,12 +304,12 @@ static void pl011_write(void *opaque, hwaddr offset,
>>        }
>>    }
>>
>> -static int pl011_can_receive(void *opaque)
>> +static int pl011_receivable_bytes(void *opaque)
>>    {
>>        PL011State *s = (PL011State *)opaque;
>>        int r;
>>
>> -    if (!(s->cr & PL011_CR_UARTEN) || !(s->cr & PL011_CR_RXE)) {
>> +    if (!pl011_can_receive(s)) {
>>            r = 0;
>>        } else {
>>            r = s->read_count < pl011_get_fifo_depth(s);
>> @@ -459,7 +464,7 @@ static void pl011_realize(DeviceState *dev, Error
>> **errp)
>>    {
>>        PL011State *s = PL011(dev);
>>
>> -    qemu_chr_fe_set_handlers(&s->chr, pl011_can_receive, pl011_receive,
>> +    qemu_chr_fe_set_handlers(&s->chr, pl011_receivable_bytes,
>> pl011_receive,
>>                                 pl011_event, NULL, s, NULL, true);
>>    }
>>
>> ---
>>
>> with maybe a better name for pl011_receivable_bytes().
> 
> Our standard-ish name for the function you pass to
> qemu_chr_fe_set_handlers() is either foo_can_receive
> or foo_can_read, though. That is followed through in
> the name of the function argument (fd_can_read),
> its type (IOCanReadHandler), and the field it gets stored
> in (CharBackend::chr_can_read). It's not a great convention
> but at least it is a convention...

I agree this deserves a better name; maybe this is not a
convention but I'd expect functions starting with is_* / can_*
to return a boolean value, not a number:

/**
  * IOCanReadHandler: Return the number of bytes that #IOReadHandler can 
accept
  *
  * This function reports how many bytes #IOReadHandler is prepared to 
accept.
  * #IOReadHandler may be invoked with up to this number of bytes.  If this
  * function returns 0 then #IOReadHandler is not invoked.
  *
  * This function is typically called from an event loop.  If the number of
  * bytes changes outside the event loop (e.g. because a vcpu thread 
drained the
  * buffer), then it is necessary to kick the event loop so that this 
function
  * is called again.  aio_notify() or qemu_notify_event() can be used to 
kick
  * the event loop.
  */
typedef int IOCanReadHandler(void *opaque);

Also, maybe using unsigned or size_t type for the return value would
better fit. Anyhow, not really a priority :)

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

[Philippe Mathieu-Daudé]

On 23/1/23 17:09, Evgeny Iakovlev wrote:
> 
> On 1/23/2023 16:59, Evgeny Iakovlev wrote:
>>
>> On 1/23/2023 16:21, Philippe Mathieu-Daudé wrote:
>>> On 23/1/23 15:43, Evgeny Iakovlev wrote:
>>>>
>>>> On 1/23/2023 09:14, Philippe Mathieu-Daudé wrote:
>>>>> On 20/1/23 16:54, Evgeny Iakovlev 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>
>>>>>> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>>>>>> ---
>>>>>>   hw/char/pl011.c | 22 +++++++++++++++++++---
>>>>>>   1 file changed, 19 insertions(+), 3 deletions(-)
>>>>>
>>>>>> +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)
>>>>>>   {
>>>>>> @@ -221,7 +231,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 */
>>>>>> @@ -292,7 +304,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)) {
>>>>>
>>>>> Maybe add pl011_can_receive() similarly to pl011_can_transmit().
>>>>>
>>>>> Otherwise:
>>>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>>
>>>>
>>>> Thanks! There's already a pl011_can_receive though, its the 
>>>> pl011_can_transmit that's new :)
>>>
>>> pl011_can_receive() returns the number of bytes that pl011_receive() 
>>> can accept, pl011_can_transmit() returns a boolean.
>>
>>
>> Hm, no, pl011_can_receive never actually returned the number of bytes. 
>> It's a boolean value as an int. Which is a bug, because 
>> qemu_chr_fe_set_handlers expects it to return the byte count, not a 
>> 0/1 value.
>>
>> I'll fix it then.
> 
> 
> Actually, no, i spoke too soon. qemu_chr_fe_set_handlers indeed expects 
> a number of bytes from pl011_can_receive, but pl011_can_receive also 
> deliberately gives it 1 and not how many bytes are there in queue 
> really, because everything else on the receive code path is written with 
> an assumption to only accept 1 element at a time (see pl011_put_fifo).

That would be great if we had a model actually using the FIFO, but
since nobody complained about it ...

> If we want to optimize this part, we would need to change that 
> assumption. That should better be done as a separate change though, 
> which i can send later.
No problem, this patch is fine. Thanks!

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

[Evgeny Iakovlev]

On 1/23/2023 17:41, Philippe Mathieu-Daudé wrote:
> On 23/1/23 17:23, Peter Maydell wrote:
>> On Mon, 23 Jan 2023 at 15:21, Philippe Mathieu-Daudé 
>> <philmd@linaro.org> wrote:
>>> pl011_can_receive() returns the number of bytes that pl011_receive() 
>>> can
>>> accept, pl011_can_transmit() returns a boolean.
>>>
>>> I was thinking of:
>>>
>>> -- >8 --
>>> diff --git a/hw/char/pl011.c b/hw/char/pl011.c
>>> index dd20b76609..ea5769a31c 100644
>>> --- a/hw/char/pl011.c
>>> +++ b/hw/char/pl011.c
>>> @@ -221,6 +221,11 @@ static inline bool 
>>> pl011_can_transmit(PL011State *s)
>>>        return s->cr & PL011_CR_UARTEN && s->cr & PL011_CR_TXE;
>>>    }
>>>
>>> +static inline bool pl011_can_receive(PL011State *s)
>>> +{
>>> +    return s->cr & PL011_CR_UARTEN && s->cr & PL011_CR_RXE;
>>> +}
>>> +
>>>    static void pl011_write(void *opaque, hwaddr offset,
>>>                            uint64_t value, unsigned size)
>>>    {
>>> @@ -299,12 +304,12 @@ static void pl011_write(void *opaque, hwaddr 
>>> offset,
>>>        }
>>>    }
>>>
>>> -static int pl011_can_receive(void *opaque)
>>> +static int pl011_receivable_bytes(void *opaque)
>>>    {
>>>        PL011State *s = (PL011State *)opaque;
>>>        int r;
>>>
>>> -    if (!(s->cr & PL011_CR_UARTEN) || !(s->cr & PL011_CR_RXE)) {
>>> +    if (!pl011_can_receive(s)) {
>>>            r = 0;
>>>        } else {
>>>            r = s->read_count < pl011_get_fifo_depth(s);
>>> @@ -459,7 +464,7 @@ static void pl011_realize(DeviceState *dev, Error
>>> **errp)
>>>    {
>>>        PL011State *s = PL011(dev);
>>>
>>> -    qemu_chr_fe_set_handlers(&s->chr, pl011_can_receive, 
>>> pl011_receive,
>>> +    qemu_chr_fe_set_handlers(&s->chr, pl011_receivable_bytes,
>>> pl011_receive,
>>>                                 pl011_event, NULL, s, NULL, true);
>>>    }
>>>
>>> ---
>>>
>>> with maybe a better name for pl011_receivable_bytes().
>>
>> Our standard-ish name for the function you pass to
>> qemu_chr_fe_set_handlers() is either foo_can_receive
>> or foo_can_read, though. That is followed through in
>> the name of the function argument (fd_can_read),
>> its type (IOCanReadHandler), and the field it gets stored
>> in (CharBackend::chr_can_read). It's not a great convention
>> but at least it is a convention...
>
> I agree this deserves a better name; maybe this is not a
> convention but I'd expect functions starting with is_* / can_*
> to return a boolean value, not a number:
>
> /**
>  * IOCanReadHandler: Return the number of bytes that #IOReadHandler 
> can accept
>  *
>  * This function reports how many bytes #IOReadHandler is prepared to 
> accept.
>  * #IOReadHandler may be invoked with up to this number of bytes. If this
>  * function returns 0 then #IOReadHandler is not invoked.
>  *
>  * This function is typically called from an event loop.  If the 
> number of
>  * bytes changes outside the event loop (e.g. because a vcpu thread 
> drained the
>  * buffer), then it is necessary to kick the event loop so that this 
> function
>  * is called again.  aio_notify() or qemu_notify_event() can be used 
> to kick
>  * the event loop.
>  */
> typedef int IOCanReadHandler(void *opaque);
>
> Also, maybe using unsigned or size_t type for the return value would
> better fit. Anyhow, not really a priority :)


I've made a rename for v4 though :) Feel free to ignore it then and take 
the current version, there's no functional change in 5/5 otherwise.