[PATCH v2 0/6] virtio-console: notify about the terminal size

[PATCH v2 0/6] virtio-console: notify about the terminal size

[Szymon Lukasz]

The goal of this series is to have a resizable terminal into a guest
without having to set up networking and using, e.g. ssh.

The virtio spec allows a virtio-console device to notify the guest about
terminal resizes in the host. Linux Kernel implements the driver part of
the spec. This series implement the device part in QEMU.

In this series resize notifications are only supported for the stdio
backend but I think it should be easy to add support for the vc backend.
Support for tty/serial backends is complicated by the fact that there is
no clean way to detect resizes of the underlying terminal.

Also there is a problem with the virtio spec and Linux Kernel
implementation, the order of fields in virtio_console_resize struct
differs between the kernel and the spec. I do not know if there is any
implementation of the virtio-console driver that handles resize messages
and uses a different order than Linux.


v2:
fix adding a new virtio feature bit to the virtio console device


Szymon Lukasz (6):
  main-loop: change the handling of SIGWINCH
  chardev: add support for retrieving the terminal size
  chardev: add support for notifying about terminal resizes
  char-stdio: add support for the terminal size
  virtio-serial-bus: add terminal resize messages
  virtio-console: notify the guest about terminal resizes

 backends/cryptodev-vhost-user.c   |  1 +
 chardev/char-fe.c                 | 11 ++++++
 chardev/char-mux.c                |  7 ++++
 chardev/char-stdio.c              | 35 +++++++++++++++++
 chardev/char.c                    |  1 +
 hw/block/vhost-user-blk.c         |  1 +
 hw/char/terminal3270.c            |  1 +
 hw/char/trace-events              |  1 +
 hw/char/virtio-console.c          | 65 +++++++++++++++++++++++++++++--
 hw/char/virtio-serial-bus.c       | 42 +++++++++++++++++++-
 hw/core/machine.c                 |  1 +
 hw/ipmi/ipmi_bmc_extern.c         |  1 +
 hw/usb/ccid-card-passthru.c       |  1 +
 hw/usb/dev-serial.c               |  1 +
 hw/usb/redirect.c                 |  1 +
 include/chardev/char-fe.h         | 11 ++++++
 include/chardev/char.h            |  2 +
 include/hw/virtio/virtio-serial.h |  5 +++
 include/qemu/main-loop.h          |  4 ++
 monitor/hmp.c                     |  1 +
 monitor/qmp.c                     |  1 +
 net/vhost-user.c                  |  1 +
 ui/curses.c                       | 11 +++---
 util/main-loop.c                  | 21 ++++++++++
 24 files changed, 216 insertions(+), 11 deletions(-)

-- 
2.27.0

[PATCH v2 1/6] main-loop: change the handling of SIGWINCH

[Szymon Lukasz]

Block SIGWINCH, so it is delivered only via signalfd.
Install a handler that uses NotifierList to tell
interested parties about SIGWINCH delivery.

Signed-off-by: Szymon Lukasz <noh4hss@gmail.com>
---
 include/qemu/main-loop.h |  4 ++++
 ui/curses.c              | 11 ++++++-----
 util/main-loop.c         | 21 +++++++++++++++++++++
 3 files changed, 31 insertions(+), 5 deletions(-)

diff --git a/include/qemu/main-loop.h b/include/qemu/main-loop.h
index a6d20b0719..f27dba1fd8 100644
--- a/include/qemu/main-loop.h
+++ b/include/qemu/main-loop.h
@@ -325,4 +325,8 @@ typedef struct MainLoopPoll {
 void main_loop_poll_add_notifier(Notifier *notify);
 void main_loop_poll_remove_notifier(Notifier *notify);
 
+#ifndef _WIN32
+void sigwinch_add_notifier(Notifier *n);
+#endif
+
 #endif
diff --git a/ui/curses.c b/ui/curses.c
index a59b23a9cf..e5895d506f 100644
--- a/ui/curses.c
+++ b/ui/curses.c
@@ -34,6 +34,7 @@
 #include <iconv.h>
 
 #include "qapi/error.h"
+#include "qemu/main-loop.h"
 #include "qemu/module.h"
 #include "ui/console.h"
 #include "ui/input.h"
@@ -146,7 +147,7 @@ static void curses_resize(DisplayChangeListener *dcl,
 }
 
 #if !defined(_WIN32) && defined(SIGWINCH) && defined(KEY_RESIZE)
-static volatile sig_atomic_t got_sigwinch;
+static bool got_sigwinch;
 static void curses_winch_check(void)
 {
     struct winsize {
@@ -169,17 +170,17 @@ static void curses_winch_check(void)
     invalidate = 1;
 }
 
-static void curses_winch_handler(int signum)
+static void curses_winch_handler(Notifier *n, void *data)
 {
     got_sigwinch = true;
 }
 
 static void curses_winch_init(void)
 {
-    struct sigaction old, winch = {
-        .sa_handler  = curses_winch_handler,
+    static Notifier n = {
+        .notify = curses_winch_handler
     };
-    sigaction(SIGWINCH, &winch, &old);
+    sigwinch_add_notifier(&n);
 }
 #else
 static void curses_winch_check(void) {}
diff --git a/util/main-loop.c b/util/main-loop.c
index eda63fe4e0..0f5c8f3af1 100644
--- a/util/main-loop.c
+++ b/util/main-loop.c
@@ -90,6 +90,7 @@ static int qemu_signal_init(Error **errp)
     sigaddset(&set, SIGIO);
     sigaddset(&set, SIGALRM);
     sigaddset(&set, SIGBUS);
+    sigaddset(&set, SIGWINCH);
     /* SIGINT cannot be handled via signalfd, so that ^C can be used
      * to interrupt QEMU when it is being run under gdb.  SIGHUP and
      * SIGTERM are also handled asynchronously, even though it is not
@@ -111,6 +112,26 @@ static int qemu_signal_init(Error **errp)
     return 0;
 }
 
+static NotifierList sigwinch_notifiers =
+    NOTIFIER_LIST_INITIALIZER(sigwinch_notifiers);
+
+static void sigwinch_handler(int signum)
+{
+    notifier_list_notify(&sigwinch_notifiers, NULL);
+}
+
+void sigwinch_add_notifier(Notifier *n)
+{
+    if (notifier_list_empty(&sigwinch_notifiers)) {
+        struct sigaction action = {
+            .sa_handler = sigwinch_handler,
+        };
+        sigaction(SIGWINCH, &action, NULL);
+    }
+
+    notifier_list_add(&sigwinch_notifiers, n);
+}
+
 #else /* _WIN32 */
 
 static int qemu_signal_init(Error **errp)
-- 
2.27.0

[PATCH v2 2/6] chardev: add support for retrieving the terminal size

[Szymon Lukasz]

Extend the class of chardevs with a new function - chr_get_winsize.
A chardev backend should implement if it is able to get the size of
the connected terminal and can detect changes in the terminal size,
i.e. if the backend cannot detect resizes it must not implement this
(e.g. if we have a tty backend connected to some (pseudo)terminal
there is no clean way to detect resizes since SIGWINCH is sent only
for the controlling terminal).

Signed-off-by: Szymon Lukasz <noh4hss@gmail.com>
---
 chardev/char-fe.c         | 11 +++++++++++
 chardev/char-mux.c        |  7 +++++++
 include/chardev/char-fe.h | 11 +++++++++++
 include/chardev/char.h    |  1 +
 4 files changed, 30 insertions(+)

diff --git a/chardev/char-fe.c b/chardev/char-fe.c
index f3530a90e6..802d3096cd 100644
--- a/chardev/char-fe.c
+++ b/chardev/char-fe.c
@@ -336,6 +336,17 @@ void qemu_chr_fe_set_echo(CharBackend *be, bool echo)
     }
 }
 
+int qemu_chr_fe_get_winsize(CharBackend *be, uint16_t *cols, uint16_t *rows)
+{
+    Chardev *chr = be->chr;
+
+    if (chr && CHARDEV_GET_CLASS(chr)->chr_get_winsize) {
+        return CHARDEV_GET_CLASS(chr)->chr_get_winsize(chr, cols, rows);
+    }
+
+    return -1;
+}
+
 void qemu_chr_fe_set_open(CharBackend *be, int fe_open)
 {
     Chardev *chr = be->chr;
diff --git a/chardev/char-mux.c b/chardev/char-mux.c
index 46c44af67c..368ce2334e 100644
--- a/chardev/char-mux.c
+++ b/chardev/char-mux.c
@@ -293,6 +293,12 @@ static void mux_chr_update_read_handlers(Chardev *chr)
                                   chr->gcontext, true, false);
 }
 
+static int mux_chr_get_winsize(Chardev *chr, uint16_t *cols, uint16_t *rows)
+{
+    MuxChardev *d = MUX_CHARDEV(chr);
+    return qemu_chr_fe_get_winsize(&d->chr, cols, rows);
+}
+
 void mux_set_focus(Chardev *chr, int focus)
 {
     MuxChardev *d = MUX_CHARDEV(chr);
@@ -385,6 +391,7 @@ static void char_mux_class_init(ObjectClass *oc, void *data)
     cc->chr_be_event = mux_chr_be_event;
     cc->chr_machine_done = open_muxes;
     cc->chr_update_read_handler = mux_chr_update_read_handlers;
+    cc->chr_get_winsize = mux_chr_get_winsize;
 }
 
 static const TypeInfo char_mux_type_info = {
diff --git a/include/chardev/char-fe.h b/include/chardev/char-fe.h
index a553843364..b7943df93a 100644
--- a/include/chardev/char-fe.h
+++ b/include/chardev/char-fe.h
@@ -154,6 +154,17 @@ int qemu_chr_fe_wait_connected(CharBackend *be, Error **errp);
  */
 void qemu_chr_fe_set_echo(CharBackend *be, bool echo);
 
+/**
+ * qemu_chr_fe_get_winsize:
+ * @cols: the address for storing columns
+ * @rows: the address for storing rows
+ *
+ * Get the terminal size of the backend.
+ *
+ * Returns: 0 on success and < 0 on error
+ */
+int qemu_chr_fe_get_winsize(CharBackend *be, uint16_t *cols, uint16_t *rows);
+
 /**
  * qemu_chr_fe_set_open:
  *
diff --git a/include/chardev/char.h b/include/chardev/char.h
index 00589a6025..fb20707917 100644
--- a/include/chardev/char.h
+++ b/include/chardev/char.h
@@ -276,6 +276,7 @@ typedef struct ChardevClass {
     void (*chr_be_event)(Chardev *s, QEMUChrEvent event);
     /* Return 0 if succeeded, 1 if failed */
     int (*chr_machine_done)(Chardev *chr);
+    int (*chr_get_winsize)(Chardev *chr, uint16_t *cols, uint16_t *rows);
 } ChardevClass;
 
 Chardev *qemu_chardev_new(const char *id, const char *typename,
-- 
2.27.0

[PATCH v2 3/6] chardev: add support for notifying about terminal resizes

[Szymon Lukasz]

Add a new chardev event, CHR_EVENT_RESIZE, which a backend should
trigger if detects the size of the connected terminal changed.

Signed-off-by: Szymon Lukasz <noh4hss@gmail.com>
---
 backends/cryptodev-vhost-user.c | 1 +
 chardev/char.c                  | 1 +
 hw/block/vhost-user-blk.c       | 1 +
 hw/char/terminal3270.c          | 1 +
 hw/char/virtio-console.c        | 1 +
 hw/ipmi/ipmi_bmc_extern.c       | 1 +
 hw/usb/ccid-card-passthru.c     | 1 +
 hw/usb/dev-serial.c             | 1 +
 hw/usb/redirect.c               | 1 +
 include/chardev/char.h          | 1 +
 monitor/hmp.c                   | 1 +
 monitor/qmp.c                   | 1 +
 net/vhost-user.c                | 1 +
 13 files changed, 13 insertions(+)

diff --git a/backends/cryptodev-vhost-user.c b/backends/cryptodev-vhost-user.c
index 8b8cbc4223..bbf8ad426a 100644
--- a/backends/cryptodev-vhost-user.c
+++ b/backends/cryptodev-vhost-user.c
@@ -174,6 +174,7 @@ static void cryptodev_vhost_user_event(void *opaque, QEMUChrEvent event)
     case CHR_EVENT_BREAK:
     case CHR_EVENT_MUX_IN:
     case CHR_EVENT_MUX_OUT:
+    case CHR_EVENT_RESIZE:
         /* Ignore */
         break;
     }
diff --git a/chardev/char.c b/chardev/char.c
index e3051295ac..904f8bf6e3 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -74,6 +74,7 @@ void qemu_chr_be_event(Chardev *s, QEMUChrEvent event)
     case CHR_EVENT_BREAK:
     case CHR_EVENT_MUX_IN:
     case CHR_EVENT_MUX_OUT:
+    case CHR_EVENT_RESIZE:
         /* Ignore */
         break;
     }
diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index a00b854736..1a656a27c3 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -403,6 +403,7 @@ static void vhost_user_blk_event(void *opaque, QEMUChrEvent event)
     case CHR_EVENT_BREAK:
     case CHR_EVENT_MUX_IN:
     case CHR_EVENT_MUX_OUT:
+    case CHR_EVENT_RESIZE:
         /* Ignore */
         break;
     }
diff --git a/hw/char/terminal3270.c b/hw/char/terminal3270.c
index 2c47ebf007..eadccbb617 100644
--- a/hw/char/terminal3270.c
+++ b/hw/char/terminal3270.c
@@ -169,6 +169,7 @@ static void chr_event(void *opaque, QEMUChrEvent event)
     case CHR_EVENT_BREAK:
     case CHR_EVENT_MUX_IN:
     case CHR_EVENT_MUX_OUT:
+    case CHR_EVENT_RESIZE:
         /* Ignore */
         break;
     }
diff --git a/hw/char/virtio-console.c b/hw/char/virtio-console.c
index 4f46753ea3..97b9240ef5 100644
--- a/hw/char/virtio-console.c
+++ b/hw/char/virtio-console.c
@@ -165,6 +165,7 @@ static void chr_event(void *opaque, QEMUChrEvent event)
     case CHR_EVENT_BREAK:
     case CHR_EVENT_MUX_IN:
     case CHR_EVENT_MUX_OUT:
+    case CHR_EVENT_RESIZE:
         /* Ignore */
         break;
     }
diff --git a/hw/ipmi/ipmi_bmc_extern.c b/hw/ipmi/ipmi_bmc_extern.c
index f9a13e0a44..9562584309 100644
--- a/hw/ipmi/ipmi_bmc_extern.c
+++ b/hw/ipmi/ipmi_bmc_extern.c
@@ -439,6 +439,7 @@ static void chr_event(void *opaque, QEMUChrEvent event)
     case CHR_EVENT_BREAK:
     case CHR_EVENT_MUX_IN:
     case CHR_EVENT_MUX_OUT:
+    case CHR_EVENT_RESIZE:
         /* Ignore */
         break;
     }
diff --git a/hw/usb/ccid-card-passthru.c b/hw/usb/ccid-card-passthru.c
index bb325dbc4a..3c26b16ed0 100644
--- a/hw/usb/ccid-card-passthru.c
+++ b/hw/usb/ccid-card-passthru.c
@@ -321,6 +321,7 @@ static void ccid_card_vscard_event(void *opaque, QEMUChrEvent event)
     case CHR_EVENT_MUX_IN:
     case CHR_EVENT_MUX_OUT:
     case CHR_EVENT_CLOSED:
+    case CHR_EVENT_RESIZE:
         /* Ignore */
         break;
     }
diff --git a/hw/usb/dev-serial.c b/hw/usb/dev-serial.c
index 7e50e3ba47..e8e960d0e6 100644
--- a/hw/usb/dev-serial.c
+++ b/hw/usb/dev-serial.c
@@ -507,6 +507,7 @@ static void usb_serial_event(void *opaque, QEMUChrEvent event)
             break;
         case CHR_EVENT_MUX_IN:
         case CHR_EVENT_MUX_OUT:
+        case CHR_EVENT_RESIZE:
             /* Ignore */
             break;
     }
diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
index 417a60a2e6..b716c4fdd7 100644
--- a/hw/usb/redirect.c
+++ b/hw/usb/redirect.c
@@ -1383,6 +1383,7 @@ static void usbredir_chardev_event(void *opaque, QEMUChrEvent event)
     case CHR_EVENT_BREAK:
     case CHR_EVENT_MUX_IN:
     case CHR_EVENT_MUX_OUT:
+    case CHR_EVENT_RESIZE:
         /* Ignore */
         break;
     }
diff --git a/include/chardev/char.h b/include/chardev/char.h
index fb20707917..c3d108ce82 100644
--- a/include/chardev/char.h
+++ b/include/chardev/char.h
@@ -22,6 +22,7 @@ typedef enum {
     CHR_EVENT_OPENED, /* new connection established */
     CHR_EVENT_MUX_IN, /* mux-focus was set to this terminal */
     CHR_EVENT_MUX_OUT, /* mux-focus will move on */
+    CHR_EVENT_RESIZE, /* the terminal size of the chardev changed */
     CHR_EVENT_CLOSED /* connection closed.  NOTE: currently this event
                       * is only bound to the read port of the chardev.
                       * Normally the read port and write port of a
diff --git a/monitor/hmp.c b/monitor/hmp.c
index d598dd02bb..020be03d61 100644
--- a/monitor/hmp.c
+++ b/monitor/hmp.c
@@ -1373,6 +1373,7 @@ static void monitor_event(void *opaque, QEMUChrEvent event)
         break;
 
     case CHR_EVENT_BREAK:
+    case CHR_EVENT_RESIZE:
         /* Ignored */
         break;
     }
diff --git a/monitor/qmp.c b/monitor/qmp.c
index d433ceae5b..58aecb475b 100644
--- a/monitor/qmp.c
+++ b/monitor/qmp.c
@@ -371,6 +371,7 @@ static void monitor_qmp_event(void *opaque, QEMUChrEvent event)
     case CHR_EVENT_BREAK:
     case CHR_EVENT_MUX_IN:
     case CHR_EVENT_MUX_OUT:
+    case CHR_EVENT_RESIZE:
         /* Ignore */
         break;
     }
diff --git a/net/vhost-user.c b/net/vhost-user.c
index 17532daaf3..e30cbe74bd 100644
--- a/net/vhost-user.c
+++ b/net/vhost-user.c
@@ -297,6 +297,7 @@ static void net_vhost_user_event(void *opaque, QEMUChrEvent event)
     case CHR_EVENT_BREAK:
     case CHR_EVENT_MUX_IN:
     case CHR_EVENT_MUX_OUT:
+    case CHR_EVENT_RESIZE:
         /* Ignore */
         break;
     }
-- 
2.27.0

[PATCH v2 4/6] char-stdio: add support for the terminal size

[Szymon Lukasz]

Implement chr_get_winsize for the stdio backend
and trigger CHR_EVENT_RESIZE upon SIGWINCH delivery.

Signed-off-by: Szymon Lukasz <noh4hss@gmail.com>
---
 chardev/char-stdio.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/chardev/char-stdio.c b/chardev/char-stdio.c
index 82eaebc1db..1edc82fc6e 100644
--- a/chardev/char-stdio.c
+++ b/chardev/char-stdio.c
@@ -34,7 +34,9 @@
 #include "chardev/char-win-stdio.h"
 #else
 #include <termios.h>
+#include <sys/ioctl.h>
 #include "chardev/char-fd.h"
+#include "qemu/main-loop.h"
 #endif
 
 #ifndef _WIN32
@@ -45,6 +47,13 @@ static bool stdio_in_use;
 static bool stdio_allow_signal;
 static bool stdio_echo_state;
 
+typedef struct {
+    FDChardev parent;
+    Notifier resize_notifier;
+} StdioChardev;
+
+#define STDIO_CHARDEV(obj) OBJECT_CHECK(StdioChardev, (obj), TYPE_CHARDEV_STDIO)
+
 static void term_exit(void)
 {
     if (stdio_in_use) {
@@ -82,11 +91,32 @@ static void term_stdio_handler(int sig)
     qemu_chr_set_echo_stdio(NULL, stdio_echo_state);
 }
 
+static int qemu_chr_get_winsize_stdio(Chardev *chr, uint16_t *cols,
+                                      uint16_t *rows)
+{
+    struct winsize ws;
+
+    if (ioctl(1, TIOCGWINSZ, &ws) < 0) {
+        return -1;
+    }
+
+    *cols = ws.ws_col;
+    *rows = ws.ws_row;
+    return 0;
+}
+
+static void term_resize_notify(Notifier *n, void *data)
+{
+    StdioChardev *s = container_of(n, StdioChardev, resize_notifier);
+    qemu_chr_be_event(CHARDEV(s), CHR_EVENT_RESIZE);
+}
+
 static void qemu_chr_open_stdio(Chardev *chr,
                                 ChardevBackend *backend,
                                 bool *be_opened,
                                 Error **errp)
 {
+    StdioChardev *s = STDIO_CHARDEV(chr);
     ChardevStdio *opts = backend->u.stdio.data;
     struct sigaction act;
 
@@ -116,6 +146,9 @@ static void qemu_chr_open_stdio(Chardev *chr,
         stdio_allow_signal = opts->signal;
     }
     qemu_chr_set_echo_stdio(chr, false);
+
+    s->resize_notifier.notify = term_resize_notify;
+    sigwinch_add_notifier(&s->resize_notifier);
 }
 #endif
 
@@ -139,6 +172,7 @@ static void char_stdio_class_init(ObjectClass *oc, void *data)
 #ifndef _WIN32
     cc->open = qemu_chr_open_stdio;
     cc->chr_set_echo = qemu_chr_set_echo_stdio;
+    cc->chr_get_winsize = qemu_chr_get_winsize_stdio;
 #endif
 }
 
@@ -155,6 +189,7 @@ static const TypeInfo char_stdio_type_info = {
     .parent = TYPE_CHARDEV_WIN_STDIO,
 #else
     .parent = TYPE_CHARDEV_FD,
+    .instance_size = sizeof(StdioChardev),
 #endif
     .instance_finalize = char_stdio_finalize,
     .class_init = char_stdio_class_init,
-- 
2.27.0

[PATCH v2 5/6] virtio-serial-bus: add terminal resize messages

[Szymon Lukasz]

Implement the part of the virtio spec that allows to notify the virtio
driver about terminal resizes. The virtio spec contains two methods to
achieve that:

For legacy drivers, we have only one port and we put the terminal size
in the config space and inject the config changed interrupt.

For multiport devices, we use the control virtqueue to send a packet
containing the terminal size. Note that the Linux kernel expects
the fields indicating the number of rows and columns in a packet to be
in a different order than the one specified in the current version of
the virtio spec. We follow the Linux implementation, so hopefully there
is no implementation of this functionality conforming to the spec.

Signed-off-by: Szymon Lukasz <noh4hss@gmail.com>
---
 hw/char/trace-events              |  1 +
 hw/char/virtio-serial-bus.c       | 42 +++++++++++++++++++++++++++++--
 hw/core/machine.c                 |  1 +
 include/hw/virtio/virtio-serial.h |  5 ++++
 4 files changed, 47 insertions(+), 2 deletions(-)

diff --git a/hw/char/trace-events b/hw/char/trace-events
index d20eafd56f..be40df47ea 100644
--- a/hw/char/trace-events
+++ b/hw/char/trace-events
@@ -10,6 +10,7 @@ serial_ioport_write(uint16_t addr, uint8_t value) "write addr 0x%02x val 0x%02x"
 
 # virtio-serial-bus.c
 virtio_serial_send_control_event(unsigned int port, uint16_t event, uint16_t value) "port %u, event %u, value %u"
+virtio_serial_send_console_resize(unsigned int port, uint16_t cols, uint16_t rows) "port %u, cols %u, rows %u"
 virtio_serial_throttle_port(unsigned int port, bool throttle) "port %u, throttle %d"
 virtio_serial_handle_control_message(uint16_t event, uint16_t value) "event %u, value %u"
 virtio_serial_handle_control_message_port(unsigned int port) "port %u"
diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
index 262089c0c9..6d9e94a64e 100644
--- a/hw/char/virtio-serial-bus.c
+++ b/hw/char/virtio-serial-bus.c
@@ -261,6 +261,42 @@ static size_t send_control_event(VirtIOSerial *vser, uint32_t port_id,
     return send_control_msg(vser, &cpkt, sizeof(cpkt));
 }
 
+/*
+ * This struct should be added to the Linux kernel uapi headers
+ * and later imported to standard-headers/linux/virtio_console.h
+ */
+struct virtio_console_resize {
+    __virtio16 rows;
+    __virtio16 cols;
+};
+
+void virtio_serial_send_console_resize(VirtIOSerialPort *port,
+                                       uint16_t cols, uint16_t rows)
+{
+    VirtIOSerial *vser = port->vser;
+    VirtIODevice *vdev = VIRTIO_DEVICE(vser);
+
+    if (virtio_vdev_has_feature(vdev, VIRTIO_CONSOLE_F_MULTIPORT)) {
+        struct {
+            struct virtio_console_control control;
+            struct virtio_console_resize resize;
+        } buffer;
+
+        virtio_stl_p(vdev, &buffer.control.id, port->id);
+        virtio_stw_p(vdev, &buffer.control.event, VIRTIO_CONSOLE_RESIZE);
+        virtio_stw_p(vdev, &buffer.resize.cols, cols);
+        virtio_stw_p(vdev, &buffer.resize.rows, rows);
+
+        trace_virtio_serial_send_console_resize(port->id, cols, rows);
+        send_control_msg(vser, &buffer, sizeof(buffer));
+
+    } else if (virtio_vdev_has_feature(vdev, VIRTIO_CONSOLE_F_SIZE)) {
+        vser->port0_cols = cols;
+        vser->port0_rows = rows;
+        virtio_notify_config(vdev);
+    }
+}
+
 /* Functions for use inside qemu to open and read from/write to ports */
 int virtio_serial_open(VirtIOSerialPort *port)
 {
@@ -572,8 +608,8 @@ static void get_config(VirtIODevice *vdev, uint8_t *config_data)
     struct virtio_console_config *config =
         (struct virtio_console_config *)config_data;
 
-    config->cols = 0;
-    config->rows = 0;
+    config->cols = virtio_tswap16(vdev, vser->port0_cols);
+    config->rows = virtio_tswap16(vdev, vser->port0_rows);
     config->max_nr_ports = virtio_tswap32(vdev,
                                           vser->serial.max_virtserial_ports);
 }
@@ -1168,6 +1204,8 @@ static Property virtio_serial_properties[] = {
                                                   31),
     DEFINE_PROP_BIT64("emergency-write", VirtIOSerial, host_features,
                       VIRTIO_CONSOLE_F_EMERG_WRITE, true),
+    DEFINE_PROP_BIT64("console-size", VirtIOSerial, host_features,
+                      VIRTIO_CONSOLE_F_SIZE, true),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/hw/core/machine.c b/hw/core/machine.c
index 1d80ab0e1d..c370c220f0 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -30,6 +30,7 @@
 
 GlobalProperty hw_compat_5_0[] = {
     { "virtio-balloon-device", "page-poison", "false" },
+    { "virtio-serial-device", "console-size", "off" },
 };
 const size_t hw_compat_5_0_len = G_N_ELEMENTS(hw_compat_5_0);
 
diff --git a/include/hw/virtio/virtio-serial.h b/include/hw/virtio/virtio-serial.h
index ed3e916b68..1d6436c0b1 100644
--- a/include/hw/virtio/virtio-serial.h
+++ b/include/hw/virtio/virtio-serial.h
@@ -188,6 +188,8 @@ struct VirtIOSerial {
     virtio_serial_conf serial;
 
     uint64_t host_features;
+
+    uint16_t port0_cols, port0_rows;
 };
 
 /* Interface to the virtio-serial bus */
@@ -222,6 +224,9 @@ size_t virtio_serial_guest_ready(VirtIOSerialPort *port);
  */
 void virtio_serial_throttle_port(VirtIOSerialPort *port, bool throttle);
 
+void virtio_serial_send_console_resize(VirtIOSerialPort *port,
+                                       uint16_t cols, uint16_t rows);
+
 #define TYPE_VIRTIO_SERIAL "virtio-serial-device"
 #define VIRTIO_SERIAL(obj) \
         OBJECT_CHECK(VirtIOSerial, (obj), TYPE_VIRTIO_SERIAL)
-- 
2.27.0

Re: [PATCH v2 5/6] virtio-serial-bus: add terminal resize messages

[Michael S. Tsirkin]

On Wed, Jun 24, 2020 at 01:26:39PM +0200, Szymon Lukasz wrote:
> Implement the part of the virtio spec that allows to notify the virtio
> driver about terminal resizes. The virtio spec contains two methods to
> achieve that:
> 
> For legacy drivers, we have only one port and we put the terminal size
> in the config space and inject the config changed interrupt.
> 
> For multiport devices, we use the control virtqueue to send a packet
> containing the terminal size. Note that the Linux kernel expects
> the fields indicating the number of rows and columns in a packet to be
> in a different order than the one specified in the current version of
> the virtio spec. We follow the Linux implementation, so hopefully there
> is no implementation of this functionality conforming to the spec.
> 
> Signed-off-by: Szymon Lukasz <noh4hss@gmail.com>

Looks right to me:


Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

> ---
>  hw/char/trace-events              |  1 +
>  hw/char/virtio-serial-bus.c       | 42 +++++++++++++++++++++++++++++--
>  hw/core/machine.c                 |  1 +
>  include/hw/virtio/virtio-serial.h |  5 ++++
>  4 files changed, 47 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/char/trace-events b/hw/char/trace-events
> index d20eafd56f..be40df47ea 100644
> --- a/hw/char/trace-events
> +++ b/hw/char/trace-events
> @@ -10,6 +10,7 @@ serial_ioport_write(uint16_t addr, uint8_t value) "write addr 0x%02x val 0x%02x"
>  
>  # virtio-serial-bus.c
>  virtio_serial_send_control_event(unsigned int port, uint16_t event, uint16_t value) "port %u, event %u, value %u"
> +virtio_serial_send_console_resize(unsigned int port, uint16_t cols, uint16_t rows) "port %u, cols %u, rows %u"
>  virtio_serial_throttle_port(unsigned int port, bool throttle) "port %u, throttle %d"
>  virtio_serial_handle_control_message(uint16_t event, uint16_t value) "event %u, value %u"
>  virtio_serial_handle_control_message_port(unsigned int port) "port %u"
> diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
> index 262089c0c9..6d9e94a64e 100644
> --- a/hw/char/virtio-serial-bus.c
> +++ b/hw/char/virtio-serial-bus.c
> @@ -261,6 +261,42 @@ static size_t send_control_event(VirtIOSerial *vser, uint32_t port_id,
>      return send_control_msg(vser, &cpkt, sizeof(cpkt));
>  }
>  
> +/*
> + * This struct should be added to the Linux kernel uapi headers
> + * and later imported to standard-headers/linux/virtio_console.h
> + */
> +struct virtio_console_resize {
> +    __virtio16 rows;
> +    __virtio16 cols;
> +};
> +
> +void virtio_serial_send_console_resize(VirtIOSerialPort *port,
> +                                       uint16_t cols, uint16_t rows)
> +{
> +    VirtIOSerial *vser = port->vser;
> +    VirtIODevice *vdev = VIRTIO_DEVICE(vser);
> +
> +    if (virtio_vdev_has_feature(vdev, VIRTIO_CONSOLE_F_MULTIPORT)) {
> +        struct {
> +            struct virtio_console_control control;
> +            struct virtio_console_resize resize;
> +        } buffer;
> +
> +        virtio_stl_p(vdev, &buffer.control.id, port->id);
> +        virtio_stw_p(vdev, &buffer.control.event, VIRTIO_CONSOLE_RESIZE);
> +        virtio_stw_p(vdev, &buffer.resize.cols, cols);
> +        virtio_stw_p(vdev, &buffer.resize.rows, rows);
> +
> +        trace_virtio_serial_send_console_resize(port->id, cols, rows);
> +        send_control_msg(vser, &buffer, sizeof(buffer));
> +
> +    } else if (virtio_vdev_has_feature(vdev, VIRTIO_CONSOLE_F_SIZE)) {
> +        vser->port0_cols = cols;
> +        vser->port0_rows = rows;
> +        virtio_notify_config(vdev);
> +    }
> +}
> +
>  /* Functions for use inside qemu to open and read from/write to ports */
>  int virtio_serial_open(VirtIOSerialPort *port)
>  {
> @@ -572,8 +608,8 @@ static void get_config(VirtIODevice *vdev, uint8_t *config_data)
>      struct virtio_console_config *config =
>          (struct virtio_console_config *)config_data;
>  
> -    config->cols = 0;
> -    config->rows = 0;
> +    config->cols = virtio_tswap16(vdev, vser->port0_cols);
> +    config->rows = virtio_tswap16(vdev, vser->port0_rows);
>      config->max_nr_ports = virtio_tswap32(vdev,
>                                            vser->serial.max_virtserial_ports);
>  }
> @@ -1168,6 +1204,8 @@ static Property virtio_serial_properties[] = {
>                                                    31),
>      DEFINE_PROP_BIT64("emergency-write", VirtIOSerial, host_features,
>                        VIRTIO_CONSOLE_F_EMERG_WRITE, true),
> +    DEFINE_PROP_BIT64("console-size", VirtIOSerial, host_features,
> +                      VIRTIO_CONSOLE_F_SIZE, true),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 1d80ab0e1d..c370c220f0 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -30,6 +30,7 @@
>  
>  GlobalProperty hw_compat_5_0[] = {
>      { "virtio-balloon-device", "page-poison", "false" },
> +    { "virtio-serial-device", "console-size", "off" },
>  };
>  const size_t hw_compat_5_0_len = G_N_ELEMENTS(hw_compat_5_0);
>  
> diff --git a/include/hw/virtio/virtio-serial.h b/include/hw/virtio/virtio-serial.h
> index ed3e916b68..1d6436c0b1 100644
> --- a/include/hw/virtio/virtio-serial.h
> +++ b/include/hw/virtio/virtio-serial.h
> @@ -188,6 +188,8 @@ struct VirtIOSerial {
>      virtio_serial_conf serial;
>  
>      uint64_t host_features;
> +
> +    uint16_t port0_cols, port0_rows;
>  };
>  
>  /* Interface to the virtio-serial bus */
> @@ -222,6 +224,9 @@ size_t virtio_serial_guest_ready(VirtIOSerialPort *port);
>   */
>  void virtio_serial_throttle_port(VirtIOSerialPort *port, bool throttle);
>  
> +void virtio_serial_send_console_resize(VirtIOSerialPort *port,
> +                                       uint16_t cols, uint16_t rows);
> +
>  #define TYPE_VIRTIO_SERIAL "virtio-serial-device"
>  #define VIRTIO_SERIAL(obj) \
>          OBJECT_CHECK(VirtIOSerial, (obj), TYPE_VIRTIO_SERIAL)
> -- 
> 2.27.0

[PATCH v2 6/6] virtio-console: notify the guest about terminal resizes

[Szymon Lukasz]

If a virtio serial port is a console port forward terminal resize
messages from the chardev backend to the guest.

Signed-off-by: Szymon Lukasz <noh4hss@gmail.com>
---
 hw/char/virtio-console.c | 64 +++++++++++++++++++++++++++++++++++++---
 1 file changed, 60 insertions(+), 4 deletions(-)

diff --git a/hw/char/virtio-console.c b/hw/char/virtio-console.c
index 97b9240ef5..1ea06aad08 100644
--- a/hw/char/virtio-console.c
+++ b/hw/char/virtio-console.c
@@ -29,6 +29,7 @@ typedef struct VirtConsole {
 
     CharBackend chr;
     guint watch;
+    uint16_t cols, rows;
 } VirtConsole;
 
 /*
@@ -104,6 +105,36 @@ static ssize_t flush_buf(VirtIOSerialPort *port,
     return ret;
 }
 
+static void virtconsole_send_resize(VirtIOSerialPort *port)
+{
+    uint16_t cols, rows;
+    VirtConsole *vcon = VIRTIO_CONSOLE(port);
+
+    /*
+     * We probably shouldn't send these messages before
+     * we told the guest it is a console port (which we do
+     * by sending VIRTIO_CONSOLE_CONSOLE_PORT message).
+     * Instead of adding a new field to the device state
+     * lets just use the guest_connected field for that purpose
+     * since the guest should not care about the terminal size
+     * before opening the port.
+     */
+    if (!port->guest_connected) {
+        return;
+    }
+
+    if (qemu_chr_fe_get_winsize(&vcon->chr, &cols, &rows) < 0) {
+        cols = 0;
+        rows = 0;
+    }
+
+    if (cols != vcon->cols || rows != vcon->rows) {
+        vcon->cols = cols;
+        vcon->rows = rows;
+        virtio_serial_send_console_resize(port, cols, rows);
+    }
+}
+
 /* Callback function that's called when the guest opens/closes the port */
 static void set_guest_connected(VirtIOSerialPort *port, int guest_connected)
 {
@@ -111,7 +142,9 @@ static void set_guest_connected(VirtIOSerialPort *port, int guest_connected)
     DeviceState *dev = DEVICE(port);
     VirtIOSerialPortClass *k = VIRTIO_SERIAL_PORT_GET_CLASS(port);
 
-    if (!k->is_console) {
+    if (k->is_console) {
+        virtconsole_send_resize(port);
+    } else {
         qemu_chr_fe_set_open(&vcon->chr, guest_connected);
     }
 
@@ -171,6 +204,22 @@ static void chr_event(void *opaque, QEMUChrEvent event)
     }
 }
 
+static void chr_event_console(void *opaque, QEMUChrEvent event)
+{
+    VirtConsole *vcon = opaque;
+    VirtIOSerialPort *port = VIRTIO_SERIAL_PORT(vcon);
+
+    switch (event) {
+    case CHR_EVENT_OPENED:
+    case CHR_EVENT_RESIZE:
+        trace_virtio_console_chr_event(port->id, event);
+        virtconsole_send_resize(port);
+        break;
+    default:
+        break;
+    }
+}
+
 static int chr_be_change(void *opaque)
 {
     VirtConsole *vcon = opaque;
@@ -179,7 +228,9 @@ static int chr_be_change(void *opaque)
 
     if (k->is_console) {
         qemu_chr_fe_set_handlers(&vcon->chr, chr_can_read, chr_read,
-                                 NULL, chr_be_change, vcon, NULL, true);
+                                 chr_event_console, chr_be_change,
+                                 vcon, NULL, true);
+        virtconsole_send_resize(port);
     } else {
         qemu_chr_fe_set_handlers(&vcon->chr, chr_can_read, chr_read,
                                  chr_event, chr_be_change, vcon, NULL, false);
@@ -207,7 +258,7 @@ static void virtconsole_enable_backend(VirtIOSerialPort *port, bool enable)
         VirtIOSerialPortClass *k = VIRTIO_SERIAL_PORT_GET_CLASS(port);
 
         qemu_chr_fe_set_handlers(&vcon->chr, chr_can_read, chr_read,
-                                 k->is_console ? NULL : chr_event,
+                                 k->is_console ? chr_event_console : chr_event,
                                  chr_be_change, vcon, NULL, false);
     } else {
         qemu_chr_fe_set_handlers(&vcon->chr, NULL, NULL, NULL,
@@ -227,6 +278,11 @@ static void virtconsole_realize(DeviceState *dev, Error **errp)
         return;
     }
 
+    if (k->is_console) {
+        vcon->cols = (uint16_t) -1;
+        vcon->rows = (uint16_t) -1;
+    }
+
     if (qemu_chr_fe_backend_connected(&vcon->chr)) {
         /*
          * For consoles we don't block guest data transfer just
@@ -239,7 +295,7 @@ static void virtconsole_realize(DeviceState *dev, Error **errp)
          */
         if (k->is_console) {
             qemu_chr_fe_set_handlers(&vcon->chr, chr_can_read, chr_read,
-                                     NULL, chr_be_change,
+                                     chr_event_console, chr_be_change,
                                      vcon, NULL, true);
             virtio_serial_open(port);
         } else {
-- 
2.27.0

Re: [PATCH v2 0/6] virtio-console: notify about the terminal size

[Daniel P. Berrangé]

On Wed, Jun 24, 2020 at 01:26:34PM +0200, Szymon Lukasz wrote:
> Also there is a problem with the virtio spec and Linux Kernel
> implementation, the order of fields in virtio_console_resize struct
> differs between the kernel and the spec. I do not know if there is any
> implementation of the virtio-console driver that handles resize messages
> and uses a different order than Linux.

Well this is a bit of a mess :-(

The main virtio_console_config struct has cols, then rows.

The Linux impl of resizing appears to have arrived in 2010, and created
a new struct with rows, then cols.

commit 8345adbf96fc1bde7d9846aadbe5af9b2ae90882
Author: Amit Shah <amit.shah@redhat.com>
Date:   Thu May 6 02:05:09 2010 +0530

    virtio: console: Accept console size along with resize control message
    
    The VIRTIO_CONSOLE_RESIZE control message sent to us by the host now
    contains the new {rows, cols} values for the console. This ensures each
    console port gets its own size, and we don't depend on the config-space
    rows and cols values at all now.
    
    Signed-off-by: Amit Shah <amit.shah@redhat.com>
    CC: Christian Borntraeger <borntraeger@de.ibm.com>
    CC: linuxppc-dev@ozlabs.org
    CC: Kusanagi Kouichi <slash@ac.auone-net.jp>
    Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>


The virtio spec documenting this came 4 years later in 2014 and documented
the resize struct with cols, then rows, which differs from Linux impl,
but matches ordering of the main virtio_console_config:

commit 908cfaa782e950d6656d947599d7a6c9fb16cad1
Author: rusty <rusty@0c8fb4dd-22a2-4bb5-bc14-6c75a5f43652>
Date:   Wed Feb 12 03:15:57 2014 +0000

    Feedback #6: Applied
    
    As per minutes:
            https://lists.oasis-open.org/archives/virtio/201402/msg00121.html
    
    Signed-off-by: Rusty Russell <rusty@au1.ibm.com>
    
    git-svn-id: https://tools.oasis-open.org/version-control/svn/virtio@237 0c8fb4dd-22a2-4bb5-bc14-6c75a5f43652

I can understand why it is desirable for the resize struct to match
the order of the initial config struct.  I'm guessing it just wasn't
realized that the Linux impl was inverted for resize

The FreeBSD impl of virtio-console doesn't do resize:

  https://github.com/freebsd/freebsd/blob/master/sys/dev/virtio/console/virtio_console.c#L874

Not sure what other impls are going to be around, but I feel like
Linux is going to be the most commonly deployed by orders of magnitude.

So I'd say QEMU should match Linux, and the spec should be fixed.


Have you reported this bug to the virtio spec people directly yet ?

I don't see an issue open at

  https://github.com/oasis-tcs/virtio-spec/issues/

so I think one should be filed there

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v2 0/6] virtio-console: notify about the terminal size

[Szymon Lukasz]

On Wed, Jun 24, 2020 at 12:49:15PM +0100, Daniel P. Berrangé wrote:
> On Wed, Jun 24, 2020 at 01:26:34PM +0200, Szymon Lukasz wrote:
> > Also there is a problem with the virtio spec and Linux Kernel
> > implementation, the order of fields in virtio_console_resize struct
> > differs between the kernel and the spec. I do not know if there is any
> > implementation of the virtio-console driver that handles resize messages
> > and uses a different order than Linux.
> 
> Well this is a bit of a mess :-(
> 
> The main virtio_console_config struct has cols, then rows.
> 
> The Linux impl of resizing appears to have arrived in 2010, and created
> a new struct with rows, then cols.
> 
> commit 8345adbf96fc1bde7d9846aadbe5af9b2ae90882
> Author: Amit Shah <amit.shah@redhat.com>
> Date:   Thu May 6 02:05:09 2010 +0530
> 
>     virtio: console: Accept console size along with resize control message
>     
>     The VIRTIO_CONSOLE_RESIZE control message sent to us by the host now
>     contains the new {rows, cols} values for the console. This ensures each
>     console port gets its own size, and we don't depend on the config-space
>     rows and cols values at all now.
>     
>     Signed-off-by: Amit Shah <amit.shah@redhat.com>
>     CC: Christian Borntraeger <borntraeger@de.ibm.com>
>     CC: linuxppc-dev@ozlabs.org
>     CC: Kusanagi Kouichi <slash@ac.auone-net.jp>
>     Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> 
> 
> The virtio spec documenting this came 4 years later in 2014 and documented
> the resize struct with cols, then rows, which differs from Linux impl,
> but matches ordering of the main virtio_console_config:
> 
> commit 908cfaa782e950d6656d947599d7a6c9fb16cad1
> Author: rusty <rusty@0c8fb4dd-22a2-4bb5-bc14-6c75a5f43652>
> Date:   Wed Feb 12 03:15:57 2014 +0000
> 
>     Feedback #6: Applied
>     
>     As per minutes:
>             https://lists.oasis-open.org/archives/virtio/201402/msg00121.html
>     
>     Signed-off-by: Rusty Russell <rusty@au1.ibm.com>
>     
>     git-svn-id: https://tools.oasis-open.org/version-control/svn/virtio@237 0c8fb4dd-22a2-4bb5-bc14-6c75a5f43652
> 
> I can understand why it is desirable for the resize struct to match
> the order of the initial config struct.  I'm guessing it just wasn't
> realized that the Linux impl was inverted for resize
> 
> The FreeBSD impl of virtio-console doesn't do resize:
> 
>   https://github.com/freebsd/freebsd/blob/master/sys/dev/virtio/console/virtio_console.c#L874
> 
> Not sure what other impls are going to be around, but I feel like
> Linux is going to be the most commonly deployed by orders of magnitude.
> 
> So I'd say QEMU should match Linux, and the spec should be fixed.

I had the same thoughts. I will ask for a change in the spec.
> 
> 
> Have you reported this bug to the virtio spec people directly yet ?
> 
> I don't see an issue open at
> 
>   https://github.com/oasis-tcs/virtio-spec/issues/
> 
> so I think one should be filed there
> 
> Regards,
> Daniel
> -- 
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
> 

Re: [PATCH v2 0/6] virtio-console: notify about the terminal size

[Michael S. Tsirkin]

On Wed, Jun 24, 2020 at 12:49:15PM +0100, Daniel P. Berrangé wrote:
> On Wed, Jun 24, 2020 at 01:26:34PM +0200, Szymon Lukasz wrote:
> > Also there is a problem with the virtio spec and Linux Kernel
> > implementation, the order of fields in virtio_console_resize struct
> > differs between the kernel and the spec. I do not know if there is any
> > implementation of the virtio-console driver that handles resize messages
> > and uses a different order than Linux.
> 
> Well this is a bit of a mess :-(
> 
> The main virtio_console_config struct has cols, then rows.
> 
> The Linux impl of resizing appears to have arrived in 2010, and created
> a new struct with rows, then cols.
> 
> commit 8345adbf96fc1bde7d9846aadbe5af9b2ae90882
> Author: Amit Shah <amit.shah@redhat.com>
> Date:   Thu May 6 02:05:09 2010 +0530
> 
>     virtio: console: Accept console size along with resize control message
>     
>     The VIRTIO_CONSOLE_RESIZE control message sent to us by the host now
>     contains the new {rows, cols} values for the console. This ensures each
>     console port gets its own size, and we don't depend on the config-space
>     rows and cols values at all now.
>     
>     Signed-off-by: Amit Shah <amit.shah@redhat.com>
>     CC: Christian Borntraeger <borntraeger@de.ibm.com>
>     CC: linuxppc-dev@ozlabs.org
>     CC: Kusanagi Kouichi <slash@ac.auone-net.jp>
>     Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> 
> 
> The virtio spec documenting this came 4 years later in 2014 and documented
> the resize struct with cols, then rows, which differs from Linux impl,
> but matches ordering of the main virtio_console_config:
> 
> commit 908cfaa782e950d6656d947599d7a6c9fb16cad1
> Author: rusty <rusty@0c8fb4dd-22a2-4bb5-bc14-6c75a5f43652>
> Date:   Wed Feb 12 03:15:57 2014 +0000
> 
>     Feedback #6: Applied
>     
>     As per minutes:
>             https://lists.oasis-open.org/archives/virtio/201402/msg00121.html
>     
>     Signed-off-by: Rusty Russell <rusty@au1.ibm.com>
>     
>     git-svn-id: https://tools.oasis-open.org/version-control/svn/virtio@237 0c8fb4dd-22a2-4bb5-bc14-6c75a5f43652
> 
> I can understand why it is desirable for the resize struct to match
> the order of the initial config struct.  I'm guessing it just wasn't
> realized that the Linux impl was inverted for resize
> 
> The FreeBSD impl of virtio-console doesn't do resize:
> 
>   https://github.com/freebsd/freebsd/blob/master/sys/dev/virtio/console/virtio_console.c#L874
> 
> Not sure what other impls are going to be around, but I feel like
> Linux is going to be the most commonly deployed by orders of magnitude.
> 
> So I'd say QEMU should match Linux, and the spec should be fixed.
> 
> 
> Have you reported this bug to the virtio spec people directly yet ?
> 
> I don't see an issue open at
> 
>   https://github.com/oasis-tcs/virtio-spec/issues/
> 
> so I think one should be filed there
> 
> Regards,
> Daniel


One reports defects on the virtio-comments mailing list, issue tracker is just for
tracking spec changes.

> -- 
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v2 0/6] virtio-console: notify about the terminal size

[Daniel P. Berrangé]

On Thu, Jun 25, 2020 at 09:18:51AM -0400, Michael S. Tsirkin wrote:
> On Wed, Jun 24, 2020 at 12:49:15PM +0100, Daniel P. Berrangé wrote:
> > On Wed, Jun 24, 2020 at 01:26:34PM +0200, Szymon Lukasz wrote:
> > > Also there is a problem with the virtio spec and Linux Kernel
> > > implementation, the order of fields in virtio_console_resize struct
> > > differs between the kernel and the spec. I do not know if there is any
> > > implementation of the virtio-console driver that handles resize messages
> > > and uses a different order than Linux.
> > 
> > Well this is a bit of a mess :-(
> > 
> > The main virtio_console_config struct has cols, then rows.
> > 
> > The Linux impl of resizing appears to have arrived in 2010, and created
> > a new struct with rows, then cols.
> > 
> > commit 8345adbf96fc1bde7d9846aadbe5af9b2ae90882
> > Author: Amit Shah <amit.shah@redhat.com>
> > Date:   Thu May 6 02:05:09 2010 +0530
> > 
> >     virtio: console: Accept console size along with resize control message
> >     
> >     The VIRTIO_CONSOLE_RESIZE control message sent to us by the host now
> >     contains the new {rows, cols} values for the console. This ensures each
> >     console port gets its own size, and we don't depend on the config-space
> >     rows and cols values at all now.
> >     
> >     Signed-off-by: Amit Shah <amit.shah@redhat.com>
> >     CC: Christian Borntraeger <borntraeger@de.ibm.com>
> >     CC: linuxppc-dev@ozlabs.org
> >     CC: Kusanagi Kouichi <slash@ac.auone-net.jp>
> >     Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> > 
> > 
> > The virtio spec documenting this came 4 years later in 2014 and documented
> > the resize struct with cols, then rows, which differs from Linux impl,
> > but matches ordering of the main virtio_console_config:
> > 
> > commit 908cfaa782e950d6656d947599d7a6c9fb16cad1
> > Author: rusty <rusty@0c8fb4dd-22a2-4bb5-bc14-6c75a5f43652>
> > Date:   Wed Feb 12 03:15:57 2014 +0000
> > 
> >     Feedback #6: Applied
> >     
> >     As per minutes:
> >             https://lists.oasis-open.org/archives/virtio/201402/msg00121.html
> >     
> >     Signed-off-by: Rusty Russell <rusty@au1.ibm.com>
> >     
> >     git-svn-id: https://tools.oasis-open.org/version-control/svn/virtio@237 0c8fb4dd-22a2-4bb5-bc14-6c75a5f43652
> > 
> > I can understand why it is desirable for the resize struct to match
> > the order of the initial config struct.  I'm guessing it just wasn't
> > realized that the Linux impl was inverted for resize
> > 
> > The FreeBSD impl of virtio-console doesn't do resize:
> > 
> >   https://github.com/freebsd/freebsd/blob/master/sys/dev/virtio/console/virtio_console.c#L874
> > 
> > Not sure what other impls are going to be around, but I feel like
> > Linux is going to be the most commonly deployed by orders of magnitude.
> > 
> > So I'd say QEMU should match Linux, and the spec should be fixed.
> > 
> > 
> > Have you reported this bug to the virtio spec people directly yet ?
> > 
> > I don't see an issue open at
> > 
> >   https://github.com/oasis-tcs/virtio-spec/issues/
> > 
> > so I think one should be filed there
> > 
> > Regards,
> > Daniel
> 
> 
> One reports defects on the virtio-comments mailing list, issue tracker is just for
> tracking spec changes.

NB That contradicts what the CONTRIBUTING.md file in virtio-spec says, which
welcomes use of the issue tracker:

   "Persons who are not TC members are invited to open issues and
    provide comments using this repository's GitHub Issues tracking
    facility or using the TC's comment list. "

https://github.com/oasis-tcs/virtio-spec/blob/master/CONTRIBUTING.md


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v2 0/6] virtio-console: notify about the terminal size

[Michael S. Tsirkin]

On Thu, Jun 25, 2020 at 02:23:28PM +0100, Daniel P. Berrangé wrote:
> On Thu, Jun 25, 2020 at 09:18:51AM -0400, Michael S. Tsirkin wrote:
> > On Wed, Jun 24, 2020 at 12:49:15PM +0100, Daniel P. Berrangé wrote:
> > > On Wed, Jun 24, 2020 at 01:26:34PM +0200, Szymon Lukasz wrote:
> > > > Also there is a problem with the virtio spec and Linux Kernel
> > > > implementation, the order of fields in virtio_console_resize struct
> > > > differs between the kernel and the spec. I do not know if there is any
> > > > implementation of the virtio-console driver that handles resize messages
> > > > and uses a different order than Linux.
> > > 
> > > Well this is a bit of a mess :-(
> > > 
> > > The main virtio_console_config struct has cols, then rows.
> > > 
> > > The Linux impl of resizing appears to have arrived in 2010, and created
> > > a new struct with rows, then cols.
> > > 
> > > commit 8345adbf96fc1bde7d9846aadbe5af9b2ae90882
> > > Author: Amit Shah <amit.shah@redhat.com>
> > > Date:   Thu May 6 02:05:09 2010 +0530
> > > 
> > >     virtio: console: Accept console size along with resize control message
> > >     
> > >     The VIRTIO_CONSOLE_RESIZE control message sent to us by the host now
> > >     contains the new {rows, cols} values for the console. This ensures each
> > >     console port gets its own size, and we don't depend on the config-space
> > >     rows and cols values at all now.
> > >     
> > >     Signed-off-by: Amit Shah <amit.shah@redhat.com>
> > >     CC: Christian Borntraeger <borntraeger@de.ibm.com>
> > >     CC: linuxppc-dev@ozlabs.org
> > >     CC: Kusanagi Kouichi <slash@ac.auone-net.jp>
> > >     Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> > > 
> > > 
> > > The virtio spec documenting this came 4 years later in 2014 and documented
> > > the resize struct with cols, then rows, which differs from Linux impl,
> > > but matches ordering of the main virtio_console_config:
> > > 
> > > commit 908cfaa782e950d6656d947599d7a6c9fb16cad1
> > > Author: rusty <rusty@0c8fb4dd-22a2-4bb5-bc14-6c75a5f43652>
> > > Date:   Wed Feb 12 03:15:57 2014 +0000
> > > 
> > >     Feedback #6: Applied
> > >     
> > >     As per minutes:
> > >             https://lists.oasis-open.org/archives/virtio/201402/msg00121.html
> > >     
> > >     Signed-off-by: Rusty Russell <rusty@au1.ibm.com>
> > >     
> > >     git-svn-id: https://tools.oasis-open.org/version-control/svn/virtio@237 0c8fb4dd-22a2-4bb5-bc14-6c75a5f43652
> > > 
> > > I can understand why it is desirable for the resize struct to match
> > > the order of the initial config struct.  I'm guessing it just wasn't
> > > realized that the Linux impl was inverted for resize
> > > 
> > > The FreeBSD impl of virtio-console doesn't do resize:
> > > 
> > >   https://github.com/freebsd/freebsd/blob/master/sys/dev/virtio/console/virtio_console.c#L874
> > > 
> > > Not sure what other impls are going to be around, but I feel like
> > > Linux is going to be the most commonly deployed by orders of magnitude.
> > > 
> > > So I'd say QEMU should match Linux, and the spec should be fixed.
> > > 
> > > 
> > > Have you reported this bug to the virtio spec people directly yet ?
> > > 
> > > I don't see an issue open at
> > > 
> > >   https://github.com/oasis-tcs/virtio-spec/issues/
> > > 
> > > so I think one should be filed there
> > > 
> > > Regards,
> > > Daniel
> > 
> > 
> > One reports defects on the virtio-comments mailing list, issue tracker is just for
> > tracking spec changes.
> 
> NB That contradicts what the CONTRIBUTING.md file in virtio-spec says, which
> welcomes use of the issue tracker:
> 
>    "Persons who are not TC members are invited to open issues and
>     provide comments using this repository's GitHub Issues tracking
>     facility or using the TC's comment list. "
> 
> https://github.com/oasis-tcs/virtio-spec/blob/master/CONTRIBUTING.md

I know, it's confusing.  See

https://github.com/oasis-tcs/virtio-spec#use-of-github-issues

OASIS admins asked us not to change CONTRIBUTING.md so we
had to resort to adding clarifications in a separate file like that.



> 
> Regards,
> Daniel
> -- 
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v2 0/6] virtio-console: notify about the terminal size

[Daniel P. Berrangé]

On Wed, Jun 24, 2020 at 01:26:34PM +0200, Szymon Lukasz wrote:
> In this series resize notifications are only supported for the stdio
> backend but I think it should be easy to add support for the vc backend.
> Support for tty/serial backends is complicated by the fact that there is
> no clean way to detect resizes of the underlying terminal.

In a libvirt managed scenario it is typical to have the virtio console
connected to a UNIX socket. It would be desirable to have a way to
deal with resizes there.

QEMU socket chardev (TCP & UNIX socket) supports a "telnet" protocol
addition. Currently it doesn't almost nothing useful, but in theory
we could wire up support for the telnet resize message:

   https://tools.ietf.org/html/rfc1073


Another option is to allow dealing with resizes out of band, via the
QMP monitor. ie we can introduce a qmp_chardev_winsize command that
a client app can use to trigger the resize message to the guest OS.
From libvirt's POV, this would be quite easy to support & useful to
have.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v2 0/6] virtio-console: notify about the terminal size

[Szymon Lukasz]

On Wed, Jun 24, 2020 at 12:56:15PM +0100, Daniel P. Berrangé wrote:
> On Wed, Jun 24, 2020 at 01:26:34PM +0200, Szymon Lukasz wrote:
> > In this series resize notifications are only supported for the stdio
> > backend but I think it should be easy to add support for the vc backend.
> > Support for tty/serial backends is complicated by the fact that there is
> > no clean way to detect resizes of the underlying terminal.
> 
> In a libvirt managed scenario it is typical to have the virtio console
> connected to a UNIX socket. It would be desirable to have a way to
> deal with resizes there.
> 
> QEMU socket chardev (TCP & UNIX socket) supports a "telnet" protocol
> addition. Currently it doesn't almost nothing useful, but in theory
> we could wire up support for the telnet resize message:
> 
>    https://tools.ietf.org/html/rfc1073
> 
> 
> Another option is to allow dealing with resizes out of band, via the
> QMP monitor. ie we can introduce a qmp_chardev_winsize command that
> a client app can use to trigger the resize message to the guest OS.
> From libvirt's POV, this would be quite easy to support & useful to
> have.

I will look into that.
> 
> 
> Regards,
> Daniel
> -- 
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
>