[Qemu-devel] qemu serial: lost tx irqs (affectig FreeBSD's new uart(4) driver)

[Qemu-devel] qemu serial: lost tx irqs (affectig FreeBSD's new uart(4) driver)

[Juergen Lock]

Hi!

 I got a report of FreeBSD guest's new uart(4) driver misbehaving in
qemu again(?) (output stopping for no apparent reason), and now found
out the problem is tx irqs (UART_IIR_THRI) are getting lost because
serial_update_irq() checks for the rx condtion,
	... if ((s->ier & UART_IER_RDI) && (s->lsr & UART_LSR_DR))
first before checking for the tx irq condition,
	... if ((s->ier & UART_IER_THRI) && s->thr_ipending)
which at least in this case (FreeBSD 8 guest after doing
	set console="comconsole"
at the loader prompt or when simply echo'ing text to /dev/ttyu0
or typing to the serial port from cu(1) on a `regular' vga console)
causes the second condition (.. && s->thr_ipending) to be never
reached anymore, or only after a very long delay.  Moving that
condition up so it is checked first like this,

Index: qemu/hw/serial.c
@@ -189,7 +188,9 @@ static void serial_update_irq(SerialStat
 {
     uint8_t tmp_iir = UART_IIR_NO_INT;
 
-    if ((s->ier & UART_IER_RLSI) && (s->lsr & UART_LSR_INT_ANY)) {
+    if ((s->ier & UART_IER_THRI) && s->thr_ipending) {
+        tmp_iir = UART_IIR_THRI;
+    } else if ((s->ier & UART_IER_RLSI) && (s->lsr & UART_LSR_INT_ANY)) {
         tmp_iir = UART_IIR_RLSI;
     } else if ((s->ier & UART_IER_RDI) && s->timeout_ipending) {
         /* Note that(s->ier & UART_IER_RDI) can mask this interrupt,
@@ -202,8 +203,6 @@ static void serial_update_irq(SerialStat
         } else if (s->recv_fifo.count >= s->recv_fifo.itl) {
            tmp_iir = UART_IIR_RDI;
         }
-    } else if ((s->ier & UART_IER_THRI) && s->thr_ipending) {
-        tmp_iir = UART_IIR_THRI;
     } else if ((s->ier & UART_IER_MSI) && (s->msr & UART_MSR_ANY_DELTA)) {
         tmp_iir = UART_IIR_MSI;
     }

...fixes the issue for me, but I'm not 100% sure if this might cause
rx irqs to come (too?) late when a guest keeps sending while its
receiving at the same time.  Anyone care to comment? :)

 Thanx,
	Juergen

[Qemu-devel] Re: qemu serial: lost tx irqs (affectig FreeBSD's new uart(4) driver)

[Olivier Cochard-Labbé]

Hi,

On Fri, Sep 11, 2009 at 11:35 PM, Juergen Lock <nox@jelal.kn-bremen.de> wrote:

> ...fixes the issue for me, but I'm not 100% sure if this might cause
> rx irqs to come (too?) late when a guest keeps sending while its
> receiving at the same time.  Anyone care to comment? :)
>

Your patch fix the issue for me too.

Thanks a lot's Juergen !

Regards,

Olivier

[Qemu-devel] Re: qemu serial: lost tx irqs (affectig FreeBSD's new uart(4) driver)

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 2256 bytes --]

Juergen Lock wrote:
> Hi!
> 
>  I got a report of FreeBSD guest's new uart(4) driver misbehaving in
> qemu again(?) (output stopping for no apparent reason), and now found
> out the problem is tx irqs (UART_IIR_THRI) are getting lost because
> serial_update_irq() checks for the rx condtion,
> 	... if ((s->ier & UART_IER_RDI) && (s->lsr & UART_LSR_DR))
> first before checking for the tx irq condition,
> 	... if ((s->ier & UART_IER_THRI) && s->thr_ipending)
> which at least in this case (FreeBSD 8 guest after doing
> 	set console="comconsole"
> at the loader prompt or when simply echo'ing text to /dev/ttyu0
> or typing to the serial port from cu(1) on a `regular' vga console)
> causes the second condition (.. && s->thr_ipending) to be never
> reached anymore, or only after a very long delay.  Moving that
> condition up so it is checked first like this,
> 
> Index: qemu/hw/serial.c
> @@ -189,7 +188,9 @@ static void serial_update_irq(SerialStat
>  {
>      uint8_t tmp_iir = UART_IIR_NO_INT;
>  
> -    if ((s->ier & UART_IER_RLSI) && (s->lsr & UART_LSR_INT_ANY)) {
> +    if ((s->ier & UART_IER_THRI) && s->thr_ipending) {
> +        tmp_iir = UART_IIR_THRI;
> +    } else if ((s->ier & UART_IER_RLSI) && (s->lsr & UART_LSR_INT_ANY)) {
>          tmp_iir = UART_IIR_RLSI;
>      } else if ((s->ier & UART_IER_RDI) && s->timeout_ipending) {
>          /* Note that(s->ier & UART_IER_RDI) can mask this interrupt,
> @@ -202,8 +203,6 @@ static void serial_update_irq(SerialStat
>          } else if (s->recv_fifo.count >= s->recv_fifo.itl) {
>             tmp_iir = UART_IIR_RDI;
>          }
> -    } else if ((s->ier & UART_IER_THRI) && s->thr_ipending) {
> -        tmp_iir = UART_IIR_THRI;
>      } else if ((s->ier & UART_IER_MSI) && (s->msr & UART_MSR_ANY_DELTA)) {
>          tmp_iir = UART_IIR_MSI;
>      }
> 
> ...fixes the issue for me, but I'm not 100% sure if this might cause
> rx irqs to come (too?) late when a guest keeps sending while its
> receiving at the same time.  Anyone care to comment? :)

The reordering violates the 16550A spec in that RX event overrules TX in
the IRQ status register. Maybe something else is wrong but it's not the
ordering in serial_update_irq.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

[Qemu-devel] Re: qemu serial: lost tx irqs (affectig FreeBSD's new uart(4) driver)

[Juergen Lock]

On Sat, Sep 12, 2009 at 02:26:51PM +0200, Jan Kiszka wrote:
> Juergen Lock wrote:
> > Hi!
> > 
> >  I got a report of FreeBSD guest's new uart(4) driver misbehaving in
> > qemu again(?) (output stopping for no apparent reason), and now found
> > out the problem is tx irqs (UART_IIR_THRI) are getting lost because
> > serial_update_irq() checks for the rx condtion,
> > 	... if ((s->ier & UART_IER_RDI) && (s->lsr & UART_LSR_DR))
> > first before checking for the tx irq condition,
> > 	... if ((s->ier & UART_IER_THRI) && s->thr_ipending)
> > which at least in this case (FreeBSD 8 guest after doing
> > 	set console="comconsole"
> > at the loader prompt or when simply echo'ing text to /dev/ttyu0
> > or typing to the serial port from cu(1) on a `regular' vga console)
> > causes the second condition (.. && s->thr_ipending) to be never
> > reached anymore, or only after a very long delay.  Moving that
> > condition up so it is checked first like this,
> > 
> > Index: qemu/hw/serial.c
> > @@ -189,7 +188,9 @@ static void serial_update_irq(SerialStat
> >  {
> >      uint8_t tmp_iir = UART_IIR_NO_INT;
> >  
> > -    if ((s->ier & UART_IER_RLSI) && (s->lsr & UART_LSR_INT_ANY)) {
> > +    if ((s->ier & UART_IER_THRI) && s->thr_ipending) {
> > +        tmp_iir = UART_IIR_THRI;
> > +    } else if ((s->ier & UART_IER_RLSI) && (s->lsr & UART_LSR_INT_ANY)) {
> >          tmp_iir = UART_IIR_RLSI;
> >      } else if ((s->ier & UART_IER_RDI) && s->timeout_ipending) {
> >          /* Note that(s->ier & UART_IER_RDI) can mask this interrupt,
> > @@ -202,8 +203,6 @@ static void serial_update_irq(SerialStat
> >          } else if (s->recv_fifo.count >= s->recv_fifo.itl) {
> >             tmp_iir = UART_IIR_RDI;
> >          }
> > -    } else if ((s->ier & UART_IER_THRI) && s->thr_ipending) {
> > -        tmp_iir = UART_IIR_THRI;
> >      } else if ((s->ier & UART_IER_MSI) && (s->msr & UART_MSR_ANY_DELTA)) {
> >          tmp_iir = UART_IIR_MSI;
> >      }
> > 
> > ...fixes the issue for me, but I'm not 100% sure if this might cause
> > rx irqs to come (too?) late when a guest keeps sending while its
> > receiving at the same time.  Anyone care to comment? :)
> 
> The reordering violates the 16550A spec in that RX event overrules TX in
> the IRQ status register. Maybe something else is wrong but it's not the
> ordering in serial_update_irq.

Well one problem seems to be the rx condition,
 	... if ((s->ier & UART_IER_RDI) && (s->lsr & UART_LSR_DR))
is not enough to trigger an irq, yet still causes the following
conditions not to be checked anymore at all.  And ideed, fixing that
seems to get my FreeBSD 8 guest back to working order as well:

Index: qemu/hw/serial.c
@@ -196,12 +195,10 @@ static void serial_update_irq(SerialStat
          * this is not in the specification but is observed on existing
          * hardware.  */
         tmp_iir = UART_IIR_CTI;
-    } else if ((s->ier & UART_IER_RDI) && (s->lsr & UART_LSR_DR)) {
-        if (!(s->fcr & UART_FCR_FE)) {
-           tmp_iir = UART_IIR_RDI;
-        } else if (s->recv_fifo.count >= s->recv_fifo.itl) {
-           tmp_iir = UART_IIR_RDI;
-        }
+    } else if ((s->ier & UART_IER_RDI) && (s->lsr & UART_LSR_DR) &&
+               (!(s->fcr & UART_FCR_FE) ||
+                s->recv_fifo.count >= s->recv_fifo.itl)) {
+        tmp_iir = UART_IIR_RDI;
     } else if ((s->ier & UART_IER_THRI) && s->thr_ipending) {
         tmp_iir = UART_IIR_THRI;
     } else if ((s->ier & UART_IER_MSI) && (s->msr & UART_MSR_ANY_DELTA)) {

 Signed-off-by: Juergen Lock <nox@jelal.kn-bremen.de>

[Qemu-devel] Re: qemu serial: lost tx irqs (affectig FreeBSD's new uart(4) driver)

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 3865 bytes --]

Juergen Lock wrote:
> On Sat, Sep 12, 2009 at 02:26:51PM +0200, Jan Kiszka wrote:
>> Juergen Lock wrote:
>>> Hi!
>>>
>>>  I got a report of FreeBSD guest's new uart(4) driver misbehaving in
>>> qemu again(?) (output stopping for no apparent reason), and now found
>>> out the problem is tx irqs (UART_IIR_THRI) are getting lost because
>>> serial_update_irq() checks for the rx condtion,
>>> 	... if ((s->ier & UART_IER_RDI) && (s->lsr & UART_LSR_DR))
>>> first before checking for the tx irq condition,
>>> 	... if ((s->ier & UART_IER_THRI) && s->thr_ipending)
>>> which at least in this case (FreeBSD 8 guest after doing
>>> 	set console="comconsole"
>>> at the loader prompt or when simply echo'ing text to /dev/ttyu0
>>> or typing to the serial port from cu(1) on a `regular' vga console)
>>> causes the second condition (.. && s->thr_ipending) to be never
>>> reached anymore, or only after a very long delay.  Moving that
>>> condition up so it is checked first like this,
>>>
>>> Index: qemu/hw/serial.c
>>> @@ -189,7 +188,9 @@ static void serial_update_irq(SerialStat
>>>  {
>>>      uint8_t tmp_iir = UART_IIR_NO_INT;
>>>  
>>> -    if ((s->ier & UART_IER_RLSI) && (s->lsr & UART_LSR_INT_ANY)) {
>>> +    if ((s->ier & UART_IER_THRI) && s->thr_ipending) {
>>> +        tmp_iir = UART_IIR_THRI;
>>> +    } else if ((s->ier & UART_IER_RLSI) && (s->lsr & UART_LSR_INT_ANY)) {
>>>          tmp_iir = UART_IIR_RLSI;
>>>      } else if ((s->ier & UART_IER_RDI) && s->timeout_ipending) {
>>>          /* Note that(s->ier & UART_IER_RDI) can mask this interrupt,
>>> @@ -202,8 +203,6 @@ static void serial_update_irq(SerialStat
>>>          } else if (s->recv_fifo.count >= s->recv_fifo.itl) {
>>>             tmp_iir = UART_IIR_RDI;
>>>          }
>>> -    } else if ((s->ier & UART_IER_THRI) && s->thr_ipending) {
>>> -        tmp_iir = UART_IIR_THRI;
>>>      } else if ((s->ier & UART_IER_MSI) && (s->msr & UART_MSR_ANY_DELTA)) {
>>>          tmp_iir = UART_IIR_MSI;
>>>      }
>>>
>>> ...fixes the issue for me, but I'm not 100% sure if this might cause
>>> rx irqs to come (too?) late when a guest keeps sending while its
>>> receiving at the same time.  Anyone care to comment? :)
>> The reordering violates the 16550A spec in that RX event overrules TX in
>> the IRQ status register. Maybe something else is wrong but it's not the
>> ordering in serial_update_irq.
> 
> Well one problem seems to be the rx condition,
>  	... if ((s->ier & UART_IER_RDI) && (s->lsr & UART_LSR_DR))
> is not enough to trigger an irq, yet still causes the following
> conditions not to be checked anymore at all.  And ideed, fixing that
> seems to get my FreeBSD 8 guest back to working order as well:
> 
> Index: qemu/hw/serial.c
> @@ -196,12 +195,10 @@ static void serial_update_irq(SerialStat
>           * this is not in the specification but is observed on existing
>           * hardware.  */
>          tmp_iir = UART_IIR_CTI;
> -    } else if ((s->ier & UART_IER_RDI) && (s->lsr & UART_LSR_DR)) {
> -        if (!(s->fcr & UART_FCR_FE)) {
> -           tmp_iir = UART_IIR_RDI;
> -        } else if (s->recv_fifo.count >= s->recv_fifo.itl) {
> -           tmp_iir = UART_IIR_RDI;
> -        }
> +    } else if ((s->ier & UART_IER_RDI) && (s->lsr & UART_LSR_DR) &&
> +               (!(s->fcr & UART_FCR_FE) ||
> +                s->recv_fifo.count >= s->recv_fifo.itl)) {
> +        tmp_iir = UART_IIR_RDI;
>      } else if ((s->ier & UART_IER_THRI) && s->thr_ipending) {
>          tmp_iir = UART_IIR_THRI;
>      } else if ((s->ier & UART_IER_MSI) && (s->msr & UART_MSR_ANY_DELTA)) {
> 
>  Signed-off-by: Juergen Lock <nox@jelal.kn-bremen.de>

Yep, that does make sense!

Acked-by: Jan Kiszka <jan.kiszka@web.de>

But I also but Stefano on CC as he introduced the logic above.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: [Qemu-devel] Re: qemu serial: lost tx irqs (affectig FreeBSD's new uart(4) driver)

[Stefano Stabellini]

On Sat, 12 Sep 2009, Juergen Lock wrote:
> Well one problem seems to be the rx condition,
>  	... if ((s->ier & UART_IER_RDI) && (s->lsr & UART_LSR_DR))
> is not enough to trigger an irq, yet still causes the following
> conditions not to be checked anymore at all.  And ideed, fixing that
> seems to get my FreeBSD 8 guest back to working order as well:

Good spot!
The fix also seems correct to me.

Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

> Index: qemu/hw/serial.c
> @@ -196,12 +195,10 @@ static void serial_update_irq(SerialStat
>           * this is not in the specification but is observed on existing
>           * hardware.  */
>          tmp_iir = UART_IIR_CTI;
> -    } else if ((s->ier & UART_IER_RDI) && (s->lsr & UART_LSR_DR)) {
> -        if (!(s->fcr & UART_FCR_FE)) {
> -           tmp_iir = UART_IIR_RDI;
> -        } else if (s->recv_fifo.count >= s->recv_fifo.itl) {
> -           tmp_iir = UART_IIR_RDI;
> -        }
> +    } else if ((s->ier & UART_IER_RDI) && (s->lsr & UART_LSR_DR) &&
> +               (!(s->fcr & UART_FCR_FE) ||
> +                s->recv_fifo.count >= s->recv_fifo.itl)) {
> +        tmp_iir = UART_IIR_RDI;
>      } else if ((s->ier & UART_IER_THRI) && s->thr_ipending) {
>          tmp_iir = UART_IIR_THRI;
>      } else if ((s->ier & UART_IER_MSI) && (s->msr & UART_MSR_ANY_DELTA)) {
> 
>  Signed-off-by: Juergen Lock <nox@jelal.kn-bremen.de>
> 
> 

Re: [Qemu-devel] Re: qemu serial: lost tx irqs (affectig FreeBSD's new uart(4) driver)

[Aurelien Jarno]

On Sat, Sep 12, 2009 at 06:52:22PM +0200, Juergen Lock wrote:
> On Sat, Sep 12, 2009 at 02:26:51PM +0200, Jan Kiszka wrote:
> > Juergen Lock wrote:
> > > Hi!
> > > 
> > >  I got a report of FreeBSD guest's new uart(4) driver misbehaving in
> > > qemu again(?) (output stopping for no apparent reason), and now found
> > > out the problem is tx irqs (UART_IIR_THRI) are getting lost because
> > > serial_update_irq() checks for the rx condtion,
> > > 	... if ((s->ier & UART_IER_RDI) && (s->lsr & UART_LSR_DR))
> > > first before checking for the tx irq condition,
> > > 	... if ((s->ier & UART_IER_THRI) && s->thr_ipending)
> > > which at least in this case (FreeBSD 8 guest after doing
> > > 	set console="comconsole"
> > > at the loader prompt or when simply echo'ing text to /dev/ttyu0
> > > or typing to the serial port from cu(1) on a `regular' vga console)
> > > causes the second condition (.. && s->thr_ipending) to be never
> > > reached anymore, or only after a very long delay.  Moving that
> > > condition up so it is checked first like this,
> > > 
> > > Index: qemu/hw/serial.c
> > > @@ -189,7 +188,9 @@ static void serial_update_irq(SerialStat
> > >  {
> > >      uint8_t tmp_iir = UART_IIR_NO_INT;
> > >  
> > > -    if ((s->ier & UART_IER_RLSI) && (s->lsr & UART_LSR_INT_ANY)) {
> > > +    if ((s->ier & UART_IER_THRI) && s->thr_ipending) {
> > > +        tmp_iir = UART_IIR_THRI;
> > > +    } else if ((s->ier & UART_IER_RLSI) && (s->lsr & UART_LSR_INT_ANY)) {
> > >          tmp_iir = UART_IIR_RLSI;
> > >      } else if ((s->ier & UART_IER_RDI) && s->timeout_ipending) {
> > >          /* Note that(s->ier & UART_IER_RDI) can mask this interrupt,
> > > @@ -202,8 +203,6 @@ static void serial_update_irq(SerialStat
> > >          } else if (s->recv_fifo.count >= s->recv_fifo.itl) {
> > >             tmp_iir = UART_IIR_RDI;
> > >          }
> > > -    } else if ((s->ier & UART_IER_THRI) && s->thr_ipending) {
> > > -        tmp_iir = UART_IIR_THRI;
> > >      } else if ((s->ier & UART_IER_MSI) && (s->msr & UART_MSR_ANY_DELTA)) {
> > >          tmp_iir = UART_IIR_MSI;
> > >      }
> > > 
> > > ...fixes the issue for me, but I'm not 100% sure if this might cause
> > > rx irqs to come (too?) late when a guest keeps sending while its
> > > receiving at the same time.  Anyone care to comment? :)
> > 
> > The reordering violates the 16550A spec in that RX event overrules TX in
> > the IRQ status register. Maybe something else is wrong but it's not the
> > ordering in serial_update_irq.
> 
> Well one problem seems to be the rx condition,
>  	... if ((s->ier & UART_IER_RDI) && (s->lsr & UART_LSR_DR))
> is not enough to trigger an irq, yet still causes the following
> conditions not to be checked anymore at all.  And ideed, fixing that
> seems to get my FreeBSD 8 guest back to working order as well:

Applied. In the future, could you please make sure to send patches with
a correct unified headers?

> Index: qemu/hw/serial.c
> @@ -196,12 +195,10 @@ static void serial_update_irq(SerialStat
>           * this is not in the specification but is observed on existing
>           * hardware.  */
>          tmp_iir = UART_IIR_CTI;
> -    } else if ((s->ier & UART_IER_RDI) && (s->lsr & UART_LSR_DR)) {
> -        if (!(s->fcr & UART_FCR_FE)) {
> -           tmp_iir = UART_IIR_RDI;
> -        } else if (s->recv_fifo.count >= s->recv_fifo.itl) {
> -           tmp_iir = UART_IIR_RDI;
> -        }
> +    } else if ((s->ier & UART_IER_RDI) && (s->lsr & UART_LSR_DR) &&
> +               (!(s->fcr & UART_FCR_FE) ||
> +                s->recv_fifo.count >= s->recv_fifo.itl)) {
> +        tmp_iir = UART_IIR_RDI;
>      } else if ((s->ier & UART_IER_THRI) && s->thr_ipending) {
>          tmp_iir = UART_IIR_THRI;
>      } else if ((s->ier & UART_IER_MSI) && (s->msr & UART_MSR_ANY_DELTA)) {
> 
>  Signed-off-by: Juergen Lock <nox@jelal.kn-bremen.de>
> 
> 
> 

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] Re: qemu serial: lost tx irqs (affectig FreeBSD's new uart(4) driver)

[Juergen Lock]

In article <20090916190142.GC770@volta.aurel32.net> you write:
>On Sat, Sep 12, 2009 at 06:52:22PM +0200, Juergen Lock wrote:
>> On Sat, Sep 12, 2009 at 02:26:51PM +0200, Jan Kiszka wrote:
>> > Juergen Lock wrote:
>> > > Hi!
>> > > 
>> > >  I got a report of FreeBSD guest's new uart(4) driver misbehaving in
>> > > qemu again(?) (output stopping for no apparent reason), and now found
>> > > out the problem is tx irqs (UART_IIR_THRI) are getting lost because
>> > > serial_update_irq() checks for the rx condtion,
>> > > 	... if ((s->ier & UART_IER_RDI) && (s->lsr & UART_LSR_DR))
>> > > first before checking for the tx irq condition,
>> > > 	... if ((s->ier & UART_IER_THRI) && s->thr_ipending)
>> > > which at least in this case (FreeBSD 8 guest after doing
>> > > 	set console="comconsole"
>> > > at the loader prompt or when simply echo'ing text to /dev/ttyu0
>> > > or typing to the serial port from cu(1) on a `regular' vga console)
>> > > causes the second condition (.. && s->thr_ipending) to be never
>> > > reached anymore, or only after a very long delay.  Moving that
>> > > condition up so it is checked first like this,
>> > > 
>> > > Index: qemu/hw/serial.c
>> > > @@ -189,7 +188,9 @@ static void serial_update_irq(SerialStat
>> > >  {
>> > >      uint8_t tmp_iir = UART_IIR_NO_INT;
>> > >  
>> > > -    if ((s->ier & UART_IER_RLSI) && (s->lsr & UART_LSR_INT_ANY)) {
>> > > +    if ((s->ier & UART_IER_THRI) && s->thr_ipending) {
>> > > +        tmp_iir = UART_IIR_THRI;
>> > > +    } else if ((s->ier & UART_IER_RLSI) && (s->lsr & UART_LSR_INT_ANY)) {
>> > >          tmp_iir = UART_IIR_RLSI;
>> > >      } else if ((s->ier & UART_IER_RDI) && s->timeout_ipending) {
>> > >          /* Note that(s->ier & UART_IER_RDI) can mask this interrupt,
>> > > @@ -202,8 +203,6 @@ static void serial_update_irq(SerialStat
>> > >          } else if (s->recv_fifo.count >= s->recv_fifo.itl) {
>> > >             tmp_iir = UART_IIR_RDI;
>> > >          }
>> > > -    } else if ((s->ier & UART_IER_THRI) && s->thr_ipending) {
>> > > -        tmp_iir = UART_IIR_THRI;
>> > >      } else if ((s->ier & UART_IER_MSI) && (s->msr & UART_MSR_ANY_DELTA)) {
>> > >          tmp_iir = UART_IIR_MSI;
>> > >      }
>> > > 
>> > > ...fixes the issue for me, but I'm not 100% sure if this might cause
>> > > rx irqs to come (too?) late when a guest keeps sending while its
>> > > receiving at the same time.  Anyone care to comment? :)
>> > 
>> > The reordering violates the 16550A spec in that RX event overrules TX in
>> > the IRQ status register. Maybe something else is wrong but it's not the
>> > ordering in serial_update_irq.
>> 
>> Well one problem seems to be the rx condition,
>>  	... if ((s->ier & UART_IER_RDI) && (s->lsr & UART_LSR_DR))
>> is not enough to trigger an irq, yet still causes the following
>> conditions not to be checked anymore at all.  And ideed, fixing that
>> seems to get my FreeBSD 8 guest back to working order as well:
>
>Applied. In the future, could you please make sure to send patches with
>a correct unified headers?

Alright, if thats is what you guys prefer...  (I just didn't want to
break the thread.)

 Anyway, I guess this is also material for the stable branch(es)?
(I just saw 0.11.0 has already been tagged but not announced yet, and
another patch merged to the same branch after that, maybe the tag can
still be slided if this is possible with git?)

 Thanx,
	Juergen

Re: [Qemu-devel] Re: qemu serial: lost tx irqs (affectig FreeBSD's new uart(4) driver)

[Aurelien Jarno]

On Wed, Sep 23, 2009 at 08:47:23PM +0200, Juergen Lock wrote:
> In article <20090916190142.GC770@volta.aurel32.net> you write:
> >On Sat, Sep 12, 2009 at 06:52:22PM +0200, Juergen Lock wrote:
> >> On Sat, Sep 12, 2009 at 02:26:51PM +0200, Jan Kiszka wrote:
> >> > Juergen Lock wrote:
> >> > > Hi!
> >> > > 
> >> > >  I got a report of FreeBSD guest's new uart(4) driver misbehaving in
> >> > > qemu again(?) (output stopping for no apparent reason), and now found
> >> > > out the problem is tx irqs (UART_IIR_THRI) are getting lost because
> >> > > serial_update_irq() checks for the rx condtion,
> >> > > 	... if ((s->ier & UART_IER_RDI) && (s->lsr & UART_LSR_DR))
> >> > > first before checking for the tx irq condition,
> >> > > 	... if ((s->ier & UART_IER_THRI) && s->thr_ipending)
> >> > > which at least in this case (FreeBSD 8 guest after doing
> >> > > 	set console="comconsole"
> >> > > at the loader prompt or when simply echo'ing text to /dev/ttyu0
> >> > > or typing to the serial port from cu(1) on a `regular' vga console)
> >> > > causes the second condition (.. && s->thr_ipending) to be never
> >> > > reached anymore, or only after a very long delay.  Moving that
> >> > > condition up so it is checked first like this,
> >> > > 
> >> > > Index: qemu/hw/serial.c
> >> > > @@ -189,7 +188,9 @@ static void serial_update_irq(SerialStat
> >> > >  {
> >> > >      uint8_t tmp_iir = UART_IIR_NO_INT;
> >> > >  
> >> > > -    if ((s->ier & UART_IER_RLSI) && (s->lsr & UART_LSR_INT_ANY)) {
> >> > > +    if ((s->ier & UART_IER_THRI) && s->thr_ipending) {
> >> > > +        tmp_iir = UART_IIR_THRI;
> >> > > +    } else if ((s->ier & UART_IER_RLSI) && (s->lsr & UART_LSR_INT_ANY)) {
> >> > >          tmp_iir = UART_IIR_RLSI;
> >> > >      } else if ((s->ier & UART_IER_RDI) && s->timeout_ipending) {
> >> > >          /* Note that(s->ier & UART_IER_RDI) can mask this interrupt,
> >> > > @@ -202,8 +203,6 @@ static void serial_update_irq(SerialStat
> >> > >          } else if (s->recv_fifo.count >= s->recv_fifo.itl) {
> >> > >             tmp_iir = UART_IIR_RDI;
> >> > >          }
> >> > > -    } else if ((s->ier & UART_IER_THRI) && s->thr_ipending) {
> >> > > -        tmp_iir = UART_IIR_THRI;
> >> > >      } else if ((s->ier & UART_IER_MSI) && (s->msr & UART_MSR_ANY_DELTA)) {
> >> > >          tmp_iir = UART_IIR_MSI;
> >> > >      }
> >> > > 
> >> > > ...fixes the issue for me, but I'm not 100% sure if this might cause
> >> > > rx irqs to come (too?) late when a guest keeps sending while its
> >> > > receiving at the same time.  Anyone care to comment? :)
> >> > 
> >> > The reordering violates the 16550A spec in that RX event overrules TX in
> >> > the IRQ status register. Maybe something else is wrong but it's not the
> >> > ordering in serial_update_irq.
> >> 
> >> Well one problem seems to be the rx condition,
> >>  	... if ((s->ier & UART_IER_RDI) && (s->lsr & UART_LSR_DR))
> >> is not enough to trigger an irq, yet still causes the following
> >> conditions not to be checked anymore at all.  And ideed, fixing that
> >> seems to get my FreeBSD 8 guest back to working order as well:
> >
> >Applied. In the future, could you please make sure to send patches with
> >a correct unified headers?
> 
> Alright, if thats is what you guys prefer...  (I just didn't want to
> break the thread.)

Don't need to break the thread, just use

| --- a/hw/serial.c
| +++ a/hw/serial.c

instead of simply 

| Index: qemu/hw/serial.c

ie the output of diff -u

>  Anyway, I guess this is also material for the stable branch(es)?
> (I just saw 0.11.0 has already been tagged but not announced yet, and
> another patch merged to the same branch after that, maybe the tag can
> still be slided if this is possible with git?)
> 

Pushed to the branch. For the details about the release, I let Anthony
handling that. Worst case scenario, it will be in 0.11.1.

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] Re: qemu serial: lost tx irqs (affectig FreeBSD's new uart(4) driver)

[Juergen Lock]

On Thu, Sep 24, 2009 at 06:20:00PM +0200, Aurelien Jarno wrote:
> On Wed, Sep 23, 2009 at 08:47:23PM +0200, Juergen Lock wrote:
> > In article <20090916190142.GC770@volta.aurel32.net> you write:
> > >On Sat, Sep 12, 2009 at 06:52:22PM +0200, Juergen Lock wrote:
> > >> On Sat, Sep 12, 2009 at 02:26:51PM +0200, Jan Kiszka wrote:
> > >> > Juergen Lock wrote:
> > >> > > Hi!
> > >> > > 
> > >> > >  I got a report of FreeBSD guest's new uart(4) driver misbehaving in
> > >> > > qemu again(?) (output stopping for no apparent reason), and now found
> > >> > > out the problem is tx irqs (UART_IIR_THRI) are getting lost because
> > >> > > serial_update_irq() checks for the rx condtion,
> > >> > > 	... if ((s->ier & UART_IER_RDI) && (s->lsr & UART_LSR_DR))
> > >> > > first before checking for the tx irq condition,
> > >> > > 	... if ((s->ier & UART_IER_THRI) && s->thr_ipending)
> > >> > > which at least in this case (FreeBSD 8 guest after doing
> > >> > > 	set console="comconsole"
> > >> > > at the loader prompt or when simply echo'ing text to /dev/ttyu0
> > >> > > or typing to the serial port from cu(1) on a `regular' vga console)
> > >> > > causes the second condition (.. && s->thr_ipending) to be never
> > >> > > reached anymore, or only after a very long delay.  Moving that
> > >> > > condition up so it is checked first like this,
> > >> > > 
> > >> > > Index: qemu/hw/serial.c
> > >> > > @@ -189,7 +188,9 @@ static void serial_update_irq(SerialStat
> > >> > >  {
> > >> > >      uint8_t tmp_iir = UART_IIR_NO_INT;
> > >> > >  
> > >> > > -    if ((s->ier & UART_IER_RLSI) && (s->lsr & UART_LSR_INT_ANY)) {
> > >> > > +    if ((s->ier & UART_IER_THRI) && s->thr_ipending) {
> > >> > > +        tmp_iir = UART_IIR_THRI;
> > >> > > +    } else if ((s->ier & UART_IER_RLSI) && (s->lsr & UART_LSR_INT_ANY)) {
> > >> > >          tmp_iir = UART_IIR_RLSI;
> > >> > >      } else if ((s->ier & UART_IER_RDI) && s->timeout_ipending) {
> > >> > >          /* Note that(s->ier & UART_IER_RDI) can mask this interrupt,
> > >> > > @@ -202,8 +203,6 @@ static void serial_update_irq(SerialStat
> > >> > >          } else if (s->recv_fifo.count >= s->recv_fifo.itl) {
> > >> > >             tmp_iir = UART_IIR_RDI;
> > >> > >          }
> > >> > > -    } else if ((s->ier & UART_IER_THRI) && s->thr_ipending) {
> > >> > > -        tmp_iir = UART_IIR_THRI;
> > >> > >      } else if ((s->ier & UART_IER_MSI) && (s->msr & UART_MSR_ANY_DELTA)) {
> > >> > >          tmp_iir = UART_IIR_MSI;
> > >> > >      }
> > >> > > 
> > >> > > ...fixes the issue for me, but I'm not 100% sure if this might cause
> > >> > > rx irqs to come (too?) late when a guest keeps sending while its
> > >> > > receiving at the same time.  Anyone care to comment? :)
> > >> > 
> > >> > The reordering violates the 16550A spec in that RX event overrules TX in
> > >> > the IRQ status register. Maybe something else is wrong but it's not the
> > >> > ordering in serial_update_irq.
> > >> 
> > >> Well one problem seems to be the rx condition,
> > >>  	... if ((s->ier & UART_IER_RDI) && (s->lsr & UART_LSR_DR))
> > >> is not enough to trigger an irq, yet still causes the following
> > >> conditions not to be checked anymore at all.  And ideed, fixing that
> > >> seems to get my FreeBSD 8 guest back to working order as well:
> > >
> > >Applied. In the future, could you please make sure to send patches with
> > >a correct unified headers?
> > 
> > Alright, if thats is what you guys prefer...  (I just didn't want to
> > break the thread.)
> 
> Don't need to break the thread, just use
> 
> | --- a/hw/serial.c
> | +++ a/hw/serial.c
> 
> instead of simply 
> 
> | Index: qemu/hw/serial.c
> 
> ie the output of diff -u
> 
Oh ok, then I completely misunderstood.  (And I had no idea this makes
a difference... :)  Anyway, I'll try to remember.

> >  Anyway, I guess this is also material for the stable branch(es)?
> > (I just saw 0.11.0 has already been tagged but not announced yet, and
> > another patch merged to the same branch after that, maybe the tag can
> > still be slided if this is possible with git?)
> > 
> 
> Pushed to the branch. For the details about the release, I let Anthony
> handling that. Worst case scenario, it will be in 0.11.1.
> 
 Yeah, looks like it...

 Thanx,
	Juergen