Re: [Qemu-devel] [PATCH 2/3] spapr: Clean up misuse of qdev_init() in xics-kvm creation

[Markus Armbruster]

David, your commit 11ad93f suggests you're highly qualified to review.
No good deed shall go unpunished ;)

Markus Armbruster <address@hidden> writes:

> We call try_create_xics() to create a "xics-kvm".  If it fails, we
> call it again to fall back to plain "xics".
>
> try_create_xics() uses qdev_init().  qdev_init()'s error handling has
> an unwanted side effect: it calls qerror_report_err(), which prints to
> stderr.  Looks like an error, but isn't.
>
> In QMP context, it would stash the error in the monitor instead,
> making the QMP command fail.  Fortunately, it's only called from board
> initialization, never in QMP context.
>
> Clean up by cutting out the qdev_init() middle-man: set property
> "realized" directly.
>
> While there, improve the error message when we can't satisfy an
> explicit user request for "xics-kvm", and exit(1) instead of abort().
> Simplify the abort when we can't create "xics".
>
> Cc: Alexander Graf <address@hidden>
> Cc: address@hidden
> Signed-off-by: Markus Armbruster <address@hidden>
> ---
>  hw/ppc/spapr.c | 25 ++++++++++++-------------
>  1 file changed, 12 insertions(+), 13 deletions(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index b560459..de6d0bb 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -109,17 +109,20 @@ struct sPAPRMachineState {
>  sPAPREnvironment *spapr;
>  
>  static XICSState *try_create_xics(const char *type, int nr_servers,
> -                                  int nr_irqs)
> +                                  int nr_irqs, Error **errp)
>  {
> +    Error *err;
>      DeviceState *dev;
>  
>      dev = qdev_create(NULL, type);
>      qdev_prop_set_uint32(dev, "nr_servers", nr_servers);
>      qdev_prop_set_uint32(dev, "nr_irqs", nr_irqs);
> -    if (qdev_init(dev) < 0) {
> +    object_property_set_bool(OBJECT(dev), true, "realized", &err);
> +    if (err) {
> +        error_propagate(errp, err);
> +        object_unparent(OBJECT(dev));
>          return NULL;
>      }
> -
>      return XICS_COMMON(dev);
>  }
>  
> @@ -133,23 +136,19 @@ static XICSState *xics_system_init(int nr_servers, int 
> nr_irqs)
>                                                  "kernel_irqchip", true);
>          bool irqchip_required = qemu_opt_get_bool(machine_opts,
>                                                    "kernel_irqchip", false);
> +        Error *err = NULL;
> +
>          if (irqchip_allowed) {
> -            icp = try_create_xics(TYPE_KVM_XICS, nr_servers, nr_irqs);
> +            icp = try_create_xics(TYPE_KVM_XICS, nr_servers, nr_irqs, &err);
>          }
> -
>          if (irqchip_required && !icp) {
> -            perror("Failed to create in-kernel XICS\n");
> -            abort();
> +            error_report("kernel_irqchip requested but unavailable: %s",
> +                         error_get_pretty(err));
>          }
>      }
>  
>      if (!icp) {
> -        icp = try_create_xics(TYPE_XICS, nr_servers, nr_irqs);
> -    }
> -
> -    if (!icp) {
> -        perror("Failed to create XICS\n");
> -        abort();
> +        icp = try_create_xics(TYPE_XICS, nr_servers, nr_irqs, &error_abort);
>      }
>  
>      return icp;