[PATCH 0/2] hw/arm/aspeed: Improve QOM usage

[PATCH 0/2] hw/arm/aspeed: Improve QOM usage

[Philippe Mathieu-Daudé]

Yet another cleanup.
Simplify aspeed machine QOM usage.

Philippe Mathieu-Daudé (2):
  hw/arm/aspeed: Remove extraneous MemoryRegion object owner
  hw/arm/aspeed: QOM'ify AspeedBoardState

 include/hw/arm/aspeed.h |  8 +-------
 hw/arm/aspeed.c         | 17 ++++++++++-------
 2 files changed, 11 insertions(+), 14 deletions(-)

-- 
2.21.3

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

[Philippe Mathieu-Daudé]

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.

This probably breaks migration for this machine.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 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);
-- 
2.21.3

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

[PATCH 2/2] hw/arm/aspeed: QOM'ify AspeedBoardState

[Philippe Mathieu-Daudé]

AspeedBoardState seems crippled. We use incorrectly 2
different structures to do the same thing. Merge them
altogether:
- Move AspeedMachine fields to AspeedBoardState
- AspeedBoardState is now QOM
- Remove unused AspeedMachine

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/arm/aspeed.h |  8 +-------
 hw/arm/aspeed.c         | 11 +++++++----
 2 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/include/hw/arm/aspeed.h b/include/hw/arm/aspeed.h
index 95b4daece8..9c9bac1263 100644
--- a/include/hw/arm/aspeed.h
+++ b/include/hw/arm/aspeed.h
@@ -15,13 +15,7 @@ typedef struct AspeedBoardState AspeedBoardState;
 
 #define TYPE_ASPEED_MACHINE       MACHINE_TYPE_NAME("aspeed")
 #define ASPEED_MACHINE(obj) \
-    OBJECT_CHECK(AspeedMachine, (obj), TYPE_ASPEED_MACHINE)
-
-typedef struct AspeedMachine {
-    MachineState parent_obj;
-
-    bool mmio_exec;
-} AspeedMachine;
+    OBJECT_CHECK(AspeedBoardState, (obj), TYPE_ASPEED_MACHINE)
 
 #define ASPEED_MAC0_ON   (1 << 0)
 #define ASPEED_MAC1_ON   (1 << 1)
diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index 31765792a2..6b7533aeee 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -33,9 +33,14 @@ static struct arm_boot_info aspeed_board_binfo = {
 };
 
 struct AspeedBoardState {
+    /* Private */
+    MachineState parent_obj;
+    /* Public */
+
     AspeedSoCState soc;
     MemoryRegion ram_container;
     MemoryRegion max_ram;
+    bool mmio_exec;
 };
 
 /* Palmetto hardware value: 0x120CE416 */
@@ -253,7 +258,7 @@ static void sdhci_attach_drive(SDHCIState *sdhci, DriveInfo *dinfo)
 
 static void aspeed_machine_init(MachineState *machine)
 {
-    AspeedBoardState *bmc;
+    AspeedBoardState *bmc = ASPEED_MACHINE(machine);
     AspeedMachineClass *amc = ASPEED_MACHINE_GET_CLASS(machine);
     AspeedSoCClass *sc;
     DriveInfo *drive0 = drive_get(IF_MTD, 0, 0);
@@ -261,8 +266,6 @@ static void aspeed_machine_init(MachineState *machine)
     int i;
     NICInfo *nd = &nd_table[0];
 
-    bmc = g_new0(AspeedBoardState, 1);
-
     memory_region_init(&bmc->ram_container, NULL, "aspeed-ram-container",
                        4 * GiB);
     memory_region_add_subregion(&bmc->ram_container, 0, machine->ram);
@@ -751,7 +754,7 @@ static const TypeInfo aspeed_machine_types[] = {
     }, {
         .name          = TYPE_ASPEED_MACHINE,
         .parent        = TYPE_MACHINE,
-        .instance_size = sizeof(AspeedMachine),
+        .instance_size = sizeof(AspeedBoardState),
         .instance_init = aspeed_machine_instance_init,
         .class_size    = sizeof(AspeedMachineClass),
         .class_init    = aspeed_machine_class_init,
-- 
2.21.3

Re: [PATCH 2/2] hw/arm/aspeed: QOM'ify AspeedBoardState

[Cédric Le Goater]

On 6/20/20 6:28 PM, Philippe Mathieu-Daudé wrote:
> AspeedBoardState seems crippled. We use incorrectly 2
> different structures to do the same thing. Merge them
> altogether:
> - Move AspeedMachine fields to AspeedBoardState
> - AspeedBoardState is now QOM
> - Remove unused AspeedMachine

I like that but maybe we should rename AspeedBoardState in AspeedMachine ? 
The naming would be more consistent.

Thanks,

C. 

> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  include/hw/arm/aspeed.h |  8 +-------
>  hw/arm/aspeed.c         | 11 +++++++----
>  2 files changed, 8 insertions(+), 11 deletions(-)
> 
> diff --git a/include/hw/arm/aspeed.h b/include/hw/arm/aspeed.h
> index 95b4daece8..9c9bac1263 100644
> --- a/include/hw/arm/aspeed.h
> +++ b/include/hw/arm/aspeed.h
> @@ -15,13 +15,7 @@ typedef struct AspeedBoardState AspeedBoardState;
>  
>  #define TYPE_ASPEED_MACHINE       MACHINE_TYPE_NAME("aspeed")
>  #define ASPEED_MACHINE(obj) \
> -    OBJECT_CHECK(AspeedMachine, (obj), TYPE_ASPEED_MACHINE)
> -
> -typedef struct AspeedMachine {
> -    MachineState parent_obj;
> -
> -    bool mmio_exec;
> -} AspeedMachine;
> +    OBJECT_CHECK(AspeedBoardState, (obj), TYPE_ASPEED_MACHINE)
>  
>  #define ASPEED_MAC0_ON   (1 << 0)
>  #define ASPEED_MAC1_ON   (1 << 1)
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index 31765792a2..6b7533aeee 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -33,9 +33,14 @@ static struct arm_boot_info aspeed_board_binfo = {
>  };
>  
>  struct AspeedBoardState {
> +    /* Private */
> +    MachineState parent_obj;
> +    /* Public */
> +
>      AspeedSoCState soc;
>      MemoryRegion ram_container;
>      MemoryRegion max_ram;
> +    bool mmio_exec;
>  };
>  
>  /* Palmetto hardware value: 0x120CE416 */
> @@ -253,7 +258,7 @@ static void sdhci_attach_drive(SDHCIState *sdhci, DriveInfo *dinfo)
>  
>  static void aspeed_machine_init(MachineState *machine)
>  {
> -    AspeedBoardState *bmc;
> +    AspeedBoardState *bmc = ASPEED_MACHINE(machine);
>      AspeedMachineClass *amc = ASPEED_MACHINE_GET_CLASS(machine);
>      AspeedSoCClass *sc;
>      DriveInfo *drive0 = drive_get(IF_MTD, 0, 0);
> @@ -261,8 +266,6 @@ static void aspeed_machine_init(MachineState *machine)
>      int i;
>      NICInfo *nd = &nd_table[0];
>  
> -    bmc = g_new0(AspeedBoardState, 1);
> -
>      memory_region_init(&bmc->ram_container, NULL, "aspeed-ram-container",
>                         4 * GiB);
>      memory_region_add_subregion(&bmc->ram_container, 0, machine->ram);
> @@ -751,7 +754,7 @@ static const TypeInfo aspeed_machine_types[] = {
>      }, {
>          .name          = TYPE_ASPEED_MACHINE,
>          .parent        = TYPE_MACHINE,
> -        .instance_size = sizeof(AspeedMachine),
> +        .instance_size = sizeof(AspeedBoardState),
>          .instance_init = aspeed_machine_instance_init,
>          .class_size    = sizeof(AspeedMachineClass),
>          .class_init    = aspeed_machine_class_init,
>