[PATCH 0/7] hw/riscv: fix leak, add more g_autofree

[PATCH 0/7] hw/riscv: fix leak, add more g_autofree

[Daniel Henrique Barboza]

Hi,

First patch fixes a leak found when using Valgrind. The root cause is a
missing g_free() in a string. 

In fact, I found while doing reviews that we keep repeating the same
pattern:

====
char *name;
name = g_strdup_printf(...);
(...)
g_free(name);
====

With this in mind, I ended up making this rather trivial series to
introduce more string/array autocleaning in the 'virt' machine code. The
advantage of doing 'g_autofree' is that we'll guarantee that we'll clean
ourselves up when the variable goes out of scope, avoiding leaks like
the one patch 1 fixes. We want to enforce this autoclean style in
reviews, and for that we need to get rid of at least some of the uses we
do it right now.

I didn't bother changing the 'spike' and the 'sifive' boards for now
because the bulk of new patches is done on top of the 'virt' machine,
so it's more important to tidy this board first.


Daniel Henrique Barboza (7):
  hw/riscv/virt-acpi-build.c: fix leak in build_rhct()
  hw/riscv/numa.c: use g_autofree in socket_fdt_write_distance_matrix()
  hw/riscv/virt.c: use g_autofree in create_fdt_socket_cpus()
  hw/riscv/virt.c: use g_autofree in create_fdt_sockets()
  hw/riscv/virt.c: use g_autofree in create_fdt_virtio()
  hw/riscv/virt.c: use g_autofree in virt_machine_init()
  hw/riscv/virt.c: use g_autofree in create_fdt_*

 hw/riscv/numa.c            |   4 +-
 hw/riscv/virt-acpi-build.c |   2 +-
 hw/riscv/virt.c            | 109 ++++++++++++-------------------------
 3 files changed, 37 insertions(+), 78 deletions(-)

-- 
2.43.0

[PATCH 1/7] hw/riscv/virt-acpi-build.c: fix leak in build_rhct()

[Daniel Henrique Barboza]

The 'isa' char pointer isn't being freed after use.

Issue detected by Valgrind:

==38752== 128 bytes in 1 blocks are definitely lost in loss record 3,190 of 3,884
==38752==    at 0x484280F: malloc (vg_replace_malloc.c:442)
==38752==    by 0x5189619: g_malloc (gmem.c:130)
==38752==    by 0x51A5BF2: g_strconcat (gstrfuncs.c:628)
==38752==    by 0x6C1E3E: riscv_isa_string_ext (cpu.c:2321)
==38752==    by 0x6C1E3E: riscv_isa_string (cpu.c:2343)
==38752==    by 0x6BD2EA: build_rhct (virt-acpi-build.c:232)
==38752==    by 0x6BD2EA: virt_acpi_build (virt-acpi-build.c:556)
==38752==    by 0x6BDC86: virt_acpi_setup (virt-acpi-build.c:662)
==38752==    by 0x9C8DC6: notifier_list_notify (notify.c:39)
==38752==    by 0x4A595A: qdev_machine_creation_done (machine.c:1589)
==38752==    by 0x61E052: qemu_machine_creation_done (vl.c:2680)
==38752==    by 0x61E052: qmp_x_exit_preconfig.part.0 (vl.c:2709)
==38752==    by 0x6220C6: qmp_x_exit_preconfig (vl.c:2702)
==38752==    by 0x6220C6: qemu_init (vl.c:3758)
==38752==    by 0x425858: main (main.c:47)

Fixes: ebfd392893 ("hw/riscv/virt: virt-acpi-build.c: Add RHCT Table")
Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 hw/riscv/virt-acpi-build.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/riscv/virt-acpi-build.c b/hw/riscv/virt-acpi-build.c
index 26c7e4482d..fb8baf64f6 100644
--- a/hw/riscv/virt-acpi-build.c
+++ b/hw/riscv/virt-acpi-build.c
@@ -196,7 +196,7 @@ static void build_rhct(GArray *table_data,
     RISCVCPU *cpu = &s->soc[0].harts[0];
     uint32_t mmu_offset = 0;
     uint8_t satp_mode_max;
-    char *isa;
+    g_autofree char *isa = NULL;
 
     AcpiTable table = { .sig = "RHCT", .rev = 1, .oem_id = s->oem_id,
                         .oem_table_id = s->oem_table_id };
-- 
2.43.0

Re: [PATCH 1/7] hw/riscv/virt-acpi-build.c: fix leak in build_rhct()

[Alistair Francis]

On Tue, Jan 23, 2024 at 8:16 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> The 'isa' char pointer isn't being freed after use.
>
> Issue detected by Valgrind:
>
> ==38752== 128 bytes in 1 blocks are definitely lost in loss record 3,190 of 3,884
> ==38752==    at 0x484280F: malloc (vg_replace_malloc.c:442)
> ==38752==    by 0x5189619: g_malloc (gmem.c:130)
> ==38752==    by 0x51A5BF2: g_strconcat (gstrfuncs.c:628)
> ==38752==    by 0x6C1E3E: riscv_isa_string_ext (cpu.c:2321)
> ==38752==    by 0x6C1E3E: riscv_isa_string (cpu.c:2343)
> ==38752==    by 0x6BD2EA: build_rhct (virt-acpi-build.c:232)
> ==38752==    by 0x6BD2EA: virt_acpi_build (virt-acpi-build.c:556)
> ==38752==    by 0x6BDC86: virt_acpi_setup (virt-acpi-build.c:662)
> ==38752==    by 0x9C8DC6: notifier_list_notify (notify.c:39)
> ==38752==    by 0x4A595A: qdev_machine_creation_done (machine.c:1589)
> ==38752==    by 0x61E052: qemu_machine_creation_done (vl.c:2680)
> ==38752==    by 0x61E052: qmp_x_exit_preconfig.part.0 (vl.c:2709)
> ==38752==    by 0x6220C6: qmp_x_exit_preconfig (vl.c:2702)
> ==38752==    by 0x6220C6: qemu_init (vl.c:3758)
> ==38752==    by 0x425858: main (main.c:47)
>
> Fixes: ebfd392893 ("hw/riscv/virt: virt-acpi-build.c: Add RHCT Table")
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/riscv/virt-acpi-build.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/riscv/virt-acpi-build.c b/hw/riscv/virt-acpi-build.c
> index 26c7e4482d..fb8baf64f6 100644
> --- a/hw/riscv/virt-acpi-build.c
> +++ b/hw/riscv/virt-acpi-build.c
> @@ -196,7 +196,7 @@ static void build_rhct(GArray *table_data,
>      RISCVCPU *cpu = &s->soc[0].harts[0];
>      uint32_t mmu_offset = 0;
>      uint8_t satp_mode_max;
> -    char *isa;
> +    g_autofree char *isa = NULL;
>
>      AcpiTable table = { .sig = "RHCT", .rev = 1, .oem_id = s->oem_id,
>                          .oem_table_id = s->oem_table_id };
> --
> 2.43.0
>
>

[PATCH 2/7] hw/riscv/numa.c: use g_autofree in socket_fdt_write_distance_matrix()

[Daniel Henrique Barboza]

Use g_autofree in 'dist_matrix' to avoid the manual g_free().

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 hw/riscv/numa.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/riscv/numa.c b/hw/riscv/numa.c
index d319aefb45..cf686f4ff1 100644
--- a/hw/riscv/numa.c
+++ b/hw/riscv/numa.c
@@ -167,7 +167,8 @@ void riscv_socket_fdt_write_id(const MachineState *ms, const char *node_name,
 void riscv_socket_fdt_write_distance_matrix(const MachineState *ms)
 {
     int i, j, idx;
-    uint32_t *dist_matrix, dist_matrix_size;
+    g_autofree uint32_t *dist_matrix = NULL;
+    uint32_t dist_matrix_size;
 
     if (numa_enabled(ms) && ms->numa_state->have_numa_distance) {
         dist_matrix_size = riscv_socket_count(ms) * riscv_socket_count(ms);
@@ -189,7 +190,6 @@ void riscv_socket_fdt_write_distance_matrix(const MachineState *ms)
                                 "numa-distance-map-v1");
         qemu_fdt_setprop(ms->fdt, "/distance-map", "distance-matrix",
                          dist_matrix, dist_matrix_size);
-        g_free(dist_matrix);
     }
 }
 
-- 
2.43.0

Re: [PATCH 2/7] hw/riscv/numa.c: use g_autofree in socket_fdt_write_distance_matrix()

[Alistair Francis]

On Tue, Jan 23, 2024 at 8:17 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> Use g_autofree in 'dist_matrix' to avoid the manual g_free().
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/riscv/numa.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/riscv/numa.c b/hw/riscv/numa.c
> index d319aefb45..cf686f4ff1 100644
> --- a/hw/riscv/numa.c
> +++ b/hw/riscv/numa.c
> @@ -167,7 +167,8 @@ void riscv_socket_fdt_write_id(const MachineState *ms, const char *node_name,
>  void riscv_socket_fdt_write_distance_matrix(const MachineState *ms)
>  {
>      int i, j, idx;
> -    uint32_t *dist_matrix, dist_matrix_size;
> +    g_autofree uint32_t *dist_matrix = NULL;
> +    uint32_t dist_matrix_size;
>
>      if (numa_enabled(ms) && ms->numa_state->have_numa_distance) {
>          dist_matrix_size = riscv_socket_count(ms) * riscv_socket_count(ms);
> @@ -189,7 +190,6 @@ void riscv_socket_fdt_write_distance_matrix(const MachineState *ms)
>                                  "numa-distance-map-v1");
>          qemu_fdt_setprop(ms->fdt, "/distance-map", "distance-matrix",
>                           dist_matrix, dist_matrix_size);
> -        g_free(dist_matrix);
>      }
>  }
>
> --
> 2.43.0
>
>

[PATCH 3/7] hw/riscv/virt.c: use g_autofree in create_fdt_socket_cpus()

[Daniel Henrique Barboza]

Move all char pointers to the loop. Use g_autofree in all of them to
avoid the g_free() calls.

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 hw/riscv/virt.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index f9fd1341fc..373b1dd96b 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -215,12 +215,16 @@ static void create_fdt_socket_cpus(RISCVVirtState *s, int socket,
     int cpu;
     uint32_t cpu_phandle;
     MachineState *ms = MACHINE(s);
-    char *name, *cpu_name, *core_name, *intc_name, *sv_name;
     bool is_32_bit = riscv_is_32bit(&s->soc[0]);
     uint8_t satp_mode_max;
 
     for (cpu = s->soc[socket].num_harts - 1; cpu >= 0; cpu--) {
         RISCVCPU *cpu_ptr = &s->soc[socket].harts[cpu];
+        g_autofree char *name = NULL;
+        g_autofree char *cpu_name = NULL;
+        g_autofree char *core_name = NULL;
+        g_autofree char *intc_name = NULL;
+        g_autofree char *sv_name = NULL;
 
         cpu_phandle = (*phandle)++;
 
@@ -233,12 +237,10 @@ static void create_fdt_socket_cpus(RISCVVirtState *s, int socket,
             sv_name = g_strdup_printf("riscv,%s",
                                       satp_mode_str(satp_mode_max, is_32_bit));
             qemu_fdt_setprop_string(ms->fdt, cpu_name, "mmu-type", sv_name);
-            g_free(sv_name);
         }
 
         name = riscv_isa_string(cpu_ptr);
         qemu_fdt_setprop_string(ms->fdt, cpu_name, "riscv,isa", name);
-        g_free(name);
 
         if (cpu_ptr->cfg.ext_zicbom) {
             qemu_fdt_setprop_cell(ms->fdt, cpu_name, "riscv,cbom-block-size",
@@ -277,10 +279,6 @@ static void create_fdt_socket_cpus(RISCVVirtState *s, int socket,
         core_name = g_strdup_printf("%s/core%d", clust_name, cpu);
         qemu_fdt_add_subnode(ms->fdt, core_name);
         qemu_fdt_setprop_cell(ms->fdt, core_name, "cpu", cpu_phandle);
-
-        g_free(core_name);
-        g_free(intc_name);
-        g_free(cpu_name);
     }
 }
 
-- 
2.43.0

Re: [PATCH 3/7] hw/riscv/virt.c: use g_autofree in create_fdt_socket_cpus()

[Philippe Mathieu-Daudé]

On 22/1/24 23:15, Daniel Henrique Barboza wrote:
> Move all char pointers to the loop. Use g_autofree in all of them to
> avoid the g_free() calls.
> 
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>   hw/riscv/virt.c | 12 +++++-------
>   1 file changed, 5 insertions(+), 7 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 3/7] hw/riscv/virt.c: use g_autofree in create_fdt_socket_cpus()

[Alistair Francis]

On Tue, Jan 23, 2024 at 8:16 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> Move all char pointers to the loop. Use g_autofree in all of them to
> avoid the g_free() calls.
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/riscv/virt.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index f9fd1341fc..373b1dd96b 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -215,12 +215,16 @@ static void create_fdt_socket_cpus(RISCVVirtState *s, int socket,
>      int cpu;
>      uint32_t cpu_phandle;
>      MachineState *ms = MACHINE(s);
> -    char *name, *cpu_name, *core_name, *intc_name, *sv_name;
>      bool is_32_bit = riscv_is_32bit(&s->soc[0]);
>      uint8_t satp_mode_max;
>
>      for (cpu = s->soc[socket].num_harts - 1; cpu >= 0; cpu--) {
>          RISCVCPU *cpu_ptr = &s->soc[socket].harts[cpu];
> +        g_autofree char *name = NULL;
> +        g_autofree char *cpu_name = NULL;
> +        g_autofree char *core_name = NULL;
> +        g_autofree char *intc_name = NULL;
> +        g_autofree char *sv_name = NULL;
>
>          cpu_phandle = (*phandle)++;
>
> @@ -233,12 +237,10 @@ static void create_fdt_socket_cpus(RISCVVirtState *s, int socket,
>              sv_name = g_strdup_printf("riscv,%s",
>                                        satp_mode_str(satp_mode_max, is_32_bit));
>              qemu_fdt_setprop_string(ms->fdt, cpu_name, "mmu-type", sv_name);
> -            g_free(sv_name);
>          }
>
>          name = riscv_isa_string(cpu_ptr);
>          qemu_fdt_setprop_string(ms->fdt, cpu_name, "riscv,isa", name);
> -        g_free(name);
>
>          if (cpu_ptr->cfg.ext_zicbom) {
>              qemu_fdt_setprop_cell(ms->fdt, cpu_name, "riscv,cbom-block-size",
> @@ -277,10 +279,6 @@ static void create_fdt_socket_cpus(RISCVVirtState *s, int socket,
>          core_name = g_strdup_printf("%s/core%d", clust_name, cpu);
>          qemu_fdt_add_subnode(ms->fdt, core_name);
>          qemu_fdt_setprop_cell(ms->fdt, core_name, "cpu", cpu_phandle);
> -
> -        g_free(core_name);
> -        g_free(intc_name);
> -        g_free(cpu_name);
>      }
>  }
>
> --
> 2.43.0
>
>

[PATCH 4/7] hw/riscv/virt.c: use g_autofree in create_fdt_sockets()

[Daniel Henrique Barboza]

Move 'clust_name' inside the loop, and g_autofree, to avoid having to
g_free() manually in each loop iteration.

'intc_phandles' is also g_autofreed to avoid another manual g_free().

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 hw/riscv/virt.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 373b1dd96b..d0f402e0d5 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -721,11 +721,11 @@ static void create_fdt_sockets(RISCVVirtState *s, const MemMapEntry *memmap,
                                uint32_t *irq_virtio_phandle,
                                uint32_t *msi_pcie_phandle)
 {
-    char *clust_name;
     int socket, phandle_pos;
     MachineState *ms = MACHINE(s);
     uint32_t msi_m_phandle = 0, msi_s_phandle = 0;
-    uint32_t *intc_phandles, xplic_phandles[MAX_NODES];
+    uint32_t xplic_phandles[MAX_NODES];
+    g_autofree uint32_t *intc_phandles = NULL;
     int socket_count = riscv_socket_count(ms);
 
     qemu_fdt_add_subnode(ms->fdt, "/cpus");
@@ -739,6 +739,7 @@ static void create_fdt_sockets(RISCVVirtState *s, const MemMapEntry *memmap,
 
     phandle_pos = ms->smp.cpus;
     for (socket = (socket_count - 1); socket >= 0; socket--) {
+        g_autofree char *clust_name = NULL;
         phandle_pos -= s->soc[socket].num_harts;
 
         clust_name = g_strdup_printf("/cpus/cpu-map/cluster%d", socket);
@@ -749,8 +750,6 @@ static void create_fdt_sockets(RISCVVirtState *s, const MemMapEntry *memmap,
 
         create_fdt_socket_memory(s, memmap, socket);
 
-        g_free(clust_name);
-
         if (tcg_enabled()) {
             if (s->have_aclint) {
                 create_fdt_socket_aclint(s, memmap, socket,
@@ -793,8 +792,6 @@ static void create_fdt_sockets(RISCVVirtState *s, const MemMapEntry *memmap,
         }
     }
 
-    g_free(intc_phandles);
-
     if (kvm_enabled() && virt_use_kvm_aia(s)) {
         *irq_mmio_phandle = xplic_phandles[0];
         *irq_virtio_phandle = xplic_phandles[0];
-- 
2.43.0

Re: [PATCH 4/7] hw/riscv/virt.c: use g_autofree in create_fdt_sockets()

[Alistair Francis]

On Tue, Jan 23, 2024 at 8:16 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> Move 'clust_name' inside the loop, and g_autofree, to avoid having to
> g_free() manually in each loop iteration.
>
> 'intc_phandles' is also g_autofreed to avoid another manual g_free().
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/riscv/virt.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index 373b1dd96b..d0f402e0d5 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -721,11 +721,11 @@ static void create_fdt_sockets(RISCVVirtState *s, const MemMapEntry *memmap,
>                                 uint32_t *irq_virtio_phandle,
>                                 uint32_t *msi_pcie_phandle)
>  {
> -    char *clust_name;
>      int socket, phandle_pos;
>      MachineState *ms = MACHINE(s);
>      uint32_t msi_m_phandle = 0, msi_s_phandle = 0;
> -    uint32_t *intc_phandles, xplic_phandles[MAX_NODES];
> +    uint32_t xplic_phandles[MAX_NODES];
> +    g_autofree uint32_t *intc_phandles = NULL;
>      int socket_count = riscv_socket_count(ms);
>
>      qemu_fdt_add_subnode(ms->fdt, "/cpus");
> @@ -739,6 +739,7 @@ static void create_fdt_sockets(RISCVVirtState *s, const MemMapEntry *memmap,
>
>      phandle_pos = ms->smp.cpus;
>      for (socket = (socket_count - 1); socket >= 0; socket--) {
> +        g_autofree char *clust_name = NULL;
>          phandle_pos -= s->soc[socket].num_harts;
>
>          clust_name = g_strdup_printf("/cpus/cpu-map/cluster%d", socket);
> @@ -749,8 +750,6 @@ static void create_fdt_sockets(RISCVVirtState *s, const MemMapEntry *memmap,
>
>          create_fdt_socket_memory(s, memmap, socket);
>
> -        g_free(clust_name);
> -
>          if (tcg_enabled()) {
>              if (s->have_aclint) {
>                  create_fdt_socket_aclint(s, memmap, socket,
> @@ -793,8 +792,6 @@ static void create_fdt_sockets(RISCVVirtState *s, const MemMapEntry *memmap,
>          }
>      }
>
> -    g_free(intc_phandles);
> -
>      if (kvm_enabled() && virt_use_kvm_aia(s)) {
>          *irq_mmio_phandle = xplic_phandles[0];
>          *irq_virtio_phandle = xplic_phandles[0];
> --
> 2.43.0
>
>

[PATCH 5/7] hw/riscv/virt.c: use g_autofree in create_fdt_virtio()

[Daniel Henrique Barboza]

Put 'name' declaration inside the loop, with g_autofree, to avoid
manually doing g_free() in each iteration.

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 hw/riscv/virt.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index d0f402e0d5..f8278df83f 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -820,12 +820,12 @@ static void create_fdt_virtio(RISCVVirtState *s, const MemMapEntry *memmap,
                               uint32_t irq_virtio_phandle)
 {
     int i;
-    char *name;
     MachineState *ms = MACHINE(s);
 
     for (i = 0; i < VIRTIO_COUNT; i++) {
-        name = g_strdup_printf("/soc/virtio_mmio@%lx",
+        g_autofree char *name =  g_strdup_printf("/soc/virtio_mmio@%lx",
             (long)(memmap[VIRT_VIRTIO].base + i * memmap[VIRT_VIRTIO].size));
+
         qemu_fdt_add_subnode(ms->fdt, name);
         qemu_fdt_setprop_string(ms->fdt, name, "compatible", "virtio,mmio");
         qemu_fdt_setprop_cells(ms->fdt, name, "reg",
@@ -840,7 +840,6 @@ static void create_fdt_virtio(RISCVVirtState *s, const MemMapEntry *memmap,
             qemu_fdt_setprop_cells(ms->fdt, name, "interrupts",
                                    VIRTIO_IRQ + i, 0x4);
         }
-        g_free(name);
     }
 }
 
-- 
2.43.0

Re: [PATCH 5/7] hw/riscv/virt.c: use g_autofree in create_fdt_virtio()

[Philippe Mathieu-Daudé]

On 22/1/24 23:15, Daniel Henrique Barboza wrote:
> Put 'name' declaration inside the loop, with g_autofree, to avoid
> manually doing g_free() in each iteration.
> 
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>   hw/riscv/virt.c | 5 ++---
>   1 file changed, 2 insertions(+), 3 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 5/7] hw/riscv/virt.c: use g_autofree in create_fdt_virtio()

[Alistair Francis]

On Tue, Jan 23, 2024 at 9:38 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> Put 'name' declaration inside the loop, with g_autofree, to avoid
> manually doing g_free() in each iteration.
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/riscv/virt.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index d0f402e0d5..f8278df83f 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -820,12 +820,12 @@ static void create_fdt_virtio(RISCVVirtState *s, const MemMapEntry *memmap,
>                                uint32_t irq_virtio_phandle)
>  {
>      int i;
> -    char *name;
>      MachineState *ms = MACHINE(s);
>
>      for (i = 0; i < VIRTIO_COUNT; i++) {
> -        name = g_strdup_printf("/soc/virtio_mmio@%lx",
> +        g_autofree char *name =  g_strdup_printf("/soc/virtio_mmio@%lx",
>              (long)(memmap[VIRT_VIRTIO].base + i * memmap[VIRT_VIRTIO].size));
> +
>          qemu_fdt_add_subnode(ms->fdt, name);
>          qemu_fdt_setprop_string(ms->fdt, name, "compatible", "virtio,mmio");
>          qemu_fdt_setprop_cells(ms->fdt, name, "reg",
> @@ -840,7 +840,6 @@ static void create_fdt_virtio(RISCVVirtState *s, const MemMapEntry *memmap,
>              qemu_fdt_setprop_cells(ms->fdt, name, "interrupts",
>                                     VIRTIO_IRQ + i, 0x4);
>          }
> -        g_free(name);
>      }
>  }
>
> --
> 2.43.0
>
>

[PATCH 6/7] hw/riscv/virt.c: use g_autofree in virt_machine_init()

[Daniel Henrique Barboza]

Move 'soc_name' to the loop, and give it g_autofree, to avoid the manual
g_free().

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 hw/riscv/virt.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index f8278df83f..710fbbda2c 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -1356,7 +1356,6 @@ static void virt_machine_init(MachineState *machine)
     RISCVVirtState *s = RISCV_VIRT_MACHINE(machine);
     MemoryRegion *system_memory = get_system_memory();
     MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
-    char *soc_name;
     DeviceState *mmio_irqchip, *virtio_irqchip, *pcie_irqchip;
     int i, base_hartid, hart_count;
     int socket_count = riscv_socket_count(machine);
@@ -1376,6 +1375,8 @@ static void virt_machine_init(MachineState *machine)
     /* Initialize sockets */
     mmio_irqchip = virtio_irqchip = pcie_irqchip = NULL;
     for (i = 0; i < socket_count; i++) {
+        g_autofree char *soc_name = g_strdup_printf("soc%d", i);
+
         if (!riscv_socket_check_hartids(machine, i)) {
             error_report("discontinuous hartids in socket%d", i);
             exit(1);
@@ -1393,10 +1394,8 @@ static void virt_machine_init(MachineState *machine)
             exit(1);
         }
 
-        soc_name = g_strdup_printf("soc%d", i);
         object_initialize_child(OBJECT(machine), soc_name, &s->soc[i],
                                 TYPE_RISCV_HART_ARRAY);
-        g_free(soc_name);
         object_property_set_str(OBJECT(&s->soc[i]), "cpu-type",
                                 machine->cpu_type, &error_abort);
         object_property_set_int(OBJECT(&s->soc[i]), "hartid-base",
-- 
2.43.0

Re: [PATCH 6/7] hw/riscv/virt.c: use g_autofree in virt_machine_init()

[Philippe Mathieu-Daudé]

On 22/1/24 23:15, Daniel Henrique Barboza wrote:
> Move 'soc_name' to the loop, and give it g_autofree, to avoid the manual
> g_free().
> 
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>   hw/riscv/virt.c | 5 ++---
>   1 file changed, 2 insertions(+), 3 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 6/7] hw/riscv/virt.c: use g_autofree in virt_machine_init()

[Alistair Francis]

On Tue, Jan 23, 2024 at 9:38 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> Move 'soc_name' to the loop, and give it g_autofree, to avoid the manual
> g_free().
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/riscv/virt.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index f8278df83f..710fbbda2c 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -1356,7 +1356,6 @@ static void virt_machine_init(MachineState *machine)
>      RISCVVirtState *s = RISCV_VIRT_MACHINE(machine);
>      MemoryRegion *system_memory = get_system_memory();
>      MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
> -    char *soc_name;
>      DeviceState *mmio_irqchip, *virtio_irqchip, *pcie_irqchip;
>      int i, base_hartid, hart_count;
>      int socket_count = riscv_socket_count(machine);
> @@ -1376,6 +1375,8 @@ static void virt_machine_init(MachineState *machine)
>      /* Initialize sockets */
>      mmio_irqchip = virtio_irqchip = pcie_irqchip = NULL;
>      for (i = 0; i < socket_count; i++) {
> +        g_autofree char *soc_name = g_strdup_printf("soc%d", i);
> +
>          if (!riscv_socket_check_hartids(machine, i)) {
>              error_report("discontinuous hartids in socket%d", i);
>              exit(1);
> @@ -1393,10 +1394,8 @@ static void virt_machine_init(MachineState *machine)
>              exit(1);
>          }
>
> -        soc_name = g_strdup_printf("soc%d", i);
>          object_initialize_child(OBJECT(machine), soc_name, &s->soc[i],
>                                  TYPE_RISCV_HART_ARRAY);
> -        g_free(soc_name);
>          object_property_set_str(OBJECT(&s->soc[i]), "cpu-type",
>                                  machine->cpu_type, &error_abort);
>          object_property_set_int(OBJECT(&s->soc[i]), "hartid-base",
> --
> 2.43.0
>
>

[PATCH 7/7] hw/riscv/virt.c: use g_autofree in create_fdt_*

[Daniel Henrique Barboza]

We have a lot of cases where a char or an uint32_t pointer is used once
to alloc a string/array, read/written during the function, and then
g_free() at the end. There's no pointer re-use - a single alloc, a
single g_free().

Use 'g_autofree' to avoid the g_free() calls.

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 hw/riscv/virt.c | 78 ++++++++++++++-----------------------------------
 1 file changed, 22 insertions(+), 56 deletions(-)

diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 710fbbda2c..1c257e89d2 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -285,7 +285,7 @@ static void create_fdt_socket_cpus(RISCVVirtState *s, int socket,
 static void create_fdt_socket_memory(RISCVVirtState *s,
                                      const MemMapEntry *memmap, int socket)
 {
-    char *mem_name;
+    g_autofree char *mem_name = NULL;
     uint64_t addr, size;
     MachineState *ms = MACHINE(s);
 
@@ -297,7 +297,6 @@ static void create_fdt_socket_memory(RISCVVirtState *s,
         addr >> 32, addr, size >> 32, size);
     qemu_fdt_setprop_string(ms->fdt, mem_name, "device_type", "memory");
     riscv_socket_fdt_write_id(ms, mem_name, socket);
-    g_free(mem_name);
 }
 
 static void create_fdt_socket_clint(RISCVVirtState *s,
@@ -305,8 +304,8 @@ static void create_fdt_socket_clint(RISCVVirtState *s,
                                     uint32_t *intc_phandles)
 {
     int cpu;
-    char *clint_name;
-    uint32_t *clint_cells;
+    g_autofree char *clint_name = NULL;
+    g_autofree uint32_t *clint_cells = NULL;
     unsigned long clint_addr;
     MachineState *ms = MACHINE(s);
     static const char * const clint_compat[2] = {
@@ -333,9 +332,6 @@ static void create_fdt_socket_clint(RISCVVirtState *s,
     qemu_fdt_setprop(ms->fdt, clint_name, "interrupts-extended",
         clint_cells, s->soc[socket].num_harts * sizeof(uint32_t) * 4);
     riscv_socket_fdt_write_id(ms, clint_name, socket);
-    g_free(clint_name);
-
-    g_free(clint_cells);
 }
 
 static void create_fdt_socket_aclint(RISCVVirtState *s,
@@ -346,9 +342,9 @@ static void create_fdt_socket_aclint(RISCVVirtState *s,
     char *name;
     unsigned long addr, size;
     uint32_t aclint_cells_size;
-    uint32_t *aclint_mswi_cells;
-    uint32_t *aclint_sswi_cells;
-    uint32_t *aclint_mtimer_cells;
+    g_autofree uint32_t *aclint_mswi_cells = NULL;
+    g_autofree uint32_t *aclint_sswi_cells = NULL;
+    g_autofree uint32_t *aclint_mtimer_cells = NULL;
     MachineState *ms = MACHINE(s);
 
     aclint_mswi_cells = g_new0(uint32_t, s->soc[socket].num_harts * 2);
@@ -420,10 +416,6 @@ static void create_fdt_socket_aclint(RISCVVirtState *s,
         riscv_socket_fdt_write_id(ms, name, socket);
         g_free(name);
     }
-
-    g_free(aclint_mswi_cells);
-    g_free(aclint_mtimer_cells);
-    g_free(aclint_sswi_cells);
 }
 
 static void create_fdt_socket_plic(RISCVVirtState *s,
@@ -432,8 +424,8 @@ static void create_fdt_socket_plic(RISCVVirtState *s,
                                    uint32_t *plic_phandles)
 {
     int cpu;
-    char *plic_name;
-    uint32_t *plic_cells;
+    g_autofree char *plic_name = NULL;
+    g_autofree uint32_t *plic_cells;
     unsigned long plic_addr;
     MachineState *ms = MACHINE(s);
     static const char * const plic_compat[2] = {
@@ -493,10 +485,6 @@ static void create_fdt_socket_plic(RISCVVirtState *s,
                                        memmap[VIRT_PLATFORM_BUS].size,
                                        VIRT_PLATFORM_BUS_IRQ);
     }
-
-    g_free(plic_name);
-
-    g_free(plic_cells);
 }
 
 uint32_t imsic_num_bits(uint32_t count)
@@ -515,11 +503,12 @@ static void create_fdt_one_imsic(RISCVVirtState *s, hwaddr base_addr,
                                  bool m_mode, uint32_t imsic_guest_bits)
 {
     int cpu, socket;
-    char *imsic_name;
+    g_autofree char *imsic_name = NULL;
     MachineState *ms = MACHINE(s);
     int socket_count = riscv_socket_count(ms);
-    uint32_t imsic_max_hart_per_socket;
-    uint32_t *imsic_cells, *imsic_regs, imsic_addr, imsic_size;
+    uint32_t imsic_max_hart_per_socket, imsic_addr, imsic_size;
+    g_autofree uint32_t *imsic_cells = NULL;
+    g_autofree uint32_t *imsic_regs = NULL;
 
     imsic_cells = g_new0(uint32_t, ms->smp.cpus * 2);
     imsic_regs = g_new0(uint32_t, socket_count * 4);
@@ -571,10 +560,6 @@ static void create_fdt_one_imsic(RISCVVirtState *s, hwaddr base_addr,
                               IMSIC_MMIO_GROUP_MIN_SHIFT);
     }
     qemu_fdt_setprop_cell(ms->fdt, imsic_name, "phandle", msi_phandle);
-
-    g_free(imsic_name);
-    g_free(imsic_regs);
-    g_free(imsic_cells);
 }
 
 static void create_fdt_imsic(RISCVVirtState *s, const MemMapEntry *memmap,
@@ -606,12 +591,10 @@ static void create_fdt_one_aplic(RISCVVirtState *s, int socket,
                                  bool m_mode, int num_harts)
 {
     int cpu;
-    char *aplic_name;
-    uint32_t *aplic_cells;
+    g_autofree char *aplic_name = NULL;
+    g_autofree uint32_t *aplic_cells = g_new0(uint32_t, num_harts * 2);
     MachineState *ms = MACHINE(s);
 
-    aplic_cells = g_new0(uint32_t, num_harts * 2);
-
     for (cpu = 0; cpu < num_harts; cpu++) {
         aplic_cells[cpu * 2 + 0] = cpu_to_be32(intc_phandles[cpu]);
         aplic_cells[cpu * 2 + 1] = cpu_to_be32(m_mode ? IRQ_M_EXT : IRQ_S_EXT);
@@ -646,9 +629,6 @@ static void create_fdt_one_aplic(RISCVVirtState *s, int socket,
 
     riscv_socket_fdt_write_id(ms, aplic_name, socket);
     qemu_fdt_setprop_cell(ms->fdt, aplic_name, "phandle", aplic_phandle);
-
-    g_free(aplic_name);
-    g_free(aplic_cells);
 }
 
 static void create_fdt_socket_aplic(RISCVVirtState *s,
@@ -660,7 +640,7 @@ static void create_fdt_socket_aplic(RISCVVirtState *s,
                                     uint32_t *aplic_phandles,
                                     int num_harts)
 {
-    char *aplic_name;
+    g_autofree char *aplic_name = NULL;
     unsigned long aplic_addr;
     MachineState *ms = MACHINE(s);
     uint32_t aplic_m_phandle, aplic_s_phandle;
@@ -695,23 +675,18 @@ static void create_fdt_socket_aplic(RISCVVirtState *s,
                                        VIRT_PLATFORM_BUS_IRQ);
     }
 
-    g_free(aplic_name);
-
     aplic_phandles[socket] = aplic_s_phandle;
 }
 
 static void create_fdt_pmu(RISCVVirtState *s)
 {
-    char *pmu_name;
+    g_autofree char *pmu_name = g_strdup_printf("/pmu");
     MachineState *ms = MACHINE(s);
     RISCVCPU hart = s->soc[0].harts[0];
 
-    pmu_name = g_strdup_printf("/pmu");
     qemu_fdt_add_subnode(ms->fdt, pmu_name);
     qemu_fdt_setprop_string(ms->fdt, pmu_name, "compatible", "riscv,pmu");
     riscv_pmu_generate_fdt_node(ms->fdt, hart.pmu_avail_ctrs, pmu_name);
-
-    g_free(pmu_name);
 }
 
 static void create_fdt_sockets(RISCVVirtState *s, const MemMapEntry *memmap,
@@ -847,7 +822,7 @@ static void create_fdt_pcie(RISCVVirtState *s, const MemMapEntry *memmap,
                             uint32_t irq_pcie_phandle,
                             uint32_t msi_pcie_phandle)
 {
-    char *name;
+    g_autofree char *name = NULL;
     MachineState *ms = MACHINE(s);
 
     name = g_strdup_printf("/soc/pci@%lx",
@@ -881,7 +856,6 @@ static void create_fdt_pcie(RISCVVirtState *s, const MemMapEntry *memmap,
         2, virt_high_pcie_memmap.base, 2, virt_high_pcie_memmap.size);
 
     create_pcie_irq_map(s, ms->fdt, name, irq_pcie_phandle);
-    g_free(name);
 }
 
 static void create_fdt_reset(RISCVVirtState *s, const MemMapEntry *memmap,
@@ -928,7 +902,7 @@ static void create_fdt_reset(RISCVVirtState *s, const MemMapEntry *memmap,
 static void create_fdt_uart(RISCVVirtState *s, const MemMapEntry *memmap,
                             uint32_t irq_mmio_phandle)
 {
-    char *name;
+    g_autofree char *name = NULL;
     MachineState *ms = MACHINE(s);
 
     name = g_strdup_printf("/soc/serial@%lx", (long)memmap[VIRT_UART0].base);
@@ -946,13 +920,12 @@ static void create_fdt_uart(RISCVVirtState *s, const MemMapEntry *memmap,
     }
 
     qemu_fdt_setprop_string(ms->fdt, "/chosen", "stdout-path", name);
-    g_free(name);
 }
 
 static void create_fdt_rtc(RISCVVirtState *s, const MemMapEntry *memmap,
                            uint32_t irq_mmio_phandle)
 {
-    char *name;
+    g_autofree char *name = NULL;
     MachineState *ms = MACHINE(s);
 
     name = g_strdup_printf("/soc/rtc@%lx", (long)memmap[VIRT_RTC].base);
@@ -968,41 +941,36 @@ static void create_fdt_rtc(RISCVVirtState *s, const MemMapEntry *memmap,
     } else {
         qemu_fdt_setprop_cells(ms->fdt, name, "interrupts", RTC_IRQ, 0x4);
     }
-    g_free(name);
 }
 
 static void create_fdt_flash(RISCVVirtState *s, const MemMapEntry *memmap)
 {
-    char *name;
     MachineState *ms = MACHINE(s);
     hwaddr flashsize = virt_memmap[VIRT_FLASH].size / 2;
     hwaddr flashbase = virt_memmap[VIRT_FLASH].base;
+    g_autofree char *name = g_strdup_printf("/flash@%" PRIx64, flashbase);
 
-    name = g_strdup_printf("/flash@%" PRIx64, flashbase);
     qemu_fdt_add_subnode(ms->fdt, name);
     qemu_fdt_setprop_string(ms->fdt, name, "compatible", "cfi-flash");
     qemu_fdt_setprop_sized_cells(ms->fdt, name, "reg",
                                  2, flashbase, 2, flashsize,
                                  2, flashbase + flashsize, 2, flashsize);
     qemu_fdt_setprop_cell(ms->fdt, name, "bank-width", 4);
-    g_free(name);
 }
 
 static void create_fdt_fw_cfg(RISCVVirtState *s, const MemMapEntry *memmap)
 {
-    char *nodename;
     MachineState *ms = MACHINE(s);
     hwaddr base = memmap[VIRT_FW_CFG].base;
     hwaddr size = memmap[VIRT_FW_CFG].size;
+    g_autofree char *nodename = g_strdup_printf("/fw-cfg@%" PRIx64, base);
 
-    nodename = g_strdup_printf("/fw-cfg@%" PRIx64, base);
     qemu_fdt_add_subnode(ms->fdt, nodename);
     qemu_fdt_setprop_string(ms->fdt, nodename,
                             "compatible", "qemu,fw-cfg-mmio");
     qemu_fdt_setprop_sized_cells(ms->fdt, nodename, "reg",
                                  2, base, 2, size);
     qemu_fdt_setprop(ms->fdt, nodename, "dma-coherent", NULL, 0);
-    g_free(nodename);
 }
 
 static void finalize_fdt(RISCVVirtState *s)
@@ -1149,7 +1117,7 @@ static DeviceState *virt_create_plic(const MemMapEntry *memmap, int socket,
                                      int base_hartid, int hart_count)
 {
     DeviceState *ret;
-    char *plic_hart_config;
+    g_autofree char *plic_hart_config = NULL;
 
     /* Per-socket PLIC hart topology configuration string */
     plic_hart_config = riscv_plic_hart_config_string(hart_count);
@@ -1168,8 +1136,6 @@ static DeviceState *virt_create_plic(const MemMapEntry *memmap, int socket,
             VIRT_PLIC_CONTEXT_STRIDE,
             memmap[VIRT_PLIC].size);
 
-    g_free(plic_hart_config);
-
     return ret;
 }
 
-- 
2.43.0

Re: [PATCH 7/7] hw/riscv/virt.c: use g_autofree in create_fdt_*

[Alistair Francis]

On Tue, Jan 23, 2024 at 8:18 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> We have a lot of cases where a char or an uint32_t pointer is used once
> to alloc a string/array, read/written during the function, and then
> g_free() at the end. There's no pointer re-use - a single alloc, a
> single g_free().
>
> Use 'g_autofree' to avoid the g_free() calls.
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/riscv/virt.c | 78 ++++++++++++++-----------------------------------
>  1 file changed, 22 insertions(+), 56 deletions(-)
>
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index 710fbbda2c..1c257e89d2 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -285,7 +285,7 @@ static void create_fdt_socket_cpus(RISCVVirtState *s, int socket,
>  static void create_fdt_socket_memory(RISCVVirtState *s,
>                                       const MemMapEntry *memmap, int socket)
>  {
> -    char *mem_name;
> +    g_autofree char *mem_name = NULL;
>      uint64_t addr, size;
>      MachineState *ms = MACHINE(s);
>
> @@ -297,7 +297,6 @@ static void create_fdt_socket_memory(RISCVVirtState *s,
>          addr >> 32, addr, size >> 32, size);
>      qemu_fdt_setprop_string(ms->fdt, mem_name, "device_type", "memory");
>      riscv_socket_fdt_write_id(ms, mem_name, socket);
> -    g_free(mem_name);
>  }
>
>  static void create_fdt_socket_clint(RISCVVirtState *s,
> @@ -305,8 +304,8 @@ static void create_fdt_socket_clint(RISCVVirtState *s,
>                                      uint32_t *intc_phandles)
>  {
>      int cpu;
> -    char *clint_name;
> -    uint32_t *clint_cells;
> +    g_autofree char *clint_name = NULL;
> +    g_autofree uint32_t *clint_cells = NULL;
>      unsigned long clint_addr;
>      MachineState *ms = MACHINE(s);
>      static const char * const clint_compat[2] = {
> @@ -333,9 +332,6 @@ static void create_fdt_socket_clint(RISCVVirtState *s,
>      qemu_fdt_setprop(ms->fdt, clint_name, "interrupts-extended",
>          clint_cells, s->soc[socket].num_harts * sizeof(uint32_t) * 4);
>      riscv_socket_fdt_write_id(ms, clint_name, socket);
> -    g_free(clint_name);
> -
> -    g_free(clint_cells);
>  }
>
>  static void create_fdt_socket_aclint(RISCVVirtState *s,
> @@ -346,9 +342,9 @@ static void create_fdt_socket_aclint(RISCVVirtState *s,
>      char *name;
>      unsigned long addr, size;
>      uint32_t aclint_cells_size;
> -    uint32_t *aclint_mswi_cells;
> -    uint32_t *aclint_sswi_cells;
> -    uint32_t *aclint_mtimer_cells;
> +    g_autofree uint32_t *aclint_mswi_cells = NULL;
> +    g_autofree uint32_t *aclint_sswi_cells = NULL;
> +    g_autofree uint32_t *aclint_mtimer_cells = NULL;
>      MachineState *ms = MACHINE(s);
>
>      aclint_mswi_cells = g_new0(uint32_t, s->soc[socket].num_harts * 2);
> @@ -420,10 +416,6 @@ static void create_fdt_socket_aclint(RISCVVirtState *s,
>          riscv_socket_fdt_write_id(ms, name, socket);
>          g_free(name);
>      }
> -
> -    g_free(aclint_mswi_cells);
> -    g_free(aclint_mtimer_cells);
> -    g_free(aclint_sswi_cells);
>  }
>
>  static void create_fdt_socket_plic(RISCVVirtState *s,
> @@ -432,8 +424,8 @@ static void create_fdt_socket_plic(RISCVVirtState *s,
>                                     uint32_t *plic_phandles)
>  {
>      int cpu;
> -    char *plic_name;
> -    uint32_t *plic_cells;
> +    g_autofree char *plic_name = NULL;
> +    g_autofree uint32_t *plic_cells;
>      unsigned long plic_addr;
>      MachineState *ms = MACHINE(s);
>      static const char * const plic_compat[2] = {
> @@ -493,10 +485,6 @@ static void create_fdt_socket_plic(RISCVVirtState *s,
>                                         memmap[VIRT_PLATFORM_BUS].size,
>                                         VIRT_PLATFORM_BUS_IRQ);
>      }
> -
> -    g_free(plic_name);
> -
> -    g_free(plic_cells);
>  }
>
>  uint32_t imsic_num_bits(uint32_t count)
> @@ -515,11 +503,12 @@ static void create_fdt_one_imsic(RISCVVirtState *s, hwaddr base_addr,
>                                   bool m_mode, uint32_t imsic_guest_bits)
>  {
>      int cpu, socket;
> -    char *imsic_name;
> +    g_autofree char *imsic_name = NULL;
>      MachineState *ms = MACHINE(s);
>      int socket_count = riscv_socket_count(ms);
> -    uint32_t imsic_max_hart_per_socket;
> -    uint32_t *imsic_cells, *imsic_regs, imsic_addr, imsic_size;
> +    uint32_t imsic_max_hart_per_socket, imsic_addr, imsic_size;
> +    g_autofree uint32_t *imsic_cells = NULL;
> +    g_autofree uint32_t *imsic_regs = NULL;
>
>      imsic_cells = g_new0(uint32_t, ms->smp.cpus * 2);
>      imsic_regs = g_new0(uint32_t, socket_count * 4);
> @@ -571,10 +560,6 @@ static void create_fdt_one_imsic(RISCVVirtState *s, hwaddr base_addr,
>                                IMSIC_MMIO_GROUP_MIN_SHIFT);
>      }
>      qemu_fdt_setprop_cell(ms->fdt, imsic_name, "phandle", msi_phandle);
> -
> -    g_free(imsic_name);
> -    g_free(imsic_regs);
> -    g_free(imsic_cells);
>  }
>
>  static void create_fdt_imsic(RISCVVirtState *s, const MemMapEntry *memmap,
> @@ -606,12 +591,10 @@ static void create_fdt_one_aplic(RISCVVirtState *s, int socket,
>                                   bool m_mode, int num_harts)
>  {
>      int cpu;
> -    char *aplic_name;
> -    uint32_t *aplic_cells;
> +    g_autofree char *aplic_name = NULL;
> +    g_autofree uint32_t *aplic_cells = g_new0(uint32_t, num_harts * 2);
>      MachineState *ms = MACHINE(s);
>
> -    aplic_cells = g_new0(uint32_t, num_harts * 2);
> -
>      for (cpu = 0; cpu < num_harts; cpu++) {
>          aplic_cells[cpu * 2 + 0] = cpu_to_be32(intc_phandles[cpu]);
>          aplic_cells[cpu * 2 + 1] = cpu_to_be32(m_mode ? IRQ_M_EXT : IRQ_S_EXT);
> @@ -646,9 +629,6 @@ static void create_fdt_one_aplic(RISCVVirtState *s, int socket,
>
>      riscv_socket_fdt_write_id(ms, aplic_name, socket);
>      qemu_fdt_setprop_cell(ms->fdt, aplic_name, "phandle", aplic_phandle);
> -
> -    g_free(aplic_name);
> -    g_free(aplic_cells);
>  }
>
>  static void create_fdt_socket_aplic(RISCVVirtState *s,
> @@ -660,7 +640,7 @@ static void create_fdt_socket_aplic(RISCVVirtState *s,
>                                      uint32_t *aplic_phandles,
>                                      int num_harts)
>  {
> -    char *aplic_name;
> +    g_autofree char *aplic_name = NULL;
>      unsigned long aplic_addr;
>      MachineState *ms = MACHINE(s);
>      uint32_t aplic_m_phandle, aplic_s_phandle;
> @@ -695,23 +675,18 @@ static void create_fdt_socket_aplic(RISCVVirtState *s,
>                                         VIRT_PLATFORM_BUS_IRQ);
>      }
>
> -    g_free(aplic_name);
> -
>      aplic_phandles[socket] = aplic_s_phandle;
>  }
>
>  static void create_fdt_pmu(RISCVVirtState *s)
>  {
> -    char *pmu_name;
> +    g_autofree char *pmu_name = g_strdup_printf("/pmu");
>      MachineState *ms = MACHINE(s);
>      RISCVCPU hart = s->soc[0].harts[0];
>
> -    pmu_name = g_strdup_printf("/pmu");
>      qemu_fdt_add_subnode(ms->fdt, pmu_name);
>      qemu_fdt_setprop_string(ms->fdt, pmu_name, "compatible", "riscv,pmu");
>      riscv_pmu_generate_fdt_node(ms->fdt, hart.pmu_avail_ctrs, pmu_name);
> -
> -    g_free(pmu_name);
>  }
>
>  static void create_fdt_sockets(RISCVVirtState *s, const MemMapEntry *memmap,
> @@ -847,7 +822,7 @@ static void create_fdt_pcie(RISCVVirtState *s, const MemMapEntry *memmap,
>                              uint32_t irq_pcie_phandle,
>                              uint32_t msi_pcie_phandle)
>  {
> -    char *name;
> +    g_autofree char *name = NULL;
>      MachineState *ms = MACHINE(s);
>
>      name = g_strdup_printf("/soc/pci@%lx",
> @@ -881,7 +856,6 @@ static void create_fdt_pcie(RISCVVirtState *s, const MemMapEntry *memmap,
>          2, virt_high_pcie_memmap.base, 2, virt_high_pcie_memmap.size);
>
>      create_pcie_irq_map(s, ms->fdt, name, irq_pcie_phandle);
> -    g_free(name);
>  }
>
>  static void create_fdt_reset(RISCVVirtState *s, const MemMapEntry *memmap,
> @@ -928,7 +902,7 @@ static void create_fdt_reset(RISCVVirtState *s, const MemMapEntry *memmap,
>  static void create_fdt_uart(RISCVVirtState *s, const MemMapEntry *memmap,
>                              uint32_t irq_mmio_phandle)
>  {
> -    char *name;
> +    g_autofree char *name = NULL;
>      MachineState *ms = MACHINE(s);
>
>      name = g_strdup_printf("/soc/serial@%lx", (long)memmap[VIRT_UART0].base);
> @@ -946,13 +920,12 @@ static void create_fdt_uart(RISCVVirtState *s, const MemMapEntry *memmap,
>      }
>
>      qemu_fdt_setprop_string(ms->fdt, "/chosen", "stdout-path", name);
> -    g_free(name);
>  }
>
>  static void create_fdt_rtc(RISCVVirtState *s, const MemMapEntry *memmap,
>                             uint32_t irq_mmio_phandle)
>  {
> -    char *name;
> +    g_autofree char *name = NULL;
>      MachineState *ms = MACHINE(s);
>
>      name = g_strdup_printf("/soc/rtc@%lx", (long)memmap[VIRT_RTC].base);
> @@ -968,41 +941,36 @@ static void create_fdt_rtc(RISCVVirtState *s, const MemMapEntry *memmap,
>      } else {
>          qemu_fdt_setprop_cells(ms->fdt, name, "interrupts", RTC_IRQ, 0x4);
>      }
> -    g_free(name);
>  }
>
>  static void create_fdt_flash(RISCVVirtState *s, const MemMapEntry *memmap)
>  {
> -    char *name;
>      MachineState *ms = MACHINE(s);
>      hwaddr flashsize = virt_memmap[VIRT_FLASH].size / 2;
>      hwaddr flashbase = virt_memmap[VIRT_FLASH].base;
> +    g_autofree char *name = g_strdup_printf("/flash@%" PRIx64, flashbase);
>
> -    name = g_strdup_printf("/flash@%" PRIx64, flashbase);
>      qemu_fdt_add_subnode(ms->fdt, name);
>      qemu_fdt_setprop_string(ms->fdt, name, "compatible", "cfi-flash");
>      qemu_fdt_setprop_sized_cells(ms->fdt, name, "reg",
>                                   2, flashbase, 2, flashsize,
>                                   2, flashbase + flashsize, 2, flashsize);
>      qemu_fdt_setprop_cell(ms->fdt, name, "bank-width", 4);
> -    g_free(name);
>  }
>
>  static void create_fdt_fw_cfg(RISCVVirtState *s, const MemMapEntry *memmap)
>  {
> -    char *nodename;
>      MachineState *ms = MACHINE(s);
>      hwaddr base = memmap[VIRT_FW_CFG].base;
>      hwaddr size = memmap[VIRT_FW_CFG].size;
> +    g_autofree char *nodename = g_strdup_printf("/fw-cfg@%" PRIx64, base);
>
> -    nodename = g_strdup_printf("/fw-cfg@%" PRIx64, base);
>      qemu_fdt_add_subnode(ms->fdt, nodename);
>      qemu_fdt_setprop_string(ms->fdt, nodename,
>                              "compatible", "qemu,fw-cfg-mmio");
>      qemu_fdt_setprop_sized_cells(ms->fdt, nodename, "reg",
>                                   2, base, 2, size);
>      qemu_fdt_setprop(ms->fdt, nodename, "dma-coherent", NULL, 0);
> -    g_free(nodename);
>  }
>
>  static void finalize_fdt(RISCVVirtState *s)
> @@ -1149,7 +1117,7 @@ static DeviceState *virt_create_plic(const MemMapEntry *memmap, int socket,
>                                       int base_hartid, int hart_count)
>  {
>      DeviceState *ret;
> -    char *plic_hart_config;
> +    g_autofree char *plic_hart_config = NULL;
>
>      /* Per-socket PLIC hart topology configuration string */
>      plic_hart_config = riscv_plic_hart_config_string(hart_count);
> @@ -1168,8 +1136,6 @@ static DeviceState *virt_create_plic(const MemMapEntry *memmap, int socket,
>              VIRT_PLIC_CONTEXT_STRIDE,
>              memmap[VIRT_PLIC].size);
>
> -    g_free(plic_hart_config);
> -
>      return ret;
>  }
>
> --
> 2.43.0
>
>

Re: [PATCH 0/7] hw/riscv: fix leak, add more g_autofree

[Alistair Francis]

On Tue, Jan 23, 2024 at 9:39 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> Hi,
>
> First patch fixes a leak found when using Valgrind. The root cause is a
> missing g_free() in a string.
>
> In fact, I found while doing reviews that we keep repeating the same
> pattern:
>
> ====
> char *name;
> name = g_strdup_printf(...);
> (...)
> g_free(name);
> ====
>
> With this in mind, I ended up making this rather trivial series to
> introduce more string/array autocleaning in the 'virt' machine code. The
> advantage of doing 'g_autofree' is that we'll guarantee that we'll clean
> ourselves up when the variable goes out of scope, avoiding leaks like
> the one patch 1 fixes. We want to enforce this autoclean style in
> reviews, and for that we need to get rid of at least some of the uses we
> do it right now.
>
> I didn't bother changing the 'spike' and the 'sifive' boards for now
> because the bulk of new patches is done on top of the 'virt' machine,
> so it's more important to tidy this board first.
>
>
> Daniel Henrique Barboza (7):
>   hw/riscv/virt-acpi-build.c: fix leak in build_rhct()
>   hw/riscv/numa.c: use g_autofree in socket_fdt_write_distance_matrix()
>   hw/riscv/virt.c: use g_autofree in create_fdt_socket_cpus()
>   hw/riscv/virt.c: use g_autofree in create_fdt_sockets()
>   hw/riscv/virt.c: use g_autofree in create_fdt_virtio()
>   hw/riscv/virt.c: use g_autofree in virt_machine_init()
>   hw/riscv/virt.c: use g_autofree in create_fdt_*

Thanks!

Applied to riscv-to-apply.next

Alistair

>
>  hw/riscv/numa.c            |   4 +-
>  hw/riscv/virt-acpi-build.c |   2 +-
>  hw/riscv/virt.c            | 109 ++++++++++++-------------------------
>  3 files changed, 37 insertions(+), 78 deletions(-)
>
> --
> 2.43.0
>
>