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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Thu, Aug 10, 2017 at 06:51:24PM +0100, Julien Grall wrote:
> 
> 
> 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.
> 

Sure, I think this is reasonable.

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

What I meant was actually "If the data is still in the ring". There is
no synchronisation between Xen and xenconsoled to let the latter pull
out all data before destroying the guest.

As it stands, if we go with the fully-synchronised approach for now,
that won't be a problem. But when you want to fiddle with the BUSY bit
or any other optimisation -- leaving more data in the ring -- that might
cause data loss.

I'm not against bumping the rate-limit, but I want a justification and
want to consider as many approaches as possible. Ultimately the final
decision is up to you and Stefano.

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