+Vitaly
On Sat, May 29, 2021 at 11:13:12AM +0200, Claudio Fontana wrote:
> we need to expand features first, before we attempt to check them
> in the accel realizefn code called by cpu_exec_realizefn().
>
> At the same time we need checks for code_urev and host_cpuid_required,
> and modifications to cpu->mwait to happen after the initial setting
> of them inside the accel realizefn code.
I miss an explanation why those 3 steps need to happen after
accel realizefn.
I'm worried by the fragility of the ordering. If there are
specific things that must be done before/after
cpu_exec_realizefn(), this needs to be clear in the code.
>
> Partial Fix.
>
> Fixes: 48afe6e4eabf ("i386: split cpu accelerators from cpu.c, using AccelCPUClass")
Shouldn't this be
f5cc5a5c1686 ("i386: split cpu accelerators from cpu.c, using AccelCPUClass")
?
Also, it looks like part of the ordering change was made in
commit 30565f10e907 ("cpu: call AccelCPUClass::cpu_realizefn in
cpu_exec_realizefn").
> Signed-off-by: Claudio Fontana <cfontana@xxxxxxx>
> ---
> target/i386/cpu.c | 56 +++++++++++++++++++++++------------------------
> 1 file changed, 28 insertions(+), 28 deletions(-)
>
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 9e211ac2ce..6bcb7dbc2c 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -6133,34 +6133,6 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
> Error *local_err = NULL;
> static bool ht_warned;
>
> - /* Process Hyper-V enlightenments */
> - x86_cpu_hyperv_realize(cpu);
Vitaly, is this reordering going to affect the Hyper-V cleanup
work you are doing? It seems harmless and it makes sense to keep
the "realize" functions close together, but I'd like to confirm.
> -
> - cpu_exec_realizefn(cs, &local_err);
> - if (local_err != NULL) {
> - error_propagate(errp, local_err);
> - return;
> - }
> -
> - if (xcc->host_cpuid_required && !accel_uses_host_cpuid()) {
> - g_autofree char *name = x86_cpu_class_get_model_name(xcc);
> - error_setg(&local_err, "CPU model '%s' requires KVM or HVF", name);
> - goto out;
> - }
> -
> - if (cpu->ucode_rev == 0) {
> - /* The default is the same as KVM's. */
> - if (IS_AMD_CPU(env)) {
> - cpu->ucode_rev = 0x01000065;
> - } else {
> - cpu->ucode_rev = 0x100000000ULL;
> - }
> - }
> -
> - /* mwait extended info: needed for Core compatibility */
> - /* We always wake on interrupt even if host does not have the capability */
> - cpu->mwait.ecx |= CPUID_MWAIT_EMX | CPUID_MWAIT_IBE;
> -
> if (cpu->apic_id == UNASSIGNED_APIC_ID) {
> error_setg(errp, "apic-id property was not initialized properly");
> return;
> @@ -6190,6 +6162,34 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
> & CPUID_EXT2_AMD_ALIASES);
> }
>
> + /* Process Hyper-V enlightenments */
> + x86_cpu_hyperv_realize(cpu);
> +
> + cpu_exec_realizefn(cs, &local_err);
I'm worried by the reordering of cpu_exec_realizefn(). That
function does a lot, and reordering it might have even more
unwanted side effects.
I wonder if it wouldn't be easier to revert commit 30565f10e907
("cpu: call AccelCPUClass::cpu_realizefn in cpu_exec_realizefn").
> + if (local_err != NULL) {
> + error_propagate(errp, local_err);
> + return;
> + }
> +
> + if (xcc->host_cpuid_required && !accel_uses_host_cpuid()) {
> + g_autofree char *name = x86_cpu_class_get_model_name(xcc);
> + error_setg(&local_err, "CPU model '%s' requires KVM or HVF", name);
> + goto out;
> + }
> +
> + if (cpu->ucode_rev == 0) {
> + /* The default is the same as KVM's. */
> + if (IS_AMD_CPU(env)) {
> + cpu->ucode_rev = 0x01000065;
> + } else {
> + cpu->ucode_rev = 0x100000000ULL;
> + }
> + }
> +
> + /* mwait extended info: needed for Core compatibility */
> + /* We always wake on interrupt even if host does not have the capability */
> + cpu->mwait.ecx |= CPUID_MWAIT_EMX | CPUID_MWAIT_IBE;
> +
The dependency between those lines and cpu_exec_realizefn() is
completely unclear here. What can we do to make this clearer and
less fragile?
Note that this is not a comment on this fix, specifically, but on
how the initialization ordering is easy to break here.
> /* For 64bit systems think about the number of physical bits to present.
> * ideally this should be the same as the host; anything other than matching
> * the host can cause incorrect guest behaviour.
> --
> 2.26.2
>
--
Eduardo
