Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 4/4] ppc: dont overwrite initialized idle_timer

[Greg Kurz]

On Wed, 17 Jul 2019 03:20:55 -0500
Shivaprasad G Bhat <address@hidden> wrote:

> The check to see if the idle_timer is already initialized is
> missing. Every vcpu thread would call kvm_arch_init_vcpu()
> and overwrite the idle_timer resulting in a memory leak.
> Patch fixes that.
> 
> Signed-off-by: Shivaprasad G Bhat <address@hidden>
> ---
>  target/ppc/kvm.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index 8a06d3171e..498ca6d53b 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -491,7 +491,8 @@ int kvm_arch_init_vcpu(CPUState *cs)
>          return ret;
>      }
>  
> -    idle_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, kvm_kick_cpu, cpu);
> +    if (!idle_timer)
> +        idle_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, kvm_kick_cpu, cpu);
>  

This timer is a per-cpu thingy, but it is stored in a global :-\ which
means it is broken if there's more than one vcpu.

Also timer_new_*() aren't the preferred way to create timers as stated
in the header of timer_new_full():

/*
...
 * This is not the preferred interface unless you know you
 * are going to call timer_free. Use timer_init or timer_init_full instead.
...
*/

I think you'd rather add a QEMUTimer idle_timer field to PowerPCCPU and
call initialize it with timer_init() instead.

>      switch (cenv->mmu_model) {
>      case POWERPC_MMU_BOOKE206:
> 
>