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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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. 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.


> > +    } 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?


> 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.


> 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?


> Also if you have the buffer full, you will write the line without a newline.
> So, the next line is going to be wrong. In that case, you need to add a
> newline before printed. I think you want to look at what we did in
> vuart_print_char.

Yes, this is a problem. I'll do the same as vuart_print_char.

 
> >         vpl011->uartris |= TXI;
> >       vpl011->uartfr &= ~TXFE;
> > diff --git a/xen/include/asm-arm/vpl011.h b/xen/include/asm-arm/vpl011.h
> > index c9918c1..30487e8 100644
> > --- a/xen/include/asm-arm/vpl011.h
> > +++ b/xen/include/asm-arm/vpl011.h
> > @@ -30,9 +30,12 @@
> >   #define VPL011_UNLOCK(d,flags)
> > spin_unlock_irqrestore(&(d)->arch.vpl011.lock, flags)
> >     #define SBSA_UART_FIFO_SIZE 32
> > +#define SBSA_UART_OUT_BUF_SIZE 91
> >   struct vpl011_xen_backend {
> >       char in[SBSA_UART_FIFO_SIZE];
> > +    char out[SBSA_UART_OUT_BUF_SIZE];
> >       XENCONS_RING_IDX in_cons, in_prod;
> > +    XENCONS_RING_IDX out_prod;
> >   };
> >     struct vpl011 {
> > 
> 
> Cheers,
> 
> -- 
> Julien Grall
>

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