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é <[email protected]>
Signed-off-by: BALATON Zoltan <[email protected]>
---
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
