[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.