Re: [Xen-devel] [PATCH v4 21/23] xen/vpl011: buffer out chars when the backend is xen

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Mon, 15 Oct 2018, Julien Grall wrote:
> Hi,
> 
> On 05/10/2018 19:47, 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, choose the same size
> > as VUART_BUT_SIZE used in vuart.c.
> > 
> > Export a function named console_input_domain() to allow others to know
> > which domains has input at a given time.
> > 
> > Signed-off-by: Stefano Stabellini <stefanos@xxxxxxxxxx>
> > CC: andrew.cooper3@xxxxxxxxxx
> > CC: George.Dunlap@xxxxxxxxxxxxx
> > CC: ian.jackson@xxxxxxxxxxxxx
> > CC: jbeulich@xxxxxxxx
> > CC: konrad.wilk@xxxxxxxxxx
> > CC: tim@xxxxxxx
> > CC: wei.liu2@xxxxxxxxxx
> > ---
> > XXX: merge this patch with "xen/arm: Allow vpl011 to be used by DomU" on
> >       commit
> 
> That's not going to make the series bisectable as it depends on an
> intermediate patch for console_input_domain().

A tiny patch reordering will fix this. I'll do.


> The new logic looks better to me, few comments below.

Good!


> > 
> > Changes in v4:
> > - make SBSA_UART_OUT_BUF_SIZE the same size of VUART_BUT_SIZE
> > - rearrange the code to be clearer input and != input cases
> > - code style
> > - add \n when printing the out buffer because is full
> > - don't print prefix for input domain
> > ---
> >   xen/arch/arm/vpl011.c        | 35 ++++++++++++++++++++++++++++++++---
> >   xen/drivers/char/console.c   |  7 +++++++
> >   xen/include/asm-arm/vpl011.h |  4 ++++
> >   xen/include/xen/console.h    |  2 ++
> >   4 files changed, 45 insertions(+), 3 deletions(-)
> > 
> > diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
> > index 131507e..5e57ada 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,40 @@ 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->backend.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 )
> > +    {
> > +        if ( intf->out_prod == 1 )
> > +        {
> > +            printk("%c", data);
> > +            intf->out_prod = 0;
> > +        }
> > +        else
> > +        {
> > +            if ( data != '\n' )
> > +                intf->out[intf->out_prod++] = '\n';
> > +            intf->out[intf->out_prod++] = '\0';
> > +            printk("%s", 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;
> > +        }
> > +    }
> >         vpl011->uartris |= TXI;
> >       vpl011->uartfr &= ~TXFE;
> > diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
> > index 0808bac..9a2b29a 100644
> > --- a/xen/drivers/char/console.c
> > +++ b/xen/drivers/char/console.c
> > @@ -406,6 +406,13 @@ static void dump_console_ring_key(unsigned char key)
> >    */
> >   static unsigned int __read_mostly console_rx = 0;
> >   +struct domain *console_input_domain(void)
> > +{
> > +    if ( console_rx == 0 )
> > +            return NULL;
> > +    return get_domain_by_id(console_rx - 1);
> > +}
> > +
> >   static void switch_serial_input(void)
> >   {
> >       if ( console_rx++ == max_init_domid + 1 )
> > diff --git a/xen/include/asm-arm/vpl011.h b/xen/include/asm-arm/vpl011.h
> > index 5eb6d25..ab6fd79 100644
> > --- a/xen/include/asm-arm/vpl011.h
> > +++ b/xen/include/asm-arm/vpl011.h
> > @@ -30,9 +30,13 @@
> >   #define VPL011_UNLOCK(d,flags)
> > spin_unlock_irqrestore(&(d)->arch.vpl011.lock, flags)
> >     #define SBSA_UART_FIFO_SIZE 32
> > +/* Same size as VUART_BUT_SIZE, used in vuart.c */
> 
> s/BUT/BUF/

I'll fix


> > +#define SBSA_UART_OUT_BUF_SIZE 128
> 
> You could directly use VUART_BUF_SIZE here to avoid the comment above.

That is true, but I think it is nicer to keep the two #define separate


> >   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 {
> > diff --git a/xen/include/xen/console.h b/xen/include/xen/console.h
> > index 70c9911..c5a85c8 100644
> > --- a/xen/include/xen/console.h
> > +++ b/xen/include/xen/console.h
> > @@ -31,6 +31,8 @@ void console_end_sync(void);
> >   void console_start_log_everything(void);
> >   void console_end_log_everything(void);
> >   +struct domain *console_input_domain(void);
> > +
> >   /*
> >    * Steal output from the console. Returns +ve identifier, else -ve error.
> >    * Takes the handle of the serial line to steal, and steal callback
> > function.
> > 
> 
> -- 
> Julien Grall
> 

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