On 2025/09/06 18:03, BALATON Zoltan wrote:
On Sat, 6 Sep 2025, Akihiko Odaki wrote:
raven-pcihost is not hotpluggable but its instance can still be created
and finalized when processing the device-list-properties QMP command.
Exposing such a temporary instance to AddressSpace should be
avoided because it leaks the instance.
This may suggest there's an issue with freeing address spaces that
should be fixed there at one place if possible rather than adding a new
rule to remember not to create address spaces in init methods. Should
adress space be a child of the device too so it's freed with it? This
series simplifies some devices removing the rule to remember to unparent
memory regions but this now adds another rule to remember avoid creating
address spaces in init so overall we're in the same situation and still
have to work around issues. This trades one workaround for another so
still cannot fix all issues with freeing all created objects.
Making address spaces children will fix memory leaks, but it still
leaves external side effects in init methods. Avoiding external side
effect in init methods is necessary because devices can be created and
removed at any time for introspection and it is not a new rule.
It is not necessary to remember to avoid creating address spaces in
init; roughly speaking, int methods should only add properties and
anything else should be done during realization.
"[PATCH v2 0/3] memory: Stop piggybacking on memory region owners" will
cause an assertion failure if someone still manages to create an address
space accidentally in init, so it is practically not possible to make
such a mistake.
This series do not remove the rule to remember to unparent memory
regions; it instead removes the rule to remember to delete memory
regions from containers, which are distinct operations. And you still
cannot add memory regions to containers during initialization; you
should instead add them during realization just as like address space
creation.
Expose instances to the AddressSpace at their realization time so that
it won't happen for the temporary instances.
I had a series here: https://patchew.org/QEMU/
cover.1751493467.git.bala...@eik.bme.hu/
that changes this for raven (among other clean ups) but I could not get
that merged in the last devel cycle because of PPC being a bit
unmaintained. I'd prefer that series to be taken first instead of this
patch so I don't have to rebase that.
Looking at the series, "[PATCH v2 13/14] hw/pci-host/raven: Do not map
regions in init method" moves memory_region_init() and
memory_region_init_io() from raven_pcihost_initfn() to
raven_pcihost_realize(). This should be avoided because these function
calls add memory regions as child properties, which may be introspected
without realization. Perhaps you may drop the patch and rebase your
series on top of this patch, or let me rebase this patch on that series
without it.
I wrote a documentation update to reflect my understanding of
initialization and realization so please check it out too.
https://patchew.org/QEMU/20250908-qdev-v1-1-df236f7ce...@rsg.ci.i.u-tokyo.ac.jp/
Regards,
Akihiko Odaki
