[PULL,15/19] hw/arm/highbank: don't make sysram 'nomigrate'

Currently we use memory_region_init_ram_nomigrate() to create
the "highbank.sysram" memory region, and we don't manually
register it with vmstate_register_ram(). This currently
means that its contents are migrated but as a ram block
whose name is the empty string; in future it may mean they
are not migrated at all. Use memory_region_init_ram() instead.

Note that this is a cross-version migration compatibility
break for the "highbank" and "midway" machines.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Message-id: 20180420124835.7268-2-peter.maydell@linaro.org
---
 hw/arm/highbank.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

On Thu, Apr 26, 2018 at 10:47 PM, Peter Maydell <peter.maydell@linaro.org>
wrote:

> Currently we use memory_region_init_ram_nomigrate() to create
> the "highbank.sysram" memory region, and we don't manually
> register it with vmstate_register_ram(). This currently
> means that its contents are migrated but as a ram block
> whose name is the empty string; in future it may mean they
> are not migrated at all. Use memory_region_init_ram() instead.
>

It is self evident that memory_region_init_ram() implies normal migration.


> Note that this is a cross-version migration compatibility
> break for the "highbank" and "midway" machines.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>

Reviewed-by: Michael Clark <mjc@sifive.com>


> Message-id: 20180420124835.7268-2-peter.maydell@linaro.org
> ---
>  hw/arm/highbank.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/arm/highbank.c b/hw/arm/highbank.c
> index 1742cf6f6c..88326d1bfd 100644
> --- a/hw/arm/highbank.c
> +++ b/hw/arm/highbank.c
> @@ -291,7 +291,7 @@ static void calxeda_init(MachineState *machine, enum
> cxmachines machine_id)
>      memory_region_add_subregion(sysmem, 0, dram);
>
>      sysram = g_new(MemoryRegion, 1);
> -    memory_region_init_ram_nomigrate(sysram, NULL, "highbank.sysram",
> 0x8000,
> +    memory_region_init_ram(sysram, NULL, "highbank.sysram", 0x8000,
>                             &error_fatal);
>      memory_region_add_subregion(sysmem, 0xfff88000, sysram);
>

magic constants are better as #define or enum but there is a lot of code
that already does this, and that would be a separate change. It's peeking
through in the diff context :-D


>      if (bios_name != NULL) {
> --
> 2.17.0
>
>
>

On 26 April 2018 at 23:03, Michael Clark <mjc@sifive.com> wrote:
>
>
> On Thu, Apr 26, 2018 at 10:47 PM, Peter Maydell <peter.maydell@linaro.org>
> wrote:
>>
>> Currently we use memory_region_init_ram_nomigrate() to create
>> the "highbank.sysram" memory region, and we don't manually
>> register it with vmstate_register_ram(). This currently
>> means that its contents are migrated but as a ram block
>> whose name is the empty string; in future it may mean they
>> are not migrated at all. Use memory_region_init_ram() instead.
>
>
> It is self evident that memory_region_init_ram() implies normal migration.

Yes; using the _nomigrate functions in these files looks odd
but it only looks odd as a result of a refactor we did.

Previously we had:
 * memory_region_init_ram() -- doesn't register the ram for migration
 * everybody has to manually call vmstate_register_ram()
Then we refactored so that:
 * the old memory_region_init_ram() is renamed to _nomigrate()
 * a new memory_region_init_ram() does that plus calls
   vmstate_register_ram() for you
 * callsites that were doing both calls got automatically rewritten
   to call the new memory_region_init_ram()
 * callsites that forgot the vmstate_register_ram() got rewritten
   to call memory_region_init_ram_nomigrate()

So board code that was previously making an easy and hard-to-spot
mistake (forgetting to call vmstate_register_ram()) got rewritten
so the mistake is more obvious (calling memory_region_init_ram_nomigrate
for no good reason).

thanks
-- PMM