Re: [Qemu-devel] [Qemu-ppc] [PATCHv2 2/3] target-ppc: Add helpers for updating a CPU's SDR1 and external HPT

[David Gibson]

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