|
[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 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 <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.
> >
> > 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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |