Hi Julien,
I'll remove the DOM prefix for the input domain, relying to the
"Switching..." message as you suggested.
Cheers,
Stefano
On Wed, 22 Aug 2018, Julien Grall wrote:
> Hi,
>
> On 16/08/18 20:41, Stefano Stabellini wrote:
> > 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 <stefa...@xilinx.com>
> > > > ---
> > > > 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.
>
> How come this is impossible? DOM1 may have print very late and therefore you
> have time to switch to DOM1 before any print.
>
> > 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.
>
> I don't like the idea that in some case DOM%u: is not printed in front of the
> line. This is making more difficult to read the logs.
>
> >
> >
> > > > + } 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?
>
> Looks easier to read.
>
> >
> >
> > > 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.
>
> Well it is convenient if you manage to put "DOM" string in front. From a look
> at your implementation and your own comment this is not always the case.
>
> I am also quite surprised that this does not make VIM (or any other editor)
> more difficult to use as "DOM:" is printed in front of each line.
>
> >
> >
> > > 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?
>
> Let's take an example:
> 1) DOM1 writes "\nab"
> 2) DOM2 writes "Foobar\n"
> 3) DOM1 write "cde"
>
> The output would be:
>
> DOM1: ab DOM2: Foobar
> cde
>
> DOM1 and DOM2 has the line combined (not a big deal). However, the next few
> characters for DOM1 "cde" will not be prefixed with "DOM1:".
>
> Cheers,
>
> --
> Julien Grall
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
