[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v15 21/23] hw/core/cpu: call qemu_init_vcpu in cpu_common_rea
From: |
Claudio Fontana |
Subject: |
Re: [PATCH v15 21/23] hw/core/cpu: call qemu_init_vcpu in cpu_common_realizefn |
Date: |
Thu, 4 Feb 2021 15:18:40 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.12.0 |
On 2/4/21 2:41 PM, Philippe Mathieu-Daudé wrote:
> On 2/4/21 11:23 AM, Claudio Fontana wrote:
>> On 2/3/21 5:51 PM, Alex Bennée wrote:
>>>
>>> Claudio Fontana <cfontana@suse.de> writes:
>>>
>>>> move the call to qemu_init_vcpu inside cpu_common_realizefn,
>>>> so it does not need to be done explicitly in each target cpu.
>>>>
>>>> Despite this, the way cpu realize is done continues to be not ideal;
>>>>
>>>> ideally the cpu_list_add would be done in common_cpu,
>>>> and in this case we could avoid even more redundant open coded
>>>> additional calls in target/xxx/cpu.c,
>>>>
>>>> but this cannot happen because target cpu code, plugins, etc
>>>> now all came to rely on cpu->index
>>>> (which is updated in cpu_list_add), since no particular order
>>>> was defined previously, so we are stuck with the freak call
>>>> order for the target cpu realizefn.
>>>>
>>>> After this patch the target/xxx/cpu.c realizefn body becomes:
>>>>
>>>> void mycpu_realizefn(DeviceState *dev, Error **errp)
>>>> {
>>>> /* ... */
>>>> cpu_exec_realizefn(CPU_STATE(dev), errp);
>>>>
>>>> /* ... anything that needs done pre-qemu_vcpu_init */
>>>>
>>>> xcc->parent_realize(dev, errp); /* does qemu_vcpu_init */
>>>>
>>>> /* ... anything that needs to be done after qemu_vcpu_init */
>>>> }
>>>
>>> Uggh, introducing a magic order seems like inviting trouble for later
>>> on. Is there anyway we can improve things? Paolo?
>>>
>>
>>
>> The magic order is there already. I call it "freak order" instead of
>> "magic", because this is more the result of uncontrolled code growth rather
>> than the work of magic :-)
>
> Eventually related to this unsolved problem:
>
> cpu_reset() might modify architecture-specific fields allocated
> by qemu_init_vcpu(). To avoid bugs similar to the one fixed in
> commit 00d0f7cb66 when introducing new architectures, move the
> cpu_reset() calls after qemu_init_vcpu().
Hi Philippe,
>
> See discussion:
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg686480.html
thanks to the pointer, it is useful.
Maybe there is a point to try to face this challenge.
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.
I could cook the change as at least an aspirational goal, if there is a way to
test it maybe it could help targets get progressively fixed in that direction?
It should be an RFC outside of this context I guess.
Ciao, thanks,
Claudio
>
>>
>> This patch attempts to remove one degree of freedom, but the current
>> situation of cpu realizing is basically fubar. This patch attempts to
>> improve things slightly,
>> but as mentioned elsewhere it basically fails to achieve the goal,
>>
>> so I am tempted to just retire it. Maybe someone interested could look at
>> the situation with new eyes (possibly even me later on).
>
- Re: [PATCH v15 22/23] accel: introduce new accessor functions, (continued)
[PATCH v15 21/23] hw/core/cpu: call qemu_init_vcpu in cpu_common_realizefn, Claudio Fontana, 2021/02/01
[PATCH v15 19/23] i386: split cpu accelerators from cpu.c, using AccelCPUClass, Claudio Fontana, 2021/02/01
[PATCH v15 18/23] accel: introduce AccelCPUClass extending CPUClass, Claudio Fontana, 2021/02/01
[PATCH v15 17/23] accel: replace struct CpusAccel with AccelOpsClass, Claudio Fontana, 2021/02/01