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

["Alex Bennée" <alex.bennee@linaro.org>]

Alexander Mikhalitsyn <alexander@mihalicyn.com> writes:

> From: Aleksanr Mikhalitsyn <alexander@mihalicyn.com>
>
> 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 <stgraber@stgraber.org>
> Signed-off-by: Alexander Mikhalitsyn
> <aleksandr.mikhalitsyn@futurfusion.io>

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.

> ---
>  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?

> +
> +    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:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

>  
>  static void pl011_reset(DeviceState *dev)

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro