[v6,0/7] hw/char/pl011: Implement TX (async) FIFO to avoid blocking the main loop

Message

Philippe Mathieu-Daudé Feb. 8, 2025, 4:39 p.m. UTC

Hi,

This series add support for (async) FIFO on the transmit path
of the PL011 UART.

Since v5:
- Rebased (few patches already merged)
- Do not forbid disabled UART/receiver (Peter)
- Use fifo8_peek API for wrapped buffer (Mark)

Since v4:
- Rebased (loopback)
- Addressed Richard & Juan migration comments
- Split in smaller patches

Since v3:
- Document migration bits (Alex, Richard)
- Just check FIFO is not empty in pl011_xmit_fifo_state_needed (rth)
- In pl011_xmit check TX enabled first, and ignore < 8-bit TX (rth)

Since v2:
- Added R-b tags
- Addressed Richard comments on migration

Since v1:
- Restrict pl011_ops[] impl access_size,
- Do not check transmitter is enabled (Peter),
- Addressed Alex's review comments,
- Simplified migration trying to care about backward compat,
  but still unsure...

Philippe Mathieu-Daudé (7):
  hw/char/pl011: Warn when using disabled receiver
  hw/char/pl011: Add transmit FIFO to PL011State
  hw/char/pl011: Introduce pl011_xmit() as GSource
  hw/char/pl011: Trace FIFO enablement
  hw/char/pl011: Consider TX FIFO overrun error
  hw/char/pl011: Drain TX FIFO when no backend connected
  hw/char/pl011: Implement TX FIFO

 include/hw/char/pl011.h |   2 +
 hw/char/pl011.c         | 140 +++++++++++++++++++++++++++++++++++++---
 hw/char/trace-events    |   9 +++
 3 files changed, 141 insertions(+), 10 deletions(-)

Comments

Peter Maydell Feb. 17, 2025, 2:55 p.m. UTC | #1

On Sat, 8 Feb 2025 at 16:39, Philippe Mathieu-Daudé <[e-mail]> wrote:
>
> Hi,
>
> This series add support for (async) FIFO on the transmit path
> of the PL011 UART.
>

Applied to target-arm.next, thanks (with a couple of minor
tweaks to two of the patches).

-- PMM

Peter Maydell Feb. 18, 2025, 1:54 p.m. UTC | #2

On Mon, 17 Feb 2025 at 14:55, Peter Maydell <[e-mail]> wrote:
>
> On Sat, 8 Feb 2025 at 16:39, Philippe Mathieu-Daudé <[e-mail]> wrote:
> >
> > Hi,
> >
> > This series add support for (async) FIFO on the transmit path
> > of the PL011 UART.
> >
>
> Applied to target-arm.next, thanks (with a couple of minor
> tweaks to two of the patches).

Unfortunately I seem to get failures in 'make check-functional'
with the last patch of this series applied. This is with a
clang sanitizer build on ubuntu 24.04:
'../../configure' '--cc=clang' '--cxx=clang++' '--enable-ubsan'
'--target-list=arm-softmmu,arm-linux-user,aarch64-softmmu,aarch64-linux-user'

and the tests that fail are:
42/44 qemu:func-thorough+func-aarch64-thorough+thorough /
func-aarch64-aarch64_xlnx_versal     TIMEOUT          90.01s   killed
by signal 15 SIGTERM
43/44 qemu:func-thorough+func-aarch64-thorough+thorough /
func-aarch64-aarch64_raspi4          TIMEOUT         480.01s   killed
by signal 15 SIGTERM
44/44 qemu:func-thorough+func-aarch64-thorough+thorough /
func-aarch64-aarch64_virt            TIMEOUT         720.01s   killed
by signal 15 SIGTERM

Looking at the test logs it looks like the test framework
starts QEMU but there is never any output from the guest
console.

I've dropped the patchset from target-arm.next for the moment.

thanks
-- PMM

Peter Maydell Feb. 20, 2025, 10:43 a.m. UTC | #3

On Tue, 18 Feb 2025 at 13:54, Peter Maydell <[e-mail]> wrote:
>
> On Mon, 17 Feb 2025 at 14:55, Peter Maydell <[e-mail]> wrote:
> >
> > On Sat, 8 Feb 2025 at 16:39, Philippe Mathieu-Daudé <[e-mail]> wrote:
> > >
> > > Hi,
> > >
> > > This series add support for (async) FIFO on the transmit path
> > > of the PL011 UART.
> > >
> >
> > Applied to target-arm.next, thanks (with a couple of minor
> > tweaks to two of the patches).
>
> Unfortunately I seem to get failures in 'make check-functional'
> with the last patch of this series applied.

I had a look at this this morning because I wondered if it
was a mistake in the style fixups I'd applied to the patches
on my end, and I found the bug fairly quickly. The problem is
that pl011_xmit() doesn't update the TXFE and TXFF FIFO empty/full
status flag bits when it removes characters from the FIFO.
So the guest kernel spins forever because TXFF is never unset.

The following patch fixes this for me (and also makes us not
set INT_TX for the case where we couldn't send any bytes to
the chardev, which I noticed reading the code rather than
because it had any visible bad effects):

--- a/hw/char/pl011.c
+++ b/hw/char/pl011.c
@@ -256,6 +256,7 @@ static gboolean pl011_xmit(void *do_not_use,
GIOCondition cond, void *opaque)
     int bytes_consumed;
     uint8_t buf[PL011_FIFO_DEPTH];
     uint32_t count;
+    bool emptied_fifo;

     count = fifo8_num_used(&s->xmit_fifo);
     trace_pl011_fifo_tx_xmit_used(count);
@@ -280,15 +281,24 @@ static gboolean pl011_xmit(void *do_not_use,
GIOCondition cond, void *opaque)
         /* Error in back-end: drain the fifo. */
         pl011_drain_tx(s);
         return G_SOURCE_REMOVE;
+    } else if (bytes_consumed == 0) {
+        /* Couldn't send anything, try again later */
+        return G_SOURCE_CONTINUE;
     }

     /* Pop the data we could transmit. */
     fifo8_drop(&s->xmit_fifo, bytes_consumed);
     s->int_level |= INT_TX;
+    s->flags &= ~PL011_FLAG_TXFF;
+
+    emptied_fifo = fifo8_is_empty(&s->xmit_fifo);
+    if (emptied_fifo) {
+        s->flags |= PL011_FLAG_TXFE;
+    }

     pl011_update(s);

-    if (!fifo8_is_empty(&s->xmit_fifo)) {
+    if (!emptied_fifo) {
         /* Reschedule another transmission if we couldn't transmit all. */
         return G_SOURCE_CONTINUE;
     }

If you're OK with that as a fix then I'll squash it in
and keep the series in target-arm.next.

thanks
-- PMM

Peter Maydell Feb. 20, 2025, 10:52 a.m. UTC | #4

On Thu, 20 Feb 2025 at 10:43, Peter Maydell <[e-mail]> wrote:
>
> On Tue, 18 Feb 2025 at 13:54, Peter Maydell <[e-mail]> wrote:
> >
> > On Mon, 17 Feb 2025 at 14:55, Peter Maydell <[e-mail]> wrote:
> > >
> > > On Sat, 8 Feb 2025 at 16:39, Philippe Mathieu-Daudé <[e-mail]> wrote:
> > > >
> > > > Hi,
> > > >
> > > > This series add support for (async) FIFO on the transmit path
> > > > of the PL011 UART.
> > > >
> > >
> > > Applied to target-arm.next, thanks (with a couple of minor
> > > tweaks to two of the patches).
> >
> > Unfortunately I seem to get failures in 'make check-functional'
> > with the last patch of this series applied.
>
> I had a look at this this morning because I wondered if it
> was a mistake in the style fixups I'd applied to the patches
> on my end, and I found the bug fairly quickly. The problem is
> that pl011_xmit() doesn't update the TXFE and TXFF FIFO empty/full
> status flag bits when it removes characters from the FIFO.
> So the guest kernel spins forever because TXFF is never unset.
>
> The following patch fixes this for me (and also makes us not
> set INT_TX for the case where we couldn't send any bytes to
> the chardev, which I noticed reading the code rather than
> because it had any visible bad effects):

Hmm, but that's clearly not the only problem -- it fixed the
"no output at all issue", but now I see a test failure because
of garbled console output:

2025-02-20 10:34:51,562: Booting Linux on physical CPU 0x0
2025-02-20 10:34:51,563: Linux version 4.18.16-300.fc29.armv7hl
([e-mail]) (gcc version 8.2.1
20180801 (Red Hat 8.2.1-2) (GCC)) #1 SMP Sun Oct 21 00:56:28 UTC 2018
2025-02-20 10:34:51,563: CPU: ARMv7 Processor [414fc0f0] revision 0
(ARMv7), cr=10c5387d
2025-02-20 10:34:51,564: CPU: div instructions available: patching division code
2025-02-20 10:34:51,564: CPU: PIPT / VIPT nonaliasing data cache, PIPT
instruction cache
2025-02-20 10:34:51,565: OF: fdt: Machine model: linux,dummy-virt
2025-02-20 10:34:51,565: Memory policy: Data cache writealloc
2025-02-20 10:34:51,565: efi: Getting EFI parameters from FDT:
2025-02-20 10:34:51,566: efi: UEFI not found.
2025-02-20 10:34:51,566: cma: aetrsv6i
2025-02-20 10:34:51,566: psci: oi ocdteofmT
2025-02-20 10:34:51,566: ps:Sv1ecdniwe^Mps:Usdvni spsc:rt  gtnot psci:
SMC Calling Conventiov0
2025-02-20 10:34:51,567: pec:Eee1pec pvls9 1 2865^MBul1olt bi4^MKerloa
n:itte nltAA
2025-02-20 10:34:51,568: Det c stlere 3 rr4 5i 9(dr336bt)Memr 06112aib
6Keece11K ra0 ,21 sv,Kmrsv,Kim)Virtual kernel memory layout:
2025-02-20 10:34:51,568: vt :xf0  fa  f00 0f00 30 (0M^M lm  c00
0800xe0 0000  2M^M mue:x00 0f00  4B
2025-02-20 10:34:51,568: et x(tvl) (rl 97B
2025-02-20 10:34:51,569: n 0(trvl-xta) 0  0pv) 3 B
2025-02-20 10:34:51,570: bs prl-xta) 5 )random: get_random_u32 ca
___+:ag6 d=3MOes,P=,os^Mfta:aotg78ni  1asHierhlC pmti.        RCrtcnC
oN_S2orpi=.   TasCebd
2025-02-20 10:34:51,571: RC jtgeeyor_o_a1 __d^MNRIS1 rr GIC physical
location is 0x800000^MGIC2 n[mx00-02f,P81]arcte 1tm(
nna65H(r.cemk0fffffacl:0d20,aish_o:6i  M,routo1n per49419sn^MCocrm
dexCalibrating delay ose eceiife..oIl2^Mpxdl6i3
2025-02-20 10:34:51,572: SertFmo ia^MYemgmdl
2025-02-20 10:34:51,572: SEn: ili.Moutcea b te 2(d:,4t^MMoupnch
stlers14oe  9oeyo
2025-02-20 10:34:51,572: CP0Screv:iwein tuia norieI tst lrl
2025-02-20 10:34:51,573: /cpus/cpu@0 missing clock-frequen or^MCPU
rd1c  k  i 00^MSetgptiintm rx30 000Hircc Cmento^MEF rc ln  aae^Msm ii
cdrCs.smp ohu1o, USMPTao1reo cvae(15. gP.CPUA U)tt  Code.devtmpfs:
initialize^Mmo4aheu pr3vit e0cloejfe s fffm_cs0fffm_d_:924250sfut stbs
2025-02-20 10:34:51,574: piclo:niidit uye
2025-02-20 10:34:51,574: DM tre  vi^MNETeDM:rlae2  o tmi oerent aloaios
2025-02-20 10:34:51,574: audit: initializing netlink subsys (disabled)
2025-02-20 10:34:51,574: audit: type=2000 audit(0.480:1):
sat=ntalzdadt_nbe= e=^Mcpuidl:usn oenrmnu
2025-02-20 10:34:51,574: No ATG?
2025-02-20 10:34:51,575: hw-brapit fud5(1rsrvd rapon n  achon
eitrs^Mhw-brapit:mxmmwthontsz s8bts^MSerial MAP01 Ap1:ttAA tMIO09000ud
= 0) saP01 e1
2025-02-20 10:34:51,575: console [ttyAMA0] enabled
2025-02-20 10:34:51,668: cryptd: max_cpu_qlen set to 1000
2025-02-20 10:34:51,690: vgaarb: loaded
2025-02-20 10:34:51,697: SCSI subsystem initialized
2025-02-20 10:34:51,702: usbcore: registered new interface driver usbfs
2025-02-20 10:34:51,703: usbcore: registered new interface driver hub

(the log has no garbling after that so it's presumably OK with
the interrupt-driven real UART driver but the polling one you get
for earlycon has trouble.)

I also noticed that pl011_write_txdata() doesn't clear TXFE
when it puts a byte into the fifo -- I'm testing to see if
fixing that helps.

-- PMM

Peter Maydell Feb. 20, 2025, 11:04 a.m. UTC | #5

On Thu, 20 Feb 2025 at 10:52, Peter Maydell <[e-mail]> wrote:
>
> On Thu, 20 Feb 2025 at 10:43, Peter Maydell <[e-mail]> wrote:
> >
> > On Tue, 18 Feb 2025 at 13:54, Peter Maydell <[e-mail]> wrote:
> > >
> > > On Mon, 17 Feb 2025 at 14:55, Peter Maydell <[e-mail]> wrote:
> > > >
> > > > On Sat, 8 Feb 2025 at 16:39, Philippe Mathieu-Daudé <[e-mail]> wrote:
> > > > >
> > > > > Hi,
> > > > >
> > > > > This series add support for (async) FIFO on the transmit path
> > > > > of the PL011 UART.
> > > > >
> > > >
> > > > Applied to target-arm.next, thanks (with a couple of minor
> > > > tweaks to two of the patches).
> > >
> > > Unfortunately I seem to get failures in 'make check-functional'
> > > with the last patch of this series applied.
> >
> > I had a look at this this morning because I wondered if it
> > was a mistake in the style fixups I'd applied to the patches
> > on my end, and I found the bug fairly quickly. The problem is
> > that pl011_xmit() doesn't update the TXFE and TXFF FIFO empty/full
> > status flag bits when it removes characters from the FIFO.
> > So the guest kernel spins forever because TXFF is never unset.
> >
> > The following patch fixes this for me (and also makes us not
> > set INT_TX for the case where we couldn't send any bytes to
> > the chardev, which I noticed reading the code rather than
> > because it had any visible bad effects):
>
> Hmm, but that's clearly not the only problem -- it fixed the
> "no output at all issue", but now I see a test failure because
> of garbled console output:

> I also noticed that pl011_write_txdata() doesn't clear TXFE
> when it puts a byte into the fifo -- I'm testing to see if
> fixing that helps.

Yes, with this patch also:

--- a/hw/char/pl011.c
+++ b/hw/char/pl011.c
@@ -330,6 +330,7 @@ static void pl011_write_txdata(PL011State *s, uint8_t data)
     if (pl011_is_tx_fifo_full(s)) {
         s->flags |= PL011_FLAG_TXFF;
     }
+    s->flags &= ~PL011_FLAG_TXFE;

     pl011_xmit(NULL, G_IO_OUT, s);
 }

this remaining failure is fixed and I get a clean pass (other than
the gpu test failure, but that's not related to the pl011).

-- PMM