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:
> 
> 
> 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 <address@hidden>

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 / address@hidden / Manchester, UK