Re: [Qemu-devel] [RFC PATCH v3 07/49] kvmapic: fixing loading vmstate

[Paolo Bonzini <pbonzini@redhat.com>]

Il 31/07/2014 17:21, Pavel Dovgalyuk ha scritto:
> Pre load is necessary, because we switched off resetting VM while
> loading in the replay mode.

Then you should not add it now, but rather when you add replay.  Treat
this part of the series as merely fixing migration bugs.  In fact, I
suggest that you start by progressively refining these patches.  You can
also post the other patches that I mentioned were good to go in my
review of v1.  Then pass to the next steps (in the meanwhile, Fred's
icount reverse-exec patches might get merged too).

Related to this: do _not_ assume that people review the entire series.
It's normal to notice that the initial part should stand on its own
legs, and then review it as if the remaining patches do not exist!  It's
up to _you_, in the commit messages, to annotate things that you do now
for the sake of future patches.

> Calling reset handlers generates irqs, that
> make loading process non-deterministic.

What irqs are these, and why do they make the loading process
non-deterministic?  Is it important that they not be generated, as long
as the final state is deterministic?

Have you audited all subsections and add a pre-load function there?
Where do you do this in the series?  These non-mechanical, sweeping
changes suggest to me that you find a way to keep resets during load.
Compare for example the patches at

   http://lists.gnu.org/archive/html/qemu-devel/2013-09/msg00477.html

with the series at

   http://lists.gnu.org/archive/html/qemu-devel/2014-06/msg02652.html
   http://lists.gnu.org/archive/html/qemu-devel/2014-07/msg03934.html

The former adds 230 lines of code and adds code to 40-odd files.

The latter includes a preparatory part that is complicated but only
touches 4 files, handling of the odd cases that touches about 10 files,
and the final sweeping change that is mechanical and hardly requires
review.  Overall it adds ~20 lines of code, and adds actual
functionality in addition to fixing the same bug as the first!

Paolo