Re: [Qemu-devel] [PATCH for-4.1?] hw/arm/fsl-imx6ul.c: Remove dead SMP-related code

[Philippe Mathieu-Daudé]

On 7/12/19 1:50 PM, Peter Maydell wrote:
> The i.MX6UL always has a single Cortex-A7 CPU (we set FSL_IMX6UL_NUM_CPUS
> to 1 in line with this). This means that all the code in fsl-imx6ul.c to
> handle multiple CPUs is dead code, and Coverity is now complaining that
> it is unreachable (CID 1403008, 1403011).
> 
> Remove the unreachable code and the only-executes-once loops,
> and replace the single-entry cpu[] array in the FSLIMX6ULState
> with a simple cpu member.

The header says "Based on hw/arm/fsl-imx7.c" which has
FSL_IMX7_NUM_CPUS=2, makes sense.

Looking at the i.MX 6:

include/hw/arm/fsl-imx6.h:38:#define FSL_IMX6_NUM_CPUS 4

Ok, we can have 1/2/4 cpus in the family.

The UltraLite series is part of the i.MX 6 family, I wonder why we
needed a different FslIMX6ULState than FslIMX6State.

Anyway, not related to this cleanup patch, so meanwhile:

Reviewed-by: Philippe Mathieu-Daudé <address@hidden>
Tested-by: Philippe Mathieu-Daudé <address@hidden>

> Signed-off-by: Peter Maydell <address@hidden>
> ---
> The only real reason to put this into 4.1 is because it fixes
> some Coverity issues, and it would be nice to be able to get
> down to no Coverity issues for the release. I think that pre-rc1
> that's a reasonable reason to put this in.

Agreed.

> Disclaimer: tested with "make check" as I have no test image for
> this board.
> 
>  include/hw/arm/fsl-imx6ul.h |  2 +-
>  hw/arm/fsl-imx6ul.c         | 62 +++++++++++--------------------------
>  hw/arm/mcimx6ul-evk.c       |  2 +-
>  3 files changed, 20 insertions(+), 46 deletions(-)
> 
> diff --git a/include/hw/arm/fsl-imx6ul.h b/include/hw/arm/fsl-imx6ul.h
> index 9e94e98f8ee..eda389aec7d 100644
> --- a/include/hw/arm/fsl-imx6ul.h
> +++ b/include/hw/arm/fsl-imx6ul.h
> @@ -61,7 +61,7 @@ typedef struct FslIMX6ULState {
>      DeviceState    parent_obj;
>  
>      /*< public >*/
> -    ARMCPU             cpu[FSL_IMX6UL_NUM_CPUS];
> +    ARMCPU             cpu;
>      A15MPPrivState     a7mpcore;
>      IMXGPTState        gpt[FSL_IMX6UL_NUM_GPTS];
>      IMXEPITState       epit[FSL_IMX6UL_NUM_EPITS];
> diff --git a/hw/arm/fsl-imx6ul.c b/hw/arm/fsl-imx6ul.c
> index f8601654388..b074177a71d 100644
> --- a/hw/arm/fsl-imx6ul.c
> +++ b/hw/arm/fsl-imx6ul.c
> @@ -29,16 +29,12 @@
>  
>  static void fsl_imx6ul_init(Object *obj)
>  {
> -    MachineState *ms = MACHINE(qdev_get_machine());
>      FslIMX6ULState *s = FSL_IMX6UL(obj);
>      char name[NAME_SIZE];
>      int i;
>  
> -    for (i = 0; i < MIN(ms->smp.cpus, FSL_IMX6UL_NUM_CPUS); i++) {
> -        snprintf(name, NAME_SIZE, "cpu%d", i);
> -        object_initialize_child(obj, name, &s->cpu[i], sizeof(s->cpu[i]),
> -                                "cortex-a7-" TYPE_ARM_CPU, &error_abort, 
> NULL);
> -    }
> +    object_initialize_child(obj, "cpu0", &s->cpu, sizeof(s->cpu),
> +                            "cortex-a7-" TYPE_ARM_CPU, &error_abort, NULL);
>  
>      /*
>       * A7MPCORE
> @@ -161,42 +157,25 @@ static void fsl_imx6ul_realize(DeviceState *dev, Error 
> **errp)
>      MachineState *ms = MACHINE(qdev_get_machine());
>      FslIMX6ULState *s = FSL_IMX6UL(dev);
>      int i;
> -    qemu_irq irq;
>      char name[NAME_SIZE];
> -    unsigned int smp_cpus = ms->smp.cpus;
> +    SysBusDevice *sbd;
> +    DeviceState *d;
>  
> -    if (smp_cpus > FSL_IMX6UL_NUM_CPUS) {
> -        error_setg(errp, "%s: Only %d CPUs are supported (%d requested)",
> -                   TYPE_FSL_IMX6UL, FSL_IMX6UL_NUM_CPUS, smp_cpus);
> +    if (ms->smp.cpus > 1) {
> +        error_setg(errp, "%s: Only a single CPU is supported (%d requested)",
> +                   TYPE_FSL_IMX6UL, ms->smp.cpus);
>          return;
>      }
>  
> -    for (i = 0; i < smp_cpus; i++) {
> -        Object *o = OBJECT(&s->cpu[i]);
> -
> -        object_property_set_int(o, QEMU_PSCI_CONDUIT_SMC,
> -                                "psci-conduit", &error_abort);
> -
> -        /* On uniprocessor, the CBAR is set to 0 */
> -        if (smp_cpus > 1) {
> -            object_property_set_int(o, FSL_IMX6UL_A7MPCORE_ADDR,
> -                                    "reset-cbar", &error_abort);
> -        }
> -
> -        if (i) {
> -            /* Secondary CPUs start in PSCI powered-down state */
> -            object_property_set_bool(o, true,
> -                                     "start-powered-off", &error_abort);
> -        }
> -
> -        object_property_set_bool(o, true, "realized", &error_abort);
> -    }
> +    object_property_set_int(OBJECT(&s->cpu), QEMU_PSCI_CONDUIT_SMC,
> +                            "psci-conduit", &error_abort);
> +    object_property_set_bool(OBJECT(&s->cpu), true,
> +                             "realized", &error_abort);
>  
>      /*
>       * A7MPCORE
>       */
> -    object_property_set_int(OBJECT(&s->a7mpcore), smp_cpus, "num-cpu",
> -                            &error_abort);
> +    object_property_set_int(OBJECT(&s->a7mpcore), 1, "num-cpu", 
> &error_abort);
>      object_property_set_int(OBJECT(&s->a7mpcore),
>                              FSL_IMX6UL_MAX_IRQ + GIC_INTERNAL,
>                              "num-irq", &error_abort);
> @@ -204,18 +183,13 @@ static void fsl_imx6ul_realize(DeviceState *dev, Error 
> **errp)
>                               &error_abort);
>      sysbus_mmio_map(SYS_BUS_DEVICE(&s->a7mpcore), 0, 
> FSL_IMX6UL_A7MPCORE_ADDR);
>  
> -    for (i = 0; i < smp_cpus; i++) {
> -        SysBusDevice *sbd = SYS_BUS_DEVICE(&s->a7mpcore);
> -        DeviceState  *d   = DEVICE(qemu_get_cpu(i));
> +    sbd = SYS_BUS_DEVICE(&s->a7mpcore);
> +    d = DEVICE(&s->cpu);
>  
> -        irq = qdev_get_gpio_in(d, ARM_CPU_IRQ);
> -        sysbus_connect_irq(sbd, i, irq);
> -        sysbus_connect_irq(sbd, i + smp_cpus, qdev_get_gpio_in(d, 
> ARM_CPU_FIQ));
> -        sysbus_connect_irq(sbd, i + 2 * smp_cpus,
> -                           qdev_get_gpio_in(d, ARM_CPU_VIRQ));
> -        sysbus_connect_irq(sbd, i + 3 * smp_cpus,
> -                           qdev_get_gpio_in(d, ARM_CPU_VFIQ));
> -    }
> +    sysbus_connect_irq(sbd, 0, qdev_get_gpio_in(d, ARM_CPU_IRQ));
> +    sysbus_connect_irq(sbd, 1, qdev_get_gpio_in(d, ARM_CPU_FIQ));
> +    sysbus_connect_irq(sbd, 2, qdev_get_gpio_in(d, ARM_CPU_VIRQ));
> +    sysbus_connect_irq(sbd, 3, qdev_get_gpio_in(d, ARM_CPU_VFIQ));
>  
>      /*
>       * A7MPCORE DAP
> diff --git a/hw/arm/mcimx6ul-evk.c b/hw/arm/mcimx6ul-evk.c
> index bbffb11c2a8..1f6f4aed97c 100644
> --- a/hw/arm/mcimx6ul-evk.c
> +++ b/hw/arm/mcimx6ul-evk.c
> @@ -71,7 +71,7 @@ static void mcimx6ul_evk_init(MachineState *machine)
>      }
>  
>      if (!qtest_enabled()) {
> -        arm_load_kernel(&s->soc.cpu[0], &boot_info);
> +        arm_load_kernel(&s->soc.cpu, &boot_info);
>      }
>  }
>  
>