On Mon, Mar 02, 2026 at 04:20:10PM +0100, BALATON Zoltan wrote:
> On Mon, 2 Mar 2026, Akihiko Odaki wrote:
> > On 2026/03/02 21:26, BALATON Zoltan wrote:
> > > On Mon, 2 Mar 2026, Akihiko Odaki wrote:
> > > > On 2026/02/26 1:30, Peter Xu wrote:
> > > > > On Tue, Feb 24, 2026 at 06:44:58PM +0100, BALATON Zoltan wrote:
> > > > > > On Tue, 24 Feb 2026, Peter Xu wrote:
> > > > > > > On Thu, Feb 05, 2026 at 02:13:18AM +0100, BALATON Zoltan wrote:
> > > > > > > > v4:
> > > > > > > > - separate patch converting Sun machines from
> > > > > > > > memory_region_init_ram_nomigrate
> > > > > > > > - split helper to init ram into two functions:
> > > > > > > > setup and error_propagate
> > > > > > > > - also use memory_region_init_io in
> > > > > > > > memory_region_init_ram_device_ptr
> > > > > > > >
> > > > > > > > v3:
> > > > > > > > - rebased on master after some patches were merged
> > > > > > > > - drop some more line from memory-region-housekeeping.cocci
> > > > > > > > - added comment to explain what factored out helper does
> > > > > > > > - some more clean ups included
> > > > > > > >
> > > > > > > > BALATON Zoltan (8):
> > > > > > > > hw/display/{cg3,tcx}: Do not use
> > > > > > > > memory_region_init_rom_nomigrate()
> > > > > > > > memory: Remove memory_region_init_rom_nomigrate()
> > > > > > > > sun4m,sun4u,tcx: Do not use
> > > > > > > > memory_region_init_ram_nomigrate()
> > > > > > > > memory: Remove memory_region_init_ram_nomigrate()
> > > > > > >
> > > > > > > Could you help explain why we need to remove the
> > > > > > > above two small helpers?
> > > > > >
> > > > > > Ideally we should remove all of the nomigrate variants as there are
> > > > > > only
> > > > > > about 3 uses left, everything else is already moved to
> > > > > > the init_{ram,rom}
> > > > > > variants that register the memory region for migration.
> > > > > > The is probably one
> > > > > > legitimate use that is served by memory_region_init_flags_nomigrate
> > > > > > and
> > > > > > other 3 uses are the xtfpga which I think is just old code and does
> > > > > > not
> > > > > > support migration so it will be converted in the second series to
> > > > > > memory_region_new as the maintainers did not say anyting to that so
> > > > > > far.
> > > > > > Then there's one in hw/display/vga but that's gated
> > > > > > behind a compatibility
> > > > > > switch that's only set in 2.12 (global-vmstate=true) and
> > > > > > earlier. This is
> > > > > > not active any more but cannot yet be removed as there
> > > > > > are eariler compat
> > > > > > props still need cleaning up but the last visible
> > > > > > machine version is 5.1 now
> > > > > > so this is long deprecated and not used. And there are
> > > > > > these Sun machines
> > > > > > that I've converted in the first version but Mark said
> > > > > > he'd like to keep the
> > > > > > current behaviour until it's the last user left. See:
> > > > > > https://
> > > > > > patchew.org/QEMU/[email protected]/
> > > > > > b530cf54609b853c6b34306fd4aa3ce2f8087d05.1769703287.git.bala...@eik.bme.hu/
> > > > > >
> > > > > > > Not saying that we can't remove them, but I don't yet see the
> > > > > > > point of
> > > > > > > open-code those readonly=true setup either, or any
> > > > > > > problem with having the
> > > > > > > smaller helpers when ram_flags=0. Small helpers
> > > > > > > sometimes help readability.
> > > > > > >
> > > > > > > Some of them are going backwards againist what the cocci scripts
> > > > > > > were
> > > > > > > suggesting previously, like:
> > > > > > >
> > > > > > > - memory_region_init_ram_nomigrate(E1, E2, E3, E4, E5);
> > > > > > > + memory_region_init_rom_nomigrate(E1, E2, E3, E4, E5);
> > > > > > > ... WHEN != E1
> > > > > > > - memory_region_set_readonly(E1, true);
> > > > > > >
> > > > > > > So you suggest open-code instead. Why?
> > > > > >
> > > > > > This is temporary until the usage in vga is gone with
> > > > > > hw_compat_2_12 then
> > > > > > the Sun machines will be the last users so they can also be
> > > > > > converted to
> > > > > > init_{ram,rom} (unless Mark changes his mind and agrees
> > > > > > changing these now
> > > > > > as was in the first version).
> > > > > >
> > > > > > > Is it required for your follow up cleanups?
> > > > > >
> > > > > > It makes those simpler as then we don't need to add
> > > > > > memory_region_new
> > > > > > variants for the nomigrate versions that we would remove later
> > > > > > anyway.
> > > > >
> > > > > Personally I don't see the memory_region_new() whole thing solid yet,
> > > > > but
> > > > > still during review. I shared my thoughts. So far, I'm not fully
> > > > > convinced, but I also don't have the full picture to say no, either.
> > > > > We
> > > > > can wait for others to chime in with more thoughts.
> > > > >
> > > > > For this series, I would prefer if we can discuss it separately from
> > > > > the
> > > > > other work.
> > > > >
> > > > > Removing _nomigrate() variances makes sense to me in general, it
> > > > > means we
> > > > > want to by default suggest new devices only use RAM regions that is
> > > > > migratable, with proper owner device specified along with
> > > > > the ramblock name
> > > > > (as part of migration API).
> > > > >
> > > > > However, we have two sets of these _nomigrate APIs:
> > > > >
> > > > > (1) memory_region_init_r[ao]m_nomigrate
> > > > > (2) memory_region_init_r[ao]m_flags_nomigrate
> > > > >
> > > > > What I am still confused about is why this series used (2) to replace
> > > > > (1)
> > > > > rather than removing both. I don't understand how this helps if we
> > > > > still
> > > > > have (2) around: it means new devices at least can still use
> > > > > (2) to bypass
> > > > > migration.
> > > > >
> > > > > There's actually one way to enforce migratable RAMs, taking
> > > > > ROM as example,
> > > > > it could be:
> > > > >
> > > > > memory_region_init_rom_internal(..., global_vmstate=XXX)
> > > > >
> > > > > Then memory_region_init_rom() should use that, setting
> > > > > global_vmstate=off.
> > > > >
> > > > > Then we can unexport/remove memory_region_init_rom_[flag_]nomigrate()
> > > > > but
> > > > > use that _internal() in relevant paths; we should also mark
> > > > > the _internal()
> > > > > functions to say "new code is suggested to use the normal
> > > > > version". But I
> > > > > do not know if that is a slight overkill just for this..
> > > > >
> > > > > If the other series (to avoid introducing new APIs) is the
> > > > > only reason for
> > > > > this part, then IMHO this part of the series needs to be in the other
> > > > > series, not this one. The last cleanup patches look more self
> > > > > contained
> > > > > OTOH to me, if the whole purpose of the latter half was code
> > > > > dedup, then it
> > > > > makes more sense at least to me.
> > > >
> > > > +1 for what Peter Xu suggested. Most part of this series looks
> > > > independently useful, but I don't think the removal of
> > > > memory_region_init_ram_nomigrate() is.
> > >
> > > But if there are no useful uses why keep it? This version shows that
> > > converting the Sun machines as Peter Maydell suggested only leaves
> > > very few uses that can be removed eventually too after some further
> > > clean up that I don't want to take up now. Do you have any usage in
> > > mind that needs these nomigrate variants?
> >
> > Well, my threshold for "useful" is > 1 uses, but maintainers may have
> > other preferences.
Personally I share the same preference..
When >1 it means we can dedup some code with a function and I normally
think it worthwhile. That's also why I don't understand why we need to
open code _set_readonly() in some of this series.
Even if for the sake of a new set of APIs, it doesn't need to provide every
single API that we have had in the old set if we don't need it, IMHO.
>
> After this series when converting the Sun machines as in v5 the only
> nomigrate variant left is memory_region_init_ram_flags_nomigrate that is
> equivalent to memory_region_init_ram_nomigrate with flags=0 which appears
> outside of memory.c only in hw/xtensa/xtfpga.c (that will be converted in
> the next series to memory_region_new) and in hw/display/vga.c that can be
> removed when the hw_compat_2_12 is removed. There are two more uses of
> memory_region_init_ram_flags_nomigrate in backends/hostmem-ram.c and
> hw/m68k/next-cube.c but those have non-0 flags parameter. Therefore I think
> there's no need to keep other nomigrate variants than
> memory_region_init_ram_flags_nomigrate as the only two uses outside of
> memory.c that would need memory_region_init_ram_nomigrate are to be removed
> soon and can use flags=0 until then.
>
> I don't know if the other two uses with non-0 flags can be replaced and
> memory_region_init_ram_flags_nomigrate made internal to memory.c but that's
> beyond the scope of this series and could be considered separately. Creating
> nomigrate memory region is not something that should be done or need several
> functions and the very few cases where this might be needed can use
> memory_region_init_ram_flags_nomigrate.
The hostmem-ram.c one is slightly special, when created as an object it
still doesn't know its parent device.
I still think it might be helpful if you can split your "cleanup" patches
out of this series. Note that softfreeze will be next week.
Thanks,
--
Peter Xu
