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

[Philippe Mathieu-Daudé]

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.