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

but the need for the second function call is not obvious and if
omitted the bug is subtle (possible migration failure). This series
wraps the two calls up into a single new function
memory_region_allocate_aux_memory(), named to parallel the existing
memory_region_allocate_system_memory().

Patch 1 documents memory_region_allocate_system_memory() which
has a couple of non-obvious wrinkles. Patch 2 implements the new
utility function. Patch 3 changes a lot of callsites to use it
with the aid of the included coccinelle patch. (I think most
of the remaining callsites of vmstate_register_ram_global() are
not changed just because they report the error up to their caller
rather than using error_fatal. I'm not sure what kind of error you
might get back that wasn't "out of memory", which we usually make
fatal anyway, so perhaps we could change those callsites too.)

thanks
-- PMM

Peter Maydell (3):
  include/hw/boards.h: Document memory_region_allocate_system_memory()
  memory.h: Add new utility function memory_region_allocate_aux_memory()
  hw: Use new memory_region_allocate_aux_memory() function

 include/exec/memory.h                     | 23 +++++++++++++++++++++++
 include/hw/boards.h                       | 29 +++++++++++++++++++++++++++++
 hw/arm/exynos4210.c                       | 10 ++++------
 hw/arm/exynos4_boards.c                   | 12 +++++-------
 hw/arm/integratorcp.c                     |  5 ++---
 hw/arm/mainstone.c                        |  5 ++---
 hw/arm/musicpal.c                         |  5 ++---
 hw/arm/omap1.c                            |  5 ++---
 hw/arm/omap2.c                            |  5 ++---
 hw/arm/omap_sx1.c                         | 10 ++++------
 hw/arm/palm.c                             |  4 +---
 hw/arm/pxa2xx.c                           | 20 ++++++++------------
 hw/arm/realview.c                         | 14 +++++---------
 hw/arm/spitz.c                            |  3 +--
 hw/arm/stellaris.c                        |  9 +++------
 hw/arm/stm32f205_soc.c                    | 10 +++-------
 hw/arm/tosa.c                             |  3 +--
 hw/arm/vexpress.c                         | 12 +++---------
 hw/arm/virt.c                             |  3 +--
 hw/arm/xilinx_zynq.c                      |  5 ++---
 hw/arm/xlnx-zynqmp.c                      |  5 ++---
 hw/block/onenand.c                        |  5 ++---
 hw/cris/axis_dev88.c                      |  5 ++---
 hw/display/cg3.c                          |  5 ++---
 hw/display/sm501.c                        |  5 ++---
 hw/display/tc6393xb.c                     |  5 ++---
 hw/display/tcx.c                          |  5 ++---
 hw/display/vmware_vga.c                   |  5 ++---
 hw/i386/pc.c                              |  5 ++---
 hw/i386/pc_sysfw.c                        |  8 +++-----
 hw/i386/xen/xen-hvm.c                     |  4 +---
 hw/input/milkymist-softusb.c              | 10 ++++------
 hw/m68k/an5206.c                          |  3 +--
 hw/m68k/mcf5208.c                         |  3 +--
 hw/microblaze/petalogix_ml605_mmu.c       | 11 +++++------
 hw/microblaze/petalogix_s3adsp1800_mmu.c  | 12 +++++-------
 hw/mips/mips_fulong2e.c                   |  4 +---
 hw/mips/mips_jazz.c                       | 10 ++++------
 hw/mips/mips_mipssim.c                    |  5 ++---
 hw/mips/mips_r4k.c                        |  5 ++---
 hw/moxie/moxiesim.c                       |  6 ++----
 hw/net/milkymist-minimac2.c               |  6 +++---
 hw/openrisc/openrisc_sim.c                |  3 +--
 hw/pci-host/prep.c                        |  5 +----
 hw/ppc/mac_newworld.c                     |  5 ++---
 hw/ppc/mac_oldworld.c                     |  5 ++---
 hw/ppc/ppc405_boards.c                    | 14 +++++---------
 hw/ppc/ppc405_uc.c                        |  5 ++---
 hw/s390x/sclp.c                           |  5 ++---
 hw/sh4/r2d.c                              |  3 +--
 hw/sh4/shix.c                             | 13 +++++--------
 hw/sparc/leon3.c                          |  3 +--
 hw/sparc/sun4m.c                          | 13 +++++--------
 hw/sparc64/sun4u.c                        | 10 ++++------
 hw/tricore/tricore_testboard.c            | 30 ++++++++++++------------------
 hw/unicore32/puv3.c                       |  4 +---
 hw/xtensa/sim.c                           |  6 ++----
 hw/xtensa/xtfpga.c                        | 14 +++++---------
 memory.c                                  |  8 ++++++++
 scripts/coccinelle/allocate_aux_mem.cocci | 14 ++++++++++++++
 60 files changed, 228 insertions(+), 256 deletions(-)
 create mode 100644 scripts/coccinelle/allocate_aux_mem.cocci

-- 
2.7.4

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.

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.

The destructor then can unregister the RAM.  Maybe there need to be two
separate destructors.

Unlike your patch 3/3, I suspect all occurrences have to be fixed in the
same patch that introduces the change, so the Coccinelle patch would
become something like

@@
(
 memory_region_init_ram(MR, OWNER, NAME, SIZE, ERR);
|
 memory_region_init_ram_ptr(MR, OWNER, NAME, SIZE, PTR);
|
 memory_region_init_ram_from_file(MR, OWNER, NAME, SHARE, PATH, ERR);
)
 ... when exists
-vmstate_register_ram(MR, OWNER);
@@
(
 memory_region_init_ram(MR, NULL, NAME, SIZE, ERR);
|
 memory_region_init_ram_ptr(MR, NULL, NAME, SIZE, PTR);
|
 memory_region_init_ram_from_file(MR, NULL, NAME, SHARE, PATH, ERR);
)
 ... when exists
-vmstate_register_ram_global(MR);


with the initial disjunction extended to all other functions if there
are any I forgot.  But it should work.

> but the need for the second function call is not obvious and if
> omitted the bug is subtle (possible migration failure). This series
> wraps the two calls up into a single new function
> memory_region_allocate_aux_memory(), named to parallel the existing
> memory_region_allocate_system_memory().
> 
> Patch 1 documents memory_region_allocate_system_memory() which
> has a couple of non-obvious wrinkles. Patch 2 implements the new
> utility function. Patch 3 changes a lot of callsites to use it
> with the aid of the included coccinelle patch. (I think most
> of the remaining callsites of vmstate_register_ram_global() are
> not changed just because they report the error up to their caller
> rather than using error_fatal. I'm not sure what kind of error you
> might get back that wasn't "out of memory", which we usually make
> fatal anyway, so perhaps we could change those callsites too.)

We don't make out of memory fatal on very large allocations, so I think
it would be better if memory_region_allocate_aux_memory had an Error**
argument and did propagation correctly.  It's reasonable to avoid
exiting when memory is allocated from e.g. device hotplug.  But
hopefully you can implement the alternative plan above and
vmstate_register_ram can become an implementation detail of memory.c.

Paolo

> thanks
> -- PMM
> 
> Peter Maydell (3):
>   include/hw/boards.h: Document memory_region_allocate_system_memory()
>   memory.h: Add new utility function memory_region_allocate_aux_memory()
>   hw: Use new memory_region_allocate_aux_memory() function
> 
>  include/exec/memory.h                     | 23 +++++++++++++++++++++++
>  include/hw/boards.h                       | 29 +++++++++++++++++++++++++++++
>  hw/arm/exynos4210.c                       | 10 ++++------
>  hw/arm/exynos4_boards.c                   | 12 +++++-------
>  hw/arm/integratorcp.c                     |  5 ++---
>  hw/arm/mainstone.c                        |  5 ++---
>  hw/arm/musicpal.c                         |  5 ++---
>  hw/arm/omap1.c                            |  5 ++---
>  hw/arm/omap2.c                            |  5 ++---
>  hw/arm/omap_sx1.c                         | 10 ++++------
>  hw/arm/palm.c                             |  4 +---
>  hw/arm/pxa2xx.c                           | 20 ++++++++------------
>  hw/arm/realview.c                         | 14 +++++---------
>  hw/arm/spitz.c                            |  3 +--
>  hw/arm/stellaris.c                        |  9 +++------
>  hw/arm/stm32f205_soc.c                    | 10 +++-------
>  hw/arm/tosa.c                             |  3 +--
>  hw/arm/vexpress.c                         | 12 +++---------
>  hw/arm/virt.c                             |  3 +--
>  hw/arm/xilinx_zynq.c                      |  5 ++---
>  hw/arm/xlnx-zynqmp.c                      |  5 ++---
>  hw/block/onenand.c                        |  5 ++---
>  hw/cris/axis_dev88.c                      |  5 ++---
>  hw/display/cg3.c                          |  5 ++---
>  hw/display/sm501.c                        |  5 ++---
>  hw/display/tc6393xb.c                     |  5 ++---
>  hw/display/tcx.c                          |  5 ++---
>  hw/display/vmware_vga.c                   |  5 ++---
>  hw/i386/pc.c                              |  5 ++---
>  hw/i386/pc_sysfw.c                        |  8 +++-----
>  hw/i386/xen/xen-hvm.c                     |  4 +---
>  hw/input/milkymist-softusb.c              | 10 ++++------
>  hw/m68k/an5206.c                          |  3 +--
>  hw/m68k/mcf5208.c                         |  3 +--
>  hw/microblaze/petalogix_ml605_mmu.c       | 11 +++++------
>  hw/microblaze/petalogix_s3adsp1800_mmu.c  | 12 +++++-------
>  hw/mips/mips_fulong2e.c                   |  4 +---
>  hw/mips/mips_jazz.c                       | 10 ++++------
>  hw/mips/mips_mipssim.c                    |  5 ++---
>  hw/mips/mips_r4k.c                        |  5 ++---
>  hw/moxie/moxiesim.c                       |  6 ++----
>  hw/net/milkymist-minimac2.c               |  6 +++---
>  hw/openrisc/openrisc_sim.c                |  3 +--
>  hw/pci-host/prep.c                        |  5 +----
>  hw/ppc/mac_newworld.c                     |  5 ++---
>  hw/ppc/mac_oldworld.c                     |  5 ++---
>  hw/ppc/ppc405_boards.c                    | 14 +++++---------
>  hw/ppc/ppc405_uc.c                        |  5 ++---
>  hw/s390x/sclp.c                           |  5 ++---
>  hw/sh4/r2d.c                              |  3 +--
>  hw/sh4/shix.c                             | 13 +++++--------
>  hw/sparc/leon3.c                          |  3 +--
>  hw/sparc/sun4m.c                          | 13 +++++--------
>  hw/sparc64/sun4u.c                        | 10 ++++------
>  hw/tricore/tricore_testboard.c            | 30 ++++++++++++------------------
>  hw/unicore32/puv3.c                       |  4 +---
>  hw/xtensa/sim.c                           |  6 ++----
>  hw/xtensa/xtfpga.c                        | 14 +++++---------
>  memory.c                                  |  8 ++++++++
>  scripts/coccinelle/allocate_aux_mem.cocci | 14 ++++++++++++++
>  60 files changed, 228 insertions(+), 256 deletions(-)
>  create mode 100644 scripts/coccinelle/allocate_aux_mem.cocci
> 

On 5 July 2017 at 13:21, Paolo Bonzini <pbonzini@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?)

and if we add an implicit call then we break migration compat
for those boards/devices.

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

Callers of memory_region_init_ram_from_file() which don't
call vmstate_register_ram():
backends/hostmem-ram.c

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

thanks
-- PMM

On 06/07/2017 16:52, Peter Maydell wrote:
> On 5 July 2017 at 13:21, Paolo Bonzini <pbonzini@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.

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

>> 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.
> 
> Callers of memory_region_init_ram_from_file() which don't
> call vmstate_register_ram():
> backends/hostmem-ram.c

Same as above.

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

Paolo

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

On 06/07/2017 18:26, Peter Maydell wrote:
> 
> 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.

memory_region_init_ram_from_file theoretically would also fit this
description, but all of its callers would use the _nomigrate version.

Paolo

> "Be consistent for
> the full set of functions" would be the other obvious approach;
> I don't think I care too much either way.

On Thu, 6 Jul 2017 17:26:10 +0100
Peter Maydell <peter.maydell@linaro.org> wrote:

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

On 7 July 2017 at 14:18, Igor Mammedov <imammedo@redhat.com> wrote:
> On Thu, 6 Jul 2017 17:26:10 +0100
> Peter Maydell <peter.maydell@linaro.org> wrote:
>>  * 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()

I think this is the wrong way round, because it leads to lots of
bugs. Basically it's really not obvious that you (as the author
of board or device code) need to also call a second function
to not just create a RAM region but also make it work correctly
under migration, and migration is often not tested. Working
through the automatic renames here I've found a small pile of
cases where the memory region was just not registered for
migration at all, or where it was registered using _global()
rather than passing the device as owner (which means the device
can only be created once).

Conversely, the cases which really really want to handle the
migration of the contents of the RAM MemoryRegion themselves
are very few in number, the authors of that code generally
know what they're doing, and the fact that they call a function
named foo_nomigrate() provides them with a helpful clue that
they need to think about and handle migration themselves.

This falls I think under item 7 ("the obvious use is (probably)
the correct one") in Rusty Russell's set of hard-to-misuse API
design guidelines:
http://ozlabs.org/~rusty/index.cgi/tech/2008-03-30.html

thanks
-- PMM

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

On Thu, 6 Jul 2017 15:52:44 +0100
Peter Maydell <peter.maydell@linaro.org> wrote:

> On 5 July 2017 at 13:21, Paolo Bonzini <pbonzini@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?)
> 
> and if we add an implicit call then we break migration compat
> for those boards/devices.
> 
> > 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.  
> 
> Callers of memory_region_init_ram_from_file() which don't
> call vmstate_register_ram():
> backends/hostmem-ram.c

if backend is used by pc-dimm,
then it's pc-dimm job to (un)register vmstate:

pc_dimm_memory_plug()->vmstate_register_ram(vmstate_mr, dev);

> 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 ??
> 
> thanks
> -- PMM
> 

On 5 July 2017 at 13:21, Paolo Bonzini <pbonzini@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.

Slightly awkward because owner is an Object but vmstate_register_ram()
needs a DeviceState. Is this OK, or too much magic?

    DeviceState *owner_dev;
    Error *err = NULL;

    memory_region_init_ram(mr, owner, name, ram_size, &err);
    if (err) {
        error_propagate(errp, err);
        return;
    }
    /* Note that owner_dev may be NULL if owner is not a DeviceState;
     * in that case this is equivalent to calling vmstate_register_ram_global().
     */
    owner_dev = object_dynamic_cast(owner, TYPE_DEVICE);
    vmstate_register_ram(mr, owner_dev);

thanks
-- PMM

On 06/07/2017 19:13, Peter Maydell wrote:
> Slightly awkward because owner is an Object but vmstate_register_ram()
> needs a DeviceState. Is this OK, or too much magic?
> 
>     DeviceState *owner_dev;
>     Error *err = NULL;
> 
>     memory_region_init_ram(mr, owner, name, ram_size, &err);
>     if (err) {
>         error_propagate(errp, err);
>         return;
>     }
>     /* Note that owner_dev may be NULL if owner is not a DeviceState;
>      * in that case this is equivalent to calling vmstate_register_ram_global().
>      */
>     owner_dev = object_dynamic_cast(owner, TYPE_DEVICE);
>     vmstate_register_ram(mr, owner_dev);

Maybe, for memory_region_init_ram only, the owner argument can be made a
DeviceState (or NULL)?

Paolo

On 6 July 2017 at 18:26, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 06/07/2017 19:13, Peter Maydell wrote:
>> Slightly awkward because owner is an Object but vmstate_register_ram()
>> needs a DeviceState. Is this OK, or too much magic?
>>
>>     DeviceState *owner_dev;
>>     Error *err = NULL;
>>
>>     memory_region_init_ram(mr, owner, name, ram_size, &err);
>>     if (err) {
>>         error_propagate(errp, err);
>>         return;
>>     }
>>     /* Note that owner_dev may be NULL if owner is not a DeviceState;
>>      * in that case this is equivalent to calling vmstate_register_ram_global().
>>      */
>>     owner_dev = object_dynamic_cast(owner, TYPE_DEVICE);
>>     vmstate_register_ram(mr, owner_dev);
>
> Maybe, for memory_region_init_ram only, the owner argument can be made a
> DeviceState (or NULL)?

Something that might influence things here -- of the twelve
callers of vmstate_register_ram(), only 6 use an 'owner'
pointer that's the same as the one they use for initializing
the memory region (and 3 of those are using _init_rom or
_init_rom_device rather than init_ram directly).

thanks
-- PMM

On 6 July 2017 at 18:47, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 6 July 2017 at 18:26, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> Maybe, for memory_region_init_ram only, the owner argument can be made a
>> DeviceState (or NULL)?
>
> Something that might influence things here -- of the twelve
> callers of vmstate_register_ram(), only 6 use an 'owner'
> pointer that's the same as the one they use for initializing
> the memory region (and 3 of those are using _init_rom or
> _init_rom_device rather than init_ram directly).

Conversely there are 14 or so places that init a RAM MR
with a non-NULL owner but then use vmstate_register_ram_global
rather than vmstate_register_ram, and so which would be
stuck using the old _nomigrate() function if we make it
use the owner pointer.

I guess we should think about the long term and design
the API to encourage the behaviour we want for new code,
rather than to catch as much of possible of existing
usage, though?

thanks
-- PMM

On 07/07/2017 12:43, Peter Maydell wrote:
> On 6 July 2017 at 18:47, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 6 July 2017 at 18:26, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>> Maybe, for memory_region_init_ram only, the owner argument can be made a
>>> DeviceState (or NULL)?
>>
>> Something that might influence things here -- of the twelve
>> callers of vmstate_register_ram(), only 6 use an 'owner'
>> pointer that's the same as the one they use for initializing
>> the memory region (and 3 of those are using _init_rom or
>> _init_rom_device rather than init_ram directly).
> 
> Conversely there are 14 or so places that init a RAM MR
> with a non-NULL owner but then use vmstate_register_ram_global
> rather than vmstate_register_ram, and so which would be
> stuck using the old _nomigrate() function if we make it
> use the owner pointer.

I think whenever feasible we should pay the price of breaking migration.
 I found these:

- aspeed_soc_realize

- integratorcm_init

- onenand_initfn (nseries)

- cg3_initfn (one init_ram in there is not using owner, might be changed
as well)

- sm501_init

- tcx_initfn

- milkymist_softusb_init

- milkymist_minimac2_init

- raven_realize

- idreg_init1

- afx_init1

- prom_init1 (two functions with the same name in two files)

- ram_realize

- lx60_net_init

The only problematic one for backwards compatibility is rom_set_mr.

> I guess we should think about the long term and design
> the API to encourage the behaviour we want for new code,
> rather than to catch as much of possible of existing
> usage, though?

Yes, though that leaves the possibility of people copying from the wrong
code, so if we can break migration compatibility that would be better in
the long term.

Paolo

On 6 July 2017 at 18:26, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 06/07/2017 19:13, Peter Maydell wrote:
>> Slightly awkward because owner is an Object but vmstate_register_ram()
>> needs a DeviceState. Is this OK, or too much magic?
>>
>>     DeviceState *owner_dev;
>>     Error *err = NULL;
>>
>>     memory_region_init_ram(mr, owner, name, ram_size, &err);
>>     if (err) {
>>         error_propagate(errp, err);
>>         return;
>>     }
>>     /* Note that owner_dev may be NULL if owner is not a DeviceState;
>>      * in that case this is equivalent to calling vmstate_register_ram_global().
>>      */
>>     owner_dev = object_dynamic_cast(owner, TYPE_DEVICE);
>>     vmstate_register_ram(mr, owner_dev);
>
> Maybe, for memory_region_init_ram only, the owner argument can be made a
> DeviceState (or NULL)?

Hmm. I don't much like the way that breaks the symmetry of the
API with the other memory region init functions, and it makes
it harder to script the conversion. I'd rather keep it as an
Object*, especially since the only purpose of the pointer is to
make the RAM name unique for migration purposes.
(Better still would be if we had a uniquification mechanism
for Objects...)

Incidentally if you use vmstate_register_ram_global() in
a device then it implicitly makes the device "only usable once"
since if you create a 2nd one it'll abort in qemu_ram_set_idstr().
For instance:

qemu-system-ppc -device sm501 -device sm501
RAMBlock "sm501.local" already registered, abort!
Aborted (core dumped)

thanks
-- PMM

On 07/07/2017 13:58, Peter Maydell wrote:
>>>
>>>     memory_region_init_ram(mr, owner, name, ram_size, &err);
>>>     if (err) {
>>>         error_propagate(errp, err);
>>>         return;
>>>     }
>>>     /* Note that owner_dev may be NULL if owner is not a DeviceState;
>>>      * in that case this is equivalent to calling vmstate_register_ram_global().
>>>      */
>>>     owner_dev = object_dynamic_cast(owner, TYPE_DEVICE);
>>>     vmstate_register_ram(mr, owner_dev);
>> Maybe, for memory_region_init_ram only, the owner argument can be made a
>> DeviceState (or NULL)?
> Hmm. I don't much like the way that breaks the symmetry of the
> API with the other memory region init functions, and it makes
> it harder to script the conversion. I'd rather keep it as an
> Object*, especially since the only purpose of the pointer is to
> make the RAM name unique for migration purposes.

In that case, I would assert the object is a device and add a TODO to
support non-device objects.

Paolo

> (Better still would be if we had a uniquification mechanism
> for Objects...)