Re: [patch 0/3] kvm tool: Serial emulation overhaul

[Thomas Gleixner <tglx@linutronix.de>]

On Sat, 10 Dec 2011, Pekka Enberg wrote:

> On Sat, 10 Dec 2011, Thomas Gleixner wrote:
> > We observed massive hangs of the serial console in kvm tool which
> > turned out to be caused by the bogus emulation of the interrupt line
> > behaviour.
> > 
> > This series cleans up and simplifies the serial code and in the last
> > step fixes the interrupt handling proper.
> 
> Applied, thanks Thomas!

There is a slight problem with them. I only tested them with the
rt-guest kernels, but non rt kernels are slightly unhappy. Sorry for
not thinking about that.

Fix below.

Thanks,

	tglx
------------------>
Subject: kvm tools: serial: Make it work with non rt guests as well
From: Thomas Gleixner <tglx@linutronix.de>
Date: Sat, 10 Dec 2011 21:27:26 +0100

Sasha reported, that a non RT guest reports "too much work for irq 4"
with the previous serial overhaul.

The reason is, that the new code allows unlimited tx transfers, which
triggers the sanity check in the 8250.c interrupt handler.

Limit the consecutive TX chars to 16 and let the guest kernel escape
from the 8250 interrupt handler. Set the TEMT/THRE bits in the
periodic serial console update.

Reported-by: Sasha Levin  <levinsasha928@gmail.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 tools/kvm/hw/serial.c |   30 ++++++++++++++++++++++--------
 1 file changed, 22 insertions(+), 8 deletions(-)

Index: linux-kvm/tools/kvm/hw/serial.c
===================================================================
--- linux-kvm.orig/tools/kvm/hw/serial.c
+++ linux-kvm/tools/kvm/hw/serial.c
@@ -19,6 +19,7 @@ struct serial8250_device {
 	u16			iobase;
 	u8			irq;
 	u8			irq_state;
+	int			txcnt;
 
 	u8			rbr;		/* receive buffer */
 	u8			dll;
@@ -105,6 +106,16 @@ static void serial8250_update_irq(struct
 			kvm__irq_line(kvm, dev->irq, 1);
 	}
 	dev->irq_state = iir;
+
+	/*
+	 * If the kernel disabled the tx interrupt, we know that there
+	 * is nothing more to transmit, so we can reset our tx logic
+	 * here.
+	 */
+	if (!(dev->ier & UART_IER_THRI)) {
+		dev->lsr |= UART_LSR_TEMT | UART_LSR_THRE;
+		dev->txcnt = 0;
+	}
 }
 
 #define SYSRQ_PENDING_NONE		0
@@ -134,6 +145,15 @@ static void serial8250__receive(struct k
 {
 	int c;
 
+	/*
+	 * If the guest transmitted 16 chars in a row, we clear the
+	 * TEMT/THRE bits to let the kernel escape from the 8250
+	 * interrupt handler. We come here only once a ms, so that
+	 * should give the kernel the desired pause.
+	 */
+	dev->lsr |= UART_LSR_TEMT | UART_LSR_THRE;
+	dev->txcnt = 0;
+
 	if (dev->lsr & UART_LSR_DR)
 		return;
 
@@ -212,14 +232,8 @@ static bool serial8250_out(struct ioport
 				term_putc(CONSOLE_8250, addr, size, dev->id);
 			/* else FIXME: Inject data into rcv path for LOOP */
 
-			/*
-			 * Set transmitter and transmit hold register
-			 * empty.  We have no FIFO at the moment and
-			 * on the TX side it's only interesting, when
-			 * we could coalesce port io on the kernel
-			 * kernel.
-			 */
-			dev->lsr |= UART_LSR_TEMT | UART_LSR_THRE;
+			if (++dev->txcnt == 16)
+				dev->lsr &= ~(UART_LSR_TEMT | UART_LSR_THRE);
 			break;
 		} else {
 			dev->dll = ioport__read8(data);