On Thu, 6 Jul 2017 17:26:10 +0100 Peter Maydell <peter.mayd...@linaro.org> wrote:
> On 6 July 2017 at 15:56, Paolo Bonzini <pbonz...@redhat.com> wrote: > > > > > > On 06/07/2017 16:52, Peter Maydell wrote: > >> On 5 July 2017 at 13:21, Paolo Bonzini <pbonz...@redhat.com> wrote: > >>> > >>> > >>> On 04/07/2017 19:02, Peter Maydell wrote: > >>>> Many board models and several devices need to create auxiliary > >>>> regions of RAM (in addition to the main lump of 'system' memory), > >>>> to model static RAMs, video memory, ROMs, etc. Currently they do > >>>> this with a sequence like: > >>>> memory_region_init_ram(sram, NULL, "sram", 0x10000, &error_fatal); > >>>> vmstate_register_ram_global(sram); > >>> > >>> Instead of vmstate_register_ram_global, you should use > >>> > >>> vmstate_register_ram(mr, owner); > >>> > >>> You should even do it for all memory regions, probably. > >> > >> This sounds like a good thing, but it's awkward for migration > >> compatibility, because these callers to memory_region_init_ram() > >> don't call vmstate_register_ram(): > >> > >> hw/arm/highbank.c (a bug) > >> hw/mips/mips_malta.c (region is ro) > >> hw/net/dp3893x.c (prom, ro, contains mac address) > >> hw/pci-host/xilinx-pcie.c (dummy region; migrating wouldn't hurt) > >> backends/hostmem-ram.c (bug, or is migration handled elsewhere?) > > > > It's handled by memory_region_allocate_system_memory. > > OK. To sum up an IRC conversation... > > I think that to avoid getting tangled up in trying to fix > or deal with these handful of oddball cases at the same time > as doing a global change to the easy cases, we should: > > * globally rename memory_region_init_ram to memory_region_init_ram_nomigrate > (and document that it is the caller's job to arrange to migrate the RAM) it would be a bit weird when MR created as _nomigrate() is then registered with vmstate_register_ram() and being migrated. maybe other way around, places that don't wanna to use explicit vmstate_register_ram() could replace 2 calls with helper memory_region_init_ram_automigrate() > * add new memory_region_init_ram which does memory_region_init_ram_nomigrate > + vmstate_register_ram > * coccinelle patch to switch to using that new function where possible > * get that lot committed, because it touches so many files and > is a recipe for conflicts > * look at the remaining handful of _nomigrate calls and perhaps > switch them where that would be a bug fix > > Q: should we have _nomigrate versions of any of the other > memory_region_init_ram* functions? I think it makes sense > for only the basic _init_ram to do the migration for you, > because that's the only case where the memory system is > creating the backing storage for the caller, rather than the > caller passing in the backing storage. "Be consistent for > the full set of functions" would be the other obvious approach; > I don't think I care too much either way. I 'd keep current way where memory_region and vmstate APIs are split and consistent in what they are doing instead of making Frankenstein from memory_region API. > thanks > -- PMM >