Re: [Xen-staging] [xen-unstable] [QEMU-DM] Upgrade emulated UART to 16550A.

Re: [Xen-staging] [xen-unstable] [QEMU-DM] Upgrade emulated UART to 16550A.

[Alex Williamson]

On Wed, 2007-12-05 at 14:22 +0000, Xen staging patchbot-unstable wrote:
> # HG changeset patch
> # User Keir Fraser
> # Date 1196864460 0
> # Node ID f0ac46de680cc6fe8c91699fdda153b125ae515c
> # Parent  bf21e00155b7dd76653c5340099ecedac7a7de08
> [QEMU-DM] Upgrade emulated UART to 16550A.
> 
> This patch adds 16550 emulation to qemu-dm. I still consider it a work
> in progress, but from my testing, it consistently performs better than
> the old code already, sometimes considerably so (on my laptop, dumping
> out data over serial from a HVM DomU -> pty in Dom0 was up to 5.3
> times faster than with the old 16450 code). I can consistenly reach
> full 115200 baud speeds over physical serial lines when the virtual
> device is backed by a phys port, which I never could with the old code.
> 
> There are still some things to be done. I want to add proper error
> handling, so that overflow/framing & parity errors on the physical
> port get detected and reported by the virtual port. Also, now that
> FIFOs are in place, I believe performance could be improved
> significantly by putting some code into the hypervisor so that reads &
> writes from the FIFOs don't have to exit into qemu-dm at all. I'm also
> reading up on the specs for newer uarts with deeper FIFOs.

   I notice with this patch that my HVM domain, which specifies
serial=pty in the config file, no longer boots without me attaching to
the console via xm.  Previously it would boot regardless of whether I
was attached to the console.  Is this change in behavior intentional?
Note that I'm testing on ia64 where the HVM firmware spews lots of debug
info out onto the serial port.  Maybe this new UART model isn't dropping
transmit chars when there's no client connected as it did before?
Thanks,

	Alex

-- 
Alex Williamson                             HP Open Source & Linux Org.

Re: [Xen-staging] [xen-unstable] [QEMU-DM] Upgrade emulated UART to 16550A.

[Trolle Selander]

[-- Attachment #1.1: Type: text/plain, Size: 2228 bytes --]

As you describe this, it does occur to me as you describe this that
serial_xmit() never gives up on trying to retransmit characters out of the
FIFO, which isn't very good for backends like pty's or pipes. I'll fix it
asap.

On Dec 5, 2007 9:15 PM, Alex Williamson <alex.williamson@hp.com> wrote:

>
> On Wed, 2007-12-05 at 14:22 +0000, Xen staging patchbot-unstable wrote:
> > # HG changeset patch
> > # User Keir Fraser
> > # Date 1196864460 0
> > # Node ID f0ac46de680cc6fe8c91699fdda153b125ae515c
> > # Parent  bf21e00155b7dd76653c5340099ecedac7a7de08
> > [QEMU-DM] Upgrade emulated UART to 16550A.
> >
> > This patch adds 16550 emulation to qemu-dm. I still consider it a work
> > in progress, but from my testing, it consistently performs better than
> > the old code already, sometimes considerably so (on my laptop, dumping
> > out data over serial from a HVM DomU -> pty in Dom0 was up to 5.3
> > times faster than with the old 16450 code). I can consistenly reach
> > full 115200 baud speeds over physical serial lines when the virtual
> > device is backed by a phys port, which I never could with the old code.
> >
> > There are still some things to be done. I want to add proper error
> > handling, so that overflow/framing & parity errors on the physical
> > port get detected and reported by the virtual port. Also, now that
> > FIFOs are in place, I believe performance could be improved
> > significantly by putting some code into the hypervisor so that reads &
> > writes from the FIFOs don't have to exit into qemu-dm at all. I'm also
> > reading up on the specs for newer uarts with deeper FIFOs.
>
>   I notice with this patch that my HVM domain, which specifies
> serial=pty in the config file, no longer boots without me attaching to
> the console via xm.  Previously it would boot regardless of whether I
> was attached to the console.  Is this change in behavior intentional?
> Note that I'm testing on ia64 where the HVM firmware spews lots of debug
> info out onto the serial port.  Maybe this new UART model isn't dropping
> transmit chars when there's no client connected as it did before?
> Thanks,
>
>        Alex
>
> --
> Alex Williamson                             HP Open Source & Linux Org.
>
>

[-- Attachment #1.2: Type: text/html, Size: 2802 bytes --]

[-- Attachment #2: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [Xen-staging] [xen-unstable] [QEMU-DM] Upgrade emulated UART to 16550A.

[Trolle Selander]

[-- Attachment #1.1: Type: text/plain, Size: 2561 bytes --]

This patch fixes the issue. It also changes the name of an inappropriately
named timer.

Signed-off-by: Trolle Selander <trolle.selander@gmail.com>

On Dec 5, 2007 9:30 PM, Trolle Selander <trolle.selander@gmail.com> wrote:

> As you describe this, it does occur to me as you describe this that
> serial_xmit() never gives up on trying to retransmit characters out of the
> FIFO, which isn't very good for backends like pty's or pipes. I'll fix it
> asap.
>
>
> On Dec 5, 2007 9:15 PM, Alex Williamson <alex.williamson@hp.com> wrote:
>
> >
> > On Wed, 2007-12-05 at 14:22 +0000, Xen staging patchbot-unstable wrote:
> > > # HG changeset patch
> > > # User Keir Fraser
> > > # Date 1196864460 0
> > > # Node ID f0ac46de680cc6fe8c91699fdda153b125ae515c
> > > # Parent  bf21e00155b7dd76653c5340099ecedac7a7de08
> > > [QEMU-DM] Upgrade emulated UART to 16550A.
> > >
> > > This patch adds 16550 emulation to qemu-dm. I still consider it a work
> > > in progress, but from my testing, it consistently performs better than
> >
> > > the old code already, sometimes considerably so (on my laptop, dumping
> > > out data over serial from a HVM DomU -> pty in Dom0 was up to 5.3
> > > times faster than with the old 16450 code). I can consistenly reach
> > > full 115200 baud speeds over physical serial lines when the virtual
> > > device is backed by a phys port, which I never could with the old
> > code.
> > >
> > > There are still some things to be done. I want to add proper error
> > > handling, so that overflow/framing & parity errors on the physical
> > > port get detected and reported by the virtual port. Also, now that
> > > FIFOs are in place, I believe performance could be improved
> > > significantly by putting some code into the hypervisor so that reads &
> > > writes from the FIFOs don't have to exit into qemu-dm at all. I'm also
> > > reading up on the specs for newer uarts with deeper FIFOs.
> >
> >   I notice with this patch that my HVM domain, which specifies
> > serial=pty in the config file, no longer boots without me attaching to
> > the console via xm.  Previously it would boot regardless of whether I
> > was attached to the console.  Is this change in behavior intentional?
> > Note that I'm testing on ia64 where the HVM firmware spews lots of debug
> > info out onto the serial port.  Maybe this new UART model isn't dropping
> >
> > transmit chars when there's no client connected as it did before?
> > Thanks,
> >
> >        Alex
> >
> > --
> > Alex Williamson                             HP Open Source & Linux Org.
> >
> >
>

[-- Attachment #1.2: Type: text/html, Size: 3479 bytes --]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: xen-qemudm-16550A-limit-retransmit.patch --]
[-- Type: text/x-patch; name=xen-qemudm-16550A-limit-retransmit.patch, Size: 2001 bytes --]

diff -uNr xen-unstable.hg-patched/tools/ioemu/hw/serial.c xen-unstable.hg-patched2/tools/ioemu/hw/serial.c
--- xen-unstable.hg-patched/tools/ioemu/hw/serial.c	2007-12-06 23:50:58.652054883 +0100
+++ xen-unstable.hg-patched2/tools/ioemu/hw/serial.c	2007-12-07 01:01:13.236230348 +0100
@@ -96,6 +96,7 @@
 
 #define XMIT_FIFO           0
 #define RECV_FIFO           1
+#define MAX_XMIT_RETRY      4
  
 struct SerialFIFO {
     uint8_t data[UART_FIFO_LENGTH];
@@ -136,7 +137,7 @@
 
     struct QEMUTimer *fifo_timeout_timer;
     int timeout_ipending;                   /* timeout interrupt pending state */
-    struct QEMUTimer *fifo_transmit_timer;
+    struct QEMUTimer *transmit_timer;
 
 
     uint64_t char_transmit_time;               /* time to transmit a char in ticks*/
@@ -370,15 +371,17 @@
 
     if ( qemu_chr_write(s->chr, &s->tsr, 1) != 1 ) {
         s->tsr_retry++;
-        qemu_mod_timer(s->fifo_transmit_timer,  new_xmit_ts + s->char_transmit_time );
-        return;
+        if ( s->tsr_retry <= MAX_XMIT_RETRY ) {
+            qemu_mod_timer(s->transmit_timer,  new_xmit_ts + s->char_transmit_time );
+            return;
+        }
     }
  
     s->tsr_retry = 0;
     s->last_xmit_ts = qemu_get_clock(vm_clock);
 
-    if ( s->xmit_fifo.count > 0 )
-        qemu_mod_timer(s->fifo_transmit_timer, s->last_xmit_ts + s->char_transmit_time );
+    if ( !(s->lsr & UART_LSR_THRE) )
+        qemu_mod_timer(s->transmit_timer, s->last_xmit_ts + s->char_transmit_time );
 
     if ( s->lsr & UART_LSR_THRE ) {
         s->lsr |= UART_LSR_TEMT;
@@ -752,7 +755,7 @@
     fifo_clear(s,XMIT_FIFO);
     s->last_xmit_ts = qemu_get_clock(vm_clock);
     s->fifo_timeout_timer = qemu_new_timer(vm_clock, ( QEMUTimerCB *) fifo_timeout_int, s);
-    s->fifo_transmit_timer = qemu_new_timer(vm_clock, ( QEMUTimerCB *) serial_xmit, s);
+    s->transmit_timer = qemu_new_timer(vm_clock, ( QEMUTimerCB *) serial_xmit, s);
 
     register_savevm("serial", base, 2, serial_save, serial_load, s);
 

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [Xen-staging] [xen-unstable] [QEMU-DM] Upgrade emulated UART to 16550A.

[Alex Williamson]

On Wed, 2007-12-05 at 23:07 +0100, Trolle Selander wrote:
> This patch fixes the issue. It also changes the name of an
> inappropriately named timer.

Hi Trolle,

  Yes, with this patch, my HVM domain will eventually boot.
Unfortunately it takes 2 minutes now to get through all the UART
timeouts and get to the firmware menu.  Prior to the new 16550A UART
driver, it took 10 seconds.  I think we need to do something smarter
than drop characters after some number of timeouts.  We're doomed to
terrible slowness anytime the console is disconnected that way.  Thanks,

	Alex

-- 
Alex Williamson                             HP Open Source & Linux Org.

Re: [Xen-staging] [xen-unstable] [QEMU-DM] Upgrade emulated UART to 16550A.

[Keir Fraser]

On 7/12/07 17:21, "Alex Williamson" <alex.williamson@hp.com> wrote:

> On Wed, 2007-12-05 at 23:07 +0100, Trolle Selander wrote:
>> This patch fixes the issue. It also changes the name of an
>> inappropriately named timer.
> 
> Hi Trolle,
> 
>   Yes, with this patch, my HVM domain will eventually boot.
> Unfortunately it takes 2 minutes now to get through all the UART
> timeouts and get to the firmware menu.  Prior to the new 16550A UART
> driver, it took 10 seconds.  I think we need to do something smarter
> than drop characters after some number of timeouts.  We're doomed to
> terrible slowness anytime the console is disconnected that way.  Thanks,

Perhaps we should latch the first timeout and fast-drop bursts of characters
until the next character that is not immediately rejected?

 -- Keir

Re: [Xen-staging] [xen-unstable] [QEMU-DM] Upgrade emulated UART to 16550A.

[Trolle Selander]

[-- Attachment #1.1: Type: text/plain, Size: 990 bytes --]

That sounds like a workable idea, at least for the non-phys-port case.

On Dec 7, 2007 6:43 PM, Keir Fraser <Keir.Fraser@cl.cam.ac.uk> wrote:

> On 7/12/07 17:21, "Alex Williamson" <alex.williamson@hp.com> wrote:
>
> > On Wed, 2007-12-05 at 23:07 +0100, Trolle Selander wrote:
> >> This patch fixes the issue. It also changes the name of an
> >> inappropriately named timer.
> >
> > Hi Trolle,
> >
> >   Yes, with this patch, my HVM domain will eventually boot.
> > Unfortunately it takes 2 minutes now to get through all the UART
> > timeouts and get to the firmware menu.  Prior to the new 16550A UART
> > driver, it took 10 seconds.  I think we need to do something smarter
> > than drop characters after some number of timeouts.  We're doomed to
> > terrible slowness anytime the console is disconnected that way.  Thanks,
>
> Perhaps we should latch the first timeout and fast-drop bursts of
> characters
> until the next character that is not immediately rejected?
>
>  -- Keir
>
>
>

[-- Attachment #1.2: Type: text/html, Size: 1447 bytes --]

[-- Attachment #2: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [Xen-staging] [xen-unstable] [QEMU-DM] Upgrade emulated UART to 16550A.

[Trolle Selander]

[-- Attachment #1.1: Type: text/plain, Size: 1332 bytes --]

Here's a patch based on Keir's idea. Let's hope this resolves the issue.
It also includes the termios.h & sys/ioctl.h include fix discussed earlier,
since I didn't see that committed to xen-unstable yet.

On Dec 7, 2007 6:53 PM, Trolle Selander <trolle.selander@gmail.com> wrote:

> That sounds like a workable idea, at least for the non-phys-port case.
>
>
> On Dec 7, 2007 6:43 PM, Keir Fraser <Keir.Fraser@cl.cam.ac.uk> wrote:
>
> > On 7/12/07 17:21, "Alex Williamson" < alex.williamson@hp.com> wrote:
> >
> > > On Wed, 2007-12-05 at 23:07 +0100, Trolle Selander wrote:
> > >> This patch fixes the issue. It also changes the name of an
> > >> inappropriately named timer.
> > >
> > > Hi Trolle,
> > >
> > >   Yes, with this patch, my HVM domain will eventually boot.
> > > Unfortunately it takes 2 minutes now to get through all the UART
> > > timeouts and get to the firmware menu.  Prior to the new 16550A UART
> > > driver, it took 10 seconds.  I think we need to do something smarter
> > > than drop characters after some number of timeouts.  We're doomed to
> > > terrible slowness anytime the console is disconnected that way.
> >  Thanks,
> >
> > Perhaps we should latch the first timeout and fast-drop bursts of
> > characters
> > until the next character that is not immediately rejected?
> >
> >  -- Keir
> >
> >
> >
>

[-- Attachment #1.2: Type: text/html, Size: 2061 bytes --]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: xen-qemudm-serial-retransmit-fix.patch --]
[-- Type: text/x-patch; name=xen-qemudm-serial-retransmit-fix.patch, Size: 2035 bytes --]

diff -uNr xen-unstable.hg/tools/ioemu/hw/serial.c xen-unstable.hg-patched/tools/ioemu/hw/serial.c
--- xen-unstable.hg/tools/ioemu/hw/serial.c	2007-12-08 22:23:57.345820400 +0100
+++ xen-unstable.hg-patched/tools/ioemu/hw/serial.c	2007-12-08 22:26:41.551177930 +0100
@@ -26,16 +26,11 @@
 #include <sys/time.h>
 #include <time.h>
 #include <assert.h>
+#include <termios.h>
+#include <sys/ioctl.h>
 
 //#define DEBUG_SERIAL
 
-#define TIOCM_DTR	0x002
-#define TIOCM_RTS	0x004
-#define TIOCM_CTS	0x020
-#define TIOCM_CAR	0x040
-#define TIOCM_RI	0x080
-#define TIOCM_DSR	0x100
-
 #define UART_LCR_DLAB	0x80	/* Divisor latch access bit */
 
 #define UART_IER_MSI	0x08	/* Enable Modem status interrupt */
@@ -364,7 +359,7 @@
     SerialState *s = opaque;
     uint64_t new_xmit_ts = qemu_get_clock(vm_clock);
     
-    if ( s->tsr_retry == 0 ) {
+    if ( s->tsr_retry <= 0 ) {
         if (s->fcr & UART_FCR_FE) {
             s->tsr = fifo_get(s,XMIT_FIFO);
             if ( !s->xmit_fifo.count )
@@ -376,16 +371,22 @@
     }
 
     if ( qemu_chr_write(s->chr, &s->tsr, 1) != 1 ) {
-        s->tsr_retry++;
-        if ( s->tsr_retry <= MAX_XMIT_RETRY ) {
+        if ( ( s->tsr_retry > 0 ) && ( s->tsr_retry <= MAX_XMIT_RETRY ) ) {
+            s->tsr_retry++;
             qemu_mod_timer(s->transmit_timer,  new_xmit_ts + s->char_transmit_time );
             return;
+        } else if ( s->poll_msl < 0 ) {
+            /* If we exceed MAX_XMIT_RETRY and the backend is not a real serial port, then
+            drop any further failed writes instantly, until we get one that goes through.
+            This is to prevent guests that log to unconnected pipes or pty's from stalling. */
+            s->tsr_retry = -1;
         }
     }
- 
-    s->tsr_retry = 0;
-    s->last_xmit_ts = qemu_get_clock(vm_clock);
+    else {
+        s->tsr_retry = 0;
+    }
 
+    s->last_xmit_ts = qemu_get_clock(vm_clock);
     if ( !(s->lsr & UART_LSR_THRE) )
         qemu_mod_timer(s->transmit_timer, s->last_xmit_ts + s->char_transmit_time );
 

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [Xen-staging] [xen-unstable] [QEMU-DM] Upgrade emulated UART to 16550A.

[Alex Williamson]

On Fri, 2007-12-07 at 20:36 +0100, Trolle Selander wrote:
> Here's a patch based on Keir's idea. Let's hope this resolves the
> issue.

   Great, this seems to work fine.  Thanks!

> It also includes the termios.h & sys/ioctl.h include fix discussed
> earlier, since I didn't see that committed to xen-unstable yet. 

   This is already in the staging tree, so I only the last 2 chunks of
the patch are necessary.  Thanks,

	Alex


-- 
Alex Williamson                             HP Open Source & Linux Org.

Re: [Xen-staging] [xen-unstable] [QEMU-DM] Upgrade emulated UART to 16550A.

[Trolle Selander]

[-- Attachment #1.1: Type: text/plain, Size: 790 bytes --]

Alright, good to have this resolved. Here's the patch with only the
significant bits, and a signed-off-by line.

Signed-off-by: Trolle Selander <trolle.selander@gmail.com>

On Dec 7, 2007 9:03 PM, Alex Williamson <alex.williamson@hp.com> wrote:

>
> On Fri, 2007-12-07 at 20:36 +0100, Trolle Selander wrote:
> > Here's a patch based on Keir's idea. Let's hope this resolves the
> > issue.
>
>   Great, this seems to work fine.  Thanks!
>
> > It also includes the termios.h & sys/ioctl.h include fix discussed
> > earlier, since I didn't see that committed to xen-unstable yet.
>
>   This is already in the staging tree, so I only the last 2 chunks of
> the patch are necessary.  Thanks,
>
>        Alex
>
>
> --
> Alex Williamson                             HP Open Source & Linux Org.
>
>

[-- Attachment #1.2: Type: text/html, Size: 1468 bytes --]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: xen-qemudm-serial-retransmit-final.patch --]
[-- Type: text/x-patch; name=xen-qemudm-serial-retransmit-final.patch, Size: 1621 bytes --]

diff -uNr xen-unstable.hg-patched/tools/ioemu/hw/serial.c xen-unstable.hg-patched2/tools/ioemu/hw/serial.c
--- xen-unstable.hg-patched/tools/ioemu/hw/serial.c	2007-12-08 23:08:19.649536443 +0100
+++ xen-unstable.hg-patched2/tools/ioemu/hw/serial.c	2007-12-08 23:10:11.787926845 +0100
@@ -359,7 +359,7 @@
     SerialState *s = opaque;
     uint64_t new_xmit_ts = qemu_get_clock(vm_clock);
     
-    if ( s->tsr_retry == 0 ) {
+    if ( s->tsr_retry <= 0 ) {
         if (s->fcr & UART_FCR_FE) {
             s->tsr = fifo_get(s,XMIT_FIFO);
             if ( !s->xmit_fifo.count )
@@ -371,16 +371,22 @@
     }
 
     if ( qemu_chr_write(s->chr, &s->tsr, 1) != 1 ) {
-        s->tsr_retry++;
-        if ( s->tsr_retry <= MAX_XMIT_RETRY ) {
+        if ( ( s->tsr_retry > 0 ) && ( s->tsr_retry <= MAX_XMIT_RETRY ) ) {
+            s->tsr_retry++;
             qemu_mod_timer(s->transmit_timer,  new_xmit_ts + s->char_transmit_time );
             return;
+        } else if ( s->poll_msl < 0 ) {
+            /* If we exceed MAX_XMIT_RETRY and the backend is not a real serial port, then
+            drop any further failed writes instantly, until we get one that goes through.
+            This is to prevent guests that log to unconnected pipes or pty's from stalling. */
+            s->tsr_retry = -1;
         }
     }
- 
-    s->tsr_retry = 0;
-    s->last_xmit_ts = qemu_get_clock(vm_clock);
+    else {
+        s->tsr_retry = 0;
+    }
 
+    s->last_xmit_ts = qemu_get_clock(vm_clock);
     if ( !(s->lsr & UART_LSR_THRE) )
         qemu_mod_timer(s->transmit_timer, s->last_xmit_ts + s->char_transmit_time );
 

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [Xen-staging] [xen-unstable] [QEMU-DM] Upgrade emulated UART to 16550A.

[Trolle Selander]

[-- Attachment #1.1: Type: text/plain, Size: 1379 bytes --]

I'll look into it further, though I'm a bit puzzled as to why it was faster
before - the old code would attempt to retransmit every 100ms, for up to
300ms. That's a LOT longer than my code will retry at any kind of sane
baud-rates. What guest are you running in the HVM?
At one point I was considering adding a "backend type" field for the qemu
char devices to prevent having to detect if the backend device was a
physical port from within serial.c. Perhaps I'll need to go back to that
plan and treat pty's differently, or simply skip the transmit retries
entirely when the backend isn't a physical serial port.

On Dec 7, 2007 6:21 PM, Alex Williamson <alex.williamson@hp.com> wrote:

>
> On Wed, 2007-12-05 at 23:07 +0100, Trolle Selander wrote:
> > This patch fixes the issue. It also changes the name of an
> > inappropriately named timer.
>
> Hi Trolle,
>
>  Yes, with this patch, my HVM domain will eventually boot.
> Unfortunately it takes 2 minutes now to get through all the UART
> timeouts and get to the firmware menu.  Prior to the new 16550A UART
> driver, it took 10 seconds.  I think we need to do something smarter
> than drop characters after some number of timeouts.  We're doomed to
> terrible slowness anytime the console is disconnected that way.  Thanks,
>
>        Alex
>
> --
> Alex Williamson                             HP Open Source & Linux Org.
>
>

[-- Attachment #1.2: Type: text/html, Size: 1885 bytes --]

[-- Attachment #2: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel