Re: [Qemu-arm] [PATCH 06/19] aspeed: introduce a configurable number of CPU per machine

[Cédric Le Goater]

On 12/06/2019 03:32, Joel Stanley wrote:
> On Sat, 25 May 2019 at 15:13, Cédric Le Goater <address@hidden> wrote:
>>
>> The current models of the Aspeed SoCs only have one CPU but future
>> ones will support SMP. Introduce a way to configure the maximum number
>> of CPU per machine. SMP support will be activated when models for such
>> SoCs are implemented.
>>
>> Signed-off-by: Cédric Le Goater <address@hidden>
>> ---
>>  include/hw/arm/aspeed.h     |  1 +
>>  include/hw/arm/aspeed_soc.h |  3 ++-
>>  hw/arm/aspeed.c             |  8 ++++++--
>>  hw/arm/aspeed_soc.c         | 17 +++++++++++------
>>  4 files changed, 20 insertions(+), 9 deletions(-)
>>
>> diff --git a/include/hw/arm/aspeed.h b/include/hw/arm/aspeed.h
>> index 02073a6b4d61..f2f238ea83cc 100644
>> --- a/include/hw/arm/aspeed.h
>> +++ b/include/hw/arm/aspeed.h
>> @@ -23,6 +23,7 @@ typedef struct AspeedBoardConfig {
>>      uint32_t num_cs;
>>      void (*i2c_init)(AspeedBoardState *bmc);
>>      uint32_t ram;
>> +    uint32_t num_cpus;
>>  } AspeedBoardConfig;
>>
>>  #define TYPE_ASPEED_MACHINE       MACHINE_TYPE_NAME("aspeed")
>> diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h
>> index fa0ba957a611..7247f6da2505 100644
>> --- a/include/hw/arm/aspeed_soc.h
>> +++ b/include/hw/arm/aspeed_soc.h
>> @@ -24,13 +24,14 @@
>>
>>  #define ASPEED_SPIS_NUM  2
>>  #define ASPEED_WDTS_NUM  3
>> +#define ASPEED_CPUS_NUM  2
>>
>>  typedef struct AspeedSoCState {
>>      /*< private >*/
>>      DeviceState parent;
>>
>>      /*< public >*/
>> -    ARMCPU cpu;
>> +    ARMCPU cpu[ASPEED_CPUS_NUM];
>>      MemoryRegion sram;
>>      AspeedVICState vic;
>>      AspeedRtcState rtc;
>> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
>> index 10ba3f50481a..004b0c318951 100644
>> --- a/hw/arm/aspeed.c
>> +++ b/hw/arm/aspeed.c
>> @@ -29,7 +29,6 @@
>>
>>  static struct arm_boot_info aspeed_board_binfo = {
>>      .board_id = -1, /* device-tree-only board */
>> -    .nb_cpus = 1,
>>  };
>>
>>  struct AspeedBoardState {
>> @@ -231,6 +230,7 @@ static void aspeed_board_init(MachineState *machine,
>>      aspeed_board_binfo.kernel_cmdline = machine->kernel_cmdline;
>>      aspeed_board_binfo.ram_size = ram_size;
>>      aspeed_board_binfo.loader_start = sc->info->memmap[ASPEED_SDRAM];
>> +    aspeed_board_binfo.nb_cpus = cfg->num_cpus;
>>
>>      if (cfg->i2c_init) {
>>          cfg->i2c_init(bmc);
>> @@ -327,7 +327,7 @@ static void aspeed_machine_class_init(ObjectClass *oc, 
>> void *data)
>>
>>      mc->desc = board->desc;
>>      mc->init = aspeed_machine_init;
>> -    mc->max_cpus = 1;
>> +    mc->max_cpus = ASPEED_CPUS_NUM;
>>      mc->no_sdcard = 1;
>>      mc->no_floppy = 1;
>>      mc->no_cdrom = 1;
>> @@ -357,6 +357,7 @@ static const AspeedBoardConfig aspeed_boards[] = {
>>          .num_cs    = 1,
>>          .i2c_init  = palmetto_bmc_i2c_init,
>>          .ram       = 256 * MiB,
>> +        .num_cpus  = 1,
>>      }, {
>>          .name      = MACHINE_TYPE_NAME("ast2500-evb"),
>>          .desc      = "Aspeed AST2500 EVB (ARM1176)",
>> @@ -367,6 +368,7 @@ static const AspeedBoardConfig aspeed_boards[] = {
>>          .num_cs    = 1,
>>          .i2c_init  = ast2500_evb_i2c_init,
>>          .ram       = 512 * MiB,
>> +        .num_cpus  = 1,
>>      }, {
>>          .name      = MACHINE_TYPE_NAME("romulus-bmc"),
>>          .desc      = "OpenPOWER Romulus BMC (ARM1176)",
>> @@ -377,6 +379,7 @@ static const AspeedBoardConfig aspeed_boards[] = {
>>          .num_cs    = 2,
>>          .i2c_init  = romulus_bmc_i2c_init,
>>          .ram       = 512 * MiB,
>> +        .num_cpus  = 1,
>>      }, {
>>          .name      = MACHINE_TYPE_NAME("witherspoon-bmc"),
>>          .desc      = "OpenPOWER Witherspoon BMC (ARM1176)",
>> @@ -387,6 +390,7 @@ static const AspeedBoardConfig aspeed_boards[] = {
>>          .num_cs    = 2,
>>          .i2c_init  = witherspoon_bmc_i2c_init,
>>          .ram       = 512 * MiB,
>> +        .num_cpus  = 1,
>>      },
>>  };
>>
>> diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
>> index d1dc8f03f35c..b983d5efc5d1 100644
>> --- a/hw/arm/aspeed_soc.c
>> +++ b/hw/arm/aspeed_soc.c
>> @@ -172,8 +172,11 @@ static void aspeed_soc_init(Object *obj)
>>      AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s);
>>      int i;
>>
>> -    object_initialize_child(obj, "cpu", OBJECT(&s->cpu), sizeof(s->cpu),
>> -                            sc->info->cpu_type, &error_abort, NULL);
>> +    for (i = 0; i < MIN(smp_cpus, ASPEED_CPUS_NUM); i++) {
> 
> What's the intent of this test?
> 
> If we're checking that the user hasn't requested more CPUs that the
> SoC supports, shoudln't it be testing against ->num_cpus instead of
> ASPEED_CPUS_NUM?

yes but we are in the SoC and ->num_cpus is currently a board level 
information. As we are in the _init routine I can't use a SoC property. 

This can be improved. I will send a v2.

Thanks,

C.

> 
>> +        object_initialize_child(obj, "cpu[*]", OBJECT(&s->cpu[i]),
>> +                                sizeof(s->cpu[i]), sc->info->cpu_type,
>> +                                &error_abort, NULL);
>> +    }
>>
>>      sysbus_init_child_obj(obj, "scu", OBJECT(&s->scu), sizeof(s->scu),
>>                            TYPE_ASPEED_SCU);
>> @@ -242,10 +245,12 @@ static void aspeed_soc_realize(DeviceState *dev, Error 
>> **errp)
>>                                  ASPEED_SOC_IOMEM_SIZE);
>>
>>      /* CPU */
>> -    object_property_set_bool(OBJECT(&s->cpu), true, "realized", &err);
>> -    if (err) {
>> -        error_propagate(errp, err);
>> -        return;
>> +    for (i = 0; i < smp_cpus; i++) {
> 
> Can you explain why we use smp_cpus instead of ->num_cpus?
> 
>> +        object_property_set_bool(OBJECT(&s->cpu[i]), true, "realized", 
>> &err);
>> +        if (err) {
>> +            error_propagate(errp, err);
>> +            return;
>> +        }
>>      }
>>
>>      /* SRAM */
>> --
>> 2.20.1
>>