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

Artem Pisarenko posted 1 patch 5 years, 5 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/49e9640f3fc040a9c9ab40f93935a6572b34bdff.1541085026.git.artem.k.pisarenko@gmail.com
Test docker-clang@ubuntu passed
Test checkpatch passed
Test asan passed
Test docker-mingw@fedora passed
Test docker-quick@centos7 passed
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(-)
[Qemu-devel] [PATCH] virtserialport/virtconsole: fix messy opening/closing port
Posted by Artem Pisarenko 5 years, 5 months ago
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(-)

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


Re: [Qemu-devel] [PATCH] virtserialport/virtconsole: fix messy opening/closing port
Posted by Paolo Bonzini 5 years, 5 months ago
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).
>   */
>