[PATCH 0/2] chardev: implement backend chardev multiplexing

[PATCH 0/2] chardev: implement backend chardev multiplexing

[Roman Penyaev]

Mux is a character backend (host side) device, which multiplexes
multiple frontends with one backend device. The following is a
few lines from the QEMU manpage [1]:

  A multiplexer is a "1:N" device, and here the "1" end is your
  specified chardev backend, and the "N" end is the various parts
  of QEMU that can talk to a chardev.

But sadly multiple backends are not supported.

This work implements multiplexing capability of several backend
devices, which opens up an opportunity to use a single frontend
device on the guest, which can be manipulated from several
backend devices.

The main motivation of this work is to use a virtio console
frontend device on the guest, which can be manipulated from
several backend devices. The following is QEMU command line
example:

  -chardev socket,path=/tmp/sock,server=on,wait=off,id=sock0 \
  -chardev vc,id=vc0 \
  -chardev mux,id=mux0,chardev=vc0,,sock0 \
  -device virtconsole,chardev=mux0 \
  -vnc 0.0.0.0:0

Which creates 2 backend devices: text virtual console (`vc0`)
and a socket (`sock0`) connected to the single virtio hvc
console with the multiplexer (`mux0`) help. `vc0` renders
text to an image, which can be shared over the VNC protocol.
`sock0` is a socket backend which provides biderectional
communication to the virtio hvc console.

Once QEMU starts VNC client and any TTY emulator can be used to
control a single hvc console, for example these two different
consoles should have similar input and output due the buffer
multiplexing:

   # VNC client
   vncviewer :0

   # TTY emulator
   socat unix:connect:/tmp/sock pty,link=/tmp/pty
   tio /tmp/pty

[1] https://www.qemu.org/docs/master/system/qemu-manpage.html#hxtool-6

Roman Penyaev (2):
  chardev: implement backend chardev multiplexing
  qemu-options.hx: describe multiplexing of several backend devices

 chardev/char-fe.c          |  14 +++--
 chardev/char-mux.c         | 120 +++++++++++++++++++++++++++++--------
 chardev/char.c             |   2 +-
 chardev/chardev-internal.h |   7 ++-
 qemu-options.hx            |  44 ++++++++++++--
 5 files changed, 151 insertions(+), 36 deletions(-)

Signed-off-by: Roman Penyaev <r.peniaev@gmail.com>
Cc: "Marc-André Lureau" <marcandre.lureau@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: qemu-devel@nongnu.org
-- 
2.34.1

[PATCH 1/2] chardev: implement backend chardev multiplexing

[Roman Penyaev]

This patch implements multiplexing capability of several backend
devices, which opens up an opportunity to use a single frontend
device on the guest, which can be manipulated from several
backend devices.

The idea of the change is trivial: keep list of backend devices
(up to 4), init them on demand and forward data buffer back and
forth.

The following is QEMU command line example:

  -chardev socket,path=/tmp/sock,server=on,wait=off,id=sock0 \
  -chardev vc,id=vc0 \
  -chardev mux,id=mux0,chardev=vc0,,sock0 \
  -device virtconsole,chardev=mux0 \
  -vnc 0.0.0.0:0

Which creates 2 backend devices: text virtual console (`vc0`)
and a socket (`sock0`) connected to the single virtio hvc
console with the multiplexer (`mux0`) help. `vc0` renders
text to an image, which can be shared over the VNC protocol.
`sock0` is a socket backend which provides biderectional
communication to the virtio hvc console.

Signed-off-by: Roman Penyaev <r.peniaev@gmail.com>
Cc: "Marc-André Lureau" <marcandre.lureau@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: qemu-devel@nongnu.org
---
 chardev/char-fe.c          |  14 +++--
 chardev/char-mux.c         | 120 +++++++++++++++++++++++++++++--------
 chardev/char.c             |   2 +-
 chardev/chardev-internal.h |   7 ++-
 4 files changed, 111 insertions(+), 32 deletions(-)

diff --git a/chardev/char-fe.c b/chardev/char-fe.c
index b214ba3802b1..d1f67338084d 100644
--- a/chardev/char-fe.c
+++ b/chardev/char-fe.c
@@ -197,16 +197,22 @@ bool qemu_chr_fe_init(CharBackend *b, Chardev *s, Error **errp)
         if (CHARDEV_IS_MUX(s)) {
             MuxChardev *d = MUX_CHARDEV(s);
 
-            if (d->mux_cnt >= MAX_MUX) {
+            if (d->fe_cnt >= MAX_MUX) {
                 error_setg(errp,
                            "too many uses of multiplexed chardev '%s'"
                            " (maximum is " stringify(MAX_MUX) ")",
                            s->label);
                 return false;
             }
-
-            d->backends[d->mux_cnt] = b;
-            tag = d->mux_cnt++;
+            if (d->fe_cnt > 0 && d->be_cnt > 1) {
+                error_setg(errp,
+                           "multiplexed chardev '%s' is already used "
+                           "for backend multiplexing",
+                           s->label);
+                return false;
+            }
+            d->backends[d->fe_cnt] = b;
+            tag = d->fe_cnt++;
         } else if (s->be) {
             error_setg(errp, "chardev '%s' is already in use", s->label);
             return false;
diff --git a/chardev/char-mux.c b/chardev/char-mux.c
index ee2d47b20d9b..82f728b5caf8 100644
--- a/chardev/char-mux.c
+++ b/chardev/char-mux.c
@@ -26,6 +26,7 @@
 #include "qapi/error.h"
 #include "qemu/module.h"
 #include "qemu/option.h"
+#include "qemu/cutils.h"
 #include "chardev/char.h"
 #include "sysemu/block-backend.h"
 #include "qapi/qapi-commands-control.h"
@@ -40,13 +41,39 @@
  */
 static bool muxes_opened = true;
 
+/* Write to all backends  */
+static int mux_chr_fe_write(MuxChardev *mux, const uint8_t *buf, int len)
+{
+        int r, ret = -1, i;
+
+        for (i = 0; i < mux->be_cnt; i++) {
+                r = qemu_chr_fe_write(&mux->chrs[i], buf, len);
+                ret = ret < 0 ? r : MAX(r, ret);
+        }
+
+        return ret;
+}
+
+/* Write to all backends  */
+static int mux_chr_fe_write_all(MuxChardev *mux, const uint8_t *buf, int len)
+{
+        int r, ret = -1, i;
+
+        for (i = 0; i < mux->be_cnt; i++) {
+                r = qemu_chr_fe_write_all(&mux->chrs[i], buf, len);
+                ret = ret < 0 ? r : MAX(r, ret);
+        }
+
+        return ret;
+}
+
 /* Called with chr_write_lock held.  */
 static int mux_chr_write(Chardev *chr, const uint8_t *buf, int len)
 {
     MuxChardev *d = MUX_CHARDEV(chr);
     int ret;
     if (!d->timestamps) {
-        ret = qemu_chr_fe_write(&d->chr, buf, len);
+        ret = mux_chr_fe_write(d, buf, len);
     } else {
         int i;
 
@@ -71,11 +98,11 @@ static int mux_chr_write(Chardev *chr, const uint8_t *buf, int len)
                          (int)(ti % 1000));
                 /* XXX this blocks entire thread. Rewrite to use
                  * qemu_chr_fe_write and background I/O callbacks */
-                qemu_chr_fe_write_all(&d->chr,
+                mux_chr_fe_write_all(d,
                                       (uint8_t *)buf1, strlen(buf1));
                 d->linestart = 0;
             }
-            ret += qemu_chr_fe_write(&d->chr, buf + i, 1);
+            ret += mux_chr_fe_write(d, buf + i, 1);
             if (buf[i] == '\n') {
                 d->linestart = 1;
             }
@@ -168,9 +195,9 @@ static int mux_proc_byte(Chardev *chr, MuxChardev *d, int ch)
             qemu_chr_be_event(chr, CHR_EVENT_BREAK);
             break;
         case 'c':
-            assert(d->mux_cnt > 0); /* handler registered with first fe */
+            assert(d->fe_cnt > 0); /* handler registered with first fe */
             /* Switch to the next registered device */
-            mux_set_focus(chr, (d->focus + 1) % d->mux_cnt);
+            mux_set_focus(chr, (d->focus + 1) % d->fe_cnt);
             break;
         case 't':
             d->timestamps = !d->timestamps;
@@ -248,8 +275,8 @@ void mux_chr_send_all_event(Chardev *chr, QEMUChrEvent event)
         return;
     }
 
-    /* Send the event to all registered listeners */
-    for (i = 0; i < d->mux_cnt; i++) {
+    /* Send the event to all registered frontend listeners */
+    for (i = 0; i < d->fe_cnt; i++) {
         mux_chr_send_event(d, i, event);
     }
 }
@@ -262,8 +289,16 @@ static void mux_chr_event(void *opaque, QEMUChrEvent event)
 static GSource *mux_chr_add_watch(Chardev *s, GIOCondition cond)
 {
     MuxChardev *d = MUX_CHARDEV(s);
-    Chardev *chr = qemu_chr_fe_get_driver(&d->chr);
-    ChardevClass *cc = CHARDEV_GET_CLASS(chr);
+    Chardev *chr;
+    ChardevClass *cc;
+
+    if (d->be_cnt > 1) {
+            /* TODO: multiple backends have to be combined on a single watch */
+            return NULL;
+    }
+
+    chr = qemu_chr_fe_get_driver(&d->chrs[0]);
+    cc = CHARDEV_GET_CLASS(chr);
 
     if (!cc->chr_add_watch) {
         return NULL;
@@ -277,27 +312,32 @@ static void char_mux_finalize(Object *obj)
     MuxChardev *d = MUX_CHARDEV(obj);
     int i;
 
-    for (i = 0; i < d->mux_cnt; i++) {
+    for (i = 0; i < d->fe_cnt; i++) {
         CharBackend *be = d->backends[i];
         if (be) {
             be->chr = NULL;
         }
     }
-    qemu_chr_fe_deinit(&d->chr, false);
+    for (i = 0; i < d->be_cnt; i++) {
+        qemu_chr_fe_deinit(&d->chrs[i], false);
+    }
 }
 
 static void mux_chr_update_read_handlers(Chardev *chr)
 {
     MuxChardev *d = MUX_CHARDEV(chr);
+    int i;
 
-    /* Fix up the real driver with mux routines */
-    qemu_chr_fe_set_handlers_full(&d->chr,
-                                  mux_chr_can_read,
-                                  mux_chr_read,
-                                  mux_chr_event,
-                                  NULL,
-                                  chr,
-                                  chr->gcontext, true, false);
+    for (i = 0; i < d->be_cnt; i++) {
+        /* Fix up the real driver with mux routines */
+        qemu_chr_fe_set_handlers_full(&d->chrs[i],
+                                      mux_chr_can_read,
+                                      mux_chr_read,
+                                      mux_chr_event,
+                                      NULL,
+                                      chr,
+                                      chr->gcontext, true, false);
+    }
 }
 
 void mux_set_focus(Chardev *chr, int focus)
@@ -305,7 +345,7 @@ void mux_set_focus(Chardev *chr, int focus)
     MuxChardev *d = MUX_CHARDEV(chr);
 
     assert(focus >= 0);
-    assert(focus < d->mux_cnt);
+    assert(focus < d->fe_cnt);
 
     if (d->focus != -1) {
         mux_chr_send_event(d, d->focus, CHR_EVENT_MUX_OUT);
@@ -324,19 +364,49 @@ static void qemu_chr_open_mux(Chardev *chr,
     ChardevMux *mux = backend->u.mux.data;
     Chardev *drv;
     MuxChardev *d = MUX_CHARDEV(chr);
-
-    drv = qemu_chr_find(mux->chardev);
-    if (drv == NULL) {
-        error_setg(errp, "mux: base chardev %s not found", mux->chardev);
+    const char *offset, *chardevs;
+    int length, i;
+
+    if (d->fe_cnt > 1) {
+        error_setg(errp,
+                   "multiplexed chardev '%s' is already used "
+                   "for frontend multiplexing",
+                   chr->label);
         return;
     }
 
+    chardevs = mux->chardev;
+    for (i = 0; i < MAX_MUX; i++) {
+        char *chardev;
+
+        offset = qemu_strchrnul(chardevs, ',');
+        length = offset - chardevs;
+        if (!length) {
+            break;
+        }
+        chardev = strndupa(chardevs, length);
+        chardevs += length + 1;
+        drv = qemu_chr_find(chardev);
+        if (drv == NULL) {
+            error_setg(errp, "mux: base chardev %s not found", chardev);
+            goto deinit_on_error;
+        }
+        qemu_chr_fe_init(&d->chrs[i], drv, errp);
+        d->be_cnt += 1;
+    }
     d->focus = -1;
     /* only default to opened state if we've realized the initial
      * set of muxes
      */
     *be_opened = muxes_opened;
-    qemu_chr_fe_init(&d->chr, drv, errp);
+
+    return;
+
+deinit_on_error:
+    for (i = 0; i < d->be_cnt; i++) {
+        qemu_chr_fe_deinit(&d->chrs[i], false);
+    }
+    d->be_cnt = 0;
 }
 
 static void qemu_chr_parse_mux(QemuOpts *opts, ChardevBackend *backend,
diff --git a/chardev/char.c b/chardev/char.c
index ba847b6e9eff..2643c79e5749 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -333,7 +333,7 @@ static bool qemu_chr_is_busy(Chardev *s)
 {
     if (CHARDEV_IS_MUX(s)) {
         MuxChardev *d = MUX_CHARDEV(s);
-        return d->mux_cnt >= 0;
+        return d->fe_cnt >= 0;
     } else {
         return s->be != NULL;
     }
diff --git a/chardev/chardev-internal.h b/chardev/chardev-internal.h
index 4e03af31476c..72c2e4da7552 100644
--- a/chardev/chardev-internal.h
+++ b/chardev/chardev-internal.h
@@ -35,10 +35,13 @@
 
 struct MuxChardev {
     Chardev parent;
+    /* Linked frontends */
     CharBackend *backends[MAX_MUX];
-    CharBackend chr;
+    /* Linked backends */
+    CharBackend chrs[MAX_MUX];
     int focus;
-    int mux_cnt;
+    int fe_cnt;
+    int be_cnt;
     int term_got_escape;
     int max_size;
     /* Intermediate input buffer catches escape sequences even if the
-- 
2.34.1

Re: [PATCH 1/2] chardev: implement backend chardev multiplexing

[Marc-André Lureau]

Hi Roman

On Fri, Sep 13, 2024 at 8:37 PM Roman Penyaev <r.peniaev@gmail.com> wrote:
>
> This patch implements multiplexing capability of several backend
> devices, which opens up an opportunity to use a single frontend
> device on the guest, which can be manipulated from several
> backend devices.
>
> The idea of the change is trivial: keep list of backend devices
> (up to 4), init them on demand and forward data buffer back and
> forth.
>
> The following is QEMU command line example:
>
>   -chardev socket,path=/tmp/sock,server=on,wait=off,id=sock0 \
>   -chardev vc,id=vc0 \
>   -chardev mux,id=mux0,chardev=vc0,,sock0 \
>   -device virtconsole,chardev=mux0 \
>   -vnc 0.0.0.0:0
>
> Which creates 2 backend devices: text virtual console (`vc0`)
> and a socket (`sock0`) connected to the single virtio hvc
> console with the multiplexer (`mux0`) help. `vc0` renders
> text to an image, which can be shared over the VNC protocol.
> `sock0` is a socket backend which provides biderectional
> communication to the virtio hvc console.

I think I would rather implement a new mux for this purpose, like
"mux-be" perhaps?

"mux" has been a bit hidden (behind mux=on) for various reasons, and
is probably not at production quality level.

I am not sure how CLI should handle option arrays these days
(especially with -chardev CLI not being json-friendly).

>
> Signed-off-by: Roman Penyaev <r.peniaev@gmail.com>
> Cc: "Marc-André Lureau" <marcandre.lureau@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: qemu-devel@nongnu.org
> ---
>  chardev/char-fe.c          |  14 +++--
>  chardev/char-mux.c         | 120 +++++++++++++++++++++++++++++--------
>  chardev/char.c             |   2 +-
>  chardev/chardev-internal.h |   7 ++-
>  4 files changed, 111 insertions(+), 32 deletions(-)
>
> diff --git a/chardev/char-fe.c b/chardev/char-fe.c
> index b214ba3802b1..d1f67338084d 100644
> --- a/chardev/char-fe.c
> +++ b/chardev/char-fe.c
> @@ -197,16 +197,22 @@ bool qemu_chr_fe_init(CharBackend *b, Chardev *s, Error **errp)
>          if (CHARDEV_IS_MUX(s)) {
>              MuxChardev *d = MUX_CHARDEV(s);
>
> -            if (d->mux_cnt >= MAX_MUX) {
> +            if (d->fe_cnt >= MAX_MUX) {
>                  error_setg(errp,
>                             "too many uses of multiplexed chardev '%s'"
>                             " (maximum is " stringify(MAX_MUX) ")",
>                             s->label);
>                  return false;
>              }
> -
> -            d->backends[d->mux_cnt] = b;
> -            tag = d->mux_cnt++;
> +            if (d->fe_cnt > 0 && d->be_cnt > 1) {
> +                error_setg(errp,
> +                           "multiplexed chardev '%s' is already used "
> +                           "for backend multiplexing",
> +                           s->label);
> +                return false;
> +            }
> +            d->backends[d->fe_cnt] = b;
> +            tag = d->fe_cnt++;
>          } else if (s->be) {
>              error_setg(errp, "chardev '%s' is already in use", s->label);
>              return false;
> diff --git a/chardev/char-mux.c b/chardev/char-mux.c
> index ee2d47b20d9b..82f728b5caf8 100644
> --- a/chardev/char-mux.c
> +++ b/chardev/char-mux.c
> @@ -26,6 +26,7 @@
>  #include "qapi/error.h"
>  #include "qemu/module.h"
>  #include "qemu/option.h"
> +#include "qemu/cutils.h"
>  #include "chardev/char.h"
>  #include "sysemu/block-backend.h"
>  #include "qapi/qapi-commands-control.h"
> @@ -40,13 +41,39 @@
>   */
>  static bool muxes_opened = true;
>
> +/* Write to all backends  */
> +static int mux_chr_fe_write(MuxChardev *mux, const uint8_t *buf, int len)
> +{
> +        int r, ret = -1, i;
> +
> +        for (i = 0; i < mux->be_cnt; i++) {
> +                r = qemu_chr_fe_write(&mux->chrs[i], buf, len);
> +                ret = ret < 0 ? r : MAX(r, ret);

I think it should be conservative and fail early if one of the backend
fails. Also if different frontends can write different amounts, there
needs to be some buffering... or it should always use write_all()
which has also a shortcoming since it blocks the thread.

> +        }
> +
> +        return ret;
> +}
> +
> +/* Write to all backends  */
> +static int mux_chr_fe_write_all(MuxChardev *mux, const uint8_t *buf, int len)
> +{
> +        int r, ret = -1, i;
> +
> +        for (i = 0; i < mux->be_cnt; i++) {
> +                r = qemu_chr_fe_write_all(&mux->chrs[i], buf, len);
> +                ret = ret < 0 ? r : MAX(r, ret);
> +        }
> +
> +        return ret;
> +}
> +
>  /* Called with chr_write_lock held.  */
>  static int mux_chr_write(Chardev *chr, const uint8_t *buf, int len)
>  {
>      MuxChardev *d = MUX_CHARDEV(chr);
>      int ret;
>      if (!d->timestamps) {
> -        ret = qemu_chr_fe_write(&d->chr, buf, len);
> +        ret = mux_chr_fe_write(d, buf, len);
>      } else {
>          int i;
>
> @@ -71,11 +98,11 @@ static int mux_chr_write(Chardev *chr, const uint8_t *buf, int len)
>                           (int)(ti % 1000));
>                  /* XXX this blocks entire thread. Rewrite to use
>                   * qemu_chr_fe_write and background I/O callbacks */
> -                qemu_chr_fe_write_all(&d->chr,
> +                mux_chr_fe_write_all(d,
>                                        (uint8_t *)buf1, strlen(buf1));
>                  d->linestart = 0;
>              }
> -            ret += qemu_chr_fe_write(&d->chr, buf + i, 1);
> +            ret += mux_chr_fe_write(d, buf + i, 1);
>              if (buf[i] == '\n') {
>                  d->linestart = 1;
>              }
> @@ -168,9 +195,9 @@ static int mux_proc_byte(Chardev *chr, MuxChardev *d, int ch)
>              qemu_chr_be_event(chr, CHR_EVENT_BREAK);
>              break;
>          case 'c':
> -            assert(d->mux_cnt > 0); /* handler registered with first fe */
> +            assert(d->fe_cnt > 0); /* handler registered with first fe */
>              /* Switch to the next registered device */
> -            mux_set_focus(chr, (d->focus + 1) % d->mux_cnt);
> +            mux_set_focus(chr, (d->focus + 1) % d->fe_cnt);
>              break;
>          case 't':
>              d->timestamps = !d->timestamps;
> @@ -248,8 +275,8 @@ void mux_chr_send_all_event(Chardev *chr, QEMUChrEvent event)
>          return;
>      }
>
> -    /* Send the event to all registered listeners */
> -    for (i = 0; i < d->mux_cnt; i++) {
> +    /* Send the event to all registered frontend listeners */
> +    for (i = 0; i < d->fe_cnt; i++) {
>          mux_chr_send_event(d, i, event);
>      }
>  }
> @@ -262,8 +289,16 @@ static void mux_chr_event(void *opaque, QEMUChrEvent event)
>  static GSource *mux_chr_add_watch(Chardev *s, GIOCondition cond)
>  {
>      MuxChardev *d = MUX_CHARDEV(s);
> -    Chardev *chr = qemu_chr_fe_get_driver(&d->chr);
> -    ChardevClass *cc = CHARDEV_GET_CLASS(chr);
> +    Chardev *chr;
> +    ChardevClass *cc;
> +
> +    if (d->be_cnt > 1) {
> +            /* TODO: multiple backends have to be combined on a single watch */

I think this must be done, otherwise there is a severe limitation.

> +            return NULL;
> +    }
> +
> +    chr = qemu_chr_fe_get_driver(&d->chrs[0]);
> +    cc = CHARDEV_GET_CLASS(chr);
>
>      if (!cc->chr_add_watch) {
>          return NULL;
> @@ -277,27 +312,32 @@ static void char_mux_finalize(Object *obj)
>      MuxChardev *d = MUX_CHARDEV(obj);
>      int i;
>
> -    for (i = 0; i < d->mux_cnt; i++) {
> +    for (i = 0; i < d->fe_cnt; i++) {
>          CharBackend *be = d->backends[i];
>          if (be) {
>              be->chr = NULL;
>          }
>      }
> -    qemu_chr_fe_deinit(&d->chr, false);
> +    for (i = 0; i < d->be_cnt; i++) {
> +        qemu_chr_fe_deinit(&d->chrs[i], false);
> +    }
>  }
>
>  static void mux_chr_update_read_handlers(Chardev *chr)
>  {
>      MuxChardev *d = MUX_CHARDEV(chr);
> +    int i;
>
> -    /* Fix up the real driver with mux routines */
> -    qemu_chr_fe_set_handlers_full(&d->chr,
> -                                  mux_chr_can_read,
> -                                  mux_chr_read,
> -                                  mux_chr_event,
> -                                  NULL,
> -                                  chr,
> -                                  chr->gcontext, true, false);
> +    for (i = 0; i < d->be_cnt; i++) {
> +        /* Fix up the real driver with mux routines */
> +        qemu_chr_fe_set_handlers_full(&d->chrs[i],
> +                                      mux_chr_can_read,
> +                                      mux_chr_read,
> +                                      mux_chr_event,
> +                                      NULL,
> +                                      chr,
> +                                      chr->gcontext, true, false);
> +    }
>  }
>
>  void mux_set_focus(Chardev *chr, int focus)
> @@ -305,7 +345,7 @@ void mux_set_focus(Chardev *chr, int focus)
>      MuxChardev *d = MUX_CHARDEV(chr);
>
>      assert(focus >= 0);
> -    assert(focus < d->mux_cnt);
> +    assert(focus < d->fe_cnt);
>
>      if (d->focus != -1) {
>          mux_chr_send_event(d, d->focus, CHR_EVENT_MUX_OUT);
> @@ -324,19 +364,49 @@ static void qemu_chr_open_mux(Chardev *chr,
>      ChardevMux *mux = backend->u.mux.data;
>      Chardev *drv;
>      MuxChardev *d = MUX_CHARDEV(chr);
> -
> -    drv = qemu_chr_find(mux->chardev);
> -    if (drv == NULL) {
> -        error_setg(errp, "mux: base chardev %s not found", mux->chardev);
> +    const char *offset, *chardevs;
> +    int length, i;
> +
> +    if (d->fe_cnt > 1) {
> +        error_setg(errp,
> +                   "multiplexed chardev '%s' is already used "
> +                   "for frontend multiplexing",
> +                   chr->label);
>          return;
>      }
>
> +    chardevs = mux->chardev;
> +    for (i = 0; i < MAX_MUX; i++) {
> +        char *chardev;
> +
> +        offset = qemu_strchrnul(chardevs, ',');
> +        length = offset - chardevs;
> +        if (!length) {
> +            break;
> +        }
> +        chardev = strndupa(chardevs, length);
> +        chardevs += length + 1;
> +        drv = qemu_chr_find(chardev);
> +        if (drv == NULL) {
> +            error_setg(errp, "mux: base chardev %s not found", chardev);
> +            goto deinit_on_error;
> +        }
> +        qemu_chr_fe_init(&d->chrs[i], drv, errp);
> +        d->be_cnt += 1;
> +    }
>      d->focus = -1;
>      /* only default to opened state if we've realized the initial
>       * set of muxes
>       */
>      *be_opened = muxes_opened;
> -    qemu_chr_fe_init(&d->chr, drv, errp);
> +
> +    return;
> +
> +deinit_on_error:
> +    for (i = 0; i < d->be_cnt; i++) {
> +        qemu_chr_fe_deinit(&d->chrs[i], false);
> +    }
> +    d->be_cnt = 0;
>  }
>
>  static void qemu_chr_parse_mux(QemuOpts *opts, ChardevBackend *backend,
> diff --git a/chardev/char.c b/chardev/char.c
> index ba847b6e9eff..2643c79e5749 100644
> --- a/chardev/char.c
> +++ b/chardev/char.c
> @@ -333,7 +333,7 @@ static bool qemu_chr_is_busy(Chardev *s)
>  {
>      if (CHARDEV_IS_MUX(s)) {
>          MuxChardev *d = MUX_CHARDEV(s);
> -        return d->mux_cnt >= 0;
> +        return d->fe_cnt >= 0;
>      } else {
>          return s->be != NULL;
>      }
> diff --git a/chardev/chardev-internal.h b/chardev/chardev-internal.h
> index 4e03af31476c..72c2e4da7552 100644
> --- a/chardev/chardev-internal.h
> +++ b/chardev/chardev-internal.h
> @@ -35,10 +35,13 @@
>
>  struct MuxChardev {
>      Chardev parent;
> +    /* Linked frontends */
>      CharBackend *backends[MAX_MUX];
> -    CharBackend chr;
> +    /* Linked backends */
> +    CharBackend chrs[MAX_MUX];
>      int focus;
> -    int mux_cnt;
> +    int fe_cnt;
> +    int be_cnt;
>      int term_got_escape;
>      int max_size;
>      /* Intermediate input buffer catches escape sequences even if the
> --
> 2.34.1
>


It would also require some tests and QAPI support.

Re: [PATCH 1/2] chardev: implement backend chardev multiplexing

[Peter Maydell]

On Tue, 17 Sept 2024 at 13:32, Marc-André Lureau
<marcandre.lureau@redhat.com> wrote:
>
> Hi Roman
>
> On Fri, Sep 13, 2024 at 8:37 PM Roman Penyaev <r.peniaev@gmail.com> wrote:
> >
> > This patch implements multiplexing capability of several backend
> > devices, which opens up an opportunity to use a single frontend
> > device on the guest, which can be manipulated from several
> > backend devices.
> >
> > The idea of the change is trivial: keep list of backend devices
> > (up to 4), init them on demand and forward data buffer back and
> > forth.
> >
> > The following is QEMU command line example:
> >
> >   -chardev socket,path=/tmp/sock,server=on,wait=off,id=sock0 \
> >   -chardev vc,id=vc0 \
> >   -chardev mux,id=mux0,chardev=vc0,,sock0 \
> >   -device virtconsole,chardev=mux0 \
> >   -vnc 0.0.0.0:0
> >
> > Which creates 2 backend devices: text virtual console (`vc0`)
> > and a socket (`sock0`) connected to the single virtio hvc
> > console with the multiplexer (`mux0`) help. `vc0` renders
> > text to an image, which can be shared over the VNC protocol.
> > `sock0` is a socket backend which provides biderectional
> > communication to the virtio hvc console.
>
> I think I would rather implement a new mux for this purpose, like
> "mux-be" perhaps?
>
> "mux" has been a bit hidden (behind mux=on) for various reasons, and
> is probably not at production quality level.

You get a mux by default (for serial vs HMP monitor), so
I think it's pretty heavily used and tested in that sense...

-- PMM

Re: [PATCH 1/2] chardev: implement backend chardev multiplexing

[Peter Maydell]

On Tue, 17 Sept 2024 at 13:40, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Tue, 17 Sept 2024 at 13:32, Marc-André Lureau
> <marcandre.lureau@redhat.com> wrote:
> >
> > Hi Roman
> >
> > On Fri, Sep 13, 2024 at 8:37 PM Roman Penyaev <r.peniaev@gmail.com> wrote:
> > >
> > > This patch implements multiplexing capability of several backend
> > > devices, which opens up an opportunity to use a single frontend
> > > device on the guest, which can be manipulated from several
> > > backend devices.
> > >
> > > The idea of the change is trivial: keep list of backend devices
> > > (up to 4), init them on demand and forward data buffer back and
> > > forth.
> > >
> > > The following is QEMU command line example:
> > >
> > >   -chardev socket,path=/tmp/sock,server=on,wait=off,id=sock0 \
> > >   -chardev vc,id=vc0 \
> > >   -chardev mux,id=mux0,chardev=vc0,,sock0 \
> > >   -device virtconsole,chardev=mux0 \
> > >   -vnc 0.0.0.0:0
> > >
> > > Which creates 2 backend devices: text virtual console (`vc0`)
> > > and a socket (`sock0`) connected to the single virtio hvc
> > > console with the multiplexer (`mux0`) help. `vc0` renders
> > > text to an image, which can be shared over the VNC protocol.
> > > `sock0` is a socket backend which provides biderectional
> > > communication to the virtio hvc console.
> >
> > I think I would rather implement a new mux for this purpose, like
> > "mux-be" perhaps?
> >
> > "mux" has been a bit hidden (behind mux=on) for various reasons, and
> > is probably not at production quality level.
>
> You get a mux by default (for serial vs HMP monitor), so
> I think it's pretty heavily used and tested in that sense...

I should have said "by default for any -nographic invocation";
that's still a pretty common usage pattern, though.

-- PMM

Re: [PATCH 1/2] chardev: implement backend chardev multiplexing

[Roman Penyaev]

Hi Marc-André,

On Tue, Sep 17, 2024 at 2:31 PM Marc-André Lureau
<marcandre.lureau@redhat.com> wrote:
>
> Hi Roman
>
> On Fri, Sep 13, 2024 at 8:37 PM Roman Penyaev <r.peniaev@gmail.com> wrote:
> >
> > This patch implements multiplexing capability of several backend
> > devices, which opens up an opportunity to use a single frontend
> > device on the guest, which can be manipulated from several
> > backend devices.
> >
> > The idea of the change is trivial: keep list of backend devices
> > (up to 4), init them on demand and forward data buffer back and
> > forth.
> >
> > The following is QEMU command line example:
> >
> >   -chardev socket,path=/tmp/sock,server=on,wait=off,id=sock0 \
> >   -chardev vc,id=vc0 \
> >   -chardev mux,id=mux0,chardev=vc0,,sock0 \
> >   -device virtconsole,chardev=mux0 \
> >   -vnc 0.0.0.0:0
> >
> > Which creates 2 backend devices: text virtual console (`vc0`)
> > and a socket (`sock0`) connected to the single virtio hvc
> > console with the multiplexer (`mux0`) help. `vc0` renders
> > text to an image, which can be shared over the VNC protocol.
> > `sock0` is a socket backend which provides biderectional
> > communication to the virtio hvc console.
>
> I think I would rather implement a new mux for this purpose, like
> "mux-be" perhaps?

Do you mean a completely different implementation or just an alias
for the cli needs? Because code of the backend multiplexing nicely fits
current mux and I tried to avoid code duplication.

>
> "mux" has been a bit hidden (behind mux=on) for various reasons, and
> is probably not at production quality level.

Well, indeed creating "chardev mux" is not described in the doc, but you
can do this anyway :) Here I just followed the same pattern as we do for
other devices: "chardev NAME". Having a "mux-be" (or any other) alias
won't expose the default "mux", so configurations can be separated.
Is that what you meant?

Also, mux is used implicitly for various of configurations, this is
described in the manual:

   Note that some other command line options may implicitly create
   multiplexed character backends; for instance -serial mon:stdio creates
   a multiplexed stdio backend connected to the serial port and the QEMU
   monitor, and -nographic also multiplexes the console and the
monitor to stdio.

So it seems to me mux is used (tested) quite extensively.

>
> I am not sure how CLI should handle option arrays these days
> (especially with -chardev CLI not being json-friendly).

By CLI do you mean the default QEMU command line interface?
The command line with arrays (double commas ",,") I specified above
works fine. Can you suggest any other tool (or json formatting) you have
in mind so I can verify?

>
> >
> > Signed-off-by: Roman Penyaev <r.peniaev@gmail.com>
> > Cc: "Marc-André Lureau" <marcandre.lureau@redhat.com>
> > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > Cc: qemu-devel@nongnu.org
> > ---
> >  chardev/char-fe.c          |  14 +++--
> >  chardev/char-mux.c         | 120 +++++++++++++++++++++++++++++--------
> >  chardev/char.c             |   2 +-
> >  chardev/chardev-internal.h |   7 ++-
> >  4 files changed, 111 insertions(+), 32 deletions(-)
> >
> > diff --git a/chardev/char-fe.c b/chardev/char-fe.c
> > index b214ba3802b1..d1f67338084d 100644
> > --- a/chardev/char-fe.c
> > +++ b/chardev/char-fe.c
> > @@ -197,16 +197,22 @@ bool qemu_chr_fe_init(CharBackend *b, Chardev *s, Error **errp)
> >          if (CHARDEV_IS_MUX(s)) {
> >              MuxChardev *d = MUX_CHARDEV(s);
> >
> > -            if (d->mux_cnt >= MAX_MUX) {
> > +            if (d->fe_cnt >= MAX_MUX) {
> >                  error_setg(errp,
> >                             "too many uses of multiplexed chardev '%s'"
> >                             " (maximum is " stringify(MAX_MUX) ")",
> >                             s->label);
> >                  return false;
> >              }
> > -
> > -            d->backends[d->mux_cnt] = b;
> > -            tag = d->mux_cnt++;
> > +            if (d->fe_cnt > 0 && d->be_cnt > 1) {
> > +                error_setg(errp,
> > +                           "multiplexed chardev '%s' is already used "
> > +                           "for backend multiplexing",
> > +                           s->label);
> > +                return false;
> > +            }
> > +            d->backends[d->fe_cnt] = b;
> > +            tag = d->fe_cnt++;
> >          } else if (s->be) {
> >              error_setg(errp, "chardev '%s' is already in use", s->label);
> >              return false;
> > diff --git a/chardev/char-mux.c b/chardev/char-mux.c
> > index ee2d47b20d9b..82f728b5caf8 100644
> > --- a/chardev/char-mux.c
> > +++ b/chardev/char-mux.c
> > @@ -26,6 +26,7 @@
> >  #include "qapi/error.h"
> >  #include "qemu/module.h"
> >  #include "qemu/option.h"
> > +#include "qemu/cutils.h"
> >  #include "chardev/char.h"
> >  #include "sysemu/block-backend.h"
> >  #include "qapi/qapi-commands-control.h"
> > @@ -40,13 +41,39 @@
> >   */
> >  static bool muxes_opened = true;
> >
> > +/* Write to all backends  */
> > +static int mux_chr_fe_write(MuxChardev *mux, const uint8_t *buf, int len)
> > +{
> > +        int r, ret = -1, i;
> > +
> > +        for (i = 0; i < mux->be_cnt; i++) {
> > +                r = qemu_chr_fe_write(&mux->chrs[i], buf, len);
> > +                ret = ret < 0 ? r : MAX(r, ret);
>
> I think it should be conservative and fail early if one of the backend
> fails. Also if different frontends can write different amounts, there
> needs to be some buffering... or it should always use write_all()
> which has also a shortcoming since it blocks the thread.

Yes, early fail and buffers make sense.

>
> > +        }
> > +
> > +        return ret;
> > +}
> > +
> > +/* Write to all backends  */
> > +static int mux_chr_fe_write_all(MuxChardev *mux, const uint8_t *buf, int len)
> > +{
> > +        int r, ret = -1, i;
> > +
> > +        for (i = 0; i < mux->be_cnt; i++) {
> > +                r = qemu_chr_fe_write_all(&mux->chrs[i], buf, len);
> > +                ret = ret < 0 ? r : MAX(r, ret);
> > +        }
> > +
> > +        return ret;
> > +}
> > +
> >  /* Called with chr_write_lock held.  */
> >  static int mux_chr_write(Chardev *chr, const uint8_t *buf, int len)
> >  {
> >      MuxChardev *d = MUX_CHARDEV(chr);
> >      int ret;
> >      if (!d->timestamps) {
> > -        ret = qemu_chr_fe_write(&d->chr, buf, len);
> > +        ret = mux_chr_fe_write(d, buf, len);
> >      } else {
> >          int i;
> >
> > @@ -71,11 +98,11 @@ static int mux_chr_write(Chardev *chr, const uint8_t *buf, int len)
> >                           (int)(ti % 1000));
> >                  /* XXX this blocks entire thread. Rewrite to use
> >                   * qemu_chr_fe_write and background I/O callbacks */
> > -                qemu_chr_fe_write_all(&d->chr,
> > +                mux_chr_fe_write_all(d,
> >                                        (uint8_t *)buf1, strlen(buf1));
> >                  d->linestart = 0;
> >              }
> > -            ret += qemu_chr_fe_write(&d->chr, buf + i, 1);
> > +            ret += mux_chr_fe_write(d, buf + i, 1);
> >              if (buf[i] == '\n') {
> >                  d->linestart = 1;
> >              }
> > @@ -168,9 +195,9 @@ static int mux_proc_byte(Chardev *chr, MuxChardev *d, int ch)
> >              qemu_chr_be_event(chr, CHR_EVENT_BREAK);
> >              break;
> >          case 'c':
> > -            assert(d->mux_cnt > 0); /* handler registered with first fe */
> > +            assert(d->fe_cnt > 0); /* handler registered with first fe */
> >              /* Switch to the next registered device */
> > -            mux_set_focus(chr, (d->focus + 1) % d->mux_cnt);
> > +            mux_set_focus(chr, (d->focus + 1) % d->fe_cnt);
> >              break;
> >          case 't':
> >              d->timestamps = !d->timestamps;
> > @@ -248,8 +275,8 @@ void mux_chr_send_all_event(Chardev *chr, QEMUChrEvent event)
> >          return;
> >      }
> >
> > -    /* Send the event to all registered listeners */
> > -    for (i = 0; i < d->mux_cnt; i++) {
> > +    /* Send the event to all registered frontend listeners */
> > +    for (i = 0; i < d->fe_cnt; i++) {
> >          mux_chr_send_event(d, i, event);
> >      }
> >  }
> > @@ -262,8 +289,16 @@ static void mux_chr_event(void *opaque, QEMUChrEvent event)
> >  static GSource *mux_chr_add_watch(Chardev *s, GIOCondition cond)
> >  {
> >      MuxChardev *d = MUX_CHARDEV(s);
> > -    Chardev *chr = qemu_chr_fe_get_driver(&d->chr);
> > -    ChardevClass *cc = CHARDEV_GET_CLASS(chr);
> > +    Chardev *chr;
> > +    ChardevClass *cc;
> > +
> > +    if (d->be_cnt > 1) {
> > +            /* TODO: multiple backends have to be combined on a single watch */
>
> I think this must be done, otherwise there is a severe limitation.

Ok.

>
> > +            return NULL;
> > +    }
> > +
> > +    chr = qemu_chr_fe_get_driver(&d->chrs[0]);
> > +    cc = CHARDEV_GET_CLASS(chr);
> >
> >      if (!cc->chr_add_watch) {
> >          return NULL;
> > @@ -277,27 +312,32 @@ static void char_mux_finalize(Object *obj)
> >      MuxChardev *d = MUX_CHARDEV(obj);
> >      int i;
> >
> > -    for (i = 0; i < d->mux_cnt; i++) {
> > +    for (i = 0; i < d->fe_cnt; i++) {
> >          CharBackend *be = d->backends[i];
> >          if (be) {
> >              be->chr = NULL;
> >          }
> >      }
> > -    qemu_chr_fe_deinit(&d->chr, false);
> > +    for (i = 0; i < d->be_cnt; i++) {
> > +        qemu_chr_fe_deinit(&d->chrs[i], false);
> > +    }
> >  }
> >
> >  static void mux_chr_update_read_handlers(Chardev *chr)
> >  {
> >      MuxChardev *d = MUX_CHARDEV(chr);
> > +    int i;
> >
> > -    /* Fix up the real driver with mux routines */
> > -    qemu_chr_fe_set_handlers_full(&d->chr,
> > -                                  mux_chr_can_read,
> > -                                  mux_chr_read,
> > -                                  mux_chr_event,
> > -                                  NULL,
> > -                                  chr,
> > -                                  chr->gcontext, true, false);
> > +    for (i = 0; i < d->be_cnt; i++) {
> > +        /* Fix up the real driver with mux routines */
> > +        qemu_chr_fe_set_handlers_full(&d->chrs[i],
> > +                                      mux_chr_can_read,
> > +                                      mux_chr_read,
> > +                                      mux_chr_event,
> > +                                      NULL,
> > +                                      chr,
> > +                                      chr->gcontext, true, false);
> > +    }
> >  }
> >
> >  void mux_set_focus(Chardev *chr, int focus)
> > @@ -305,7 +345,7 @@ void mux_set_focus(Chardev *chr, int focus)
> >      MuxChardev *d = MUX_CHARDEV(chr);
> >
> >      assert(focus >= 0);
> > -    assert(focus < d->mux_cnt);
> > +    assert(focus < d->fe_cnt);
> >
> >      if (d->focus != -1) {
> >          mux_chr_send_event(d, d->focus, CHR_EVENT_MUX_OUT);
> > @@ -324,19 +364,49 @@ static void qemu_chr_open_mux(Chardev *chr,
> >      ChardevMux *mux = backend->u.mux.data;
> >      Chardev *drv;
> >      MuxChardev *d = MUX_CHARDEV(chr);
> > -
> > -    drv = qemu_chr_find(mux->chardev);
> > -    if (drv == NULL) {
> > -        error_setg(errp, "mux: base chardev %s not found", mux->chardev);
> > +    const char *offset, *chardevs;
> > +    int length, i;
> > +
> > +    if (d->fe_cnt > 1) {
> > +        error_setg(errp,
> > +                   "multiplexed chardev '%s' is already used "
> > +                   "for frontend multiplexing",
> > +                   chr->label);
> >          return;
> >      }
> >
> > +    chardevs = mux->chardev;
> > +    for (i = 0; i < MAX_MUX; i++) {
> > +        char *chardev;
> > +
> > +        offset = qemu_strchrnul(chardevs, ',');
> > +        length = offset - chardevs;
> > +        if (!length) {
> > +            break;
> > +        }
> > +        chardev = strndupa(chardevs, length);
> > +        chardevs += length + 1;
> > +        drv = qemu_chr_find(chardev);
> > +        if (drv == NULL) {
> > +            error_setg(errp, "mux: base chardev %s not found", chardev);
> > +            goto deinit_on_error;
> > +        }
> > +        qemu_chr_fe_init(&d->chrs[i], drv, errp);
> > +        d->be_cnt += 1;
> > +    }
> >      d->focus = -1;
> >      /* only default to opened state if we've realized the initial
> >       * set of muxes
> >       */
> >      *be_opened = muxes_opened;
> > -    qemu_chr_fe_init(&d->chr, drv, errp);
> > +
> > +    return;
> > +
> > +deinit_on_error:
> > +    for (i = 0; i < d->be_cnt; i++) {
> > +        qemu_chr_fe_deinit(&d->chrs[i], false);
> > +    }
> > +    d->be_cnt = 0;
> >  }
> >
> >  static void qemu_chr_parse_mux(QemuOpts *opts, ChardevBackend *backend,
> > diff --git a/chardev/char.c b/chardev/char.c
> > index ba847b6e9eff..2643c79e5749 100644
> > --- a/chardev/char.c
> > +++ b/chardev/char.c
> > @@ -333,7 +333,7 @@ static bool qemu_chr_is_busy(Chardev *s)
> >  {
> >      if (CHARDEV_IS_MUX(s)) {
> >          MuxChardev *d = MUX_CHARDEV(s);
> > -        return d->mux_cnt >= 0;
> > +        return d->fe_cnt >= 0;
> >      } else {
> >          return s->be != NULL;
> >      }
> > diff --git a/chardev/chardev-internal.h b/chardev/chardev-internal.h
> > index 4e03af31476c..72c2e4da7552 100644
> > --- a/chardev/chardev-internal.h
> > +++ b/chardev/chardev-internal.h
> > @@ -35,10 +35,13 @@
> >
> >  struct MuxChardev {
> >      Chardev parent;
> > +    /* Linked frontends */
> >      CharBackend *backends[MAX_MUX];
> > -    CharBackend chr;
> > +    /* Linked backends */
> > +    CharBackend chrs[MAX_MUX];
> >      int focus;
> > -    int mux_cnt;
> > +    int fe_cnt;
> > +    int be_cnt;
> >      int term_got_escape;
> >      int max_size;
> >      /* Intermediate input buffer catches escape sequences even if the
> > --
> > 2.34.1
> >
>
>
> It would also require some tests and QAPI support.

I will take a look at tests and will try to come up with some extension
for backend multiplexing.

Since the mux object API was not changed and the current change heavily
relies on what is already in the code, do you think there should be any
QAPI related change? In my understanding this should work out of the box
(not tested though).

--
Roman

Re: [PATCH 1/2] chardev: implement backend chardev multiplexing

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 16456 bytes --]

Hi

On Tue, Sep 17, 2024 at 6:17 PM Roman Penyaev <r.peniaev@gmail.com> wrote:

> Hi Marc-André,
>
> On Tue, Sep 17, 2024 at 2:31 PM Marc-André Lureau
> <marcandre.lureau@redhat.com> wrote:
> >
> > Hi Roman
> >
> > On Fri, Sep 13, 2024 at 8:37 PM Roman Penyaev <r.peniaev@gmail.com>
> wrote:
> > >
> > > This patch implements multiplexing capability of several backend
> > > devices, which opens up an opportunity to use a single frontend
> > > device on the guest, which can be manipulated from several
> > > backend devices.
> > >
> > > The idea of the change is trivial: keep list of backend devices
> > > (up to 4), init them on demand and forward data buffer back and
> > > forth.
> > >
> > > The following is QEMU command line example:
> > >
> > >   -chardev socket,path=/tmp/sock,server=on,wait=off,id=sock0 \
> > >   -chardev vc,id=vc0 \
> > >   -chardev mux,id=mux0,chardev=vc0,,sock0 \
> > >   -device virtconsole,chardev=mux0 \
> > >   -vnc 0.0.0.0:0
> > >
> > > Which creates 2 backend devices: text virtual console (`vc0`)
> > > and a socket (`sock0`) connected to the single virtio hvc
> > > console with the multiplexer (`mux0`) help. `vc0` renders
> > > text to an image, which can be shared over the VNC protocol.
> > > `sock0` is a socket backend which provides biderectional
> > > communication to the virtio hvc console.
> >
> > I think I would rather implement a new mux for this purpose, like
> > "mux-be" perhaps?
>
> Do you mean a completely different implementation or just an alias
> for the cli needs? Because code of the backend multiplexing nicely fits
> current mux and I tried to avoid code duplication.
>

It's not the same behaviour though, and discoverability/compatibility would
be easier to handle. There is also various code in mux that isn't needed.

You can still factorize common code, or have a common base class.
Eventually, the two chardev can be from the same file. I don't know if that
makes sense.


> >
> > "mux" has been a bit hidden (behind mux=on) for various reasons, and
> > is probably not at production quality level.
>
> Well, indeed creating "chardev mux" is not described in the doc, but you
> can do this anyway :) Here I just followed the same pattern as we do for
> other devices: "chardev NAME". Having a "mux-be" (or any other) alias
> won't expose the default "mux", so configurations can be separated.
> Is that what you meant?
>

yes, this would help in general misconfiguration (although -chardev option
handling has a lot to be desired)


>
> Also, mux is used implicitly for various of configurations, this is
> described in the manual:
>
>    Note that some other command line options may implicitly create
>    multiplexed character backends; for instance -serial mon:stdio creates
>    a multiplexed stdio backend connected to the serial port and the QEMU
>    monitor, and -nographic also multiplexes the console and the
> monitor to stdio.
>
> So it seems to me mux is used (tested) quite extensively.
>

kinda, it's used by end-users who run qemu from the CLI. Probably less in
production systems.


>
> >
> > I am not sure how CLI should handle option arrays these days
> > (especially with -chardev CLI not being json-friendly).
>
> By CLI do you mean the default QEMU command line interface?
>

yes


> The command line with arrays (double commas ",,") I specified above
> works fine. Can you suggest any other tool (or json formatting) you have
> in mind so I can verify?
>

Indeed, this double commas stuff is weird, I don't know that "trick". Is it
used elsewhere and documented? I don't see a test in test-qemu-opts.c
either.

There is another ongoing discussion about json support for -chardev:
"-chardev with a JSON argument".


> >
> > >
> > > Signed-off-by: Roman Penyaev <r.peniaev@gmail.com>
> > > Cc: "Marc-André Lureau" <marcandre.lureau@redhat.com>
> > > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > > Cc: qemu-devel@nongnu.org
> > > ---
> > >  chardev/char-fe.c          |  14 +++--
> > >  chardev/char-mux.c         | 120 +++++++++++++++++++++++++++++--------
> > >  chardev/char.c             |   2 +-
> > >  chardev/chardev-internal.h |   7 ++-
> > >  4 files changed, 111 insertions(+), 32 deletions(-)
> > >
> > > diff --git a/chardev/char-fe.c b/chardev/char-fe.c
> > > index b214ba3802b1..d1f67338084d 100644
> > > --- a/chardev/char-fe.c
> > > +++ b/chardev/char-fe.c
> > > @@ -197,16 +197,22 @@ bool qemu_chr_fe_init(CharBackend *b, Chardev
> *s, Error **errp)
> > >          if (CHARDEV_IS_MUX(s)) {
> > >              MuxChardev *d = MUX_CHARDEV(s);
> > >
> > > -            if (d->mux_cnt >= MAX_MUX) {
> > > +            if (d->fe_cnt >= MAX_MUX) {
> > >                  error_setg(errp,
> > >                             "too many uses of multiplexed chardev '%s'"
> > >                             " (maximum is " stringify(MAX_MUX) ")",
> > >                             s->label);
> > >                  return false;
> > >              }
> > > -
> > > -            d->backends[d->mux_cnt] = b;
> > > -            tag = d->mux_cnt++;
> > > +            if (d->fe_cnt > 0 && d->be_cnt > 1) {
> > > +                error_setg(errp,
> > > +                           "multiplexed chardev '%s' is already used "
> > > +                           "for backend multiplexing",
> > > +                           s->label);
> > > +                return false;
> > > +            }
> > > +            d->backends[d->fe_cnt] = b;
> > > +            tag = d->fe_cnt++;
> > >          } else if (s->be) {
> > >              error_setg(errp, "chardev '%s' is already in use",
> s->label);
> > >              return false;
> > > diff --git a/chardev/char-mux.c b/chardev/char-mux.c
> > > index ee2d47b20d9b..82f728b5caf8 100644
> > > --- a/chardev/char-mux.c
> > > +++ b/chardev/char-mux.c
> > > @@ -26,6 +26,7 @@
> > >  #include "qapi/error.h"
> > >  #include "qemu/module.h"
> > >  #include "qemu/option.h"
> > > +#include "qemu/cutils.h"
> > >  #include "chardev/char.h"
> > >  #include "sysemu/block-backend.h"
> > >  #include "qapi/qapi-commands-control.h"
> > > @@ -40,13 +41,39 @@
> > >   */
> > >  static bool muxes_opened = true;
> > >
> > > +/* Write to all backends  */
> > > +static int mux_chr_fe_write(MuxChardev *mux, const uint8_t *buf, int
> len)
> > > +{
> > > +        int r, ret = -1, i;
> > > +
> > > +        for (i = 0; i < mux->be_cnt; i++) {
> > > +                r = qemu_chr_fe_write(&mux->chrs[i], buf, len);
> > > +                ret = ret < 0 ? r : MAX(r, ret);
> >
> > I think it should be conservative and fail early if one of the backend
> > fails. Also if different frontends can write different amounts, there
> > needs to be some buffering... or it should always use write_all()
> > which has also a shortcoming since it blocks the thread.
>
> Yes, early fail and buffers make sense.
>
> >
> > > +        }
> > > +
> > > +        return ret;
> > > +}
> > > +
> > > +/* Write to all backends  */
> > > +static int mux_chr_fe_write_all(MuxChardev *mux, const uint8_t *buf,
> int len)
> > > +{
> > > +        int r, ret = -1, i;
> > > +
> > > +        for (i = 0; i < mux->be_cnt; i++) {
> > > +                r = qemu_chr_fe_write_all(&mux->chrs[i], buf, len);
> > > +                ret = ret < 0 ? r : MAX(r, ret);
> > > +        }
> > > +
> > > +        return ret;
> > > +}
> > > +
> > >  /* Called with chr_write_lock held.  */
> > >  static int mux_chr_write(Chardev *chr, const uint8_t *buf, int len)
> > >  {
> > >      MuxChardev *d = MUX_CHARDEV(chr);
> > >      int ret;
> > >      if (!d->timestamps) {
> > > -        ret = qemu_chr_fe_write(&d->chr, buf, len);
> > > +        ret = mux_chr_fe_write(d, buf, len);
> > >      } else {
> > >          int i;
> > >
> > > @@ -71,11 +98,11 @@ static int mux_chr_write(Chardev *chr, const
> uint8_t *buf, int len)
> > >                           (int)(ti % 1000));
> > >                  /* XXX this blocks entire thread. Rewrite to use
> > >                   * qemu_chr_fe_write and background I/O callbacks */
> > > -                qemu_chr_fe_write_all(&d->chr,
> > > +                mux_chr_fe_write_all(d,
> > >                                        (uint8_t *)buf1, strlen(buf1));
> > >                  d->linestart = 0;
> > >              }
> > > -            ret += qemu_chr_fe_write(&d->chr, buf + i, 1);
> > > +            ret += mux_chr_fe_write(d, buf + i, 1);
> > >              if (buf[i] == '\n') {
> > >                  d->linestart = 1;
> > >              }
> > > @@ -168,9 +195,9 @@ static int mux_proc_byte(Chardev *chr, MuxChardev
> *d, int ch)
> > >              qemu_chr_be_event(chr, CHR_EVENT_BREAK);
> > >              break;
> > >          case 'c':
> > > -            assert(d->mux_cnt > 0); /* handler registered with first
> fe */
> > > +            assert(d->fe_cnt > 0); /* handler registered with first
> fe */
> > >              /* Switch to the next registered device */
> > > -            mux_set_focus(chr, (d->focus + 1) % d->mux_cnt);
> > > +            mux_set_focus(chr, (d->focus + 1) % d->fe_cnt);
> > >              break;
> > >          case 't':
> > >              d->timestamps = !d->timestamps;
> > > @@ -248,8 +275,8 @@ void mux_chr_send_all_event(Chardev *chr,
> QEMUChrEvent event)
> > >          return;
> > >      }
> > >
> > > -    /* Send the event to all registered listeners */
> > > -    for (i = 0; i < d->mux_cnt; i++) {
> > > +    /* Send the event to all registered frontend listeners */
> > > +    for (i = 0; i < d->fe_cnt; i++) {
> > >          mux_chr_send_event(d, i, event);
> > >      }
> > >  }
> > > @@ -262,8 +289,16 @@ static void mux_chr_event(void *opaque,
> QEMUChrEvent event)
> > >  static GSource *mux_chr_add_watch(Chardev *s, GIOCondition cond)
> > >  {
> > >      MuxChardev *d = MUX_CHARDEV(s);
> > > -    Chardev *chr = qemu_chr_fe_get_driver(&d->chr);
> > > -    ChardevClass *cc = CHARDEV_GET_CLASS(chr);
> > > +    Chardev *chr;
> > > +    ChardevClass *cc;
> > > +
> > > +    if (d->be_cnt > 1) {
> > > +            /* TODO: multiple backends have to be combined on a
> single watch */
> >
> > I think this must be done, otherwise there is a severe limitation.
>
> Ok.
>
> >
> > > +            return NULL;
> > > +    }
> > > +
> > > +    chr = qemu_chr_fe_get_driver(&d->chrs[0]);
> > > +    cc = CHARDEV_GET_CLASS(chr);
> > >
> > >      if (!cc->chr_add_watch) {
> > >          return NULL;
> > > @@ -277,27 +312,32 @@ static void char_mux_finalize(Object *obj)
> > >      MuxChardev *d = MUX_CHARDEV(obj);
> > >      int i;
> > >
> > > -    for (i = 0; i < d->mux_cnt; i++) {
> > > +    for (i = 0; i < d->fe_cnt; i++) {
> > >          CharBackend *be = d->backends[i];
> > >          if (be) {
> > >              be->chr = NULL;
> > >          }
> > >      }
> > > -    qemu_chr_fe_deinit(&d->chr, false);
> > > +    for (i = 0; i < d->be_cnt; i++) {
> > > +        qemu_chr_fe_deinit(&d->chrs[i], false);
> > > +    }
> > >  }
> > >
> > >  static void mux_chr_update_read_handlers(Chardev *chr)
> > >  {
> > >      MuxChardev *d = MUX_CHARDEV(chr);
> > > +    int i;
> > >
> > > -    /* Fix up the real driver with mux routines */
> > > -    qemu_chr_fe_set_handlers_full(&d->chr,
> > > -                                  mux_chr_can_read,
> > > -                                  mux_chr_read,
> > > -                                  mux_chr_event,
> > > -                                  NULL,
> > > -                                  chr,
> > > -                                  chr->gcontext, true, false);
> > > +    for (i = 0; i < d->be_cnt; i++) {
> > > +        /* Fix up the real driver with mux routines */
> > > +        qemu_chr_fe_set_handlers_full(&d->chrs[i],
> > > +                                      mux_chr_can_read,
> > > +                                      mux_chr_read,
> > > +                                      mux_chr_event,
> > > +                                      NULL,
> > > +                                      chr,
> > > +                                      chr->gcontext, true, false);
> > > +    }
> > >  }
> > >
> > >  void mux_set_focus(Chardev *chr, int focus)
> > > @@ -305,7 +345,7 @@ void mux_set_focus(Chardev *chr, int focus)
> > >      MuxChardev *d = MUX_CHARDEV(chr);
> > >
> > >      assert(focus >= 0);
> > > -    assert(focus < d->mux_cnt);
> > > +    assert(focus < d->fe_cnt);
> > >
> > >      if (d->focus != -1) {
> > >          mux_chr_send_event(d, d->focus, CHR_EVENT_MUX_OUT);
> > > @@ -324,19 +364,49 @@ static void qemu_chr_open_mux(Chardev *chr,
> > >      ChardevMux *mux = backend->u.mux.data;
> > >      Chardev *drv;
> > >      MuxChardev *d = MUX_CHARDEV(chr);
> > > -
> > > -    drv = qemu_chr_find(mux->chardev);
> > > -    if (drv == NULL) {
> > > -        error_setg(errp, "mux: base chardev %s not found",
> mux->chardev);
> > > +    const char *offset, *chardevs;
> > > +    int length, i;
> > > +
> > > +    if (d->fe_cnt > 1) {
> > > +        error_setg(errp,
> > > +                   "multiplexed chardev '%s' is already used "
> > > +                   "for frontend multiplexing",
> > > +                   chr->label);
> > >          return;
> > >      }
> > >
> > > +    chardevs = mux->chardev;
> > > +    for (i = 0; i < MAX_MUX; i++) {
> > > +        char *chardev;
> > > +
> > > +        offset = qemu_strchrnul(chardevs, ',');
> > > +        length = offset - chardevs;
> > > +        if (!length) {
> > > +            break;
> > > +        }
> > > +        chardev = strndupa(chardevs, length);
> > > +        chardevs += length + 1;
> > > +        drv = qemu_chr_find(chardev);
> > > +        if (drv == NULL) {
> > > +            error_setg(errp, "mux: base chardev %s not found",
> chardev);
> > > +            goto deinit_on_error;
> > > +        }
> > > +        qemu_chr_fe_init(&d->chrs[i], drv, errp);
> > > +        d->be_cnt += 1;
> > > +    }
> > >      d->focus = -1;
> > >      /* only default to opened state if we've realized the initial
> > >       * set of muxes
> > >       */
> > >      *be_opened = muxes_opened;
> > > -    qemu_chr_fe_init(&d->chr, drv, errp);
> > > +
> > > +    return;
> > > +
> > > +deinit_on_error:
> > > +    for (i = 0; i < d->be_cnt; i++) {
> > > +        qemu_chr_fe_deinit(&d->chrs[i], false);
> > > +    }
> > > +    d->be_cnt = 0;
> > >  }
> > >
> > >  static void qemu_chr_parse_mux(QemuOpts *opts, ChardevBackend
> *backend,
> > > diff --git a/chardev/char.c b/chardev/char.c
> > > index ba847b6e9eff..2643c79e5749 100644
> > > --- a/chardev/char.c
> > > +++ b/chardev/char.c
> > > @@ -333,7 +333,7 @@ static bool qemu_chr_is_busy(Chardev *s)
> > >  {
> > >      if (CHARDEV_IS_MUX(s)) {
> > >          MuxChardev *d = MUX_CHARDEV(s);
> > > -        return d->mux_cnt >= 0;
> > > +        return d->fe_cnt >= 0;
> > >      } else {
> > >          return s->be != NULL;
> > >      }
> > > diff --git a/chardev/chardev-internal.h b/chardev/chardev-internal.h
> > > index 4e03af31476c..72c2e4da7552 100644
> > > --- a/chardev/chardev-internal.h
> > > +++ b/chardev/chardev-internal.h
> > > @@ -35,10 +35,13 @@
> > >
> > >  struct MuxChardev {
> > >      Chardev parent;
> > > +    /* Linked frontends */
> > >      CharBackend *backends[MAX_MUX];
> > > -    CharBackend chr;
> > > +    /* Linked backends */
> > > +    CharBackend chrs[MAX_MUX];
> > >      int focus;
> > > -    int mux_cnt;
> > > +    int fe_cnt;
> > > +    int be_cnt;
> > >      int term_got_escape;
> > >      int max_size;
> > >      /* Intermediate input buffer catches escape sequences even if the
> > > --
> > > 2.34.1
> > >
> >
> >
> > It would also require some tests and QAPI support.
>
> I will take a look at tests and will try to come up with some extension
> for backend multiplexing.
>
> Since the mux object API was not changed and the current change heavily
> relies on what is already in the code, do you think there should be any
> QAPI related change? In my understanding this should work out of the box
> (not tested though).
>
> --
> Roman
>
>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 22822 bytes --]

Re: [PATCH 1/2] chardev: implement backend chardev multiplexing

[Roman Penyaev]

Hi,

Thanks for the review, I will try to come up with the next version of
this series.

On Wed, Sep 18, 2024 at 12:52 PM Marc-André Lureau
<marcandre.lureau@gmail.com> wrote:
[cut]
>
>
> Indeed, this double commas stuff is weird, I don't know that "trick". Is it used elsewhere and documented? I don't see a test in test-qemu-opts.c either.

In few places, for example here:
https://www.qemu.org/docs/master/system/qemu-manpage.html#options
(look for "double")

The second quote acts as an escape symbol to avoid splitting of the
command line.
Then in the modified mux I used a single comma as a separator for an array of
backends.

>
> There is another ongoing discussion about json support for -chardev: "-chardev with a JSON argument".

Thanks, found.

--
Roman

[PATCH 2/2] qemu-options.hx: describe multiplexing of several backend devices

[Roman Penyaev]

This adds a few lines describing multiplexer configuration for
multiplexing several backend devices with a single frontend
device.

Signed-off-by: Roman Penyaev <r.peniaev@gmail.com>
Cc: "Marc-André Lureau" <marcandre.lureau@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: qemu-devel@nongnu.org
---
 qemu-options.hx | 44 ++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 40 insertions(+), 4 deletions(-)

diff --git a/qemu-options.hx b/qemu-options.hx
index d94e2cbbaeb1..de1d540b85cd 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -3782,7 +3782,7 @@ SRST
 The general form of a character device option is:
 
 ``-chardev backend,id=id[,mux=on|off][,options]``
-    Backend is one of: ``null``, ``socket``, ``udp``, ``msmouse``,
+    Backend is one of: ``null``, ``socket``, ``udp``, ``msmouse``, ``mux``,
     ``vc``, ``ringbuf``, ``file``, ``pipe``, ``console``, ``serial``,
     ``pty``, ``stdio``, ``braille``, ``parallel``,
     ``spicevmc``, ``spiceport``. The specific backend will determine the
@@ -3839,9 +3839,9 @@ The general form of a character device option is:
     the QEMU monitor, and ``-nographic`` also multiplexes the console
     and the monitor to stdio.
 
-    There is currently no support for multiplexing in the other
-    direction (where a single QEMU front end takes input and output from
-    multiple chardevs).
+    If you need to multiplex in the opposite direction (where one QEMU
+    interface receives input and output from multiple chardev devices),
+    please refer to the paragraph below regarding chardev mux configuration.
 
     Every backend supports the ``logfile`` option, which supplies the
     path to a file to record all data transmitted via the backend. The
@@ -3941,6 +3941,42 @@ The available backends are:
     Forward QEMU's emulated msmouse events to the guest. ``msmouse``
     does not take any options.
 
+``-chardev mux,id=id,chardev=chardev-id[,,chardev-idN]``
+    Explicitly create chardev multiplexer with possibility to multiplex
+    in the opposite direction, where one QEMU interface (frontend device)
+    receives input and output from multiple chardev backend devices.
+
+    For example the following is a use case of 2 backend devices: text
+    virtual console ``vc0`` and a socket ``sock0`` connected
+    to a single virtio hvc console frontend device with multiplexer
+    ``mux0`` help. Virtual console renders text to an image, which
+    can be shared over the VNC protocol, in turn socket backend provides
+    biderectional communication to the virtio hvc console over socket.
+    The example configuration can be the following:
+
+    ::
+
+        -chardev socket,path=/tmp/sock,server=on,wait=off,id=sock0 \
+        -chardev vc,id=vc0 \
+        -chardev mux,id=mux0,chardev=vc0,,sock0 \
+        -device virtconsole,chardev=mux0 \
+        -vnc 0.0.0.0:0
+
+    Once QEMU starts VNC client and any TTY emulator can be used to
+    control a single hvc console:
+
+    ::
+
+       # VNC client
+       vncviewer :0
+
+       # TTY emulator
+       socat unix:connect:/tmp/sock pty,link=/tmp/pty & \
+       tio /tmp/pty
+
+    Multiplexing of several backend devices with serveral frontend devices
+    is not supported.
+
 ``-chardev vc,id=id[[,width=width][,height=height]][[,cols=cols][,rows=rows]]``
     Connect to a QEMU text console. ``vc`` may optionally be given a
     specific size.
-- 
2.34.1