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

[Julien Grall <julien.grall@xxxxxxx>]

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().
The new logic looks better to me, few comments below.

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

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

>   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