Re: [Xen-devel] [PATCH v3 24/25] xen/vpl011: buffer out chars when the backend is xen

[Julien Grall <julien.grall@xxxxxxx>]

Hi,

On 16/08/18 20:41, Stefano Stabellini wrote:
> On Mon, 13 Aug 2018, Julien Grall wrote:
> > Hi,
> >
> > On 01/08/18 00:28, Stefano Stabellini wrote:
> > > To avoid mixing the output of different domains on the console, buffer
> > > the output chars and print line by line. Unless the domain has input
> > > from the serial, in which case we want to print char by char for a
> > > smooth user experience.
> > >
> > > The size of SBSA_UART_OUT_BUF_SIZE is arbitrary. 90 should be large
> > > enough to accommodate the length of most lines of output (typically they
> > > are limited to 80 characters on Unix systems), plus one extra char for
> > > the string terminator.
> > How about using the same value as vuart (e.g VUART_BUT_SIZE) instead? So we
> > buffer the same way for the vpl011?
> Yes, I can do that.
>
>
> > > Signed-off-by: Stefano Stabellini <stefanos@xxxxxxxxxx>
> > > ---
> > >    xen/arch/arm/vpl011.c        | 21 ++++++++++++++++++---
> > >    xen/include/asm-arm/vpl011.h |  3 +++
> > >    2 files changed, 21 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
> > > index f206c61..8137371 100644
> > > --- a/xen/arch/arm/vpl011.c
> > > +++ b/xen/arch/arm/vpl011.c
> > > @@ -28,6 +28,7 @@
> > >    #include <xen/lib.h>
> > >    #include <xen/mm.h>
> > >    #include <xen/sched.h>
> > > +#include <xen/console.h>
> > >    #include <public/domctl.h>
> > >    #include <public/io/console.h>
> > >    #include <asm/pl011-uart.h>
> > > @@ -85,12 +86,26 @@ static void vpl011_write_data_xen(struct domain *d,
> > > uint8_t data)
> > >    {
> > >        unsigned long flags;
> > >        struct vpl011 *vpl011 = &d->arch.vpl011;
> > > +    struct vpl011_xen_backend *intf = vpl011->xen;
> > > +    struct domain *input = console_input_domain();
> > >          VPL011_LOCK(d, flags);
> > >    -    printk("%c", data);
> > > -    if (data == '\n')
> > > -        printk("DOM%u: ", d->domain_id);
> > > +    intf->out[intf->out_prod++] = data;
> > > +    if ( d == input && intf->out_prod == 1 )
> > > +    {
> > > +        printk("%c", data);
> > > +        if ( data == '\n' )
> > > +            printk("DOM%u: ", d->domain_id);
> > > +        intf->out_prod = 0;
> > See my remark on the patch implementing vpl011_write_data_xen.
> With this patch, the missing "DOM" at the beginning cannot happen
> anymore due to the out buffering. Theoretically it can still happen by
> switching to DOM1 before DOM1 prints anything, but it is impossible to
> do in practice.
How come this is impossible? DOM1 may have print very late and therefore 
you have time to switch to DOM1 before any print.
> Even in this theoretical scenario, the user would still
> get the useful "Switching to DOM1" message, that would help clarify what
> is going on. Thus, my preference is to avoid making the code more
> complex to fix this issue.
I don't like the idea that in some case DOM%u: is not printed in front 
of the line. This is making more difficult to read the logs.
> > > +    } else if ( d == input ||
> > Coding style:
> >
> > }
> > else if
> > {
> I'll fix
>
>
> > > +                intf->out_prod == SBSA_UART_OUT_BUF_SIZE - 1 ||
> > > +                data == '\n' )
> > > +    {
> > > +        intf->out[intf->out_prod++] = '\0';
> > > +        printk("DOM%u: %s", d->domain_id, intf->out);
> > > +        intf->out_prod = 0;
> > > +    }
> > The code is quite difficult to read. It would be easier to differentiate
> > (domain == input vs domain != input) even if it means a bit of duplication.
> OK, I can rearrange the code that way. For example:
>
>      if ( d == input ){
>          if ( intf->out_prod == 1 )
>          {
>              printk("%c", data);
>              if ( data == '\n' )
>                  printk("DOM%u: ", d->domain_id);
>              intf->out_prod = 0;
>          }
>          else
>          {
>              if ( data != '\n' )
>                  intf->out[intf->out_prod++] = '\n';
>              intf->out[intf->out_prod++] = '\0';
>              printk("DOM%u: %s", d->domain_id, intf->out);
>              intf->out_prod = 0;
>          }
>      }
>      else
>      {
>          if ( intf->out_prod == SBSA_UART_OUT_BUF_SIZE - 2 ||
>               data == '\n' )
>          {
>              if ( data != '\n' )
>                  intf->out[intf->out_prod++] = '\n';
>              intf->out[intf->out_prod++] = '\0';
>              printk("DOM%u: %s", d->domain_id, intf->out);
>              intf->out_prod = 0;
>          }
>      }
>
> Is it better?
Looks easier to read.

> > You also don't handle all the cases.
> >
> > For the input domain, I don't think you want to print the domain in front.
> > Instead I would rely on the "Switch to ...".
> Actually it is very convenient to know at any given time which domain
> you are talking to. I couldn't find any problems with the prefix, even
> using VIM, etc. I would rather keep the "DOM" string around.
Well it is convenient if you manage to put "DOM" string in front. From a 
look at your implementation and your own comment this is not always the 
case.
I am also quite surprised that this does not make VIM (or any other 
editor) more difficult to use as "DOM:" is printed in front of each line.
> > This would avoid the problem
> > where DomB needs to have his line printed while it is not the console input.
> > If this happens in the middle of DomA, then you are loosing track what's going
> > on.
> I don't understand the example: if DOM1 has input, and DOM2 prints
> something, the DOM2 output will be prepended by "DOM2", avoiding any
> confusion. What am I missing?
Let's take an example:
        1) DOM1 writes "\nab"
        2) DOM2 writes "Foobar\n"
        3) DOM1 write "cde"

The output would be:

DOM1: ab DOM2: Foobar
cde

DOM1 and DOM2 has the line combined (not a big deal). However, the next 
few characters for DOM1 "cde" will not be prefixed with "DOM1:".
Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel