Re: [PATCH 1/2] i386: reorder call to cpu_exec_realizefn in x86_cpu_realizefn

[Eduardo Habkost]

+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@suse.de>
> ---
>  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