Re: [PATCH 1/2] hw/arm/aspeed: Remove extraneous MemoryRegion object owner
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, );
> 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",
> >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é
Reviewed-by: Cédric Le Goater
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",
> >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, _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, _abort);
> memory_region_add_subregion(get_system_memory(),
> AST_SMP_MAILBOX_BASE, smpboot);
>
[PATCH 1/2] hw/arm/aspeed: Remove extraneous MemoryRegion object owner
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, );
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",
>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.
This probably breaks migration for this machine.
Signed-off-by: Philippe Mathieu-Daudé
---
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",
>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, _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, _abort);
memory_region_add_subregion(get_system_memory(),
AST_SMP_MAILBOX_BASE, smpboot);
--
2.21.3
