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?
(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
