Re: [PATCH for-5.0 1/4] xics: Link ICS_PROP_XICS property to ICSState::xics pointer

[Cédric Le Goater]

On 18/11/2019 00:20, Greg Kurz wrote:
> The ICS object has both a pointer and an ICS_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.
> 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     |   13 +++----------
>  hw/ppc/pnv_psi.c   |    3 +--
>  hw/ppc/spapr_irq.c |    9 ++-------
>  3 files changed, 6 insertions(+), 19 deletions(-)
> 
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index e7ac9ba618fa..f7a454808992 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -609,17 +609,8 @@ static void ics_reset_handler(void *dev)
>  static void ics_realize(DeviceState *dev, Error **errp)
>  {
>      ICSState *ics = ICS(dev);
> -    Error *local_err = NULL;
> -    Object *obj;
>  
> -    obj = object_property_get_link(OBJECT(dev), ICS_PROP_XICS, &local_err);
> -    if (!obj) {
> -        error_propagate_prepend(errp, local_err,
> -                                "required link '" ICS_PROP_XICS
> -                                "' not found: ");
> -        return;
> -    }
> -    ics->xics = XICS_FABRIC(obj);
> +    assert(ics->xics);
>  
>      if (!ics->nr_irqs) {
>          error_setg(errp, "Number of interrupts needs to be greater 0");
> @@ -699,6 +690,8 @@ static const VMStateDescription vmstate_ics = {
>  
>  static Property ics_properties[] = {
>      DEFINE_PROP_UINT32("nr-irqs", ICSState, nr_irqs, 0),
> +    DEFINE_PROP_LINK(ICS_PROP_XICS, ICSState, xics, TYPE_XICS_FABRIC,
> +                     XICSFabric *),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> diff --git a/hw/ppc/pnv_psi.c b/hw/ppc/pnv_psi.c
> index a360515a86f8..7e725aaf2bd5 100644
> --- a/hw/ppc/pnv_psi.c
> +++ b/hw/ppc/pnv_psi.c
> @@ -497,8 +497,7 @@ static void pnv_psi_power8_realize(DeviceState *dev, 
> Error **errp)
>      }
>  
>      /* Create PSI interrupt control source */
> -    object_property_add_const_link(OBJECT(ics), ICS_PROP_XICS, obj,
> -                                   &error_abort);
> +    object_property_set_link(OBJECT(ics), obj, ICS_PROP_XICS, &error_abort);
>      object_property_set_int(OBJECT(ics), PSI_NUM_INTERRUPTS, "nr-irqs", 
> &err);
>      if (err) {
>          error_propagate(errp, err);
> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> index 168044be853a..487c8ceb35a3 100644
> --- a/hw/ppc/spapr_irq.c
> +++ b/hw/ppc/spapr_irq.c
> @@ -319,13 +319,8 @@ void spapr_irq_init(SpaprMachineState *spapr, Error 
> **errp)
>              return;
>          }
>  
> -        object_property_add_const_link(obj, ICS_PROP_XICS, OBJECT(spapr),
> -                                       &local_err);
> -        if (local_err) {
> -            error_propagate(errp, local_err);
> -            return;
> -        }
> -
> +        object_property_set_link(obj, OBJECT(spapr), ICS_PROP_XICS,
> +                                 &error_abort);
>          object_property_set_int(obj, smc->nr_xirqs, "nr-irqs", &local_err);
>          if (local_err) {
>              error_propagate(errp, local_err);
>