From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52393) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Y0Y9h-0005So-1i for qemu-devel@nongnu.org; Mon, 15 Dec 2014 11:05:59 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Y0Y9b-0004Zj-EN for qemu-devel@nongnu.org; Mon, 15 Dec 2014 11:05:53 -0500 Received: from mx1.redhat.com ([209.132.183.28]:39439) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Y0Y9a-0004ZU-Oq for qemu-devel@nongnu.org; Mon, 15 Dec 2014 11:05:47 -0500 Date: Mon, 15 Dec 2014 16:05:38 +0000 From: "Dr. David Alan Gilbert" Message-ID: <20141215160538.GJ5502@work-vm> References: <1418388243-1886-1-git-send-email-pbonzini@redhat.com> <1418388243-1886-5-git-send-email-pbonzini@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1418388243-1886-5-git-send-email-pbonzini@redhat.com> Subject: Re: [Qemu-devel] [PATCH v3 4/4] serial: only resample THR interrupt on rising edge of IER.THRI List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: imammedo@redhat.com, andrey@xdel.ru, qemu-devel@nongnu.org, batuzovk@ispras.ru * 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 > Signed-off-by: Paolo Bonzini > --- > 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