Re: [Qemu-ppc] [Qemu-devel] [PATCH 02/11] pseries: Cleanup error handling of spapr_cpu_init()

[Bharata B Rao]

On Fri, Dec 11, 2015 at 5:41 AM, David Gibson
<address@hidden> wrote:
> Currently spapr_cpu_init() is hardcoded to handle any errors as fatal.
> That works for now, since it's only called from initial setup where an
> error here means we really can't proceed.
>
> However, we'll want to handle this more flexibly for cpu hotplug in future
> so generalize this using the error reporting infrastructure.  While we're
> at it make a small cleanup in a related part of ppc_spapr_init() to use
> the error infrastructure instead of an old-style explicit fprintf / exit.
>
> Signed-off-by: David Gibson <address@hidden>
> ---
>  hw/ppc/spapr.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 1a5500f..91396cc 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1615,7 +1615,8 @@ static void spapr_boot_set(void *opaque, const char 
> *boot_device,
>      machine->boot_order = g_strdup(boot_device);
>  }
>
> -static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu)
> +static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu,
> +                           Error **errp)
>  {
>      CPUPPCState *env = &cpu->env;
>
> @@ -1633,7 +1634,7 @@ static void spapr_cpu_init(sPAPRMachineState *spapr, 
> PowerPCCPU *cpu)
>      }
>
>      if (cpu->max_compat) {
> -        ppc_set_compat(cpu, cpu->max_compat, &error_fatal);
> +        ppc_set_compat(cpu, cpu->max_compat, errp);
>      }
>
>      xics_cpu_setup(spapr->icp, cpu);
> @@ -1802,10 +1803,9 @@ static void ppc_spapr_init(MachineState *machine)
>      for (i = 0; i < smp_cpus; i++) {
>          cpu = cpu_ppc_init(machine->cpu_model);
>          if (cpu == NULL) {
> -            fprintf(stderr, "Unable to find PowerPC CPU definition\n");
> -            exit(1);
> +            error_setg(&error_fatal, "Unable to find PowerPC CPU 
> definition");
>          }
> -        spapr_cpu_init(spapr, cpu);
> +        spapr_cpu_init(spapr, cpu, &error_fatal);

Using error_fatal is fine here for now, but just want to sound out
that with CPU hotplug, spapr_cpu_init() would move to ->plug() where
we can't be calling this unconditionally with error_fatal as we need a
graceful recovery for hotplugged CPUs and termination for boot CPUs.

Otherwise, Reviewed-by: Bharata B Rao <address@hidden>

Regards,
Bharata.