Re: [PATCH v15 21/23] hw/core/cpu: call qemu_init_vcpu in cpu_common_realizefn

[Claudio Fontana]

On 2/4/21 3:24 PM, Peter Maydell wrote:
> On Thu, 4 Feb 2021 at 14:18, Claudio Fontana <cfontana@suse.de> wrote:
>> If we could consistently move not only qemu_vcpu_init, but also reset in the 
>> common code in cpu_common_realizefn,
>> and force the sequence: qemu_vcpu_init(); cpu_reset(); in there,
>>
>> and this actually works for all targets, maybe this could actually be an 
>> improvement.
> 
> So my question here would be: what is special about CPUs that
> we have to reset them in their realize function?  For other
> devices, we don't reset them in the init/realize sequence;
> we rely on the whole system getting reset and that doing
> reset of devices.
> 
> There almost certainly *is* a reason here (my guess would be
> that it's related to the mess that is reset and in particular
> that CPU objects don't get automatically reset on system reset
> because they're not on a qbus). But if we're thinking about
> aspirational goals, I think the aspiration should be to not
> need to reset the CPU in its own realize function at all...
> 
> thanks
> -- PMM
> 

Ok, but if we want to be really aspirational, the whole realizefn mechanism for 
cpu should be revisited.

The current calling sequence is nonsensical,
and probably leads to the kind of issues we see, as the target realizefn is the 
only real thing that controls the sequence of events really.

The object and device hierarchy and initialization order is completely ignored, 
and there is no structure provided by the framework as seen by the target 
implementer to incentive the right behavior when implementing a target.

Even the cpu_list_add is actually controlled via target realizefn, via 
cpu_exec_realizefn(). With other components that came to depend on this for 
cpu_index update.

In my view most of the code we currently see in target/xxx/cpu.c cpu_realize_fn 
should not be there at all,
and should be in the framework instead, with only the creation of resources 
specific to that target cpu needed to be there in realizefn ultimately.

Currently we have this situation:

static void xxx_cpu_realizefn(DeviceState *dev, Error **errp)
{
    Error local_err = NULL;
    CPUState *cs = CPU(dev);

    /* mix of target-specific resource creation here */

    /* mix of accelerator specific resource creation here */

    cpu_exec_realizefn(cs, &local_err); /* this updates the "cpus" list in 
cpus-common.c, and registers vmstate for the cpu */

    if (local_err != NULL) {
        error_propagate(errp, local_err);
        return;
    }

    /* mix of target-specific resource creation here */

    qemu_init_vcpu(cs); /* somewhere in the sequence somewhere after 
cpu_exec_realizefn

    /* mix of target-specific resource creation here */

    cpu_reset(cs); /* sometimes present, somewhere in the sequence after 
cpu_exec_realizefn, sometimes before init_vcpu, sometimes after.
    

    tcc->parent_realize(dev, errp); /* here we call the parent 
cpu_common_realizefn, which in case of hotplug does cpu_synchronize_post_init 
and cpu_resume.

    /* mix of target-specific resource creation here */
}


Can we do some of this boilerplate in the framework?

All of this is repeated every single time:

cpu_exec_realizefn()
qemu_init_vpcu()
cpu_reset() /* sometimes */
parent_realize()

with slight order variation between targets and apparently with bug potential.

Could we consolidate some or all of this?

Ciao, thanks,

Claudio