Re: [Qemu-ppc] [PATCH] target-ppc: Correct KVM synchronization for ppc_hash64_set_external_hpt()

[David Gibson]

On Mon, Apr 04, 2016 at 11:10:56AM +1000, David Gibson wrote:
> On Fri, Apr 01, 2016 at 12:28:31PM +0200, Paolo Bonzini wrote:
> > 
> > 
> > On 01/04/2016 05:52, David Gibson wrote:
> > > This seems like the right minimal fix in the qemu-2.6 timeframe to fix
> > > the actual bug.  However, longer term it seems like the correct thing
> > > to do might be to set kvm_vcpu_dirty early in the reset path.  Thoughts?
> > 
> > Isn't it done already? vl.c does:
> > 
> >         pause_all_vcpus();
> >         cpu_synchronize_all_states();
> >         qemu_system_reset(VMRESET_REPORT);
> >         resume_all_vcpus();
> 
> 
> Huh.. now I'm really confused.  Given this I would indeed have
> expected kvm_vpcu_dirty to be set in the reset path, but that would
> make the cpu_synchronize_state() causing the problem to act as a
> no-op, so it shouldn't be causing the problem.
> 
> Investigating...

Ok, I've found it.

So, if you reset the system after it's up and running, the fragment
above is indeed what will happen and I think everything will work
correct from there.

The problem occurs only on the very first reset in main() (vl.c:4624
as of de1d099): kvm_vcpu_dirty is false during this call.
Specifically, although kvm_vcpu_dirty was initialized to true in
kvm_vcpu_init(), it gets set to false in cpu_synchronize_post_init()
and never set true again before the qemu_system_reset().

Seems to me we either need a cpu_synchronize_all_states() before that
qemu_system_reset(), or we need to fold the
cpu_synchronize_all_states() right into qemu_system_reset() itself.

Opinion on which option is preferred?

> 
> > 
> > Thanks,
> > 
> > Paolo
> > 
> > > diff --git a/target-ppc/mmu-hash64.c b/target-ppc/mmu-hash64.c
> > > index 72c4ab5..caf41ce 100644
> > > --- a/target-ppc/mmu-hash64.c
> > > +++ b/target-ppc/mmu-hash64.c
> > > @@ -283,8 +283,6 @@ void ppc_hash64_set_external_hpt(PowerPCCPU *cpu, 
> > > void *hpt, int shift,
> > >      CPUPPCState *env = &cpu->env;
> > >      Error *local_err = NULL;
> > >  
> > > -    cpu_synchronize_state(CPU(cpu));
> > > -
> > >      if (hpt) {
> > >          env->external_htab = hpt;
> > >      } else {
> > 
> 



-- 
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