Re: [PATCH for-5.0 2/4] xics: Link ICP_PROP_XICS property to ICPState::xics pointer

[Cédric Le Goater]

On 18/11/2019 00:20, Greg Kurz wrote:
> The ICP object has both a pointer and an ICP_PROP_XICS property pointing
> to the XICS fabric. Confusing bugs could arise if these ever go out of
> sync.
> 
> Change the property definition so that it explicitely sets the pointer.

explicitly

> The property isn't optional : not being able to set the link is a bug
> and QEMU should rather abort than exit in this case.
> 
> Signed-off-by: Greg Kurz <address@hidden>

Reviewed-by: Cédric Le Goater <address@hidden>

> ---
>  hw/intc/xics.c |   22 +++++++++-------------
>  1 file changed, 9 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index f7a454808992..35dddb88670e 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -310,15 +310,7 @@ static void icp_realize(DeviceState *dev, Error **errp)
>      Object *obj;
>      Error *err = NULL;
>  
> -    obj = object_property_get_link(OBJECT(dev), ICP_PROP_XICS, &err);
> -    if (!obj) {
> -        error_propagate_prepend(errp, err,
> -                                "required link '" ICP_PROP_XICS
> -                                "' not found: ");
> -        return;
> -    }
> -
> -    icp->xics = XICS_FABRIC(obj);
> +    assert(icp->xics);
>  
>      obj = object_property_get_link(OBJECT(dev), ICP_PROP_CPU, &err);
>      if (!obj) {
> @@ -368,12 +360,19 @@ static void icp_unrealize(DeviceState *dev, Error 
> **errp)
>      vmstate_unregister(NULL, &vmstate_icp_server, icp);
>  }
>  
> +static Property icp_properties[] = {
> +    DEFINE_PROP_LINK(ICP_PROP_XICS, ICPState, xics, TYPE_XICS_FABRIC,
> +                     XICSFabric *),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
>  static void icp_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
>  
>      dc->realize = icp_realize;
>      dc->unrealize = icp_unrealize;
> +    dc->props = icp_properties;
>      /*
>       * Reason: part of XICS interrupt controller, needs to be wired up
>       * by icp_create().
> @@ -397,9 +396,7 @@ Object *icp_create(Object *cpu, const char *type, 
> XICSFabric *xi, Error **errp)
>      obj = object_new(type);
>      object_property_add_child(cpu, type, obj, &error_abort);
>      object_unref(obj);
> -    object_ref(OBJECT(xi));
> -    object_property_add_const_link(obj, ICP_PROP_XICS, OBJECT(xi),
> -                                   &error_abort);
> +    object_property_set_link(obj, OBJECT(xi), ICP_PROP_XICS, &error_abort);
>      object_ref(cpu);
>      object_property_add_const_link(obj, ICP_PROP_CPU, cpu, &error_abort);
>      object_property_set_bool(obj, true, "realized", &local_err);
> @@ -417,7 +414,6 @@ void icp_destroy(ICPState *icp)
>      Object *obj = OBJECT(icp);
>  
>      object_unref(object_property_get_link(obj, ICP_PROP_CPU, &error_abort));
> -    object_unref(object_property_get_link(obj, ICP_PROP_XICS, &error_abort));
>      object_unparent(obj);
>  }
>  
>