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

[Dr. David Alan Gilbert]

* Paolo Bonzini (address@hidden) 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: address@hidden
> Reported-by: Igor Mammedov <address@hidden>
> Signed-off-by: Paolo Bonzini <address@hidden>
> ---
>  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 / address@hidden / Manchester, UK