Re: [Qemu-devel] [PATCH v2 03/20] target-ppc: move back cpu_exec_init() to init

[Laurent Vivier]

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 
>>> CC: Alexander Graf 
>>> CC: qemu-...@nongnu.org
>>> Signed-off-by: Laurent Vivier 
>>> ---
>>>  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, _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

Re: [Qemu-devel] [PATCH v2 03/20] target-ppc: move back cpu_exec_init() to init

[Bharata B Rao]

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 
> > CC: Alexander Graf 
> > CC: qemu-...@nongnu.org
> > Signed-off-by: Laurent Vivier 
> > ---
> >  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, _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.

Re: [Qemu-devel] [PATCH v2 03/20] target-ppc: move back cpu_exec_init() to init

[Laurent Vivier]

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 
>> CC: Alexander Graf 
>> CC: qemu-...@nongnu.org
>> Signed-off-by: Laurent Vivier 
>> ---
>>  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, _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

Re: [Qemu-devel] [PATCH v2 03/20] target-ppc: move back cpu_exec_init() to init

[David Gibson]

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 
> CC: Alexander Graf 
> CC: qemu-...@nongnu.org
> Signed-off-by: Laurent Vivier 
> ---
>  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, _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] == _handler) {
>  continue;
> @@ -10435,6 +10432,7 @@ static void ppc_cpu_initfn(Object *obj)
>  CPUPPCState *env = >env;
>  
>  cs->env_ptr = env;
> +cpu_exec_init(cs);
>  
>  env->msr_mask = pcc->msr_mask;
>  env->mmu_model = pcc->mmu_model;

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

[Qemu-devel] [PATCH v2 03/20] target-ppc: move back cpu_exec_init() to init

[Laurent Vivier]

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 
CC: Alexander Graf 
CC: qemu-...@nongnu.org
Signed-off-by: Laurent Vivier 
---
 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, _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] == _handler) {
 continue;
@@ -10435,6 +10432,7 @@ static void ppc_cpu_initfn(Object *obj)
 CPUPPCState *env = >env;
 
 cs->env_ptr = env;
+cpu_exec_init(cs);
 
 env->msr_mask = pcc->msr_mask;
 env->mmu_model = pcc->mmu_model;
-- 
2.7.4