Re: [PATCH] arm/aspeed: Rework NIC attachment

[Markus Armbruster <armbru@redhat.com>]

Cédric Le Goater <clg@kaod.org> writes:

> On 5/20/20 8:34 AM, Markus Armbruster wrote:
>> Cédric Le Goater <clg@kaod.org> writes:
>> 
>>> The AST2400 and AST2500 SoCs have two MACs but only the first MAC0 is
>>> active on the Aspeed machines using these SoCs. The AST2600 has four
>>> MACs. The AST2600 EVB machine activates MAC1, MAC2 and MAC3 and the
>>> Tacoma BMC machine activates MAC2.
>>>
>>> Introduce a bit-field property "macs-mask" under the Aspeed SoC model
>>> to link the active MACs of the machine being started with the available
>>> network devices.
>>>
>>> Inactive MACs will have no peer and QEMU will warn the user with :
>>>
>>>     qemu-system-arm: warning: nic ftgmac100.0 has no peer
>>>     qemu-system-arm: warning: nic ftgmac100.1 has no peer
>>>     qemu-system-arm: warning: nic ftgmac100.3 has no peer
>> 
>> I can't reproduce this warning.  What's your exact command line?
>
> Get a witherspoon-tacoma flash image :
>
>     $ wget https://openpower.xyz/job/openbmc-build/distro=ubuntu,label=builder,target=witherspoon-tacoma/lastSuccessfulBuild/artifact/deploy/images/witherspoon-tacoma/flash-witherspoon-tacoma
>
> Run :
>
>     $ qemu-system-arm -M tacoma-bmc -nic user -drive file=./flash-witherspoon-tacoma,format=raw,if=mtd -nographic -nodefaults -serial mon:stdio
>     qemu-system-arm: warning: nic ftgmac100.0 has no peer
>     qemu-system-arm: warning: nic ftgmac100.1 has no peer
>     qemu-system-arm: warning: nic ftgmac100.3 has no peer

I must have run the wrong binary somehow.

>> 
>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>> ---
>>>
>>>  To be applied on top of patch "arm/aspeed: Compute the number of CPUs
>>>  from the SoC definition" 
>>>  
>>>  http://patchwork.ozlabs.org/project/qemu-devel/patch/20200519091631.1006073-1-clg@kaod.org/
>>>  
>>>  include/hw/arm/aspeed.h     |  1 +
>>>  include/hw/arm/aspeed_soc.h |  6 ++++++
>>>  hw/arm/aspeed.c             |  6 ++++++
>>>  hw/arm/aspeed_ast2600.c     | 11 ++++++++---
>>>  hw/arm/aspeed_soc.c         | 10 ++++++++--
>>>  5 files changed, 29 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/include/hw/arm/aspeed.h b/include/hw/arm/aspeed.h
>>> index 18521484b90e..842dff485f5b 100644
>>> --- a/include/hw/arm/aspeed.h
>>> +++ b/include/hw/arm/aspeed.h
>>> @@ -39,6 +39,7 @@ typedef struct AspeedMachineClass {
>>>      const char *fmc_model;
>>>      const char *spi_model;
>>>      uint32_t num_cs;
>>> +    uint32_t macs_mask;
>>>      void (*i2c_init)(AspeedBoardState *bmc);
>>>  } AspeedMachineClass;
>>>  
>>> diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h
>>> index 914115f3ef77..fdb9e05bc47c 100644
>>> --- a/include/hw/arm/aspeed_soc.h
>>> +++ b/include/hw/arm/aspeed_soc.h
>>> @@ -34,6 +34,11 @@
>>>  #define ASPEED_CPUS_NUM  2
>>>  #define ASPEED_MACS_NUM  4
>>>  
>>> +#define ASPEED_MAC0_ON   (1 << 0)
>>> +#define ASPEED_MAC1_ON   (1 << 1)
>>> +#define ASPEED_MAC2_ON   (1 << 2)
>>> +#define ASPEED_MAC3_ON   (1 << 3)
>>> +
>>>  typedef struct AspeedSoCState {
>>>      /*< private >*/
>>>      DeviceState parent;
>>> @@ -55,6 +60,7 @@ typedef struct AspeedSoCState {
>>>      AspeedSDMCState sdmc;
>>>      AspeedWDTState wdt[ASPEED_WDTS_NUM];
>>>      FTGMAC100State ftgmac100[ASPEED_MACS_NUM];
>>> +    uint32_t macs_mask;
>> 
>> What's the purpose of this member?  When and how would it be different
>> from AspeedMachineClass's macs_mask?
>
> Each machine activates a different set of MACs even if using the same SoC.
> So, the SoC macs_mask is overiden when the machine initializes the SoC in 
> aspeed_machine_init().

Let me try to rephrase my question below.

> That said, I think the default SoC macs_mask should be all MACS, a value 
> of 0xFFFFFFFF would be fine, and not only the first MAC as this patch does.
>
>>>      AspeedMiiState mii[ASPEED_MACS_NUM];
>>>      AspeedGPIOState gpio;
>>>      AspeedGPIOState gpio_1_8v;
>>> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
>>> index 6f8f4b88f8ab..79c683864d7e 100644
>>> --- a/hw/arm/aspeed.c
>>> +++ b/hw/arm/aspeed.c
>>> @@ -283,6 +283,8 @@ static void aspeed_machine_init(MachineState *machine)
>>>                              &error_abort);
>>>      object_property_set_int(OBJECT(&bmc->soc), amc->num_cs, "num-cs",
>>>                              &error_abort);
>>> +    object_property_set_int(OBJECT(&bmc->soc), amc->macs_mask, "macs-mask",
>>> +                            &error_abort);

Here, you set AspeedSocState member macs_mask to AspeedMachineClass
member macs_mask.

>>>      object_property_set_link(OBJECT(&bmc->soc), OBJECT(&bmc->ram_container),
>>>                               "dram", &error_abort);
>>>      if (machine->kernel_filename) {
>>> @@ -556,12 +558,14 @@ static int aspeed_soc_num_cpus(const char *soc_name)
>>>  static void aspeed_machine_class_init(ObjectClass *oc, void *data)
>>>  {
>>>      MachineClass *mc = MACHINE_CLASS(oc);
>>> +    AspeedMachineClass *amc = ASPEED_MACHINE_CLASS(oc);
>>>  
>>>      mc->init = aspeed_machine_init;
>>>      mc->no_floppy = 1;
>>>      mc->no_cdrom = 1;
>>>      mc->no_parallel = 1;
>>>      mc->default_ram_id = "ram";
>>> +    amc->macs_mask = ASPEED_MAC0_ON;

Abstract base type's .class_init() sets AspeedMachineClass's macs_mask
to "just first one".

>>>  
>>>      aspeed_machine_class_props_init(oc);
>>>  }
>>> @@ -680,6 +684,7 @@ static void aspeed_machine_ast2600_evb_class_init(ObjectClass *oc, void *data)
>>>      amc->fmc_model = "w25q512jv";
>>>      amc->spi_model = "mx66u51235f";
>>>      amc->num_cs    = 1;
>>> +    amc->macs_mask  = ASPEED_MAC1_ON | ASPEED_MAC2_ON | ASPEED_MAC3_ON;
>>>      amc->i2c_init  = ast2600_evb_i2c_init;
>>>      mc->default_ram_size = 1 * GiB;
>>>      mc->default_cpus = mc->min_cpus = mc->max_cpus =
>>> @@ -698,6 +703,7 @@ static void aspeed_machine_tacoma_class_init(ObjectClass *oc, void *data)
>>>      amc->fmc_model = "mx66l1g45g";
>>>      amc->spi_model = "mx66l1g45g";
>>>      amc->num_cs    = 2;
>>> +    amc->macs_mask  = ASPEED_MAC2_ON;
>>>      amc->i2c_init  = witherspoon_bmc_i2c_init; /* Same board layout */
>>>      mc->default_ram_size = 1 * GiB;
>>>      mc->default_cpus = mc->min_cpus = mc->max_cpus =

Two concrete types' .class_init() override this default.

Fine with me.

>>> diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
>>> index 114b94f8f44d..fa85122f6d78 100644
>>> --- a/hw/arm/aspeed_ast2600.c
>>> +++ b/hw/arm/aspeed_ast2600.c
>>> @@ -247,6 +247,7 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
>>>      AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s);
>>>      Error *err = NULL, *local_err = NULL;
>>>      qemu_irq irq;
>>> +    NICInfo *nd = &nd_table[0];
>>>  
>>>      /* IO space */
>>>      create_unimplemented_device("aspeed_soc.io", sc->memmap[ASPEED_IOMEM],
>>> @@ -462,8 +463,12 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
>>>      }
>>>  
>>>      /* Net */
>>> -    for (i = 0; i < nb_nics && i < sc->macs_num; i++) {
>>> -        qdev_set_nic_properties(DEVICE(&s->ftgmac100[i]), &nd_table[i]);
>>> +    for (i = 0; i < sc->macs_num; i++) {
>>> +        if ((s->macs_mask & (1 << i)) && nd->used) {

This checks AspeedSocState's member.

>>> +            qemu_check_nic_model(nd, TYPE_FTGMAC100);
>>> +            qdev_set_nic_properties(DEVICE(&s->ftgmac100[i]), nd);
>>> +            nd++;
>>> +        }
>>>          object_property_set_bool(OBJECT(&s->ftgmac100[i]), true, "aspeed",
>>>                                   &err);
>>>          object_property_set_bool(OBJECT(&s->ftgmac100[i]), true, "realized",
>>> @@ -471,7 +476,7 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
>>>          error_propagate(&err, local_err);
>>>          if (err) {
>>>              error_propagate(errp, err);
>>> -           return;
>>> +            return;
>>>          }
>>>          sysbus_mmio_map(SYS_BUS_DEVICE(&s->ftgmac100[i]), 0,
>>>                          sc->memmap[ASPEED_ETH1 + i]);
>>> diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
>>> index 984d29087dce..d2c6a5760790 100644
>>> --- a/hw/arm/aspeed_soc.c
>>> +++ b/hw/arm/aspeed_soc.c
>>> @@ -234,6 +234,7 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
>>>      AspeedSoCState *s = ASPEED_SOC(dev);
>>>      AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s);
>>>      Error *err = NULL, *local_err = NULL;
>>> +    NICInfo *nd = &nd_table[0];
>>>  
>>>      /* IO space */
>>>      create_unimplemented_device("aspeed_soc.io", sc->memmap[ASPEED_IOMEM],
>>> @@ -405,8 +406,12 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
>>>      }
>>>  
>>>      /* Net */
>>> -    for (i = 0; i < nb_nics && i < sc->macs_num; i++) {
>>> -        qdev_set_nic_properties(DEVICE(&s->ftgmac100[i]), &nd_table[i]);
>>> +    for (i = 0; i < sc->macs_num; i++) {
>>> +        if ((s->macs_mask & (1 << i)) && nd->used) {

Likewise.

Now my rephrased question: why do you need to store macs_mask both in
AspeedSoCState and in AspeedMachineClass?  As far as I can tell, their
values are the same for any given machine.

>>> +            qemu_check_nic_model(nd, TYPE_FTGMAC100);
>>> +            qdev_set_nic_properties(DEVICE(&s->ftgmac100[i]), nd);
>>> +            nd++;
>>> +        }
>>>          object_property_set_bool(OBJECT(&s->ftgmac100[i]), true, "aspeed",
>>>                                   &err);
>>>          object_property_set_bool(OBJECT(&s->ftgmac100[i]), true, "realized",
>>> @@ -455,6 +460,7 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
>>>                         aspeed_soc_get_irq(s, ASPEED_SDHCI));
>>>  }
>>>  static Property aspeed_soc_properties[] = {
>>> +    DEFINE_PROP_UINT32("macs-mask", AspeedSoCState, macs_mask, 0x1),

Shouldn't this be ASPEED_MAC0_ON rather than 0x1?

Hmm, isn't aspeed_machine_class_init()'s amc->macs_mask = ASPEED_MAC0_ON
redundant with this?

>>>      DEFINE_PROP_LINK("dram", AspeedSoCState, dram_mr, TYPE_MEMORY_REGION,
>>>                       MemoryRegion *),
>>>      DEFINE_PROP_END_OF_LIST(),
>>