Re: [Qemu-devel] [PATCH] virtserialport/virtconsole: fix messy opening/closing port

[Paolo Bonzini]

On 01/11/18 16:11, Artem Pisarenko wrote:
> This fixes wrong interfacing between virtio serial port and bus
> models, and corresponding chardev backends, caused extra and incorrect
> activity during guest boot process (when virtserialport device used).
> 
> Signed-off-by: Artem Pisarenko <address@hidden>
> ---
> 
> Notes:
>     Although this doesn't trigger any issue/bug (known to me), this may 
> prevent them in future.
>     Also this patch optimizes emulation performance by avoiding extra 
> activity and fixes a case, when serial port wasn't closed if backend closed 
> while being disabled (even worse - serial port will open again!).

Marc-André, can you review this?

Thanks,

Paolo

>  hw/char/virtio-console.c          | 24 ++++++++++++++++++++----
>  hw/char/virtio-serial-bus.c       |  8 +++++++-
>  include/hw/virtio/virtio-serial.h |  8 ++++++++
>  3 files changed, 35 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/char/virtio-console.c b/hw/char/virtio-console.c
> index 2cbe1d4..634e9bf 100644
> --- a/hw/char/virtio-console.c
> +++ b/hw/char/virtio-console.c
> @@ -25,6 +25,7 @@
>  typedef struct VirtConsole {
>      VirtIOSerialPort parent_obj;
>  
> +    bool backend_active;
>      CharBackend chr;
>      guint watch;
>  } VirtConsole;
> @@ -149,6 +150,11 @@ static void chr_event(void *opaque, int event)
>      VirtIOSerialPort *port = VIRTIO_SERIAL_PORT(vcon);
>  
>      trace_virtio_console_chr_event(port->id, event);
> +
> +    if (!vcon->backend_active) {
> +        return;
> +    }
> +
>      switch (event) {
>      case CHR_EVENT_OPENED:
>          virtio_serial_open(port);
> @@ -187,17 +193,24 @@ static int chr_be_change(void *opaque)
>      return 0;
>  }
>  
> +static bool virtconsole_is_backend_enabled(VirtIOSerialPort *port)
> +{
> +    VirtConsole *vcon = VIRTIO_CONSOLE(port);
> +
> +    return vcon->backend_active;
> +}
> +
>  static void virtconsole_enable_backend(VirtIOSerialPort *port, bool enable)
>  {
>      VirtConsole *vcon = VIRTIO_CONSOLE(port);
>  
> -    if (!qemu_chr_fe_backend_connected(&vcon->chr)) {
> -        return;
> -    }
> -
>      if (enable) {
>          VirtIOSerialPortClass *k = VIRTIO_SERIAL_PORT_GET_CLASS(port);
>  
> +        if (!k->is_console && virtio_serial_is_opened(port)
> +            && !qemu_chr_fe_backend_open(&vcon->chr)) {
> +            virtio_serial_close(port);
> +        }
>          qemu_chr_fe_set_handlers(&vcon->chr, chr_can_read, chr_read,
>                                   k->is_console ? NULL : chr_event,
>                                   chr_be_change, vcon, NULL, false);
> @@ -205,6 +218,7 @@ static void virtconsole_enable_backend(VirtIOSerialPort 
> *port, bool enable)
>          qemu_chr_fe_set_handlers(&vcon->chr, NULL, NULL, NULL,
>                                   NULL, NULL, NULL, false);
>      }
> +    vcon->backend_active = enable;
>  }
>  
>  static void virtconsole_realize(DeviceState *dev, Error **errp)
> @@ -220,6 +234,7 @@ static void virtconsole_realize(DeviceState *dev, Error 
> **errp)
>      }
>  
>      if (qemu_chr_fe_backend_connected(&vcon->chr)) {
> +        vcon->backend_active = true;
>          /*
>           * For consoles we don't block guest data transfer just
>           * because nothing is connected - we'll just let it go
> @@ -278,6 +293,7 @@ static void virtserialport_class_init(ObjectClass *klass, 
> void *data)
>      k->unrealize = virtconsole_unrealize;
>      k->have_data = flush_buf;
>      k->set_guest_connected = set_guest_connected;
> +    k->is_backend_enabled = virtconsole_is_backend_enabled;
>      k->enable_backend = virtconsole_enable_backend;
>      k->guest_writable = guest_writable;
>      dc->props = virtserialport_properties;
> diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
> index 04e3ebe..d23d99d 100644
> --- a/hw/char/virtio-serial-bus.c
> +++ b/hw/char/virtio-serial-bus.c
> @@ -258,6 +258,11 @@ static size_t send_control_event(VirtIOSerial *vser, 
> uint32_t port_id,
>  }
>  
>  /* Functions for use inside qemu to open and read from/write to ports */
> +bool virtio_serial_is_opened(VirtIOSerialPort *port)
> +{
> +    return port->host_connected;
> +}
> +
>  int virtio_serial_open(VirtIOSerialPort *port)
>  {
>      /* Don't allow opening an already-open port */
> @@ -643,7 +648,8 @@ static void set_status(VirtIODevice *vdev, uint8_t status)
>  
>      QTAILQ_FOREACH(port, &vser->ports, next) {
>          VirtIOSerialPortClass *vsc = VIRTIO_SERIAL_PORT_GET_CLASS(port);
> -        if (vsc->enable_backend) {
> +        if (vsc->is_backend_enabled && vsc->enable_backend
> +            && (vsc->is_backend_enabled(port) != vdev->vm_running)) {
>              vsc->enable_backend(port, vdev->vm_running);
>          }
>      }
> diff --git a/include/hw/virtio/virtio-serial.h 
> b/include/hw/virtio/virtio-serial.h
> index 12657a9..4b72562 100644
> --- a/include/hw/virtio/virtio-serial.h
> +++ b/include/hw/virtio/virtio-serial.h
> @@ -58,6 +58,8 @@ typedef struct VirtIOSerialPortClass {
>          /* Guest opened/closed device. */
>      void (*set_guest_connected)(VirtIOSerialPort *port, int guest_connected);
>  
> +    /* Is backend currently enabled for virtio serial port */
> +    bool (*is_backend_enabled)(VirtIOSerialPort *port);
>      /* Enable/disable backend for virtio serial port */
>      void (*enable_backend)(VirtIOSerialPort *port, bool enable);
>  
> @@ -194,6 +196,12 @@ struct VirtIOSerial {
>  /* Interface to the virtio-serial bus */
>  
>  /*
> + * Checks status of connection to the port
> + *   Returns true if connected, false otherwise.
> + */
> +bool virtio_serial_is_opened(VirtIOSerialPort *port);
> +
> +/*
>   * Open a connection to the port
>   *   Returns 0 on success (always).
>   */
>