Re: [PATCH 14/33] hw/cpu/arm: Handle 'has_el2/3' properties once in MPCore parent

[Fabiano Rosas <farosas@suse.de>]

Philippe Mathieu-Daudé <philmd@linaro.org> writes:

> Move the 'has_el2' and 'has_el3' properties to the abstract
> QOM parent.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  include/hw/cpu/cortex_mpcore.h |  5 +++++
>  hw/arm/exynos4210.c            | 10 ++++++++--
>  hw/arm/vexpress.c              |  6 ++++++
>  hw/arm/xilinx_zynq.c           |  6 ++++++
>  hw/cpu/a15mpcore.c             | 18 ++++++------------
>  hw/cpu/a9mpcore.c              |  5 +----
>  hw/cpu/cortex_mpcore.c         |  3 +++
>  7 files changed, 35 insertions(+), 18 deletions(-)
>
> diff --git a/include/hw/cpu/cortex_mpcore.h b/include/hw/cpu/cortex_mpcore.h
> index 0e7cca9e93..387552468c 100644
> --- a/include/hw/cpu/cortex_mpcore.h
> +++ b/include/hw/cpu/cortex_mpcore.h
> @@ -30,6 +30,8 @@
>   * QEMU interface:
>   *  + QOM property "num-cores" which set the number of cores present in
>   *    the cluster.
> + *  + QOM properties "cpu-has-el3", "cpu-has-el2" which set whether the CPUs
> + *    have the exception level features present.
>   */
>  #define TYPE_CORTEX_MPCORE_PRIV "cortex_mpcore_priv"
>  OBJECT_DECLARE_TYPE(CortexMPPrivState, CortexMPPrivClass, CORTEX_MPCORE_PRIV)
> @@ -53,6 +55,9 @@ struct CortexMPPrivState {
>  
>      /* Properties */
>      uint32_t num_cores;
> +
> +    bool cpu_has_el3;
> +    bool cpu_has_el2;
>  };
>  
>  #define TYPE_A9MPCORE_PRIV "a9mpcore_priv"
> diff --git a/hw/arm/exynos4210.c b/hw/arm/exynos4210.c
> index ea1364499d..7386a8fe57 100644
> --- a/hw/arm/exynos4210.c
> +++ b/hw/arm/exynos4210.c
> @@ -548,7 +548,7 @@ static void exynos4210_realize(DeviceState *socdev, Error **errp)
>      Exynos4210State *s = EXYNOS4210_SOC(socdev);
>      MemoryRegion *system_mem = get_system_memory();
>      SysBusDevice *busdev;
> -    DeviceState *dev, *uart[4], *pl330[3];
> +    DeviceState *dev, *mpdev, *uart[4], *pl330[3];
>      int i, n;
>  
>      for (n = 0; n < EXYNOS4210_NCPUS; n++) {
> @@ -582,7 +582,13 @@ static void exynos4210_realize(DeviceState *socdev, Error **errp)
>      }
>  
>      /* Private memory region and Internal GIC */
> -    qdev_prop_set_uint32(DEVICE(&s->a9mpcore), "num-cores", EXYNOS4210_NCPUS);
> +    mpdev = DEVICE(&s->a9mpcore);
> +    qdev_prop_set_uint32(mpdev, "num-cores", EXYNOS4210_NCPUS);
> +    /*
> +     * By default A9 CPUs have EL3 enabled.  This board does not currently
> +     * support EL3 so the CPU EL3 property is disabled before realization.
> +     */
> +    qdev_prop_set_bit(mpdev, "cpu-has-el3", false);
>      busdev = SYS_BUS_DEVICE(&s->a9mpcore);
>      sysbus_realize(busdev, &error_fatal);
>      sysbus_mmio_map(busdev, 0, EXYNOS4210_SMP_PRIVATE_BASE_ADDR);
> diff --git a/hw/arm/vexpress.c b/hw/arm/vexpress.c
> index a320d1c181..294b6f15f2 100644
> --- a/hw/arm/vexpress.c
> +++ b/hw/arm/vexpress.c
> @@ -239,6 +239,12 @@ static void init_cpus(MachineState *ms, const char *cpu_type,
>       * wires itself up to the CPU's generic_timer gpio out lines.
>       */
>      dev = qdev_new(privdev);
> +    if (!secure) {
> +        qdev_prop_set_bit(dev, "cpu-has-el3", false);
> +    }
> +    if (!virt) {
> +        qdev_prop_set_bit(dev, "cpu-has-el2", false);
> +    }
>      qdev_prop_set_uint32(dev, "num-cpu", smp_cpus);
>      busdev = SYS_BUS_DEVICE(dev);
>      sysbus_realize_and_unref(busdev, &error_fatal);
> diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
> index fc9c927d09..28430dcfba 100644
> --- a/hw/arm/xilinx_zynq.c
> +++ b/hw/arm/xilinx_zynq.c
> @@ -240,6 +240,12 @@ static void zynq_init(MachineState *machine)
>  
>      dev = qdev_new(TYPE_A9MPCORE_PRIV);
>      qdev_prop_set_uint32(dev, "num-cpu", 1);
> +    /*
> +     * By default A9 CPUs have EL3 enabled.  This board does not
> +     * currently support EL3 so the CPU EL3 property is disabled before
> +     * realization.
> +     */
> +    qdev_prop_set_bit(dev, "cpu-has-el3", false);
>      busdev = SYS_BUS_DEVICE(dev);
>      sysbus_realize_and_unref(busdev, &error_fatal);
>      sysbus_mmio_map(busdev, 0, MPCORE_PERIPHBASE);
> diff --git a/hw/cpu/a15mpcore.c b/hw/cpu/a15mpcore.c
> index 16874426e1..a16544fdde 100644
> --- a/hw/cpu/a15mpcore.c
> +++ b/hw/cpu/a15mpcore.c
> @@ -52,9 +52,6 @@ static void a15mp_priv_realize(DeviceState *dev, Error **errp)
>      SysBusDevice *gicsbd;
>      Error *local_err = NULL;
>      int i;
> -    bool has_el3;
> -    bool has_el2 = false;
> -    Object *cpuobj;
>  
>      cc->parent_realize(dev, &local_err);
>      if (local_err) {
> @@ -70,14 +67,11 @@ static void a15mp_priv_realize(DeviceState *dev, Error **errp)
>          /* Make the GIC's TZ support match the CPUs. We assume that
>           * either all the CPUs have TZ, or none do.
>           */
> -        cpuobj = OBJECT(qemu_get_cpu(0));
> -        has_el3 = object_property_find(cpuobj, "has_el3") &&
> -            object_property_get_bool(cpuobj, "has_el3", &error_abort);
> -        qdev_prop_set_bit(gicdev, "has-security-extensions", has_el3);
> +        qdev_prop_set_bit(gicdev, "has-security-extensions",
> +                          c->cpu_has_el3);
>          /* Similarly for virtualization support */
> -        has_el2 = object_property_find(cpuobj, "has_el2") &&
> -            object_property_get_bool(cpuobj, "has_el2", &error_abort);
> -        qdev_prop_set_bit(gicdev, "has-virtualization-extensions", has_el2);
> +        qdev_prop_set_bit(gicdev, "has-virtualization-extensions",
> +                          c->cpu_has_el2);
>      }
>  
>      if (!sysbus_realize(SYS_BUS_DEVICE(&s->gic), errp)) {
> @@ -112,7 +106,7 @@ static void a15mp_priv_realize(DeviceState *dev, Error **errp)
>                                    qdev_get_gpio_in(gicdev,
>                                                     ppibase + timer_irq[irq]));
>          }
> -        if (has_el2) {
> +        if (c->cpu_has_el2) {
>              /* Connect the GIC maintenance interrupt to PPI ID 25 */
>              sysbus_connect_irq(SYS_BUS_DEVICE(gicdev), i + 4 * c->num_cores,
>                                 qdev_get_gpio_in(gicdev, ppibase + 25));
> @@ -134,7 +128,7 @@ static void a15mp_priv_realize(DeviceState *dev, Error **errp)
>                                  sysbus_mmio_get_region(gicsbd, 0));
>      memory_region_add_subregion(&c->container, 0x2000,
>                                  sysbus_mmio_get_region(gicsbd, 1));
> -    if (has_el2) {
> +    if (c->cpu_has_el2) {
>          memory_region_add_subregion(&c->container, 0x4000,
>                                      sysbus_mmio_get_region(gicsbd, 2));
>          memory_region_add_subregion(&c->container, 0x6000,
> diff --git a/hw/cpu/a9mpcore.c b/hw/cpu/a9mpcore.c
> index 9c138f4442..54949314f8 100644
> --- a/hw/cpu/a9mpcore.c
> +++ b/hw/cpu/a9mpcore.c
> @@ -51,7 +51,6 @@ static void a9mp_priv_realize(DeviceState *dev, Error **errp)
>      SysBusDevice *scubusdev, *gicbusdev, *gtimerbusdev, *mptimerbusdev,
>                   *wdtbusdev;
>      Error *local_err = NULL;
> -    bool has_el3;
>      CPUState *cpu0;
>      Object *cpuobj;
>  
> @@ -86,9 +85,7 @@ static void a9mp_priv_realize(DeviceState *dev, Error **errp)
>      /* Make the GIC's TZ support match the CPUs. We assume that
>       * either all the CPUs have TZ, or none do.
>       */
> -    has_el3 = object_property_find(cpuobj, "has_el3") &&
> -        object_property_get_bool(cpuobj, "has_el3", &error_abort);
> -    qdev_prop_set_bit(gicdev, "has-security-extensions", has_el3);
> +    qdev_prop_set_bit(gicdev, "has-security-extensions", c->cpu_has_el3);
>  
>      if (!sysbus_realize(SYS_BUS_DEVICE(&s->gic), errp)) {
>          return;
> diff --git a/hw/cpu/cortex_mpcore.c b/hw/cpu/cortex_mpcore.c
> index d7ea633e4e..549b81f708 100644
> --- a/hw/cpu/cortex_mpcore.c
> +++ b/hw/cpu/cortex_mpcore.c
> @@ -27,6 +27,9 @@ static Property cortex_mpcore_priv_properties[] = {
>      DEFINE_PROP_UINT32("num-cores", CortexMPPrivState, num_cores, 1),
>      DEFINE_PROP_UINT32("num-cpu", CortexMPPrivState, num_cores, 1), /* alias */
>  
> +    DEFINE_PROP_BOOL("cpu-has-el3", CortexMPPrivState, cpu_has_el3, true),
> +    DEFINE_PROP_BOOL("cpu-has-el2", CortexMPPrivState, cpu_has_el2, false),

Are we missing setting cpu_has_el2 somewhere else? This^ results in fewer
cpregs being registered and is what breaks migration.

You can test with:

$ (echo "migrate file:migfile"; echo "quit") | ./qemu-system-arm -M ast2600-evb -monitor stdio
$ ./scripts/analyze-migration.py -f migfile | grep array_len

Before series:
$ ./scripts/analyze-migration.py -f migfile | grep array_len
        "cpreg_vmstate_array_len": "0x0000010a",
        "cpreg_vmstate_array_len": "0x0000010a",

After series:
$ ./scripts/analyze-migration.py -f migfile | grep array_len
        "cpreg_vmstate_array_len": "0x000000df",
        "cpreg_vmstate_array_len": "0x000000df",