| Message ID | 1476375902-11715-4-git-send-email-lvivier@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Oct 13, 2016 at 06:24:45PM +0200, Laurent Vivier wrote: > We have now the cpu_exec_realize() in realize, > so the init part must be in init. > > As cpu_exec_unrealize() is called from cpu_common_finalize(), > remove the call from ppc_cpu_unrealizefn(). > > CC: Bharata B Rao <bharata@linux.vnet.ibm.com> > CC: Alexander Graf <agraf@suse.de> > CC: qemu-ppc@nongnu.org > Signed-off-by: Laurent Vivier <lvivier@redhat.com> > --- > target-ppc/translate_init.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c > index 094f28a..bbca8b5 100644 > --- a/target-ppc/translate_init.c > +++ b/target-ppc/translate_init.c > @@ -9678,7 +9678,6 @@ static void ppc_cpu_realizefn(DeviceState *dev, Error **errp) > } > #endif > > - cpu_exec_init(cs); > cpu_exec_realize(cs, &local_err); > if (local_err != NULL) { > error_propagate(errp, local_err); > @@ -9911,8 +9910,6 @@ static void ppc_cpu_unrealizefn(DeviceState *dev, Error **errp) > opc_handler_t **table, **table_2; > int i, j, k; > > - cpu_exec_unrealize(CPU(dev)); > - This doesn't seem right. As you said in 0/20, cpu_exec_unrealize() is called from cpu_common_finalize(). But finalize should mirror init, not unrealize(). So it seems that unrealize() really should belong here, not in finalize. > for (i = 0; i < PPC_CPU_OPCODES_LEN; i++) { > if (env->opcodes[i] == &invalid_handler) { > continue; > @@ -10435,6 +10432,7 @@ static void ppc_cpu_initfn(Object *obj) > CPUPPCState *env = &cpu->env; > > cs->env_ptr = env; > + cpu_exec_init(cs); > > env->msr_mask = pcc->msr_mask; > env->mmu_model = pcc->mmu_model;
On 14/10/2016 06:07, David Gibson wrote: > On Thu, Oct 13, 2016 at 06:24:45PM +0200, Laurent Vivier wrote: >> We have now the cpu_exec_realize() in realize, >> so the init part must be in init. >> >> As cpu_exec_unrealize() is called from cpu_common_finalize(), >> remove the call from ppc_cpu_unrealizefn(). >> >> CC: Bharata B Rao <bharata@linux.vnet.ibm.com> >> CC: Alexander Graf <agraf@suse.de> >> CC: qemu-ppc@nongnu.org >> Signed-off-by: Laurent Vivier <lvivier@redhat.com> >> --- >> target-ppc/translate_init.c | 4 +--- >> 1 file changed, 1 insertion(+), 3 deletions(-) >> >> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c >> index 094f28a..bbca8b5 100644 >> --- a/target-ppc/translate_init.c >> +++ b/target-ppc/translate_init.c >> @@ -9678,7 +9678,6 @@ static void ppc_cpu_realizefn(DeviceState *dev, Error **errp) >> } >> #endif >> >> - cpu_exec_init(cs); >> cpu_exec_realize(cs, &local_err); >> if (local_err != NULL) { >> error_propagate(errp, local_err); >> @@ -9911,8 +9910,6 @@ static void ppc_cpu_unrealizefn(DeviceState *dev, Error **errp) >> opc_handler_t **table, **table_2; >> int i, j, k; >> >> - cpu_exec_unrealize(CPU(dev)); >> - > > This doesn't seem right. As you said in 0/20, cpu_exec_unrealize() is > called from cpu_common_finalize(). But finalize should mirror init, > not unrealize(). So it seems that unrealize() really should belong > here, not in finalize. OK, I was not sure for this part. So I guess I have to add an cpu_common_unrealize(). Thanks, Laurent
On Fri, Oct 14, 2016 at 03:07:51PM +1100, David Gibson wrote: > On Thu, Oct 13, 2016 at 06:24:45PM +0200, Laurent Vivier wrote: > > We have now the cpu_exec_realize() in realize, > > so the init part must be in init. > > > > As cpu_exec_unrealize() is called from cpu_common_finalize(), > > remove the call from ppc_cpu_unrealizefn(). > > > > CC: Bharata B Rao <bharata@linux.vnet.ibm.com> > > CC: Alexander Graf <agraf@suse.de> > > CC: qemu-ppc@nongnu.org > > Signed-off-by: Laurent Vivier <lvivier@redhat.com> > > --- > > target-ppc/translate_init.c | 4 +--- > > 1 file changed, 1 insertion(+), 3 deletions(-) > > > > diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c > > index 094f28a..bbca8b5 100644 > > --- a/target-ppc/translate_init.c > > +++ b/target-ppc/translate_init.c > > @@ -9678,7 +9678,6 @@ static void ppc_cpu_realizefn(DeviceState *dev, Error **errp) > > } > > #endif > > > > - cpu_exec_init(cs); > > cpu_exec_realize(cs, &local_err); > > if (local_err != NULL) { > > error_propagate(errp, local_err); > > @@ -9911,8 +9910,6 @@ static void ppc_cpu_unrealizefn(DeviceState *dev, Error **errp) > > opc_handler_t **table, **table_2; > > int i, j, k; > > > > - cpu_exec_unrealize(CPU(dev)); > > - > > This doesn't seem right. As you said in 0/20, cpu_exec_unrealize() is > called from cpu_common_finalize(). But finalize should mirror init, > not unrealize(). So it seems that unrealize() really should belong > here, not in finalize. For archs like PowerPC, cpu_exec_exit() was being called twice: once from PowerPC CPU's unrealize function and once from cpu_common_finalize(). cpu_exec_exit() had two vmstate_unregister() calls and it used to ensure that they are not called twice, but looks like this got changed sometime back and we are now executing these two vmstate_unregister() calls twice. While you are here, could you please take care of this ? Regards, Bharata.
On 14/10/2016 16:49, Bharata B Rao wrote: > On Fri, Oct 14, 2016 at 03:07:51PM +1100, David Gibson wrote: >> On Thu, Oct 13, 2016 at 06:24:45PM +0200, Laurent Vivier wrote: >>> We have now the cpu_exec_realize() in realize, >>> so the init part must be in init. >>> >>> As cpu_exec_unrealize() is called from cpu_common_finalize(), >>> remove the call from ppc_cpu_unrealizefn(). >>> >>> CC: Bharata B Rao <bharata@linux.vnet.ibm.com> >>> CC: Alexander Graf <agraf@suse.de> >>> CC: qemu-ppc@nongnu.org >>> Signed-off-by: Laurent Vivier <lvivier@redhat.com> >>> --- >>> target-ppc/translate_init.c | 4 +--- >>> 1 file changed, 1 insertion(+), 3 deletions(-) >>> >>> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c >>> index 094f28a..bbca8b5 100644 >>> --- a/target-ppc/translate_init.c >>> +++ b/target-ppc/translate_init.c >>> @@ -9678,7 +9678,6 @@ static void ppc_cpu_realizefn(DeviceState *dev, Error **errp) >>> } >>> #endif >>> >>> - cpu_exec_init(cs); >>> cpu_exec_realize(cs, &local_err); >>> if (local_err != NULL) { >>> error_propagate(errp, local_err); >>> @@ -9911,8 +9910,6 @@ static void ppc_cpu_unrealizefn(DeviceState *dev, Error **errp) >>> opc_handler_t **table, **table_2; >>> int i, j, k; >>> >>> - cpu_exec_unrealize(CPU(dev)); >>> - >> >> This doesn't seem right. As you said in 0/20, cpu_exec_unrealize() is >> called from cpu_common_finalize(). But finalize should mirror init, >> not unrealize(). So it seems that unrealize() really should belong >> here, not in finalize. > > For archs like PowerPC, cpu_exec_exit() was being called twice: once > from PowerPC CPU's unrealize function and once from cpu_common_finalize(). > cpu_exec_exit() had two vmstate_unregister() calls and it used to ensure > that they are not called twice, but looks like this got changed sometime > back and we are now executing these two vmstate_unregister() calls twice. > > While you are here, could you please take care of this ? Yes, I'm aware of that and taking care :) Thanks, Lauret
diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c index 094f28a..bbca8b5 100644 --- a/target-ppc/translate_init.c +++ b/target-ppc/translate_init.c @@ -9678,7 +9678,6 @@ static void ppc_cpu_realizefn(DeviceState *dev, Error **errp) } #endif - cpu_exec_init(cs); cpu_exec_realize(cs, &local_err); if (local_err != NULL) { error_propagate(errp, local_err); @@ -9911,8 +9910,6 @@ static void ppc_cpu_unrealizefn(DeviceState *dev, Error **errp) opc_handler_t **table, **table_2; int i, j, k; - cpu_exec_unrealize(CPU(dev)); - for (i = 0; i < PPC_CPU_OPCODES_LEN; i++) { if (env->opcodes[i] == &invalid_handler) { continue; @@ -10435,6 +10432,7 @@ static void ppc_cpu_initfn(Object *obj) CPUPPCState *env = &cpu->env; cs->env_ptr = env; + cpu_exec_init(cs); env->msr_mask = pcc->msr_mask; env->mmu_model = pcc->mmu_model;
We have now the cpu_exec_realize() in realize, so the init part must be in init. As cpu_exec_unrealize() is called from cpu_common_finalize(), remove the call from ppc_cpu_unrealizefn(). CC: Bharata B Rao <bharata@linux.vnet.ibm.com> CC: Alexander Graf <agraf@suse.de> CC: qemu-ppc@nongnu.org Signed-off-by: Laurent Vivier <lvivier@redhat.com> --- target-ppc/translate_init.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)