[Qemu-devel] [PATCH, RFC] mips: fix cpu_reset memory leak

[Qemu-devel] [PATCH, RFC] mips: fix cpu_reset memory leak

[Blue Swirl]

Both mmu_init() and mvp_init() allocate structures, so call cpu_mips_register
only when creating a CPU.

In addition, maybe some of the some of the field initialization stuff
in  cpu_mips_register, mmu_init, mvp_init, fpu_init etc. should be
moved to cpu_reset instead, in case the fields should be reset to
original values during CPU reset. Maximally only the env->mvp etc.
structure allocation would be left to cpu_mips_register. This is the
minimal version, but it may be incorrect. Comments?

Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
---
 target-mips/translate.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/target-mips/translate.c b/target-mips/translate.c
index 58f483f..738efb7 100644
--- a/target-mips/translate.c
+++ b/target-mips/translate.c
@@ -8601,6 +8601,7 @@ CPUMIPSState *cpu_mips_init (const char *cpu_model)

     cpu_exec_init(env);
     env->cpu_model_str = cpu_model;
+    cpu_mips_register(env, def);
     mips_tcg_init();
     cpu_reset(env);
     qemu_init_vcpu(env);
@@ -8654,7 +8655,6 @@ void cpu_reset (CPUMIPSState *env)
     env->hflags = MIPS_HFLAG_CP0;
 #endif
     env->exception_index = EXCP_NONE;
-    cpu_mips_register(env, env->cpu_model);
 }

 void gen_pc_load(CPUState *env, TranslationBlock *tb,
-- 
1.6.2.4

Re: [Qemu-devel] [PATCH, RFC] mips: fix cpu_reset memory leak

[Aurelien Jarno]

On Sun, Nov 08, 2009 at 12:50:21PM +0200, Blue Swirl wrote:
> Both mmu_init() and mvp_init() allocate structures, so call cpu_mips_register
> only when creating a CPU.
> 
> In addition, maybe some of the some of the field initialization stuff
> in  cpu_mips_register, mmu_init, mvp_init, fpu_init etc. should be
> moved to cpu_reset instead, in case the fields should be reset to
> original values during CPU reset. Maximally only the env->mvp etc.
> structure allocation would be left to cpu_mips_register. This is the
> minimal version, but it may be incorrect. Comments?

There is clearly a problem with some _init functions being called at
each reset. However, your solution does not reset all the registers upon
reset. osme of the registers are read-only (e.g. CP0_PRid) so it's not a
problem, but some other are read-write (e.g. CP0_Config2). It looks like
we need more code move to fix the problem.

> Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
> ---
>  target-mips/translate.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/target-mips/translate.c b/target-mips/translate.c
> index 58f483f..738efb7 100644
> --- a/target-mips/translate.c
> +++ b/target-mips/translate.c
> @@ -8601,6 +8601,7 @@ CPUMIPSState *cpu_mips_init (const char *cpu_model)
> 
>      cpu_exec_init(env);
>      env->cpu_model_str = cpu_model;
> +    cpu_mips_register(env, def);
>      mips_tcg_init();
>      cpu_reset(env);
>      qemu_init_vcpu(env);
> @@ -8654,7 +8655,6 @@ void cpu_reset (CPUMIPSState *env)
>      env->hflags = MIPS_HFLAG_CP0;
>  #endif
>      env->exception_index = EXCP_NONE;
> -    cpu_mips_register(env, env->cpu_model);
>  }
> 
>  void gen_pc_load(CPUState *env, TranslationBlock *tb,
> -- 
> 1.6.2.4
> 
> 
> 

-- 
Aurelien Jarno	                        GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH, RFC] mips: fix cpu_reset memory leak

[Aurelien Jarno]

On Tue, Nov 10, 2009 at 04:30:59PM +0100, Aurelien Jarno wrote:
> On Sun, Nov 08, 2009 at 12:50:21PM +0200, Blue Swirl wrote:
> > Both mmu_init() and mvp_init() allocate structures, so call cpu_mips_register
> > only when creating a CPU.
> > 
> > In addition, maybe some of the some of the field initialization stuff
> > in  cpu_mips_register, mmu_init, mvp_init, fpu_init etc. should be
> > moved to cpu_reset instead, in case the fields should be reset to
> > original values during CPU reset. Maximally only the env->mvp etc.
> > structure allocation would be left to cpu_mips_register. This is the
> > minimal version, but it may be incorrect. Comments?
> 
> There is clearly a problem with some _init functions being called at
> each reset. However, your solution does not reset all the registers upon
> reset. osme of the registers are read-only (e.g. CP0_PRid) so it's not a
> problem, but some other are read-write (e.g. CP0_Config2). It looks like
> we need more code move to fix the problem.
> 

I have just pushed a patch that should fix the problem.

-- 
Aurelien Jarno	                        GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net