diff mbox series

virtserialport/virtconsole: fix messy opening/closing port

Message ID 49e9640f3fc040a9c9ab40f93935a6572b34bdff.1541085026.git.artem.k.pisarenko@gmail.com
State New
Headers show
Series virtserialport/virtconsole: fix messy opening/closing port | expand

Commit Message

Artem Pisarenko Nov. 1, 2018, 3:11 p.m. UTC
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 <artem.k.pisarenko@gmail.com>
---

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!).

 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(-)

Comments

Paolo Bonzini Nov. 23, 2018, 5:18 p.m. UTC | #1
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 <artem.k.pisarenko@gmail.com>
> ---
> 
> 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).
>   */
>
diff mbox series

Patch

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).
  */