Re: [Xen-devel] [PATCH 00/25 v7] SBSA UART emulation support in Xen

[Julien Grall <julien.grall@xxxxxxx>]

On 10/08/17 17:38, Wei Liu wrote:
> On Thu, Aug 10, 2017 at 05:11:52PM +0100, Julien Grall wrote:
> > On 10/08/17 17:00, Wei Liu wrote:
> > > On Thu, Aug 10, 2017 at 03:26:07PM +0100, Julien Grall wrote:
> > > > On 09/08/17 11:58, Bhupinder Thakur wrote:
> > > > > Hi Julien,
> > > > Hi Bhupinder,
> > > >
> > > > > Thanks for the testing.
> > > > >
> > > > > On 8 August 2017 at 21:29, Julien Grall <julien.grall@xxxxxxx> wrote:
> > > > > > Hi Bhupinder,
> > > > > >
> > > > > > I gave another and I have a couple of comments.
> > > > > >
> > > > > > Booting Linux with earlycon enabled take quite a while. I can see the
> > > > > > characters coming slower than on the minitel. It seems to be a bit better
> > > > > > after switching off the bootconsole. Overall Linux is taking ~20 times to
> > > > > > boot with pl011 vs HVC console.
> > > > > >
> > > > > > I do agree that pl011 is emulated and therefore you have to trap after each
> > > > > > character. But 20 times sounds far too much.
> > > > > I think this slowness could be due to ratelimiting of the pl011 events
> > > > > in xenconosle. Currently, the rate limit is
> > > > > set to 30 events per 200 msecs (see RATE_LIMIT_ALLOWANCE/RATE_LIMIT_PERIOD).
> > > > >
> > > > > I increased the rate limit to 600 events (30 * 20) per 200 msecs. With
> > > > > this change,
> > > > > I see that the the find command is running faster and smoother.
> > > > > Earlier the find output would be jerky.
> > > > I think there might be another solution avoiding increasing the rate limit.
> > > >
> > > > If you look at the earlycon code for pl011 in Linux:
> > > >
> > > > static void pl011_putc(struct uart_port *port, int c)
> > > > {
> > > >         while (readl(port->membase + UART01x_FR) & UART01x_FR_TXFF)
> > > >                 cpu_relax();
> > > >         if (port->iotype == UPIO_MEM32)
> > > >                 writel(c, port->membase + UART01x_DR);
> > > >         else
> > > >                 writeb(c, port->membase + UART01x_DR);
> > > >         while (readl(port->membase + UART01x_FR) & UART01x_FR_BUSY)
> > > >                 cpu_relax();
> > > > }
> > > >
> > > > Linux will wait the UART to be idle before sending a new character.
> > > >
> > > > Now looking at vpl011 emulation, the busy bit set when a new character is
> > > > queued (see vpl011_write_data). This bit will only be cleared when the
> > > > console daemon will raise an event and the queue is empty (see
> > > > vpl011_data_avail).
> > > >
> > > > This means for earlycon, you will need a round trip Guest -> Xen -> Dom0 ->
> > > > Xen -> Guest for each single character. This is a bit counterproductive and
> > > > combined with the limit it makes it worse.
> > > >
> > > > I would take a different approach on the BUSY bit. We can consider the queue
> > > > between Xen and xenconsoled as outside of the UART. If the character is
> > > > queued, then job done. I think this would improve quite a lot of the
> > > > performance.
> > > Yes. This.
> > >
> > > The guest sees a register, which is essentially a synchronous interface
> > > to the guest. The current code, as you already see, will issue one event
> > > for every character. That's excessive.
> > I am actually not suggesting to modify that at the moment. I think you may
> > have other trouble with the interaction between the user and th console by
> > doing that. Imagine you want to print the prompt, it may lag a bit before
> > getting it.
> >
> > The only thing I suggest is to not set the BUSY bit in the UART everytime a
> > character is queued.
> Did you come to that conclusion that this would work by looking at the
> spec or Linux source code? I think it should conform to the spec, not a
> specific guest. But you're the maintainer, you have the final say.
I read both the spec and the code. From the spec:

"UART busy. If this bit is set to 1, the UART is busy transmitting data. 
This bit remains set until the
complete byte, including all the stop bits, has been sent from the shift 
register.
This bit is set as soon as the transmit FIFO becomes non-empty, 
regardless of whether the UART is
enabled or not."

Currently, we considered that the shared ring is the FIFO of the UART. 
Meaning that the BUSY bit is set until xenconsoled read everything.
I don't think implementing a FIFO is highly critical in an emulation 
(QEMU does not implement it for instance). And definitely using the 
shared ring brings slow down (involve multiple context switch).
I would suggest to take a different approach where the BUSY is only set 
if we can't add more data in the shared ring. This would be clear as 
soon as the ring has space.
If we really we could implement is small FIFO (the SBSA requested a 
least 32-entry separate for transmit and receive). But I don't think 
this is critically for a first approach.
> > > The interface between Xen and xenconsoled can be asynchronous, it can
> > > opt to queue X characters before sending an event, also setup a oneshot
> > > timer to avoid hanging.
> > >
> > > This however has some other implications -- it might not be as reliable
> > > as the original method because data is not guaranteed to hit backend. If
> > > the guest crashes very early on, depending the actual implementation you
> > > might not be able get the data.
> > Would it be possible to ask xenconsoled to dump everything on domain crash?
> > Some kind of synchronization.
> No, not at the moment. If the data is still in Xen and destroyed,
> xenconsoled can't do anything.
The vUART emulation is directly queuing the data, there are no 
intermediate buffer. So all the data would be in the shared ring 
available for xenconsoled to go through.
It would be quite a useful enhancement for when the guest crash.

Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel