Re: [Qemu-devel] [PATCH 0/3] Add new utility function memory_region_allocate_aux_memory()

[Peter Maydell]

On 6 July 2017 at 15:56, Paolo Bonzini <address@hidden> wrote:
>
>
> On 06/07/2017 16:52, Peter Maydell wrote:
>> On 5 July 2017 at 13:21, Paolo Bonzini <address@hidden> 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)
 * 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.

thanks
-- PMM