Re: [PATCH 2/6] xive, xics: Fix reference counting on CPU objects

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3312 bytes --]

On Wed, Oct 23, 2019 at 04:52:05PM +0200, Greg Kurz wrote:
> When a VCPU gets connected to the XIVE interrupt controller, we add a
> const link targetting the CPU object to the TCTX object. Similar links
> are added to the ICP object when using the XICS interrupt controller.
> 
> As explained in <qom/object.h>:
> 
>  * The caller must ensure that @target stays alive as long as
>  * this property exists.  In the case @target is a child of @obj,
>  * this will be the case.  Otherwise, the caller is responsible for
>  * taking a reference.
> 
> We're in the latter case for both XICS and XIVE. Add the missing
> calls to object_ref() and object_unref().
> 
> This doesn't fix any known issue because the life cycle of the TCTX or
> ICP happens to be shorter than the one of the CPU or XICS fabric, but
> better safe than sorry.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

LGTM
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  hw/intc/xics.c |    8 +++++++-
>  hw/intc/xive.c |    6 +++++-
>  2 files changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index 935f325749cb..5f746079be46 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -388,8 +388,10 @@ 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_ref(cpu);
>      object_property_add_const_link(obj, ICP_PROP_CPU, cpu, &error_abort);
>      object_property_set_bool(obj, true, "realized", &local_err);
>      if (local_err) {
> @@ -403,7 +405,11 @@ Object *icp_create(Object *cpu, const char *type, XICSFabric *xi, Error **errp)
>  
>  void icp_destroy(ICPState *icp)
>  {
> -    object_unparent(OBJECT(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);
>  }
>  
>  /*
> diff --git a/hw/intc/xive.c b/hw/intc/xive.c
> index 38257aa02083..952a461d5329 100644
> --- a/hw/intc/xive.c
> +++ b/hw/intc/xive.c
> @@ -682,6 +682,7 @@ Object *xive_tctx_create(Object *cpu, XiveRouter *xrtr, Error **errp)
>      obj = object_new(TYPE_XIVE_TCTX);
>      object_property_add_child(cpu, TYPE_XIVE_TCTX, obj, &error_abort);
>      object_unref(obj);
> +    object_ref(cpu);
>      object_property_add_const_link(obj, "cpu", cpu, &error_abort);
>      object_property_set_bool(obj, true, "realized", &local_err);
>      if (local_err) {
> @@ -698,7 +699,10 @@ error:
>  
>  void xive_tctx_destroy(XiveTCTX *tctx)
>  {
> -    object_unparent(OBJECT(tctx));
> +    Object *obj = OBJECT(tctx);
> +
> +    object_unref(object_property_get_link(obj, "cpu", &error_abort));
> +    object_unparent(obj);
>  }
>  
>  /*
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]