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.
>
>> and if we add an implicit call then we break migration compat
>> for those boards/devices.
>
> Forward migration should still work.  The backwards-incompatible part
> would be that unused memory backend objects would have to be present on
> the destination as well when migrating.  I think it's acceptable.

Migration compatibility turns out not to be quite this simple.
What seems to happen is that the migration code migrates *all*
RAMBlocks, named or otherwise, and fails migration if it
can't match up the names. So you can have one (1) region for
which you forgot to call vmstate_register_ram(), and that will
work (it gets the "" empty string name). If you have more than
one then they'll get confused, especially if they have different
sizes, and loading the VM state will fail.

>>> Only memory_region_init_ram_device_ptr (which sets mr->ram_device) must
>>> not call vmstate_register_ram.  This is a bit ugly because it requires
>>> inlining memory_region_init_ram_ptr into it.
>>>
>>> memory_region_init_ram_from_fd probably needs to be excluded, as well,
>>> based on its sole user.

Note that the above means that since these do create RAMBlocks
their contents *will* be getting migrated.

>> Callers of memory_region_init_ram_ptr() which don't call
>> vmstate_register_ram():
>> hw/misc/mmio_interface.c
>>  -- seems to be an implementation detail of the exceute-from-device
>>     support so maybe it doesn't need migration support ??
> I think so...

I think we need to look rather more closely at how this code
works with migration...

thanks
-- PMM