qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PATCH for-5.0 0/4] ppc: Some more QOM cleanup for XICS
@ 2019-11-17 23:20 Greg Kurz
  2019-11-17 23:20 ` [PATCH for-5.0 1/4] xics: Link ICS_PROP_XICS property to ICSState::xics pointer Greg Kurz
                   ` (4 more replies)
  0 siblings, 5 replies; 10+ messages in thread
From: Greg Kurz @ 2019-11-17 23:20 UTC (permalink / raw)
  To: David Gibson; +Cc: qemu-ppc, Cédric Le Goater, qemu-devel

This series consolidates some more QOM links and pointers that point to
the same object. While here also simplify the XICS interrupt controller
init in the machine code.

--
Greg

---

Greg Kurz (4):
      xics: Link ICS_PROP_XICS property to ICSState::xics pointer
      xics: Link ICP_PROP_XICS property to ICPState::xics pointer
      xics: Link ICP_PROP_CPU property to ICPState::cs pointer
      spapr: Abort if XICS interrupt controller cannot be initialized


 hw/intc/xics.c     |   54 +++++++++++++++-------------------------------------
 hw/ppc/pnv_psi.c   |    3 +--
 hw/ppc/spapr_irq.c |   22 ++++-----------------
 3 files changed, 21 insertions(+), 58 deletions(-)



^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH for-5.0 1/4] xics: Link ICS_PROP_XICS property to ICSState::xics pointer
  2019-11-17 23:20 [PATCH for-5.0 0/4] ppc: Some more QOM cleanup for XICS Greg Kurz
@ 2019-11-17 23:20 ` Greg Kurz
  2019-11-18  8:03   ` Cédric Le Goater
  2019-11-17 23:20 ` [PATCH for-5.0 2/4] xics: Link ICP_PROP_XICS property to ICPState::xics pointer Greg Kurz
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 10+ messages in thread
From: Greg Kurz @ 2019-11-17 23:20 UTC (permalink / raw)
  To: David Gibson; +Cc: qemu-ppc, Cédric Le Goater, qemu-devel

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 <groug@kaod.org>
---
 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);



^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH for-5.0 2/4] xics: Link ICP_PROP_XICS property to ICPState::xics pointer
  2019-11-17 23:20 [PATCH for-5.0 0/4] ppc: Some more QOM cleanup for XICS Greg Kurz
  2019-11-17 23:20 ` [PATCH for-5.0 1/4] xics: Link ICS_PROP_XICS property to ICSState::xics pointer Greg Kurz
@ 2019-11-17 23:20 ` Greg Kurz
  2019-11-18  8:03   ` Cédric Le Goater
  2019-11-17 23:20 ` [PATCH for-5.0 3/4] xics: Link ICP_PROP_CPU property to ICPState::cs pointer Greg Kurz
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 10+ messages in thread
From: Greg Kurz @ 2019-11-17 23:20 UTC (permalink / raw)
  To: David Gibson; +Cc: qemu-ppc, Cédric Le Goater, qemu-devel

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.
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 <groug@kaod.org>
---
 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);
 }
 



^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH for-5.0 3/4] xics: Link ICP_PROP_CPU property to ICPState::cs pointer
  2019-11-17 23:20 [PATCH for-5.0 0/4] ppc: Some more QOM cleanup for XICS Greg Kurz
  2019-11-17 23:20 ` [PATCH for-5.0 1/4] xics: Link ICS_PROP_XICS property to ICSState::xics pointer Greg Kurz
  2019-11-17 23:20 ` [PATCH for-5.0 2/4] xics: Link ICP_PROP_XICS property to ICPState::xics pointer Greg Kurz
@ 2019-11-17 23:20 ` Greg Kurz
  2019-11-18  8:04   ` Cédric Le Goater
  2019-11-17 23:20 ` [PATCH for-5.0 4/4] spapr: Abort if XICS interrupt controller cannot be initialized Greg Kurz
  2019-11-19  0:37 ` [PATCH for-5.0 0/4] ppc: Some more QOM cleanup for XICS David Gibson
  4 siblings, 1 reply; 10+ messages in thread
From: Greg Kurz @ 2019-11-17 23:20 UTC (permalink / raw)
  To: David Gibson; +Cc: qemu-ppc, Cédric Le Goater, qemu-devel

The ICP object has both a pointer and an ICP_PROP_CPU property pointing
to the cpu. 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 <groug@kaod.org>
---
 hw/intc/xics.c |   21 ++++-----------------
 1 file changed, 4 insertions(+), 17 deletions(-)

diff --git a/hw/intc/xics.c b/hw/intc/xics.c
index 35dddb88670e..0b259a09c545 100644
--- a/hw/intc/xics.c
+++ b/hw/intc/xics.c
@@ -305,25 +305,13 @@ void icp_reset(ICPState *icp)
 static void icp_realize(DeviceState *dev, Error **errp)
 {
     ICPState *icp = ICP(dev);
-    PowerPCCPU *cpu;
     CPUPPCState *env;
-    Object *obj;
     Error *err = NULL;
 
     assert(icp->xics);
+    assert(icp->cs);
 
-    obj = object_property_get_link(OBJECT(dev), ICP_PROP_CPU, &err);
-    if (!obj) {
-        error_propagate_prepend(errp, err,
-                                "required link '" ICP_PROP_CPU
-                                "' not found: ");
-        return;
-    }
-
-    cpu = POWERPC_CPU(obj);
-    icp->cs = CPU(obj);
-
-    env = &cpu->env;
+    env = &POWERPC_CPU(icp->cs)->env;
     switch (PPC_INPUT(env)) {
     case PPC_FLAGS_INPUT_POWER7:
         icp->output = env->irq_inputs[POWER7_INPUT_INT];
@@ -363,6 +351,7 @@ static void icp_unrealize(DeviceState *dev, Error **errp)
 static Property icp_properties[] = {
     DEFINE_PROP_LINK(ICP_PROP_XICS, ICPState, xics, TYPE_XICS_FABRIC,
                      XICSFabric *),
+    DEFINE_PROP_LINK(ICP_PROP_CPU, ICPState, cs, TYPE_CPU, CPUState *),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -397,8 +386,7 @@ Object *icp_create(Object *cpu, const char *type, XICSFabric *xi, Error **errp)
     object_property_add_child(cpu, type, obj, &error_abort);
     object_unref(obj);
     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_link(obj, cpu, ICP_PROP_CPU, &error_abort);
     object_property_set_bool(obj, true, "realized", &local_err);
     if (local_err) {
         object_unparent(obj);
@@ -413,7 +401,6 @@ void icp_destroy(ICPState *icp)
 {
     Object *obj = OBJECT(icp);
 
-    object_unref(object_property_get_link(obj, ICP_PROP_CPU, &error_abort));
     object_unparent(obj);
 }
 



^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH for-5.0 4/4] spapr: Abort if XICS interrupt controller cannot be initialized
  2019-11-17 23:20 [PATCH for-5.0 0/4] ppc: Some more QOM cleanup for XICS Greg Kurz
                   ` (2 preceding siblings ...)
  2019-11-17 23:20 ` [PATCH for-5.0 3/4] xics: Link ICP_PROP_CPU property to ICPState::cs pointer Greg Kurz
@ 2019-11-17 23:20 ` Greg Kurz
  2019-11-18  8:07   ` Cédric Le Goater
  2019-11-19  0:37 ` [PATCH for-5.0 0/4] ppc: Some more QOM cleanup for XICS David Gibson
  4 siblings, 1 reply; 10+ messages in thread
From: Greg Kurz @ 2019-11-17 23:20 UTC (permalink / raw)
  To: David Gibson; +Cc: qemu-ppc, Cédric Le Goater, qemu-devel

Failing to set any of the ICS property should really never happen:
- object_property_add_child() always succeed unless the child object
  already has a parent, which isn't the case here obviously since the
  ICS has just been created with object_new()
- the ICS has an "nr-irqs" property than can be set as long as the ICS
  isn't realized

In both cases, an error indicates there is a bug in QEMU. Propagating the
error, ie. exiting QEMU since spapr_irq_init() is called with &error_fatal
doesn't make much sense. Abort instead. This is consistent with what is
done with XIVE : both qdev_create() and qdev_prop_set_uint32() abort QEMU
on error.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/spapr_irq.c |   13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
index 487c8ceb35a3..37f65dac9ca8 100644
--- a/hw/ppc/spapr_irq.c
+++ b/hw/ppc/spapr_irq.c
@@ -313,20 +313,11 @@ void spapr_irq_init(SpaprMachineState *spapr, Error **errp)
         Object *obj;
 
         obj = object_new(TYPE_ICS_SPAPR);
-        object_property_add_child(OBJECT(spapr), "ics", obj, &local_err);
-        if (local_err) {
-            error_propagate(errp, local_err);
-            return;
-        }
 
+        object_property_add_child(OBJECT(spapr), "ics", obj, &error_abort);
         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);
-            return;
-        }
-
+        object_property_set_int(obj, smc->nr_xirqs, "nr-irqs", &error_abort);
         object_property_set_bool(obj, true, "realized", &local_err);
         if (local_err) {
             error_propagate(errp, local_err);



^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH for-5.0 1/4] xics: Link ICS_PROP_XICS property to ICSState::xics pointer
  2019-11-17 23:20 ` [PATCH for-5.0 1/4] xics: Link ICS_PROP_XICS property to ICSState::xics pointer Greg Kurz
@ 2019-11-18  8:03   ` Cédric Le Goater
  0 siblings, 0 replies; 10+ messages in thread
From: Cédric Le Goater @ 2019-11-18  8:03 UTC (permalink / raw)
  To: Greg Kurz, David Gibson; +Cc: qemu-ppc, qemu-devel

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 <groug@kaod.org>

Reviewed-by: Cédric Le Goater <clg@kaod.org>


> ---
>  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);
> 



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH for-5.0 2/4] xics: Link ICP_PROP_XICS property to ICPState::xics pointer
  2019-11-17 23:20 ` [PATCH for-5.0 2/4] xics: Link ICP_PROP_XICS property to ICPState::xics pointer Greg Kurz
@ 2019-11-18  8:03   ` Cédric Le Goater
  0 siblings, 0 replies; 10+ messages in thread
From: Cédric Le Goater @ 2019-11-18  8:03 UTC (permalink / raw)
  To: Greg Kurz, David Gibson; +Cc: qemu-ppc, qemu-devel

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 <groug@kaod.org>

Reviewed-by: Cédric Le Goater <clg@kaod.org>

> ---
>  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);
>  }
>  
> 



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH for-5.0 3/4] xics: Link ICP_PROP_CPU property to ICPState::cs pointer
  2019-11-17 23:20 ` [PATCH for-5.0 3/4] xics: Link ICP_PROP_CPU property to ICPState::cs pointer Greg Kurz
@ 2019-11-18  8:04   ` Cédric Le Goater
  0 siblings, 0 replies; 10+ messages in thread
From: Cédric Le Goater @ 2019-11-18  8:04 UTC (permalink / raw)
  To: Greg Kurz, David Gibson; +Cc: qemu-ppc, qemu-devel

On 18/11/2019 00:20, Greg Kurz wrote:
> The ICP object has both a pointer and an ICP_PROP_CPU property pointing
> to the cpu. 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 <groug@kaod.org>



Reviewed-by: Cédric Le Goater <clg@kaod.org>


> ---
>  hw/intc/xics.c |   21 ++++-----------------
>  1 file changed, 4 insertions(+), 17 deletions(-)
> 
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index 35dddb88670e..0b259a09c545 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -305,25 +305,13 @@ void icp_reset(ICPState *icp)
>  static void icp_realize(DeviceState *dev, Error **errp)
>  {
>      ICPState *icp = ICP(dev);
> -    PowerPCCPU *cpu;
>      CPUPPCState *env;
> -    Object *obj;
>      Error *err = NULL;
>  
>      assert(icp->xics);
> +    assert(icp->cs);
>  
> -    obj = object_property_get_link(OBJECT(dev), ICP_PROP_CPU, &err);
> -    if (!obj) {
> -        error_propagate_prepend(errp, err,
> -                                "required link '" ICP_PROP_CPU
> -                                "' not found: ");
> -        return;
> -    }
> -
> -    cpu = POWERPC_CPU(obj);
> -    icp->cs = CPU(obj);
> -
> -    env = &cpu->env;
> +    env = &POWERPC_CPU(icp->cs)->env;
>      switch (PPC_INPUT(env)) {
>      case PPC_FLAGS_INPUT_POWER7:
>          icp->output = env->irq_inputs[POWER7_INPUT_INT];
> @@ -363,6 +351,7 @@ static void icp_unrealize(DeviceState *dev, Error **errp)
>  static Property icp_properties[] = {
>      DEFINE_PROP_LINK(ICP_PROP_XICS, ICPState, xics, TYPE_XICS_FABRIC,
>                       XICSFabric *),
> +    DEFINE_PROP_LINK(ICP_PROP_CPU, ICPState, cs, TYPE_CPU, CPUState *),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> @@ -397,8 +386,7 @@ Object *icp_create(Object *cpu, const char *type, XICSFabric *xi, Error **errp)
>      object_property_add_child(cpu, type, obj, &error_abort);
>      object_unref(obj);
>      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_link(obj, cpu, ICP_PROP_CPU, &error_abort);
>      object_property_set_bool(obj, true, "realized", &local_err);
>      if (local_err) {
>          object_unparent(obj);
> @@ -413,7 +401,6 @@ void icp_destroy(ICPState *icp)
>  {
>      Object *obj = OBJECT(icp);
>  
> -    object_unref(object_property_get_link(obj, ICP_PROP_CPU, &error_abort));
>      object_unparent(obj);
>  }
>  
> 



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH for-5.0 4/4] spapr: Abort if XICS interrupt controller cannot be initialized
  2019-11-17 23:20 ` [PATCH for-5.0 4/4] spapr: Abort if XICS interrupt controller cannot be initialized Greg Kurz
@ 2019-11-18  8:07   ` Cédric Le Goater
  0 siblings, 0 replies; 10+ messages in thread
From: Cédric Le Goater @ 2019-11-18  8:07 UTC (permalink / raw)
  To: Greg Kurz, David Gibson; +Cc: qemu-ppc, qemu-devel

On 18/11/2019 00:20, Greg Kurz wrote:
> Failing to set any of the ICS property should really never happen:
> - object_property_add_child() always succeed unless the child object
>   already has a parent, which isn't the case here obviously since the
>   ICS has just been created with object_new()
> - the ICS has an "nr-irqs" property than can be set as long as the ICS
>   isn't realized
> 
> In both cases, an error indicates there is a bug in QEMU. Propagating the
> error, ie. exiting QEMU since spapr_irq_init() is called with &error_fatal
> doesn't make much sense. Abort instead. This is consistent with what is
> done with XIVE : both qdev_create() and qdev_prop_set_uint32() abort QEMU
> on error.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>


Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C.

> ---
>  hw/ppc/spapr_irq.c |   13 ++-----------
>  1 file changed, 2 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> index 487c8ceb35a3..37f65dac9ca8 100644
> --- a/hw/ppc/spapr_irq.c
> +++ b/hw/ppc/spapr_irq.c
> @@ -313,20 +313,11 @@ void spapr_irq_init(SpaprMachineState *spapr, Error **errp)
>          Object *obj;
>  
>          obj = object_new(TYPE_ICS_SPAPR);
> -        object_property_add_child(OBJECT(spapr), "ics", obj, &local_err);
> -        if (local_err) {
> -            error_propagate(errp, local_err);
> -            return;
> -        }
>  
> +        object_property_add_child(OBJECT(spapr), "ics", obj, &error_abort);
>          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);
> -            return;
> -        }
> -
> +        object_property_set_int(obj, smc->nr_xirqs, "nr-irqs", &error_abort);
>          object_property_set_bool(obj, true, "realized", &local_err);
>          if (local_err) {
>              error_propagate(errp, local_err);
> 



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH for-5.0 0/4] ppc: Some more QOM cleanup for XICS
  2019-11-17 23:20 [PATCH for-5.0 0/4] ppc: Some more QOM cleanup for XICS Greg Kurz
                   ` (3 preceding siblings ...)
  2019-11-17 23:20 ` [PATCH for-5.0 4/4] spapr: Abort if XICS interrupt controller cannot be initialized Greg Kurz
@ 2019-11-19  0:37 ` David Gibson
  4 siblings, 0 replies; 10+ messages in thread
From: David Gibson @ 2019-11-19  0:37 UTC (permalink / raw)
  To: Greg Kurz; +Cc: qemu-ppc, Cédric Le Goater, qemu-devel

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

On Mon, Nov 18, 2019 at 12:20:30AM +0100, Greg Kurz wrote:
> This series consolidates some more QOM links and pointers that point to
> the same object. While here also simplify the XICS interrupt controller
> init in the machine code.

Applied to ppc-for-5.0 (correcting the spelling errors Cédric noted).

-- 
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 --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2019-11-19  0:38 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-17 23:20 [PATCH for-5.0 0/4] ppc: Some more QOM cleanup for XICS Greg Kurz
2019-11-17 23:20 ` [PATCH for-5.0 1/4] xics: Link ICS_PROP_XICS property to ICSState::xics pointer Greg Kurz
2019-11-18  8:03   ` Cédric Le Goater
2019-11-17 23:20 ` [PATCH for-5.0 2/4] xics: Link ICP_PROP_XICS property to ICPState::xics pointer Greg Kurz
2019-11-18  8:03   ` Cédric Le Goater
2019-11-17 23:20 ` [PATCH for-5.0 3/4] xics: Link ICP_PROP_CPU property to ICPState::cs pointer Greg Kurz
2019-11-18  8:04   ` Cédric Le Goater
2019-11-17 23:20 ` [PATCH for-5.0 4/4] spapr: Abort if XICS interrupt controller cannot be initialized Greg Kurz
2019-11-18  8:07   ` Cédric Le Goater
2019-11-19  0:37 ` [PATCH for-5.0 0/4] ppc: Some more QOM cleanup for XICS David Gibson

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).