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/
Regards,
Akihiko Odaki
