On Fri, Mar 25, 2016 at 10:13:59AM +0100, Greg Kurz wrote: > Hi Laurent, > > On Thu, 24 Mar 2016 09:41:59 +0100 > Laurent Vivier <lviv...@redhat.com> wrote: > > > On 24/03/2016 06:35, David Gibson wrote: > > > On Tue, Mar 22, 2016 at 05:33:45PM +0100, Laurent Vivier wrote: > > >> Hi David, > > >> > > >> using kvm-unit-tests, I've found a side effect of your patches: the MSR > > >> is cleared (and perhaps some others). > > >> > > >> I was trying to test my patch on top of QEMU master: > > >> > > >> "ppc64: set MSR_SF bit" > > >> http://patchwork.ozlabs.org/patch/598198/ > > >> > > >> and it was not working anymore. > > >> > > >> By bisecting, I've found this commit. > > >> > > >> I think "cpu_synchronize_state()" in "ppc_hash64_set_external_hpt()" > > >> restores the MSR from KVM whereas the one from QEMU has not been saved, > > >> because cpu_synchronize_all_post_reset() is called later. > > >> > > >> So it is cleared. > > >> > > >> You can test this by applying the MSR_SF patch and using the "emulator" > > >> test of kvm-unit-tests (the "emulator: 64bit" test case) > > > > > > Ugh, you're right of course. But, I'm having a bit of trouble > > > figuring out how to fix it propertly. > > > > Perhaps you can just remove the cpu_synchronize_state()? > > > > As this is in the reset phase (spapr_cpu_reset()), I think the content > > of the QEMU side registers are correct, and they will be synchronized at > > the end of the reset phase. > > > > I think this is right because qemu_system_reset() is called either: > - during system startup: > > cpu_synchronize_all_post_init(); => push regs to KVM, kvm_vcpu_dirty = > false > ... > qemu_system_reset(VMRESET_SILENT); > > QEMU still have good values for the registers though, since KVM hasn't run > yet. > > But ppc_hash64_set_external_hpt()->cpu_synchronize_state() will indeed pull > the registers from KVM and clear MSR_SF, since we have kvm_vcpu_dirty == > false. > > - or from main_loop_should_exit() and we have: > > cpu_synchronize_all_states(); > qemu_system_reset(VMRESET_REPORT); > and > cpu_synchronize_all_states(); > qemu_system_reset(VMRESET_SILENT); > > In which case ppc_hash64_set_external_hpt()->cpu_synchronize_state() isn't > needed. > > Makes sense ?
Ugh. So, I think this is the simplest short term fix, and I've
applied a change to ppc-for-2.6 removing the cpu_synchronize_state().
But, longer term I don't think this is really right. Removing the
cpu_synchronize_state() corrects using this in the reset path, but
breaks it if it is used outside the reset path. At the moment we
don't do that, but I have code which will do so in my HPT resizing
branch. Obviously I can fix that by putting cpu_synchronize_state()
in the caller, but it still seems a bit odd having a put_sregs().
Really, it seems to me that kvm_vcpu_dirty should be set to true
(either via cpu_synchronize_state() or directly) *before* the core CPU
reset path, in the same way that it is set to true in kvm_init_vcpu().
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
