[Qemu-devel] sane char device writes?

[Qemu-devel] sane char device writes?

[Michal Suchánek]

Hello,

I have reported the issue with qemu aborting in spapr_vty.c because
gtk.c submitted more data than can be sent to the emulated serial port.

While the abort has been resolved and spapr_vty.c should truncate the
data now getting the data through is still not possible.

Looking in the code I see that console.c has this code (which is only
piece of code in UI corresponding the the gtk part I found):

static void kbd_send_chars(void *opaque)
{
    QemuConsole *s = opaque;
    int len;
    uint8_t buf[16];

    len = qemu_chr_be_can_write(s->chr);
    if (len > s->out_fifo.count)
        len = s->out_fifo.count;
    if (len > 0) {
        if (len > sizeof(buf))
            len = sizeof(buf);
        qemu_fifo_read(&s->out_fifo, buf, len);
        qemu_chr_be_write(s->chr, buf, len);
    }
    /* characters are pending: we send them a bit later (XXX:
       horrible, should change char device API) */
    if (s->out_fifo.count > 0) {
        timer_mod(s->kbd_timer, qemu_clock_get_ms(QEMU_CLOCK_REALTIME)
    + 1); }
}

The corresponding piece of code in gtk.c is AFAICT

static gboolean  (VteTerminal *terminal, gchar *text, guint size,
                         gpointer user_data)
{
    VirtualConsole *vc = user_data;

    if (vc->vte.echo) {
        VteTerminal *term = VTE_TERMINAL(vc->vte.terminal);
        int i;
        for (i = 0; i < size; i++) {
            uint8_t c = text[i];
            if (c >= 128 || isprint(c)) {
                /* 8-bit characters are considered printable.  */
                vte_terminal_feed(term, &text[i], 1);
            } else if (c == '\r' || c == '\n') {
                vte_terminal_feed(term, "\r\n", 2);
            } else {
                char ctrl[2] = { '^', 0};
                ctrl[1] = text[i] ^ 64;
                vte_terminal_feed(term, ctrl, 2);
            }
        }
    }

    qemu_chr_be_write(vc->vte.chr, (uint8_t  *)text, (unsigned
    int)size); return TRUE;
}

meaning there is no loop to split the submitted text buffer.

gd_vc_in is VTE callback handling input so I suspect it either handles
it or not and it cannot say it handled only part of the "commit" event.

So for this to work an extra buffer would have to be stored in gtk.c
somewhere, and possibly similar timer trick used as in console.c

Any ideas how to do this without introducing too much insanity?

Presumably using a GTK timer for repeating gd_vc_in the handler would
run in the same GTK UI thread as the "commit" signal handler and
excessive locking would not be required.

The data passed to gd_vc_in is presumably freed when it ends so it
would have to be copied somewhere. It's quite possible to create a
static list in gd_vc_in or some extra field in VirtualConsole.

Thanks

Michal

Re: [Qemu-devel] sane char device writes?

[Thomas Huth]

On 23.11.2016 19:09, Michal Suchánek wrote:
> Hello,
> 
> I have reported the issue with qemu aborting in spapr_vty.c because
> gtk.c submitted more data than can be sent to the emulated serial port.
> 
> While the abort has been resolved and spapr_vty.c should truncate the
> data now getting the data through is still not possible.
> 
> Looking in the code I see that console.c has this code (which is only
> piece of code in UI corresponding the the gtk part I found):
> 
> static void kbd_send_chars(void *opaque)
> {
>     QemuConsole *s = opaque;
>     int len;
>     uint8_t buf[16];
> 
>     len = qemu_chr_be_can_write(s->chr);
>     if (len > s->out_fifo.count)
>         len = s->out_fifo.count;
>     if (len > 0) {
>         if (len > sizeof(buf))
>             len = sizeof(buf);
>         qemu_fifo_read(&s->out_fifo, buf, len);
>         qemu_chr_be_write(s->chr, buf, len);
>     }
>     /* characters are pending: we send them a bit later (XXX:
>        horrible, should change char device API) */
>     if (s->out_fifo.count > 0) {
>         timer_mod(s->kbd_timer, qemu_clock_get_ms(QEMU_CLOCK_REALTIME)
>     + 1); }
> }
> 
> The corresponding piece of code in gtk.c is AFAICT
> 
> static gboolean  (VteTerminal *terminal, gchar *text, guint size,
>                          gpointer user_data)
> {
>     VirtualConsole *vc = user_data;
> 
>     if (vc->vte.echo) {
>         VteTerminal *term = VTE_TERMINAL(vc->vte.terminal);
>         int i;
>         for (i = 0; i < size; i++) {
>             uint8_t c = text[i];
>             if (c >= 128 || isprint(c)) {
>                 /* 8-bit characters are considered printable.  */
>                 vte_terminal_feed(term, &text[i], 1);
>             } else if (c == '\r' || c == '\n') {
>                 vte_terminal_feed(term, "\r\n", 2);
>             } else {
>                 char ctrl[2] = { '^', 0};
>                 ctrl[1] = text[i] ^ 64;
>                 vte_terminal_feed(term, ctrl, 2);
>             }
>         }
>     }
> 
>     qemu_chr_be_write(vc->vte.chr, (uint8_t  *)text, (unsigned
>     int)size); return TRUE;
> }
> 
> meaning there is no loop to split the submitted text buffer.
> 
> gd_vc_in is VTE callback handling input so I suspect it either handles
> it or not and it cannot say it handled only part of the "commit" event.
> 
> So for this to work an extra buffer would have to be stored in gtk.c
> somewhere, and possibly similar timer trick used as in console.c
> 
> Any ideas how to do this without introducing too much insanity?
>
> Presumably using a GTK timer for repeating gd_vc_in the handler would
> run in the same GTK UI thread as the "commit" signal handler and
> excessive locking would not be required.
> 
> The data passed to gd_vc_in is presumably freed when it ends so it
> would have to be copied somewhere. It's quite possible to create a
> static list in gd_vc_in or some extra field in VirtualConsole.

Not sure how the best solution should really look like, but Paolo
suggested something here:

http://lists.gnu.org/archive/html/qemu-devel/2016-11/msg02222.html

... so I'm putting him on CC: ... maybe he's got some spare minutes to
elaborate on his idea.

 Thomas

Re: [Qemu-devel] sane char device writes?

[Paolo Bonzini]

On 24/11/2016 08:51, Thomas Huth wrote:
> > So for this to work an extra buffer would have to be stored in gtk.c
> > somewhere, and possibly similar timer trick used as in console.c
> > 
> > Any ideas how to do this without introducing too much insanity?
> >
> > Presumably using a GTK timer for repeating gd_vc_in the handler would
> > run in the same GTK UI thread as the "commit" signal handler and
> > excessive locking would not be required.
> > 
> > The data passed to gd_vc_in is presumably freed when it ends so it
> > would have to be copied somewhere. It's quite possible to create a
> > static list in gd_vc_in or some extra field in VirtualConsole.
>
> Not sure how the best solution should really look like, but Paolo
> suggested something here:
> 
> http://lists.gnu.org/archive/html/qemu-devel/2016-11/msg02222.html
> 
> ... so I'm putting him on CC: ... maybe he's got some spare minutes to
> elaborate on his idea.

My idea looks very much like Michal's.  I hadn't gone very much beyond
the "you need a buffer" step, but anyway you don't need a timer---you
can just record a chr_accept_input callback in gd_vc_handler.  It will
be called when the front-end is ready to get more characters.

Thanks,

Paolo

Re: [Qemu-devel] sane char device writes?

[Michal Suchánek]

Hello,

On Fri, 25 Nov 2016 17:16:39 +0100
Paolo Bonzini <pbonzini@redhat.com> wrote:

> On 24/11/2016 08:51, Thomas Huth wrote:
> > > So for this to work an extra buffer would have to be stored in
> > > gtk.c somewhere, and possibly similar timer trick used as in
> > > console.c
> > > 
> > > Any ideas how to do this without introducing too much insanity?
> > >
> > > Presumably using a GTK timer for repeating gd_vc_in the handler
> > > would run in the same GTK UI thread as the "commit" signal
> > > handler and excessive locking would not be required.
> > > 
> > > The data passed to gd_vc_in is presumably freed when it ends so it
> > > would have to be copied somewhere. It's quite possible to create a
> > > static list in gd_vc_in or some extra field in VirtualConsole.  
> >
> > Not sure how the best solution should really look like, but Paolo
> > suggested something here:
> > 
> > http://lists.gnu.org/archive/html/qemu-devel/2016-11/msg02222.html
> > 
> > ... so I'm putting him on CC: ... maybe he's got some spare minutes
> > to elaborate on his idea.  
> 
> My idea looks very much like Michal's.  I hadn't gone very much beyond
> the "you need a buffer" step, but anyway you don't need a timer---you
> can just record a chr_accept_input callback in gd_vc_handler.  It will
> be called when the front-end is ready to get more characters.
> 
Where will the characters come from, though? 

This might solve the console ui hack since it has a pipe buffer it can
peek and read in pieces but the gtk ui gets a whole paste buffer in an
event callback and has to handle it whole before it returns from the
gtk event callback. Unless it stores the data that does not fit into
the serial fifo somewhere it has to drop it on the floor or block.

Thanks

Michal

Re: [Qemu-devel] sane char device writes?

[Paolo Bonzini]

On 28/11/2016 16:53, Michal Suchánek wrote:
>> > 
>> > My idea looks very much like Michal's.  I hadn't gone very much beyond
>> > the "you need a buffer" step, but anyway you don't need a timer---you
>> > can just record a chr_accept_input callback in gd_vc_handler.  It will
>> > be called when the front-end is ready to get more characters.
> 
> Where will the characters come from, though? 

>From ui/gtk.c's own buffer.

> This might solve the console ui hack since it has a pipe buffer it can
> peek and read in pieces but the gtk ui gets a whole paste buffer in an
> event callback and has to handle it whole before it returns from the
> gtk event callback. Unless it stores the data that does not fit into
> the serial fifo somewhere it has to drop it on the floor or block.

Yes, gtk needs to have a buffer of its own (probably it should
dynamically allocate it too, so that it can grow and shrink and the
limit on the size can be higher).

Paolo