[v3,4/4] serial: only resample THR interrupt on rising edge of IER.THRI

There is disagreement on whether LSR.THRE should be resampled when
IER.THRI goes from 1 to 1.  Bochs only does it if IER.THRI goes from 0
to 1; PCE does it even if IER.THRI is unchanged.  But the Windows driver
seems to always go from 1 to 0 and back to 1, so do things in agreement
with Bochs, because the handling of thr_ipending was reported in 2010
(https://lists.gnu.org/archive/html/qemu-devel/2010-03/msg01914.html)
as breaking DR-DOS Plus.

Reported-by: Roy Tam <roytam@gmail.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/char/serial.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

* Paolo Bonzini (pbonzini@redhat.com) wrote:
> There is disagreement on whether LSR.THRE should be resampled when
> IER.THRI goes from 1 to 1.  Bochs only does it if IER.THRI goes from 0
> to 1; PCE does it even if IER.THRI is unchanged.  But the Windows driver
> seems to always go from 1 to 0 and back to 1, so do things in agreement
> with Bochs, because the handling of thr_ipending was reported in 2010
> (https://lists.gnu.org/archive/html/qemu-devel/2010-03/msg01914.html)
> as breaking DR-DOS Plus.
> 
> Reported-by: Roy Tam <roytam@gmail.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/char/serial.c | 25 ++++++++++++++++---------
>  1 file changed, 16 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/char/serial.c b/hw/char/serial.c
> index 0a6747c..5488900 100644
> --- a/hw/char/serial.c
> +++ b/hw/char/serial.c
> @@ -336,10 +336,12 @@ static void serial_ioport_write(void *opaque, hwaddr addr, uint64_t val,
>              s->divider = (s->divider & 0x00ff) | (val << 8);
>              serial_update_parameters(s);
>          } else {
> +            int changed = (s->ier ^ val) & 0x0f;

uint8_t perhaps?

>              s->ier = val & 0x0f;
>              /* If the backend device is a real serial port, turn polling of the modem
> -               status lines on physical port on or off depending on UART_IER_MSI state */
> -            if (s->poll_msl >= 0) {
> +             * status lines on physical port on or off depending on UART_IER_MSI state.
> +             */
> +            if ((changed & UART_IER_MSI) && s->poll_msl >= 0) {
>                  if (s->ier & UART_IER_MSI) {
>                       s->poll_msl = 1;
>                       serial_update_msl(s);

This MSI stuff, this change is just intended to do the same as you're
doing for THRI and making it change based?   The commit message and title
doens't mention it.

> @@ -354,18 +356,23 @@ static void serial_ioport_write(void *opaque, hwaddr addr, uint64_t val,
>               * This is not in the datasheet, but Windows relies on it.  It is
>               * unclear if THRE has to be resampled every time THRI becomes
>               * 1, or only on the rising edge.  Bochs does the latter, and Windows
> -             * always toggles IER to all zeroes and back to all ones.  But for
> -             * now leave it as it has always been in QEMU.
> +             * always toggles IER to all zeroes and back to all ones, so do the
> +             * same.
>               *
>               * If IER.THRI is zero, thr_ipending is not used.  Set it to zero
>               * so that the thr_ipending subsection is not migrated.
>               */
> -            if ((s->ier & UART_IER_THRI) && (s->lsr & UART_LSR_THRE)) {
> -                s->thr_ipending = 1;
> -            } else {
> -                s->thr_ipending = 0;
> +            if (changed & UART_IER_THRI) {
> +                if ((s->ier & UART_IER_THRI) && (s->lsr & UART_LSR_THRE)) {
> +                    s->thr_ipending = 1;
> +                } else {
> +                    s->thr_ipending = 0;
> +                }
> +            }
> +
> +            if (changed) {
> +                serial_update_irq(s);
>              }
> -            serial_update_irq(s);
>          }
>          break;

But the rest of this looks OK.

Dave
>      case 2:
> -- 
> 1.8.3.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

On 15/12/2014 17:05, Dr. David Alan Gilbert wrote:
> > +            if ((changed & UART_IER_MSI) && s->poll_msl >= 0) {
> >                  if (s->ier & UART_IER_MSI) {
> >                       s->poll_msl = 1;
> >                       serial_update_msl(s);
> 
> This MSI stuff, this change is just intended to do the same as you're
> doing for THRI and making it change based?   The commit message and title
> doens't mention it.

It has no change.  If MSI = 1, there is already a timer ticking every
100th of a second to poll the modem status, so it's useless to go
through serial_udpate_msl/timer_del/...

It's just because I have the changed bits at hand now.

BTW, looks like the poll_msl subsection after all was not necessary.  It
can be gleaned by ANDing "does CHR_IOCTL_SERIAL_GET_TIOCM work?" (which
is immutable state depending on the command-line configuration" with
IER.MSI.

Paolo