Re: [PATCH v15 23/23] accel-cpu: make cpu_realizefn return a bool

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 2/1/21 11:09 AM, Claudio Fontana wrote:
> overall, all devices' realize functions take an Error **errp, but return void.
> 
> hw/core/qdev.c code, which realizes devices, therefore does:
> 
> local_err = NULL;
> dc->realize(dev, &local_err);
> if (local_err != NULL) {
>     goto fail;
> }
> 
> However, we can improve at least accel_cpu to return a meaningful bool value.
> 
> Signed-off-by: Claudio Fontana <cfontana@suse.de>
> ---
>  include/hw/core/accel-cpu.h |  2 +-
>  include/qemu/accel.h        |  2 +-
>  target/i386/host-cpu.h      |  2 +-
>  accel/accel-common.c        |  6 +++---
>  cpu.c                       |  5 +++--
>  target/i386/host-cpu.c      | 25 ++++++++++++++-----------
>  target/i386/kvm/kvm-cpu.c   |  4 ++--
>  target/i386/tcg/tcg-cpu.c   |  6 ++++--
>  8 files changed, 29 insertions(+), 23 deletions(-)
...

> diff --git a/target/i386/host-cpu.c b/target/i386/host-cpu.c
> index 9cfe56ce41..4ea9e354ea 100644
> --- a/target/i386/host-cpu.c
> +++ b/target/i386/host-cpu.c
> @@ -50,7 +50,7 @@ static void host_cpu_enable_cpu_pm(X86CPU *cpu)
>      env->features[FEAT_1_ECX] |= CPUID_EXT_MONITOR;
>  }
>  
> -static uint32_t host_cpu_adjust_phys_bits(X86CPU *cpu, Error **errp)
> +static uint32_t host_cpu_adjust_phys_bits(X86CPU *cpu)
>  {
>      uint32_t host_phys_bits = host_cpu_phys_bits();
>      uint32_t phys_bits = cpu->phys_bits;
> @@ -77,18 +77,10 @@ static uint32_t host_cpu_adjust_phys_bits(X86CPU *cpu, Error **errp)
>          }
>      }
>  
> -    if (phys_bits &&
> -        (phys_bits > TARGET_PHYS_ADDR_SPACE_BITS ||
> -         phys_bits < 32)) {
> -        error_setg(errp, "phys-bits should be between 32 and %u "
> -                   " (but is %u)",
> -                   TARGET_PHYS_ADDR_SPACE_BITS, phys_bits);
> -    }
> -
>      return phys_bits;
>  }
>  
> -void host_cpu_realizefn(CPUState *cs, Error **errp)
> +bool host_cpu_realizefn(CPUState *cs, Error **errp)
>  {
>      X86CPU *cpu = X86_CPU(cs);
>      CPUX86State *env = &cpu->env;
> @@ -97,8 +89,19 @@ void host_cpu_realizefn(CPUState *cs, Error **errp)
>          host_cpu_enable_cpu_pm(cpu);
>      }
>      if (env->features[FEAT_8000_0001_EDX] & CPUID_EXT2_LM) {
> -        cpu->phys_bits = host_cpu_adjust_phys_bits(cpu, errp);
> +        uint32_t phys_bits = host_cpu_adjust_phys_bits(cpu);
> +
> +        if (phys_bits &&
> +            (phys_bits > TARGET_PHYS_ADDR_SPACE_BITS ||
> +             phys_bits < 32)) {
> +            error_setg(errp, "phys-bits should be between 32 and %u "
> +                       " (but is %u)",
> +                       TARGET_PHYS_ADDR_SPACE_BITS, phys_bits);

Please this change in a preliminary patch (preferably), or comment it
in the commit description.
Either ways:
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> +            return false;
> +        }
> +        cpu->phys_bits = phys_bits;
>      }
> +    return true;
>  }