On Wed, 28 Jan 2026, Akihiko Odaki wrote:
On 2026/01/28 5:22, BALATON Zoltan wrote:
On Tue, 27 Jan 2026, Peter Xu wrote:
On Tue, Jan 27, 2026 at 03:10:28PM +0100, BALATON Zoltan wrote:
This series does not convert everything but opens the way to do that and
simplify objects by only storing things in the state that are really
needed
by the object and let QOM manage what it can. I'd really like to avoid
having a list of unused stuff in the state struct just because I can't
use
something that works and documented just no API to actually use it. Cf.
sii3112 state after this series:
struct SiI3112PCIState {
PCIIDEState i;
SiI3112Regs regs[2];
};
vs. your standard:
struct SiI3112PCIState {
PCIIDEState i;
MemoryRegion mmio;
MemoryRegion bar0;
MemoryRegion bar1;
MemoryRegion bar2;
MemoryRegion bar3;
MemoryRegion bar4;
SiI3112Regs regs[2];
};
The latter solution is fine at least to me.
I respect your preference on how it can be done, but IMHO it is
subjective,
and I tend to agree with Peter that we should try to stick with one way of
solving problems, unless very necessary.
In the last three patches the MRs do not look need dynamic deallocation to
me at all.. Introducing fully dynamic MR allocations only for those is an
overkill to me. I'm not fully convinced we need to merge 600+ new LOCs
for
new APIs just for them.
It's not a new API in the sense that it's the same as existing
memory_region_init functions just a variant of it to allow using QOM to
manage the lifetime of the memory region which the current API does not
allow. Also this series does not introduce this dynamic lifetime
management, it's already implemented and documented just there's currently
no way to use it. The actual change is much less, most of the lines are
just the documentation comments. If it's a problem to increase
documentation size I could make it much shorter not duplicating a separate
doc comment for these just mention in the corresponding memory_region_init
doc comment that there's also an analogous memory_region_new variant of it.
That also shows there's no new API just two ways of using the same API.
We have real but rare users of dynamically allocated MRs, currently AFAIU
Since it's already implemented why not use it if it makes the object state
simpler? Adding additional fields that would already be managed by QOM
seems to be unnecessary duplication and complication to me so I'd like to
avoid that and keep the subclass simple.
I feel the discussion is going a wrong direction. This thread and the cover
letter talks different things as the motivation of this series; The cover
letter says the documentation mismatches with the implementation and that is
the motivation for this series. This thread talks about duplicate memory
management and extra fields, which is not mentioned in the cover letter.
Besides, the "extra fields" problem should have been already discussed in [1]
and I don't see a new argument here. So, in summary, for a constructive
discussion I think:
- There should be a consistent goal of discussion or patch series
(documentation mismatch or extra fields) and
- A new point needs to be made; reminding the points of past discussion
may be worthwhile but is insufficient.
[1]
https://lore.kernel.org/qemu-devel/ceda4c28887c40e1c8eae3f561ee381ca98b0484.1761346145.git.bala...@eik.bme.hu/
Adding some QOM people in case they have something to add.
OK I try to summarise the motivation again:
1. Documentation in docs/devel/memory.rst says that memory regions'
lifecycle is managed by QOM and they are freed with their owner or when
nothing else uses them. This is also already implemented for a long time
as described but cannot be used because the only constructors available
kill this feature when calling object_initialize that clears the free
function added by object_new. (The life time management is implemented
through adding memory regions as children to the owner and unparenting
them on freeing the owner which decreases ref count of the memory region
and will free it when nothing else references it as far as I can tell.)
2. This also allows to keep subclasses' state simpler as demonstrated by
the last two patches by allowing QOM to manage memory regions as
originally intended (based on the documentation and implementation) and
not needing to embed memory regions not otherwise needed in the subclass
as they are already handled by super classes (e.g. registering as PCI
regions or sysbus mmio regions and their lifetime managed by QOM; this way
the subclass only contains what it has to add to the superclass and kept
simpler as expected in an object oriented design).
So the motivation is to match documentation and allow using already
implemented memory region lifecycle management to make device models
simpler.
These are my motivation for this change. What is the motivation for using
embedded memory regions instead and against this change? If that is found
to be a better way then the lifecycle management as implementated and
documentated is likely no longer needed as it cannot and will not be used
with embedded memory regions that are initialsed with object_initialize
and thus could be removed. But I think that would lead to more complex
device models not using what QOM and QDev was meant to help with. (More
complex device models is a problem because they are harder to understand
and find bugs in them having additional things that disctract from actual
functionality of the subclass so I'd like to make them as simple as
possible only containing things that are related to the subclass. So
less QOM boilet plate, less additional static fields in the state is the
better.)
Regards,
BALATON Zoltan
