Re: [PATCH] hw/char/pl011: support backend hotswap

Am Mo., 10. Aug. 2026 um 16:58 Uhr schrieb Alex Bennée <[email protected]>:
>
> Alexander Mikhalitsyn <[email protected]> writes:
>
> > From: Aleksanr Mikhalitsyn <[email protected]>

Ugh, I did git format-patch and copied this patch from my Raspberry PI
dev/test machine
and it turns out that I have a stupid typo in my `git config get
user.name` on that machine.
I'll drop -v2 a bit later.

> >
> > Currently, when Incus issues "chardev-change" QMP command to change
> > chardev backend from ringbuf to socket it receives an error (with aarch64 VM):
> > "Chardev user does not support chardev hotswap" [1], [2]
> >
> > Let's fix this by properly implementing BackendChangeHandler for pl011.
> >
> > Link: https://discuss.linuxcontainers.org/t/unable-to-connect-to-vm-console-on-arm-architecture/23096/3 [1]
> > Link: https://github.com/lxc/distrobuilder/issues/892 [2]
> > Reported-by: Stéphane Graber <[email protected]>
> > Signed-off-by: Alexander Mikhalitsyn
> > <[email protected]>

Dear Alex,

first of all thank you very much for such a fast reaction and review ;-)

>
> At first I wondered why the FE needs to care about where the BE is
> routed to but 7bb86085e61 (char: chardevice hotswap) explains:
>
>   However, backends are not stateless and are set up by the frontends
>   via qemu_chr_fe_<> functions, and it's not (generally) possible to
>   replay that setup entirely in a backend code, as different chardevs
>   respond to the setup calls differently, so do frontends work
>   differently basing on those setup responses. Moreover, some frontend
>   can generally get and save the backend pointer
>   (qemu_chr_fe_get_driver()), and it will become invalid after backend
>   change.

yes, exactly

>
> > ---
> >  hw/char/pl011.c | 16 +++++++++++++++-
> >  1 file changed, 15 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/char/pl011.c b/hw/char/pl011.c
> > index cb12c3e224f..3622248ec0c 100644
> > --- a/hw/char/pl011.c
> > +++ b/hw/char/pl011.c
> > @@ -660,12 +660,26 @@ static void pl011_init(Object *obj)
> >      s->id = pl011_id_arm;
> >  }
> >
> > +static int pl011_be_change(void *opaque)
> > +{
> > +    PL011State *s = opaque;
> > +    int break_enable = s->lcr & LCR_BRK;
> > +
> > +    qemu_chr_fe_set_handlers(&s->chr, pl011_can_receive, pl011_receive,
> > +                             pl011_event, pl011_be_change, s, NULL, true);
> > +
> > +    qemu_chr_fe_ioctl(&s->chr, CHR_IOCTL_SERIAL_SET_BREAK,
> > +                      &break_enable);
>
> So this is just ensuring that if we switch to a serial backend we
> properly register the break behaviour on the backend? Is it idempotent?

Right. And AFAIU, `CHR_IOCTL_SERIAL_SET_BREAK` is specific to the
char-serial device only.

So when we switch, let's say, from a serial to a socket backend, this
call is basically a no-op.
But when we switch back from the socket to the serial, this will
trigger `tcsendbreak(fioc->fd, 1)`, which is not idempotent (and is a
single-shot action).

But I believe it's not a problem here, because we do it once when we
*switch* between two different backends, and `fioc->fd` is a fresh fd
every time.
My point is that we can't really trigger this thing twice for the same fd.

Please, correct me if I'm wrong. I took inspiration from 16550A's
serial_be_change() when working on this.

==== BEGIN offtopic :-)

Btw, thanks to your question I did some more research on tcsendbreak()
and BRK behavior [1] and it feels to me that what we currently have in
pl011 UARTLCR_H's register handling code is not really correct. See:

static void pl011_write(void *opaque, hwaddr offset,
                        uint64_t value, unsigned size)
{
<..>
    case 11: /* UARTLCR_H */
        /* Reset the FIFO state on FIFO enable or disable */
        if ((s->lcr ^ value) & LCR_FEN) {
            pl011_reset_rx_fifo(s);
            pl011_reset_tx_fifo(s);
        }
        if ((s->lcr ^ value) & LCR_BRK) {

           // << spec [1] says that setting BRK means "If this bit is
set to 1, a low-level is continually output on the UARTTXD output,
after completing transmission of the current character."
           // what we do is CHR_IOCTL_SERIAL_SET_BREAK which is just
tcsendbreak(fioc->fd, 1) under the hood which can't emulate continuous
low-level signal on the UARTTXD output
           // for the time of BRK bit is set. It only does this for a
short period of time.

            int break_enable = value & LCR_BRK;
            qemu_chr_fe_ioctl(&s->chr, CHR_IOCTL_SERIAL_SET_BREAK,
                              &break_enable);
            pl011_loopback_break(s, break_enable);
        }

I guess we need some pl011 expert in here :-)

[1] https://support.arm.com/documentation/ddi0183/g/programmers-model/register-descriptions/line-control-register--uartlcr-h

>
> > +
> > +    return 0;
> > +}
> > +
> >  static void pl011_realize(DeviceState *dev, Error **errp)
> >  {
> >      PL011State *s = PL011(dev);
> >
> >      qemu_chr_fe_set_handlers(&s->chr, pl011_can_receive, pl011_receive,
> > -                             pl011_event, NULL, s, NULL, true);
> > +                             pl011_event, pl011_be_change, s, NULL, true);
> >  }
>
> This does make me wonder if having a static pl011_set_handlers helper
> would keep things tidy and in one place? Either way:

I'm happy to do this too if you want me to. And probably for
hw/char/serial.c too, then?

>
> Reviewed-by: Alex Bennée <[email protected]>

Thanks ;-)

Giving all the suspicion around UARTLCR_H and BRK behavior I still
believe that I'm not making things
worse by this change, but better, cause now we can do backend hotswap.
But it would be great to fully
understand that part too.

Kind regards,
Alex

>
> >
> >  static void pl011_reset(DeviceState *dev)
>
> --
> Alex Bennée
> Virtualisation Tech Lead @ Linaro