[PATCH 0/4] Some more pegasos patches

[PATCH 0/4] Some more pegasos patches

[BALATON Zoltan]

Some more short patches to finish the already merged pegasos clean up
series.

BALATON Zoltan (4):
  hw/ppc/pegasos2: Add /chosen/stdin node with VOF
  hw/pci-host/articia: Map PCI memory windows in realize
  hw/ppc/pegasos2: Rename to pegasos
  hw/ppc/pegasos: Update documentation for pegasos1

 MAINTAINERS                             |  4 ++--
 configs/devices/ppc-softmmu/default.mak |  7 +++---
 docs/system/ppc/amigang.rst             | 29 +++++++++++++++----------
 hw/pci-host/articia.c                   | 15 ++++++++++++-
 hw/ppc/Kconfig                          |  2 +-
 hw/ppc/amigaone.c                       | 28 +++++-------------------
 hw/ppc/meson.build                      |  4 ++--
 hw/ppc/{pegasos2.c => pegasos.c}        | 14 +-----------
 8 files changed, 45 insertions(+), 58 deletions(-)
 rename hw/ppc/{pegasos2.c => pegasos.c} (98%)

-- 
2.41.3

[PATCH 1/4] hw/ppc/pegasos2: Add /chosen/stdin node with VOF

[BALATON Zoltan]

Some very old Linux kernels fail to start if /chosen/stdin is not
found so add it to the device tree when using VOF.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/ppc/pegasos2.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/ppc/pegasos2.c b/hw/ppc/pegasos2.c
index 93696ed381..21299dde3c 100644
--- a/hw/ppc/pegasos2.c
+++ b/hw/ppc/pegasos2.c
@@ -565,6 +565,7 @@ static void pegasos_machine_reset(MachineState *machine, ResetType type)
     qemu_fdt_setprop(fdt, "/chosen", "qemu,boot-kernel", d, sizeof(d));
 
     vof_build_dt(fdt, pm->vof);
+    vof_client_open_store(fdt, pm->vof, "/chosen", "stdin", "/failsafe");
     vof_client_open_store(fdt, pm->vof, "/chosen", "stdout", "/failsafe");
 
     /* Set machine->fdt for 'dumpdtb' QMP/HMP command */
-- 
2.41.3

[PATCH 2/4] hw/pci-host/articia: Map PCI memory windows in realize

[BALATON Zoltan]

These memory windows are a result of the address decoding in the
Articia S north bridge so better model it there and not in board code.

Suggested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/pci-host/articia.c | 15 ++++++++++++++-
 hw/ppc/amigaone.c     | 28 +++++-----------------------
 hw/ppc/pegasos2.c     | 13 -------------
 3 files changed, 19 insertions(+), 37 deletions(-)

diff --git a/hw/pci-host/articia.c b/hw/pci-host/articia.c
index cc65aac2a8..761e89bc8f 100644
--- a/hw/pci-host/articia.c
+++ b/hw/pci-host/articia.c
@@ -22,6 +22,11 @@
  * Most features are missing but those are not needed by firmware and guests.
  */
 
+#define PCI_HIGH_ADDR 0x80000000
+#define PCI_HIGH_SIZE 0x7d000000
+#define PCI_LOW_ADDR  0xfd000000
+#define PCI_LOW_SIZE   0x1000000
+
 OBJECT_DECLARE_SIMPLE_TYPE(ArticiaState, ARTICIA)
 
 OBJECT_DECLARE_SIMPLE_TYPE(ArticiaHostState, ARTICIA_PCI_HOST)
@@ -169,6 +174,7 @@ static void articia_realize(DeviceState *dev, Error **errp)
 {
     ArticiaState *s = ARTICIA(dev);
     PCIHostState *h = PCI_HOST_BRIDGE(dev);
+    MemoryRegion *mr;
     PCIDevice *pdev;
 
     bitbang_i2c_init(&s->smbus, i2c_init_bus(dev, "smbus"));
@@ -180,6 +186,14 @@ static void articia_realize(DeviceState *dev, Error **errp)
     memory_region_init_io(&s->reg, OBJECT(s), &articia_reg_ops, s,
                           TYPE_ARTICIA, 0x1000000);
     memory_region_add_subregion_overlap(&s->reg, 0, &s->io, 1);
+    mr = g_new(MemoryRegion, 1);
+    memory_region_init_alias(mr, OBJECT(dev), "pci-mem-low", &s->mem,
+                             0, PCI_LOW_SIZE);
+    memory_region_add_subregion(get_system_memory(), PCI_LOW_ADDR, mr);
+    mr = g_new(MemoryRegion, 1);
+    memory_region_init_alias(mr, OBJECT(dev), "pci-mem-high", &s->mem,
+                             PCI_HIGH_ADDR, PCI_HIGH_SIZE);
+    memory_region_add_subregion(get_system_memory(), PCI_HIGH_ADDR, mr);
 
     /* devfn_min is 8 that matches first PCI slot in AmigaOne */
     h->bus = pci_register_root_bus(dev, NULL, articia_pcihost_set_irq,
@@ -191,7 +205,6 @@ static void articia_realize(DeviceState *dev, Error **errp)
     pci_create_simple(h->bus, PCI_DEVFN(0, 1), TYPE_ARTICIA_PCI_BRIDGE);
 
     sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->reg);
-    sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->mem);
     qdev_init_gpio_out(dev, s->irq, ARRAY_SIZE(s->irq));
 }
 
diff --git a/hw/ppc/amigaone.c b/hw/ppc/amigaone.c
index 47fb016b4a..ddfb5c9ec5 100644
--- a/hw/ppc/amigaone.c
+++ b/hw/ppc/amigaone.c
@@ -34,13 +34,6 @@
 #define INITRD_MIN_ADDR 0x600000
 #define INIT_RAM_ADDR 0x40000000
 
-#define PCI_HIGH_ADDR 0x80000000
-#define PCI_HIGH_SIZE 0x7d000000
-#define PCI_LOW_ADDR  0xfd000000
-#define PCI_LOW_SIZE  0xe0000
-
-#define ARTICIA_ADDR 0xfe000000
-
 /*
  * Firmware binary available at
  * https://www.hyperion-entertainment.com/index.php/downloads?view=files&parent=28
@@ -266,7 +259,7 @@ static void amigaone_init(MachineState *machine)
 {
     PowerPCCPU *cpu;
     CPUPPCState *env;
-    MemoryRegion *rom, *pci_mem, *mr;
+    MemoryRegion *rom, *mr;
     ssize_t sz;
     PCIBus *pci_bus;
     Object *via;
@@ -307,8 +300,8 @@ static void amigaone_init(MachineState *machine)
         qdev_prop_set_drive(dev, "drive", blk_by_legacy_dinfo(di));
     }
     sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
-    memory_region_add_subregion(get_system_memory(), NVRAM_ADDR,
-                                sysbus_mmio_get_region(SYS_BUS_DEVICE(dev), 0));
+    mr = sysbus_mmio_get_region(SYS_BUS_DEVICE(dev), 0);
+    memory_region_add_subregion_overlap(get_system_memory(), NVRAM_ADDR, mr, 1);
 
     /* allocate and load firmware */
     rom = g_new(MemoryRegion, 1);
@@ -332,8 +325,8 @@ static void amigaone_init(MachineState *machine)
     }
 
     /* Articia S */
-    dev = sysbus_create_simple(TYPE_ARTICIA, ARTICIA_ADDR, NULL);
-
+    dev = sysbus_create_simple(TYPE_ARTICIA, 0xfe000000, NULL);
+    pci_bus = PCI_BUS(qdev_get_child_bus(dev, "pci.0"));
     i2c_bus = I2C_BUS(qdev_get_child_bus(dev, "smbus"));
     if (machine->ram_size > 512 * MiB) {
         spd_data = spd_data_generate(SDR, machine->ram_size / 2);
@@ -346,17 +339,6 @@ static void amigaone_init(MachineState *machine)
         smbus_eeprom_init_one(i2c_bus, 0x52, spd_data);
     }
 
-    pci_mem = sysbus_mmio_get_region(SYS_BUS_DEVICE(dev), 1);
-    mr = g_new(MemoryRegion, 1);
-    memory_region_init_alias(mr, OBJECT(dev), "pci-mem-low", pci_mem,
-                             0, PCI_LOW_SIZE);
-    memory_region_add_subregion(get_system_memory(), PCI_LOW_ADDR, mr);
-    mr = g_new(MemoryRegion, 1);
-    memory_region_init_alias(mr, OBJECT(dev), "pci-mem-high", pci_mem,
-                             PCI_HIGH_ADDR, PCI_HIGH_SIZE);
-    memory_region_add_subregion(get_system_memory(), PCI_HIGH_ADDR, mr);
-    pci_bus = PCI_BUS(qdev_get_child_bus(dev, "pci.0"));
-
     /* VIA VT82c686B South Bridge (multifunction PCI device) */
     via = OBJECT(pci_create_simple_multifunction(pci_bus, PCI_DEVFN(7, 0),
                                                  TYPE_VT82C686B_ISA));
diff --git a/hw/ppc/pegasos2.c b/hw/ppc/pegasos2.c
index 21299dde3c..c89102cfdc 100644
--- a/hw/ppc/pegasos2.c
+++ b/hw/ppc/pegasos2.c
@@ -211,23 +211,10 @@ static void pegasos_init(MachineState *machine)
     /* north bridge */
     switch (pm->type) {
     case PEGASOS1:
-    {
-        MemoryRegion *pci_mem, *mr;
-
         /* Articia S */
         pm->nb = DEVICE(sysbus_create_simple(TYPE_ARTICIA, 0xfe000000, NULL));
-        pci_mem = sysbus_mmio_get_region(SYS_BUS_DEVICE(pm->nb), 1);
-        mr = g_new(MemoryRegion, 1);
-        memory_region_init_alias(mr, OBJECT(pm->nb), "pci-mem-low", pci_mem,
-                                 0, 0x1000000);
-        memory_region_add_subregion(get_system_memory(), 0xfd000000, mr);
-        mr = g_new(MemoryRegion, 1);
-        memory_region_init_alias(mr, OBJECT(pm->nb), "pci-mem-high", pci_mem,
-                                 0x80000000, 0x7d000000);
-        memory_region_add_subregion(get_system_memory(), 0x80000000, mr);
         pci_bus = PCI_BUS(qdev_get_child_bus(pm->nb, "pci.0"));
         break;
-    }
     case PEGASOS2:
         /* Marvell Discovery II system controller */
         pm->nb = DEVICE(sysbus_create_simple(TYPE_MV64361, -1,
-- 
2.41.3

[PATCH 3/4] hw/ppc/pegasos2: Rename to pegasos

[BALATON Zoltan]

Now that we also emulate pegasos1 it is not only about pegasos2 so
rename to a more generic name encompassing both.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 MAINTAINERS                             | 4 ++--
 configs/devices/ppc-softmmu/default.mak | 7 +++----
 hw/ppc/Kconfig                          | 2 +-
 hw/ppc/meson.build                      | 4 ++--
 hw/ppc/{pegasos2.c => pegasos.c}        | 0
 5 files changed, 8 insertions(+), 9 deletions(-)
 rename hw/ppc/{pegasos2.c => pegasos.c} (100%)

diff --git a/MAINTAINERS b/MAINTAINERS
index f33f95ceea..c85b79ad2d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1646,11 +1646,11 @@ F: roms/u-boot-sam460ex
 F: docs/system/ppc/amigang.rst
 F: tests/functional/ppc/test_sam460ex.py
 
-pegasos2
+pegasos
 M: BALATON Zoltan <balaton@eik.bme.hu>
 L: qemu-ppc@nongnu.org
 S: Maintained
-F: hw/ppc/pegasos2.c
+F: hw/ppc/pegasos.c
 F: hw/pci-host/mv64361.c
 F: hw/pci-host/mv643xx.h
 F: include/hw/pci-host/mv64361.h
diff --git a/configs/devices/ppc-softmmu/default.mak b/configs/devices/ppc-softmmu/default.mak
index 460d15e676..180ae31e2d 100644
--- a/configs/devices/ppc-softmmu/default.mak
+++ b/configs/devices/ppc-softmmu/default.mak
@@ -13,15 +13,14 @@
 # CONFIG_PPC440=n
 # CONFIG_VIRTEX=n
 
-# For Sam460ex
+# AmigaNG
+# CONFIG_AMIGAONE=n
+# CONFIG_PEGASOS=n
 # CONFIG_SAM460EX=n
 
 # For Macs
 # CONFIG_MAC_OLDWORLD=n
 # CONFIG_MAC_NEWWORLD=n
 
-# CONFIG_AMIGAONE=n
-# CONFIG_PEGASOS2=n
-
 # For PReP
 # CONFIG_PREP=n
diff --git a/hw/ppc/Kconfig b/hw/ppc/Kconfig
index 7091d72fd8..347dcce690 100644
--- a/hw/ppc/Kconfig
+++ b/hw/ppc/Kconfig
@@ -92,7 +92,7 @@ config AMIGAONE
     select VT82C686
     select SMBUS_EEPROM
 
-config PEGASOS2
+config PEGASOS
     bool
     default y
     depends on PPC
diff --git a/hw/ppc/meson.build b/hw/ppc/meson.build
index 6b7c1f4f49..f7dac87a2a 100644
--- a/hw/ppc/meson.build
+++ b/hw/ppc/meson.build
@@ -87,8 +87,8 @@ ppc_ss.add(when: 'CONFIG_E500', if_true: files(
 ppc_ss.add(when: 'CONFIG_VIRTEX', if_true: files('virtex_ml507.c'))
 # AmigaOne
 ppc_ss.add(when: 'CONFIG_AMIGAONE', if_true: files('amigaone.c'))
-# Pegasos2
-ppc_ss.add(when: 'CONFIG_PEGASOS2', if_true: files('pegasos2.c'))
+# Pegasos
+ppc_ss.add(when: 'CONFIG_PEGASOS', if_true: files('pegasos.c'))
 
 ppc_ss.add(when: 'CONFIG_VOF', if_true: files('vof.c'))
 ppc_ss.add(when: ['CONFIG_VOF', 'CONFIG_PSERIES'], if_true: files('spapr_vof.c'))
diff --git a/hw/ppc/pegasos2.c b/hw/ppc/pegasos.c
similarity index 100%
rename from hw/ppc/pegasos2.c
rename to hw/ppc/pegasos.c
-- 
2.41.3

[PATCH 4/4] hw/ppc/pegasos: Update documentation for pegasos1

[BALATON Zoltan]

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 docs/system/ppc/amigang.rst | 29 +++++++++++++++++------------
 1 file changed, 17 insertions(+), 12 deletions(-)

diff --git a/docs/system/ppc/amigang.rst b/docs/system/ppc/amigang.rst
index 21bb14ed09..e290412369 100644
--- a/docs/system/ppc/amigang.rst
+++ b/docs/system/ppc/amigang.rst
@@ -1,6 +1,6 @@
-=========================================================
-AmigaNG boards (``amigaone``, ``pegasos2``, ``sam460ex``)
-=========================================================
+=======================================================================
+AmigaNG boards (``amigaone``, ``pegasos1``, ``pegasos2``, ``sam460ex``)
+=======================================================================
 
 These PowerPC machines emulate boards that are primarily used for
 running Amiga like OSes (AmigaOS 4, MorphOS and AROS) but these can
@@ -64,18 +64,23 @@ eventually it boots and the installer becomes visible. The ``ati-vga`` RV100
 emulation is not complete yet so only frame buffer works, DRM and 3D is not
 available.
 
-Genesi/bPlan Pegasos II (``pegasos2``)
-======================================
+Genesi/bPlan Pegasos (``pegasos1``, ``pegasos2``)
+=================================================
 
-The ``pegasos2`` machine emulates the Pegasos II sold by Genesi and
-designed by bPlan. Its schematics are available at
-https://www.powerdeveloper.org/platforms/pegasos/schematics.
+The ``pegasos1`` machine emulates the original Pegasos (later marked I) sold by
+Genesi and designed by bPlan. It uses the same Articia S north bridge as the
+``amigaone`` machine, otherwise it is mostly the same as the later Pegasos II.
+
+The ``pegasos2`` machine emulates the Pegasos II which is a redesigned version
+of Pegasos I to fix problems with its north bridge. Its schematics are available
+at https://www.powerdeveloper.org/platforms/pegasos/schematics.
 
 Emulated devices
 ----------------
 
  * PowerPC 7457 CPU (can also use ``-cpu g3`` or ``750cxe``)
- * Marvell MV64361 Discovery II north bridge
+ * Articia S north bridge (for ``pegasos1``)
+ * Marvell MV64361 Discovery II north bridge (for ``pegasos2``)
  * VIA VT8231 south bridge
  * PCI VGA compatible card (guests may need other card instead)
  * PS/2 keyboard and mouse
@@ -83,9 +88,9 @@ Emulated devices
 Firmware
 --------
 
-The Pegasos II board has an Open Firmware compliant ROM based on
+The Pegasos boards have an Open Firmware compliant ROM based on
 SmartFirmware with some changes that are not open-sourced therefore
-the ROM binary cannot be included in QEMU. An updater was available
+the ROM binary cannot be included in QEMU. A Pegasos II updater was available
 from bPlan, it can be found in the `Internet Archive
 <http://web.archive.org/web/20071021223056/http://www.bplan-gmbh.de/up050404/up050404>`_.
 The ROM image can be extracted from it with the following command:
@@ -111,7 +116,7 @@ At the firmware ``ok`` prompt enter ``boot cd install/pegasos``.
 
 Alternatively, it is possible to boot the kernel directly without
 firmware ROM using the QEMU built-in minimal Virtual Open Firmware
-(VOF) emulation which is also supported on ``pegasos2``. For this,
+(VOF) emulation which is also supported on ``pegasos1`` and ``pegasos2``. For this,
 extract the kernel ``install/powerpc/vmlinuz-chrp.initrd`` from the CD
 image, then run:
 
-- 
2.41.3

Re: [PATCH 2/4] hw/pci-host/articia: Map PCI memory windows in realize

[Philippe Mathieu-Daudé]

On 25/10/25 01:31, BALATON Zoltan wrote:
> These memory windows are a result of the address decoding in the
> Articia S north bridge so better model it there and not in board code.
> 
> Suggested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>   hw/pci-host/articia.c | 15 ++++++++++++++-
>   hw/ppc/amigaone.c     | 28 +++++-----------------------
>   hw/ppc/pegasos2.c     | 13 -------------
>   3 files changed, 19 insertions(+), 37 deletions(-)


> @@ -169,6 +174,7 @@ static void articia_realize(DeviceState *dev, Error **errp)
>   {
>       ArticiaState *s = ARTICIA(dev);
>       PCIHostState *h = PCI_HOST_BRIDGE(dev);
> +    MemoryRegion *mr;
>       PCIDevice *pdev;
>   
>       bitbang_i2c_init(&s->smbus, i2c_init_bus(dev, "smbus"));
> @@ -180,6 +186,14 @@ static void articia_realize(DeviceState *dev, Error **errp)
>       memory_region_init_io(&s->reg, OBJECT(s), &articia_reg_ops, s,
>                             TYPE_ARTICIA, 0x1000000);
>       memory_region_add_subregion_overlap(&s->reg, 0, &s->io, 1);
> +    mr = g_new(MemoryRegion, 1);

Won't Coverity or other analysis tools complain about the leak?
(this is why we usually keep a reference in the device state, here
ArticiaState). Otherwise:

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

> +    memory_region_init_alias(mr, OBJECT(dev), "pci-mem-low", &s->mem,
> +                             0, PCI_LOW_SIZE);
> +    memory_region_add_subregion(get_system_memory(), PCI_LOW_ADDR, mr);
> +    mr = g_new(MemoryRegion, 1);
> +    memory_region_init_alias(mr, OBJECT(dev), "pci-mem-high", &s->mem,
> +                             PCI_HIGH_ADDR, PCI_HIGH_SIZE);
> +    memory_region_add_subregion(get_system_memory(), PCI_HIGH_ADDR, mr);

Re: [PATCH 3/4] hw/ppc/pegasos2: Rename to pegasos

[Philippe Mathieu-Daudé]

On 25/10/25 01:31, BALATON Zoltan wrote:
> Now that we also emulate pegasos1 it is not only about pegasos2 so
> rename to a more generic name encompassing both.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>   MAINTAINERS                             | 4 ++--
>   configs/devices/ppc-softmmu/default.mak | 7 +++----
>   hw/ppc/Kconfig                          | 2 +-
>   hw/ppc/meson.build                      | 4 ++--
>   hw/ppc/{pegasos2.c => pegasos.c}        | 0
>   5 files changed, 8 insertions(+), 9 deletions(-)
>   rename hw/ppc/{pegasos2.c => pegasos.c} (100%)

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

Re: [PATCH 4/4] hw/ppc/pegasos: Update documentation for pegasos1

[Philippe Mathieu-Daudé]

On 25/10/25 01:31, BALATON Zoltan wrote:
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>   docs/system/ppc/amigang.rst | 29 +++++++++++++++++------------
>   1 file changed, 17 insertions(+), 12 deletions(-)

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

Re: [PATCH 2/4] hw/pci-host/articia: Map PCI memory windows in realize

[BALATON Zoltan]

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

On Mon, 27 Oct 2025, Philippe Mathieu-Daudé wrote:
> On 25/10/25 01:31, BALATON Zoltan wrote:
>> These memory windows are a result of the address decoding in the
>> Articia S north bridge so better model it there and not in board code.
>> 
>> Suggested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>>   hw/pci-host/articia.c | 15 ++++++++++++++-
>>   hw/ppc/amigaone.c     | 28 +++++-----------------------
>>   hw/ppc/pegasos2.c     | 13 -------------
>>   3 files changed, 19 insertions(+), 37 deletions(-)
>
>
>> @@ -169,6 +174,7 @@ static void articia_realize(DeviceState *dev, Error 
>> **errp)
>>   {
>>       ArticiaState *s = ARTICIA(dev);
>>       PCIHostState *h = PCI_HOST_BRIDGE(dev);
>> +    MemoryRegion *mr;
>>       PCIDevice *pdev;
>>         bitbang_i2c_init(&s->smbus, i2c_init_bus(dev, "smbus"));
>> @@ -180,6 +186,14 @@ static void articia_realize(DeviceState *dev, Error 
>> **errp)
>>       memory_region_init_io(&s->reg, OBJECT(s), &articia_reg_ops, s,
>>                             TYPE_ARTICIA, 0x1000000);
>>       memory_region_add_subregion_overlap(&s->reg, 0, &s->io, 1);
>> +    mr = g_new(MemoryRegion, 1);
>
> Won't Coverity or other analysis tools complain about the leak?
> (this is why we usually keep a reference in the device state, here
> ArticiaState). Otherwise:

According to 
https://www.qemu.org/docs/master/devel/memory.html#region-lifecycle
there should be no leak and keeping a reference should not be necessary as 
the lifetime is managed by attaching it to the owner object so no need to 
keep a reference when it's not needed otherwise. Not littering the state 
struct with unneded references makes it easier to comprehend so I'd only 
keep things there that are necessary.

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

Can I keep this R-b considering the above?

Regards,
BALATON Zoltan

>> +    memory_region_init_alias(mr, OBJECT(dev), "pci-mem-low", &s->mem,
>> +                             0, PCI_LOW_SIZE);
>> +    memory_region_add_subregion(get_system_memory(), PCI_LOW_ADDR, mr);
>> +    mr = g_new(MemoryRegion, 1);
>> +    memory_region_init_alias(mr, OBJECT(dev), "pci-mem-high", &s->mem,
>> +                             PCI_HIGH_ADDR, PCI_HIGH_SIZE);
>> +    memory_region_add_subregion(get_system_memory(), PCI_HIGH_ADDR, mr);
>
>

Re: [PATCH 2/4] hw/pci-host/articia: Map PCI memory windows in realize

[Philippe Mathieu-Daudé]

On 27/10/25 20:47, BALATON Zoltan wrote:
> On Mon, 27 Oct 2025, Philippe Mathieu-Daudé wrote:
>> On 25/10/25 01:31, BALATON Zoltan wrote:
>>> These memory windows are a result of the address decoding in the
>>> Articia S north bridge so better model it there and not in board code.
>>>
>>> Suggested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>> ---
>>>   hw/pci-host/articia.c | 15 ++++++++++++++-
>>>   hw/ppc/amigaone.c     | 28 +++++-----------------------
>>>   hw/ppc/pegasos2.c     | 13 -------------
>>>   3 files changed, 19 insertions(+), 37 deletions(-)
>>
>>
>>> @@ -169,6 +174,7 @@ static void articia_realize(DeviceState *dev, 
>>> Error **errp)
>>>   {
>>>       ArticiaState *s = ARTICIA(dev);
>>>       PCIHostState *h = PCI_HOST_BRIDGE(dev);
>>> +    MemoryRegion *mr;
>>>       PCIDevice *pdev;
>>>         bitbang_i2c_init(&s->smbus, i2c_init_bus(dev, "smbus"));
>>> @@ -180,6 +186,14 @@ static void articia_realize(DeviceState *dev, 
>>> Error **errp)
>>>       memory_region_init_io(&s->reg, OBJECT(s), &articia_reg_ops, s,
>>>                             TYPE_ARTICIA, 0x1000000);
>>>       memory_region_add_subregion_overlap(&s->reg, 0, &s->io, 1);
>>> +    mr = g_new(MemoryRegion, 1);
>>
>> Won't Coverity or other analysis tools complain about the leak?
>> (this is why we usually keep a reference in the device state, here
>> ArticiaState). Otherwise:
> 
> According to https://www.qemu.org/docs/master/devel/memory.html#region- 
> lifecycle
> there should be no leak and keeping a reference should not be necessary 
> as the lifetime is managed by attaching it to the owner object so no 
> need to keep a reference when it's not needed otherwise. Not littering 
> the state struct with unneded references makes it easier to comprehend 
> so I'd only keep things there that are necessary.

IIUC this doc is about what happens within the allocated MemoryRegion,
regardless of where it is allocated.

> 
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> 
> Can I keep this R-b considering the above?
> 
> Regards,
> BALATON Zoltan
> 
>>> +    memory_region_init_alias(mr, OBJECT(dev), "pci-mem-low", &s->mem,
>>> +                             0, PCI_LOW_SIZE);
>>> +    memory_region_add_subregion(get_system_memory(), PCI_LOW_ADDR, mr);
>>> +    mr = g_new(MemoryRegion, 1);
>>> +    memory_region_init_alias(mr, OBJECT(dev), "pci-mem-high", &s->mem,
>>> +                             PCI_HIGH_ADDR, PCI_HIGH_SIZE);
>>> +    memory_region_add_subregion(get_system_memory(), PCI_HIGH_ADDR, 
>>> mr);
>>
>>

Re: [PATCH 2/4] hw/pci-host/articia: Map PCI memory windows in realize

[BALATON Zoltan]

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

On Tue, 28 Oct 2025, Philippe Mathieu-Daudé wrote:
> On 27/10/25 20:47, BALATON Zoltan wrote:
>> On Mon, 27 Oct 2025, Philippe Mathieu-Daudé wrote:
>>> On 25/10/25 01:31, BALATON Zoltan wrote:
>>>> These memory windows are a result of the address decoding in the
>>>> Articia S north bridge so better model it there and not in board code.
>>>> 
>>>> Suggested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>>> ---
>>>>   hw/pci-host/articia.c | 15 ++++++++++++++-
>>>>   hw/ppc/amigaone.c     | 28 +++++-----------------------
>>>>   hw/ppc/pegasos2.c     | 13 -------------
>>>>   3 files changed, 19 insertions(+), 37 deletions(-)
>>> 
>>> 
>>>> @@ -169,6 +174,7 @@ static void articia_realize(DeviceState *dev, Error 
>>>> **errp)
>>>>   {
>>>>       ArticiaState *s = ARTICIA(dev);
>>>>       PCIHostState *h = PCI_HOST_BRIDGE(dev);
>>>> +    MemoryRegion *mr;
>>>>       PCIDevice *pdev;
>>>>         bitbang_i2c_init(&s->smbus, i2c_init_bus(dev, "smbus"));
>>>> @@ -180,6 +186,14 @@ static void articia_realize(DeviceState *dev, Error 
>>>> **errp)
>>>>       memory_region_init_io(&s->reg, OBJECT(s), &articia_reg_ops, s,
>>>>                             TYPE_ARTICIA, 0x1000000);
>>>>       memory_region_add_subregion_overlap(&s->reg, 0, &s->io, 1);
>>>> +    mr = g_new(MemoryRegion, 1);
>>> 
>>> Won't Coverity or other analysis tools complain about the leak?
>>> (this is why we usually keep a reference in the device state, here
>>> ArticiaState). Otherwise:
>> 
>> According to https://www.qemu.org/docs/master/devel/memory.html#region- 
>> lifecycle
>> there should be no leak and keeping a reference should not be necessary as 
>> the lifetime is managed by attaching it to the owner object so no need to 
>> keep a reference when it's not needed otherwise. Not littering the state 
>> struct with unneded references makes it easier to comprehend so I'd only 
>> keep things there that are necessary.
>
> IIUC this doc is about what happens within the allocated MemoryRegion,
> regardless of where it is allocated.

That doc explicitely says:

"Destruction of a memory region happens automatically when the owner 
object dies. When there are multiple memory regions under the same owner 
object, the memory API will guarantee all memory regions will be properly 
detached and finalized one by one. The order in which memory regions will 
be finalized is not guaranteed."

(and these pci-host objects are created once at machine init and never die 
so the question seems quite theoretical). I'd like to keep object state 
simple and not keep around references in it that nothing uses and should 
be managed automatically. I'd only add fields to the state struct that 
other methods need.

Regards,
BALATON Zoltan

Re: [PATCH 2/4] hw/pci-host/articia: Map PCI memory windows in realize

[Akihiko Odaki]

On 2025/10/28 21:59, BALATON Zoltan wrote:
> On Tue, 28 Oct 2025, Philippe Mathieu-Daudé wrote:
>> On 27/10/25 20:47, BALATON Zoltan wrote:
>>> On Mon, 27 Oct 2025, Philippe Mathieu-Daudé wrote:
>>>> On 25/10/25 01:31, BALATON Zoltan wrote:
>>>>> These memory windows are a result of the address decoding in the
>>>>> Articia S north bridge so better model it there and not in board code.
>>>>>
>>>>> Suggested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>>>> ---
>>>>>   hw/pci-host/articia.c | 15 ++++++++++++++-
>>>>>   hw/ppc/amigaone.c     | 28 +++++-----------------------
>>>>>   hw/ppc/pegasos2.c     | 13 -------------
>>>>>   3 files changed, 19 insertions(+), 37 deletions(-)
>>>>
>>>>
>>>>> @@ -169,6 +174,7 @@ static void articia_realize(DeviceState *dev, 
>>>>> Error **errp)
>>>>>   {
>>>>>       ArticiaState *s = ARTICIA(dev);
>>>>>       PCIHostState *h = PCI_HOST_BRIDGE(dev);
>>>>> +    MemoryRegion *mr;
>>>>>       PCIDevice *pdev;
>>>>>         bitbang_i2c_init(&s->smbus, i2c_init_bus(dev, "smbus"));
>>>>> @@ -180,6 +186,14 @@ static void articia_realize(DeviceState *dev, 
>>>>> Error **errp)
>>>>>       memory_region_init_io(&s->reg, OBJECT(s), &articia_reg_ops, s,
>>>>>                             TYPE_ARTICIA, 0x1000000);
>>>>>       memory_region_add_subregion_overlap(&s->reg, 0, &s->io, 1);
>>>>> +    mr = g_new(MemoryRegion, 1);
>>>>
>>>> Won't Coverity or other analysis tools complain about the leak?
>>>> (this is why we usually keep a reference in the device state, here
>>>> ArticiaState). Otherwise:
>>>
>>> According to https://www.qemu.org/docs/master/devel/ 
>>> memory.html#region- lifecycle
>>> there should be no leak and keeping a reference should not be 
>>> necessary as the lifetime is managed by attaching it to the owner 
>>> object so no need to keep a reference when it's not needed otherwise. 
>>> Not littering the state struct with unneded references makes it 
>>> easier to comprehend so I'd only keep things there that are necessary.
>>
>> IIUC this doc is about what happens within the allocated MemoryRegion,
>> regardless of where it is allocated.
> 
> That doc explicitely says:
> 
> "Destruction of a memory region happens automatically when the owner 
> object dies. When there are multiple memory regions under the same owner 
> object, the memory API will guarantee all memory regions will be 
> properly detached and finalized one by one. The order in which memory 
> regions will be finalized is not guaranteed."

Destruction in this context does not imply freeing the storage.

The documentation describes destruction in QOM. QOM performs the 
following steps during destruction:
1. Delete properties
2. Call finalization callbacks
3. Free the storage

However, 3 will not happen in this case since you allocate the storage 
by yourself and it is not managed by QOM.

Please also note that the documentation also says:
 > If however the memory region is part of a dynamically allocated data
 > structure, you should free the memory region in the instance_finalize
 > callback.  For an example see VFIOMSIXInfo and VFIOQuirk in
 > hw/vfio/pci.c.

> 
> (and these pci-host objects are created once at machine init and never 
> die so the question seems quite theoretical). I'd like to keep object 
> state simple and not keep around references in it that nothing uses and 
> should be managed automatically. I'd only add fields to the state struct 
> that other methods need.

It is indeed theoretical. That said, I prefer the memory region to be 
embedded into the device state struct as it will clarify that the 
lifetime of the memory region is bound to the device.

Regards,
Akihiko Odaki

Re: [PATCH 2/4] hw/pci-host/articia: Map PCI memory windows in realize

[BALATON Zoltan]

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

On Wed, 29 Oct 2025, Akihiko Odaki wrote:
> On 2025/10/28 21:59, BALATON Zoltan wrote:
>> On Tue, 28 Oct 2025, Philippe Mathieu-Daudé wrote:
>>> On 27/10/25 20:47, BALATON Zoltan wrote:
>>>> On Mon, 27 Oct 2025, Philippe Mathieu-Daudé wrote:
>>>>> On 25/10/25 01:31, BALATON Zoltan wrote:
>>>>>> These memory windows are a result of the address decoding in the
>>>>>> Articia S north bridge so better model it there and not in board code.
>>>>>> 
>>>>>> Suggested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>>>>> ---
>>>>>>   hw/pci-host/articia.c | 15 ++++++++++++++-
>>>>>>   hw/ppc/amigaone.c     | 28 +++++-----------------------
>>>>>>   hw/ppc/pegasos2.c     | 13 -------------
>>>>>>   3 files changed, 19 insertions(+), 37 deletions(-)
>>>>> 
>>>>> 
>>>>>> @@ -169,6 +174,7 @@ static void articia_realize(DeviceState *dev, Error 
>>>>>> **errp)
>>>>>>   {
>>>>>>       ArticiaState *s = ARTICIA(dev);
>>>>>>       PCIHostState *h = PCI_HOST_BRIDGE(dev);
>>>>>> +    MemoryRegion *mr;
>>>>>>       PCIDevice *pdev;
>>>>>>         bitbang_i2c_init(&s->smbus, i2c_init_bus(dev, "smbus"));
>>>>>> @@ -180,6 +186,14 @@ static void articia_realize(DeviceState *dev, 
>>>>>> Error **errp)
>>>>>>       memory_region_init_io(&s->reg, OBJECT(s), &articia_reg_ops, s,
>>>>>>                             TYPE_ARTICIA, 0x1000000);
>>>>>>       memory_region_add_subregion_overlap(&s->reg, 0, &s->io, 1);
>>>>>> +    mr = g_new(MemoryRegion, 1);
>>>>> 
>>>>> Won't Coverity or other analysis tools complain about the leak?
>>>>> (this is why we usually keep a reference in the device state, here
>>>>> ArticiaState). Otherwise:
>>>> 
>>>> According to https://www.qemu.org/docs/master/devel/ memory.html#region- 
>>>> lifecycle
>>>> there should be no leak and keeping a reference should not be necessary 
>>>> as the lifetime is managed by attaching it to the owner object so no need 
>>>> to keep a reference when it's not needed otherwise. Not littering the 
>>>> state struct with unneded references makes it easier to comprehend so I'd 
>>>> only keep things there that are necessary.
>>> 
>>> IIUC this doc is about what happens within the allocated MemoryRegion,
>>> regardless of where it is allocated.
>> 
>> That doc explicitely says:
>> 
>> "Destruction of a memory region happens automatically when the owner object 
>> dies. When there are multiple memory regions under the same owner object, 
>> the memory API will guarantee all memory regions will be properly detached 
>> and finalized one by one. The order in which memory regions will be 
>> finalized is not guaranteed."
>
> Destruction in this context does not imply freeing the storage.
>
> The documentation describes destruction in QOM. QOM performs the following 
> steps during destruction:
> 1. Delete properties
> 2. Call finalization callbacks
> 3. Free the storage
>
> However, 3 will not happen in this case since you allocate the storage by 
> yourself and it is not managed by QOM.
>
> Please also note that the documentation also says:
>> If however the memory region is part of a dynamically allocated data
>> structure, you should free the memory region in the instance_finalize
>> callback.  For an example see VFIOMSIXInfo and VFIOQuirk in
>> hw/vfio/pci.c.

So the problem is probably using g_new() to allocate it. Maybe this should 
be object_new() instead? I was trying to figure out how this should work 
but not sure I understand everything. It looks like as long as the mr has 
a name, memory_region_init() will add the mr as a child property to the 
owner and unref it so the reference will be passed to the owner which will 
own this mr after that. On destructing the owner it will delete all its 
properties which should dereference the memory region so it would be freed 
then if it had a free function set but it seems allocating with g_new and 
init-ing the region with memory_region_init() won't set the free function. 
Only way to set that seems to be object_new() but that also inits the 
object so maybe using it would double init the mr or add an extra 
reference so that may also not work. The extra ref could be solved by 
unref'ing after memory_region_init() but double init seems to be 
unnecessary. Maybe we need a memory_region_new_* set of functions or 
simpler than that an object_alloc() that allocates the object without
init it that could be used in this case when init is done by other 
function like memory_region_init in this case?

>> (and these pci-host objects are created once at machine init and never die 
>> so the question seems quite theoretical). I'd like to keep object state 
>> simple and not keep around references in it that nothing uses and should be 
>> managed automatically. I'd only add fields to the state struct that other 
>> methods need.
>
> It is indeed theoretical. That said, I prefer the memory region to be 
> embedded into the device state struct as it will clarify that the lifetime of 
> the memory region is bound to the device.

But that way a lot of otherwise not needed fields would make the state 
struct more crowded and harder to see what's actually used there. Maybe 
this could be remedied by grouping these at the end below a comment but if 
ref counting worked as the docs state it this should not be necessary.

Before my cleanup the state strcut in raven.c looked like this:

struct PRePPCIState {
     PCIHostState parent_obj;

     OrIRQState *or_irq;
     qemu_irq pci_irqs[PCI_NUM_PINS];
     PCIBus pci_bus;
     AddressSpace pci_io_as;
     MemoryRegion pci_io;
     MemoryRegion pci_io_non_contiguous;
     MemoryRegion pci_memory;
     MemoryRegion pci_intack;
     MemoryRegion bm;
     MemoryRegion bm_ram_alias;
     MemoryRegion bm_pci_memory_alias;
     AddressSpace bm_as;
     RavenPCIState pci_dev;

     int contiguous_map;
};

which would become this after this series:

struct PREPPCIState {
     PCIHostState parent_obj;

     qemu_irq irq;
     MemoryRegion pci_io;
     MemoryRegion pci_discontiguous_io;
     MemoryRegion pci_memory;
     MemoryRegion pci_intack;
     AddressSpace bm_as;
};

if we don't have to keep regions we don't use after realize so those can 
be managed by QOM which is much more readable and comprehensible to me.

(Unrelated question but I'm still not sure what the bm_as is for. This 
seems to be present in some pci-hosts and may be needed for bus master 
cards but I just carried it over to my devices as a cargo cult without 
really getting why is it needed and why is it needed here and not in 
PCIHostState. Is there any explanation on that somewhere?)

Regards,
BALATON Zoltan

Re: [PATCH 2/4] hw/pci-host/articia: Map PCI memory windows in realize

[BALATON Zoltan]

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

On Mon, 27 Oct 2025, Philippe Mathieu-Daudé wrote:
> On 25/10/25 01:31, BALATON Zoltan wrote:
>> These memory windows are a result of the address decoding in the
>> Articia S north bridge so better model it there and not in board code.
>> 
>> Suggested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>>   hw/pci-host/articia.c | 15 ++++++++++++++-
>>   hw/ppc/amigaone.c     | 28 +++++-----------------------
>>   hw/ppc/pegasos2.c     | 13 -------------
>>   3 files changed, 19 insertions(+), 37 deletions(-)
>
>
>> @@ -169,6 +174,7 @@ static void articia_realize(DeviceState *dev, Error 
>> **errp)
>>   {
>>       ArticiaState *s = ARTICIA(dev);
>>       PCIHostState *h = PCI_HOST_BRIDGE(dev);
>> +    MemoryRegion *mr;
>>       PCIDevice *pdev;
>>         bitbang_i2c_init(&s->smbus, i2c_init_bus(dev, "smbus"));
>> @@ -180,6 +186,14 @@ static void articia_realize(DeviceState *dev, Error 
>> **errp)
>>       memory_region_init_io(&s->reg, OBJECT(s), &articia_reg_ops, s,
>>                             TYPE_ARTICIA, 0x1000000);
>>       memory_region_add_subregion_overlap(&s->reg, 0, &s->io, 1);
>> +    mr = g_new(MemoryRegion, 1);
>
> Won't Coverity or other analysis tools complain about the leak?
> (this is why we usually keep a reference in the device state, here
> ArticiaState). Otherwise:
>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

In any case, this same code is already present in amigaone.c and 
pegasos2.c and this patch just moves it to articia.c so it should not make 
it worse than it already is. Therefore can we take it for this release 
with or without your R-b just to not block the whole series and come back 
to clean this up later? Otherwise I'll have to send a v2 dropping this 
patch and redo the next one without it but since this is preexisting I'd 
avoid that and go with it for now that then leaves us time to find a 
solution for it and other simliar code which then can be fixed together. 
Is that OK with you or should I go for a v2?

Regards,
BALATON Zoltan

>> +    memory_region_init_alias(mr, OBJECT(dev), "pci-mem-low", &s->mem,
>> +                             0, PCI_LOW_SIZE);
>> +    memory_region_add_subregion(get_system_memory(), PCI_LOW_ADDR, mr);
>> +    mr = g_new(MemoryRegion, 1);
>> +    memory_region_init_alias(mr, OBJECT(dev), "pci-mem-high", &s->mem,
>> +                             PCI_HIGH_ADDR, PCI_HIGH_SIZE);
>> +    memory_region_add_subregion(get_system_memory(), PCI_HIGH_ADDR, mr);
>
>

Re: [PATCH 2/4] hw/pci-host/articia: Map PCI memory windows in realize

[Akihiko Odaki]

On 2025/10/29 6:28, BALATON Zoltan wrote:
> On Wed, 29 Oct 2025, Akihiko Odaki wrote:
>> On 2025/10/28 21:59, BALATON Zoltan wrote:
>>> On Tue, 28 Oct 2025, Philippe Mathieu-Daudé wrote:
>>>> On 27/10/25 20:47, BALATON Zoltan wrote:
>>>>> On Mon, 27 Oct 2025, Philippe Mathieu-Daudé wrote:
>>>>>> On 25/10/25 01:31, BALATON Zoltan wrote:
>>>>>>> These memory windows are a result of the address decoding in the
>>>>>>> Articia S north bridge so better model it there and not in board 
>>>>>>> code.
>>>>>>>
>>>>>>> Suggested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>>>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>>>>>> ---
>>>>>>>   hw/pci-host/articia.c | 15 ++++++++++++++-
>>>>>>>   hw/ppc/amigaone.c     | 28 +++++-----------------------
>>>>>>>   hw/ppc/pegasos2.c     | 13 -------------
>>>>>>>   3 files changed, 19 insertions(+), 37 deletions(-)
>>>>>>
>>>>>>
>>>>>>> @@ -169,6 +174,7 @@ static void articia_realize(DeviceState *dev, 
>>>>>>> Error **errp)
>>>>>>>   {
>>>>>>>       ArticiaState *s = ARTICIA(dev);
>>>>>>>       PCIHostState *h = PCI_HOST_BRIDGE(dev);
>>>>>>> +    MemoryRegion *mr;
>>>>>>>       PCIDevice *pdev;
>>>>>>>         bitbang_i2c_init(&s->smbus, i2c_init_bus(dev, "smbus"));
>>>>>>> @@ -180,6 +186,14 @@ static void articia_realize(DeviceState 
>>>>>>> *dev, Error **errp)
>>>>>>>       memory_region_init_io(&s->reg, OBJECT(s), &articia_reg_ops, s,
>>>>>>>                             TYPE_ARTICIA, 0x1000000);
>>>>>>>       memory_region_add_subregion_overlap(&s->reg, 0, &s->io, 1);
>>>>>>> +    mr = g_new(MemoryRegion, 1);
>>>>>>
>>>>>> Won't Coverity or other analysis tools complain about the leak?
>>>>>> (this is why we usually keep a reference in the device state, here
>>>>>> ArticiaState). Otherwise:
>>>>>
>>>>> According to https://www.qemu.org/docs/master/devel/ 
>>>>> memory.html#region- lifecycle
>>>>> there should be no leak and keeping a reference should not be 
>>>>> necessary as the lifetime is managed by attaching it to the owner 
>>>>> object so no need to keep a reference when it's not needed 
>>>>> otherwise. Not littering the state struct with unneded references 
>>>>> makes it easier to comprehend so I'd only keep things there that 
>>>>> are necessary.
>>>>
>>>> IIUC this doc is about what happens within the allocated MemoryRegion,
>>>> regardless of where it is allocated.
>>>
>>> That doc explicitely says:
>>>
>>> "Destruction of a memory region happens automatically when the owner 
>>> object dies. When there are multiple memory regions under the same 
>>> owner object, the memory API will guarantee all memory regions will 
>>> be properly detached and finalized one by one. The order in which 
>>> memory regions will be finalized is not guaranteed."
>>
>> Destruction in this context does not imply freeing the storage.
>>
>> The documentation describes destruction in QOM. QOM performs the 
>> following steps during destruction:
>> 1. Delete properties
>> 2. Call finalization callbacks
>> 3. Free the storage
>>
>> However, 3 will not happen in this case since you allocate the storage 
>> by yourself and it is not managed by QOM.
>>
>> Please also note that the documentation also says:
>>> If however the memory region is part of a dynamically allocated data
>>> structure, you should free the memory region in the instance_finalize
>>> callback.  For an example see VFIOMSIXInfo and VFIOQuirk in
>>> hw/vfio/pci.c.
> 
> So the problem is probably using g_new() to allocate it. Maybe this 
> should be object_new() instead? I was trying to figure out how this 
> should work but not sure I understand everything. It looks like as long 
> as the mr has a name, memory_region_init() will add the mr as a child 
> property to the owner and unref it so the reference will be passed to 
> the owner which will own this mr after that. On destructing the owner it 
> will delete all its properties which should dereference the memory 
> region so it would be freed then if it had a free function set but it 
> seems allocating with g_new and init-ing the region with 
> memory_region_init() won't set the free function. Only way to set that 
> seems to be object_new() but that also inits the object so maybe using 
> it would double init the mr or add an extra reference so that may also 
> not work. The extra ref could be solved by unref'ing after 
> memory_region_init() but double init seems to be unnecessary. Maybe we 
> need a memory_region_new_* set of functions or simpler than that an 
> object_alloc() that allocates the object without
> init it that could be used in this case when init is done by other 
> function like memory_region_init in this case?

memory_region_new_* will work, but I don't think removing unnecessary 
but harmless fields from a device state structure does not sufficiently 
motivates adding them.

> 
>>> (and these pci-host objects are created once at machine init and 
>>> never die so the question seems quite theoretical). I'd like to keep 
>>> object state simple and not keep around references in it that nothing 
>>> uses and should be managed automatically. I'd only add fields to the 
>>> state struct that other methods need.
>>
>> It is indeed theoretical. That said, I prefer the memory region to be 
>> embedded into the device state struct as it will clarify that the 
>> lifetime of the memory region is bound to the device.
> 
> But that way a lot of otherwise not needed fields would make the state 
> struct more crowded and harder to see what's actually used there. Maybe 
> this could be remedied by grouping these at the end below a comment but 
> if ref counting worked as the docs state it this should not be necessary.
> 
> Before my cleanup the state strcut in raven.c looked like this:
> 
> struct PRePPCIState {
>      PCIHostState parent_obj;
> 
>      OrIRQState *or_irq;
>      qemu_irq pci_irqs[PCI_NUM_PINS];
>      PCIBus pci_bus;
>      AddressSpace pci_io_as;
>      MemoryRegion pci_io;
>      MemoryRegion pci_io_non_contiguous;
>      MemoryRegion pci_memory;
>      MemoryRegion pci_intack;
>      MemoryRegion bm;
>      MemoryRegion bm_ram_alias;
>      MemoryRegion bm_pci_memory_alias;
>      AddressSpace bm_as;
>      RavenPCIState pci_dev;
> 
>      int contiguous_map;
> };
> 
> which would become this after this series:
> 
> struct PREPPCIState {
>      PCIHostState parent_obj;
> 
>      qemu_irq irq;
>      MemoryRegion pci_io;
>      MemoryRegion pci_discontiguous_io;
>      MemoryRegion pci_memory;
>      MemoryRegion pci_intack;
>      AddressSpace bm_as;
> };
> 
> if we don't have to keep regions we don't use after realize so those can 
> be managed by QOM which is much more readable and comprehensible to me.

It still requires a trade-off that obscures the duration of MemoryRegion 
and may confuse Coverity or other tools. I also prefer the fields 
present in the state as I can be immediately sure they do not leak even 
without checking that the device is not hotpluggable.

I don't think it needs comments. It is a common pattern that 
MemoryRegions are directly referenced only at initialization and 
uninitialization. The presence of MemoryRegions in a device state 
structure only tells that they are alive as long as the device is alive, 
and does not imply that it is directly referenced later.

Regards,
Akihiko Odaki

Re: [PATCH 2/4] hw/pci-host/articia: Map PCI memory windows in realize

[BALATON Zoltan]

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

On Wed, 29 Oct 2025, Akihiko Odaki wrote:
> On 2025/10/29 6:28, BALATON Zoltan wrote:
>> On Wed, 29 Oct 2025, Akihiko Odaki wrote:
>>> On 2025/10/28 21:59, BALATON Zoltan wrote:
>>>> On Tue, 28 Oct 2025, Philippe Mathieu-Daudé wrote:
>>>>> On 27/10/25 20:47, BALATON Zoltan wrote:
>>>>>> On Mon, 27 Oct 2025, Philippe Mathieu-Daudé wrote:
>>>>>>> On 25/10/25 01:31, BALATON Zoltan wrote:
>>>>>>>> These memory windows are a result of the address decoding in the
>>>>>>>> Articia S north bridge so better model it there and not in board 
>>>>>>>> code.
>>>>>>>> 
>>>>>>>> Suggested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>>>>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>>>>>>> ---
>>>>>>>>   hw/pci-host/articia.c | 15 ++++++++++++++-
>>>>>>>>   hw/ppc/amigaone.c     | 28 +++++-----------------------
>>>>>>>>   hw/ppc/pegasos2.c     | 13 -------------
>>>>>>>>   3 files changed, 19 insertions(+), 37 deletions(-)
>>>>>>> 
>>>>>>> 
>>>>>>>> @@ -169,6 +174,7 @@ static void articia_realize(DeviceState *dev, 
>>>>>>>> Error **errp)
>>>>>>>>   {
>>>>>>>>       ArticiaState *s = ARTICIA(dev);
>>>>>>>>       PCIHostState *h = PCI_HOST_BRIDGE(dev);
>>>>>>>> +    MemoryRegion *mr;
>>>>>>>>       PCIDevice *pdev;
>>>>>>>>         bitbang_i2c_init(&s->smbus, i2c_init_bus(dev, "smbus"));
>>>>>>>> @@ -180,6 +186,14 @@ static void articia_realize(DeviceState *dev, 
>>>>>>>> Error **errp)
>>>>>>>>       memory_region_init_io(&s->reg, OBJECT(s), &articia_reg_ops, s,
>>>>>>>>                             TYPE_ARTICIA, 0x1000000);
>>>>>>>>       memory_region_add_subregion_overlap(&s->reg, 0, &s->io, 1);
>>>>>>>> +    mr = g_new(MemoryRegion, 1);
>>>>>>> 
>>>>>>> Won't Coverity or other analysis tools complain about the leak?
>>>>>>> (this is why we usually keep a reference in the device state, here
>>>>>>> ArticiaState). Otherwise:
>>>>>> 
>>>>>> According to https://www.qemu.org/docs/master/devel/ 
>>>>>> memory.html#region- lifecycle
>>>>>> there should be no leak and keeping a reference should not be necessary 
>>>>>> as the lifetime is managed by attaching it to the owner object so no 
>>>>>> need to keep a reference when it's not needed otherwise. Not littering 
>>>>>> the state struct with unneded references makes it easier to comprehend 
>>>>>> so I'd only keep things there that are necessary.
>>>>> 
>>>>> IIUC this doc is about what happens within the allocated MemoryRegion,
>>>>> regardless of where it is allocated.
>>>> 
>>>> That doc explicitely says:
>>>> 
>>>> "Destruction of a memory region happens automatically when the owner 
>>>> object dies. When there are multiple memory regions under the same owner 
>>>> object, the memory API will guarantee all memory regions will be properly 
>>>> detached and finalized one by one. The order in which memory regions will 
>>>> be finalized is not guaranteed."
>>> 
>>> Destruction in this context does not imply freeing the storage.
>>> 
>>> The documentation describes destruction in QOM. QOM performs the following 
>>> steps during destruction:
>>> 1. Delete properties
>>> 2. Call finalization callbacks
>>> 3. Free the storage
>>> 
>>> However, 3 will not happen in this case since you allocate the storage by 
>>> yourself and it is not managed by QOM.
>>> 
>>> Please also note that the documentation also says:
>>>> If however the memory region is part of a dynamically allocated data
>>>> structure, you should free the memory region in the instance_finalize
>>>> callback.  For an example see VFIOMSIXInfo and VFIOQuirk in
>>>> hw/vfio/pci.c.
>> 
>> So the problem is probably using g_new() to allocate it. Maybe this should 
>> be object_new() instead? I was trying to figure out how this should work 
>> but not sure I understand everything. It looks like as long as the mr has a 
>> name, memory_region_init() will add the mr as a child property to the owner 
>> and unref it so the reference will be passed to the owner which will own 
>> this mr after that. On destructing the owner it will delete all its 
>> properties which should dereference the memory region so it would be freed 
>> then if it had a free function set but it seems allocating with g_new and 
>> init-ing the region with memory_region_init() won't set the free function. 
>> Only way to set that seems to be object_new() but that also inits the 
>> object so maybe using it would double init the mr or add an extra reference 
>> so that may also not work. The extra ref could be solved by unref'ing after 
>> memory_region_init() but double init seems to be unnecessary. Maybe we need 
>> a memory_region_new_* set of functions or simpler than that an 
>> object_alloc() that allocates the object without
>> init it that could be used in this case when init is done by other function 
>> like memory_region_init in this case?
>
> memory_region_new_* will work, but I don't think removing unnecessary but 
> harmless fields from a device state structure does not sufficiently motivates 
> adding them.

I haven't given up on this yet, that's why I alternatively proposed

object_alloc (same as object_new without object_initialize)
memory_region_init

which is just a small change but should also work without adding 
memory_region_new convenience functions. Then only object_alloc needs to 
be added.

>>>> (and these pci-host objects are created once at machine init and never 
>>>> die so the question seems quite theoretical). I'd like to keep object 
>>>> state simple and not keep around references in it that nothing uses and 
>>>> should be managed automatically. I'd only add fields to the state struct 
>>>> that other methods need.
>>> 
>>> It is indeed theoretical. That said, I prefer the memory region to be 
>>> embedded into the device state struct as it will clarify that the lifetime 
>>> of the memory region is bound to the device.
>> 
>> But that way a lot of otherwise not needed fields would make the state 
>> struct more crowded and harder to see what's actually used there. Maybe 
>> this could be remedied by grouping these at the end below a comment but if 
>> ref counting worked as the docs state it this should not be necessary.
>> 
>> Before my cleanup the state strcut in raven.c looked like this:
>> 
>> struct PRePPCIState {
>>      PCIHostState parent_obj;
>>
>>      OrIRQState *or_irq;
>>      qemu_irq pci_irqs[PCI_NUM_PINS];
>>      PCIBus pci_bus;
>>      AddressSpace pci_io_as;
>>      MemoryRegion pci_io;
>>      MemoryRegion pci_io_non_contiguous;
>>      MemoryRegion pci_memory;
>>      MemoryRegion pci_intack;
>>      MemoryRegion bm;
>>      MemoryRegion bm_ram_alias;
>>      MemoryRegion bm_pci_memory_alias;
>>      AddressSpace bm_as;
>>      RavenPCIState pci_dev;
>>
>>      int contiguous_map;
>> };
>> 
>> which would become this after this series:
>> 
>> struct PREPPCIState {
>>      PCIHostState parent_obj;
>>
>>      qemu_irq irq;
>>      MemoryRegion pci_io;
>>      MemoryRegion pci_discontiguous_io;
>>      MemoryRegion pci_memory;
>>      MemoryRegion pci_intack;
>>      AddressSpace bm_as;
>> };
>> 
>> if we don't have to keep regions we don't use after realize so those can be 
>> managed by QOM which is much more readable and comprehensible to me.
>
> It still requires a trade-off that obscures the duration of MemoryRegion and 
> may confuse Coverity or other tools. I also prefer the fields present in the 
> state as I can be immediately sure they do not leak even without checking 
> that the device is not hotpluggable.

What I don't like is that it blows up the object state and makes it look 
more complex than needed. Memory regions are often created at realize then 
never used again. The documentation also says that these should be managed 
automatically and when memory_region_init'ing it adds a reference to the 
owner that should be enough to manage the lifetime of the mr. Also when 
using sysbus_init_mmio it also stores a pointer to the mr so actually no 
need to store those MemoryRegions in the object state for SysBusDevices. 
Then most simple devices could do without a subclass or only a few fields 
in their state that they add to their superclass but now we have these 
full of these memory regions unnecessarily.

> I don't think it needs comments. It is a common pattern that MemoryRegions 
> are directly referenced only at initialization and uninitialization. The 
> presence of MemoryRegions in a device state structure only tells that they 
> are alive as long as the device is alive, and does not imply that it is 
> directly referenced later.

But this makes the device more difficult to comprehend. As in the raven 
device above we had a lot of memory regions plus an unneeded init method 
that we could do without to make the device simpler and easier to 
understand what actual functionality it adds. Having additional fields in 
the state struct just distracts from that.

It looks like we won't be able to come to an agreement before the freeze 
and I don't have time now to change this patch but don't want to miss the 
release with this series that finishes pegasos renaming because of this. 
So for this patch I'd say since this is already how it is now and it does 
not make it worse and this object is not user creatable anyway so cannot 
leak please take it as it is and we'll do a clean up later after we finish 
discussion.

Regards,
BALATON Zoltan

Re: [PATCH 2/4] hw/pci-host/articia: Map PCI memory windows in realize

[BALATON Zoltan]

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

On Wed, 29 Oct 2025, BALATON Zoltan wrote:
> On Wed, 29 Oct 2025, Akihiko Odaki wrote:
>> On 2025/10/29 6:28, BALATON Zoltan wrote:
>>> On Wed, 29 Oct 2025, Akihiko Odaki wrote:
>>>> On 2025/10/28 21:59, BALATON Zoltan wrote:
>>>>> On Tue, 28 Oct 2025, Philippe Mathieu-Daudé wrote:
>>>>>> On 27/10/25 20:47, BALATON Zoltan wrote:
>>>>>>> On Mon, 27 Oct 2025, Philippe Mathieu-Daudé wrote:
>>>>>>>> On 25/10/25 01:31, BALATON Zoltan wrote:
>>>>>>>>> These memory windows are a result of the address decoding in the
>>>>>>>>> Articia S north bridge so better model it there and not in board 
>>>>>>>>> code.
>>>>>>>>> 
>>>>>>>>> Suggested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>>>>>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>>>>>>>> ---
>>>>>>>>>   hw/pci-host/articia.c | 15 ++++++++++++++-
>>>>>>>>>   hw/ppc/amigaone.c     | 28 +++++-----------------------
>>>>>>>>>   hw/ppc/pegasos2.c     | 13 -------------
>>>>>>>>>   3 files changed, 19 insertions(+), 37 deletions(-)
>>>>>>>> 
[...]
> It looks like we won't be able to come to an agreement before the freeze and 
> I don't have time now to change this patch but don't want to miss the release 
> with this series that finishes pegasos renaming because of this. So for this 
> patch I'd say since this is already how it is now and it does not make it 
> worse and this object is not user creatable anyway so cannot leak please take 
> it as it is and we'll do a clean up later after we finish discussion.

As for all of these files I'm the maintainer let me make an executive 
decision here to keep this patch without Philippe's reviewed-by for now to 
be able to move on with this series before the freeze. Fixing the 
theoretical leak can be done on top and since that's a fix it can be done 
during soft freeze that would give us more time. So Harsh please go ahead 
and merge this series too if there are no other concerns. I'll then 
address this later together with other similar issues elsewhere.

Regards,
BALATON Zoltan

Re: [PATCH 2/4] hw/pci-host/articia: Map PCI memory windows in realize

[Peter Xu]

On Wed, Oct 29, 2025 at 11:30:10AM +0100, BALATON Zoltan wrote:
> > memory_region_new_* will work, but I don't think removing unnecessary
> > but harmless fields from a device state structure does not sufficiently
> > motivates adding them.
> 
> I haven't given up on this yet, that's why I alternatively proposed
> 
> object_alloc (same as object_new without object_initialize)
> memory_region_init
> 
> which is just a small change but should also work without adding
> memory_region_new convenience functions. Then only object_alloc needs to be
> added.

IMHO if this will ever happen, memory_region_new*() is better, unless
object_alloc() can be used anywhere besides MemoryRegion..

It seems to me, MemoryRegion is the only one I'm aware of that may need
such tweak, rather than using object_new() directly.

-- 
Peter Xu

Re: [PATCH 2/4] hw/pci-host/articia: Map PCI memory windows in realize

[Mark Cave-Ayland]

On 29/10/2025 13:25, BALATON Zoltan wrote:

> On Wed, 29 Oct 2025, BALATON Zoltan wrote:
>> On Wed, 29 Oct 2025, Akihiko Odaki wrote:
>>> On 2025/10/29 6:28, BALATON Zoltan wrote:
>>>> On Wed, 29 Oct 2025, Akihiko Odaki wrote:
>>>>> On 2025/10/28 21:59, BALATON Zoltan wrote:
>>>>>> On Tue, 28 Oct 2025, Philippe Mathieu-Daudé wrote:
>>>>>>> On 27/10/25 20:47, BALATON Zoltan wrote:
>>>>>>>> On Mon, 27 Oct 2025, Philippe Mathieu-Daudé wrote:
>>>>>>>>> On 25/10/25 01:31, BALATON Zoltan wrote:
>>>>>>>>>> These memory windows are a result of the address decoding in the
>>>>>>>>>> Articia S north bridge so better model it there and not in board code.
>>>>>>>>>>
>>>>>>>>>> Suggested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>>>>>>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>>>>>>>>> ---
>>>>>>>>>>   hw/pci-host/articia.c | 15 ++++++++++++++-
>>>>>>>>>>   hw/ppc/amigaone.c     | 28 +++++-----------------------
>>>>>>>>>>   hw/ppc/pegasos2.c     | 13 -------------
>>>>>>>>>>   3 files changed, 19 insertions(+), 37 deletions(-)
>>>>>>>>>
> [...]
>> It looks like we won't be able to come to an agreement before the freeze and I 
>> don't have time now to change this patch but don't want to miss the release with 
>> this series that finishes pegasos renaming because of this. So for this patch I'd 
>> say since this is already how it is now and it does not make it worse and this 
>> object is not user creatable anyway so cannot leak please take it as it is and 
>> we'll do a clean up later after we finish discussion.
> 
> As for all of these files I'm the maintainer let me make an executive decision here 
> to keep this patch without Philippe's reviewed-by for now to be able to move on with 
> this series before the freeze. Fixing the theoretical leak can be done on top and 
> since that's a fix it can be done during soft freeze that would give us more time. So 
> Harsh please go ahead and merge this series too if there are no other concerns. I'll 
> then address this later together with other similar issues elsewhere.

I think the issue here is that several people have now raised concerns as to the way 
you are trying to use MemoryRegions (both here and in the raven series): simply put, 
if reviewers didn't see any problems with this approach, your series would already 
have review tags.

If you want to suggest a different approach to managing MemoryRegions, then I would 
suggest you send a proposal to the mailing list so it can be reviewed by the QOM and 
memory people (along with updated code style documentation so we can all use it). But 
with freeze coming up in a few days this is certainly out of scope for 10.2.

For now I would suggest the easiest way to get review tags is to stick with the 
existing inline MemoryRegion approach for 10.2 to aid getting your patches merged: 
it's simply not fair to put time pressure on Harsh to merge patches in this way.


ATB,

Mark.

Re: [PATCH 1/4] hw/ppc/pegasos2: Add /chosen/stdin node with VOF

[Harsh Prateek Bora]

On 10/25/25 05:01, BALATON Zoltan wrote:
> Some very old Linux kernels fail to start if /chosen/stdin is not
> found so add it to the device tree when using VOF.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>   hw/ppc/pegasos2.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/hw/ppc/pegasos2.c b/hw/ppc/pegasos2.c
> index 93696ed381..21299dde3c 100644
> --- a/hw/ppc/pegasos2.c
> +++ b/hw/ppc/pegasos2.c
> @@ -565,6 +565,7 @@ static void pegasos_machine_reset(MachineState *machine, ResetType type)
>       qemu_fdt_setprop(fdt, "/chosen", "qemu,boot-kernel", d, sizeof(d));
>   
>       vof_build_dt(fdt, pm->vof);
> +    vof_client_open_store(fdt, pm->vof, "/chosen", "stdin", "/failsafe");

Reviewed-by: Harsh Prateek Bora <harshpb@linux.ibm.com>

>       vof_client_open_store(fdt, pm->vof, "/chosen", "stdout", "/failsafe");
>   
>       /* Set machine->fdt for 'dumpdtb' QMP/HMP command */

Re: [PATCH 2/4] hw/pci-host/articia: Map PCI memory windows in realize

[Paolo Bonzini]

On 10/29/25 20:20, Peter Xu wrote:
> On Wed, Oct 29, 2025 at 11:30:10AM +0100, BALATON Zoltan wrote:
>>> memory_region_new_* will work, but I don't think removing unnecessary
>>> but harmless fields from a device state structure does not sufficiently
>>> motivates adding them.
>>
>> I haven't given up on this yet, that's why I alternatively proposed
>>
>> object_alloc (same as object_new without object_initialize)
>> memory_region_init
>>
>> which is just a small change but should also work without adding
>> memory_region_new convenience functions. Then only object_alloc needs to be
>> added.
> 
> IMHO if this will ever happen, memory_region_new*() is better, unless
> object_alloc() can be used anywhere besides MemoryRegion..
> 
> It seems to me, MemoryRegion is the only one I'm aware of that may need
> such tweak, rather than using object_new() directly.

Yes, pretty much.  Anyhow, leaking on purpose with g_new is not a good 
idea.  It's g_new, not g_leak; and everyone else is using a field in the 
device structure so I don't see why one would want to do differently.

Paolo

Re: [PATCH 2/4] hw/pci-host/articia: Map PCI memory windows in realize

[BALATON Zoltan]

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

On Thu, 30 Oct 2025, Mark Cave-Ayland wrote:
> On 29/10/2025 13:25, BALATON Zoltan wrote:
>> On Wed, 29 Oct 2025, BALATON Zoltan wrote:
>>> On Wed, 29 Oct 2025, Akihiko Odaki wrote:
>>>> On 2025/10/29 6:28, BALATON Zoltan wrote:
>>>>> On Wed, 29 Oct 2025, Akihiko Odaki wrote:
>>>>>> On 2025/10/28 21:59, BALATON Zoltan wrote:
>>>>>>> On Tue, 28 Oct 2025, Philippe Mathieu-Daudé wrote:
>>>>>>>> On 27/10/25 20:47, BALATON Zoltan wrote:
>>>>>>>>> On Mon, 27 Oct 2025, Philippe Mathieu-Daudé wrote:
>>>>>>>>>> On 25/10/25 01:31, BALATON Zoltan wrote:
>>>>>>>>>>> These memory windows are a result of the address decoding in the
>>>>>>>>>>> Articia S north bridge so better model it there and not in board 
>>>>>>>>>>> code.
>>>>>>>>>>> 
>>>>>>>>>>> Suggested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>>>>>>>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>>>>>>>>>> ---
>>>>>>>>>>>   hw/pci-host/articia.c | 15 ++++++++++++++-
>>>>>>>>>>>   hw/ppc/amigaone.c     | 28 +++++-----------------------
>>>>>>>>>>>   hw/ppc/pegasos2.c     | 13 -------------
>>>>>>>>>>>   3 files changed, 19 insertions(+), 37 deletions(-)
>>>>>>>>>> 
>> [...]
>>> It looks like we won't be able to come to an agreement before the freeze 
>>> and I don't have time now to change this patch but don't want to miss the 
>>> release with this series that finishes pegasos renaming because of this. 
>>> So for this patch I'd say since this is already how it is now and it does 
>>> not make it worse and this object is not user creatable anyway so cannot 
>>> leak please take it as it is and we'll do a clean up later after we finish 
>>> discussion.
>> 
>> As for all of these files I'm the maintainer let me make an executive 
>> decision here to keep this patch without Philippe's reviewed-by for now to 
>> be able to move on with this series before the freeze. Fixing the 
>> theoretical leak can be done on top and since that's a fix it can be done 
>> during soft freeze that would give us more time. So Harsh please go ahead 
>> and merge this series too if there are no other concerns. I'll then address 
>> this later together with other similar issues elsewhere.
>
> I think the issue here is that several people have now raised concerns as to 
> the way you are trying to use MemoryRegions (both here and in the raven 
> series): simply put, if reviewers didn't see any problems with this approach, 
> your series would already have review tags.

I got that and did not mean to ignore those comments forever.

> If you want to suggest a different approach to managing MemoryRegions, then I 
> would suggest you send a proposal to the mailing list so it can be reviewed 
> by the QOM and memory people (along with updated code style documentation so

That's what I plan to do but got no time right now.

> we can all use it). But with freeze coming up in a few days this is certainly 
> out of scope for 10.2.

That's why I proposed to take this patch for now as it only moves existing 
code to another place so it does not introduce a new problem that's not 
already there. And the problem is theoretical as it does not cause a leak 
in this case and nobody raised concerns so far and it was only noticed by 
this patch making it clearer removing duplicated code. So I still think 
this patch has merit, it helped to find a bug and would make code simpler 
without making it worse so it could be taken for that. But I'm OK with 
dropping it too, I'll include it again in the series with the other 
proposed changes to fix the problem.

> For now I would suggest the easiest way to get review tags is to stick with 
> the existing inline MemoryRegion approach for 10.2 to aid getting your 
> patches merged: it's simply not fair to put time pressure on Harsh to merge 
> patches in this way.

I'm not putting time pressure on Harsh, the coming freeze does and I don't 
have time right now to change patches only later but I don't want to miss 
a release again. I've already submitted these pegasos1 emulation series 
for the previous release but due to missing maintainer it was missed then. 
I think it's now resolved by dropping this patch and taking the rest of 
the series which is fine with me. Thanks everybody for the reviews and 
help. I'll prepare patches to resolve concerns when I'll have time again.

Regards,
BALATON Zoltan

Re: [PATCH 2/4] hw/pci-host/articia: Map PCI memory windows in realize

[BALATON Zoltan]

On Thu, 30 Oct 2025, Paolo Bonzini wrote:
> On 10/29/25 20:20, Peter Xu wrote:
>> On Wed, Oct 29, 2025 at 11:30:10AM +0100, BALATON Zoltan wrote:
>>>> memory_region_new_* will work, but I don't think removing unnecessary
>>>> but harmless fields from a device state structure does not sufficiently
>>>> motivates adding them.
>>> 
>>> I haven't given up on this yet, that's why I alternatively proposed
>>> 
>>> object_alloc (same as object_new without object_initialize)
>>> memory_region_init
>>> 
>>> which is just a small change but should also work without adding
>>> memory_region_new convenience functions. Then only object_alloc needs to 
>>> be
>>> added.
>> 
>> IMHO if this will ever happen, memory_region_new*() is better, unless
>> object_alloc() can be used anywhere besides MemoryRegion..
>> 
>> It seems to me, MemoryRegion is the only one I'm aware of that may need
>> such tweak, rather than using object_new() directly.
>
> Yes, pretty much.  Anyhow, leaking on purpose with g_new is not a good idea. 
> It's g_new, not g_leak; and everyone else is using a field in the device 
> structure so I don't see why one would want to do differently.

I've tried to explain why I dislike that way in previous replies in this 
thread but here's a short summary:

- Not piling memory regions not otherwise needed in device struct makes it 
easier to understand. (Could you spot errors within the lot of boiler 
plate code before clean up? Having less code makes it more 
comprehensible.)

- Documentation says it should work this way QOM managing memory regions 
so it was meant to be that way. I'd rather fix code than documentation as 
I think if it just works that's easier than loosing that convenience.

- Allows simpler device models reusing superclasses without having to 
write much boiler plate code to create a subclass.

I'll send some patches that can be discussed later.

Regards,
BALATON Zoltan

Re: [PATCH 2/4] hw/pci-host/articia: Map PCI memory windows in realize

[Paolo Bonzini]

On 10/30/25 11:38, BALATON Zoltan wrote:
> I've tried to explain why I dislike that way in previous replies in this 
> thread but here's a short summary:
> 
> - Not piling memory regions not otherwise needed in device struct makes 
> it easier to understand. (Could you spot errors within the lot of boiler 
> plate code before clean up? Having less code makes it more comprehensible.)

Not sure what's different between

     MemoryRegion foo_mr;

in the struct, versus

     mr = g_new(MemoryRegion, 1);

in the realize function.  It's one line either way.

> - Documentation says it should work this way QOM managing memory regions 
> so it was meant to be that way. I'd rather fix code than documentation 
> as I think if it just works that's easier than loosing that convenience.No, that's *your* reading of the documentation, and it's based on the 
incorrect assumption that destruction implies freeing the memory. 
Akihiko explained that 
(https://lore.kernel.org/qemu-devel/802b77f2-2c23-4b5a-a739-d56b09c335de@rsg.ci.i.u-tokyo.ac.jp/).

The memory region documentation does not exist in a void, the difference 
between QOM object_initialize() and object_new() exists independent of 
that documentation.  It may be worth improving the QOM documentation on 
the object lifecycle; that could be.

Paolo

> - Allows simpler device models reusing superclasses without having to 
> write much boiler plate code to create a subclass.
> 
> I'll send some patches that can be discussed later.

Re: [PATCH 2/4] hw/pci-host/articia: Map PCI memory windows in realize

[BALATON Zoltan]

On Thu, 30 Oct 2025, Paolo Bonzini wrote:
> On 10/29/25 20:20, Peter Xu wrote:
>> On Wed, Oct 29, 2025 at 11:30:10AM +0100, BALATON Zoltan wrote:
>>>> memory_region_new_* will work, but I don't think removing unnecessary
>>>> but harmless fields from a device state structure does not sufficiently
>>>> motivates adding them.
>>> 
>>> I haven't given up on this yet, that's why I alternatively proposed
>>> 
>>> object_alloc (same as object_new without object_initialize)
>>> memory_region_init
>>> 
>>> which is just a small change but should also work without adding
>>> memory_region_new convenience functions. Then only object_alloc needs to 
>>> be
>>> added.
>> 
>> IMHO if this will ever happen, memory_region_new*() is better, unless
>> object_alloc() can be used anywhere besides MemoryRegion..

Objective-C (*step/macOS and derivatives) have two methods for creating 
objects: alloc then init. It also has "new" to combine both but exposing 
alloc is sometimes useful especially when having different init methods. I 
was thinking doing it that way which should be familiar to at least to 
some people. I'll explore that in a proposed series.

>> It seems to me, MemoryRegion is the only one I'm aware of that may need
>> such tweak, rather than using object_new() directly.
>
> Yes, pretty much.  Anyhow, leaking on purpose with g_new is not a good idea.

It does not leak in this case. This object is a host bridge created once 
and start and never removed and is not user creatable being a sysbus 
device. It's still theoretically incorrect but this patch did not 
introduce any new problems just made it clearer by moving it from board to 
device.

Regards,
BALATON Zoltan

Re: [PATCH 2/4] hw/pci-host/articia: Map PCI memory windows in realize

[BALATON Zoltan]

On Thu, 30 Oct 2025, Paolo Bonzini wrote:
> On 10/30/25 11:38, BALATON Zoltan wrote:
>> I've tried to explain why I dislike that way in previous replies in this 
>> thread but here's a short summary:
>> 
>> - Not piling memory regions not otherwise needed in device struct makes it 
>> easier to understand. (Could you spot errors within the lot of boiler plate 
>> code before clean up? Having less code makes it more comprehensible.)
>
> Not sure what's different between
>
>    MemoryRegion foo_mr;
>
> in the struct, versus
>
>    mr = g_new(MemoryRegion, 1);
>
> in the realize function.  It's one line either way.

Please read back in thread. An example here:
https://lists.nongnu.org/archive/html/qemu-ppc/2025-10/msg00785.html
from this series
https://patchew.org/QEMU/cover.1761232472.git.balaton@eik.bme.hu/

>> - Documentation says it should work this way QOM managing memory regions so 
>> it was meant to be that way. I'd rather fix code than documentation as I 
>> think if it just works that's easier than loosing that convenience.No, 
>> that's *your* reading of the documentation, and it's based on the 
> incorrect assumption that destruction implies freeing the memory. Akihiko 
> explained that 
> (https://lore.kernel.org/qemu-devel/802b77f2-2c23-4b5a-a739-d56b09c335de@rsg.ci.i.u-tokyo.ac.jp/).
>
> The memory region documentation does not exist in a void, the difference 
> between QOM object_initialize() and object_new() exists independent of that 
> documentation.  It may be worth improving the QOM documentation on the object 
> lifecycle; that could be.

I'll try to also clarify documentation but IMO the fix is not dropping 
this intended feature but fixing and using it where helps.

Regards,
BALATON Zoltan

Re: [PATCH 2/4] hw/pci-host/articia: Map PCI memory windows in realize

[Paolo Bonzini]

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

Il gio 30 ott 2025, 12:01 BALATON Zoltan <balaton@eik.bme.hu> ha scritto:

> > Not sure what's different between
> >
> >    MemoryRegion foo_mr;
> >
> > in the struct, versus
> >
> >    mr = g_new(MemoryRegion, 1);
> >
> > in the realize function.  It's one line either way.
>
> Please read back in thread. An example here:
> https://lists.nongnu.org/archive/html/qemu-ppc/2025-10/msg00785.html
> from this series
> https://patchew.org/QEMU/cover.1761232472.git.balaton@eik.bme.hu/


I did read it of course.

There's four people in this thread telling you not to do something. Just
stop arguing please.

Paolo


> >> - Documentation says it should work this way QOM managing memory
> regions so
> >> it was meant to be that way. I'd rather fix code than documentation as
> I
> >> think if it just works that's easier than loosing that convenience.No,
> >> that's *your* reading of the documentation, and it's based on the
> > incorrect assumption that destruction implies freeing the memory.
> Akihiko
> > explained that
> > (
> https://lore.kernel.org/qemu-devel/802b77f2-2c23-4b5a-a739-d56b09c335de@rsg.ci.i.u-tokyo.ac.jp/
> ).
> >
> > The memory region documentation does not exist in a void, the difference
> > between QOM object_initialize() and object_new() exists independent of
> that
> > documentation.  It may be worth improving the QOM documentation on the
> object
> > lifecycle; that could be.
>
> I'll try to also clarify documentation but IMO the fix is not dropping
> this intended feature but fixing and using it where helps.
>
> Regards,
> BALATON Zoltan
>
>

[-- Attachment #2: Type: text/html, Size: 2883 bytes --]

Re: [PATCH 2/4] hw/pci-host/articia: Map PCI memory windows in realize

[BALATON Zoltan]

On Thu, 30 Oct 2025, Paolo Bonzini wrote:
> Il gio 30 ott 2025, 12:01 BALATON Zoltan <balaton@eik.bme.hu> ha scritto:
>
>>> Not sure what's different between
>>>
>>>    MemoryRegion foo_mr;
>>>
>>> in the struct, versus
>>>
>>>    mr = g_new(MemoryRegion, 1);
>>>
>>> in the realize function.  It's one line either way.
>>
>> Please read back in thread. An example here:
>> https://lists.nongnu.org/archive/html/qemu-ppc/2025-10/msg00785.html
>> from this series
>> https://patchew.org/QEMU/cover.1761232472.git.balaton@eik.bme.hu/
>
>
> I did read it of course.
>
> There's four people in this thread telling you not to do something. Just
> stop arguing please.

I've already stopped and wasn't arguing just trying to answer your 
questions.

Regards,
BALATON Zoltan