Re: [PATCH 1/2] hw/arm/aspeed: Remove extraneous MemoryRegion object owner

[Cédric Le Goater]

On 6/20/20 6:28 PM, Philippe Mathieu-Daudé wrote:
> I'm confused by this code, 'bmc' is created as:
> 
>   bmc = g_new0(AspeedBoardState, 1);
> 
> Then we use it as QOM owner for different MemoryRegion objects.
> But looking at memory_region_init_ram (similarly for ROM):
> 
>   void memory_region_init_ram(MemoryRegion *mr,
>                               struct Object *owner,
>                               const char *name,
>                               uint64_t size,
>                               Error **errp)
>   {
>       DeviceState *owner_dev;
>       Error *err = NULL;
> 
>       memory_region_init_ram_nomigrate(mr, owner, name, size, &err);
>       if (err) {
>           error_propagate(errp, err);
>           return;
>       }
>       /* This will assert if owner is neither NULL nor a DeviceState.
>        * We only want the owner here for the purposes of defining a
>        * unique name for migration. TODO: Ideally we should implement
>        * a naming scheme for Objects which are not DeviceStates, in
>        * which case we can relax this restriction.
>        */
>       owner_dev = DEVICE(owner);
>       vmstate_register_ram(mr, owner_dev);
>   }
> 
> The expected assertion is not triggered ('bmc' is not NULL neither
> a DeviceState).
> 
> 'bmc' structure is defined as:
> 
>   struct AspeedBoardState {
>       AspeedSoCState soc;
>       MemoryRegion ram_container;
>       MemoryRegion max_ram;
>   };
> 
> Apparently
> What happens is when using 'OBJECT(bmc)', the QOM macros cast the
> memory pointed by bmc, which first member is 'soc', which is
> initialized ...:
> 
>   object_initialize_child(OBJECT(machine), "soc",
>                           &bmc->soc, amc->soc_name);
> 
> The 'soc' object is indeed a DeviceState, so the assertion passes.
> 
> Since this is fragile and only happens to work by luck, remove the
> dangerous OBJECT(bmc) owner argument.

Indeed. Nice catch. 
> 
> This probably breaks migration for this machine.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

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

Thanks

C.

> ---
>  hw/arm/aspeed.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index 0ad08a2b4c..31765792a2 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -329,12 +329,12 @@ static void aspeed_machine_init(MachineState *machine)
>           * needed by the flash modules of the Aspeed machines.
>           */
>          if (ASPEED_MACHINE(machine)->mmio_exec) {
> -            memory_region_init_alias(boot_rom, OBJECT(bmc), 
> "aspeed.boot_rom",
> +            memory_region_init_alias(boot_rom, NULL, "aspeed.boot_rom",
>                                       &fl->mmio, 0, fl->size);
>              memory_region_add_subregion(get_system_memory(), FIRMWARE_ADDR,
>                                          boot_rom);
>          } else {
> -            memory_region_init_rom(boot_rom, OBJECT(bmc), "aspeed.boot_rom",
> +            memory_region_init_rom(boot_rom, NULL, "aspeed.boot_rom",
>                                     fl->size, &error_abort);
>              memory_region_add_subregion(get_system_memory(), FIRMWARE_ADDR,
>                                          boot_rom);
> @@ -345,7 +345,7 @@ static void aspeed_machine_init(MachineState *machine)
>      if (machine->kernel_filename && sc->num_cpus > 1) {
>          /* With no u-boot we must set up a boot stub for the secondary CPU */
>          MemoryRegion *smpboot = g_new(MemoryRegion, 1);
> -        memory_region_init_ram(smpboot, OBJECT(bmc), "aspeed.smpboot",
> +        memory_region_init_ram(smpboot, NULL, "aspeed.smpboot",
>                                 0x80, &error_abort);
>          memory_region_add_subregion(get_system_memory(),
>                                      AST_SMP_MAILBOX_BASE, smpboot);
>