Re: [PATCH v15 21/23] hw/core/cpu: call qemu_init_vcpu in cpu_common_realizefn - Philippe Mathieu-Daudé

From: "Philippe Mathieu-Daudé" <philmd@redhat.com>
To: "Claudio Fontana" <cfontana@suse.de>,
	"Alex Bennée" <alex.bennee@linaro.org>,
	"Peter Maydell" <peter.maydell@linaro.org>,
	"Laurent Vivier" <lvivier@redhat.com>
Cc: Thomas Huth <thuth@redhat.com>,
	Eduardo Habkost <ehabkost@redhat.com>,
	Richard Henderson <richard.henderson@linaro.org>,
	qemu-devel@nongnu.org, Roman Bolshakov <r.bolshakov@yadro.com>,
	Alistair Francis <alistair.francis@wdc.com>,
	Paolo Bonzini <pbonzini@redhat.com>
Subject: Re: [PATCH v15 21/23] hw/core/cpu: call qemu_init_vcpu in cpu_common_realizefn
Date: Thu, 4 Feb 2021 14:41:13 +0100	[thread overview]
Message-ID: <05980f44-88a6-6ef7-b263-07c2d898b8f0@redhat.com> (raw)
In-Reply-To: <7500a412-c94a-6990-eb48-9ee78bfb94e3@suse.de>

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().

See discussion:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg686480.html

> 
> 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).