All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 0/3] hw/arm/aspeed: Improve QOM usage
@ 2020-06-23  7:21 Philippe Mathieu-Daudé
  2020-06-23  7:21 ` [PATCH v2 1/3] hw/arm/aspeed: Remove extraneous MemoryRegion object owner Philippe Mathieu-Daudé
                   ` (3 more replies)
  0 siblings, 4 replies; 7+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-06-23  7:21 UTC (permalink / raw)
  To: qemu-devel
  Cc: Peter Maydell, Andrew Jeffery, Philippe Mathieu-Daudé,
	qemu-arm, Cédric Le Goater, Joel Stanley

Yet another cleanup.
Simplify aspeed machine QOM usage.

Since v1, addressed Cédric review comments:
- Rename AspeedBoardState -> AspeedMachineState

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

 include/hw/arm/aspeed.h | 12 +++---------
 hw/arm/aspeed.c         | 33 ++++++++++++++++++---------------
 2 files changed, 21 insertions(+), 24 deletions(-)

-- 
2.21.3



^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH v2 1/3] hw/arm/aspeed: Remove extraneous MemoryRegion object owner
  2020-06-23  7:21 [PATCH v2 0/3] hw/arm/aspeed: Improve QOM usage Philippe Mathieu-Daudé
@ 2020-06-23  7:21 ` Philippe Mathieu-Daudé
  2020-06-23  7:21 ` [PATCH v2 2/3] hw/arm/aspeed: Rename AspeedBoardState as AspeedMachineState Philippe Mathieu-Daudé
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 7+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-06-23  7:21 UTC (permalink / raw)
  To: qemu-devel
  Cc: Peter Maydell, Andrew Jeffery, Philippe Mathieu-Daudé,
	qemu-arm, Cédric Le Goater, Joel Stanley

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

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.

Note, this probably breaks migration for this machine.

Reviewed-by: Cédric Le Goater <clg@kaod.org>
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



^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH v2 2/3] hw/arm/aspeed: Rename AspeedBoardState as AspeedMachineState
  2020-06-23  7:21 [PATCH v2 0/3] hw/arm/aspeed: Improve QOM usage Philippe Mathieu-Daudé
  2020-06-23  7:21 ` [PATCH v2 1/3] hw/arm/aspeed: Remove extraneous MemoryRegion object owner Philippe Mathieu-Daudé
@ 2020-06-23  7:21 ` Philippe Mathieu-Daudé
  2020-06-23  8:12   ` Cédric Le Goater
  2020-06-23  7:21 ` [PATCH v2 3/3] hw/arm/aspeed: QOM'ify AspeedMachineState Philippe Mathieu-Daudé
  2020-06-25 13:18 ` [PATCH v2 0/3] hw/arm/aspeed: Improve QOM usage Peter Maydell
  3 siblings, 1 reply; 7+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-06-23  7:21 UTC (permalink / raw)
  To: qemu-devel
  Cc: Peter Maydell, Andrew Jeffery, Philippe Mathieu-Daudé,
	qemu-arm, Cédric Le Goater, Joel Stanley

To have a more consistent naming, rename AspeedBoardState
as AspeedMachineState.

Suggested-by: Cédric Le Goater <clg@kaod.org>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/arm/aspeed.h |  4 ++--
 hw/arm/aspeed.c         | 20 ++++++++++----------
 2 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/include/hw/arm/aspeed.h b/include/hw/arm/aspeed.h
index 95b4daece8..5114ba0bd4 100644
--- a/include/hw/arm/aspeed.h
+++ b/include/hw/arm/aspeed.h
@@ -11,7 +11,7 @@
 
 #include "hw/boards.h"
 
-typedef struct AspeedBoardState AspeedBoardState;
+typedef struct AspeedMachineState AspeedMachineState;
 
 #define TYPE_ASPEED_MACHINE       MACHINE_TYPE_NAME("aspeed")
 #define ASPEED_MACHINE(obj) \
@@ -45,7 +45,7 @@ typedef struct AspeedMachineClass {
     const char *spi_model;
     uint32_t num_cs;
     uint32_t macs_mask;
-    void (*i2c_init)(AspeedBoardState *bmc);
+    void (*i2c_init)(AspeedMachineState *bmc);
 } AspeedMachineClass;
 
 
diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index 31765792a2..680345beca 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -32,7 +32,7 @@ static struct arm_boot_info aspeed_board_binfo = {
     .board_id = -1, /* device-tree-only board */
 };
 
-struct AspeedBoardState {
+struct AspeedMachineState {
     AspeedSoCState soc;
     MemoryRegion ram_container;
     MemoryRegion max_ram;
@@ -253,7 +253,7 @@ static void sdhci_attach_drive(SDHCIState *sdhci, DriveInfo *dinfo)
 
 static void aspeed_machine_init(MachineState *machine)
 {
-    AspeedBoardState *bmc;
+    AspeedMachineState *bmc;
     AspeedMachineClass *amc = ASPEED_MACHINE_GET_CLASS(machine);
     AspeedSoCClass *sc;
     DriveInfo *drive0 = drive_get(IF_MTD, 0, 0);
@@ -261,7 +261,7 @@ static void aspeed_machine_init(MachineState *machine)
     int i;
     NICInfo *nd = &nd_table[0];
 
-    bmc = g_new0(AspeedBoardState, 1);
+    bmc = g_new0(AspeedMachineState, 1);
 
     memory_region_init(&bmc->ram_container, NULL, "aspeed-ram-container",
                        4 * GiB);
@@ -374,7 +374,7 @@ static void aspeed_machine_init(MachineState *machine)
     arm_load_kernel(ARM_CPU(first_cpu), machine, &aspeed_board_binfo);
 }
 
-static void palmetto_bmc_i2c_init(AspeedBoardState *bmc)
+static void palmetto_bmc_i2c_init(AspeedMachineState *bmc)
 {
     AspeedSoCState *soc = &bmc->soc;
     DeviceState *dev;
@@ -396,7 +396,7 @@ static void palmetto_bmc_i2c_init(AspeedBoardState *bmc)
     object_property_set_int(OBJECT(dev), 110000, "temperature3", &error_abort);
 }
 
-static void ast2500_evb_i2c_init(AspeedBoardState *bmc)
+static void ast2500_evb_i2c_init(AspeedMachineState *bmc)
 {
     AspeedSoCState *soc = &bmc->soc;
     uint8_t *eeprom_buf = g_malloc0(8 * 1024);
@@ -413,13 +413,13 @@ static void ast2500_evb_i2c_init(AspeedBoardState *bmc)
     i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 11), "ds1338", 0x32);
 }
 
-static void ast2600_evb_i2c_init(AspeedBoardState *bmc)
+static void ast2600_evb_i2c_init(AspeedMachineState *bmc)
 {
     /* Start with some devices on our I2C busses */
     ast2500_evb_i2c_init(bmc);
 }
 
-static void romulus_bmc_i2c_init(AspeedBoardState *bmc)
+static void romulus_bmc_i2c_init(AspeedMachineState *bmc)
 {
     AspeedSoCState *soc = &bmc->soc;
 
@@ -428,7 +428,7 @@ static void romulus_bmc_i2c_init(AspeedBoardState *bmc)
     i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 11), "ds1338", 0x32);
 }
 
-static void swift_bmc_i2c_init(AspeedBoardState *bmc)
+static void swift_bmc_i2c_init(AspeedMachineState *bmc)
 {
     AspeedSoCState *soc = &bmc->soc;
 
@@ -457,7 +457,7 @@ static void swift_bmc_i2c_init(AspeedBoardState *bmc)
     i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 12), "tmp105", 0x4a);
 }
 
-static void sonorapass_bmc_i2c_init(AspeedBoardState *bmc)
+static void sonorapass_bmc_i2c_init(AspeedMachineState *bmc)
 {
     AspeedSoCState *soc = &bmc->soc;
 
@@ -501,7 +501,7 @@ static void sonorapass_bmc_i2c_init(AspeedBoardState *bmc)
 
 }
 
-static void witherspoon_bmc_i2c_init(AspeedBoardState *bmc)
+static void witherspoon_bmc_i2c_init(AspeedMachineState *bmc)
 {
     AspeedSoCState *soc = &bmc->soc;
     uint8_t *eeprom_buf = g_malloc0(8 * 1024);
-- 
2.21.3



^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH v2 3/3] hw/arm/aspeed: QOM'ify AspeedMachineState
  2020-06-23  7:21 [PATCH v2 0/3] hw/arm/aspeed: Improve QOM usage Philippe Mathieu-Daudé
  2020-06-23  7:21 ` [PATCH v2 1/3] hw/arm/aspeed: Remove extraneous MemoryRegion object owner Philippe Mathieu-Daudé
  2020-06-23  7:21 ` [PATCH v2 2/3] hw/arm/aspeed: Rename AspeedBoardState as AspeedMachineState Philippe Mathieu-Daudé
@ 2020-06-23  7:21 ` Philippe Mathieu-Daudé
  2020-06-23  8:12   ` Cédric Le Goater
  2020-06-25 13:18 ` [PATCH v2 0/3] hw/arm/aspeed: Improve QOM usage Peter Maydell
  3 siblings, 1 reply; 7+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-06-23  7:21 UTC (permalink / raw)
  To: qemu-devel
  Cc: Peter Maydell, Andrew Jeffery, Philippe Mathieu-Daudé,
	qemu-arm, Cédric Le Goater, Joel Stanley

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

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 5114ba0bd4..09da9d9acc 100644
--- a/include/hw/arm/aspeed.h
+++ b/include/hw/arm/aspeed.h
@@ -15,13 +15,7 @@ typedef struct AspeedMachineState AspeedMachineState;
 
 #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(AspeedMachineState, (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 680345beca..ccf127b328 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -33,9 +33,14 @@ static struct arm_boot_info aspeed_board_binfo = {
 };
 
 struct AspeedMachineState {
+    /* 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)
 {
-    AspeedMachineState *bmc;
+    AspeedMachineState *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(AspeedMachineState, 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(AspeedMachineState),
         .instance_init = aspeed_machine_instance_init,
         .class_size    = sizeof(AspeedMachineClass),
         .class_init    = aspeed_machine_class_init,
-- 
2.21.3



^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH v2 2/3] hw/arm/aspeed: Rename AspeedBoardState as AspeedMachineState
  2020-06-23  7:21 ` [PATCH v2 2/3] hw/arm/aspeed: Rename AspeedBoardState as AspeedMachineState Philippe Mathieu-Daudé
@ 2020-06-23  8:12   ` Cédric Le Goater
  0 siblings, 0 replies; 7+ messages in thread
From: Cédric Le Goater @ 2020-06-23  8:12 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé, qemu-devel
  Cc: Andrew Jeffery, Peter Maydell, qemu-arm, Joel Stanley

On 6/23/20 9:21 AM, Philippe Mathieu-Daudé wrote:
> To have a more consistent naming, rename AspeedBoardState
> as AspeedMachineState.
> 
> Suggested-by: Cédric Le Goater <clg@kaod.org>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

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

> ---
>  include/hw/arm/aspeed.h |  4 ++--
>  hw/arm/aspeed.c         | 20 ++++++++++----------
>  2 files changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/include/hw/arm/aspeed.h b/include/hw/arm/aspeed.h
> index 95b4daece8..5114ba0bd4 100644
> --- a/include/hw/arm/aspeed.h
> +++ b/include/hw/arm/aspeed.h
> @@ -11,7 +11,7 @@
>  
>  #include "hw/boards.h"
>  
> -typedef struct AspeedBoardState AspeedBoardState;
> +typedef struct AspeedMachineState AspeedMachineState;
>  
>  #define TYPE_ASPEED_MACHINE       MACHINE_TYPE_NAME("aspeed")
>  #define ASPEED_MACHINE(obj) \
> @@ -45,7 +45,7 @@ typedef struct AspeedMachineClass {
>      const char *spi_model;
>      uint32_t num_cs;
>      uint32_t macs_mask;
> -    void (*i2c_init)(AspeedBoardState *bmc);
> +    void (*i2c_init)(AspeedMachineState *bmc);
>  } AspeedMachineClass;
>  
>  
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index 31765792a2..680345beca 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -32,7 +32,7 @@ static struct arm_boot_info aspeed_board_binfo = {
>      .board_id = -1, /* device-tree-only board */
>  };
>  
> -struct AspeedBoardState {
> +struct AspeedMachineState {
>      AspeedSoCState soc;
>      MemoryRegion ram_container;
>      MemoryRegion max_ram;
> @@ -253,7 +253,7 @@ static void sdhci_attach_drive(SDHCIState *sdhci, DriveInfo *dinfo)
>  
>  static void aspeed_machine_init(MachineState *machine)
>  {
> -    AspeedBoardState *bmc;
> +    AspeedMachineState *bmc;
>      AspeedMachineClass *amc = ASPEED_MACHINE_GET_CLASS(machine);
>      AspeedSoCClass *sc;
>      DriveInfo *drive0 = drive_get(IF_MTD, 0, 0);
> @@ -261,7 +261,7 @@ static void aspeed_machine_init(MachineState *machine)
>      int i;
>      NICInfo *nd = &nd_table[0];
>  
> -    bmc = g_new0(AspeedBoardState, 1);
> +    bmc = g_new0(AspeedMachineState, 1);
>  
>      memory_region_init(&bmc->ram_container, NULL, "aspeed-ram-container",
>                         4 * GiB);
> @@ -374,7 +374,7 @@ static void aspeed_machine_init(MachineState *machine)
>      arm_load_kernel(ARM_CPU(first_cpu), machine, &aspeed_board_binfo);
>  }
>  
> -static void palmetto_bmc_i2c_init(AspeedBoardState *bmc)
> +static void palmetto_bmc_i2c_init(AspeedMachineState *bmc)
>  {
>      AspeedSoCState *soc = &bmc->soc;
>      DeviceState *dev;
> @@ -396,7 +396,7 @@ static void palmetto_bmc_i2c_init(AspeedBoardState *bmc)
>      object_property_set_int(OBJECT(dev), 110000, "temperature3", &error_abort);
>  }
>  
> -static void ast2500_evb_i2c_init(AspeedBoardState *bmc)
> +static void ast2500_evb_i2c_init(AspeedMachineState *bmc)
>  {
>      AspeedSoCState *soc = &bmc->soc;
>      uint8_t *eeprom_buf = g_malloc0(8 * 1024);
> @@ -413,13 +413,13 @@ static void ast2500_evb_i2c_init(AspeedBoardState *bmc)
>      i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 11), "ds1338", 0x32);
>  }
>  
> -static void ast2600_evb_i2c_init(AspeedBoardState *bmc)
> +static void ast2600_evb_i2c_init(AspeedMachineState *bmc)
>  {
>      /* Start with some devices on our I2C busses */
>      ast2500_evb_i2c_init(bmc);
>  }
>  
> -static void romulus_bmc_i2c_init(AspeedBoardState *bmc)
> +static void romulus_bmc_i2c_init(AspeedMachineState *bmc)
>  {
>      AspeedSoCState *soc = &bmc->soc;
>  
> @@ -428,7 +428,7 @@ static void romulus_bmc_i2c_init(AspeedBoardState *bmc)
>      i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 11), "ds1338", 0x32);
>  }
>  
> -static void swift_bmc_i2c_init(AspeedBoardState *bmc)
> +static void swift_bmc_i2c_init(AspeedMachineState *bmc)
>  {
>      AspeedSoCState *soc = &bmc->soc;
>  
> @@ -457,7 +457,7 @@ static void swift_bmc_i2c_init(AspeedBoardState *bmc)
>      i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 12), "tmp105", 0x4a);
>  }
>  
> -static void sonorapass_bmc_i2c_init(AspeedBoardState *bmc)
> +static void sonorapass_bmc_i2c_init(AspeedMachineState *bmc)
>  {
>      AspeedSoCState *soc = &bmc->soc;
>  
> @@ -501,7 +501,7 @@ static void sonorapass_bmc_i2c_init(AspeedBoardState *bmc)
>  
>  }
>  
> -static void witherspoon_bmc_i2c_init(AspeedBoardState *bmc)
> +static void witherspoon_bmc_i2c_init(AspeedMachineState *bmc)
>  {
>      AspeedSoCState *soc = &bmc->soc;
>      uint8_t *eeprom_buf = g_malloc0(8 * 1024);
> 



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2 3/3] hw/arm/aspeed: QOM'ify AspeedMachineState
  2020-06-23  7:21 ` [PATCH v2 3/3] hw/arm/aspeed: QOM'ify AspeedMachineState Philippe Mathieu-Daudé
@ 2020-06-23  8:12   ` Cédric Le Goater
  0 siblings, 0 replies; 7+ messages in thread
From: Cédric Le Goater @ 2020-06-23  8:12 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé, qemu-devel
  Cc: Andrew Jeffery, Peter Maydell, qemu-arm, Joel Stanley

On 6/23/20 9:21 AM, Philippe Mathieu-Daudé wrote:
> AspeedMachineState seems crippled. We use incorrectly 2
> different structures to do the same thing. Merge them
> altogether:
> - Move AspeedMachine fields to AspeedMachineState
> - AspeedMachineState is now QOM
> - Remove unused AspeedMachine structure
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Cédric Le Goater <clg@kaod.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 5114ba0bd4..09da9d9acc 100644
> --- a/include/hw/arm/aspeed.h
> +++ b/include/hw/arm/aspeed.h
> @@ -15,13 +15,7 @@ typedef struct AspeedMachineState AspeedMachineState;
>  
>  #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(AspeedMachineState, (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 680345beca..ccf127b328 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -33,9 +33,14 @@ static struct arm_boot_info aspeed_board_binfo = {
>  };
>  
>  struct AspeedMachineState {
> +    /* 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)
>  {
> -    AspeedMachineState *bmc;
> +    AspeedMachineState *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(AspeedMachineState, 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(AspeedMachineState),
>          .instance_init = aspeed_machine_instance_init,
>          .class_size    = sizeof(AspeedMachineClass),
>          .class_init    = aspeed_machine_class_init,
> 



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2 0/3] hw/arm/aspeed: Improve QOM usage
  2020-06-23  7:21 [PATCH v2 0/3] hw/arm/aspeed: Improve QOM usage Philippe Mathieu-Daudé
                   ` (2 preceding siblings ...)
  2020-06-23  7:21 ` [PATCH v2 3/3] hw/arm/aspeed: QOM'ify AspeedMachineState Philippe Mathieu-Daudé
@ 2020-06-25 13:18 ` Peter Maydell
  3 siblings, 0 replies; 7+ messages in thread
From: Peter Maydell @ 2020-06-25 13:18 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: Andrew Jeffery, qemu-arm, Joel Stanley, QEMU Developers,
	Cédric Le Goater

On Tue, 23 Jun 2020 at 08:21, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Yet another cleanup.
> Simplify aspeed machine QOM usage.
>
> Since v1, addressed Cédric review comments:
> - Rename AspeedBoardState -> AspeedMachineState
>



Applied to target-arm.next, thanks.

-- PMM


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2020-06-25 13:19 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-23  7:21 [PATCH v2 0/3] hw/arm/aspeed: Improve QOM usage Philippe Mathieu-Daudé
2020-06-23  7:21 ` [PATCH v2 1/3] hw/arm/aspeed: Remove extraneous MemoryRegion object owner Philippe Mathieu-Daudé
2020-06-23  7:21 ` [PATCH v2 2/3] hw/arm/aspeed: Rename AspeedBoardState as AspeedMachineState Philippe Mathieu-Daudé
2020-06-23  8:12   ` Cédric Le Goater
2020-06-23  7:21 ` [PATCH v2 3/3] hw/arm/aspeed: QOM'ify AspeedMachineState Philippe Mathieu-Daudé
2020-06-23  8:12   ` Cédric Le Goater
2020-06-25 13:18 ` [PATCH v2 0/3] hw/arm/aspeed: Improve QOM usage Peter Maydell

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.