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

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

[Akihiko Odaki]

Based-on: <3ad18bc590ef28e1526e8053568086b453e7ffde.1718211878.git.quic_mathbern@quicinc.com>
("[PATCH] cpu: fix memleak of 'halt_cond' and 'thread'")

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>
---
Changes in v2:
- Rebased to "[PATCH] cpu: fix memleak of 'halt_cond' and 'thread'".
  (Philippe Mathieu-Daudé)
- Converted IRQs into GPIO lines and removed one qemu_irq usage.
  (Peter Maydell)
- s/suppresses/fixes/ (Michael S. Tsirkin)
- Corrected title of patch "hw/virtio: Free vqs after vhost_dev_cleanup()"
  (was "hw/virtio: Free vqs before vhost_dev_cleanup()")
- Link to v1: https://lore.kernel.org/r/20240626-san-v1-0-f3cc42302189@daynix.com

---
Akihiko Odaki (15):
      cpu: Free cpu_ases
      hw/ide: Convert macio ide_irq into GPIO line
      hw/ide: Remove internal DMA qemu_irq
      hw/isa/vt82c686: Define a GPIO line between vt82c686 and i8259
      spapr: Free stdout path
      ppc/vof: Fix unaligned FDT property access
      hw/virtio: Free vqs after 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

 include/hw/ppc/mac_dbdma.h           |  5 +++--
 hw/core/cpu-common.c                 |  1 +
 hw/ide/macio.c                       | 18 +++++++++++++-----
 hw/isa/vt82c686.c                    |  7 ++++---
 hw/misc/macio/mac_dbdma.c            | 10 +++++-----
 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 +++---
 15 files changed, 73 insertions(+), 37 deletions(-)
---
base-commit: af799a2337c3e39994411f90631905d809a41da4
change-id: 20240625-san-097afaf4f1c2

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

[PATCH v2 01/15] cpu: Free cpu_ases

[Akihiko Odaki]

This fixes LeakSanitizer warnings.

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

diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c
index f131cde2c038..a3073c17d098 100644
--- a/hw/core/cpu-common.c
+++ b/hw/core/cpu-common.c
@@ -289,6 +289,7 @@ static void cpu_common_finalize(Object *obj)
     qemu_cond_destroy(cpu->halt_cond);
     g_free(cpu->halt_cond);
     g_free(cpu->thread);
+    g_free(cpu->cpu_ases);
 }
 
 static int64_t cpu_common_get_arch_id(CPUState *cpu)

-- 
2.45.2

Re: [PATCH v2 01/15] cpu: Free cpu_ases

[Peter Maydell]

On Thu, 27 Jun 2024 at 14:39, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> This fixes LeakSanitizer warnings.
>
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
>  hw/core/cpu-common.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c
> index f131cde2c038..a3073c17d098 100644
> --- a/hw/core/cpu-common.c
> +++ b/hw/core/cpu-common.c
> @@ -289,6 +289,7 @@ static void cpu_common_finalize(Object *obj)
>      qemu_cond_destroy(cpu->halt_cond);
>      g_free(cpu->halt_cond);
>      g_free(cpu->thread);
> +    g_free(cpu->cpu_ases);

I think this is likely not sufficient. There's a patch lurking
in the vcpu-hotplug series:

https://lore.kernel.org/qemu-devel/20240607115649.214622-7-salil.mehta@huawei.com/

which adds a cpu_address_space_destroy() function, which is
probably what we need to have happen on CPU unrealize.

NB that that patch isn't actually sufficient, though:
see discussion here on previous version of patchset
https://lore.kernel.org/qemu-devel/CAFEAcA92nCPPk0Qa6XjRqRGTq_XDyRSVVaz67WgJBEZcxoEtOQ@mail.gmail.com/
and the link from there to a different earlier patch from Philippe.

thanks
-- PMM

[PATCH v2 02/15] hw/ide: Convert macio ide_irq into GPIO line

[Akihiko Odaki]

macio ide_irq is connected to the IDE bus. This fixes the leak of
ide_irq.

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

diff --git a/hw/ide/macio.c b/hw/ide/macio.c
index aca90d04f0e8..9c96a857a7c1 100644
--- a/hw/ide/macio.c
+++ b/hw/ide/macio.c
@@ -427,7 +427,7 @@ static void macio_ide_realizefn(DeviceState *dev, Error **errp)
     s->bus.dma = &s->dma;
 }
 
-static void pmac_ide_irq(void *opaque, int n, int level)
+static void pmac_irq(void *opaque, int n, int level)
 {
     MACIOIDEState *s = opaque;
     uint32_t mask = 0x80000000u >> n;
@@ -446,6 +446,11 @@ static void pmac_ide_irq(void *opaque, int n, int level)
     }
 }
 
+static void pmac_ide_irq(void *opaque, int n, int level)
+{
+    pmac_irq(opaque, 1, level);
+}
+
 static void macio_ide_initfn(Object *obj)
 {
     SysBusDevice *d = SYS_BUS_DEVICE(obj);
@@ -456,8 +461,8 @@ static void macio_ide_initfn(Object *obj)
     sysbus_init_mmio(d, &s->mem);
     sysbus_init_irq(d, &s->real_ide_irq);
     sysbus_init_irq(d, &s->real_dma_irq);
-    s->dma_irq = qemu_allocate_irq(pmac_ide_irq, s, 0);
-    s->ide_irq = qemu_allocate_irq(pmac_ide_irq, s, 1);
+    s->dma_irq = qemu_allocate_irq(pmac_irq, s, 0);
+    qdev_init_gpio_in_named_with_opaque(DEVICE(obj), pmac_ide_irq, s, NULL, 1);
 
     object_property_add_link(obj, "dbdma", TYPE_MAC_DBDMA,
                              (Object **) &s->dbdma,

-- 
2.45.2

Re: [PATCH v2 02/15] hw/ide: Convert macio ide_irq into GPIO line

[Mark Cave-Ayland]

On 27/06/2024 14:37, Akihiko Odaki wrote:

> macio ide_irq is connected to the IDE bus. This fixes the leak of
> ide_irq.
> 
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
>   hw/ide/macio.c | 11 ++++++++---
>   1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/ide/macio.c b/hw/ide/macio.c
> index aca90d04f0e8..9c96a857a7c1 100644
> --- a/hw/ide/macio.c
> +++ b/hw/ide/macio.c
> @@ -427,7 +427,7 @@ static void macio_ide_realizefn(DeviceState *dev, Error **errp)
>       s->bus.dma = &s->dma;
>   }
>   
> -static void pmac_ide_irq(void *opaque, int n, int level)
> +static void pmac_irq(void *opaque, int n, int level)
>   {
>       MACIOIDEState *s = opaque;
>       uint32_t mask = 0x80000000u >> n;
> @@ -446,6 +446,11 @@ static void pmac_ide_irq(void *opaque, int n, int level)
>       }
>   }
>   
> +static void pmac_ide_irq(void *opaque, int n, int level)
> +{
> +    pmac_irq(opaque, 1, level);
> +}
> +
>   static void macio_ide_initfn(Object *obj)
>   {
>       SysBusDevice *d = SYS_BUS_DEVICE(obj);
> @@ -456,8 +461,8 @@ static void macio_ide_initfn(Object *obj)
>       sysbus_init_mmio(d, &s->mem);
>       sysbus_init_irq(d, &s->real_ide_irq);
>       sysbus_init_irq(d, &s->real_dma_irq);
> -    s->dma_irq = qemu_allocate_irq(pmac_ide_irq, s, 0);
> -    s->ide_irq = qemu_allocate_irq(pmac_ide_irq, s, 1);
> +    s->dma_irq = qemu_allocate_irq(pmac_irq, s, 0);
> +    qdev_init_gpio_in_named_with_opaque(DEVICE(obj), pmac_ide_irq, s, NULL, 1);
>   
>       object_property_add_link(obj, "dbdma", TYPE_MAC_DBDMA,
>                                (Object **) &s->dbdma,
> 

This doesn't feel quite right: generally I consider the use of 
qdev_init_gpio_in_named_with_opaque() to indicate that the underlying modelling is 
incorrect. Let me have a look and see if I can figure out what's supposed to be 
happening.

I guess I should probably be marked as maintainer of hw/ide/macio.c as it is part of 
the macio device, but it looks as if this is missing from MAINTAINERS.


ATB,

Mark.

[PATCH v2 03/15] hw/ide: Remove internal DMA qemu_irq

[Akihiko Odaki]

A function pointer is sufficient for internal usage. Replacing qemu_irq
with one fixes the leak of qemu_irq.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 include/hw/ppc/mac_dbdma.h |  5 +++--
 hw/ide/macio.c             | 11 +++++++----
 hw/misc/macio/mac_dbdma.c  | 10 +++++-----
 3 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/include/hw/ppc/mac_dbdma.h b/include/hw/ppc/mac_dbdma.h
index 4a3f644516b3..1e1973c39580 100644
--- a/include/hw/ppc/mac_dbdma.h
+++ b/include/hw/ppc/mac_dbdma.h
@@ -32,6 +32,7 @@
 typedef struct DBDMA_io DBDMA_io;
 
 typedef void (*DBDMA_flush)(DBDMA_io *io);
+typedef void (*DBDMA_irq)(DBDMA_io *io);
 typedef void (*DBDMA_rw)(DBDMA_io *io);
 typedef void (*DBDMA_end)(DBDMA_io *io);
 struct DBDMA_io {
@@ -154,7 +155,7 @@ typedef struct dbdma_cmd {
 typedef struct DBDMA_channel {
     int channel;
     uint32_t regs[DBDMA_REGS];
-    qemu_irq irq;
+    DBDMA_irq irq;
     DBDMA_io io;
     DBDMA_rw rw;
     DBDMA_flush flush;
@@ -172,7 +173,7 @@ typedef struct DBDMAState DBDMAState;
 
 /* Externally callable functions */
 
-void DBDMA_register_channel(void *dbdma, int nchan, qemu_irq irq,
+void DBDMA_register_channel(void *dbdma, int nchan, DBDMA_irq irq,
                             DBDMA_rw rw, DBDMA_flush flush,
                             void *opaque);
 void DBDMA_kick(DBDMAState *dbdma);
diff --git a/hw/ide/macio.c b/hw/ide/macio.c
index 9c96a857a7c1..425b670a52a9 100644
--- a/hw/ide/macio.c
+++ b/hw/ide/macio.c
@@ -427,9 +427,8 @@ static void macio_ide_realizefn(DeviceState *dev, Error **errp)
     s->bus.dma = &s->dma;
 }
 
-static void pmac_irq(void *opaque, int n, int level)
+static void pmac_irq(MACIOIDEState *s, int n, int level)
 {
-    MACIOIDEState *s = opaque;
     uint32_t mask = 0x80000000u >> n;
 
     /* We need to reflect the IRQ state in the irq register */
@@ -446,6 +445,11 @@ static void pmac_irq(void *opaque, int n, int level)
     }
 }
 
+static void pmac_dma_irq(DBDMA_io *io)
+{
+    pmac_irq(io->opaque, 0, 1);
+}
+
 static void pmac_ide_irq(void *opaque, int n, int level)
 {
     pmac_irq(opaque, 1, level);
@@ -461,7 +465,6 @@ static void macio_ide_initfn(Object *obj)
     sysbus_init_mmio(d, &s->mem);
     sysbus_init_irq(d, &s->real_ide_irq);
     sysbus_init_irq(d, &s->real_dma_irq);
-    s->dma_irq = qemu_allocate_irq(pmac_irq, s, 0);
     qdev_init_gpio_in_named_with_opaque(DEVICE(obj), pmac_ide_irq, s, NULL, 1);
 
     object_property_add_link(obj, "dbdma", TYPE_MAC_DBDMA,
@@ -513,7 +516,7 @@ void macio_ide_init_drives(MACIOIDEState *s, DriveInfo **hd_table)
 
 void macio_ide_register_dma(MACIOIDEState *s)
 {
-    DBDMA_register_channel(s->dbdma, s->channel, s->dma_irq,
+    DBDMA_register_channel(s->dbdma, s->channel, pmac_dma_irq,
                            pmac_ide_transfer, pmac_ide_flush, s);
 }
 
diff --git a/hw/misc/macio/mac_dbdma.c b/hw/misc/macio/mac_dbdma.c
index 2a528ea08caf..3450105ad851 100644
--- a/hw/misc/macio/mac_dbdma.c
+++ b/hw/misc/macio/mac_dbdma.c
@@ -114,7 +114,7 @@ static void kill_channel(DBDMA_channel *ch)
     ch->regs[DBDMA_STATUS] |= DEAD;
     ch->regs[DBDMA_STATUS] &= ~ACTIVE;
 
-    qemu_irq_raise(ch->irq);
+    ch->irq(&ch->io);
 }
 
 static void conditional_interrupt(DBDMA_channel *ch)
@@ -133,7 +133,7 @@ static void conditional_interrupt(DBDMA_channel *ch)
     case INTR_NEVER:  /* don't interrupt */
         return;
     case INTR_ALWAYS: /* always interrupt */
-        qemu_irq_raise(ch->irq);
+            ch->irq(&ch->io);
         DBDMA_DPRINTFCH(ch, "%s: raise\n", __func__);
         return;
     }
@@ -148,13 +148,13 @@ static void conditional_interrupt(DBDMA_channel *ch)
     switch(intr) {
     case INTR_IFSET:  /* intr if condition bit is 1 */
         if (cond) {
-            qemu_irq_raise(ch->irq);
+            ch->irq(&ch->io);
             DBDMA_DPRINTFCH(ch, "%s: raise\n", __func__);
         }
         return;
     case INTR_IFCLR:  /* intr if condition bit is 0 */
         if (!cond) {
-            qemu_irq_raise(ch->irq);
+            ch->irq(&ch->io);
             DBDMA_DPRINTFCH(ch, "%s: raise\n", __func__);
         }
         return;
@@ -562,7 +562,7 @@ void DBDMA_kick(DBDMAState *dbdma)
     qemu_bh_schedule(dbdma->bh);
 }
 
-void DBDMA_register_channel(void *dbdma, int nchan, qemu_irq irq,
+void DBDMA_register_channel(void *dbdma, int nchan, DBDMA_irq irq,
                             DBDMA_rw rw, DBDMA_flush flush,
                             void *opaque)
 {

-- 
2.45.2

Re: [PATCH v2 03/15] hw/ide: Remove internal DMA qemu_irq

[Mark Cave-Ayland]

On 27/06/2024 14:37, Akihiko Odaki wrote:

> A function pointer is sufficient for internal usage. Replacing qemu_irq
> with one fixes the leak of qemu_irq.
> 
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
>   include/hw/ppc/mac_dbdma.h |  5 +++--
>   hw/ide/macio.c             | 11 +++++++----
>   hw/misc/macio/mac_dbdma.c  | 10 +++++-----
>   3 files changed, 15 insertions(+), 11 deletions(-)
> 
> diff --git a/include/hw/ppc/mac_dbdma.h b/include/hw/ppc/mac_dbdma.h
> index 4a3f644516b3..1e1973c39580 100644
> --- a/include/hw/ppc/mac_dbdma.h
> +++ b/include/hw/ppc/mac_dbdma.h
> @@ -32,6 +32,7 @@
>   typedef struct DBDMA_io DBDMA_io;
>   
>   typedef void (*DBDMA_flush)(DBDMA_io *io);
> +typedef void (*DBDMA_irq)(DBDMA_io *io);
>   typedef void (*DBDMA_rw)(DBDMA_io *io);
>   typedef void (*DBDMA_end)(DBDMA_io *io);
>   struct DBDMA_io {
> @@ -154,7 +155,7 @@ typedef struct dbdma_cmd {
>   typedef struct DBDMA_channel {
>       int channel;
>       uint32_t regs[DBDMA_REGS];
> -    qemu_irq irq;
> +    DBDMA_irq irq;
>       DBDMA_io io;
>       DBDMA_rw rw;
>       DBDMA_flush flush;
> @@ -172,7 +173,7 @@ typedef struct DBDMAState DBDMAState;
>   
>   /* Externally callable functions */
>   
> -void DBDMA_register_channel(void *dbdma, int nchan, qemu_irq irq,
> +void DBDMA_register_channel(void *dbdma, int nchan, DBDMA_irq irq,
>                               DBDMA_rw rw, DBDMA_flush flush,
>                               void *opaque);
>   void DBDMA_kick(DBDMAState *dbdma);
> diff --git a/hw/ide/macio.c b/hw/ide/macio.c
> index 9c96a857a7c1..425b670a52a9 100644
> --- a/hw/ide/macio.c
> +++ b/hw/ide/macio.c
> @@ -427,9 +427,8 @@ static void macio_ide_realizefn(DeviceState *dev, Error **errp)
>       s->bus.dma = &s->dma;
>   }
>   
> -static void pmac_irq(void *opaque, int n, int level)
> +static void pmac_irq(MACIOIDEState *s, int n, int level)
>   {
> -    MACIOIDEState *s = opaque;
>       uint32_t mask = 0x80000000u >> n;
>   
>       /* We need to reflect the IRQ state in the irq register */
> @@ -446,6 +445,11 @@ static void pmac_irq(void *opaque, int n, int level)
>       }
>   }
>   
> +static void pmac_dma_irq(DBDMA_io *io)
> +{
> +    pmac_irq(io->opaque, 0, 1);
> +}
> +
>   static void pmac_ide_irq(void *opaque, int n, int level)
>   {
>       pmac_irq(opaque, 1, level);
> @@ -461,7 +465,6 @@ static void macio_ide_initfn(Object *obj)
>       sysbus_init_mmio(d, &s->mem);
>       sysbus_init_irq(d, &s->real_ide_irq);
>       sysbus_init_irq(d, &s->real_dma_irq);
> -    s->dma_irq = qemu_allocate_irq(pmac_irq, s, 0);
>       qdev_init_gpio_in_named_with_opaque(DEVICE(obj), pmac_ide_irq, s, NULL, 1);
>   
>       object_property_add_link(obj, "dbdma", TYPE_MAC_DBDMA,
> @@ -513,7 +516,7 @@ void macio_ide_init_drives(MACIOIDEState *s, DriveInfo **hd_table)
>   
>   void macio_ide_register_dma(MACIOIDEState *s)
>   {
> -    DBDMA_register_channel(s->dbdma, s->channel, s->dma_irq,
> +    DBDMA_register_channel(s->dbdma, s->channel, pmac_dma_irq,
>                              pmac_ide_transfer, pmac_ide_flush, s);
>   }
>   
> diff --git a/hw/misc/macio/mac_dbdma.c b/hw/misc/macio/mac_dbdma.c
> index 2a528ea08caf..3450105ad851 100644
> --- a/hw/misc/macio/mac_dbdma.c
> +++ b/hw/misc/macio/mac_dbdma.c
> @@ -114,7 +114,7 @@ static void kill_channel(DBDMA_channel *ch)
>       ch->regs[DBDMA_STATUS] |= DEAD;
>       ch->regs[DBDMA_STATUS] &= ~ACTIVE;
>   
> -    qemu_irq_raise(ch->irq);
> +    ch->irq(&ch->io);
>   }
>   
>   static void conditional_interrupt(DBDMA_channel *ch)
> @@ -133,7 +133,7 @@ static void conditional_interrupt(DBDMA_channel *ch)
>       case INTR_NEVER:  /* don't interrupt */
>           return;
>       case INTR_ALWAYS: /* always interrupt */
> -        qemu_irq_raise(ch->irq);
> +            ch->irq(&ch->io);
>           DBDMA_DPRINTFCH(ch, "%s: raise\n", __func__);
>           return;
>       }
> @@ -148,13 +148,13 @@ static void conditional_interrupt(DBDMA_channel *ch)
>       switch(intr) {
>       case INTR_IFSET:  /* intr if condition bit is 1 */
>           if (cond) {
> -            qemu_irq_raise(ch->irq);
> +            ch->irq(&ch->io);
>               DBDMA_DPRINTFCH(ch, "%s: raise\n", __func__);
>           }
>           return;
>       case INTR_IFCLR:  /* intr if condition bit is 0 */
>           if (!cond) {
> -            qemu_irq_raise(ch->irq);
> +            ch->irq(&ch->io);
>               DBDMA_DPRINTFCH(ch, "%s: raise\n", __func__);
>           }
>           return;
> @@ -562,7 +562,7 @@ void DBDMA_kick(DBDMAState *dbdma)
>       qemu_bh_schedule(dbdma->bh);
>   }
>   
> -void DBDMA_register_channel(void *dbdma, int nchan, qemu_irq irq,
> +void DBDMA_register_channel(void *dbdma, int nchan, DBDMA_irq irq,
>                               DBDMA_rw rw, DBDMA_flush flush,
>                               void *opaque)
>   {

At first glance I can't say I'm keen on this: in general we should be moving towards 
standardising on QEMU irqs or qdev gpios rather than introducing a custom function.

As per my previous email I suspect this is another symptom that something is wrong 
with the modelling, so I will take a look.


ATB,

Mark.

[PATCH v2 04/15] hw/isa/vt82c686: Define a GPIO line between vt82c686 and i8259

[Akihiko Odaki]

This fixes qemu_irq array leak.

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

diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index 8582ac0322eb..629d2d568137 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -715,13 +715,14 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
     ViaISAState *s = VIA_ISA(d);
     DeviceState *dev = DEVICE(d);
     PCIBus *pci_bus = pci_get_bus(d);
-    qemu_irq *isa_irq;
+    qemu_irq isa_irq;
     ISABus *isa_bus;
     int i;
 
     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);
+    qdev_init_gpio_in_named(dev, via_isa_request_i8259_irq, "i8259", 1);
+    isa_irq = qdev_get_gpio_in_named(dev, "i8259", 0);
     isa_bus = isa_bus_new(dev, pci_address_space(d), pci_address_space_io(d),
                           errp);
 
@@ -729,7 +730,7 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
         return;
     }
 
-    s->isa_irqs_in = i8259_init(isa_bus, *isa_irq);
+    s->isa_irqs_in = i8259_init(isa_bus, isa_irq);
     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

Re: [PATCH v2 04/15] hw/isa/vt82c686: Define a GPIO line between vt82c686 and i8259

[Mark Cave-Ayland]

On 27/06/2024 14:37, Akihiko Odaki wrote:

> This fixes qemu_irq array leak.
> 
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
>   hw/isa/vt82c686.c | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
> index 8582ac0322eb..629d2d568137 100644
> --- a/hw/isa/vt82c686.c
> +++ b/hw/isa/vt82c686.c
> @@ -715,13 +715,14 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
>       ViaISAState *s = VIA_ISA(d);
>       DeviceState *dev = DEVICE(d);
>       PCIBus *pci_bus = pci_get_bus(d);
> -    qemu_irq *isa_irq;
> +    qemu_irq isa_irq;
>       ISABus *isa_bus;
>       int i;
>   
>       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);
> +    qdev_init_gpio_in_named(dev, via_isa_request_i8259_irq, "i8259", 1);
> +    isa_irq = qdev_get_gpio_in_named(dev, "i8259", 0);
>       isa_bus = isa_bus_new(dev, pci_address_space(d), pci_address_space_io(d),
>                             errp);
>   
> @@ -729,7 +730,7 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
>           return;
>       }
>   
> -    s->isa_irqs_in = i8259_init(isa_bus, *isa_irq);
> +    s->isa_irqs_in = i8259_init(isa_bus, isa_irq);
>       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);

Have you confirmed that the machines using the VIA can still boot correctly with this 
change? I have a vague memory that Phil tried something like this, but due to legacy 
code poking around directly in the ISA IRQ array after realize it caused some things 
to break.


ATB,

Mark.

Re: [PATCH v2 04/15] hw/isa/vt82c686: Define a GPIO line between vt82c686 and i8259

[Akihiko Odaki]

On 2024/06/28 16:27, Mark Cave-Ayland wrote:
> On 27/06/2024 14:37, Akihiko Odaki wrote:
> 
>> This fixes qemu_irq array leak.
>>
>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>> ---
>>   hw/isa/vt82c686.c | 7 ++++---
>>   1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>> index 8582ac0322eb..629d2d568137 100644
>> --- a/hw/isa/vt82c686.c
>> +++ b/hw/isa/vt82c686.c
>> @@ -715,13 +715,14 @@ static void via_isa_realize(PCIDevice *d, Error 
>> **errp)
>>       ViaISAState *s = VIA_ISA(d);
>>       DeviceState *dev = DEVICE(d);
>>       PCIBus *pci_bus = pci_get_bus(d);
>> -    qemu_irq *isa_irq;
>> +    qemu_irq isa_irq;
>>       ISABus *isa_bus;
>>       int i;
>>       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);
>> +    qdev_init_gpio_in_named(dev, via_isa_request_i8259_irq, "i8259", 1);
>> +    isa_irq = qdev_get_gpio_in_named(dev, "i8259", 0);
>>       isa_bus = isa_bus_new(dev, pci_address_space(d), 
>> pci_address_space_io(d),
>>                             errp);
>> @@ -729,7 +730,7 @@ static void via_isa_realize(PCIDevice *d, Error 
>> **errp)
>>           return;
>>       }
>> -    s->isa_irqs_in = i8259_init(isa_bus, *isa_irq);
>> +    s->isa_irqs_in = i8259_init(isa_bus, isa_irq);
>>       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);
> 
> Have you confirmed that the machines using the VIA can still boot 
> correctly with this change? I have a vague memory that Phil tried 
> something like this, but due to legacy code poking around directly in 
> the ISA IRQ array after realize it caused some things to break.

I tried to boot MorphOS on pegasos2 but it didn't boot even with QEMU 
9.0.1. The command line I used is:

qemu-system-ppc -M pegasos2 -rtc base=localtime -device 
ati-vga,guest_hwcursor=true,romfile= -cdrom morphos-3.18.iso -kernel 
boot.img -serial stdio

That said, I believe no such code remain now. The IRQ array is held in a 
variable local in this function and nobody else can refer to it.

Regards,
Akihiko Odaki

Re: [PATCH v2 04/15] hw/isa/vt82c686: Define a GPIO line between vt82c686 and i8259

[Akihiko Odaki]

On 2024/06/29 16:38, Akihiko Odaki wrote:
> On 2024/06/28 16:27, Mark Cave-Ayland wrote:
>> On 27/06/2024 14:37, Akihiko Odaki wrote:
>>
>>> This fixes qemu_irq array leak.
>>>
>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>>> ---
>>>   hw/isa/vt82c686.c | 7 ++++---
>>>   1 file changed, 4 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>>> index 8582ac0322eb..629d2d568137 100644
>>> --- a/hw/isa/vt82c686.c
>>> +++ b/hw/isa/vt82c686.c
>>> @@ -715,13 +715,14 @@ static void via_isa_realize(PCIDevice *d, Error 
>>> **errp)
>>>       ViaISAState *s = VIA_ISA(d);
>>>       DeviceState *dev = DEVICE(d);
>>>       PCIBus *pci_bus = pci_get_bus(d);
>>> -    qemu_irq *isa_irq;
>>> +    qemu_irq isa_irq;
>>>       ISABus *isa_bus;
>>>       int i;
>>>       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);
>>> +    qdev_init_gpio_in_named(dev, via_isa_request_i8259_irq, "i8259", 
>>> 1);
>>> +    isa_irq = qdev_get_gpio_in_named(dev, "i8259", 0);
>>>       isa_bus = isa_bus_new(dev, pci_address_space(d), 
>>> pci_address_space_io(d),
>>>                             errp);
>>> @@ -729,7 +730,7 @@ static void via_isa_realize(PCIDevice *d, Error 
>>> **errp)
>>>           return;
>>>       }
>>> -    s->isa_irqs_in = i8259_init(isa_bus, *isa_irq);
>>> +    s->isa_irqs_in = i8259_init(isa_bus, isa_irq);
>>>       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);
>>
>> Have you confirmed that the machines using the VIA can still boot 
>> correctly with this change? I have a vague memory that Phil tried 
>> something like this, but due to legacy code poking around directly in 
>> the ISA IRQ array after realize it caused some things to break.
> 
> I tried to boot MorphOS on pegasos2 but it didn't boot even with QEMU 
> 9.0.1. The command line I used is:
> 
> qemu-system-ppc -M pegasos2 -rtc base=localtime -device 
> ati-vga,guest_hwcursor=true,romfile= -cdrom morphos-3.18.iso -kernel 
> boot.img -serial stdio

Apparently I failed to run it properly. I tried again now and it booted 
with this change.

Regards,
Akihiko Odaki

> 
> That said, I believe no such code remain now. The IRQ array is held in a 
> variable local in this function and nobody else can refer to it.

Re: [PATCH v2 04/15] hw/isa/vt82c686: Define a GPIO line between vt82c686 and i8259

[BALATON Zoltan]

On Thu, 27 Jun 2024, Akihiko Odaki wrote:
> This fixes qemu_irq array leak.
>
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
> hw/isa/vt82c686.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
> index 8582ac0322eb..629d2d568137 100644
> --- a/hw/isa/vt82c686.c
> +++ b/hw/isa/vt82c686.c
> @@ -715,13 +715,14 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
>     ViaISAState *s = VIA_ISA(d);
>     DeviceState *dev = DEVICE(d);
>     PCIBus *pci_bus = pci_get_bus(d);
> -    qemu_irq *isa_irq;
> +    qemu_irq isa_irq;
>     ISABus *isa_bus;
>     int i;
>
>     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);
> +    qdev_init_gpio_in_named(dev, via_isa_request_i8259_irq, "i8259", 1);

The chip has no such pin so this is a QEMU internal connection that I 
think should not be modelled with a named gpio. I think we really need a 
function to init a qemu_irq (for now we only have one that also allocares 
it) so then we can put this in ViaISAState and init in place. I'll make a 
patch.

Regards,
BALATON Zoltan

> +    isa_irq = qdev_get_gpio_in_named(dev, "i8259", 0);
>     isa_bus = isa_bus_new(dev, pci_address_space(d), pci_address_space_io(d),
>                           errp);
>
> @@ -729,7 +730,7 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
>         return;
>     }
>
> -    s->isa_irqs_in = i8259_init(isa_bus, *isa_irq);
> +    s->isa_irqs_in = i8259_init(isa_bus, isa_irq);
>     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);
>
>

Re: [PATCH v2 04/15] hw/isa/vt82c686: Define a GPIO line between vt82c686 and i8259

[Akihiko Odaki]

On 2024/06/29 22:08, BALATON Zoltan wrote:
> On Thu, 27 Jun 2024, Akihiko Odaki wrote:
>> This fixes qemu_irq array leak.
>>
>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>> ---
>> hw/isa/vt82c686.c | 7 ++++---
>> 1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>> index 8582ac0322eb..629d2d568137 100644
>> --- a/hw/isa/vt82c686.c
>> +++ b/hw/isa/vt82c686.c
>> @@ -715,13 +715,14 @@ static void via_isa_realize(PCIDevice *d, Error 
>> **errp)
>>     ViaISAState *s = VIA_ISA(d);
>>     DeviceState *dev = DEVICE(d);
>>     PCIBus *pci_bus = pci_get_bus(d);
>> -    qemu_irq *isa_irq;
>> +    qemu_irq isa_irq;
>>     ISABus *isa_bus;
>>     int i;
>>
>>     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);
>> +    qdev_init_gpio_in_named(dev, via_isa_request_i8259_irq, "i8259", 1);
> 
> The chip has no such pin so this is a QEMU internal connection that I 
> think should not be modelled with a named gpio. I think we really need a 
> function to init a qemu_irq (for now we only have one that also 
> allocares it) so then we can put this in ViaISAState and init in place. 
> I'll make a patch.

According to qdev_pass_gpios(), it is valid to have a GPIO line even if 
a QOM device to be connected with the GPIO line is actually included in 
one chip package. I didn't use qdev_pass_gpios() because it cannot 
expose a subset of the GPIO array.

Regards,
Akihiko Odaki

[PATCH v2 05/15] spapr: Free stdout path

[Akihiko Odaki]

This fixes LeakSanitizer warnings.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 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 v2 06/15] 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

Re: [PATCH v2 06/15] ppc/vof: Fix unaligned FDT property access

[Peter Maydell]

On Thu, 27 Jun 2024 at 14:39, Akihiko Odaki <akihiko.odaki@daynix.com> 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));
>      }

I did wonder if there was a better way to do what this is doing,
but neither we (in system/device_tree.c) nor libfdt seem to
provide one.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH v2 06/15] ppc/vof: Fix unaligned FDT property access

[David Gibson]

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

On Fri, Jun 28, 2024 at 04:20:02PM +0100, Peter Maydell wrote:
> On Thu, 27 Jun 2024 at 14:39, Akihiko Odaki <akihiko.odaki@daynix.com> 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));
> >      }
> 
> I did wonder if there was a better way to do what this is doing,
> but neither we (in system/device_tree.c) nor libfdt seem to
> provide one.

libfdt does provide unaligned access helpers (fdt32_ld() etc.), but
not an automatic aligned-or-unaligned helper.   Maybe we should add that?

-- 
David Gibson (he or they)	| 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 --]

Re: [PATCH v2 06/15] ppc/vof: Fix unaligned FDT property access

[Nicholas Piggin]

On Sat Jun 29, 2024 at 1:16 PM AEST, David Gibson wrote:
> On Fri, Jun 28, 2024 at 04:20:02PM +0100, Peter Maydell wrote:
> > On Thu, 27 Jun 2024 at 14:39, Akihiko Odaki <akihiko.odaki@daynix.com> 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));
> > >      }
> > 
> > I did wonder if there was a better way to do what this is doing,
> > but neither we (in system/device_tree.c) nor libfdt seem to
> > provide one.
>
> libfdt does provide unaligned access helpers (fdt32_ld() etc.), but
> not an automatic aligned-or-unaligned helper.   Maybe we should add that?

Runtime test if the pointer is aligned?

What about just fdt_prop32_ld() and fdt_prop64_ld() where you know it's
4 byte aligned. Then just do 2 x 4 byte loads for the 64-bit, I don't
think performance would matter so much to try get a single load.

Thanks,
Nick

Re: [PATCH v2 06/15] ppc/vof: Fix unaligned FDT property access

[Peter Maydell]

On Sat, 29 Jun 2024 at 04:17, David Gibson <david@gibson.dropbear.id.au> wrote:
>
> On Fri, Jun 28, 2024 at 04:20:02PM +0100, Peter Maydell wrote:
> > On Thu, 27 Jun 2024 at 14:39, Akihiko Odaki <akihiko.odaki@daynix.com> 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));
> > >      }
> >
> > I did wonder if there was a better way to do what this is doing,
> > but neither we (in system/device_tree.c) nor libfdt seem to
> > provide one.
>
> libfdt does provide unaligned access helpers (fdt32_ld() etc.), but
> not an automatic aligned-or-unaligned helper.   Maybe we should add that?

fdt32_ld() and friends only do the "load from this bit of memory"
part, which we already have QEMU utility functions for (and which
are this patch uses).

This particular bit of code is dealing with an fdt property ("memory")
that is an array of (address, size) tuples where address and size
can independently be either 32 or 64 bits, and it wants the
size value of tuple 0. So the missing functionality is something at
a higher level than fdt32_ld() which would let you say "give me
tuple N field X" with some way to specify the tuple layout. (Which
is an awkward kind of API to write in C.)

Slightly less general, but for this case we could perhaps have
something like the getprop equivalent of qemu_fdt_setprop_sized_cells():

  uint64_t value_array[2];
  qemu_fdt_getprop_sized_cells(fdt, nodename, "memory", &value_array,
                               ac, sc);
  /*
   * fills in value_array[0] with address, value_array[1] with size,
   * probably barfs if the varargs-list of cell-sizes doesn't
   * cover the whole property, similar to the current assert on
   * proplen.
   */
  mem0_end = value_array[0];

thanks
-- PMM

Re: [PATCH v2 06/15] ppc/vof: Fix unaligned FDT property access

[Nicholas Piggin]

On Thu Jul 4, 2024 at 10:15 PM AEST, Peter Maydell wrote:
> On Sat, 29 Jun 2024 at 04:17, David Gibson <david@gibson.dropbear.id.au> wrote:
> >
> > On Fri, Jun 28, 2024 at 04:20:02PM +0100, Peter Maydell wrote:
> > > On Thu, 27 Jun 2024 at 14:39, Akihiko Odaki <akihiko.odaki@daynix.com> 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));
> > > >      }
> > >
> > > I did wonder if there was a better way to do what this is doing,
> > > but neither we (in system/device_tree.c) nor libfdt seem to
> > > provide one.
> >
> > libfdt does provide unaligned access helpers (fdt32_ld() etc.), but
> > not an automatic aligned-or-unaligned helper.   Maybe we should add that?
>
> fdt32_ld() and friends only do the "load from this bit of memory"
> part, which we already have QEMU utility functions for (and which
> are this patch uses).
>
> This particular bit of code is dealing with an fdt property ("memory")
> that is an array of (address, size) tuples where address and size
> can independently be either 32 or 64 bits, and it wants the
> size value of tuple 0. So the missing functionality is something at
> a higher level than fdt32_ld() which would let you say "give me
> tuple N field X" with some way to specify the tuple layout. (Which
> is an awkward kind of API to write in C.)
>
> Slightly less general, but for this case we could perhaps have
> something like the getprop equivalent of qemu_fdt_setprop_sized_cells():
>
>   uint64_t value_array[2];
>   qemu_fdt_getprop_sized_cells(fdt, nodename, "memory", &value_array,
>                                ac, sc);
>   /*
>    * fills in value_array[0] with address, value_array[1] with size,
>    * probably barfs if the varargs-list of cell-sizes doesn't
>    * cover the whole property, similar to the current assert on
>    * proplen.
>    */
>   mem0_end = value_array[0];

Since 4/8 byte cells are most common and size is probably
normally known, what about something simpler to start with?

Thanks,
Nick

---
diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
index 0677fea..c4b6355 100644
--- a/libfdt/libfdt.h
+++ b/libfdt/libfdt.h
@@ -148,6 +148,15 @@ static inline uint32_t fdt32_ld(const fdt32_t *p)
 		| bp[3];
 }
 
+/*
+ * Load the value from a 32-bit cell of a property. Cells are 32-bit aligned
+ * so can use a single load.
+ */
+static inline uint32_t fdt32_ld_prop(const fdt32_t *p)
+{
+	return fdt32_to_cpu(*p);
+}
+
 static inline void fdt32_st(void *property, uint32_t value)
 {
 	uint8_t *bp = (uint8_t *)property;
@@ -172,6 +181,18 @@ static inline uint64_t fdt64_ld(const fdt64_t *p)
 		| bp[7];
 }
 
+/*
+ * Load the value from a 64-bit cell of a property. Cells are 32-bit aligned
+ * so can use two loads.
+ */
+static inline uint64_t fdt64_ld_prop(const fdt64_t *p)
+{
+	const fdt64_t *_p = p;
+
+	return ((uint64_t)fdt32_to_cpu(_p[0]) << 32)
+		| fdt32_to_cpu(_p[1]);
+}
+
 static inline void fdt64_st(void *property, uint64_t value)
 {
 	uint8_t *bp = (uint8_t *)property;

Re: [PATCH v2 06/15] ppc/vof: Fix unaligned FDT property access

[David Gibson]

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

On Fri, Jul 05, 2024 at 11:18:47AM +1000, Nicholas Piggin wrote:
> On Thu Jul 4, 2024 at 10:15 PM AEST, Peter Maydell wrote:
> > On Sat, 29 Jun 2024 at 04:17, David Gibson <david@gibson.dropbear.id.au> wrote:
> > >
> > > On Fri, Jun 28, 2024 at 04:20:02PM +0100, Peter Maydell wrote:
> > > > On Thu, 27 Jun 2024 at 14:39, Akihiko Odaki <akihiko.odaki@daynix.com> 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));
> > > > >      }
> > > >
> > > > I did wonder if there was a better way to do what this is doing,
> > > > but neither we (in system/device_tree.c) nor libfdt seem to
> > > > provide one.
> > >
> > > libfdt does provide unaligned access helpers (fdt32_ld() etc.), but
> > > not an automatic aligned-or-unaligned helper.   Maybe we should add that?
> >
> > fdt32_ld() and friends only do the "load from this bit of memory"
> > part, which we already have QEMU utility functions for (and which
> > are this patch uses).
> >
> > This particular bit of code is dealing with an fdt property ("memory")
> > that is an array of (address, size) tuples where address and size
> > can independently be either 32 or 64 bits, and it wants the
> > size value of tuple 0. So the missing functionality is something at
> > a higher level than fdt32_ld() which would let you say "give me
> > tuple N field X" with some way to specify the tuple layout. (Which
> > is an awkward kind of API to write in C.)
> >
> > Slightly less general, but for this case we could perhaps have
> > something like the getprop equivalent of qemu_fdt_setprop_sized_cells():
> >
> >   uint64_t value_array[2];
> >   qemu_fdt_getprop_sized_cells(fdt, nodename, "memory", &value_array,
> >                                ac, sc);
> >   /*
> >    * fills in value_array[0] with address, value_array[1] with size,
> >    * probably barfs if the varargs-list of cell-sizes doesn't
> >    * cover the whole property, similar to the current assert on
> >    * proplen.
> >    */
> >   mem0_end = value_array[0];
> 
> Since 4/8 byte cells are most common and size is probably
> normally known, what about something simpler to start with?

Hrm, I don't think this helps much.  As Peter points out the actual
load isn't really the issue, it's locating the right spot for it.

> 
> Thanks,
> Nick
> 
> ---
> diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
> index 0677fea..c4b6355 100644
> --- a/libfdt/libfdt.h
> +++ b/libfdt/libfdt.h
> @@ -148,6 +148,15 @@ static inline uint32_t fdt32_ld(const fdt32_t *p)
>  		| bp[3];
>  }
>  
> +/*
> + * Load the value from a 32-bit cell of a property. Cells are 32-bit aligned
> + * so can use a single load.
> + */
> +static inline uint32_t fdt32_ld_prop(const fdt32_t *p)
> +{
> +	return fdt32_to_cpu(*p);
> +}
> +
>  static inline void fdt32_st(void *property, uint32_t value)
>  {
>  	uint8_t *bp = (uint8_t *)property;
> @@ -172,6 +181,18 @@ static inline uint64_t fdt64_ld(const fdt64_t *p)
>  		| bp[7];
>  }
>  
> +/*
> + * Load the value from a 64-bit cell of a property. Cells are 32-bit aligned
> + * so can use two loads.
> + */
> +static inline uint64_t fdt64_ld_prop(const fdt64_t *p)
> +{
> +	const fdt64_t *_p = p;
> +
> +	return ((uint64_t)fdt32_to_cpu(_p[0]) << 32)
> +		| fdt32_to_cpu(_p[1]);
> +}
> +
>  static inline void fdt64_st(void *property, uint64_t value)
>  {
>  	uint8_t *bp = (uint8_t *)property;
> 

-- 
David Gibson (he or they)	| 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 --]

Re: [PATCH v2 06/15] ppc/vof: Fix unaligned FDT property access

[Nicholas Piggin]

On Fri Jul 5, 2024 at 11:41 AM AEST, David Gibson wrote:
> On Fri, Jul 05, 2024 at 11:18:47AM +1000, Nicholas Piggin wrote:
> > On Thu Jul 4, 2024 at 10:15 PM AEST, Peter Maydell wrote:
> > > On Sat, 29 Jun 2024 at 04:17, David Gibson <david@gibson.dropbear.id.au> wrote:
> > > >
> > > > On Fri, Jun 28, 2024 at 04:20:02PM +0100, Peter Maydell wrote:
> > > > > On Thu, 27 Jun 2024 at 14:39, Akihiko Odaki <akihiko.odaki@daynix.com> 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));
> > > > > >      }
> > > > >
> > > > > I did wonder if there was a better way to do what this is doing,
> > > > > but neither we (in system/device_tree.c) nor libfdt seem to
> > > > > provide one.
> > > >
> > > > libfdt does provide unaligned access helpers (fdt32_ld() etc.), but
> > > > not an automatic aligned-or-unaligned helper.   Maybe we should add that?
> > >
> > > fdt32_ld() and friends only do the "load from this bit of memory"
> > > part, which we already have QEMU utility functions for (and which
> > > are this patch uses).
> > >
> > > This particular bit of code is dealing with an fdt property ("memory")
> > > that is an array of (address, size) tuples where address and size
> > > can independently be either 32 or 64 bits, and it wants the
> > > size value of tuple 0. So the missing functionality is something at
> > > a higher level than fdt32_ld() which would let you say "give me
> > > tuple N field X" with some way to specify the tuple layout. (Which
> > > is an awkward kind of API to write in C.)
> > >
> > > Slightly less general, but for this case we could perhaps have
> > > something like the getprop equivalent of qemu_fdt_setprop_sized_cells():
> > >
> > >   uint64_t value_array[2];
> > >   qemu_fdt_getprop_sized_cells(fdt, nodename, "memory", &value_array,
> > >                                ac, sc);
> > >   /*
> > >    * fills in value_array[0] with address, value_array[1] with size,
> > >    * probably barfs if the varargs-list of cell-sizes doesn't
> > >    * cover the whole property, similar to the current assert on
> > >    * proplen.
> > >    */
> > >   mem0_end = value_array[0];
> > 
> > Since 4/8 byte cells are most common and size is probably
> > normally known, what about something simpler to start with?
>
> Hrm, I don't think this helps much.  As Peter points out the actual
> load isn't really the issue, it's locating the right spot for it.

I don't really see why that's a problem, it's just a pointer
addition - base + fdt_address_cells * 4. The problem was in
the memory access (yes it's fixed with the patch but you could
add a general libfdt way to do it).

Some fancy function like above could be used, But is it really
worth implementing such a thing for this?

Thanks,
Nick

Re: [PATCH v2 06/15] ppc/vof: Fix unaligned FDT property access

[David Gibson]

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

On Fri, Jul 05, 2024 at 02:40:19PM +1000, Nicholas Piggin wrote:
> On Fri Jul 5, 2024 at 11:41 AM AEST, David Gibson wrote:
> > On Fri, Jul 05, 2024 at 11:18:47AM +1000, Nicholas Piggin wrote:
> > > On Thu Jul 4, 2024 at 10:15 PM AEST, Peter Maydell wrote:
> > > > On Sat, 29 Jun 2024 at 04:17, David Gibson <david@gibson.dropbear.id.au> wrote:
> > > > >
> > > > > On Fri, Jun 28, 2024 at 04:20:02PM +0100, Peter Maydell wrote:
> > > > > > On Thu, 27 Jun 2024 at 14:39, Akihiko Odaki <akihiko.odaki@daynix.com> 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));
> > > > > > >      }
> > > > > >
> > > > > > I did wonder if there was a better way to do what this is doing,
> > > > > > but neither we (in system/device_tree.c) nor libfdt seem to
> > > > > > provide one.
> > > > >
> > > > > libfdt does provide unaligned access helpers (fdt32_ld() etc.), but
> > > > > not an automatic aligned-or-unaligned helper.   Maybe we should add that?
> > > >
> > > > fdt32_ld() and friends only do the "load from this bit of memory"
> > > > part, which we already have QEMU utility functions for (and which
> > > > are this patch uses).
> > > >
> > > > This particular bit of code is dealing with an fdt property ("memory")
> > > > that is an array of (address, size) tuples where address and size
> > > > can independently be either 32 or 64 bits, and it wants the
> > > > size value of tuple 0. So the missing functionality is something at
> > > > a higher level than fdt32_ld() which would let you say "give me
> > > > tuple N field X" with some way to specify the tuple layout. (Which
> > > > is an awkward kind of API to write in C.)
> > > >
> > > > Slightly less general, but for this case we could perhaps have
> > > > something like the getprop equivalent of qemu_fdt_setprop_sized_cells():
> > > >
> > > >   uint64_t value_array[2];
> > > >   qemu_fdt_getprop_sized_cells(fdt, nodename, "memory", &value_array,
> > > >                                ac, sc);
> > > >   /*
> > > >    * fills in value_array[0] with address, value_array[1] with size,
> > > >    * probably barfs if the varargs-list of cell-sizes doesn't
> > > >    * cover the whole property, similar to the current assert on
> > > >    * proplen.
> > > >    */
> > > >   mem0_end = value_array[0];
> > > 
> > > Since 4/8 byte cells are most common and size is probably
> > > normally known, what about something simpler to start with?
> >
> > Hrm, I don't think this helps much.  As Peter points out the actual
> > load isn't really the issue, it's locating the right spot for it.
> 
> I don't really see why that's a problem, it's just a pointer
> addition - base + fdt_address_cells * 4. The problem was in

This is harder if #address-cells and #size-cells are different, or if
you're parsing ranges and #address-cells is different between parent
and child node.

> the memory access (yes it's fixed with the patch but you could
> add a general libfdt way to do it).

Huh.. well I'm getting different impressions of what the problem
actually is from what I initially read versus Peter Maydell's
comments, so I don't really know what to think.

If it's just the load then fdt32_ld() etc. already exist.  Or is it
really such a hot path that unconditionally handling unaligned
accesses isn't tenable?

> Some fancy function like above could be used, But is it really
> worth implementing such a thing for this?
> 
> Thanks,
> Nick
> 

-- 
David Gibson (he or they)	| 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 --]

Re: [PATCH v2 06/15] ppc/vof: Fix unaligned FDT property access

[Nicholas Piggin]

On Fri Jul 5, 2024 at 3:12 PM AEST, David Gibson wrote:
> On Fri, Jul 05, 2024 at 02:40:19PM +1000, Nicholas Piggin wrote:
> > On Fri Jul 5, 2024 at 11:41 AM AEST, David Gibson wrote:
> > > On Fri, Jul 05, 2024 at 11:18:47AM +1000, Nicholas Piggin wrote:
> > > > On Thu Jul 4, 2024 at 10:15 PM AEST, Peter Maydell wrote:
> > > > > On Sat, 29 Jun 2024 at 04:17, David Gibson <david@gibson.dropbear.id.au> wrote:
> > > > > >
> > > > > > On Fri, Jun 28, 2024 at 04:20:02PM +0100, Peter Maydell wrote:
> > > > > > > On Thu, 27 Jun 2024 at 14:39, Akihiko Odaki <akihiko.odaki@daynix.com> 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));
> > > > > > > >      }
> > > > > > >
> > > > > > > I did wonder if there was a better way to do what this is doing,
> > > > > > > but neither we (in system/device_tree.c) nor libfdt seem to
> > > > > > > provide one.
> > > > > >
> > > > > > libfdt does provide unaligned access helpers (fdt32_ld() etc.), but
> > > > > > not an automatic aligned-or-unaligned helper.   Maybe we should add that?
> > > > >
> > > > > fdt32_ld() and friends only do the "load from this bit of memory"
> > > > > part, which we already have QEMU utility functions for (and which
> > > > > are this patch uses).
> > > > >
> > > > > This particular bit of code is dealing with an fdt property ("memory")
> > > > > that is an array of (address, size) tuples where address and size
> > > > > can independently be either 32 or 64 bits, and it wants the
> > > > > size value of tuple 0. So the missing functionality is something at
> > > > > a higher level than fdt32_ld() which would let you say "give me
> > > > > tuple N field X" with some way to specify the tuple layout. (Which
> > > > > is an awkward kind of API to write in C.)
> > > > >
> > > > > Slightly less general, but for this case we could perhaps have
> > > > > something like the getprop equivalent of qemu_fdt_setprop_sized_cells():
> > > > >
> > > > >   uint64_t value_array[2];
> > > > >   qemu_fdt_getprop_sized_cells(fdt, nodename, "memory", &value_array,
> > > > >                                ac, sc);
> > > > >   /*
> > > > >    * fills in value_array[0] with address, value_array[1] with size,
> > > > >    * probably barfs if the varargs-list of cell-sizes doesn't
> > > > >    * cover the whole property, similar to the current assert on
> > > > >    * proplen.
> > > > >    */
> > > > >   mem0_end = value_array[0];
> > > > 
> > > > Since 4/8 byte cells are most common and size is probably
> > > > normally known, what about something simpler to start with?
> > >
> > > Hrm, I don't think this helps much.  As Peter points out the actual
> > > load isn't really the issue, it's locating the right spot for it.
> > 
> > I don't really see why that's a problem, it's just a pointer
> > addition - base + fdt_address_cells * 4. The problem was in
>
> This is harder if #address-cells and #size-cells are different, or if
> you're parsing ranges and #address-cells is different between parent
> and child node.
>
> > the memory access (yes it's fixed with the patch but you could
> > add a general libfdt way to do it).
>
> Huh.. well I'm getting different impressions of what the problem
> actually is from what I initially read versus Peter Maydell's
> comments, so I don't really know what to think.

If I'm not mistaken, the sanitizer caught an unaligned 64-bit
load which is the bug.

The tuple address calculation itself I think is not buggy. I suppose
Peter was thinking of an accessor that takes care of addressing and
alignment. I don't think we're at the point it warrants it here, but
could be convinced (maybe a bunch of other code would use it).

I think the API is a little dangerous for overflows though, hard to
static check. sscanf() style could be checked by the compiler but
seems overkill to implement.

> If it's just the load then fdt32_ld() etc. already exist.  Or is it
> really such a hot path that unconditionally handling unaligned
> accesses isn't tenable?

Yeah that's true, hardly any point to adding the faster variant.

It could just be fixed like this then? The original patch is a
fix too, but I do prefer using the same style for both, and
I think using the fdt accessor is nicer to read.

Thanks,
Nick

---

diff --git a/hw/ppc/vof.c b/hw/ppc/vof.c
index e3b430a81f..a666a133d7 100644
--- a/hw/ppc/vof.c
+++ b/hw/ppc/vof.c
@@ -646,9 +646,9 @@ 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 = fdt64_ld((fdt64_t *)(mem0_reg + sizeof(uint32_t) * ac));
     } else {
-        mem0_end = be32_to_cpu(*(uint32_t *)(mem0_reg + sizeof(uint32_t) * ac));
+        mem0_end = fdt32_ld((fdt32_t *)(mem0_reg + sizeof(uint32_t) * ac));
     }

     g_array_sort(claimed, of_claimed_compare_func);

Re: [PATCH v2 06/15] ppc/vof: Fix unaligned FDT property access

[Akihiko Odaki]

On 2024/07/05 16:50, Nicholas Piggin wrote:
> On Fri Jul 5, 2024 at 3:12 PM AEST, David Gibson wrote:
>> On Fri, Jul 05, 2024 at 02:40:19PM +1000, Nicholas Piggin wrote:
>>> On Fri Jul 5, 2024 at 11:41 AM AEST, David Gibson wrote:
>>>> On Fri, Jul 05, 2024 at 11:18:47AM +1000, Nicholas Piggin wrote:
>>>>> On Thu Jul 4, 2024 at 10:15 PM AEST, Peter Maydell wrote:
>>>>>> On Sat, 29 Jun 2024 at 04:17, David Gibson <david@gibson.dropbear.id.au> wrote:
>>>>>>>
>>>>>>> On Fri, Jun 28, 2024 at 04:20:02PM +0100, Peter Maydell wrote:
>>>>>>>> On Thu, 27 Jun 2024 at 14:39, Akihiko Odaki <akihiko.odaki@daynix.com> 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));
>>>>>>>>>       }
>>>>>>>>
>>>>>>>> I did wonder if there was a better way to do what this is doing,
>>>>>>>> but neither we (in system/device_tree.c) nor libfdt seem to
>>>>>>>> provide one.
>>>>>>>
>>>>>>> libfdt does provide unaligned access helpers (fdt32_ld() etc.), but
>>>>>>> not an automatic aligned-or-unaligned helper.   Maybe we should add that?
>>>>>>
>>>>>> fdt32_ld() and friends only do the "load from this bit of memory"
>>>>>> part, which we already have QEMU utility functions for (and which
>>>>>> are this patch uses).
>>>>>>
>>>>>> This particular bit of code is dealing with an fdt property ("memory")
>>>>>> that is an array of (address, size) tuples where address and size
>>>>>> can independently be either 32 or 64 bits, and it wants the
>>>>>> size value of tuple 0. So the missing functionality is something at
>>>>>> a higher level than fdt32_ld() which would let you say "give me
>>>>>> tuple N field X" with some way to specify the tuple layout. (Which
>>>>>> is an awkward kind of API to write in C.)
>>>>>>
>>>>>> Slightly less general, but for this case we could perhaps have
>>>>>> something like the getprop equivalent of qemu_fdt_setprop_sized_cells():
>>>>>>
>>>>>>    uint64_t value_array[2];
>>>>>>    qemu_fdt_getprop_sized_cells(fdt, nodename, "memory", &value_array,
>>>>>>                                 ac, sc);
>>>>>>    /*
>>>>>>     * fills in value_array[0] with address, value_array[1] with size,
>>>>>>     * probably barfs if the varargs-list of cell-sizes doesn't
>>>>>>     * cover the whole property, similar to the current assert on
>>>>>>     * proplen.
>>>>>>     */
>>>>>>    mem0_end = value_array[0];
>>>>>
>>>>> Since 4/8 byte cells are most common and size is probably
>>>>> normally known, what about something simpler to start with?
>>>>
>>>> Hrm, I don't think this helps much.  As Peter points out the actual
>>>> load isn't really the issue, it's locating the right spot for it.
>>>
>>> I don't really see why that's a problem, it's just a pointer
>>> addition - base + fdt_address_cells * 4. The problem was in
>>
>> This is harder if #address-cells and #size-cells are different, or if
>> you're parsing ranges and #address-cells is different between parent
>> and child node.
>>
>>> the memory access (yes it's fixed with the patch but you could
>>> add a general libfdt way to do it).
>>
>> Huh.. well I'm getting different impressions of what the problem
>> actually is from what I initially read versus Peter Maydell's
>> comments, so I don't really know what to think.
> 
> If I'm not mistaken, the sanitizer caught an unaligned 64-bit
> load which is the bug.
> 
> The tuple address calculation itself I think is not buggy. I suppose
> Peter was thinking of an accessor that takes care of addressing and
> alignment. I don't think we're at the point it warrants it here, but
> could be convinced (maybe a bunch of other code would use it).
> 
> I think the API is a little dangerous for overflows though, hard to
> static check. sscanf() style could be checked by the compiler but
> seems overkill to implement.
> 
>> If it's just the load then fdt32_ld() etc. already exist.  Or is it
>> really such a hot path that unconditionally handling unaligned
>> accesses isn't tenable?
> 
> Yeah that's true, hardly any point to adding the faster variant.
> 
> It could just be fixed like this then? The original patch is a
> fix too, but I do prefer using the same style for both, and
> I think using the fdt accessor is nicer to read.
> 
> Thanks,
> Nick
> 
> ---
> 
> diff --git a/hw/ppc/vof.c b/hw/ppc/vof.c
> index e3b430a81f..a666a133d7 100644
> --- a/hw/ppc/vof.c
> +++ b/hw/ppc/vof.c
> @@ -646,9 +646,9 @@ 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 = fdt64_ld((fdt64_t *)(mem0_reg + sizeof(uint32_t) * ac));

I don't like the extra cast to fdt64_t. Strictly speaking, casting into 
uint64_t * is undefined in the standard and the compiler is free to 
optimize it as an aligned access after the cast. clang and gcc does not 
perform such an optimization, but it is better to avoid such a construct 
if possible.* It is unfortunate that libfdt requires it.

Nevertheless, I won't object to use fdt64_ld() and fdt32_ld(). That's 
what the upstream provides anyway.

Regards,
Akihiko Odaki

* By the way, I had a related discussion with sanitizer developers:
   https://github.com/llvm/llvm-project/issues/83710

Re: [PATCH v2 06/15] ppc/vof: Fix unaligned FDT property access

[Peter Maydell]

On Fri, 5 Jul 2024 at 06:13, David Gibson <david@gibson.dropbear.id.au> wrote:
>
> On Fri, Jul 05, 2024 at 02:40:19PM +1000, Nicholas Piggin wrote:
> > On Fri Jul 5, 2024 at 11:41 AM AEST, David Gibson wrote:
> > > On Fri, Jul 05, 2024 at 11:18:47AM +1000, Nicholas Piggin wrote:
> > > > On Thu Jul 4, 2024 at 10:15 PM AEST, Peter Maydell wrote:
> > > > > On Sat, 29 Jun 2024 at 04:17, David Gibson <david@gibson.dropbear.id.au> wrote:
> > > > > >
> > > > > > On Fri, Jun 28, 2024 at 04:20:02PM +0100, Peter Maydell wrote:
> > > > > > > On Thu, 27 Jun 2024 at 14:39, Akihiko Odaki <akihiko.odaki@daynix.com> 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));
> > > > > > > >      }
> > > > > > >
> > > > > > > I did wonder if there was a better way to do what this is doing,
> > > > > > > but neither we (in system/device_tree.c) nor libfdt seem to
> > > > > > > provide one.
> > > > > >
> > > > > > libfdt does provide unaligned access helpers (fdt32_ld() etc.), but
> > > > > > not an automatic aligned-or-unaligned helper.   Maybe we should add that?
> > > > >
> > > > > fdt32_ld() and friends only do the "load from this bit of memory"
> > > > > part, which we already have QEMU utility functions for (and which
> > > > > are this patch uses).
> > > > >
> > > > > This particular bit of code is dealing with an fdt property ("memory")
> > > > > that is an array of (address, size) tuples where address and size
> > > > > can independently be either 32 or 64 bits, and it wants the
> > > > > size value of tuple 0. So the missing functionality is something at
> > > > > a higher level than fdt32_ld() which would let you say "give me
> > > > > tuple N field X" with some way to specify the tuple layout. (Which
> > > > > is an awkward kind of API to write in C.)
> > > > >
> > > > > Slightly less general, but for this case we could perhaps have
> > > > > something like the getprop equivalent of qemu_fdt_setprop_sized_cells():
> > > > >
> > > > >   uint64_t value_array[2];
> > > > >   qemu_fdt_getprop_sized_cells(fdt, nodename, "memory", &value_array,
> > > > >                                ac, sc);
> > > > >   /*
> > > > >    * fills in value_array[0] with address, value_array[1] with size,
> > > > >    * probably barfs if the varargs-list of cell-sizes doesn't
> > > > >    * cover the whole property, similar to the current assert on
> > > > >    * proplen.
> > > > >    */
> > > > >   mem0_end = value_array[0];
> > > >
> > > > Since 4/8 byte cells are most common and size is probably
> > > > normally known, what about something simpler to start with?
> > >
> > > Hrm, I don't think this helps much.  As Peter points out the actual
> > > load isn't really the issue, it's locating the right spot for it.
> >
> > I don't really see why that's a problem, it's just a pointer
> > addition - base + fdt_address_cells * 4. The problem was in
>
> This is harder if #address-cells and #size-cells are different, or if
> you're parsing ranges and #address-cells is different between parent
> and child node.
>
> > the memory access (yes it's fixed with the patch but you could
> > add a general libfdt way to do it).
>
> Huh.. well I'm getting different impressions of what the problem
> actually is from what I initially read versus Peter Maydell's
> comments, so I don't really know what to think.
>
> If it's just the load then fdt32_ld() etc. already exist.  Or is it
> really such a hot path that unconditionally handling unaligned
> accesses isn't tenable?

The specific problem here is that the code as written tries to
cast a not-aligned-enough pointer to uint64_t* to do the load,
which is UB. The patch submitted fixes that, and personally I
think it would be entirely fine to say that's all we need to do here.

*If* we want to look at the broader question of "why is this
code that's reading something out of an fdt having to do the
pretty low-level action of getting the start address of the
fdt property and then doing pointer arithmetic and then fishing
a value out of it as a 64-bit unaligned load?" then we get into
"do we want to provide a helper function that lets the caller
say 'give me element X from tuple Y'?". But that seems like a
lot of effort for what's basically a single callsite we would
be tidying up...

thanks
-- PMM

Re: [PATCH v2 06/15] ppc/vof: Fix unaligned FDT property access

[David Gibson]

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

On Sat, Jul 06, 2024 at 11:37:08AM +0100, Peter Maydell wrote:
> On Fri, 5 Jul 2024 at 06:13, David Gibson <david@gibson.dropbear.id.au> wrote:
> >
> > On Fri, Jul 05, 2024 at 02:40:19PM +1000, Nicholas Piggin wrote:
> > > On Fri Jul 5, 2024 at 11:41 AM AEST, David Gibson wrote:
> > > > On Fri, Jul 05, 2024 at 11:18:47AM +1000, Nicholas Piggin wrote:
> > > > > On Thu Jul 4, 2024 at 10:15 PM AEST, Peter Maydell wrote:
> > > > > > On Sat, 29 Jun 2024 at 04:17, David Gibson <david@gibson.dropbear.id.au> wrote:
> > > > > > >
> > > > > > > On Fri, Jun 28, 2024 at 04:20:02PM +0100, Peter Maydell wrote:
> > > > > > > > On Thu, 27 Jun 2024 at 14:39, Akihiko Odaki <akihiko.odaki@daynix.com> 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));
> > > > > > > > >      }
> > > > > > > >
> > > > > > > > I did wonder if there was a better way to do what this is doing,
> > > > > > > > but neither we (in system/device_tree.c) nor libfdt seem to
> > > > > > > > provide one.
> > > > > > >
> > > > > > > libfdt does provide unaligned access helpers (fdt32_ld() etc.), but
> > > > > > > not an automatic aligned-or-unaligned helper.   Maybe we should add that?
> > > > > >
> > > > > > fdt32_ld() and friends only do the "load from this bit of memory"
> > > > > > part, which we already have QEMU utility functions for (and which
> > > > > > are this patch uses).
> > > > > >
> > > > > > This particular bit of code is dealing with an fdt property ("memory")
> > > > > > that is an array of (address, size) tuples where address and size
> > > > > > can independently be either 32 or 64 bits, and it wants the
> > > > > > size value of tuple 0. So the missing functionality is something at
> > > > > > a higher level than fdt32_ld() which would let you say "give me
> > > > > > tuple N field X" with some way to specify the tuple layout. (Which
> > > > > > is an awkward kind of API to write in C.)
> > > > > >
> > > > > > Slightly less general, but for this case we could perhaps have
> > > > > > something like the getprop equivalent of qemu_fdt_setprop_sized_cells():
> > > > > >
> > > > > >   uint64_t value_array[2];
> > > > > >   qemu_fdt_getprop_sized_cells(fdt, nodename, "memory", &value_array,
> > > > > >                                ac, sc);
> > > > > >   /*
> > > > > >    * fills in value_array[0] with address, value_array[1] with size,
> > > > > >    * probably barfs if the varargs-list of cell-sizes doesn't
> > > > > >    * cover the whole property, similar to the current assert on
> > > > > >    * proplen.
> > > > > >    */
> > > > > >   mem0_end = value_array[0];
> > > > >
> > > > > Since 4/8 byte cells are most common and size is probably
> > > > > normally known, what about something simpler to start with?
> > > >
> > > > Hrm, I don't think this helps much.  As Peter points out the actual
> > > > load isn't really the issue, it's locating the right spot for it.
> > >
> > > I don't really see why that's a problem, it's just a pointer
> > > addition - base + fdt_address_cells * 4. The problem was in
> >
> > This is harder if #address-cells and #size-cells are different, or if
> > you're parsing ranges and #address-cells is different between parent
> > and child node.
> >
> > > the memory access (yes it's fixed with the patch but you could
> > > add a general libfdt way to do it).
> >
> > Huh.. well I'm getting different impressions of what the problem
> > actually is from what I initially read versus Peter Maydell's
> > comments, so I don't really know what to think.
> >
> > If it's just the load then fdt32_ld() etc. already exist.  Or is it
> > really such a hot path that unconditionally handling unaligned
> > accesses isn't tenable?
> 
> The specific problem here is that the code as written tries to
> cast a not-aligned-enough pointer to uint64_t* to do the load,
> which is UB.

Ah... and I'm assuming it's the cast itself which triggers the UB, not
just dereferencing it.  Which makes the interface of fdt32_ld()
etc. unusable for their intended purpose.  Well.. damn.  Now... how do
I fix it without breaking compatibility any more than I have to.

> The patch submitted fixes that, and personally I
> think it would be entirely fine to say that's all we need to do here.
> 
> *If* we want to look at the broader question of "why is this
> code that's reading something out of an fdt having to do the
> pretty low-level action of getting the start address of the
> fdt property and then doing pointer arithmetic and then fishing
> a value out of it as a 64-bit unaligned load?" then we get into
> "do we want to provide a helper function that lets the caller
> say 'give me element X from tuple Y'?". But that seems like a
> lot of effort for what's basically a single callsite we would
> be tidying up...

I tend to agree.

-- 
David Gibson (he or they)	| 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 --]

Re: [PATCH v2 06/15] ppc/vof: Fix unaligned FDT property access

[Nicholas Piggin]

On Sun Jul 7, 2024 at 9:46 AM AEST, David Gibson wrote:
> On Sat, Jul 06, 2024 at 11:37:08AM +0100, Peter Maydell wrote:
> > On Fri, 5 Jul 2024 at 06:13, David Gibson <david@gibson.dropbear.id.au> wrote:
> > >
> > > On Fri, Jul 05, 2024 at 02:40:19PM +1000, Nicholas Piggin wrote:
> > > > On Fri Jul 5, 2024 at 11:41 AM AEST, David Gibson wrote:
> > > > > On Fri, Jul 05, 2024 at 11:18:47AM +1000, Nicholas Piggin wrote:
> > > > > > On Thu Jul 4, 2024 at 10:15 PM AEST, Peter Maydell wrote:
> > > > > > > On Sat, 29 Jun 2024 at 04:17, David Gibson <david@gibson.dropbear.id.au> wrote:
> > > > > > > >
> > > > > > > > On Fri, Jun 28, 2024 at 04:20:02PM +0100, Peter Maydell wrote:
> > > > > > > > > On Thu, 27 Jun 2024 at 14:39, Akihiko Odaki <akihiko.odaki@daynix.com> 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));
> > > > > > > > > >      }
> > > > > > > > >
> > > > > > > > > I did wonder if there was a better way to do what this is doing,
> > > > > > > > > but neither we (in system/device_tree.c) nor libfdt seem to
> > > > > > > > > provide one.
> > > > > > > >
> > > > > > > > libfdt does provide unaligned access helpers (fdt32_ld() etc.), but
> > > > > > > > not an automatic aligned-or-unaligned helper.   Maybe we should add that?
> > > > > > >
> > > > > > > fdt32_ld() and friends only do the "load from this bit of memory"
> > > > > > > part, which we already have QEMU utility functions for (and which
> > > > > > > are this patch uses).
> > > > > > >
> > > > > > > This particular bit of code is dealing with an fdt property ("memory")
> > > > > > > that is an array of (address, size) tuples where address and size
> > > > > > > can independently be either 32 or 64 bits, and it wants the
> > > > > > > size value of tuple 0. So the missing functionality is something at
> > > > > > > a higher level than fdt32_ld() which would let you say "give me
> > > > > > > tuple N field X" with some way to specify the tuple layout. (Which
> > > > > > > is an awkward kind of API to write in C.)
> > > > > > >
> > > > > > > Slightly less general, but for this case we could perhaps have
> > > > > > > something like the getprop equivalent of qemu_fdt_setprop_sized_cells():
> > > > > > >
> > > > > > >   uint64_t value_array[2];
> > > > > > >   qemu_fdt_getprop_sized_cells(fdt, nodename, "memory", &value_array,
> > > > > > >                                ac, sc);
> > > > > > >   /*
> > > > > > >    * fills in value_array[0] with address, value_array[1] with size,
> > > > > > >    * probably barfs if the varargs-list of cell-sizes doesn't
> > > > > > >    * cover the whole property, similar to the current assert on
> > > > > > >    * proplen.
> > > > > > >    */
> > > > > > >   mem0_end = value_array[0];
> > > > > >
> > > > > > Since 4/8 byte cells are most common and size is probably
> > > > > > normally known, what about something simpler to start with?
> > > > >
> > > > > Hrm, I don't think this helps much.  As Peter points out the actual
> > > > > load isn't really the issue, it's locating the right spot for it.
> > > >
> > > > I don't really see why that's a problem, it's just a pointer
> > > > addition - base + fdt_address_cells * 4. The problem was in
> > >
> > > This is harder if #address-cells and #size-cells are different, or if
> > > you're parsing ranges and #address-cells is different between parent
> > > and child node.
> > >
> > > > the memory access (yes it's fixed with the patch but you could
> > > > add a general libfdt way to do it).
> > >
> > > Huh.. well I'm getting different impressions of what the problem
> > > actually is from what I initially read versus Peter Maydell's
> > > comments, so I don't really know what to think.
> > >
> > > If it's just the load then fdt32_ld() etc. already exist.  Or is it
> > > really such a hot path that unconditionally handling unaligned
> > > accesses isn't tenable?
> > 
> > The specific problem here is that the code as written tries to
> > cast a not-aligned-enough pointer to uint64_t* to do the load,
> > which is UB.
>
> Ah... and I'm assuming it's the cast itself which triggers the UB, not
> just dereferencing it.

Oh it's just the cast itself that is UB? Looks like that's true.
Interesting gcc and clang don't flag it, I guess they care about
warning on practical breakage first.

> Which makes the interface of fdt32_ld()
> etc. unusable for their intended purpose.  Well.. damn.  Now... how do
> I fix it without breaking compatibility any more than I have to.

Why not just make them take a void * ptr? I don't think that would
break anything but existing code that was forced to add the cast
may be at risk of the UB.

Could also add fdt64_unaligned_t types with aligned(1) attribute
for new code. Those can just be dereferenced directly and the
caller the compiler can choose the appropriate access supported by
the host. (Actually gcc can recognise that load unaligned and
byteswap pattern and do the same anyway, but clang at least can
not yet).

Thanks,
Nick

Re: [PATCH v2 06/15] ppc/vof: Fix unaligned FDT property access

[Peter Maydell]

On Mon, 8 Jul 2024 at 08:49, Nicholas Piggin <npiggin@gmail.com> wrote:
>
> On Sun Jul 7, 2024 at 9:46 AM AEST, David Gibson wrote:
> > On Sat, Jul 06, 2024 at 11:37:08AM +0100, Peter Maydell wrote:
> > > On Fri, 5 Jul 2024 at 06:13, David Gibson <david@gibson.dropbear.id.au> wrote:
> > > > Huh.. well I'm getting different impressions of what the problem
> > > > actually is from what I initially read versus Peter Maydell's
> > > > comments, so I don't really know what to think.
> > > >
> > > > If it's just the load then fdt32_ld() etc. already exist.  Or is it
> > > > really such a hot path that unconditionally handling unaligned
> > > > accesses isn't tenable?
> > >
> > > The specific problem here is that the code as written tries to
> > > cast a not-aligned-enough pointer to uint64_t* to do the load,
> > > which is UB.
> >
> > Ah... and I'm assuming it's the cast itself which triggers the UB, not
> > just dereferencing it.
>
> Oh it's just the cast itself that is UB? Looks like that's true.
> Interesting gcc and clang don't flag it, I guess they care about
> warning on practical breakage first.

Er, I was speaking a bit vaguely there, don't take my word for
it without going and looking at the text of the C standard.

What I *meant* was that the practical problem here is that we
really do dereference a pointer for a 64-bit load when the
pointer isn't necessarily 64-bit-aligned.

As it happens, C99 says that it is the cast that is UB:
section 6.3.2.3 para 7 says:
 "A pointer to an object or incomplete type may be converted to
  a pointer to a different object or incomplete type. If the
  resulting pointer is not correctly aligned for the pointed-to
  type, the behavior is undefined. Otherwise, when converted back
  again, the result shall compare equal to the original pointer."

Presumably this is envisaging the possibility of a pointer cast
being a destructive operation somehow, such that e.g. a uint64_t*
can only represent 64-bit-aligned values. But I bet QEMU does
a lot of casting pointers around that might fall foul of this
rule, so I'm not particularly worried about trying to clean up
that kind of thing (until/unless analysers start warning about
it, in which case we have a specific set of things to clean up).

What I care about from the point of view of this patch
is that we fix the actually-broken-on-some-real-hardware problem
of doing the load as a misaligned access. My vote would be for
"take Akihiko's patch as-is, rather than gating fixing the bug
on deciding on an improvement/change to the fdt API or our
wrappers of it".

thanks
-- PMM

Re: [PATCH v2 06/15] ppc/vof: Fix unaligned FDT property access

[David Gibson]

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

On Mon, Jul 08, 2024 at 04:59:30PM +0100, Peter Maydell wrote:
> On Mon, 8 Jul 2024 at 08:49, Nicholas Piggin <npiggin@gmail.com> wrote:
> >
> > On Sun Jul 7, 2024 at 9:46 AM AEST, David Gibson wrote:
> > > On Sat, Jul 06, 2024 at 11:37:08AM +0100, Peter Maydell wrote:
> > > > On Fri, 5 Jul 2024 at 06:13, David Gibson <david@gibson.dropbear.id.au> wrote:
> > > > > Huh.. well I'm getting different impressions of what the problem
> > > > > actually is from what I initially read versus Peter Maydell's
> > > > > comments, so I don't really know what to think.
> > > > >
> > > > > If it's just the load then fdt32_ld() etc. already exist.  Or is it
> > > > > really such a hot path that unconditionally handling unaligned
> > > > > accesses isn't tenable?
> > > >
> > > > The specific problem here is that the code as written tries to
> > > > cast a not-aligned-enough pointer to uint64_t* to do the load,
> > > > which is UB.
> > >
> > > Ah... and I'm assuming it's the cast itself which triggers the UB, not
> > > just dereferencing it.
> >
> > Oh it's just the cast itself that is UB? Looks like that's true.
> > Interesting gcc and clang don't flag it, I guess they care about
> > warning on practical breakage first.
> 
> Er, I was speaking a bit vaguely there, don't take my word for
> it without going and looking at the text of the C standard.

Sure.

> What I *meant* was that the practical problem here is that we
> really do dereference a pointer for a 64-bit load when the
> pointer isn't necessarily 64-bit-aligned.

From the qemu point of view, yes.  And theoretically, the fix is easy,
since libfdt provides fdt32_ld() etc. for exactly this use case.  But..

> As it happens, C99 says that it is the cast that is UB:
> section 6.3.2.3 para 7 says:
>  "A pointer to an object or incomplete type may be converted to
>   a pointer to a different object or incomplete type. If the
>   resulting pointer is not correctly aligned for the pointed-to
>   type, the behavior is undefined. Otherwise, when converted back
>   again, the result shall compare equal to the original pointer."

.. this makes fdt32_ld() etc. unusable by design.

> Presumably this is envisaging the possibility of a pointer cast
> being a destructive operation somehow, such that e.g. a uint64_t*
> can only represent 64-bit-aligned values. But I bet QEMU does
> a lot of casting pointers around that might fall foul of this
> rule, so I'm not particularly worried about trying to clean up
> that kind of thing (until/unless analysers start warning about
> it, in which case we have a specific set of things to clean up).

Fair enough from the qemu point of view.  However, this unusable by
design interface was written by me as part of a library I maintain, so
it certainly worries *me*.

> What I care about from the point of view of this patch
> is that we fix the actually-broken-on-some-real-hardware problem
> of doing the load as a misaligned access. My vote would be for
> "take Akihiko's patch as-is, rather than gating fixing the bug
> on deciding on an improvement/change to the fdt API or our
> wrappers of it".
> 
> thanks
> -- PMM
> 

-- 
David Gibson (he or they)	| 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 --]

Re: [PATCH v2 06/15] ppc/vof: Fix unaligned FDT property access

[David Gibson]

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

On Mon, Jul 08, 2024 at 05:49:32PM +1000, Nicholas Piggin wrote:
> On Sun Jul 7, 2024 at 9:46 AM AEST, David Gibson wrote:
> > On Sat, Jul 06, 2024 at 11:37:08AM +0100, Peter Maydell wrote:
> > > On Fri, 5 Jul 2024 at 06:13, David Gibson <david@gibson.dropbear.id.au> wrote:
> > > >
> > > > On Fri, Jul 05, 2024 at 02:40:19PM +1000, Nicholas Piggin wrote:
> > > > > On Fri Jul 5, 2024 at 11:41 AM AEST, David Gibson wrote:
> > > > > > On Fri, Jul 05, 2024 at 11:18:47AM +1000, Nicholas Piggin wrote:
> > > > > > > On Thu Jul 4, 2024 at 10:15 PM AEST, Peter Maydell wrote:
> > > > > > > > On Sat, 29 Jun 2024 at 04:17, David Gibson <david@gibson.dropbear.id.au> wrote:
> > > > > > > > >
> > > > > > > > > On Fri, Jun 28, 2024 at 04:20:02PM +0100, Peter Maydell wrote:
> > > > > > > > > > On Thu, 27 Jun 2024 at 14:39, Akihiko Odaki <akihiko.odaki@daynix.com> 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));
> > > > > > > > > > >      }
> > > > > > > > > >
> > > > > > > > > > I did wonder if there was a better way to do what this is doing,
> > > > > > > > > > but neither we (in system/device_tree.c) nor libfdt seem to
> > > > > > > > > > provide one.
> > > > > > > > >
> > > > > > > > > libfdt does provide unaligned access helpers (fdt32_ld() etc.), but
> > > > > > > > > not an automatic aligned-or-unaligned helper.   Maybe we should add that?
> > > > > > > >
> > > > > > > > fdt32_ld() and friends only do the "load from this bit of memory"
> > > > > > > > part, which we already have QEMU utility functions for (and which
> > > > > > > > are this patch uses).
> > > > > > > >
> > > > > > > > This particular bit of code is dealing with an fdt property ("memory")
> > > > > > > > that is an array of (address, size) tuples where address and size
> > > > > > > > can independently be either 32 or 64 bits, and it wants the
> > > > > > > > size value of tuple 0. So the missing functionality is something at
> > > > > > > > a higher level than fdt32_ld() which would let you say "give me
> > > > > > > > tuple N field X" with some way to specify the tuple layout. (Which
> > > > > > > > is an awkward kind of API to write in C.)
> > > > > > > >
> > > > > > > > Slightly less general, but for this case we could perhaps have
> > > > > > > > something like the getprop equivalent of qemu_fdt_setprop_sized_cells():
> > > > > > > >
> > > > > > > >   uint64_t value_array[2];
> > > > > > > >   qemu_fdt_getprop_sized_cells(fdt, nodename, "memory", &value_array,
> > > > > > > >                                ac, sc);
> > > > > > > >   /*
> > > > > > > >    * fills in value_array[0] with address, value_array[1] with size,
> > > > > > > >    * probably barfs if the varargs-list of cell-sizes doesn't
> > > > > > > >    * cover the whole property, similar to the current assert on
> > > > > > > >    * proplen.
> > > > > > > >    */
> > > > > > > >   mem0_end = value_array[0];
> > > > > > >
> > > > > > > Since 4/8 byte cells are most common and size is probably
> > > > > > > normally known, what about something simpler to start with?
> > > > > >
> > > > > > Hrm, I don't think this helps much.  As Peter points out the actual
> > > > > > load isn't really the issue, it's locating the right spot for it.
> > > > >
> > > > > I don't really see why that's a problem, it's just a pointer
> > > > > addition - base + fdt_address_cells * 4. The problem was in
> > > >
> > > > This is harder if #address-cells and #size-cells are different, or if
> > > > you're parsing ranges and #address-cells is different between parent
> > > > and child node.
> > > >
> > > > > the memory access (yes it's fixed with the patch but you could
> > > > > add a general libfdt way to do it).
> > > >
> > > > Huh.. well I'm getting different impressions of what the problem
> > > > actually is from what I initially read versus Peter Maydell's
> > > > comments, so I don't really know what to think.
> > > >
> > > > If it's just the load then fdt32_ld() etc. already exist.  Or is it
> > > > really such a hot path that unconditionally handling unaligned
> > > > accesses isn't tenable?
> > > 
> > > The specific problem here is that the code as written tries to
> > > cast a not-aligned-enough pointer to uint64_t* to do the load,
> > > which is UB.
> >
> > Ah... and I'm assuming it's the cast itself which triggers the UB, not
> > just dereferencing it.
> 
> Oh it's just the cast itself that is UB? Looks like that's true.
> Interesting gcc and clang don't flag it, I guess they care about
> warning on practical breakage first.
> 
> > Which makes the interface of fdt32_ld()
> > etc. unusable for their intended purpose.  Well.. damn.  Now... how do
> > I fix it without breaking compatibility any more than I have to.
> 
> Why not just make them take a void * ptr? I don't think that would
> break anything but existing code that was forced to add the cast
> may be at risk of the UB.

That works if recompiling, but I believe it's an ABI change.
Typically library users would get these functions as inlines, but I
believe it's not impossible something could have taken function
pointers to them and get versions from the shared library.

Or... maybe it's not possible, since these aren't listed our
version.lds.  Ugh.

Given the current state is something basically impossible to use
without UB, wearing an ABI breakage might be the lesser evil.

> Could also add fdt64_unaligned_t types with aligned(1) attribute
> for new code. Those can just be dereferenced directly and the
> caller the compiler can choose the appropriate access supported by
> the host. (Actually gcc can recognise that load unaligned and
> byteswap pattern and do the same anyway, but clang at least can
> not yet).

That might be a nice idea, but doesn't solve the immediate versioning
problem.

-- 
David Gibson (he or they)	| 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 --]

Re: [PATCH v2 06/15] ppc/vof: Fix unaligned FDT property access

[David Gibson]

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

On Thu, Jul 04, 2024 at 01:15:57PM +0100, Peter Maydell wrote:
> On Sat, 29 Jun 2024 at 04:17, David Gibson <david@gibson.dropbear.id.au> wrote:
> >
> > On Fri, Jun 28, 2024 at 04:20:02PM +0100, Peter Maydell wrote:
> > > On Thu, 27 Jun 2024 at 14:39, Akihiko Odaki <akihiko.odaki@daynix.com> 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));
> > > >      }
> > >
> > > I did wonder if there was a better way to do what this is doing,
> > > but neither we (in system/device_tree.c) nor libfdt seem to
> > > provide one.
> >
> > libfdt does provide unaligned access helpers (fdt32_ld() etc.), but
> > not an automatic aligned-or-unaligned helper.   Maybe we should add that?
> 
> fdt32_ld() and friends only do the "load from this bit of memory"
> part, which we already have QEMU utility functions for (and which
> are this patch uses).
> 
> This particular bit of code is dealing with an fdt property ("memory")
> that is an array of (address, size) tuples where address and size
> can independently be either 32 or 64 bits, and it wants the
> size value of tuple 0. So the missing functionality is something at
> a higher level than fdt32_ld() which would let you say "give me
> tuple N field X" with some way to specify the tuple layout. (Which
> is an awkward kind of API to write in C.)

Ah, right.  Yeah.. that's a pretty awkward API in C.

> Slightly less general, but for this case we could perhaps have
> something like the getprop equivalent of qemu_fdt_setprop_sized_cells():
> 
>   uint64_t value_array[2];
>   qemu_fdt_getprop_sized_cells(fdt, nodename, "memory", &value_array,
>                                ac, sc);
>   /*
>    * fills in value_array[0] with address, value_array[1] with size,
>    * probably barfs if the varargs-list of cell-sizes doesn't
>    * cover the whole property, similar to the current assert on
>    * proplen.
>    */
>   mem0_end = value_array[0];

Seems reasonable to me.  The only other thought I had was something
like Python's struct.unpack() [0].  But your suggestion is probably
more natural in C.

[0] https://docs.python.org/3/library/struct.html#struct.unpack

-- 
David Gibson (he or they)	| 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 --]

[PATCH v2 07/15] hw/virtio: Free vqs after vhost_dev_cleanup()

[Akihiko Odaki]

This fixes 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 v2 08/15] migration: Free removed SaveStateEntry

[Akihiko Odaki]

This fixes LeakSanitizer warnings.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
Reviewed-by: Peter Xu <peterx@redhat.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

Re: (subset) [PATCH v2 08/15] migration: Free removed SaveStateEntry

[Fabiano Rosas]

On Thu, 27 Jun 2024 22:37:51 +0900, Akihiko Odaki wrote:
> This fixes LeakSanitizer warnings.
> 
> 

Queued, thanks!

[PATCH v2 09/15] 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 74cd73ebc78b..949f5016a68d 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

Re: [PATCH v2 09/15] memory: Do not create circular reference with subregion

[Peter Xu]

On Thu, Jun 27, 2024 at 10:37:52PM +0900, Akihiko Odaki wrote:
> 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 74cd73ebc78b..949f5016a68d 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();
>  }

This does look like a real issue.. the patch looks reasonable to me, but I
wonder whether we should start to add some good comments in code to reflect
that complexity starting from this one.  The MR refcount isn't easy to
understand to me.

It also lets me start to wonder how MR refcount went through until it looks
like today..  It's definitely not extremely intuitive to use mr->owner as
the object to do refcounting if mr itself does has its own QObject,
meanwhile it has other tricks around.

E.g. the first thing I stumbled over when looking was the optimization
where we will avoid refcounting the mr when there's no owner, and IIUC it
was for the case when the "guest memory" (which will never be freed) used
to have no owner so we can speedup DMA if we know it won't go away.

https://lore.kernel.org/qemu-devel/1450263601-2828-5-git-send-email-pbonzini@redhat.com/

commit 612263cf33062f7441a5d0e3b37c65991fdc3210
Author: Paolo Bonzini <pbonzini@redhat.com>
Date:   Wed Dec 9 11:44:25 2015 +0100

    memory: avoid unnecessary object_ref/unref
    
    For the common case of DMA into non-hotplugged RAM, it is unnecessary
    but expensive to do object_ref/unref.  Add back an owner field to
    MemoryRegion, so that these memory regions can skip the reference
    counting.

If so, it looks like it will stop working with memory-backends get
involved?  As I think those MRs will have owner set always, and I wonder
whether memory-backends should be the major way to specify guest memory now
and in the future.  So I'm not sure how important that optimization is as
of now, and whether we could "simplify" it back to always do the refcount
if the major scenarios will not adopt it.

The other issue is we used owner refcount from the start of
memory_region_ref() got introduced, since:

commit 46637be269aaaceb9867ffdf176e906401138fff
Author: Paolo Bonzini <pbonzini@redhat.com>
Date:   Tue May 7 09:06:00 2013 +0200

    memory: add ref/unref

And we still have that in our document, even though I don't think it's true
anymore:

 * ...  MemoryRegions actually do not have their
 * own reference count; they piggyback on a QOM object, their "owner".
 * This function adds a reference to the owner.

It looks like what happened is when introduced the change, MR is not a QOM
object yet.  But it later is..

I mentioned all these only because I found that _if_ we can keep mr
refcounting as simple as other objects:

memory_region_ref(mr)
{
    object_ref(OBJECT(mr));
}

Then looks like this "recursive refcount" problem can also go away.  I'm
curious whether you or anyone tried to explore that path, or whether above
doesn't make sense at all.

Thanks,

-- 
Peter Xu

Re: [PATCH v2 09/15] memory: Do not create circular reference with subregion

[Akihiko Odaki]

On 2024/07/03 2:44, Peter Xu wrote:
> On Thu, Jun 27, 2024 at 10:37:52PM +0900, Akihiko Odaki wrote:
>> 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 74cd73ebc78b..949f5016a68d 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();
>>   }
> 
> This does look like a real issue.. the patch looks reasonable to me, but I
> wonder whether we should start to add some good comments in code to reflect
> that complexity starting from this one.  The MR refcount isn't easy to
> understand to me.
> 
> It also lets me start to wonder how MR refcount went through until it looks
> like today..  It's definitely not extremely intuitive to use mr->owner as
> the object to do refcounting if mr itself does has its own QObject,
> meanwhile it has other tricks around.
> 
> E.g. the first thing I stumbled over when looking was the optimization
> where we will avoid refcounting the mr when there's no owner, and IIUC it
> was for the case when the "guest memory" (which will never be freed) used
> to have no owner so we can speedup DMA if we know it won't go away.
> 
> https://lore.kernel.org/qemu-devel/1450263601-2828-5-git-send-email-pbonzini@redhat.com/
> 
> commit 612263cf33062f7441a5d0e3b37c65991fdc3210
> Author: Paolo Bonzini <pbonzini@redhat.com>
> Date:   Wed Dec 9 11:44:25 2015 +0100
> 
>      memory: avoid unnecessary object_ref/unref
>      
>      For the common case of DMA into non-hotplugged RAM, it is unnecessary
>      but expensive to do object_ref/unref.  Add back an owner field to
>      MemoryRegion, so that these memory regions can skip the reference
>      counting.
> 
> If so, it looks like it will stop working with memory-backends get
> involved?  As I think those MRs will have owner set always, and I wonder
> whether memory-backends should be the major way to specify guest memory now
> and in the future.  So I'm not sure how important that optimization is as
> of now, and whether we could "simplify" it back to always do the refcount
> if the major scenarios will not adopt it.
> 
> The other issue is we used owner refcount from the start of
> memory_region_ref() got introduced, since:
> 
> commit 46637be269aaaceb9867ffdf176e906401138fff
> Author: Paolo Bonzini <pbonzini@redhat.com>
> Date:   Tue May 7 09:06:00 2013 +0200
> 
>      memory: add ref/unref
> 
> And we still have that in our document, even though I don't think it's true
> anymore:
> 
>   * ...  MemoryRegions actually do not have their
>   * own reference count; they piggyback on a QOM object, their "owner".
>   * This function adds a reference to the owner.
> 
> It looks like what happened is when introduced the change, MR is not a QOM
> object yet.  But it later is..
> 
> I mentioned all these only because I found that _if_ we can keep mr
> refcounting as simple as other objects:
> 
> memory_region_ref(mr)
> {
>      object_ref(OBJECT(mr));
> }
> 
> Then looks like this "recursive refcount" problem can also go away.  I'm
> curious whether you or anyone tried to explore that path, or whether above
> doesn't make sense at all.

It unfortunately does not solve the problem.

The underlying problem is that the whole device must be kept alive while 
its memory region are. Indeed MemoryRegions do have refcounts, but 
incrementing them do not extend the lifetime of the devices (i.e., the 
owners). The refcount of the owners must be incremented for correctness.

Referencing a subregion MemoryRegion from its container MemoryRegion 
owned by the same device is an exceptional case. Incrementing the 
refcount of the owner extends the owner's lifetime to forever.

Regards,
Akihiko Odaki

Re: [PATCH v2 09/15] memory: Do not create circular reference with subregion

[Philippe Mathieu-Daudé]

On 6/7/24 13:59, Akihiko Odaki wrote:
> On 2024/07/03 2:44, Peter Xu wrote:
>> On Thu, Jun 27, 2024 at 10:37:52PM +0900, Akihiko Odaki wrote:
>>> 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 74cd73ebc78b..949f5016a68d 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();
>>>   }
>>
>> This does look like a real issue.. the patch looks reasonable to me, 
>> but I
>> wonder whether we should start to add some good comments in code to 
>> reflect
>> that complexity starting from this one.  The MR refcount isn't easy to
>> understand to me.
>>
>> It also lets me start to wonder how MR refcount went through until it 
>> looks
>> like today..  It's definitely not extremely intuitive to use mr->owner as
>> the object to do refcounting if mr itself does has its own QObject,
>> meanwhile it has other tricks around.
>>
>> E.g. the first thing I stumbled over when looking was the optimization
>> where we will avoid refcounting the mr when there's no owner, and IIUC it
>> was for the case when the "guest memory" (which will never be freed) used
>> to have no owner so we can speedup DMA if we know it won't go away.
>>
>> https://lore.kernel.org/qemu-devel/1450263601-2828-5-git-send-email-pbonzini@redhat.com/
>>
>> commit 612263cf33062f7441a5d0e3b37c65991fdc3210
>> Author: Paolo Bonzini <pbonzini@redhat.com>
>> Date:   Wed Dec 9 11:44:25 2015 +0100
>>
>>      memory: avoid unnecessary object_ref/unref
>>      For the common case of DMA into non-hotplugged RAM, it is 
>> unnecessary
>>      but expensive to do object_ref/unref.  Add back an owner field to
>>      MemoryRegion, so that these memory regions can skip the reference
>>      counting.
>>
>> If so, it looks like it will stop working with memory-backends get
>> involved?  As I think those MRs will have owner set always, and I wonder
>> whether memory-backends should be the major way to specify guest 
>> memory now
>> and in the future.  So I'm not sure how important that optimization is as
>> of now, and whether we could "simplify" it back to always do the refcount
>> if the major scenarios will not adopt it.
>>
>> The other issue is we used owner refcount from the start of
>> memory_region_ref() got introduced, since:
>>
>> commit 46637be269aaaceb9867ffdf176e906401138fff
>> Author: Paolo Bonzini <pbonzini@redhat.com>
>> Date:   Tue May 7 09:06:00 2013 +0200
>>
>>      memory: add ref/unref
>>
>> And we still have that in our document, even though I don't think it's 
>> true
>> anymore:
>>
>>   * ...  MemoryRegions actually do not have their
>>   * own reference count; they piggyback on a QOM object, their "owner".
>>   * This function adds a reference to the owner.
>>
>> It looks like what happened is when introduced the change, MR is not a 
>> QOM
>> object yet.  But it later is..
>>
>> I mentioned all these only because I found that _if_ we can keep mr
>> refcounting as simple as other objects:
>>
>> memory_region_ref(mr)
>> {
>>      object_ref(OBJECT(mr));
>> }
>>
>> Then looks like this "recursive refcount" problem can also go away.  I'm
>> curious whether you or anyone tried to explore that path, or whether 
>> above
>> doesn't make sense at all.
> 
> It unfortunately does not solve the problem.
> 
> The underlying problem is that the whole device must be kept alive while 
> its memory region are. Indeed MemoryRegions do have refcounts, but 
> incrementing them do not extend the lifetime of the devices (i.e., the 
> owners). The refcount of the owners must be incremented for correctness.
> 
> Referencing a subregion MemoryRegion from its container MemoryRegion 
> owned by the same device is an exceptional case. Incrementing the 
> refcount of the owner extends the owner's lifetime to forever.

Is it really an exceptional case?

What I'm seeing are a lot of devices creating MR and never bother
to destroy them, so indeed owner (device) refcount never reaches 0.

Most of the time when we create MR in .instance_init/.realize,
we neglect to implement the undo path (.instance_finalize or
.unrealize).

Re: [PATCH v2 09/15] memory: Do not create circular reference with subregion

[Akihiko Odaki]

On 2024/07/08 17:06, Philippe Mathieu-Daudé wrote:
> On 6/7/24 13:59, Akihiko Odaki wrote:
>> On 2024/07/03 2:44, Peter Xu wrote:
>>> On Thu, Jun 27, 2024 at 10:37:52PM +0900, Akihiko Odaki wrote:
>>>> 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 74cd73ebc78b..949f5016a68d 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();
>>>>   }
>>>
>>> This does look like a real issue.. the patch looks reasonable to me, 
>>> but I
>>> wonder whether we should start to add some good comments in code to 
>>> reflect
>>> that complexity starting from this one.  The MR refcount isn't easy to
>>> understand to me.
>>>
>>> It also lets me start to wonder how MR refcount went through until it 
>>> looks
>>> like today..  It's definitely not extremely intuitive to use 
>>> mr->owner as
>>> the object to do refcounting if mr itself does has its own QObject,
>>> meanwhile it has other tricks around.
>>>
>>> E.g. the first thing I stumbled over when looking was the optimization
>>> where we will avoid refcounting the mr when there's no owner, and 
>>> IIUC it
>>> was for the case when the "guest memory" (which will never be freed) 
>>> used
>>> to have no owner so we can speedup DMA if we know it won't go away.
>>>
>>> https://lore.kernel.org/qemu-devel/1450263601-2828-5-git-send-email-pbonzini@redhat.com/
>>>
>>> commit 612263cf33062f7441a5d0e3b37c65991fdc3210
>>> Author: Paolo Bonzini <pbonzini@redhat.com>
>>> Date:   Wed Dec 9 11:44:25 2015 +0100
>>>
>>>      memory: avoid unnecessary object_ref/unref
>>>      For the common case of DMA into non-hotplugged RAM, it is 
>>> unnecessary
>>>      but expensive to do object_ref/unref.  Add back an owner field to
>>>      MemoryRegion, so that these memory regions can skip the reference
>>>      counting.
>>>
>>> If so, it looks like it will stop working with memory-backends get
>>> involved?  As I think those MRs will have owner set always, and I wonder
>>> whether memory-backends should be the major way to specify guest 
>>> memory now
>>> and in the future.  So I'm not sure how important that optimization 
>>> is as
>>> of now, and whether we could "simplify" it back to always do the 
>>> refcount
>>> if the major scenarios will not adopt it.
>>>
>>> The other issue is we used owner refcount from the start of
>>> memory_region_ref() got introduced, since:
>>>
>>> commit 46637be269aaaceb9867ffdf176e906401138fff
>>> Author: Paolo Bonzini <pbonzini@redhat.com>
>>> Date:   Tue May 7 09:06:00 2013 +0200
>>>
>>>      memory: add ref/unref
>>>
>>> And we still have that in our document, even though I don't think 
>>> it's true
>>> anymore:
>>>
>>>   * ...  MemoryRegions actually do not have their
>>>   * own reference count; they piggyback on a QOM object, their "owner".
>>>   * This function adds a reference to the owner.
>>>
>>> It looks like what happened is when introduced the change, MR is not 
>>> a QOM
>>> object yet.  But it later is..
>>>
>>> I mentioned all these only because I found that _if_ we can keep mr
>>> refcounting as simple as other objects:
>>>
>>> memory_region_ref(mr)
>>> {
>>>      object_ref(OBJECT(mr));
>>> }
>>>
>>> Then looks like this "recursive refcount" problem can also go away.  I'm
>>> curious whether you or anyone tried to explore that path, or whether 
>>> above
>>> doesn't make sense at all.
>>
>> It unfortunately does not solve the problem.
>>
>> The underlying problem is that the whole device must be kept alive 
>> while its memory region are. Indeed MemoryRegions do have refcounts, 
>> but incrementing them do not extend the lifetime of the devices (i.e., 
>> the owners). The refcount of the owners must be incremented for 
>> correctness.
>>
>> Referencing a subregion MemoryRegion from its container MemoryRegion 
>> owned by the same device is an exceptional case. Incrementing the 
>> refcount of the owner extends the owner's lifetime to forever.
> 
> Is it really an exceptional case?
> 
> What I'm seeing are a lot of devices creating MR and never bother
> to destroy them, so indeed owner (device) refcount never reaches 0.
> 
> Most of the time when we create MR in .instance_init/.realize,
> we neglect to implement the undo path (.instance_finalize or
> .unrealize).

I meant memory_region_update_container_subregions() is the only code 
that creates such a reference. The function itself is called in many 
code paths.

Re: [PATCH v2 09/15] memory: Do not create circular reference with subregion

[Peter Xu]

On Mon, Jul 08, 2024 at 10:06:44AM +0200, Philippe Mathieu-Daudé wrote:
> On 6/7/24 13:59, Akihiko Odaki wrote:
> > On 2024/07/03 2:44, Peter Xu wrote:
> > > On Thu, Jun 27, 2024 at 10:37:52PM +0900, Akihiko Odaki wrote:
> > > > 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 74cd73ebc78b..949f5016a68d 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();
> > > >   }
> > > 
> > > This does look like a real issue.. the patch looks reasonable to me,
> > > but I
> > > wonder whether we should start to add some good comments in code to
> > > reflect
> > > that complexity starting from this one.  The MR refcount isn't easy to
> > > understand to me.
> > > 
> > > It also lets me start to wonder how MR refcount went through until
> > > it looks
> > > like today..  It's definitely not extremely intuitive to use mr->owner as
> > > the object to do refcounting if mr itself does has its own QObject,
> > > meanwhile it has other tricks around.
> > > 
> > > E.g. the first thing I stumbled over when looking was the optimization
> > > where we will avoid refcounting the mr when there's no owner, and IIUC it
> > > was for the case when the "guest memory" (which will never be freed) used
> > > to have no owner so we can speedup DMA if we know it won't go away.
> > > 
> > > https://lore.kernel.org/qemu-devel/1450263601-2828-5-git-send-email-pbonzini@redhat.com/
> > > 
> > > commit 612263cf33062f7441a5d0e3b37c65991fdc3210
> > > Author: Paolo Bonzini <pbonzini@redhat.com>
> > > Date:   Wed Dec 9 11:44:25 2015 +0100
> > > 
> > >      memory: avoid unnecessary object_ref/unref
> > >      For the common case of DMA into non-hotplugged RAM, it is
> > > unnecessary
> > >      but expensive to do object_ref/unref.  Add back an owner field to
> > >      MemoryRegion, so that these memory regions can skip the reference
> > >      counting.
> > > 
> > > If so, it looks like it will stop working with memory-backends get
> > > involved?  As I think those MRs will have owner set always, and I wonder
> > > whether memory-backends should be the major way to specify guest
> > > memory now
> > > and in the future.  So I'm not sure how important that optimization is as
> > > of now, and whether we could "simplify" it back to always do the refcount
> > > if the major scenarios will not adopt it.
> > > 
> > > The other issue is we used owner refcount from the start of
> > > memory_region_ref() got introduced, since:
> > > 
> > > commit 46637be269aaaceb9867ffdf176e906401138fff
> > > Author: Paolo Bonzini <pbonzini@redhat.com>
> > > Date:   Tue May 7 09:06:00 2013 +0200
> > > 
> > >      memory: add ref/unref
> > > 
> > > And we still have that in our document, even though I don't think
> > > it's true
> > > anymore:
> > > 
> > >   * ...  MemoryRegions actually do not have their
> > >   * own reference count; they piggyback on a QOM object, their "owner".
> > >   * This function adds a reference to the owner.
> > > 
> > > It looks like what happened is when introduced the change, MR is not
> > > a QOM
> > > object yet.  But it later is..
> > > 
> > > I mentioned all these only because I found that _if_ we can keep mr
> > > refcounting as simple as other objects:
> > > 
> > > memory_region_ref(mr)
> > > {
> > >      object_ref(OBJECT(mr));
> > > }
> > > 
> > > Then looks like this "recursive refcount" problem can also go away.  I'm
> > > curious whether you or anyone tried to explore that path, or whether
> > > above
> > > doesn't make sense at all.
> > 
> > It unfortunately does not solve the problem.
> > 
> > The underlying problem is that the whole device must be kept alive while
> > its memory region are. Indeed MemoryRegions do have refcounts, but
> > incrementing them do not extend the lifetime of the devices (i.e., the
> > owners). The refcount of the owners must be incremented for correctness.
> > 
> > Referencing a subregion MemoryRegion from its container MemoryRegion
> > owned by the same device is an exceptional case. Incrementing the
> > refcount of the owner extends the owner's lifetime to forever.
> 
> Is it really an exceptional case?
> 
> What I'm seeing are a lot of devices creating MR and never bother
> to destroy them, so indeed owner (device) refcount never reaches 0.
> 
> Most of the time when we create MR in .instance_init/.realize,
> we neglect to implement the undo path (.instance_finalize or
> .unrealize).

Right.  The cross-reference of MR <-> device makes sense from the concept
level, but I'm not sure how much we rely on that from QEMU perspective
currently.  It complicates refcount and looks like it needs some thoughts.

One proof is, when I replied I actually tested all qemu qtests with below
patch and nothing yet explode:

===8<===
diff --git a/system/memory.c b/system/memory.c
index 2d69521360..a1d2c1f808 100644
--- a/system/memory.c
+++ b/system/memory.c
@@ -1801,26 +1801,12 @@ Object *memory_region_owner(MemoryRegion *mr)
 
 void memory_region_ref(MemoryRegion *mr)
 {
-    /* MMIO callbacks most likely will access data that belongs
-     * to the owner, hence the need to ref/unref the owner whenever
-     * the memory region is in use.
-     *
-     * The memory region is a child of its owner.  As long as the
-     * owner doesn't call unparent itself on the memory region,
-     * ref-ing the owner will also keep the memory region alive.
-     * Memory regions without an owner are supposed to never go away;
-     * we do not ref/unref them because it slows down DMA sensibly.
-     */
-    if (mr && mr->owner) {
-        object_ref(mr->owner);
-    }
+    object_ref(OBJECT(mr));
 }
 
 void memory_region_unref(MemoryRegion *mr)
 {
-    if (mr && mr->owner) {
-        object_unref(mr->owner);
-    }
+    object_unref(OBJECT(mr));
 }
===8<===

I'm not sure how much it covers dynamic destruction of devices, though,
where the device creates internal MRs to the guest address space.  I'm
actually wondering whether we have other barriers already to avoid having
any MR ops being invoked if a device was removed.

So I think above indeed would remove the cross-ref we used to have. It
might be a matter of whether that's fine for now to fix the cyclic ref
issue alone, and even if we still want some cross-ref.. whether we should
do it the old way.  I mean, the cross-ref can also be done in other forms,
and it may not block MR has its own refcounts.

Even if we want to have dev<->MR refcount, I wonder whether we should avoid
having MR refcount the device, because it looks like "sub-object refs the
parent" which sounds like a good source of cyclic refcount issue.  I wonder
whether device should ref the MR instead (when MR is dynamically created; I
am not sure whether we even need that if in Phil's comment where the MR
object is a sub-struct-field of the Device object).  Then the device can
provide a proper destroy() function to release the MRs under it, perhaps by
disabling them first (then any ops should already fail after that), then
unref the MRs with the hope that they're the last ref holder then MRs
destroys there too.

Thanks,

-- 
Peter Xu

Re: [PATCH v2 09/15] memory: Do not create circular reference with subregion

[Akihiko Odaki]

On 2024/07/09 1:40, Peter Xu wrote:
> On Mon, Jul 08, 2024 at 10:06:44AM +0200, Philippe Mathieu-Daudé wrote:
>> On 6/7/24 13:59, Akihiko Odaki wrote:
>>> On 2024/07/03 2:44, Peter Xu wrote:
>>>> On Thu, Jun 27, 2024 at 10:37:52PM +0900, Akihiko Odaki wrote:
>>>>> 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 74cd73ebc78b..949f5016a68d 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();
>>>>>    }
>>>>
>>>> This does look like a real issue.. the patch looks reasonable to me,
>>>> but I
>>>> wonder whether we should start to add some good comments in code to
>>>> reflect
>>>> that complexity starting from this one.  The MR refcount isn't easy to
>>>> understand to me.
>>>>
>>>> It also lets me start to wonder how MR refcount went through until
>>>> it looks
>>>> like today..  It's definitely not extremely intuitive to use mr->owner as
>>>> the object to do refcounting if mr itself does has its own QObject,
>>>> meanwhile it has other tricks around.
>>>>
>>>> E.g. the first thing I stumbled over when looking was the optimization
>>>> where we will avoid refcounting the mr when there's no owner, and IIUC it
>>>> was for the case when the "guest memory" (which will never be freed) used
>>>> to have no owner so we can speedup DMA if we know it won't go away.
>>>>
>>>> https://lore.kernel.org/qemu-devel/1450263601-2828-5-git-send-email-pbonzini@redhat.com/
>>>>
>>>> commit 612263cf33062f7441a5d0e3b37c65991fdc3210
>>>> Author: Paolo Bonzini <pbonzini@redhat.com>
>>>> Date:   Wed Dec 9 11:44:25 2015 +0100
>>>>
>>>>       memory: avoid unnecessary object_ref/unref
>>>>       For the common case of DMA into non-hotplugged RAM, it is
>>>> unnecessary
>>>>       but expensive to do object_ref/unref.  Add back an owner field to
>>>>       MemoryRegion, so that these memory regions can skip the reference
>>>>       counting.
>>>>
>>>> If so, it looks like it will stop working with memory-backends get
>>>> involved?  As I think those MRs will have owner set always, and I wonder
>>>> whether memory-backends should be the major way to specify guest
>>>> memory now
>>>> and in the future.  So I'm not sure how important that optimization is as
>>>> of now, and whether we could "simplify" it back to always do the refcount
>>>> if the major scenarios will not adopt it.
>>>>
>>>> The other issue is we used owner refcount from the start of
>>>> memory_region_ref() got introduced, since:
>>>>
>>>> commit 46637be269aaaceb9867ffdf176e906401138fff
>>>> Author: Paolo Bonzini <pbonzini@redhat.com>
>>>> Date:   Tue May 7 09:06:00 2013 +0200
>>>>
>>>>       memory: add ref/unref
>>>>
>>>> And we still have that in our document, even though I don't think
>>>> it's true
>>>> anymore:
>>>>
>>>>    * ...  MemoryRegions actually do not have their
>>>>    * own reference count; they piggyback on a QOM object, their "owner".
>>>>    * This function adds a reference to the owner.
>>>>
>>>> It looks like what happened is when introduced the change, MR is not
>>>> a QOM
>>>> object yet.  But it later is..
>>>>
>>>> I mentioned all these only because I found that _if_ we can keep mr
>>>> refcounting as simple as other objects:
>>>>
>>>> memory_region_ref(mr)
>>>> {
>>>>       object_ref(OBJECT(mr));
>>>> }
>>>>
>>>> Then looks like this "recursive refcount" problem can also go away.  I'm
>>>> curious whether you or anyone tried to explore that path, or whether
>>>> above
>>>> doesn't make sense at all.
>>>
>>> It unfortunately does not solve the problem.
>>>
>>> The underlying problem is that the whole device must be kept alive while
>>> its memory region are. Indeed MemoryRegions do have refcounts, but
>>> incrementing them do not extend the lifetime of the devices (i.e., the
>>> owners). The refcount of the owners must be incremented for correctness.
>>>
>>> Referencing a subregion MemoryRegion from its container MemoryRegion
>>> owned by the same device is an exceptional case. Incrementing the
>>> refcount of the owner extends the owner's lifetime to forever.
>>
>> Is it really an exceptional case?
>>
>> What I'm seeing are a lot of devices creating MR and never bother
>> to destroy them, so indeed owner (device) refcount never reaches 0.
>>
>> Most of the time when we create MR in .instance_init/.realize,
>> we neglect to implement the undo path (.instance_finalize or
>> .unrealize).
> 
> Right.  The cross-reference of MR <-> device makes sense from the concept
> level, but I'm not sure how much we rely on that from QEMU perspective
> currently.  It complicates refcount and looks like it needs some thoughts.
> 
> One proof is, when I replied I actually tested all qemu qtests with below
> patch and nothing yet explode:
> 
> ===8<===
> diff --git a/system/memory.c b/system/memory.c
> index 2d69521360..a1d2c1f808 100644
> --- a/system/memory.c
> +++ b/system/memory.c
> @@ -1801,26 +1801,12 @@ Object *memory_region_owner(MemoryRegion *mr)
>   
>   void memory_region_ref(MemoryRegion *mr)
>   {
> -    /* MMIO callbacks most likely will access data that belongs
> -     * to the owner, hence the need to ref/unref the owner whenever
> -     * the memory region is in use.
> -     *
> -     * The memory region is a child of its owner.  As long as the
> -     * owner doesn't call unparent itself on the memory region,
> -     * ref-ing the owner will also keep the memory region alive.
> -     * Memory regions without an owner are supposed to never go away;
> -     * we do not ref/unref them because it slows down DMA sensibly.
> -     */
> -    if (mr && mr->owner) {
> -        object_ref(mr->owner);
> -    }
> +    object_ref(OBJECT(mr));
>   }
>   
>   void memory_region_unref(MemoryRegion *mr)
>   {
> -    if (mr && mr->owner) {
> -        object_unref(mr->owner);
> -    }
> +    object_unref(OBJECT(mr));
>   }
> ===8<===
> 
> I'm not sure how much it covers dynamic destruction of devices, though,
> where the device creates internal MRs to the guest address space.  I'm
> actually wondering whether we have other barriers already to avoid having
> any MR ops being invoked if a device was removed.
> 
> So I think above indeed would remove the cross-ref we used to have. It
> might be a matter of whether that's fine for now to fix the cyclic ref
> issue alone, and even if we still want some cross-ref.. whether we should
> do it the old way.  I mean, the cross-ref can also be done in other forms,
> and it may not block MR has its own refcounts.

We don't even need reference-counting memory regions if we are sure 
there would be no reference to a MR when the device owning it is gone 
because a MR is already alive when the device owning it is.

Unfortunately that is not the case. If you grep for memory_region_ref(), 
you will find lot of places that asynchronously calls 
memory_region_unref(). Devices can be removed whenever the BQL is 
unlocked so we certainly need to count references.

> 
> Even if we want to have dev<->MR refcount, I wonder whether we should avoid
> having MR refcount the device, because it looks like "sub-object refs the
> parent" which sounds like a good source of cyclic refcount issue.  I wonder
> whether device should ref the MR instead (when MR is dynamically created; I
> am not sure whether we even need that if in Phil's comment where the MR
> object is a sub-struct-field of the Device object).  Then the device can
> provide a proper destroy() function to release the MRs under it, perhaps by
> disabling them first (then any ops should already fail after that), then
> unref the MRs with the hope that they're the last ref holder then MRs
> destroys there too.

I think it is better to remove the reference counter from Object and 
create a subclass like RefCounted to add one for sub-struct-field cases. 
There are exceptional cases in virtio-gpu-gl that dynamically allocate 
MRs but such a device can create Objects that functions as owners of 
dynamically-allocated MRs.

Such a change is independent and out of the scope of this change though. 
It also requires to convert all other usage of Object to RefCounted, 
which is not really trivial.

Regards,
Akihiko Odaki

Re: [PATCH v2 09/15] memory: Do not create circular reference with subregion

[Peter Maydell]

On Thu, 27 Jun 2024 at 14:40, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> 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 74cd73ebc78b..949f5016a68d 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();
>  }

I was having another look at leaks this week, and I tried
this patch to see how many of the leaks I was seeing it
fixed. I found however that for arm it results in an
assertion when the device-introspection-test exercises
the "imx7.analog" device. By-hand repro:

$ ./build/asan/qemu-system-aarch64 -display none -machine none -accel
qtest -monitor stdio
==712838==WARNING: ASan doesn't fully support makecontext/swapcontext
functions and may produce false positives in some cases!
QEMU 9.0.92 monitor - type 'help' for more information
(qemu) device_add imx7.analog,help
qemu-system-aarch64: ../../system/memory.c:1777: void
memory_region_finalize(Object *): Assertion `!mr->container' failed.
Aborted (core dumped)

It may be well be that this is a preexisting bug that's only
exposed by this refcount change causing us to actually try
to dispose of the memory regions.

I think that what's happening here is that the device
object has multiple MemoryRegions, each of which is a child
QOM property. One of these MRs is a "container MR", and the
other three are actual-content MRs which the device put into
the container when it created them. When we deref the device,
we go through all the child QOM properties unparenting and
unreffing them. However, there's no particular ordering
here, and it happens that we try to unref one of the
actual-content MRs first. That MR is still inside the
container MR, so we hit the assert. If we had happened to
unref the container MR first then memory_region_finalize()
would have removed all the subregions from it, avoiding
the problem.

I'm not sure what the best fix would be here -- that assert
is there as a guard that the region isn't visible in
any address space, so maybe it needs to be made a bit
cleverer about the condition it checks? e.g. in this
example although mr->container is not NULL,
mr->container->container is NULL. Or we could check
whether the mr->container->owner is the same as the
mr->owner and allow a non-NULL mr->container in that case.
I don't know this subsystem well enough so I'm just
making random stabs here, though.

thanks
-- PMM

Re: [PATCH v2 09/15] memory: Do not create circular reference with subregion

[Peter Xu]

On Thu, Aug 22, 2024 at 06:10:43PM +0100, Peter Maydell wrote:
> On Thu, 27 Jun 2024 at 14:40, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
> >
> > 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 74cd73ebc78b..949f5016a68d 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();
> >  }
> 
> I was having another look at leaks this week, and I tried
> this patch to see how many of the leaks I was seeing it
> fixed. I found however that for arm it results in an
> assertion when the device-introspection-test exercises
> the "imx7.analog" device. By-hand repro:
> 
> $ ./build/asan/qemu-system-aarch64 -display none -machine none -accel
> qtest -monitor stdio
> ==712838==WARNING: ASan doesn't fully support makecontext/swapcontext
> functions and may produce false positives in some cases!
> QEMU 9.0.92 monitor - type 'help' for more information
> (qemu) device_add imx7.analog,help
> qemu-system-aarch64: ../../system/memory.c:1777: void
> memory_region_finalize(Object *): Assertion `!mr->container' failed.
> Aborted (core dumped)
> 
> It may be well be that this is a preexisting bug that's only
> exposed by this refcount change causing us to actually try
> to dispose of the memory regions.
> 
> I think that what's happening here is that the device
> object has multiple MemoryRegions, each of which is a child
> QOM property. One of these MRs is a "container MR", and the
> other three are actual-content MRs which the device put into
> the container when it created them. When we deref the device,
> we go through all the child QOM properties unparenting and
> unreffing them. However, there's no particular ordering
> here, and it happens that we try to unref one of the
> actual-content MRs first. That MR is still inside the
> container MR, so we hit the assert. If we had happened to
> unref the container MR first then memory_region_finalize()
> would have removed all the subregions from it, avoiding
> the problem.
> 
> I'm not sure what the best fix would be here -- that assert
> is there as a guard that the region isn't visible in
> any address space, so maybe it needs to be made a bit
> cleverer about the condition it checks? e.g. in this
> example although mr->container is not NULL,
> mr->container->container is NULL.

If we keep looking at ->container we'll always see NULL, IIUC, because
either it's removed from its parent MR so it's NULL already, or at some
point it can start to point to a root mr of an address space, where should
also be NULL, afaiu.

> Or we could check whether the mr->container->owner is the same as the
> mr->owner and allow a non-NULL mr->container in that case.  I don't know
> this subsystem well enough so I'm just making random stabs here, though.

If with the assumption of this patch applied, then looks like it's pretty
legal a container MR and the child MRs be finalized in any order when the
owner device is being destroyed.

IIUC the MR should be destined to be invisible until this point, with or
without the fact that mr->container is NULL.  It's because anyone who
references the MR should do memory_region_ref() first, which takes the
owner's refcount.  Here if MR finalize() is reached I think it means the
owner refcount must be zero.  So it looks to me the only possible case when
mr->container is non-NULL is it's used internally like this.  Then it's
invisible and also safe to be detached even if container != NULL.

So.. I wonder whether below would make sense, on top of this existing
patch.

===8<===
diff --git a/system/memory.c b/system/memory.c
index 1c00df8305..54a9d9e5f9 100644
--- a/system/memory.c
+++ b/system/memory.c
@@ -1771,16 +1771,23 @@ static void memory_region_finalize(Object *obj)
 {
     MemoryRegion *mr = MEMORY_REGION(obj);
 
-    assert(!mr->container);
-
-    /* We know the region is not visible in any address space (it
-     * does not have a container and cannot be a root either because
-     * it has no references, so we can blindly clear mr->enabled.
-     * memory_region_set_enabled instead could trigger a transaction
-     * and cause an infinite loop.
+    /*
+     * We know the region is not visible in any address space, because
+     * normally MR's finalize() should be invoked by finalize() of the
+     * owner, which will remove all the properties including the MRs, and
+     * that can only happen when prior memory_region_ref() of the MR (which
+     * will ultimately take the owner's refcount) from elsewhere got
+     * properly released.
+     *
+     * So we can blindly clear mr->enabled, unlink both the upper container
+     * or all subregions.  memory_region_set_enabled() won't work instead,
+     * as it could trigger a transaction and cause an infinite loop.
      */
     mr->enabled = false;
     memory_region_transaction_begin();
+    if (mr->container) {
+        memory_region_del_subregion(mr->container, mr);
+    }
     while (!QTAILQ_EMPTY(&mr->subregions)) {
         MemoryRegion *subregion = QTAILQ_FIRST(&mr->subregions);
===8<===

Thanks,

-- 
Peter Xu

Re: [PATCH v2 09/15] memory: Do not create circular reference with subregion

[Akihiko Odaki]

On 2024/08/23 6:01, Peter Xu wrote:
> On Thu, Aug 22, 2024 at 06:10:43PM +0100, Peter Maydell wrote:
>> On Thu, 27 Jun 2024 at 14:40, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>>
>>> 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 74cd73ebc78b..949f5016a68d 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();
>>>   }
>>
>> I was having another look at leaks this week, and I tried
>> this patch to see how many of the leaks I was seeing it
>> fixed. I found however that for arm it results in an
>> assertion when the device-introspection-test exercises
>> the "imx7.analog" device. By-hand repro:
>>
>> $ ./build/asan/qemu-system-aarch64 -display none -machine none -accel
>> qtest -monitor stdio
>> ==712838==WARNING: ASan doesn't fully support makecontext/swapcontext
>> functions and may produce false positives in some cases!
>> QEMU 9.0.92 monitor - type 'help' for more information
>> (qemu) device_add imx7.analog,help
>> qemu-system-aarch64: ../../system/memory.c:1777: void
>> memory_region_finalize(Object *): Assertion `!mr->container' failed.
>> Aborted (core dumped)
>>
>> It may be well be that this is a preexisting bug that's only
>> exposed by this refcount change causing us to actually try
>> to dispose of the memory regions.
>>
>> I think that what's happening here is that the device
>> object has multiple MemoryRegions, each of which is a child
>> QOM property. One of these MRs is a "container MR", and the
>> other three are actual-content MRs which the device put into
>> the container when it created them. When we deref the device,
>> we go through all the child QOM properties unparenting and
>> unreffing them. However, there's no particular ordering
>> here, and it happens that we try to unref one of the
>> actual-content MRs first. That MR is still inside the
>> container MR, so we hit the assert. If we had happened to
>> unref the container MR first then memory_region_finalize()
>> would have removed all the subregions from it, avoiding
>> the problem.
>>
>> I'm not sure what the best fix would be here -- that assert
>> is there as a guard that the region isn't visible in
>> any address space, so maybe it needs to be made a bit
>> cleverer about the condition it checks? e.g. in this
>> example although mr->container is not NULL,
>> mr->container->container is NULL.
> 
> If we keep looking at ->container we'll always see NULL, IIUC, because
> either it's removed from its parent MR so it's NULL already, or at some
> point it can start to point to a root mr of an address space, where should
> also be NULL, afaiu.
> 
>> Or we could check whether the mr->container->owner is the same as the
>> mr->owner and allow a non-NULL mr->container in that case.  I don't know
>> this subsystem well enough so I'm just making random stabs here, though.
> 
> If with the assumption of this patch applied, then looks like it's pretty
> legal a container MR and the child MRs be finalized in any order when the
> owner device is being destroyed.
> 
> IIUC the MR should be destined to be invisible until this point, with or
> without the fact that mr->container is NULL.  It's because anyone who
> references the MR should do memory_region_ref() first, which takes the
> owner's refcount.  Here if MR finalize() is reached I think it means the
> owner refcount must be zero.  So it looks to me the only possible case when
> mr->container is non-NULL is it's used internally like this.  Then it's
> invisible and also safe to be detached even if container != NULL.

It is still nice if we can protect internal uses; it is theoretically 
possible for the owner to remove the subregion at runtime, and we want 
to ensure the subregion will be kept alive while its container is even 
in such a case. We can do so by creating a reference to the subregion 
itself instead of its owner. I sent v4 for this change.

Regards,
Akihiko Odaki

[PATCH v2 10/15] 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

Re: [PATCH v2 10/15] tests/qtest: Use qtest_add_data_func_full()

[Thomas Huth]

On 27/06/2024 15.37, Akihiko Odaki wrote:
> 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(-)

Reviewed-by: Thomas Huth <thuth@redhat.com>

[PATCH v2 11/15] tests/qtest: Free unused QMP response

[Akihiko Odaki]

This fixes 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 18e2f7f28278..f89da7b80797 100644
--- a/tests/qtest/libqtest.c
+++ b/tests/qtest/libqtest.c
@@ -744,6 +744,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 v2 12/15] tests/qtest: Free old machine variable name

[Akihiko Odaki]

This fixes 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 f89da7b80797..1605c0c9f615 100644
--- a/tests/qtest/libqtest.c
+++ b/tests/qtest/libqtest.c
@@ -1509,6 +1509,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

Re: [PATCH v2 12/15] tests/qtest: Free old machine variable name

[Peter Maydell]

On Thu, 27 Jun 2024 at 14:40, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> This fixes 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 f89da7b80797..1605c0c9f615 100644
> --- a/tests/qtest/libqtest.c
> +++ b/tests/qtest/libqtest.c
> @@ -1509,6 +1509,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

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[PATCH v2 13/15] 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

Re: [PATCH v2 13/15] tests/qtest: Delete previous boot file

[Thomas Huth]

On 27/06/2024 15.37, Akihiko Odaki wrote:
> 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
> 

I think the better fix would be to call bootfile_create() only once from 
main() since we don't have to create the bootfile multiple times, do we?

  Thomas

Re: [PATCH v2 13/15] tests/qtest: Delete previous boot file

[Akihiko Odaki]

On 2024/07/02 16:31, Thomas Huth wrote:
> On 27/06/2024 15.37, Akihiko Odaki wrote:
>> 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
>>
> 
> I think the better fix would be to call bootfile_create() only once from 
> main() since we don't have to create the bootfile multiple times, do we?

The suspend_me parameter depends on test cases so probably we actually 
need to recreate in such cases.

Regards,
Akihiko Odaki

[PATCH v2 14/15] tests/qtest: Free paths

[Akihiko Odaki]

This fixes 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 v2 15/15] 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 v2 15/15] tests/qtest: Free GThread

[Peter Maydell]

On Thu, 27 Jun 2024 at 14:41, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> These GThreads are never referenced.
>
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH v2 00/15] Fix check-qtest-ppc64 sanitizer errors

[Michael S. Tsirkin]

On Thu, Jun 27, 2024 at 10:37:43PM +0900, Akihiko Odaki wrote:
> Based-on: <3ad18bc590ef28e1526e8053568086b453e7ffde.1718211878.git.quic_mathbern@quicinc.com>
> ("[PATCH] cpu: fix memleak of 'halt_cond' and 'thread'")
> 
> 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>


Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

who's merging all this?

> ---
> Changes in v2:
> - Rebased to "[PATCH] cpu: fix memleak of 'halt_cond' and 'thread'".
>   (Philippe Mathieu-Daudé)
> - Converted IRQs into GPIO lines and removed one qemu_irq usage.
>   (Peter Maydell)
> - s/suppresses/fixes/ (Michael S. Tsirkin)
> - Corrected title of patch "hw/virtio: Free vqs after vhost_dev_cleanup()"
>   (was "hw/virtio: Free vqs before vhost_dev_cleanup()")
> - Link to v1: https://lore.kernel.org/r/20240626-san-v1-0-f3cc42302189@daynix.com
> 
> ---
> Akihiko Odaki (15):
>       cpu: Free cpu_ases
>       hw/ide: Convert macio ide_irq into GPIO line
>       hw/ide: Remove internal DMA qemu_irq
>       hw/isa/vt82c686: Define a GPIO line between vt82c686 and i8259
>       spapr: Free stdout path
>       ppc/vof: Fix unaligned FDT property access
>       hw/virtio: Free vqs after 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
> 
>  include/hw/ppc/mac_dbdma.h           |  5 +++--
>  hw/core/cpu-common.c                 |  1 +
>  hw/ide/macio.c                       | 18 +++++++++++++-----
>  hw/isa/vt82c686.c                    |  7 ++++---
>  hw/misc/macio/mac_dbdma.c            | 10 +++++-----
>  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 +++---
>  15 files changed, 73 insertions(+), 37 deletions(-)
> ---
> base-commit: af799a2337c3e39994411f90631905d809a41da4
> change-id: 20240625-san-097afaf4f1c2
> 
> Best regards,
> -- 
> Akihiko Odaki <akihiko.odaki@daynix.com>

Re: [PATCH v2 00/15] Fix check-qtest-ppc64 sanitizer errors

[BALATON Zoltan]

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

On Mon, 1 Jul 2024, Michael S. Tsirkin wrote:
> On Thu, Jun 27, 2024 at 10:37:43PM +0900, Akihiko Odaki wrote:
>> Based-on: <3ad18bc590ef28e1526e8053568086b453e7ffde.1718211878.git.quic_mathbern@quicinc.com>
>> ("[PATCH] cpu: fix memleak of 'halt_cond' and 'thread'")
>>
>> 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>
>
>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>
> who's merging all this?

Maybe needs to be split. Mark had an alternative for macio which was 
picked up by Philippe if I'm not mistaken. I've sent an alternative for 
vt82c686 which is still discussed but could belong to Philippe as well. 
PPC parts could be taken by the PPC maintainers if there were any active 
at the moment and I don't know who maintains tests normally or other misc 
areas.

Regards,
BALATON Zoltan

>> ---
>> Changes in v2:
>> - Rebased to "[PATCH] cpu: fix memleak of 'halt_cond' and 'thread'".
>>   (Philippe Mathieu-Daudé)
>> - Converted IRQs into GPIO lines and removed one qemu_irq usage.
>>   (Peter Maydell)
>> - s/suppresses/fixes/ (Michael S. Tsirkin)
>> - Corrected title of patch "hw/virtio: Free vqs after vhost_dev_cleanup()"
>>   (was "hw/virtio: Free vqs before vhost_dev_cleanup()")
>> - Link to v1: https://lore.kernel.org/r/20240626-san-v1-0-f3cc42302189@daynix.com
>>
>> ---
>> Akihiko Odaki (15):
>>       cpu: Free cpu_ases
>>       hw/ide: Convert macio ide_irq into GPIO line
>>       hw/ide: Remove internal DMA qemu_irq
>>       hw/isa/vt82c686: Define a GPIO line between vt82c686 and i8259
>>       spapr: Free stdout path
>>       ppc/vof: Fix unaligned FDT property access
>>       hw/virtio: Free vqs after 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
>>
>>  include/hw/ppc/mac_dbdma.h           |  5 +++--
>>  hw/core/cpu-common.c                 |  1 +
>>  hw/ide/macio.c                       | 18 +++++++++++++-----
>>  hw/isa/vt82c686.c                    |  7 ++++---
>>  hw/misc/macio/mac_dbdma.c            | 10 +++++-----
>>  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 +++---
>>  15 files changed, 73 insertions(+), 37 deletions(-)
>> ---
>> base-commit: af799a2337c3e39994411f90631905d809a41da4
>> change-id: 20240625-san-097afaf4f1c2
>>
>> Best regards,
>> --
>> Akihiko Odaki <akihiko.odaki@daynix.com>
>
>
>

Re: [PATCH v2 00/15] Fix check-qtest-ppc64 sanitizer errors

[Thomas Huth]

On 02/07/2024 00.23, BALATON Zoltan wrote:
> On Mon, 1 Jul 2024, Michael S. Tsirkin wrote:
>> On Thu, Jun 27, 2024 at 10:37:43PM +0900, Akihiko Odaki wrote:
>>> Based-on: 
>>> <3ad18bc590ef28e1526e8053568086b453e7ffde.1718211878.git.quic_mathbern@quicinc.com>
>>> ("[PATCH] cpu: fix memleak of 'halt_cond' and 'thread'")
>>>
>>> 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>
>>
>>
>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>>
>> who's merging all this?
> 
> Maybe needs to be split. Mark had an alternative for macio which was picked 
> up by Philippe if I'm not mistaken. I've sent an alternative for vt82c686 
> which is still discussed but could belong to Philippe as well. PPC parts 
> could be taken by the PPC maintainers if there were any active at the moment 
> and I don't know who maintains tests normally or other misc areas.

I can take the qtest patches through my tree.

  Thomas

Re: [PATCH v2 00/15] Fix check-qtest-ppc64 sanitizer errors

[Nicholas Piggin]

On Tue Jul 2, 2024 at 4:23 PM AEST, Thomas Huth wrote:
> On 02/07/2024 00.23, BALATON Zoltan wrote:
> > On Mon, 1 Jul 2024, Michael S. Tsirkin wrote:
> >> On Thu, Jun 27, 2024 at 10:37:43PM +0900, Akihiko Odaki wrote:
> >>> Based-on: 
> >>> <3ad18bc590ef28e1526e8053568086b453e7ffde.1718211878.git.quic_mathbern@quicinc.com>
> >>> ("[PATCH] cpu: fix memleak of 'halt_cond' and 'thread'")
> >>>
> >>> 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>
> >>
> >>
> >> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> >>
> >> who's merging all this?
> > 
> > Maybe needs to be split. Mark had an alternative for macio which was picked 
> > up by Philippe if I'm not mistaken. I've sent an alternative for vt82c686 
> > which is still discussed but could belong to Philippe as well. PPC parts 
> > could be taken by the PPC maintainers if there were any active at the moment 
> > and I don't know who maintains tests normally or other misc areas.
>
> I can take the qtest patches through my tree.
>       cpu: Free cpu_ases
>       migration: Free removed SaveStateEntry
>       memory: Do not create circular reference with subregion
>       hw/virtio: Free vqs after vhost_dev_cleanup()

These are all core code, maybe Philippe, Peter, or Richard,
Fabiano for migration perhaps?

>       hw/ide: Convert macio ide_irq into GPIO line
>       hw/ide: Remove internal DMA qemu_irq
>       hw/isa/vt82c686: Define a GPIO line between vt82c686 and i8259

I guess I'll let you wrangle these and go through Philippe then?

>       spapr: Free stdout path
>       ppc/vof: Fix unaligned FDT property access

These ppc ones I can take, they seem okay. Sorry for my recent
inactivity.

Thanks,
Nick

>       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