| Message ID | 20231212162935.42910-15-philmd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | hw/cpu/arm: Remove one use of qemu_get_cpu() in A7/A15 MPCore priv | expand |
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",
On 12/1/24 22:33, Fabiano Rosas wrote: > 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; >> }; >> 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", Thank you Fabiano for helping debugging. Indeed there is a problem with properties, so this patch is bogus. The good news is the QOM reparenting happened 2 commits earlier, so this discarded the doubts on qom-path change possibly affecting migration :) Regards, Phil.
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), + DEFINE_PROP_END_OF_LIST(), };
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(-)