[Qemu-devel] [PATCH v2 0/4] ppc: Fix some memory leaks

[Qemu-devel] [PATCH v2 0/4] ppc: Fix some memory leaks

[Shivaprasad G Bhat]

Valgrind showed some memory leaks while running qemu-system-ppc64.
Fixing them in this series.

---

Shivaprasad G Bhat (4):
      ppc: fix memory leak in spapr_caps_add_properties
      ppc: fix memory leak in spapr_dt_drc()
      ppc: fix leak in h_client_architecture_support
      ppc: dont overwrite initialized idle_timer


 hw/ppc/spapr_caps.c  |    4 +++-
 hw/ppc/spapr_drc.c   |    7 +++++--
 hw/ppc/spapr_hcall.c |    2 ++
 target/ppc/kvm.c     |    3 ++-
 4 files changed, 12 insertions(+), 4 deletions(-)

--
Signature

[Qemu-devel] [PATCH v2 1/4] ppc: fix memory leak in spapr_caps_add_properties

[Shivaprasad G Bhat]

Free the capability name string after setting
the capability.

Signed-off-by: Shivaprasad G Bhat <sbhat@linux.ibm.com>
---
 hw/ppc/spapr_caps.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
index bbb001f84a..0263c78d69 100644
--- a/hw/ppc/spapr_caps.c
+++ b/hw/ppc/spapr_caps.c
@@ -778,7 +778,7 @@ void spapr_caps_add_properties(SpaprMachineClass *smc, Error **errp)
 
     for (i = 0; i < ARRAY_SIZE(capability_table); i++) {
         SpaprCapabilityInfo *cap = &capability_table[i];
-        const char *name = g_strdup_printf("cap-%s", cap->name);
+        char *name = g_strdup_printf("cap-%s", cap->name);
         char *desc;
 
         object_class_property_add(klass, name, cap->type,
@@ -786,11 +786,13 @@ void spapr_caps_add_properties(SpaprMachineClass *smc, Error **errp)
                                   NULL, cap, &local_err);
         if (local_err) {
             error_propagate(errp, local_err);
+            g_free(name);
             return;
         }
 
         desc = g_strdup_printf("%s", cap->description);
         object_class_property_set_description(klass, name, desc, &local_err);
+        g_free(name);
         g_free(desc);
         if (local_err) {
             error_propagate(errp, local_err);

Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 1/4] ppc: fix memory leak in spapr_caps_add_properties

[Greg Kurz]

On Wed, 17 Jul 2019 03:19:43 -0500
Shivaprasad G Bhat <sbhat@linux.ibm.com> wrote:

> Free the capability name string after setting
> the capability.
> 
> Signed-off-by: Shivaprasad G Bhat <sbhat@linux.ibm.com>
> ---

Reviewed-by: Greg Kurz <groug@kaod.org>

>  hw/ppc/spapr_caps.c |    4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
> index bbb001f84a..0263c78d69 100644
> --- a/hw/ppc/spapr_caps.c
> +++ b/hw/ppc/spapr_caps.c
> @@ -778,7 +778,7 @@ void spapr_caps_add_properties(SpaprMachineClass *smc, Error **errp)
>  
>      for (i = 0; i < ARRAY_SIZE(capability_table); i++) {
>          SpaprCapabilityInfo *cap = &capability_table[i];
> -        const char *name = g_strdup_printf("cap-%s", cap->name);
> +        char *name = g_strdup_printf("cap-%s", cap->name);
>          char *desc;
>  
>          object_class_property_add(klass, name, cap->type,
> @@ -786,11 +786,13 @@ void spapr_caps_add_properties(SpaprMachineClass *smc, Error **errp)
>                                    NULL, cap, &local_err);
>          if (local_err) {
>              error_propagate(errp, local_err);
> +            g_free(name);
>              return;
>          }
>  
>          desc = g_strdup_printf("%s", cap->description);
>          object_class_property_set_description(klass, name, desc, &local_err);
> +        g_free(name);
>          g_free(desc);
>          if (local_err) {
>              error_propagate(errp, local_err);
> 
> 

Re: [Qemu-devel] [PATCH v2 1/4] ppc: fix memory leak in spapr_caps_add_properties

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1713 bytes --]

On Wed, Jul 17, 2019 at 03:19:43AM -0500, Shivaprasad G Bhat wrote:
> Free the capability name string after setting
> the capability.
> 
> Signed-off-by: Shivaprasad G Bhat <sbhat@linux.ibm.com>

Applied to ppc-for-4.2, thanks.

> ---
>  hw/ppc/spapr_caps.c |    4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
> index bbb001f84a..0263c78d69 100644
> --- a/hw/ppc/spapr_caps.c
> +++ b/hw/ppc/spapr_caps.c
> @@ -778,7 +778,7 @@ void spapr_caps_add_properties(SpaprMachineClass *smc, Error **errp)
>  
>      for (i = 0; i < ARRAY_SIZE(capability_table); i++) {
>          SpaprCapabilityInfo *cap = &capability_table[i];
> -        const char *name = g_strdup_printf("cap-%s", cap->name);
> +        char *name = g_strdup_printf("cap-%s", cap->name);
>          char *desc;
>  
>          object_class_property_add(klass, name, cap->type,
> @@ -786,11 +786,13 @@ void spapr_caps_add_properties(SpaprMachineClass *smc, Error **errp)
>                                    NULL, cap, &local_err);
>          if (local_err) {
>              error_propagate(errp, local_err);
> +            g_free(name);
>              return;
>          }
>  
>          desc = g_strdup_printf("%s", cap->description);
>          object_class_property_set_description(klass, name, desc, &local_err);
> +        g_free(name);
>          g_free(desc);
>          if (local_err) {
>              error_propagate(errp, local_err);
> 

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[Qemu-devel] [PATCH v2 2/4] ppc: fix memory leak in spapr_dt_drc()

[Shivaprasad G Bhat]

Leaking the drc_name while preparing the DT properties.
Fixing that.

Also, remove the const qualifier from spapr_drc_name().

Signed-off-by: Shivaprasad G Bhat <sbhat@linux.ibm.com>
---
 hw/ppc/spapr_drc.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index bacadfcac5..695a0b2285 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -226,7 +226,7 @@ static uint32_t drc_set_unusable(SpaprDrc *drc)
     return RTAS_OUT_SUCCESS;
 }
 
-static const char *spapr_drc_name(SpaprDrc *drc)
+static char *spapr_drc_name(SpaprDrc *drc)
 {
     SpaprDrcClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
 
@@ -827,6 +827,7 @@ int spapr_dt_drc(void *fdt, int offset, Object *owner, uint32_t drc_type_mask)
         Object *obj;
         SpaprDrc *drc;
         SpaprDrcClass *drck;
+        char *drc_name = NULL;
         uint32_t drc_index, drc_power_domain;
 
         if (!strstart(prop->type, "link<", NULL)) {
@@ -856,8 +857,10 @@ int spapr_dt_drc(void *fdt, int offset, Object *owner, uint32_t drc_type_mask)
         g_array_append_val(drc_power_domains, drc_power_domain);
 
         /* ibm,drc-names */
-        drc_names = g_string_append(drc_names, spapr_drc_name(drc));
+        drc_name = spapr_drc_name(drc);
+        drc_names = g_string_append(drc_names, drc_name);
         drc_names = g_string_insert_len(drc_names, -1, "\0", 1);
+        g_free(drc_name);
 
         /* ibm,drc-types */
         drc_types = g_string_append(drc_types, drck->typename);

Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 2/4] ppc: fix memory leak in spapr_dt_drc()

[Greg Kurz]

On Wed, 17 Jul 2019 03:20:01 -0500
Shivaprasad G Bhat <sbhat@linux.ibm.com> wrote:

> Leaking the drc_name while preparing the DT properties.
> Fixing that.
> 
> Also, remove the const qualifier from spapr_drc_name().
> 
> Signed-off-by: Shivaprasad G Bhat <sbhat@linux.ibm.com>
> ---
>  hw/ppc/spapr_drc.c |    7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index bacadfcac5..695a0b2285 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -226,7 +226,7 @@ static uint32_t drc_set_unusable(SpaprDrc *drc)
>      return RTAS_OUT_SUCCESS;
>  }
>  
> -static const char *spapr_drc_name(SpaprDrc *drc)
> +static char *spapr_drc_name(SpaprDrc *drc)
>  {
>      SpaprDrcClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
>  
> @@ -827,6 +827,7 @@ int spapr_dt_drc(void *fdt, int offset, Object *owner, uint32_t drc_type_mask)
>          Object *obj;
>          SpaprDrc *drc;
>          SpaprDrcClass *drck;
> +        char *drc_name = NULL;
>          uint32_t drc_index, drc_power_domain;
>  
>          if (!strstart(prop->type, "link<", NULL)) {
> @@ -856,8 +857,10 @@ int spapr_dt_drc(void *fdt, int offset, Object *owner, uint32_t drc_type_mask)
>          g_array_append_val(drc_power_domains, drc_power_domain);
>  
>          /* ibm,drc-names */
> -        drc_names = g_string_append(drc_names, spapr_drc_name(drc));
> +        drc_name = spapr_drc_name(drc);
> +        drc_names = g_string_append(drc_names, drc_name);
>          drc_names = g_string_insert_len(drc_names, -1, "\0", 1);
> +        g_free(drc_name);
>  

This could even be called just after g_string_append(). No big deal.

Reviewed-by: Greg Kurz <groug@kaod.org>

>          /* ibm,drc-types */
>          drc_types = g_string_append(drc_types, drck->typename);
> 
> 

Re: [Qemu-devel] [PATCH v2 2/4] ppc: fix memory leak in spapr_dt_drc()

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1975 bytes --]

On Wed, Jul 17, 2019 at 03:20:01AM -0500, Shivaprasad G Bhat wrote:
> Leaking the drc_name while preparing the DT properties.
> Fixing that.
> 
> Also, remove the const qualifier from spapr_drc_name().
> 
> Signed-off-by: Shivaprasad G Bhat <sbhat@linux.ibm.com>

Applied to ppc-for-4.2, thanks.

> ---
>  hw/ppc/spapr_drc.c |    7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index bacadfcac5..695a0b2285 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -226,7 +226,7 @@ static uint32_t drc_set_unusable(SpaprDrc *drc)
>      return RTAS_OUT_SUCCESS;
>  }
>  
> -static const char *spapr_drc_name(SpaprDrc *drc)
> +static char *spapr_drc_name(SpaprDrc *drc)
>  {
>      SpaprDrcClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
>  
> @@ -827,6 +827,7 @@ int spapr_dt_drc(void *fdt, int offset, Object *owner, uint32_t drc_type_mask)
>          Object *obj;
>          SpaprDrc *drc;
>          SpaprDrcClass *drck;
> +        char *drc_name = NULL;
>          uint32_t drc_index, drc_power_domain;
>  
>          if (!strstart(prop->type, "link<", NULL)) {
> @@ -856,8 +857,10 @@ int spapr_dt_drc(void *fdt, int offset, Object *owner, uint32_t drc_type_mask)
>          g_array_append_val(drc_power_domains, drc_power_domain);
>  
>          /* ibm,drc-names */
> -        drc_names = g_string_append(drc_names, spapr_drc_name(drc));
> +        drc_name = spapr_drc_name(drc);
> +        drc_names = g_string_append(drc_names, drc_name);
>          drc_names = g_string_insert_len(drc_names, -1, "\0", 1);
> +        g_free(drc_name);
>  
>          /* ibm,drc-types */
>          drc_types = g_string_append(drc_types, drck->typename);
> 

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[Qemu-devel] [PATCH v2 3/4] ppc: fix leak in h_client_architecture_support

[Shivaprasad G Bhat]

Free all SpaprOptionVector local pointers after use.

Signed-off-by: Shivaprasad G Bhat <sbhat@linux.ibm.com>
---
 hw/ppc/spapr_hcall.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index 6808d4cda8..71cfe7c41d 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -1612,6 +1612,7 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
     ov5_updates = spapr_ovec_new();
     spapr->cas_reboot = spapr_ovec_diff(ov5_updates,
                                         ov5_cas_old, spapr->ov5_cas);
+    spapr_ovec_cleanup(ov5_cas_old);
     /* Now that processing is finished, set the radix/hash bit for the
      * guest if it requested a valid mode; otherwise terminate the boot. */
     if (guest_radix) {
@@ -1629,6 +1630,7 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
     }
     spapr->cas_legacy_guest_workaround = !spapr_ovec_test(ov1_guest,
                                                           OV1_PPC_3_00);
+    spapr_ovec_cleanup(ov1_guest);
     if (!spapr->cas_reboot) {
         /* If spapr_machine_reset() did not set up a HPT but one is necessary
          * (because the guest isn't going to use radix) then set it up here. */

Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 3/4] ppc: fix leak in h_client_architecture_support

[Greg Kurz]

On Wed, 17 Jul 2019 03:20:31 -0500
Shivaprasad G Bhat <sbhat@linux.ibm.com> wrote:

> Free all SpaprOptionVector local pointers after use.
> 
> Signed-off-by: Shivaprasad G Bhat <sbhat@linux.ibm.com>
> ---

Reviewed-by: Greg Kurz <groug@kaod.org>

>  hw/ppc/spapr_hcall.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index 6808d4cda8..71cfe7c41d 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -1612,6 +1612,7 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
>      ov5_updates = spapr_ovec_new();
>      spapr->cas_reboot = spapr_ovec_diff(ov5_updates,
>                                          ov5_cas_old, spapr->ov5_cas);
> +    spapr_ovec_cleanup(ov5_cas_old);
>      /* Now that processing is finished, set the radix/hash bit for the
>       * guest if it requested a valid mode; otherwise terminate the boot. */
>      if (guest_radix) {
> @@ -1629,6 +1630,7 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
>      }
>      spapr->cas_legacy_guest_workaround = !spapr_ovec_test(ov1_guest,
>                                                            OV1_PPC_3_00);
> +    spapr_ovec_cleanup(ov1_guest);
>      if (!spapr->cas_reboot) {
>          /* If spapr_machine_reset() did not set up a HPT but one is necessary
>           * (because the guest isn't going to use radix) then set it up here. */
> 
> 

Re: [Qemu-devel] [PATCH v2 3/4] ppc: fix leak in h_client_architecture_support

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1643 bytes --]

On Wed, Jul 17, 2019 at 03:20:31AM -0500, Shivaprasad G Bhat wrote:
> Free all SpaprOptionVector local pointers after use.
> 
> Signed-off-by: Shivaprasad G Bhat <sbhat@linux.ibm.com>

Applied to ppc-for-4.2, thanks.

> ---
>  hw/ppc/spapr_hcall.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index 6808d4cda8..71cfe7c41d 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -1612,6 +1612,7 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
>      ov5_updates = spapr_ovec_new();
>      spapr->cas_reboot = spapr_ovec_diff(ov5_updates,
>                                          ov5_cas_old, spapr->ov5_cas);
> +    spapr_ovec_cleanup(ov5_cas_old);
>      /* Now that processing is finished, set the radix/hash bit for the
>       * guest if it requested a valid mode; otherwise terminate the boot. */
>      if (guest_radix) {
> @@ -1629,6 +1630,7 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
>      }
>      spapr->cas_legacy_guest_workaround = !spapr_ovec_test(ov1_guest,
>                                                            OV1_PPC_3_00);
> +    spapr_ovec_cleanup(ov1_guest);
>      if (!spapr->cas_reboot) {
>          /* If spapr_machine_reset() did not set up a HPT but one is necessary
>           * (because the guest isn't going to use radix) then set it up here. */
> 

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Shivaprasad G Bhat]

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 <sbhat@linux.ibm.com>
---
 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);
 
     switch (cenv->mmu_model) {
     case POWERPC_MMU_BOOKE206:

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 <sbhat@linux.ibm.com> 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 <sbhat@linux.ibm.com>
> ---
>  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:
> 
>