| Message ID | 20240719181041.49545-17-philmd@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/char/pl011: Implement TX (async) FIFO to avoid blocking the main loop | expand |
On 19/07/2024 19:10, Philippe Mathieu-Daudé wrote: > If the UART back-end chardev doesn't drain data as fast as stdout > does or blocks, buffer in the TX FIFO to try again later. > > This avoids having the IO-thread busy waiting on chardev back-ends, > reported recently when testing the Trusted Reference Stack and > using the socket backend. > > Implement registering a front-end 'watch' callback on back-end > events, so we can resume transmitting when the back-end is writable > again, not blocking the main loop. > > Similarly to the RX FIFO path, FIFO level selection is not > implemented (interrupt is triggered when a single byte is available > in the FIFO). > > Reported-by: Mikko Rapeli <mikko.rapeli@linaro.org> > Suggested-by: Alex Bennée <alex.bennee@linaro.org> > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > --- > RFC: Something is still broken, some characters are emitted async... > --- > hw/char/pl011.c | 60 ++++++++++++++++++++++++++++++++++++-------- > hw/char/trace-events | 1 + > 2 files changed, 51 insertions(+), 10 deletions(-) > > diff --git a/hw/char/pl011.c b/hw/char/pl011.c > index cfa3fd3da4..9f72b6a765 100644 > --- a/hw/char/pl011.c > +++ b/hw/char/pl011.c > @@ -240,7 +240,9 @@ static gboolean pl011_xmit(void *do_not_use, GIOCondition cond, void *opaque) > { > PL011State *s = opaque; > int bytes_consumed; > - uint8_t data; > + const uint8_t *buf; > + uint32_t buflen; > + uint32_t count; > > if (!(s->cr & CR_UARTEN)) { > qemu_log_mask(LOG_GUEST_ERROR, "PL011 data written to disabled UART\n"); > @@ -249,25 +251,40 @@ static gboolean pl011_xmit(void *do_not_use, GIOCondition cond, void *opaque) > qemu_log_mask(LOG_GUEST_ERROR, "PL011 data written to disabled TX UART\n"); > } > > + count = fifo8_num_used(&s->xmit_fifo); > + if (count < 1) { > + /* FIFO empty */ > + return G_SOURCE_REMOVE; > + } > + > if (!qemu_chr_fe_backend_connected(&s->chr)) { > /* Instant drain the fifo when there's no back-end. */ > pl011_drain_tx(s); > return G_SOURCE_REMOVE; > } > > - data = fifo8_pop(&s->xmit_fifo); > - bytes_consumed = 1; > + buf = fifo8_peek_buf(&s->xmit_fifo, count, &buflen); > > - /* > - * XXX this blocks entire thread. Rewrite to use > - * qemu_chr_fe_write and background I/O callbacks > - */ > - qemu_chr_fe_write_all(&s->chr, &data, bytes_consumed); > + /* Transmit as much data as we can. */ > + bytes_consumed = qemu_chr_fe_write(&s->chr, buf, buflen); > trace_pl011_fifo_tx_xmit(bytes_consumed); > + if (bytes_consumed < 0) { > + /* Error in back-end: drain the fifo. */ > + pl011_drain_tx(s); > + return G_SOURCE_REMOVE; > + } > + > + /* Pop the data we could transmit. */ > + fifo8_pop_buf(&s->xmit_fifo, bytes_consumed, NULL); > s->int_level |= INT_TX; > > pl011_update(s); One of the gotchas with Fifo8 is that whilst fifo8_push(), fifo8_pop() and fifo8_push_all() will wrap the FIFO buffer, fifo8_{peek,pop}_buf() do not. For example fifo8_num_used() could return 15, but if xmit_fifo->head is set to 15 then fifo8_{peek_pop}_buf() would return 1 leaving 14 characters in the FIFO at the end of pl011_xmit(). Possible solutions could be to use a loop to send one character at a time similar to: while (fifo8_num_used(&s->xmit_fifo)) { uint8_t c = fifo8_pop(&s->xmit_fifo); if (qemu_chr_fe_write(&s->chr, &c, 1) == -1) { fifo8_push(&s->xmit_fifo, c); break; } } Or else use a solution similar to the one I used for ESP at https://gitlab.com/qemu-project/qemu/-/blob/master/hw/scsi/esp.c?ref_type=heads#L200. I did think about whether it was worth adding a function similar to the one used for ESP to the Fifo8 API, but wasn't sure it was worth it at the time. > + if (!fifo8_is_empty(&s->xmit_fifo)) { > + /* Reschedule another transmission if we couldn't transmit all. */ > + return G_SOURCE_CONTINUE; > + } > + > return G_SOURCE_REMOVE; > } > > @@ -290,6 +307,10 @@ static void pl011_write_txdata(PL011State *s, uint8_t data) > trace_pl011_fifo_tx_put(data); > pl011_loopback_tx(s, data); > fifo8_push(&s->xmit_fifo, data); > + if (fifo8_is_full(&s->xmit_fifo)) { > + s->flags |= PL011_FLAG_TXFF; > + } > + > pl011_xmit(NULL, G_IO_OUT, s); > } > > @@ -488,10 +509,24 @@ static void pl011_write(void *opaque, hwaddr offset, > pl011_trace_baudrate_change(s); > break; > case 11: /* UARTLCR_H */ > - /* Reset the FIFO state on FIFO enable or disable */ > if ((s->lcr ^ value) & LCR_FEN) { > - pl011_reset_rx_fifo(s); > + bool fifo_enabled = value & LCR_FEN; > + > + trace_pl011_fifo_enable(fifo_enabled); > + if (fifo_enabled) { > + /* Transmit and receive FIFO buffers are enabled (FIFO mode). */ > + fifo8_change_capacity(&s->xmit_fifo, PL011_FIFO_DEPTH); > + } else { > + /* > + * FIFOs are disabled (character mode) that is, the FIFOs > + * become 1-byte-deep holding registers. > + */ > + pl011_drain_tx(s); > + fifo8_change_capacity(&s->xmit_fifo, 1); > + } Presumably this is the part where fifo8_change_capacity() is required: what does changing the FIFO size to 1 do here? Is it possible to move the fifo_enabled check into pl011_read() and pop/clear the buffer there instead of changing the FIFO size? > + /* Reset the FIFO state on FIFO enable or disable */ > pl011_reset_tx_fifo(s); > + pl011_reset_rx_fifo(s); > } > if ((s->lcr ^ value) & LCR_BRK) { > int break_enable = value & LCR_BRK; > @@ -636,6 +671,11 @@ static int pl011_post_load(void *opaque, int version_id) > s->read_pos = 0; > } > > + if (!fifo8_is_empty(&s->xmit_fifo)) { > + /* Reschedule another transmission */ > + qemu_chr_fe_add_watch(&s->chr, G_IO_OUT | G_IO_HUP, pl011_xmit, s); > + } > + > s->ibrd &= IBRD_MASK; > s->fbrd &= FBRD_MASK; > > diff --git a/hw/char/trace-events b/hw/char/trace-events > index bf586ba664..2405819812 100644 > --- a/hw/char/trace-events > +++ b/hw/char/trace-events > @@ -58,6 +58,7 @@ pl011_read(uint32_t addr, uint32_t value, const char *regname) "addr 0x%03x valu > pl011_read_fifo(int read_count) "FIFO read, read_count now %d" > pl011_write(uint32_t addr, uint32_t value, const char *regname) "addr 0x%03x value 0x%08x reg %s" > pl011_can_receive(uint32_t lcr, int read_count, int r) "LCR 0x%08x read_count %d returning %d" > +pl011_fifo_enable(bool enable) "enable:%u" > pl011_fifo_rx_put(uint32_t c, int read_count) "new char 0x%02x read_count now %d" > pl011_fifo_rx_full(void) "RX FIFO now full, RXFF set" > pl011_fifo_tx_put(uint8_t byte) "TX FIFO push char [0x%02x]" ATB, Mark.
On 19/7/24 23:25, Mark Cave-Ayland wrote: > On 19/07/2024 19:10, Philippe Mathieu-Daudé wrote: > >> If the UART back-end chardev doesn't drain data as fast as stdout >> does or blocks, buffer in the TX FIFO to try again later. >> >> This avoids having the IO-thread busy waiting on chardev back-ends, >> reported recently when testing the Trusted Reference Stack and >> using the socket backend. >> >> Implement registering a front-end 'watch' callback on back-end >> events, so we can resume transmitting when the back-end is writable >> again, not blocking the main loop. >> >> Similarly to the RX FIFO path, FIFO level selection is not >> implemented (interrupt is triggered when a single byte is available >> in the FIFO). >> >> Reported-by: Mikko Rapeli <mikko.rapeli@linaro.org> >> Suggested-by: Alex Bennée <alex.bennee@linaro.org> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >> --- >> RFC: Something is still broken, some characters are emitted async... [*] >> --- >> hw/char/pl011.c | 60 ++++++++++++++++++++++++++++++++++++-------- >> hw/char/trace-events | 1 + >> 2 files changed, 51 insertions(+), 10 deletions(-) >> >> diff --git a/hw/char/pl011.c b/hw/char/pl011.c >> index cfa3fd3da4..9f72b6a765 100644 >> --- a/hw/char/pl011.c >> +++ b/hw/char/pl011.c >> @@ -240,7 +240,9 @@ static gboolean pl011_xmit(void *do_not_use, >> GIOCondition cond, void *opaque) >> { >> PL011State *s = opaque; >> int bytes_consumed; >> - uint8_t data; >> + const uint8_t *buf; >> + uint32_t buflen; >> + uint32_t count; >> if (!(s->cr & CR_UARTEN)) { >> qemu_log_mask(LOG_GUEST_ERROR, "PL011 data written to >> disabled UART\n"); >> @@ -249,25 +251,40 @@ static gboolean pl011_xmit(void *do_not_use, >> GIOCondition cond, void *opaque) >> qemu_log_mask(LOG_GUEST_ERROR, "PL011 data written to >> disabled TX UART\n"); >> } >> + count = fifo8_num_used(&s->xmit_fifo); >> + if (count < 1) { >> + /* FIFO empty */ >> + return G_SOURCE_REMOVE; >> + } >> + >> if (!qemu_chr_fe_backend_connected(&s->chr)) { >> /* Instant drain the fifo when there's no back-end. */ >> pl011_drain_tx(s); >> return G_SOURCE_REMOVE; >> } >> - data = fifo8_pop(&s->xmit_fifo); >> - bytes_consumed = 1; >> + buf = fifo8_peek_buf(&s->xmit_fifo, count, &buflen); >> - /* >> - * XXX this blocks entire thread. Rewrite to use >> - * qemu_chr_fe_write and background I/O callbacks >> - */ >> - qemu_chr_fe_write_all(&s->chr, &data, bytes_consumed); >> + /* Transmit as much data as we can. */ >> + bytes_consumed = qemu_chr_fe_write(&s->chr, buf, buflen); >> trace_pl011_fifo_tx_xmit(bytes_consumed); >> + if (bytes_consumed < 0) { >> + /* Error in back-end: drain the fifo. */ >> + pl011_drain_tx(s); >> + return G_SOURCE_REMOVE; >> + } >> + >> + /* Pop the data we could transmit. */ >> + fifo8_pop_buf(&s->xmit_fifo, bytes_consumed, NULL); >> s->int_level |= INT_TX; >> pl011_update(s); > > One of the gotchas with Fifo8 is that whilst fifo8_push(), fifo8_pop() > and fifo8_push_all() will wrap the FIFO buffer, fifo8_{peek,pop}_buf() > do not. For example fifo8_num_used() could return 15, but if > xmit_fifo->head is set to 15 then fifo8_{peek_pop}_buf() would return 1 > leaving 14 characters in the FIFO at the end of pl011_xmit(). Wow, indeed this is the problem [*] I was facing :) <3 > Possible solutions could be to use a loop to send one character at a > time similar to: > > while (fifo8_num_used(&s->xmit_fifo)) { > uint8_t c = fifo8_pop(&s->xmit_fifo); > > if (qemu_chr_fe_write(&s->chr, &c, 1) == -1) { > fifo8_push(&s->xmit_fifo, c); > break; > } > } > > Or else use a solution similar to the one I used for ESP at > https://gitlab.com/qemu-project/qemu/-/blob/master/hw/scsi/esp.c?ref_type=heads#L200. I did think about whether it was worth adding a function similar to the one used for ESP to the Fifo8 API, but wasn't sure it was worth it at the time. Yeah clearly this DTRT. I'll extract this method. > >> + if (!fifo8_is_empty(&s->xmit_fifo)) { >> + /* Reschedule another transmission if we couldn't transmit >> all. */ >> + return G_SOURCE_CONTINUE; >> + } >> + >> return G_SOURCE_REMOVE; >> } >> @@ -290,6 +307,10 @@ static void pl011_write_txdata(PL011State *s, >> uint8_t data) >> trace_pl011_fifo_tx_put(data); >> pl011_loopback_tx(s, data); >> fifo8_push(&s->xmit_fifo, data); >> + if (fifo8_is_full(&s->xmit_fifo)) { >> + s->flags |= PL011_FLAG_TXFF; >> + } >> + >> pl011_xmit(NULL, G_IO_OUT, s); >> } >> @@ -488,10 +509,24 @@ static void pl011_write(void *opaque, hwaddr >> offset, >> pl011_trace_baudrate_change(s); >> break; >> case 11: /* UARTLCR_H */ >> - /* Reset the FIFO state on FIFO enable or disable */ >> if ((s->lcr ^ value) & LCR_FEN) { >> - pl011_reset_rx_fifo(s); >> + bool fifo_enabled = value & LCR_FEN; >> + >> + trace_pl011_fifo_enable(fifo_enabled); >> + if (fifo_enabled) { >> + /* Transmit and receive FIFO buffers are enabled >> (FIFO mode). */ >> + fifo8_change_capacity(&s->xmit_fifo, PL011_FIFO_DEPTH); >> + } else { >> + /* >> + * FIFOs are disabled (character mode) that is, the >> FIFOs >> + * become 1-byte-deep holding registers. >> + */ >> + pl011_drain_tx(s); >> + fifo8_change_capacity(&s->xmit_fifo, 1); >> + } > > Presumably this is the part where fifo8_change_capacity() is required: > what does changing the FIFO size to 1 do here? Is it possible to move > the fifo_enabled check into pl011_read() and pop/clear the buffer there > instead of changing the FIFO size? Again I tried to stay close to the datasheet description: 3.3.7 Line Control Register, UARTLCR_H Bit FEN: Enable FIFOs 0 = FIFOs are disabled (character mode) that is, the FIFOs become 1-byte-deep holding registers 1 = transmit and receive FIFO buffers are enabled (FIFO mode). But indeed it could be clearer to handle the FEN bit and do FIFO enabled check elsewhere, I'll have a look. Many thanks for your review!! >> + /* Reset the FIFO state on FIFO enable or disable */ >> pl011_reset_tx_fifo(s); >> + pl011_reset_rx_fifo(s); >> } >> if ((s->lcr ^ value) & LCR_BRK) { >> int break_enable = value & LCR_BRK; >> @@ -636,6 +671,11 @@ static int pl011_post_load(void *opaque, int >> version_id) >> s->read_pos = 0; >> } >> + if (!fifo8_is_empty(&s->xmit_fifo)) { >> + /* Reschedule another transmission */ >> + qemu_chr_fe_add_watch(&s->chr, G_IO_OUT | G_IO_HUP, >> pl011_xmit, s); >> + }
diff --git a/hw/char/pl011.c b/hw/char/pl011.c index cfa3fd3da4..9f72b6a765 100644 --- a/hw/char/pl011.c +++ b/hw/char/pl011.c @@ -240,7 +240,9 @@ static gboolean pl011_xmit(void *do_not_use, GIOCondition cond, void *opaque) { PL011State *s = opaque; int bytes_consumed; - uint8_t data; + const uint8_t *buf; + uint32_t buflen; + uint32_t count; if (!(s->cr & CR_UARTEN)) { qemu_log_mask(LOG_GUEST_ERROR, "PL011 data written to disabled UART\n"); @@ -249,25 +251,40 @@ static gboolean pl011_xmit(void *do_not_use, GIOCondition cond, void *opaque) qemu_log_mask(LOG_GUEST_ERROR, "PL011 data written to disabled TX UART\n"); } + count = fifo8_num_used(&s->xmit_fifo); + if (count < 1) { + /* FIFO empty */ + return G_SOURCE_REMOVE; + } + if (!qemu_chr_fe_backend_connected(&s->chr)) { /* Instant drain the fifo when there's no back-end. */ pl011_drain_tx(s); return G_SOURCE_REMOVE; } - data = fifo8_pop(&s->xmit_fifo); - bytes_consumed = 1; + buf = fifo8_peek_buf(&s->xmit_fifo, count, &buflen); - /* - * XXX this blocks entire thread. Rewrite to use - * qemu_chr_fe_write and background I/O callbacks - */ - qemu_chr_fe_write_all(&s->chr, &data, bytes_consumed); + /* Transmit as much data as we can. */ + bytes_consumed = qemu_chr_fe_write(&s->chr, buf, buflen); trace_pl011_fifo_tx_xmit(bytes_consumed); + if (bytes_consumed < 0) { + /* Error in back-end: drain the fifo. */ + pl011_drain_tx(s); + return G_SOURCE_REMOVE; + } + + /* Pop the data we could transmit. */ + fifo8_pop_buf(&s->xmit_fifo, bytes_consumed, NULL); s->int_level |= INT_TX; pl011_update(s); + if (!fifo8_is_empty(&s->xmit_fifo)) { + /* Reschedule another transmission if we couldn't transmit all. */ + return G_SOURCE_CONTINUE; + } + return G_SOURCE_REMOVE; } @@ -290,6 +307,10 @@ static void pl011_write_txdata(PL011State *s, uint8_t data) trace_pl011_fifo_tx_put(data); pl011_loopback_tx(s, data); fifo8_push(&s->xmit_fifo, data); + if (fifo8_is_full(&s->xmit_fifo)) { + s->flags |= PL011_FLAG_TXFF; + } + pl011_xmit(NULL, G_IO_OUT, s); } @@ -488,10 +509,24 @@ static void pl011_write(void *opaque, hwaddr offset, pl011_trace_baudrate_change(s); break; case 11: /* UARTLCR_H */ - /* Reset the FIFO state on FIFO enable or disable */ if ((s->lcr ^ value) & LCR_FEN) { - pl011_reset_rx_fifo(s); + bool fifo_enabled = value & LCR_FEN; + + trace_pl011_fifo_enable(fifo_enabled); + if (fifo_enabled) { + /* Transmit and receive FIFO buffers are enabled (FIFO mode). */ + fifo8_change_capacity(&s->xmit_fifo, PL011_FIFO_DEPTH); + } else { + /* + * FIFOs are disabled (character mode) that is, the FIFOs + * become 1-byte-deep holding registers. + */ + pl011_drain_tx(s); + fifo8_change_capacity(&s->xmit_fifo, 1); + } + /* Reset the FIFO state on FIFO enable or disable */ pl011_reset_tx_fifo(s); + pl011_reset_rx_fifo(s); } if ((s->lcr ^ value) & LCR_BRK) { int break_enable = value & LCR_BRK; @@ -636,6 +671,11 @@ static int pl011_post_load(void *opaque, int version_id) s->read_pos = 0; } + if (!fifo8_is_empty(&s->xmit_fifo)) { + /* Reschedule another transmission */ + qemu_chr_fe_add_watch(&s->chr, G_IO_OUT | G_IO_HUP, pl011_xmit, s); + } + s->ibrd &= IBRD_MASK; s->fbrd &= FBRD_MASK; diff --git a/hw/char/trace-events b/hw/char/trace-events index bf586ba664..2405819812 100644 --- a/hw/char/trace-events +++ b/hw/char/trace-events @@ -58,6 +58,7 @@ pl011_read(uint32_t addr, uint32_t value, const char *regname) "addr 0x%03x valu pl011_read_fifo(int read_count) "FIFO read, read_count now %d" pl011_write(uint32_t addr, uint32_t value, const char *regname) "addr 0x%03x value 0x%08x reg %s" pl011_can_receive(uint32_t lcr, int read_count, int r) "LCR 0x%08x read_count %d returning %d" +pl011_fifo_enable(bool enable) "enable:%u" pl011_fifo_rx_put(uint32_t c, int read_count) "new char 0x%02x read_count now %d" pl011_fifo_rx_full(void) "RX FIFO now full, RXFF set" pl011_fifo_tx_put(uint8_t byte) "TX FIFO push char [0x%02x]"
If the UART back-end chardev doesn't drain data as fast as stdout does or blocks, buffer in the TX FIFO to try again later. This avoids having the IO-thread busy waiting on chardev back-ends, reported recently when testing the Trusted Reference Stack and using the socket backend. Implement registering a front-end 'watch' callback on back-end events, so we can resume transmitting when the back-end is writable again, not blocking the main loop. Similarly to the RX FIFO path, FIFO level selection is not implemented (interrupt is triggered when a single byte is available in the FIFO). Reported-by: Mikko Rapeli <mikko.rapeli@linaro.org> Suggested-by: Alex Bennée <alex.bennee@linaro.org> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- RFC: Something is still broken, some characters are emitted async... --- hw/char/pl011.c | 60 ++++++++++++++++++++++++++++++++++++-------- hw/char/trace-events | 1 + 2 files changed, 51 insertions(+), 10 deletions(-)