| Message ID | 1459482767-12573-1-git-send-email-david@gibson.dropbear.id.au (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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(); 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 {
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... > > 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 { >
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 { > > >
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 {
ppc_hash64_set_external_hpt() was added in e5c0d3c "target-ppc: Add helpers for updating a CPU's SDR1 and external HPT". This helper contains a cpu_synchronize_state() since it may need to push state back to KVM afterwards. This turns out to break things when it is used in the reset path, which is the only current user. It appears that kvm_vcpu_dirty is not being set early in the reset path, so the cpu_synchronize_state() is clobbering state set up by the early part of the cpu reset path with stale state from KVM. To fix this, remove the cpu_synchronize_state() from ppc_hash64_set_external_hpt(). Any future non-reset-path users will need to manually invoke cpu_synchronize_state(). Reported-by: Laurent Vivier <lvivier@redhat.com> Signed-off-by: David Gibson <david@gibson.dropbear.id.au> --- target-ppc/mmu-hash64.c | 2 -- 1 file changed, 2 deletions(-) Paolo, Peter, 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?