[Qemu-devel] [PATCH 0/3] aspeed, highbank: fix RAM migration oddities

[Qemu-devel] [PATCH 0/3] aspeed, highbank: fix RAM migration oddities

[Peter Maydell]

In a couple of places in the highbank and aspeed boards,
we create RAM regions with memory_region_init_ram_nomigrate()
and then either:
 * don't call vmstate_register_ram()
 * from a device, register it with
   vmstate_register_ram_global()

The first case is a bug. Currently it means that the underlying
RAM block will have a name which is the empty string; it will
migrate, but if there is more than one such RAM block in the
system migration will fail. Future changes to the migration
code are likely to mean that the RAM block isn't migrated at all.

The second case is also a bug: since the name used for the RAM
block is global to all of QEMU, it means that you can't have
more than one instance of the device without QEMU abort()ing
because of the name clash.

Fix both of these by using memory_region_init_ram(), which
automatically registers the RAM block for migration with
a sensible name.

This patchset is a cross-version migration compatibility
break for the boards "palmetto-bmc", "ast2500-evb", "romulus-bmc",
"highbank" and "midway". We don't promise migration compat
for those boards, so this is OK.

NB: I haven't tested this code beyond "does make check pass",
because I don't have a test image for any of these boards.
Testing welcomed :-)

thanks
-- PMM

Peter Maydell (3):
  hw/arm/highbank: don't make sysram 'nomigrate'
  hw/arm/aspeed: don't make 'boot_rom' region 'nomigrate'
  hw/arm/aspeed_soc: don't use vmstate_register_ram_global for SRAM

 hw/arm/aspeed.c     | 2 +-
 hw/arm/aspeed_soc.c | 3 +--
 hw/arm/highbank.c   | 2 +-
 3 files changed, 3 insertions(+), 4 deletions(-)

-- 
2.17.0

[Qemu-devel] [PATCH 1/3] hw/arm/highbank: don't make sysram 'nomigrate'

[Peter Maydell]

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>
---
 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);
     if (bios_name != NULL) {
-- 
2.17.0

[Qemu-devel] [PATCH 2/3] hw/arm/aspeed: don't make 'boot_rom' region 'nomigrate'

[Peter Maydell]

Currently we use memory_region_init_ram_nomigrate() to create
the "aspeed.boot_rom" 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 "palmetto-bmc", "ast2500-evb" and "romulus-bmc" machines.

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

diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index 7088c907bd..aecb3c1e75 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -225,7 +225,7 @@ static void aspeed_board_init(MachineState *machine,
          * SoC and 128MB for the AST2500 SoC, which is twice as big as
          * needed by the flash modules of the Aspeed machines.
          */
-        memory_region_init_rom_nomigrate(boot_rom, OBJECT(bmc), "aspeed.boot_rom",
+        memory_region_init_rom(boot_rom, OBJECT(bmc), "aspeed.boot_rom",
                                fl->size, &error_abort);
         memory_region_add_subregion(get_system_memory(), FIRMWARE_ADDR,
                                     boot_rom);
-- 
2.17.0

[Qemu-devel] [PATCH 3/3] hw/arm/aspeed_soc: don't use vmstate_register_ram_global for SRAM

[Peter Maydell]

Currently we use vmstate_register_ram_global() for the SRAM;
this is not a good idea for devices, because it means that
you can only ever create one instance of the device, as
the second instance would get a RAM block name clash.
Instead, use memory_region_init_ram(), which automatically
registers the RAM block with a local-to-the-device name.

Note that this is a cross-version migration compatibility break
for the "palmetto-bmc", "ast2500-evb" and "romulus-bmc" machines.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/aspeed_soc.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
index 30d25f8b06..407f10d0d4 100644
--- a/hw/arm/aspeed_soc.c
+++ b/hw/arm/aspeed_soc.c
@@ -186,13 +186,12 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
     }
 
     /* SRAM */
-    memory_region_init_ram_nomigrate(&s->sram, OBJECT(dev), "aspeed.sram",
+    memory_region_init_ram(&s->sram, OBJECT(dev), "aspeed.sram",
                            sc->info->sram_size, &err);
     if (err) {
         error_propagate(errp, err);
         return;
     }
-    vmstate_register_ram_global(&s->sram);
     memory_region_add_subregion(get_system_memory(), ASPEED_SOC_SRAM_BASE,
                                 &s->sram);
 
-- 
2.17.0

Re: [Qemu-devel] [PATCH 2/3] hw/arm/aspeed: don't make 'boot_rom' region 'nomigrate'

[Cédric Le Goater]

On 04/20/2018 02:48 PM, Peter Maydell wrote:
> Currently we use memory_region_init_ram_nomigrate() to create
> the "aspeed.boot_rom" 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 "palmetto-bmc", "ast2500-evb" and "romulus-bmc" machines.

Migration does not work for these. 

> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Cédric Le Goater <clg@kaod.org>
Tested-by: Cédric Le Goater <clg@kaod.org>

On palmetto, romulus and ast2500-evb boards 

Thanks

C.

> ---
>  hw/arm/aspeed.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index 7088c907bd..aecb3c1e75 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -225,7 +225,7 @@ static void aspeed_board_init(MachineState *machine,
>           * SoC and 128MB for the AST2500 SoC, which is twice as big as
>           * needed by the flash modules of the Aspeed machines.
>           */
> -        memory_region_init_rom_nomigrate(boot_rom, OBJECT(bmc), "aspeed.boot_rom",
> +        memory_region_init_rom(boot_rom, OBJECT(bmc), "aspeed.boot_rom",
>                                 fl->size, &error_abort);
>          memory_region_add_subregion(get_system_memory(), FIRMWARE_ADDR,
>                                      boot_rom);
> 

Re: [Qemu-devel] [PATCH 3/3] hw/arm/aspeed_soc: don't use vmstate_register_ram_global for SRAM

[Cédric Le Goater]

On 04/20/2018 02:48 PM, Peter Maydell wrote:
> Currently we use vmstate_register_ram_global() for the SRAM;
> this is not a good idea for devices, because it means that
> you can only ever create one instance of the device, as
> the second instance would get a RAM block name clash.
> Instead, use memory_region_init_ram(), which automatically
> registers the RAM block with a local-to-the-device name.
> 
> Note that this is a cross-version migration compatibility break
> for the "palmetto-bmc", "ast2500-evb" and "romulus-bmc" machines.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Cédric Le Goater <clg@kaod.org>
Tested-by: Cédric Le Goater <clg@kaod.org>

On palmetto, romulus and ast2500-evb boards 

Thanks

C.> ---
>  hw/arm/aspeed_soc.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
> index 30d25f8b06..407f10d0d4 100644
> --- a/hw/arm/aspeed_soc.c
> +++ b/hw/arm/aspeed_soc.c
> @@ -186,13 +186,12 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
>      }
>  
>      /* SRAM */
> -    memory_region_init_ram_nomigrate(&s->sram, OBJECT(dev), "aspeed.sram",
> +    memory_region_init_ram(&s->sram, OBJECT(dev), "aspeed.sram",
>                             sc->info->sram_size, &err);
>      if (err) {
>          error_propagate(errp, err);
>          return;
>      }
> -    vmstate_register_ram_global(&s->sram);
>      memory_region_add_subregion(get_system_memory(), ASPEED_SOC_SRAM_BASE,
>                                  &s->sram);
>  
> 

Re: [Qemu-devel] [PATCH 2/3] hw/arm/aspeed: don't make 'boot_rom' region 'nomigrate'

[Peter Maydell]

On 21 April 2018 at 10:07, Cédric Le Goater <clg@kaod.org> wrote:
> On 04/20/2018 02:48 PM, Peter Maydell wrote:
>> Currently we use memory_region_init_ram_nomigrate() to create
>> the "aspeed.boot_rom" 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 "palmetto-bmc", "ast2500-evb" and "romulus-bmc" machines.
>
> Migration does not work for these.

Well, at least we're not breaking anything :-)
Do you know why migration doesn't work?

thanks
-- PMM

Re: [Qemu-devel] [PATCH 2/3] hw/arm/aspeed: don't make 'boot_rom' region 'nomigrate'

[Cédric Le Goater]

On 04/21/2018 11:34 AM, Peter Maydell wrote:
> On 21 April 2018 at 10:07, Cédric Le Goater <clg@kaod.org> wrote:
>> On 04/20/2018 02:48 PM, Peter Maydell wrote:
>>> Currently we use memory_region_init_ram_nomigrate() to create
>>> the "aspeed.boot_rom" 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 "palmetto-bmc", "ast2500-evb" and "romulus-bmc" machines.
>>
>> Migration does not work for these.
> 
> Well, at least we're not breaking anything :-)
> Do you know why migration doesn't work?

A quick migration test shows that :

qemu-system-arm: Missing section footer for aspeed.timerctrl
qemu-system-arm: load of migration failed: Invalid argument

which is surprising, the state looks correct. I will dig in. 

Thanks,

C.


    "aspeed.timerctrl (8)": {
        "ctrl": "0x00000003",
        "ctrl2": "0x00000000",
        "timers": [
            {
                "id": "0x00",
                "level": "0x00000000",
                "timer": "00 00 00 29 aa bd 88 44",
                "reload": "0xffffffff",
                "match": [
                    "0x00000000",
                    "0x00000000"
                ]
            },
            {
                "id": "0x01",
                "level": "0x00000000",
                "timer": "ff ff ff ff ff ff ff ff",
                "reload": "0x00000000",
                "match": [
                    "0x00000000",
                    "0x00000000"
                ]
            },
            {
                "id": "0x02",
                "level": "0x00000000",
                "timer": "ff ff ff ff ff ff ff ff",
                "reload": "0x00000000",
                "match": [
                    "0x00000000",
                    "0x00000000"
                ]
            },
            {
                "id": "0x03",
                "level": "0x00000000",
                "timer": "ff ff ff ff ff ff ff ff",
                "reload": "0xffffffff",
                "match": [
                    "0x00000000",
                    "0x00000000"
                ]
            },
            {
                "id": "0x04",
                "level": "0x00000000",
                "timer": "ff ff ff ff ff ff ff ff",
                "reload": "0x00000000",
                "match": [
                    "0x00000000",
                    "0x00000000"
                ]
            },
            {
                "id": "0x05",
                "level": "0x00000000",
                "timer": "ff ff ff ff ff ff ff ff",
                "reload": "0x00000000",
                "match": [
                    "0x00000000",
                    "0x00000000"
                ]
            },
            {
                "id": "0x06",
                "level": "0x00000000",
                "timer": "ff ff ff ff ff ff ff ff",
                "reload": "0x00000000",
                "match": [
                    "0x00000000",
                    "0x00000000"
                ]
            },
            {
                "id": "0x07",
                "level": "0x00000000",
                "timer": "ff ff ff ff ff ff ff ff",
                "reload": "0x00000000",
                "match": [
                    "0x00000000",
                    "0x00000000"
                ]
            }
        ]
    },