[Qemu-devel] [PATCH v3 0/4] serial fixes, including 2.2->2.1 migration

[Qemu-devel] [PATCH v3 0/4] serial fixes, including 2.2->2.1 migration

[Paolo Bonzini]

v3 manages to separate the migration fix from subsequent semantic changes,
to make the backport as safe as possible.

Paolo Bonzini (4):
  serial: reset thri_pending on IER writes with THRI=0
  serial: clean up THRE/TEMT handling
  serial: update LSR on enabling/disabling FIFOs
  serial: only resample THR interrupt on rising edge of IER.THRI

 hw/char/serial.c | 58 ++++++++++++++++++++++++++++++++++++++------------------
 1 file changed, 40 insertions(+), 18 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH v3 1/4] serial: reset thri_pending on IER writes with THRI=0

[Paolo Bonzini]

This is responsible for failure of migration from 2.2 to 2.1, because
thr_ipending is always one in practice.

serial.c is setting thr_ipending unconditionally.  However, thr_ipending
is not used at all if THRI=0, and it will be overwritten again the next
time THRE or THRI changes.  For that reason, we can set thr_ipending to
zero every time THRI is reset.

This has no semantic change and is enough to fix migration.  It does not
change the migration format, so 2.2.0 -> 2.1 will remain broken but we
can fix 2.2.1 -> 2.1 without breaking 2.2.1 <-> 2.2.0.

There is disagreement on whether LSR.THRE should be resampled when IER.THRI
goes from 1 to 1.  Do not touch the code for now.

Cc: qemu-stable@nongnu.org
Reported-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/char/serial.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/hw/char/serial.c b/hw/char/serial.c
index ebcacdc..8c42d03 100644
--- a/hw/char/serial.c
+++ b/hw/char/serial.c
@@ -350,10 +350,24 @@ static void serial_ioport_write(void *opaque, hwaddr addr, uint64_t val,
                      s->poll_msl = 0;
                 }
             }
-            if (s->lsr & UART_LSR_THRE) {
+
+            /* Turning on the THRE interrupt on IER can trigger the interrupt
+             * if LSR.THRE=1, even if it had been masked before by reading IIR.
+             * 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.
+             *
+             * 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;
-                serial_update_irq(s);
+            } else {
+                s->thr_ipending = 0;
             }
+            serial_update_irq(s);
         }
         break;
     case 2:
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v3 1/4] serial: reset thri_pending on IER writes with THRI=0

[Dr. David Alan Gilbert]

* Paolo Bonzini (pbonzini@redhat.com) wrote:
> This is responsible for failure of migration from 2.2 to 2.1, because
> thr_ipending is always one in practice.
> 
> serial.c is setting thr_ipending unconditionally.  However, thr_ipending
> is not used at all if THRI=0, and it will be overwritten again the next
> time THRE or THRI changes.  For that reason, we can set thr_ipending to
> zero every time THRI is reset.
> 
> This has no semantic change and is enough to fix migration.  It does not
> change the migration format, so 2.2.0 -> 2.1 will remain broken but we
> can fix 2.2.1 -> 2.1 without breaking 2.2.1 <-> 2.2.0.

I can see this causes the thr_ipending to be 0 more of the time, but I don't
see why it's sufficient.

If the transmitter has just transmitted it's last byte, then thr_ipending is set
true by serial_xmit, however if there is a higher priority interrupt then
serial_update_irq would set tmp_iir to something other than THRI,
so I think serial_thr_ipending_needed  would return true and write the
subsection.

Dave

> There is disagreement on whether LSR.THRE should be resampled when IER.THRI
> goes from 1 to 1.  Do not touch the code for now.
> 
> Cc: qemu-stable@nongnu.org
> Reported-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/char/serial.c | 18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/char/serial.c b/hw/char/serial.c
> index ebcacdc..8c42d03 100644
> --- a/hw/char/serial.c
> +++ b/hw/char/serial.c
> @@ -350,10 +350,24 @@ static void serial_ioport_write(void *opaque, hwaddr addr, uint64_t val,
>                       s->poll_msl = 0;
>                  }
>              }
> -            if (s->lsr & UART_LSR_THRE) {
> +
> +            /* Turning on the THRE interrupt on IER can trigger the interrupt
> +             * if LSR.THRE=1, even if it had been masked before by reading IIR.
> +             * 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.
> +             *
> +             * 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;
> -                serial_update_irq(s);
> +            } else {
> +                s->thr_ipending = 0;
>              }
> +            serial_update_irq(s);
>          }
>          break;
>      case 2:
> -- 
> 1.8.3.1
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v3 1/4] serial: reset thri_pending on IER writes with THRI=0

[Paolo Bonzini]

On 15/12/2014 11:46, Dr. David Alan Gilbert wrote:
> I can see this causes the thr_ipending to be 0 more of the time, but I don't
> see why it's sufficient.
> 
> If the transmitter has just transmitted it's last byte, then thr_ipending is set
> true by serial_xmit, however if there is a higher priority interrupt then
> serial_update_irq would set tmp_iir to something other than THRI,
> so I think serial_thr_ipending_needed  would return true and write the
> subsection.

It's not sufficient in all cases, and it cannot be.  If we could always
reconstruct the state, the subsection would not be necessary at all.
Subsections can be used for two things:

- stuff that was not supported in the previous release, and thus would
always be zero if you can start up the machine on the older release
(with old machine type, no new devices or properties or CPU models).
This is easy, but unfortunately not what this patch does.

- stuff that was migrated wrong in the previous release.  Here, the game
to play with the "needed" and "post_load" functions is to find a
good-enough approximation of missing state from migrated state.  This is
what the thr_ipending subsection does: thr_ipending is approximated by
"is the highest-priority active interrupt the THRI interrupt?"

So, in the case you mentioned 2.1->2.1 migration would have
malfunctioned.  The THRI would have been dropped after acknowledging the
higher priority interrupt, and probably the guest driver would have hung
(at least in the case of Windows).  So, in this case 2.2->2.1 migration
should fail, which the subsection achieves.

There are other examples of this in IDE, where PIO state was not
migrated.  Very early migration (during BIOS) would then hung the VM.
The fix is to break backwards migration in this case, unfortunately you
can't have it both ways. :(

Paolo

Re: [Qemu-devel] [PATCH v3 1/4] serial: reset thri_pending on IER writes with THRI=0

[Dr. David Alan Gilbert]

* Paolo Bonzini (pbonzini@redhat.com) wrote:
> 
> 
> On 15/12/2014 11:46, Dr. David Alan Gilbert wrote:
> > I can see this causes the thr_ipending to be 0 more of the time, but I don't
> > see why it's sufficient.
> > 
> > If the transmitter has just transmitted it's last byte, then thr_ipending is set
> > true by serial_xmit, however if there is a higher priority interrupt then
> > serial_update_irq would set tmp_iir to something other than THRI,
> > so I think serial_thr_ipending_needed  would return true and write the
> > subsection.
> 
> It's not sufficient in all cases, and it cannot be.  If we could always
> reconstruct the state, the subsection would not be necessary at all.
> Subsections can be used for two things:

> 
> - stuff that was not supported in the previous release, and thus would
> always be zero if you can start up the machine on the older release
> (with old machine type, no new devices or properties or CPU models).
> This is easy, but unfortunately not what this patch does.
> 
> - stuff that was migrated wrong in the previous release.  Here, the game
> to play with the "needed" and "post_load" functions is to find a
> good-enough approximation of missing state from migrated state.  This is
> what the thr_ipending subsection does: thr_ipending is approximated by
> "is the highest-priority active interrupt the THRI interrupt?"
> 
> So, in the case you mentioned 2.1->2.1 migration would have
> malfunctioned.  The THRI would have been dropped after acknowledging the
> higher priority interrupt, and probably the guest driver would have hung
> (at least in the case of Windows).  So, in this case 2.2->2.1 migration
> should fail, which the subsection achieves.
> 
> There are other examples of this in IDE, where PIO state was not
> migrated.  Very early migration (during BIOS) would then hung the VM.
> The fix is to break backwards migration in this case, unfortunately you
> can't have it both ways. :(

I think the situation for different versions is:

a   pre-2.2   Guest could lose an interrupt, may hang serial transmit
b   2.2       2.2->2.1 fails migration most of the time
c             2.1->2.2 better chance than older since it tries to
                       reconstruct thr_ipending from iir state, but
                       can still lose interrupt
d             2.2->2.2 works
e your patch  2.2->2.1 still might fail migration if unlucky not common
f             2.1->2.2 as (c)
g             2.2<->patch works
h             patch<->patch works

Anyone who really cares about backwards migration compatibility would
probably have to guard the subsection with the machine-type to avoid
(e) ever happening (the heuristic from (c) might be useful to add).

So since (e) is an improvement:

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

although if you do have to recut it, please clarify the text that says
 'and is enough to fix migration.' since it doesn't quite.

(This is an interesting example where with a migration format that allowed
'optional' subsections you wouldn't break backwards migration if the
reader could ignore a section marked as such that it didn't recognise).

> 
> Paolo

Dave
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v3 1/4] serial: reset thri_pending on IER writes with THRI=0

[Paolo Bonzini]

On 15/12/2014 14:30, Dr. David Alan Gilbert wrote:
> Anyone who really cares about backwards migration compatibility would
> probably have to guard the subsection with the machine-type to avoid
> (e) ever happening (the heuristic from (c) might be useful to add).

Right.

> although if you do have to recut it, please clarify the text that says
>  'and is enough to fix migration.' since it doesn't quite.

Ok, I'll clarify this, specifying which case remains broken by design.

> (This is an interesting example where with a migration format that allowed
> 'optional' subsections you wouldn't break backwards migration if the
> reader could ignore a section marked as such that it didn't recognise).

Right, the question is whether you really want to do this. :)

Paolo

Re: [Qemu-devel] [PATCH v3 1/4] serial: reset thri_pending on IER writes with THRI=0

[Dr. David Alan Gilbert]

* Paolo Bonzini (pbonzini@redhat.com) wrote:
> 
> 
> On 15/12/2014 14:30, Dr. David Alan Gilbert wrote:
> > Anyone who really cares about backwards migration compatibility would
> > probably have to guard the subsection with the machine-type to avoid
> > (e) ever happening (the heuristic from (c) might be useful to add).
> 
> Right.
> 
> > although if you do have to recut it, please clarify the text that says
> >  'and is enough to fix migration.' since it doesn't quite.
> 
> Ok, I'll clarify this, specifying which case remains broken by design.
> 
> > (This is an interesting example where with a migration format that allowed
> > 'optional' subsections you wouldn't break backwards migration if the
> > reader could ignore a section marked as such that it didn't recognise).
> 
> Right, the question is whether you really want to do this. :)

Well, it would solve this type of problem less painfully, your migration
would always succeed and destinations that knew what to do with it would
always do the right thing.  Of course you could also solve the problem
if the source knew the version of the destination, but you didn't like
that idea either.

Dave

> 
> Paolo
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v3 1/4] serial: reset thri_pending on IER writes with THRI=0

[Paolo Bonzini]

On 15/12/2014 14:37, Dr. David Alan Gilbert wrote:
>> > Right, the question is whether you really want to do this. :)
> Well, it would solve this type of problem less painfully, your migration
> would always succeed and destinations that knew what to do with it would
> always do the right thing.

Yes, that's a determination to make.  I agree that this particular case
is one where losing the subsection is not the end of the world, and
keying off the machine type could be a good call for downstreams.

But that's also because downstreams can also patch older source
versions, while QEMU will not release another 2.1 version.  In fact...

> Of course you could also solve the problem
> if the source knew the version of the destination, but you didn't like
> that idea either.

... if the source could be patched by someone to know the version of the
destination, they could also add the subsection with a "needed" function
that always returns 0.  This way it is never migrated, but it is
interpreted.  You do not need the version of the destination.

Paolo

[Qemu-devel] [PATCH v3 2/4] serial: clean up THRE/TEMT handling

[Paolo Bonzini]

- assert THRE cleared and FIFO not empty (if enabled) before
sending a character.  Also assert TEMT cleared, since it is
the combination of THRE && transmitter shift register empty.

- raise THRI immediately after setting THRE

- check THRE to see if another character has to be sent,
which makes the assertions more obvious and also means TEMT
has to be set as soon as the loop ends

- clear TEMT together with THRE even in the non-FIFO case

There are certainly a couple bugfixes in here, but nothing that
squashes known bugs.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/char/serial.c | 26 ++++++++++++--------------
 1 file changed, 12 insertions(+), 14 deletions(-)

diff --git a/hw/char/serial.c b/hw/char/serial.c
index 8c42d03..4bce268 100644
--- a/hw/char/serial.c
+++ b/hw/char/serial.c
@@ -224,21 +224,23 @@ static gboolean serial_xmit(GIOChannel *chan, GIOCondition cond, void *opaque)
     SerialState *s = opaque;
 
     do {
+        assert(!(s->lsr & UART_LSR_TEMT));
+        assert(!(s->lsr & UART_LSR_THRE));
+
         if (s->tsr_retry <= 0) {
             if (s->fcr & UART_FCR_FE) {
-                if (fifo8_is_empty(&s->xmit_fifo)) {
-                    return FALSE;
-                }
+                assert(!fifo8_is_empty(&s->xmit_fifo));
                 s->tsr = fifo8_pop(&s->xmit_fifo);
                 if (!s->xmit_fifo.num) {
                     s->lsr |= UART_LSR_THRE;
                 }
-            } else if ((s->lsr & UART_LSR_THRE)) {
-                return FALSE;
             } else {
                 s->tsr = s->thr;
                 s->lsr |= UART_LSR_THRE;
-                s->lsr &= ~UART_LSR_TEMT;
+            }
+            if ((s->lsr & UART_LSR_THRE) && !s->thr_ipending) {
+                s->thr_ipending = 1;
+                serial_update_irq(s);
             }
         }
 
@@ -256,17 +258,13 @@ static gboolean serial_xmit(GIOChannel *chan, GIOCondition cond, void *opaque)
         } else {
             s->tsr_retry = 0;
         }
+
         /* Transmit another byte if it is already available. It is only
            possible when FIFO is enabled and not empty. */
-    } while ((s->fcr & UART_FCR_FE) && !fifo8_is_empty(&s->xmit_fifo));
+    } while (!(s->lsr & UART_LSR_THRE));
 
     s->last_xmit_ts = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
-
-    if (s->lsr & UART_LSR_THRE) {
-        s->lsr |= UART_LSR_TEMT;
-        s->thr_ipending = 1;
-        serial_update_irq(s);
-    }
+    s->lsr |= UART_LSR_TEMT;
 
     return FALSE;
 }
@@ -323,10 +321,10 @@ static void serial_ioport_write(void *opaque, hwaddr addr, uint64_t val,
                     fifo8_pop(&s->xmit_fifo);
                 }
                 fifo8_push(&s->xmit_fifo, s->thr);
-                s->lsr &= ~UART_LSR_TEMT;
             }
             s->thr_ipending = 0;
             s->lsr &= ~UART_LSR_THRE;
+            s->lsr &= ~UART_LSR_TEMT;
             serial_update_irq(s);
             if (s->tsr_retry <= 0) {
                 serial_xmit(NULL, G_IO_OUT, s);
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v3 2/4] serial: clean up THRE/TEMT handling

[Dr. David Alan Gilbert]

* Paolo Bonzini (pbonzini@redhat.com) wrote:
> - assert THRE cleared and FIFO not empty (if enabled) before
> sending a character.  Also assert TEMT cleared, since it is
> the combination of THRE && transmitter shift register empty.
> 
> - raise THRI immediately after setting THRE
> 
> - check THRE to see if another character has to be sent,
> which makes the assertions more obvious and also means TEMT
> has to be set as soon as the loop ends
> 
> - clear TEMT together with THRE even in the non-FIFO case
> 
> There are certainly a couple bugfixes in here, but nothing that
> squashes known bugs.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/char/serial.c | 26 ++++++++++++--------------
>  1 file changed, 12 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/char/serial.c b/hw/char/serial.c
> index 8c42d03..4bce268 100644
> --- a/hw/char/serial.c
> +++ b/hw/char/serial.c
> @@ -224,21 +224,23 @@ static gboolean serial_xmit(GIOChannel *chan, GIOCondition cond, void *opaque)
>      SerialState *s = opaque;
>  
>      do {
> +        assert(!(s->lsr & UART_LSR_TEMT));
> +        assert(!(s->lsr & UART_LSR_THRE));
> +
>          if (s->tsr_retry <= 0) {
>              if (s->fcr & UART_FCR_FE) {
> -                if (fifo8_is_empty(&s->xmit_fifo)) {
> -                    return FALSE;
> -                }
> +                assert(!fifo8_is_empty(&s->xmit_fifo));

That's undoing dslutz@verizon.com's 

dffacd46 - Fix emptyness checking

See, http://permalink.gmane.org/gmane.comp.emulators.qemu/262412
I don't think your assumptions are safe because of that qemu_chr_fe_add_watch.

Dave

>                  s->tsr = fifo8_pop(&s->xmit_fifo);
>                  if (!s->xmit_fifo.num) {
>                      s->lsr |= UART_LSR_THRE;
>                  }
> -            } else if ((s->lsr & UART_LSR_THRE)) {
> -                return FALSE;
>              } else {
>                  s->tsr = s->thr;
>                  s->lsr |= UART_LSR_THRE;
> -                s->lsr &= ~UART_LSR_TEMT;
> +            }
> +            if ((s->lsr & UART_LSR_THRE) && !s->thr_ipending) {
> +                s->thr_ipending = 1;
> +                serial_update_irq(s);
>              }
>          }
>  
> @@ -256,17 +258,13 @@ static gboolean serial_xmit(GIOChannel *chan, GIOCondition cond, void *opaque)
>          } else {
>              s->tsr_retry = 0;
>          }
> +
>          /* Transmit another byte if it is already available. It is only
>             possible when FIFO is enabled and not empty. */
> -    } while ((s->fcr & UART_FCR_FE) && !fifo8_is_empty(&s->xmit_fifo));
> +    } while (!(s->lsr & UART_LSR_THRE));
>  
>      s->last_xmit_ts = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> -
> -    if (s->lsr & UART_LSR_THRE) {
> -        s->lsr |= UART_LSR_TEMT;
> -        s->thr_ipending = 1;
> -        serial_update_irq(s);
> -    }
> +    s->lsr |= UART_LSR_TEMT;
>  
>      return FALSE;
>  }
> @@ -323,10 +321,10 @@ static void serial_ioport_write(void *opaque, hwaddr addr, uint64_t val,
>                      fifo8_pop(&s->xmit_fifo);
>                  }
>                  fifo8_push(&s->xmit_fifo, s->thr);
> -                s->lsr &= ~UART_LSR_TEMT;
>              }
>              s->thr_ipending = 0;
>              s->lsr &= ~UART_LSR_THRE;
> +            s->lsr &= ~UART_LSR_TEMT;
>              serial_update_irq(s);
>              if (s->tsr_retry <= 0) {
>                  serial_xmit(NULL, G_IO_OUT, s);
> -- 
> 1.8.3.1
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v3 2/4] serial: clean up THRE/TEMT handling

[Paolo Bonzini]

On 15/12/2014 12:40, Dr. David Alan Gilbert wrote:
>> >      do {
>> > +        assert(!(s->lsr & UART_LSR_TEMT));
>> > +        assert(!(s->lsr & UART_LSR_THRE));
>> > +
>> >          if (s->tsr_retry <= 0) {
>> >              if (s->fcr & UART_FCR_FE) {
>> > -                if (fifo8_is_empty(&s->xmit_fifo)) {
>> > -                    return FALSE;
>> > -                }
>> > +                assert(!fifo8_is_empty(&s->xmit_fifo));
> That's undoing dslutz@verizon.com's 
> 
> dffacd46 - Fix emptyness checking
> 
> See, http://permalink.gmane.org/gmane.comp.emulators.qemu/262412
> I don't think your assumptions are safe because of that qemu_chr_fe_add_watch.

I think it's safe because:

- serial_xmit is called from outside only after resetting TEMT and THRE
and pushing a character on the FIFO

- serial_xmit iterates a second time over do...while() only if the FIFO
is not empty (both before and after this patch; this patch only changes
the condition that is used)

- if qemu_chr_fe_add_watch is called, the next call will have tsr_retry
>= 1 and thus the "if" would be skipped.

Note that in the middle we had commit f702e62 (serial: change retry
logic to avoid concurrency, 2014-07-11) that fixed some messy behavior
of qemu_chr_fe_add_watch.  The commit message talks about multiple calls
to qemu_chr_fe_add_watch triggering s->tsr_retry >= MAX_XMIT_RETRY but
this is not the only possible failure.  If you have multiple calls, the
subsequent ones will see s->tsr_retry == 0 and will find (s->lsr &
UART_LSR_THRE) != 0 on entry.  But this should really never happen.

(Thanks for making me think more about it. :))

Paolo

Re: [Qemu-devel] [PATCH v3 2/4] serial: clean up THRE/TEMT handling

[Dr. David Alan Gilbert]

* Paolo Bonzini (pbonzini@redhat.com) wrote:
> 
> 
> On 15/12/2014 12:40, Dr. David Alan Gilbert wrote:
> >> >      do {
> >> > +        assert(!(s->lsr & UART_LSR_TEMT));
> >> > +        assert(!(s->lsr & UART_LSR_THRE));
> >> > +
> >> >          if (s->tsr_retry <= 0) {
> >> >              if (s->fcr & UART_FCR_FE) {
> >> > -                if (fifo8_is_empty(&s->xmit_fifo)) {
> >> > -                    return FALSE;
> >> > -                }
> >> > +                assert(!fifo8_is_empty(&s->xmit_fifo));
> > That's undoing dslutz@verizon.com's 
> > 
> > dffacd46 - Fix emptyness checking
> > 
> > See, http://permalink.gmane.org/gmane.comp.emulators.qemu/262412
> > I don't think your assumptions are safe because of that qemu_chr_fe_add_watch.
> 
> I think it's safe because:
> 
> - serial_xmit is called from outside only after resetting TEMT and THRE
> and pushing a character on the FIFO

Are you sure about TEMT? My reading of serial_ioport_write is that if
!FCR_FE then TEMT isn't cleared.

> - serial_xmit iterates a second time over do...while() only if the FIFO
> is not empty (both before and after this patch; this patch only changes
> the condition that is used)
> 
> - if qemu_chr_fe_add_watch is called, the next call will have tsr_retry
> >= 1 and thus the "if" would be skipped.
> 
> Note that in the middle we had commit f702e62 (serial: change retry
> logic to avoid concurrency, 2014-07-11) that fixed some messy behavior
> of qemu_chr_fe_add_watch.  The commit message talks about multiple calls
> to qemu_chr_fe_add_watch triggering s->tsr_retry >= MAX_XMIT_RETRY but
> this is not the only possible failure.  If you have multiple calls, the
> subsequent ones will see s->tsr_retry == 0 and will find (s->lsr &
> UART_LSR_THRE) != 0 on entry.  But this should really never happen.
> 
> (Thanks for making me think more about it. :))

Ah yes, that changed things around a lot.

Dave

> 
> Paolo
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v3 2/4] serial: clean up THRE/TEMT handling

[Paolo Bonzini]

On 15/12/2014 16:21, Dr. David Alan Gilbert wrote:
>> > 
>> > - serial_xmit is called from outside only after resetting TEMT and THRE
>> > and pushing a character on the FIFO
> Are you sure about TEMT? My reading of serial_ioport_write is that if
> !FCR_FE then TEMT isn't cleared.

This patch changes that.

Paolo

Re: [Qemu-devel] [PATCH v3 2/4] serial: clean up THRE/TEMT handling

[Dr. David Alan Gilbert]

* Paolo Bonzini (pbonzini@redhat.com) wrote:
> 
> 
> On 15/12/2014 16:21, Dr. David Alan Gilbert wrote:
> >> > 
> >> > - serial_xmit is called from outside only after resetting TEMT and THRE
> >> > and pushing a character on the FIFO
> > Are you sure about TEMT? My reading of serial_ioport_write is that if
> > !FCR_FE then TEMT isn't cleared.
> 
> This patch changes that.

Oh yeh!

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> 
> Paolo
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[Qemu-devel] [PATCH v3 3/4] serial: update LSR on enabling/disabling FIFOs

[Paolo Bonzini]

When the transmit FIFO is emptied or enabled, the transmitter
hold register is empty.  When it is disabled, it is also emptied and
in addition the previous contents of the transmitter hold register
are discarded.  In either case, the THRE bit in LSR must be set and
THRI raised.

When the receive FIFO is emptied or enabled, the data ready and break
bits must be cleared in LSR.  Likewise when the receive FIFO is disabled.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/char/serial.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/char/serial.c b/hw/char/serial.c
index 4bce268..0a6747c 100644
--- a/hw/char/serial.c
+++ b/hw/char/serial.c
@@ -377,12 +377,15 @@ static void serial_ioport_write(void *opaque, hwaddr addr, uint64_t val,
         /* FIFO clear */
 
         if (val & UART_FCR_RFR) {
+            s->lsr &= ~(UART_LSR_DR | UART_LSR_BI);
             timer_del(s->fifo_timeout_timer);
             s->timeout_ipending = 0;
             fifo8_reset(&s->recv_fifo);
         }
 
         if (val & UART_FCR_XFR) {
+            s->lsr |= UART_LSR_THRE;
+            s->thr_ipending = 1;
             fifo8_reset(&s->xmit_fifo);
         }
 
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v3 3/4] serial: update LSR on enabling/disabling FIFOs

[Dr. David Alan Gilbert]

* Paolo Bonzini (pbonzini@redhat.com) wrote:
> When the transmit FIFO is emptied or enabled, the transmitter
> hold register is empty.  When it is disabled, it is also emptied and
> in addition the previous contents of the transmitter hold register
> are discarded.  In either case, the THRE bit in LSR must be set and
> THRI raised.
> 
> When the receive FIFO is emptied or enabled, the data ready and break
> bits must be cleared in LSR.  Likewise when the receive FIFO is disabled.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/char/serial.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/hw/char/serial.c b/hw/char/serial.c
> index 4bce268..0a6747c 100644
> --- a/hw/char/serial.c
> +++ b/hw/char/serial.c
> @@ -377,12 +377,15 @@ static void serial_ioport_write(void *opaque, hwaddr addr, uint64_t val,
>          /* FIFO clear */
>  
>          if (val & UART_FCR_RFR) {
> +            s->lsr &= ~(UART_LSR_DR | UART_LSR_BI);
>              timer_del(s->fifo_timeout_timer);
>              s->timeout_ipending = 0;
>              fifo8_reset(&s->recv_fifo);
>          }
>  
>          if (val & UART_FCR_XFR) {
> +            s->lsr |= UART_LSR_THRE;
> +            s->thr_ipending = 1;
>              fifo8_reset(&s->xmit_fifo);
>          }

Doesn't that break the assertion you added in patch 2?
i.e. if I write a character, but it can't be sent, so it's added
to the tsr_retry, but before the callback I set FCR_XFR, and that
now sets LSR_THRE, then the callback triggers and it hits the
assert?

Dave

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

Re: [Qemu-devel] [PATCH v3 3/4] serial: update LSR on enabling/disabling FIFOs

[Paolo Bonzini]

On 15/12/2014 16:50, Dr. David Alan Gilbert wrote:
>> >  
>> >          if (val & UART_FCR_XFR) {
>> > +            s->lsr |= UART_LSR_THRE;
>> > +            s->thr_ipending = 1;
>> >              fifo8_reset(&s->xmit_fifo);
>> >          }
> Doesn't that break the assertion you added in patch 2?
> i.e. if I write a character, but it can't be sent, so it's added
> to the tsr_retry, but before the callback I set FCR_XFR, and that
> now sets LSR_THRE, then the callback triggers and it hits the
> assert?

You're right.  The TEMT assertion is okay, but the THRE assertion should
be inside if (s->tsr_retry <= 0).

Paolo

[Qemu-devel] [PATCH v3 4/4] serial: only resample THR interrupt on rising edge of IER.THRI

[Paolo Bonzini]

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;
             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);
@@ -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;
     case 2:
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v3 4/4] serial: only resample THR interrupt on rising edge of IER.THRI

[Dr. David Alan Gilbert]

* 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

Re: [Qemu-devel] [PATCH v3 4/4] serial: only resample THR interrupt on rising edge of IER.THRI

[Paolo Bonzini]

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