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

[Julien Grall <julien.grall@xxxxxxx>]

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

> +    } else if ( d == input ||
Coding style:

}
else if
{

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