[Qemu-devel] Re: [PATCH 2/4] KVM: Rework VCPU state writeback API

[Marcelo Tosatti <mtosatti@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2866 bytes --]

On Tue, Mar 02, 2010 at 11:29:10PM -0300, Marcelo Tosatti wrote:
> On Tue, Mar 02, 2010 at 05:31:09PM +0100, Jan Kiszka wrote:
> > Marcelo Tosatti wrote:
> > > On Tue, Mar 02, 2010 at 09:00:04AM +0100, Jan Kiszka wrote:
> > >> Marcelo Tosatti wrote:
> > >>> On Mon, Mar 01, 2010 at 07:10:30PM +0100, Jan Kiszka wrote:
> > >>>> This grand cleanup drops all reset and vmsave/load related
> > >>>> synchronization points in favor of four(!) generic hooks:
> > >>>>
> > >>>> - cpu_synchronize_all_states in qemu_savevm_state_complete
> > >>>>   (initial sync from kernel before vmsave)
> > >>>> - cpu_synchronize_all_post_init in qemu_loadvm_state
> > >>>>   (writeback after vmload)
> > >>>> - cpu_synchronize_all_post_init in main after machine init
> > >>>> - cpu_synchronize_all_post_reset in qemu_system_reset
> > >>>>   (writeback after system reset)
> > >>>>
> > >>>> These writeback points + the existing one of VCPU exec after
> > >>>> cpu_synchronize_state map on three levels of writeback:
> > >>>>
> > >>>> - KVM_PUT_RUNTIME_STATE (during runtime, other VCPUs continue to run)
> > >>>> - KVM_PUT_RESET_STATE   (on synchronous system reset, all VCPUs stopped)
> > >>>> - KVM_PUT_FULL_STATE    (on init or vmload, all VCPUs stopped as well)
> > >>>>
> > >>>> This level is passed to the arch-specific VCPU state writing function
> > >>>> that will decide which concrete substates need to be written. That way,
> > >>>> no writer of load, save or reset functions that interact with in-kernel
> > >>>> KVM states will ever have to worry about synchronization again. That
> > >>>> also means that a lot of reasons for races, segfaults and deadlocks are
> > >>>> eliminated.
> > >>>>
> > >>>> cpu_synchronize_state remains untouched, just as Anthony suggested. We
> > >>>> continue to need it before reading or writing of VCPU states that are
> > >>>> also tracked by in-kernel KVM subsystems.
> > >>>>
> > >>>> Consequently, this patch removes many cpu_synchronize_state calls that
> > >>>> are now redundant, just like remaining explicit register syncs.
> > >>>>
> > >>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> > >>> Jan,
> > >>>
> > >>> This patch breaks system reset of WinXP.32 install (more easily
> > >>> reproducible without iothread enabled).
> > >>>
> > >>> Screenshot attached.
> > >>>
> > >> Strange - no issues with qemu-kvm? Any special command line switch? /me
> > >> goes scrounging for some installation XP32 CD in the meantime...
> > > 
> > > No issues with qemu-kvm. Could not spot anything obvious.
> > > 
> > 
> > And, of course, my WinXP installation did not trigger any reset issue,
> > even in non-iothreaded mode. :(

The regression seems to be caused by seabios commit d7e998f. Kevin, the
failure can be seen on the attached screenshot, which happens on the
first reboot of WinXP 32 installation (after copying files etc).


[-- Attachment #2: uqmaster-failure.png --]
[-- Type: image/png, Size: 4451 bytes --]