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

[Igor Mammedov]

On Thu, 6 Jul 2017 17:26:10 +0100
Peter Maydell <address@hidden> wrote:

> 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)
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
>