Hi
On Tue, Sep 17, 2024 at 6:17 PM Roman Penyaev <r.peni...@gmail.com> wrote:
> Hi Marc-André,
>
> On Tue, Sep 17, 2024 at 2:31 PM Marc-André Lureau
> <marcandre.lur...@redhat.com> wrote:
> >
> > Hi Roman
> >
> > On Fri, Sep 13, 2024 at 8:37 PM Roman Penyaev <r.peni...@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.peni...@gmail.com>
> > > Cc: "Marc-André Lureau" <marcandre.lur...@redhat.com>
> > > Cc: Paolo Bonzini <pbonz...@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
