[PATCH 00/14] Fix check-qtest-ppc64 sanitizer errors

[PATCH 00/14] Fix check-qtest-ppc64 sanitizer errors

[Akihiko Odaki]

I saw various sanitizer errors when running check-qtest-ppc64. While
I could just turn off sanitizers, I decided to tackle them this time.

Unfortunately, GLib does not free test data in some cases so some
sanitizer errors remain. All sanitizer errors will be gone with this
patch series combined with the following change for GLib:
https://gitlab.gnome.org/GNOME/glib/-/merge_requests/4120

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
Akihiko Odaki (14):
      hw/core: Free CPUState allocations
      hw/ide: Free macio-ide IRQs
      hw/isa/vt82c686: Free irqs
      spapr: Free stdout path
      ppc/vof: Fix unaligned FDT property access
      hw/virtio: Free vqs before vhost_dev_cleanup()
      migration: Free removed SaveStateEntry
      memory: Do not create circular reference with subregion
      tests/qtest: Use qtest_add_data_func_full()
      tests/qtest: Free unused QMP response
      tests/qtest: Free old machine variable name
      tests/qtest: Delete previous boot file
      tests/qtest: Free paths
      tests/qtest: Free GThread

 hw/core/cpu-common.c                 |  3 +++
 hw/ide/macio.c                       |  9 +++++++++
 hw/isa/vt82c686.c                    |  3 ++-
 hw/ppc/spapr_vof.c                   |  2 +-
 hw/ppc/vof.c                         |  2 +-
 hw/virtio/vhost-user-base.c          |  2 ++
 migration/savevm.c                   |  2 ++
 system/memory.c                      | 11 +++++++++--
 tests/qtest/device-introspect-test.c |  7 +++----
 tests/qtest/libqtest.c               |  3 +++
 tests/qtest/migration-test.c         | 18 +++++++++++-------
 tests/qtest/qos-test.c               | 16 ++++++++++++----
 tests/qtest/vhost-user-test.c        |  6 +++---
 13 files changed, 61 insertions(+), 23 deletions(-)
---
base-commit: 74abb45dac6979e7ff76172b7f0a24e869405184
change-id: 20240625-san-097afaf4f1c2

Best regards,
-- 
Akihiko Odaki <akihiko.odaki@daynix.com>

[PATCH 01/14] hw/core: Free CPUState allocations

[Akihiko Odaki]

This suppresses LeakSanitizer warnings.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 hw/core/cpu-common.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c
index 0f0a247f5642..42f38b01a97f 100644
--- a/hw/core/cpu-common.c
+++ b/hw/core/cpu-common.c
@@ -274,6 +274,9 @@ static void cpu_common_finalize(Object *obj)
 {
     CPUState *cpu = CPU(obj);
 
+    g_free(cpu->thread);
+    g_free(cpu->halt_cond);
+    g_free(cpu->cpu_ases);
     g_array_free(cpu->gdb_regs, TRUE);
     qemu_lockcnt_destroy(&cpu->in_ioctl_lock);
     qemu_mutex_destroy(&cpu->work_mutex);

-- 
2.45.2

[PATCH 02/14] hw/ide: Free macio-ide IRQs

[Akihiko Odaki]

This suppresses LeakSanitizer warnings.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 hw/ide/macio.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/hw/ide/macio.c b/hw/ide/macio.c
index aca90d04f0e8..d8fbc1a17ba6 100644
--- a/hw/ide/macio.c
+++ b/hw/ide/macio.c
@@ -464,6 +464,14 @@ static void macio_ide_initfn(Object *obj)
                              qdev_prop_allow_set_link_before_realize, 0);
 }
 
+static void macio_ide_finalize(Object *obj)
+{
+    MACIOIDEState *s = MACIO_IDE(obj);
+
+    qemu_free_irq(s->dma_irq);
+    qemu_free_irq(s->ide_irq);
+}
+
 static Property macio_ide_properties[] = {
     DEFINE_PROP_UINT32("channel", MACIOIDEState, channel, 0),
     DEFINE_PROP_UINT32("addr", MACIOIDEState, addr, -1),
@@ -486,6 +494,7 @@ static const TypeInfo macio_ide_type_info = {
     .parent = TYPE_SYS_BUS_DEVICE,
     .instance_size = sizeof(MACIOIDEState),
     .instance_init = macio_ide_initfn,
+    .instance_finalize = macio_ide_finalize,
     .class_init = macio_ide_class_init,
 };
 

-- 
2.45.2

[PATCH 03/14] hw/isa/vt82c686: Free irqs

[Akihiko Odaki]

This suppresses LeakSanitizer warnings.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 hw/isa/vt82c686.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index 8582ac0322eb..189b487f1d22 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -721,7 +721,6 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
 
     qdev_init_gpio_out(dev, &s->cpu_intr, 1);
     qdev_init_gpio_in_named(dev, via_isa_pirq, "pirq", PCI_NUM_PINS);
-    isa_irq = qemu_allocate_irqs(via_isa_request_i8259_irq, s, 1);
     isa_bus = isa_bus_new(dev, pci_address_space(d), pci_address_space_io(d),
                           errp);
 
@@ -729,7 +728,9 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
         return;
     }
 
+    isa_irq = qemu_allocate_irqs(via_isa_request_i8259_irq, s, 1);
     s->isa_irqs_in = i8259_init(isa_bus, *isa_irq);
+    qemu_free_irqs(isa_irq, 1);
     isa_bus_register_input_irqs(isa_bus, s->isa_irqs_in);
     i8254_pit_init(isa_bus, 0x40, 0, NULL);
     i8257_dma_init(OBJECT(d), isa_bus, 0);

-- 
2.45.2

[PATCH 04/14] spapr: Free stdout path

[Akihiko Odaki]

This suppresses LeakSanitizer warnings.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 hw/ppc/spapr_vof.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/ppc/spapr_vof.c b/hw/ppc/spapr_vof.c
index 09f29be0b9de..c02eaacfed0b 100644
--- a/hw/ppc/spapr_vof.c
+++ b/hw/ppc/spapr_vof.c
@@ -28,7 +28,7 @@ target_ulong spapr_h_vof_client(PowerPCCPU *cpu, SpaprMachineState *spapr,
 
 void spapr_vof_client_dt_finalize(SpaprMachineState *spapr, void *fdt)
 {
-    char *stdout_path = spapr_vio_stdout_path(spapr->vio_bus);
+    g_autofree char *stdout_path = spapr_vio_stdout_path(spapr->vio_bus);
 
     vof_build_dt(fdt, spapr->vof);
 

-- 
2.45.2

[PATCH 05/14] ppc/vof: Fix unaligned FDT property access

[Akihiko Odaki]

FDT properties are aligned by 4 bytes, not 8 bytes.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 hw/ppc/vof.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/ppc/vof.c b/hw/ppc/vof.c
index e3b430a81f4f..b5b6514d79fc 100644
--- a/hw/ppc/vof.c
+++ b/hw/ppc/vof.c
@@ -646,7 +646,7 @@ static void vof_dt_memory_available(void *fdt, GArray *claimed, uint64_t base)
     mem0_reg = fdt_getprop(fdt, offset, "reg", &proplen);
     g_assert(mem0_reg && proplen == sizeof(uint32_t) * (ac + sc));
     if (sc == 2) {
-        mem0_end = be64_to_cpu(*(uint64_t *)(mem0_reg + sizeof(uint32_t) * ac));
+        mem0_end = ldq_be_p(mem0_reg + sizeof(uint32_t) * ac);
     } else {
         mem0_end = be32_to_cpu(*(uint32_t *)(mem0_reg + sizeof(uint32_t) * ac));
     }

-- 
2.45.2

[PATCH 06/14] hw/virtio: Free vqs before vhost_dev_cleanup()

[Akihiko Odaki]

This suppresses LeakSanitizer warnings.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 hw/virtio/vhost-user-base.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/virtio/vhost-user-base.c b/hw/virtio/vhost-user-base.c
index a83167191ee6..124ef536206f 100644
--- a/hw/virtio/vhost-user-base.c
+++ b/hw/virtio/vhost-user-base.c
@@ -223,6 +223,7 @@ static void vub_disconnect(DeviceState *dev)
 {
     VirtIODevice *vdev = VIRTIO_DEVICE(dev);
     VHostUserBase *vub = VHOST_USER_BASE(vdev);
+    struct vhost_virtqueue *vhost_vqs = vub->vhost_dev.vqs;
 
     if (!vub->connected) {
         return;
@@ -231,6 +232,7 @@ static void vub_disconnect(DeviceState *dev)
 
     vub_stop(vdev);
     vhost_dev_cleanup(&vub->vhost_dev);
+    g_free(vhost_vqs);
 
     /* Re-instate the event handler for new connections */
     qemu_chr_fe_set_handlers(&vub->chardev,

-- 
2.45.2

[PATCH 07/14] migration: Free removed SaveStateEntry

[Akihiko Odaki]

This suppresses LeakSanitizer warnings.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 migration/savevm.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/migration/savevm.c b/migration/savevm.c
index c621f2359ba3..10b261823b7c 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -874,6 +874,8 @@ int vmstate_replace_hack_for_ppc(VMStateIf *obj, int instance_id,
 
     if (se) {
         savevm_state_handler_remove(se);
+        g_free(se->compat);
+        g_free(se);
     }
     return vmstate_register(obj, instance_id, vmsd, opaque);
 }

-- 
2.45.2

[PATCH 08/14] memory: Do not create circular reference with subregion

[Akihiko Odaki]

A memory region does not use their own reference counters, but instead
piggybacks on another QOM object, "owner" (unless the owner is not the
memory region itself). When creating a subregion, a new reference to the
owner of the container must be created. However, if the subregion is
owned by the same QOM object, this result in a self-reference, and make
the owner immortal. Avoid such a self-reference.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 system/memory.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/system/memory.c b/system/memory.c
index 9540caa8a1f4..6645da02c658 100644
--- a/system/memory.c
+++ b/system/memory.c
@@ -2638,7 +2638,10 @@ static void memory_region_update_container_subregions(MemoryRegion *subregion)
 
     memory_region_transaction_begin();
 
-    memory_region_ref(subregion);
+    if (mr->owner != subregion->owner) {
+        memory_region_ref(subregion);
+    }
+
     QTAILQ_FOREACH(other, &mr->subregions, subregions_link) {
         if (subregion->priority >= other->priority) {
             QTAILQ_INSERT_BEFORE(other, subregion, subregions_link);
@@ -2696,7 +2699,11 @@ void memory_region_del_subregion(MemoryRegion *mr,
         assert(alias->mapped_via_alias >= 0);
     }
     QTAILQ_REMOVE(&mr->subregions, subregion, subregions_link);
-    memory_region_unref(subregion);
+
+    if (mr->owner != subregion->owner) {
+        memory_region_unref(subregion);
+    }
+
     memory_region_update_pending |= mr->enabled && subregion->enabled;
     memory_region_transaction_commit();
 }

-- 
2.45.2

[PATCH 09/14] tests/qtest: Use qtest_add_data_func_full()

[Akihiko Odaki]

A test function may not be executed depending on the test command line
so it is wrong to free data with a test function. Use
qtest_add_data_func_full() to register a function to free data.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 tests/qtest/device-introspect-test.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/tests/qtest/device-introspect-test.c b/tests/qtest/device-introspect-test.c
index 5b0ffe43f5f4..587da59623dc 100644
--- a/tests/qtest/device-introspect-test.c
+++ b/tests/qtest/device-introspect-test.c
@@ -266,7 +266,6 @@ static void test_device_intro_concrete(const void *args)
 
     qobject_unref(types);
     qtest_quit(qts);
-    g_free((void *)args);
 }
 
 static void test_abstract_interfaces(void)
@@ -310,12 +309,12 @@ static void add_machine_test_case(const char *mname)
 
     path = g_strdup_printf("device/introspect/concrete/defaults/%s", mname);
     args = g_strdup_printf("-M %s", mname);
-    qtest_add_data_func(path, args, test_device_intro_concrete);
+    qtest_add_data_func_full(path, args, test_device_intro_concrete, g_free);
     g_free(path);
 
     path = g_strdup_printf("device/introspect/concrete/nodefaults/%s", mname);
     args = g_strdup_printf("-nodefaults -M %s", mname);
-    qtest_add_data_func(path, args, test_device_intro_concrete);
+    qtest_add_data_func_full(path, args, test_device_intro_concrete, g_free);
     g_free(path);
 }
 
@@ -330,7 +329,7 @@ int main(int argc, char **argv)
     qtest_add_func("device/introspect/abstract-interfaces", test_abstract_interfaces);
     if (g_test_quick()) {
         qtest_add_data_func("device/introspect/concrete/defaults/none",
-                            g_strdup(common_args), test_device_intro_concrete);
+                            common_args, test_device_intro_concrete);
     } else {
         qtest_cb_for_every_machine(add_machine_test_case, true);
     }

-- 
2.45.2

[PATCH 10/14] tests/qtest: Free unused QMP response

[Akihiko Odaki]

This suppresses LeakSanitizer warnings.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 tests/qtest/libqtest.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
index d8f80d335e74..28683fee28b2 100644
--- a/tests/qtest/libqtest.c
+++ b/tests/qtest/libqtest.c
@@ -743,6 +743,8 @@ QDict *qtest_qmp_receive(QTestState *s)
                         response, s->eventData)) {
             /* Stash the event for a later consumption */
             s->pending_events = g_list_append(s->pending_events, response);
+        } else {
+            qobject_unref(response);
         }
     }
 }

-- 
2.45.2

[PATCH 11/14] tests/qtest: Free old machine variable name

[Akihiko Odaki]

This suppresses LeakSanitizer warnings.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 tests/qtest/libqtest.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
index 28683fee28b2..06585104c7af 100644
--- a/tests/qtest/libqtest.c
+++ b/tests/qtest/libqtest.c
@@ -1502,6 +1502,7 @@ static struct MachInfo *qtest_get_machines(const char *var)
     int idx;
 
     if (g_strcmp0(qemu_var, var)) {
+        g_free(qemu_var);
         qemu_var = g_strdup(var);
 
         /* new qemu, clear the cache */

-- 
2.45.2

[PATCH 12/14] tests/qtest: Delete previous boot file

[Akihiko Odaki]

A test run may create boot files several times. Delete the previous boot
file before creating a new one.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 tests/qtest/migration-test.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index b7e3406471a6..5c0d669b6df3 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -129,12 +129,23 @@ static char *bootpath;
 #include "tests/migration/aarch64/a-b-kernel.h"
 #include "tests/migration/s390x/a-b-bios.h"
 
+static void bootfile_delete(void)
+{
+    unlink(bootpath);
+    g_free(bootpath);
+    bootpath = NULL;
+}
+
 static void bootfile_create(char *dir, bool suspend_me)
 {
     const char *arch = qtest_get_arch();
     unsigned char *content;
     size_t len;
 
+    if (bootpath) {
+        bootfile_delete();
+    }
+
     bootpath = g_strdup_printf("%s/bootsect", dir);
     if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
         /* the assembled x86 boot sector should be exactly one sector large */
@@ -164,13 +175,6 @@ static void bootfile_create(char *dir, bool suspend_me)
     fclose(bootfile);
 }
 
-static void bootfile_delete(void)
-{
-    unlink(bootpath);
-    g_free(bootpath);
-    bootpath = NULL;
-}
-
 /*
  * Wait for some output in the serial output file,
  * we get an 'A' followed by an endless string of 'B's

-- 
2.45.2

[PATCH 13/14] tests/qtest: Free paths

[Akihiko Odaki]

This suppresses LeakSanitizer warnings.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 tests/qtest/qos-test.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/tests/qtest/qos-test.c b/tests/qtest/qos-test.c
index 5da4091ec32b..114f6bef273a 100644
--- a/tests/qtest/qos-test.c
+++ b/tests/qtest/qos-test.c
@@ -33,7 +33,6 @@
 static char *old_path;
 
 
-
 /**
  * qos_set_machines_devices_available(): sets availability of qgraph
  * machines and devices.
@@ -191,6 +190,12 @@ static void subprocess_run_one_test(const void *arg)
     g_test_trap_assert_passed();
 }
 
+static void destroy_pathv(void *arg)
+{
+    g_free(((char **)arg)[0]);
+    g_free(arg);
+}
+
 /*
  * in this function, 2 path will be built:
  * path_str, a one-string path (ex "pc/i440FX-pcihost/...")
@@ -295,10 +300,13 @@ static void walk_path(QOSGraphNode *orig_path, int len)
     if (path->u.test.subprocess) {
         gchar *subprocess_path = g_strdup_printf("/%s/%s/subprocess",
                                                  qtest_get_arch(), path_str);
-        qtest_add_data_func(path_str, subprocess_path, subprocess_run_one_test);
-        g_test_add_data_func(subprocess_path, path_vec, run_one_test);
+        qtest_add_data_func_full(path_str, subprocess_path,
+                                 subprocess_run_one_test, g_free);
+        g_test_add_data_func_full(subprocess_path, path_vec,
+                                  run_one_test, destroy_pathv);
     } else {
-        qtest_add_data_func(path_str, path_vec, run_one_test);
+        qtest_add_data_func_full(path_str, path_vec,
+                                 run_one_test, destroy_pathv);
     }
 
     g_free(path_str);

-- 
2.45.2

[PATCH 14/14] tests/qtest: Free GThread

[Akihiko Odaki]

These GThreads are never referenced.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 tests/qtest/vhost-user-test.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tests/qtest/vhost-user-test.c b/tests/qtest/vhost-user-test.c
index d4e437265f66..929af5c183ce 100644
--- a/tests/qtest/vhost-user-test.c
+++ b/tests/qtest/vhost-user-test.c
@@ -928,7 +928,7 @@ static void *vhost_user_test_setup_reconnect(GString *cmd_line, void *arg)
 {
     TestServer *s = test_server_new("reconnect", arg);
 
-    g_thread_new("connect", connect_thread, s);
+    g_thread_unref(g_thread_new("connect", connect_thread, s));
     append_mem_opts(s, cmd_line, 256, TEST_MEMFD_AUTO);
     s->vu_ops->append_opts(s, cmd_line, ",server=on");
 
@@ -965,7 +965,7 @@ static void *vhost_user_test_setup_connect_fail(GString *cmd_line, void *arg)
 
     s->test_fail = true;
 
-    g_thread_new("connect", connect_thread, s);
+    g_thread_unref(g_thread_new("connect", connect_thread, s));
     append_mem_opts(s, cmd_line, 256, TEST_MEMFD_AUTO);
     s->vu_ops->append_opts(s, cmd_line, ",server=on");
 
@@ -980,7 +980,7 @@ static void *vhost_user_test_setup_flags_mismatch(GString *cmd_line, void *arg)
 
     s->test_flags = TEST_FLAGS_DISCONNECT;
 
-    g_thread_new("connect", connect_thread, s);
+    g_thread_unref(g_thread_new("connect", connect_thread, s));
     append_mem_opts(s, cmd_line, 256, TEST_MEMFD_AUTO);
     s->vu_ops->append_opts(s, cmd_line, ",server=on");
 

-- 
2.45.2

Re: [PATCH 06/14] hw/virtio: Free vqs before vhost_dev_cleanup()

[Michael S. Tsirkin]

On Wed, Jun 26, 2024 at 08:06:29PM +0900, Akihiko Odaki wrote:
> This suppresses LeakSanitizer warnings.
> 

more specifically, is there a leak here this fixes?
or a false positive warning?

> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
>  hw/virtio/vhost-user-base.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/hw/virtio/vhost-user-base.c b/hw/virtio/vhost-user-base.c
> index a83167191ee6..124ef536206f 100644
> --- a/hw/virtio/vhost-user-base.c
> +++ b/hw/virtio/vhost-user-base.c
> @@ -223,6 +223,7 @@ static void vub_disconnect(DeviceState *dev)
>  {
>      VirtIODevice *vdev = VIRTIO_DEVICE(dev);
>      VHostUserBase *vub = VHOST_USER_BASE(vdev);
> +    struct vhost_virtqueue *vhost_vqs = vub->vhost_dev.vqs;
>  
>      if (!vub->connected) {
>          return;
> @@ -231,6 +232,7 @@ static void vub_disconnect(DeviceState *dev)
>  
>      vub_stop(vdev);
>      vhost_dev_cleanup(&vub->vhost_dev);
> +    g_free(vhost_vqs);
>  


code does not match $subj

>      /* Re-instate the event handler for new connections */
>      qemu_chr_fe_set_handlers(&vub->chardev,
> 
> -- 
> 2.45.2

Re: [PATCH 04/14] spapr: Free stdout path

[Philippe Mathieu-Daudé]

On 26/6/24 13:06, Akihiko Odaki wrote:
> This suppresses LeakSanitizer warnings.
> 
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
>   hw/ppc/spapr_vof.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

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

Re: [PATCH 05/14] ppc/vof: Fix unaligned FDT property access

[Philippe Mathieu-Daudé]

On 26/6/24 13:06, Akihiko Odaki wrote:
> FDT properties are aligned by 4 bytes, not 8 bytes.
> 
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
>   hw/ppc/vof.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/vof.c b/hw/ppc/vof.c
> index e3b430a81f4f..b5b6514d79fc 100644
> --- a/hw/ppc/vof.c
> +++ b/hw/ppc/vof.c
> @@ -646,7 +646,7 @@ static void vof_dt_memory_available(void *fdt, GArray *claimed, uint64_t base)
>       mem0_reg = fdt_getprop(fdt, offset, "reg", &proplen);
>       g_assert(mem0_reg && proplen == sizeof(uint32_t) * (ac + sc));
>       if (sc == 2) {
> -        mem0_end = be64_to_cpu(*(uint64_t *)(mem0_reg + sizeof(uint32_t) * ac));
> +        mem0_end = ldq_be_p(mem0_reg + sizeof(uint32_t) * ac);
>       } else {
>           mem0_end = be32_to_cpu(*(uint32_t *)(mem0_reg + sizeof(uint32_t) * ac));

OK but please keep API uses consistent, so convert other uses please.

>       }
> 

Re: [PATCH 01/14] hw/core: Free CPUState allocations

[Philippe Mathieu-Daudé]

On 26/6/24 13:06, Akihiko Odaki wrote:
> This suppresses LeakSanitizer warnings.
> 
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
>   hw/core/cpu-common.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c
> index 0f0a247f5642..42f38b01a97f 100644
> --- a/hw/core/cpu-common.c
> +++ b/hw/core/cpu-common.c
> @@ -274,6 +274,9 @@ static void cpu_common_finalize(Object *obj)
>   {
>       CPUState *cpu = CPU(obj);
>   
> +    g_free(cpu->thread);
> +    g_free(cpu->halt_cond);
> +    g_free(cpu->cpu_ases);
>       g_array_free(cpu->gdb_regs, TRUE);
>       qemu_lockcnt_destroy(&cpu->in_ioctl_lock);
>       qemu_mutex_destroy(&cpu->work_mutex);
> 

Similar patch queued via trivial tree:
https://lore.kernel.org/qemu-devel/3ad18bc590ef28e1526e8053568086b453e7ffde.1718211878.git.quic_mathbern@quicinc.com/

Re: [PATCH 03/14] hw/isa/vt82c686: Free irqs

[Peter Maydell]

On Wed, 26 Jun 2024 at 12:08, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> This suppresses LeakSanitizer warnings.
>
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
>  hw/isa/vt82c686.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
> index 8582ac0322eb..189b487f1d22 100644
> --- a/hw/isa/vt82c686.c
> +++ b/hw/isa/vt82c686.c
> @@ -721,7 +721,6 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
>
>      qdev_init_gpio_out(dev, &s->cpu_intr, 1);
>      qdev_init_gpio_in_named(dev, via_isa_pirq, "pirq", PCI_NUM_PINS);
> -    isa_irq = qemu_allocate_irqs(via_isa_request_i8259_irq, s, 1);
>      isa_bus = isa_bus_new(dev, pci_address_space(d), pci_address_space_io(d),
>                            errp);
>
> @@ -729,7 +728,9 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
>          return;
>      }
>
> +    isa_irq = qemu_allocate_irqs(via_isa_request_i8259_irq, s, 1);
>      s->isa_irqs_in = i8259_init(isa_bus, *isa_irq);
> +    qemu_free_irqs(isa_irq, 1);

This looks pretty weird -- we allocate the irq array, and
then immediately free it. The memory ought not to be
freed until the end of the simulation.

The ideal way to resolve this kind of leak with qemu_allocate_irqs()
is to avoid using the function entirely, and instead use IRQ
arrays allocated via qdev_init_gpio_* or the sysbus IRQ APIs.
qemu_allocate_irqs() is old and a sort of inherently leaky API.

The less ideal way is to keep the pointer to the array in the
device stuct.

If you look through 'git log' for qemu_allocate_irqs() you'll
see various places in the past where we've refactored things
to avoid this kind of uninteresting memory leak.

>      isa_bus_register_input_irqs(isa_bus, s->isa_irqs_in);
>      i8254_pit_init(isa_bus, 0x40, 0, NULL);
>      i8257_dma_init(OBJECT(d), isa_bus, 0);

thanks
-- PMM

Re: [PATCH 02/14] hw/ide: Free macio-ide IRQs

[Peter Maydell]

On Wed, 26 Jun 2024 at 12:09, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> This suppresses LeakSanitizer warnings.
>
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
>  hw/ide/macio.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/hw/ide/macio.c b/hw/ide/macio.c
> index aca90d04f0e8..d8fbc1a17ba6 100644
> --- a/hw/ide/macio.c
> +++ b/hw/ide/macio.c
> @@ -464,6 +464,14 @@ static void macio_ide_initfn(Object *obj)
>                               qdev_prop_allow_set_link_before_realize, 0);
>  }
>
> +static void macio_ide_finalize(Object *obj)
> +{
> +    MACIOIDEState *s = MACIO_IDE(obj);
> +
> +    qemu_free_irq(s->dma_irq);
> +    qemu_free_irq(s->ide_irq);
> +}
> +
>  static Property macio_ide_properties[] = {
>      DEFINE_PROP_UINT32("channel", MACIOIDEState, channel, 0),
>      DEFINE_PROP_UINT32("addr", MACIOIDEState, addr, -1),
> @@ -486,6 +494,7 @@ static const TypeInfo macio_ide_type_info = {
>      .parent = TYPE_SYS_BUS_DEVICE,
>      .instance_size = sizeof(MACIOIDEState),
>      .instance_init = macio_ide_initfn,
> +    .instance_finalize = macio_ide_finalize,
>      .class_init = macio_ide_class_init,
>  };

Rather than this, I suspect macio_ide_initfn() should not
be using qemu_allocate_irq() in the first place. Looks like
maybe a QOM conversion that left a loose end un-tidied-up.

thanks
-- PMM

Re: [PATCH 07/14] migration: Free removed SaveStateEntry

[Peter Xu]

On Wed, Jun 26, 2024 at 08:06:30PM +0900, Akihiko Odaki wrote:
> This suppresses LeakSanitizer warnings.
> 
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

Re: [PATCH 05/14] ppc/vof: Fix unaligned FDT property access

[Akihiko Odaki]

On 2024/06/26 21:03, Philippe Mathieu-Daudé wrote:
> On 26/6/24 13:06, Akihiko Odaki wrote:
>> FDT properties are aligned by 4 bytes, not 8 bytes.
>>
>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>> ---
>>   hw/ppc/vof.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/ppc/vof.c b/hw/ppc/vof.c
>> index e3b430a81f4f..b5b6514d79fc 100644
>> --- a/hw/ppc/vof.c
>> +++ b/hw/ppc/vof.c
>> @@ -646,7 +646,7 @@ static void vof_dt_memory_available(void *fdt, 
>> GArray *claimed, uint64_t base)
>>       mem0_reg = fdt_getprop(fdt, offset, "reg", &proplen);
>>       g_assert(mem0_reg && proplen == sizeof(uint32_t) * (ac + sc));
>>       if (sc == 2) {
>> -        mem0_end = be64_to_cpu(*(uint64_t *)(mem0_reg + 
>> sizeof(uint32_t) * ac));
>> +        mem0_end = ldq_be_p(mem0_reg + sizeof(uint32_t) * ac);
>>       } else {
>>           mem0_end = be32_to_cpu(*(uint32_t *)(mem0_reg + 
>> sizeof(uint32_t) * ac));
> 
> OK but please keep API uses consistent, so convert other uses please.

This is the only unaligned access.

Re: [PATCH 05/14] ppc/vof: Fix unaligned FDT property access

[Philippe Mathieu-Daudé]

On 27/6/24 15:12, Akihiko Odaki wrote:
> On 2024/06/26 21:03, Philippe Mathieu-Daudé wrote:
>> On 26/6/24 13:06, Akihiko Odaki wrote:
>>> FDT properties are aligned by 4 bytes, not 8 bytes.
>>>
>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>>> ---
>>>   hw/ppc/vof.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/hw/ppc/vof.c b/hw/ppc/vof.c
>>> index e3b430a81f4f..b5b6514d79fc 100644
>>> --- a/hw/ppc/vof.c
>>> +++ b/hw/ppc/vof.c
>>> @@ -646,7 +646,7 @@ static void vof_dt_memory_available(void *fdt, 
>>> GArray *claimed, uint64_t base)
>>>       mem0_reg = fdt_getprop(fdt, offset, "reg", &proplen);
>>>       g_assert(mem0_reg && proplen == sizeof(uint32_t) * (ac + sc));
>>>       if (sc == 2) {
>>> -        mem0_end = be64_to_cpu(*(uint64_t *)(mem0_reg + 
>>> sizeof(uint32_t) * ac));
>>> +        mem0_end = ldq_be_p(mem0_reg + sizeof(uint32_t) * ac);
>>>       } else {
>>>           mem0_end = be32_to_cpu(*(uint32_t *)(mem0_reg + 
>>> sizeof(uint32_t) * ac));
>>
>> OK but please keep API uses consistent, so convert other uses please.
> 
> This is the only unaligned access.

What I mean with consistent API use is either use the be64_to_cpu and
be32_to_cpu API, or ldq_be_p and ldl_be_p. A mix of both makes review
more confusing.

Re: [PATCH 05/14] ppc/vof: Fix unaligned FDT property access

[Akihiko Odaki]

On 2024/06/27 23:02, Philippe Mathieu-Daudé wrote:
> On 27/6/24 15:12, Akihiko Odaki wrote:
>> On 2024/06/26 21:03, Philippe Mathieu-Daudé wrote:
>>> On 26/6/24 13:06, Akihiko Odaki wrote:
>>>> FDT properties are aligned by 4 bytes, not 8 bytes.
>>>>
>>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>>>> ---
>>>>   hw/ppc/vof.c | 2 +-
>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/hw/ppc/vof.c b/hw/ppc/vof.c
>>>> index e3b430a81f4f..b5b6514d79fc 100644
>>>> --- a/hw/ppc/vof.c
>>>> +++ b/hw/ppc/vof.c
>>>> @@ -646,7 +646,7 @@ static void vof_dt_memory_available(void *fdt, 
>>>> GArray *claimed, uint64_t base)
>>>>       mem0_reg = fdt_getprop(fdt, offset, "reg", &proplen);
>>>>       g_assert(mem0_reg && proplen == sizeof(uint32_t) * (ac + sc));
>>>>       if (sc == 2) {
>>>> -        mem0_end = be64_to_cpu(*(uint64_t *)(mem0_reg + 
>>>> sizeof(uint32_t) * ac));
>>>> +        mem0_end = ldq_be_p(mem0_reg + sizeof(uint32_t) * ac);
>>>>       } else {
>>>>           mem0_end = be32_to_cpu(*(uint32_t *)(mem0_reg + 
>>>> sizeof(uint32_t) * ac));
>>>
>>> OK but please keep API uses consistent, so convert other uses please.
>>
>> This is the only unaligned access.
> 
> What I mean with consistent API use is either use the be64_to_cpu and
> be32_to_cpu API, or ldq_be_p and ldl_be_p. A mix of both makes review
> more confusing.

The desired semantics are different in these two cases so I believe it 
is natural to use different APIs; ldq_be_p() for an unaligned 64-bit 
access and be32_to_cpu(*(uint32_t *)) for an aligned 32-bit access.

Re: [PATCH 02/14] hw/ide: Free macio-ide IRQs

[Mark Cave-Ayland]

On 26/06/2024 13:59, Peter Maydell wrote:

> On Wed, 26 Jun 2024 at 12:09, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>
>> This suppresses LeakSanitizer warnings.
>>
>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>> ---
>>   hw/ide/macio.c | 9 +++++++++
>>   1 file changed, 9 insertions(+)
>>
>> diff --git a/hw/ide/macio.c b/hw/ide/macio.c
>> index aca90d04f0e8..d8fbc1a17ba6 100644
>> --- a/hw/ide/macio.c
>> +++ b/hw/ide/macio.c
>> @@ -464,6 +464,14 @@ static void macio_ide_initfn(Object *obj)
>>                                qdev_prop_allow_set_link_before_realize, 0);
>>   }
>>
>> +static void macio_ide_finalize(Object *obj)
>> +{
>> +    MACIOIDEState *s = MACIO_IDE(obj);
>> +
>> +    qemu_free_irq(s->dma_irq);
>> +    qemu_free_irq(s->ide_irq);
>> +}
>> +
>>   static Property macio_ide_properties[] = {
>>       DEFINE_PROP_UINT32("channel", MACIOIDEState, channel, 0),
>>       DEFINE_PROP_UINT32("addr", MACIOIDEState, addr, -1),
>> @@ -486,6 +494,7 @@ static const TypeInfo macio_ide_type_info = {
>>       .parent = TYPE_SYS_BUS_DEVICE,
>>       .instance_size = sizeof(MACIOIDEState),
>>       .instance_init = macio_ide_initfn,
>> +    .instance_finalize = macio_ide_finalize,
>>       .class_init = macio_ide_class_init,
>>   };
> 
> Rather than this, I suspect macio_ide_initfn() should not
> be using qemu_allocate_irq() in the first place. Looks like
> maybe a QOM conversion that left a loose end un-tidied-up.

This is definitely old code: there used to be problems interfacing the IDE code with 
qdev due to the hard-coded bus IRQs but I think this may is now possible with the 
advent of ide_bus_init_output_irq().

I'll have a quick look and see what has changed in this area.


ATB,

Mark.