Re: [Qemu-devel] [PULL 06/14] aspeed-soc: provide a framework to add new SoCs

[Peter Maydell]

On 6 September 2016 at 14:07, Peter Maydell <address@hidden> wrote:
> From: Cédric Le Goater <address@hidden>
>
> Let's define an object class for each Aspeed SoC we support. A
> AspeedSoCInfo struct gathers the SoC specifications which can later be
> used by an instance of the class or by a board using the SoC.
>
> Signed-off-by: Cédric Le Goater <address@hidden>
> Reviewed-by: Andrew Jeffery <address@hidden>
> Message-id: address@hidden
> Reviewed-by: Peter Maydell <address@hidden>
> Signed-off-by: Peter Maydell <address@hidden>

> @@ -222,7 +232,18 @@ static const TypeInfo aspeed_soc_type_info = {
>
>  static void aspeed_soc_register_types(void)
>  {
> +    int i;
> +
>      type_register_static(&aspeed_soc_type_info);
> +    for (i = 0; i < ARRAY_SIZE(aspeed_socs); ++i) {
> +        TypeInfo ti = {
> +            .name       = aspeed_socs[i].name,
> +            .parent     = TYPE_ASPEED_SOC,
> +            .class_init = aspeed_soc_class_init,

This is kind of odd, because you now have the same class init
function for both the subclass and the parent class.
I would suggest adding ".abstract = true" to the TypeInfo for
what is now your parent class (TYPE_ASPEED_SOC), so that it
can't be accidentally created directly, and then you can
drop its class_init entirely (letting the class init for
the subclasses do the work).

> +            .class_data = (void *) &aspeed_socs[i],
> +        };
> +        type_register(&ti);
> +    }
>  }
>
>  type_init(aspeed_soc_register_types)
> diff --git a/hw/arm/palmetto-bmc.c b/hw/arm/palmetto-bmc.c
> index 4d11905..4319121 100644
> --- a/hw/arm/palmetto-bmc.c
> +++ b/hw/arm/palmetto-bmc.c
> @@ -22,8 +22,7 @@
>  #include "sysemu/blockdev.h"
>
>  static struct arm_boot_info palmetto_bmc_binfo = {
> -    .loader_start = AST2400_SDRAM_BASE,
> -    .board_id = 0,
> +    .board_id = -1, /* device-tree-only board */
>      .nb_cpus = 1,
>  };
>
> @@ -61,14 +60,17 @@ static void palmetto_bmc_init_flashes(AspeedSMCState *s, 
> const char *flashtype,
>  static void palmetto_bmc_init(MachineState *machine)
>  {
>      PalmettoBMCState *bmc;
> +    AspeedSoCClass *sc;
>
>      bmc = g_new0(PalmettoBMCState, 1);
> -    object_initialize(&bmc->soc, (sizeof(bmc->soc)), TYPE_ASPEED_SOC);
> +    object_initialize(&bmc->soc, (sizeof(bmc->soc)), "ast2400-a0");
>      object_property_add_child(OBJECT(machine), "soc", OBJECT(&bmc->soc),
>                                &error_abort);
>
> +    sc = ASPEED_SOC_GET_CLASS(&bmc->soc);
> +
>      memory_region_allocate_system_memory(&bmc->ram, NULL, "ram", ram_size);
> -    memory_region_add_subregion(get_system_memory(), AST2400_SDRAM_BASE,
> +    memory_region_add_subregion(get_system_memory(), sc->info->sdram_base,
>                                  &bmc->ram);
>      object_property_add_const_link(OBJECT(&bmc->soc), "ram", 
> OBJECT(&bmc->ram),
>                                     &error_abort);
> @@ -84,6 +86,8 @@ static void palmetto_bmc_init(MachineState *machine)
>      palmetto_bmc_binfo.initrd_filename = machine->initrd_filename;
>      palmetto_bmc_binfo.kernel_cmdline = machine->kernel_cmdline;
>      palmetto_bmc_binfo.ram_size = ram_size;
> +    palmetto_bmc_binfo.loader_start = sc->info->sdram_base;
> +
>      arm_load_kernel(ARM_CPU(first_cpu), &palmetto_bmc_binfo);
>  }
>
> diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h
> index bf63ae9..0146a2a 100644
> --- a/include/hw/arm/aspeed_soc.h
> +++ b/include/hw/arm/aspeed_soc.h
> @@ -39,6 +39,21 @@ typedef struct AspeedSoCState {
>  #define TYPE_ASPEED_SOC "aspeed-soc"
>  #define ASPEED_SOC(obj) OBJECT_CHECK(AspeedSoCState, (obj), TYPE_ASPEED_SOC)
>
> -#define AST2400_SDRAM_BASE       0x40000000
> +typedef struct AspeedSoCInfo {
> +    const char *name;
> +    const char *cpu_model;
> +    uint32_t silicon_rev;
> +    hwaddr sdram_base;
> +} AspeedSoCInfo;
> +
> +typedef struct AspeedSoCClass {
> +    DeviceState parent_class;

This is wrong -- the first thing in the subclass's Class struct
should be the Class struct of the parent type (here DeviceClass), not
the state struct of the parent type.

In particular on 32-bit hosts this means that the assignment to
sc->info in aspeed_soc_class_init() is actually writing to
where dc->props should be, and QEMU then dumps core in
device_initfn() when it tries to use dc->props and it's garbage.

Fixing this seems to cure the segfault, but I think all things
considered I'm going to declare the series insufficiently baked
and drop it from target-arm.next. Please can you fix these issues,
retest and send a new version?

thanks
-- PMM