Am Fr., 21. Aug. 2026 um 11:57 Uhr schrieb Peter Maydell
<[email protected]>:
>
> On Mon, 17 Aug 2026 at 12:33, Alexander Mikhalitsyn
> <[email protected]> wrote:
> >
> > From: Alexander Mikhalitsyn <[email protected]>
> >
> > 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.
> >
> > Please, note that we have to "replay" CHR_IOCTL_SERIAL_SET_BREAK, because
> > if BRK bit was set before backend change (i.e. (s->lcr & LCR_BRK) is true),
> > then after change we need to send break to a new backend too.
> >
> > 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]>
> > Reviewed-by: Alex Bennée <[email protected]>
> > Signed-off-by: Alexander Mikhalitsyn <[email protected]>
> > ---
> > v3:
> > - introduced pl011_set_handlers()
> > [ as suggested by Alex Bennée ]
> > - introduced pl011_set_break()
> > [ as suggested by Philippe Mathieu-Daudé ]
> > v2:
> > - fixed a typo in commit author name
> > [ 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. ]
> > - added RWB tag from Alex Bennée
> > - adjusted a commit message
>
> I have one review suggestion here:
>
> > +static inline int pl011_set_break(PL011State *s, uint64_t lcr)
> > +{
> > + int break_enable = lcr & LCR_BRK;
> > +
> > + qemu_chr_fe_ioctl(&s->chr, CHR_IOCTL_SERIAL_SET_BREAK, &break_enable);
> > +
> > + return break_enable;
> > +}
>
> I think this would be better with a similar API to
> pl011_loopback_break(): make it take a bool brk_enable,
> and return void. (pl011_loopback_break() takes an int,
> but we shouldn't copy that: it uses it as a bool, so it
> ought to take a bool.)
Dear Peter,
Sure, I've changed this ;-) The only thing is that I kept brk_enable
as int, because in serial_chr_ioctl we have:
static int serial_chr_ioctl(Chardev *chr, int cmd, void *arg)
{
<...>
case CHR_IOCTL_SERIAL_SET_BREAK:
{
int enable = *(int *)arg; // << int is assumed
if (enable) {
tcsendbreak(fioc->fd, 1);
}
}
I'll send -v4 in a moment.
Kind regards,
Alex
>
> > static void pl011_write(void *opaque, hwaddr offset,
> > uint64_t value, unsigned size)
> > {
> > @@ -462,9 +471,7 @@ static void pl011_write(void *opaque, hwaddr offset,
> > pl011_reset_tx_fifo(s);
> > }
> > if ((s->lcr ^ value) & LCR_BRK) {
> > - int break_enable = value & LCR_BRK;
> > - qemu_chr_fe_ioctl(&s->chr, CHR_IOCTL_SERIAL_SET_BREAK,
> > - &break_enable);
> > + int break_enable = pl011_set_break(s, value);
> > pl011_loopback_break(s, break_enable);
>
> Then this code becomes something like
> bool break_enable = value & LCR_BRK;
> pl011_set_break(s, break_enable);
> pl011_loopback_break(s, break_enable);
>
> which is more straightforward than passing the raw LCR value
> to pl011_set_break and relying on it returning the "is BRK set?"
> information that we then pass to pl011_loopback_break().
+
>
> > +static int pl011_be_change(void *opaque);
>
> > +static inline void pl011_set_handlers(PL011State *s)
> > +{
> > + qemu_chr_fe_set_handlers(&s->chr, pl011_can_receive, pl011_receive,
> > + pl011_event, pl011_be_change, s, NULL, true);
> > +}
> > +
> > +static int pl011_be_change(void *opaque)
> > +{
> > + PL011State *s = opaque;
> > +
> > + pl011_set_handlers(s);
> > + pl011_set_break(s, s->lcr);
>
> and here we would then pass in s->lcr & LCR_BRK.
+
>
> > +
> > + return 0;
> > +}
>
> thanks
> -- PMM
