[PATCH v2 for-5.0 0/8] ppc: Consolidate QOM links and pointers to the same object

[PATCH v2 for-5.0 0/8] ppc: Consolidate QOM links and pointers to the same object

[Greg Kurz]

There's a recurring pattern in the code where a const link is added to a
newly instanciated object and the link is then used in the object's realize
function to keep a pointer to the QOM entity which the link points to.

void create_obj_b(Object *obj_a)
{
    Object *obj_b;

    obj_b = object_new(TYPE_B);
    object_property_add_const_link(obj_b, "link-to-a", obj_a, &error_abort);
    object_property_set_bool(obj_b, true, "realized", &error_abort);
}

void object_b_realize(DeviceState *dev, Error **errp)
{
    Object *obj_a;

    obj_a = object_property_get_link(OBJECT(dev), "link-to-a", errp);
    if (!obj_a) {
        return;
    }

    obj_b->obj_a = A(obj_a); // If obj_b->obj_a is changed, the link property
                             // still points to the original obj_a that was
                             // passed to object_property_add_const_link()
}

Confusing bugs could arise if the pointer and the link go out of sync for
some reason. This can be avoided if the property is defined to directly use
the pointer with the DEFINE_PROP_LINK() macro.

This series just does that for all occurences of the fragile pattern in
the XIVE and PNV code.

Changes in v2:
- use DEFINE_PROP_LINK() instead of object_property_add_link()
- dropped public -> private changes in type definitions

--
Greg

---

Greg Kurz (8):
      xive: Link "cpu" property to XiveTCTX::cs pointer
      xive: Link "xive" property to XiveSource::xive pointer
      xive: Link "xive" property to XiveEndSource::xrtr pointer
      ppc/pnv: Link "psi" property to PnvLpc::psi pointer
      ppc/pnv: Link "psi" property to PnvOCC::psi pointer
      ppc/pnv: Link "chip" property to PnvHomer::chip pointer
      ppc/pnv: Link "chip" property to PnvCore::chip pointer
      ppc/pnv: Link "chip" property to PnvXive::chip pointer


 hw/intc/pnv_xive.c   |   21 +++++++--------------
 hw/intc/spapr_xive.c |    8 ++++----
 hw/intc/xive.c       |   48 +++++++++++++++---------------------------------
 hw/ppc/pnv.c         |   32 ++++++++++++++++----------------
 hw/ppc/pnv_core.c    |   10 ++--------
 hw/ppc/pnv_homer.c   |   20 ++++++++++----------
 hw/ppc/pnv_lpc.c     |   19 ++++++++-----------
 hw/ppc/pnv_occ.c     |   20 +++++++++-----------
 hw/ppc/pnv_psi.c     |    3 +--
 9 files changed, 72 insertions(+), 109 deletions(-)

[PATCH v2 for-5.0 1/8] xive: Link "cpu" property to XiveTCTX::cs pointer

[Greg Kurz]

The TCTX object has both a pointer and a "cpu" property pointing to the
vCPU object. Confusing bugs could arise if these ever go out of sync.

Change the property definition so that it explicitely sets the pointer.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/intc/xive.c |   22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/hw/intc/xive.c b/hw/intc/xive.c
index 75dce82fb205..9376e84aff75 100644
--- a/hw/intc/xive.c
+++ b/hw/intc/xive.c
@@ -580,19 +580,11 @@ static void xive_tctx_realize(DeviceState *dev, Error **errp)
     XiveTCTX *tctx = XIVE_TCTX(dev);
     PowerPCCPU *cpu;
     CPUPPCState *env;
-    Object *obj;
     Error *local_err = NULL;
 
-    obj = object_property_get_link(OBJECT(dev), "cpu", &local_err);
-    if (!obj) {
-        error_propagate(errp, local_err);
-        error_prepend(errp, "required link 'cpu' not found: ");
-        return;
-    }
-
-    cpu = POWERPC_CPU(obj);
-    tctx->cs = CPU(obj);
+    assert(tctx->cs);
 
+    cpu = POWERPC_CPU(tctx->cs);
     env = &cpu->env;
     switch (PPC_INPUT(env)) {
     case PPC_FLAGS_INPUT_POWER9:
@@ -662,6 +654,11 @@ static const VMStateDescription vmstate_xive_tctx = {
     },
 };
 
+static Property xive_tctx_properties[] = {
+    DEFINE_PROP_LINK("cpu", XiveTCTX, cs, TYPE_CPU, CPUState *),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 static void xive_tctx_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -669,6 +666,7 @@ static void xive_tctx_class_init(ObjectClass *klass, void *data)
     dc->desc = "XIVE Interrupt Thread Context";
     dc->realize = xive_tctx_realize;
     dc->vmsd = &vmstate_xive_tctx;
+    dc->props = xive_tctx_properties;
     /*
      * Reason: part of XIVE interrupt controller, needs to be wired up
      * by xive_tctx_create().
@@ -691,8 +689,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_link(obj, cpu, "cpu", &error_abort);
     object_property_set_bool(obj, true, "realized", &local_err);
     if (local_err) {
         goto error;
@@ -710,7 +707,6 @@ void xive_tctx_destroy(XiveTCTX *tctx)
 {
     Object *obj = OBJECT(tctx);
 
-    object_unref(object_property_get_link(obj, "cpu", &error_abort));
     object_unparent(obj);
 }
 

[PATCH v2 for-5.0 2/8] xive: Link "xive" property to XiveSource::xive pointer

[Greg Kurz]

The source object has both a pointer and a "xive" property pointing to the
notifier object. 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/pnv_xive.c   |    4 ++--
 hw/intc/spapr_xive.c |    4 ++--
 hw/intc/xive.c       |   13 +++----------
 hw/ppc/pnv_psi.c     |    3 +--
 4 files changed, 8 insertions(+), 16 deletions(-)

diff --git a/hw/intc/pnv_xive.c b/hw/intc/pnv_xive.c
index 348f2fdd263d..9e23dc705dc3 100644
--- a/hw/intc/pnv_xive.c
+++ b/hw/intc/pnv_xive.c
@@ -1695,8 +1695,8 @@ static void pnv_xive_realize(DeviceState *dev, Error **errp)
      */
     object_property_set_int(OBJECT(xsrc), PNV_XIVE_NR_IRQS, "nr-irqs",
                             &error_fatal);
-    object_property_add_const_link(OBJECT(xsrc), "xive", OBJECT(xive),
-                                   &error_fatal);
+    object_property_set_link(OBJECT(xsrc), OBJECT(xive), "xive",
+                             &error_abort);
     object_property_set_bool(OBJECT(xsrc), true, "realized", &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
index 9cb8d38a3bab..10890aeeeb5b 100644
--- a/hw/intc/spapr_xive.c
+++ b/hw/intc/spapr_xive.c
@@ -276,8 +276,8 @@ static void spapr_xive_realize(DeviceState *dev, Error **errp)
      */
     object_property_set_int(OBJECT(xsrc), xive->nr_irqs, "nr-irqs",
                             &error_fatal);
-    object_property_add_const_link(OBJECT(xsrc), "xive", OBJECT(xive),
-                                   &error_fatal);
+    object_property_set_link(OBJECT(xsrc), OBJECT(xive), "xive",
+                             &error_abort);
     object_property_set_bool(OBJECT(xsrc), true, "realized", &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
diff --git a/hw/intc/xive.c b/hw/intc/xive.c
index 9376e84aff75..2eac15efa675 100644
--- a/hw/intc/xive.c
+++ b/hw/intc/xive.c
@@ -1060,17 +1060,8 @@ static void xive_source_reset(void *dev)
 static void xive_source_realize(DeviceState *dev, Error **errp)
 {
     XiveSource *xsrc = XIVE_SOURCE(dev);
-    Object *obj;
-    Error *local_err = NULL;
-
-    obj = object_property_get_link(OBJECT(dev), "xive", &local_err);
-    if (!obj) {
-        error_propagate(errp, local_err);
-        error_prepend(errp, "required link 'xive' not found: ");
-        return;
-    }
 
-    xsrc->xive = XIVE_NOTIFIER(obj);
+    assert(xsrc->xive);
 
     if (!xsrc->nr_irqs) {
         error_setg(errp, "Number of interrupt needs to be greater than 0");
@@ -1116,6 +1107,8 @@ static Property xive_source_properties[] = {
     DEFINE_PROP_UINT64("flags", XiveSource, esb_flags, 0),
     DEFINE_PROP_UINT32("nr-irqs", XiveSource, nr_irqs, 0),
     DEFINE_PROP_UINT32("shift", XiveSource, esb_shift, XIVE_ESB_64K_2PAGE),
+    DEFINE_PROP_LINK("xive", XiveSource, xive, TYPE_XIVE_NOTIFIER,
+                     XiveNotifier *),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/hw/ppc/pnv_psi.c b/hw/ppc/pnv_psi.c
index 68d0dfacfe2b..a360515a86f8 100644
--- a/hw/ppc/pnv_psi.c
+++ b/hw/ppc/pnv_psi.c
@@ -851,8 +851,7 @@ static void pnv_psi_power9_realize(DeviceState *dev, Error **errp)
                             &error_fatal);
     object_property_set_int(OBJECT(xsrc), PSIHB9_NUM_IRQS, "nr-irqs",
                             &error_fatal);
-    object_property_add_const_link(OBJECT(xsrc), "xive", OBJECT(psi),
-                                   &error_fatal);
+    object_property_set_link(OBJECT(xsrc), OBJECT(psi), "xive", &error_abort);
     object_property_set_bool(OBJECT(xsrc), true, "realized", &local_err);
     if (local_err) {
         error_propagate(errp, local_err);

[PATCH v2 for-5.0 3/8] xive: Link "xive" property to XiveEndSource::xrtr pointer

[Greg Kurz]

The END source object has both a pointer and a "xive" property pointing to
the router object. 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/pnv_xive.c   |    4 ++--
 hw/intc/spapr_xive.c |    4 ++--
 hw/intc/xive.c       |   13 +++----------
 3 files changed, 7 insertions(+), 14 deletions(-)

diff --git a/hw/intc/pnv_xive.c b/hw/intc/pnv_xive.c
index 9e23dc705dc3..6aa7aeed6f83 100644
--- a/hw/intc/pnv_xive.c
+++ b/hw/intc/pnv_xive.c
@@ -1705,8 +1705,8 @@ static void pnv_xive_realize(DeviceState *dev, Error **errp)
 
     object_property_set_int(OBJECT(end_xsrc), PNV_XIVE_NR_ENDS, "nr-ends",
                             &error_fatal);
-    object_property_add_const_link(OBJECT(end_xsrc), "xive", OBJECT(xive),
-                                   &error_fatal);
+    object_property_set_link(OBJECT(end_xsrc), OBJECT(xive), "xive",
+                             &error_abort);
     object_property_set_bool(OBJECT(end_xsrc), true, "realized", &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
index 10890aeeeb5b..729246e906c9 100644
--- a/hw/intc/spapr_xive.c
+++ b/hw/intc/spapr_xive.c
@@ -290,8 +290,8 @@ static void spapr_xive_realize(DeviceState *dev, Error **errp)
      */
     object_property_set_int(OBJECT(end_xsrc), xive->nr_irqs, "nr-ends",
                             &error_fatal);
-    object_property_add_const_link(OBJECT(end_xsrc), "xive", OBJECT(xive),
-                                   &error_fatal);
+    object_property_set_link(OBJECT(end_xsrc), OBJECT(xive), "xive",
+                             &error_abort);
     object_property_set_bool(OBJECT(end_xsrc), true, "realized", &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
diff --git a/hw/intc/xive.c b/hw/intc/xive.c
index 2eac15efa675..3d472e29c858 100644
--- a/hw/intc/xive.c
+++ b/hw/intc/xive.c
@@ -1814,17 +1814,8 @@ static const MemoryRegionOps xive_end_source_ops = {
 static void xive_end_source_realize(DeviceState *dev, Error **errp)
 {
     XiveENDSource *xsrc = XIVE_END_SOURCE(dev);
-    Object *obj;
-    Error *local_err = NULL;
-
-    obj = object_property_get_link(OBJECT(dev), "xive", &local_err);
-    if (!obj) {
-        error_propagate(errp, local_err);
-        error_prepend(errp, "required link 'xive' not found: ");
-        return;
-    }
 
-    xsrc->xrtr = XIVE_ROUTER(obj);
+    assert(xsrc->xrtr);
 
     if (!xsrc->nr_ends) {
         error_setg(errp, "Number of interrupt needs to be greater than 0");
@@ -1850,6 +1841,8 @@ static Property xive_end_source_properties[] = {
     DEFINE_PROP_UINT8("block-id", XiveENDSource, block_id, 0),
     DEFINE_PROP_UINT32("nr-ends", XiveENDSource, nr_ends, 0),
     DEFINE_PROP_UINT32("shift", XiveENDSource, esb_shift, XIVE_ESB_64K),
+    DEFINE_PROP_LINK("xive", XiveENDSource, xrtr, TYPE_XIVE_ROUTER,
+                     XiveRouter *),
     DEFINE_PROP_END_OF_LIST(),
 };
 

[PATCH v2 for-5.0 4/8] ppc/pnv: Link "psi" property to PnvLpc::psi pointer

[Greg Kurz]

The LPC object has both a pointer and a "psi" property pointing to the
PSI object. Confusing bugs could arise if these ever go out of sync.

Change the property definition so that it explicitely sets the pointer.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/pnv.c     |    8 ++++----
 hw/ppc/pnv_lpc.c |   19 ++++++++-----------
 2 files changed, 12 insertions(+), 15 deletions(-)

diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index d0c1d4227784..286901746f50 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -898,8 +898,6 @@ static void pnv_chip_power8_instance_init(Object *obj)
 
     object_initialize_child(obj, "lpc",  &chip8->lpc, sizeof(chip8->lpc),
                             TYPE_PNV8_LPC, &error_abort, NULL);
-    object_property_add_const_link(OBJECT(&chip8->lpc), "psi",
-                                   OBJECT(&chip8->psi), &error_abort);
 
     object_initialize_child(obj, "occ",  &chip8->occ, sizeof(chip8->occ),
                             TYPE_PNV8_OCC, &error_abort, NULL);
@@ -978,6 +976,8 @@ static void pnv_chip_power8_realize(DeviceState *dev, Error **errp)
                             &PNV_PSI(psi8)->xscom_regs);
 
     /* Create LPC controller */
+    object_property_set_link(OBJECT(&chip8->lpc), OBJECT(&chip8->psi), "psi",
+                             &error_abort);
     object_property_set_bool(OBJECT(&chip8->lpc), true, "realized",
                              &error_fatal);
     pnv_xscom_add_subregion(chip, PNV_XSCOM_LPC_BASE, &chip8->lpc.xscom_regs);
@@ -1098,8 +1098,6 @@ static void pnv_chip_power9_instance_init(Object *obj)
 
     object_initialize_child(obj, "lpc",  &chip9->lpc, sizeof(chip9->lpc),
                             TYPE_PNV9_LPC, &error_abort, NULL);
-    object_property_add_const_link(OBJECT(&chip9->lpc), "psi",
-                                   OBJECT(&chip9->psi), &error_abort);
 
     object_initialize_child(obj, "occ",  &chip9->occ, sizeof(chip9->occ),
                             TYPE_PNV9_OCC, &error_abort, NULL);
@@ -1198,6 +1196,8 @@ static void pnv_chip_power9_realize(DeviceState *dev, Error **errp)
                             &PNV_PSI(psi9)->xscom_regs);
 
     /* LPC */
+    object_property_set_link(OBJECT(&chip9->lpc), OBJECT(&chip9->psi), "psi",
+                             &error_abort);
     object_property_set_bool(OBJECT(&chip9->lpc), true, "realized", &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
diff --git a/hw/ppc/pnv_lpc.c b/hw/ppc/pnv_lpc.c
index 9466d4a1be3b..fb9f93032020 100644
--- a/hw/ppc/pnv_lpc.c
+++ b/hw/ppc/pnv_lpc.c
@@ -24,7 +24,7 @@
 #include "qemu/module.h"
 #include "hw/irq.h"
 #include "hw/isa/isa.h"
-
+#include "hw/qdev-properties.h"
 #include "hw/ppc/pnv.h"
 #include "hw/ppc/pnv_lpc.h"
 #include "hw/ppc/pnv_xscom.h"
@@ -682,17 +682,8 @@ static const TypeInfo pnv_lpc_power9_info = {
 static void pnv_lpc_realize(DeviceState *dev, Error **errp)
 {
     PnvLpcController *lpc = PNV_LPC(dev);
-    Object *obj;
-    Error *local_err = NULL;
 
-    obj = object_property_get_link(OBJECT(dev), "psi", &local_err);
-    if (!obj) {
-        error_propagate(errp, local_err);
-        error_prepend(errp, "required link 'psi' not found: ");
-        return;
-    }
-    /* The LPC controller needs PSI to generate interrupts  */
-    lpc->psi = PNV_PSI(obj);
+    assert(lpc->psi);
 
     /* Reg inits */
     lpc->lpc_hc_fw_rd_acc_size = LPC_HC_FW_RD_4B;
@@ -734,12 +725,18 @@ static void pnv_lpc_realize(DeviceState *dev, Error **errp)
                                 &lpc->lpc_hc_regs);
 }
 
+static Property pnv_lpc_properties[] = {
+    DEFINE_PROP_LINK("psi", PnvLpcController, psi, TYPE_PNV_PSI, PnvPsi *),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 static void pnv_lpc_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
 
     dc->realize = pnv_lpc_realize;
     dc->desc = "PowerNV LPC Controller";
+    dc->props = pnv_lpc_properties;
 }
 
 static const TypeInfo pnv_lpc_info = {

[PATCH v2 for-5.0 5/8] ppc/pnv: Link "psi" property to PnvOCC::psi pointer

[Greg Kurz]

The OCC object has both a pointer and a "psi" property pointing to the
PSI object. Confusing bugs could arise if these ever go out of sync.

Change the property definition so that it explicitely sets the pointer.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/pnv.c     |    8 ++++----
 hw/ppc/pnv_occ.c |   20 +++++++++-----------
 2 files changed, 13 insertions(+), 15 deletions(-)

diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 286901746f50..96c5a23cd1eb 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -901,8 +901,6 @@ static void pnv_chip_power8_instance_init(Object *obj)
 
     object_initialize_child(obj, "occ",  &chip8->occ, sizeof(chip8->occ),
                             TYPE_PNV8_OCC, &error_abort, NULL);
-    object_property_add_const_link(OBJECT(&chip8->occ), "psi",
-                                   OBJECT(&chip8->psi), &error_abort);
 
     object_initialize_child(obj, "homer",  &chip8->homer, sizeof(chip8->homer),
                             TYPE_PNV8_HOMER, &error_abort, NULL);
@@ -997,6 +995,8 @@ static void pnv_chip_power8_realize(DeviceState *dev, Error **errp)
     }
 
     /* Create the simplified OCC model */
+    object_property_set_link(OBJECT(&chip8->occ), OBJECT(&chip8->psi), "psi",
+                             &error_abort);
     object_property_set_bool(OBJECT(&chip8->occ), true, "realized", &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
@@ -1101,8 +1101,6 @@ static void pnv_chip_power9_instance_init(Object *obj)
 
     object_initialize_child(obj, "occ",  &chip9->occ, sizeof(chip9->occ),
                             TYPE_PNV9_OCC, &error_abort, NULL);
-    object_property_add_const_link(OBJECT(&chip9->occ), "psi",
-                                   OBJECT(&chip9->psi), &error_abort);
 
     object_initialize_child(obj, "homer",  &chip9->homer, sizeof(chip9->homer),
                             TYPE_PNV9_HOMER, &error_abort, NULL);
@@ -1210,6 +1208,8 @@ static void pnv_chip_power9_realize(DeviceState *dev, Error **errp)
                                             (uint64_t) PNV9_LPCM_BASE(chip));
 
     /* Create the simplified OCC model */
+    object_property_set_link(OBJECT(&chip9->occ), OBJECT(&chip9->psi), "psi",
+                             &error_abort);
     object_property_set_bool(OBJECT(&chip9->occ), true, "realized", &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
diff --git a/hw/ppc/pnv_occ.c b/hw/ppc/pnv_occ.c
index 785653bb6710..765c0a6ce595 100644
--- a/hw/ppc/pnv_occ.c
+++ b/hw/ppc/pnv_occ.c
@@ -21,7 +21,7 @@
 #include "qapi/error.h"
 #include "qemu/log.h"
 #include "qemu/module.h"
-
+#include "hw/qdev-properties.h"
 #include "hw/ppc/pnv.h"
 #include "hw/ppc/pnv_xscom.h"
 #include "hw/ppc/pnv_occ.h"
@@ -257,18 +257,10 @@ static void pnv_occ_realize(DeviceState *dev, Error **errp)
 {
     PnvOCC *occ = PNV_OCC(dev);
     PnvOCCClass *poc = PNV_OCC_GET_CLASS(occ);
-    Object *obj;
-    Error *local_err = NULL;
 
-    occ->occmisc = 0;
+    assert(occ->psi);
 
-    obj = object_property_get_link(OBJECT(dev), "psi", &local_err);
-    if (!obj) {
-        error_propagate(errp, local_err);
-        error_prepend(errp, "required link 'psi' not found: ");
-        return;
-    }
-    occ->psi = PNV_PSI(obj);
+    occ->occmisc = 0;
 
     /* XScom region for OCC registers */
     pnv_xscom_region_init(&occ->xscom_regs, OBJECT(dev), poc->xscom_ops,
@@ -279,12 +271,18 @@ static void pnv_occ_realize(DeviceState *dev, Error **errp)
                           occ, "occ-common-area", poc->sram_size);
 }
 
+static Property pnv_occ_properties[] = {
+    DEFINE_PROP_LINK("psi", PnvOCC, psi, TYPE_PNV_PSI, PnvPsi *),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 static void pnv_occ_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
 
     dc->realize = pnv_occ_realize;
     dc->desc = "PowerNV OCC Controller";
+    dc->props = pnv_occ_properties;
 }
 
 static const TypeInfo pnv_occ_type_info = {

[PATCH v2 for-5.0 6/8] ppc/pnv: Link "chip" property to PnvHomer::chip pointer

[Greg Kurz]

The homer object has both a pointer and a "chip" property pointing to the
chip object. Confusing bugs could arise if these ever go out of sync.

Change the property definition so that it explicitely sets the pointer.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/pnv.c       |    8 ++++----
 hw/ppc/pnv_homer.c |   20 ++++++++++----------
 2 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 96c5a23cd1eb..232c817452c0 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -904,8 +904,6 @@ static void pnv_chip_power8_instance_init(Object *obj)
 
     object_initialize_child(obj, "homer",  &chip8->homer, sizeof(chip8->homer),
                             TYPE_PNV8_HOMER, &error_abort, NULL);
-    object_property_add_const_link(OBJECT(&chip8->homer), "chip", obj,
-                                   &error_abort);
 }
 
 static void pnv_chip_icp_realize(Pnv8Chip *chip8, Error **errp)
@@ -1009,6 +1007,8 @@ static void pnv_chip_power8_realize(DeviceState *dev, Error **errp)
                                 &chip8->occ.sram_regs);
 
     /* HOMER */
+    object_property_set_link(OBJECT(&chip8->homer), OBJECT(chip), "chip",
+                             &error_abort);
     object_property_set_bool(OBJECT(&chip8->homer), true, "realized",
                              &local_err);
     if (local_err) {
@@ -1104,8 +1104,6 @@ static void pnv_chip_power9_instance_init(Object *obj)
 
     object_initialize_child(obj, "homer",  &chip9->homer, sizeof(chip9->homer),
                             TYPE_PNV9_HOMER, &error_abort, NULL);
-    object_property_add_const_link(OBJECT(&chip9->homer), "chip", obj,
-                                   &error_abort);
 }
 
 static void pnv_chip_quad_realize(Pnv9Chip *chip9, Error **errp)
@@ -1222,6 +1220,8 @@ static void pnv_chip_power9_realize(DeviceState *dev, Error **errp)
                                 &chip9->occ.sram_regs);
 
     /* HOMER */
+    object_property_set_link(OBJECT(&chip9->homer), OBJECT(chip), "chip",
+                             &error_abort);
     object_property_set_bool(OBJECT(&chip9->homer), true, "realized",
                              &local_err);
     if (local_err) {
diff --git a/hw/ppc/pnv_homer.c b/hw/ppc/pnv_homer.c
index cc881a3b3289..994a378108da 100644
--- a/hw/ppc/pnv_homer.c
+++ b/hw/ppc/pnv_homer.c
@@ -22,6 +22,7 @@
 #include "exec/memory.h"
 #include "sysemu/cpus.h"
 #include "hw/qdev-core.h"
+#include "hw/qdev-properties.h"
 #include "hw/ppc/pnv.h"
 #include "hw/ppc/pnv_homer.h"
 
@@ -229,28 +230,27 @@ static void pnv_homer_realize(DeviceState *dev, Error **errp)
 {
     PnvHomer *homer = PNV_HOMER(dev);
     PnvHomerClass *hmrc = PNV_HOMER_GET_CLASS(homer);
-    Object *obj;
-    Error *local_err = NULL;
-
-    obj = object_property_get_link(OBJECT(dev), "chip", &local_err);
-    if (!obj) {
-        error_propagate(errp, local_err);
-        error_prepend(errp, "required link 'chip' not found: ");
-        return;
-    }
-    homer->chip = PNV_CHIP(obj);
+
+    assert(homer->chip);
+
     /* homer region */
     memory_region_init_io(&homer->regs, OBJECT(dev),
                           hmrc->homer_ops, homer, "homer-main-memory",
                           hmrc->homer_size);
 }
 
+static Property pnv_homer_properties[] = {
+    DEFINE_PROP_LINK("chip", PnvHomer, chip, TYPE_PNV_CHIP, PnvChip *),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 static void pnv_homer_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
 
     dc->realize = pnv_homer_realize;
     dc->desc = "PowerNV HOMER Memory";
+    dc->props = pnv_homer_properties;
 }
 
 static const TypeInfo pnv_homer_type_info = {

[PATCH v2 for-5.0 7/8] ppc/pnv: Link "chip" property to PnvCore::chip pointer

[Greg Kurz]

The core object has both a pointer and a "chip" property pointing to the
chip object. Confusing bugs could arise if these ever go out of sync.

Change the property definition so that it explicitely sets the pointer.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/pnv.c      |    4 ++--
 hw/ppc/pnv_core.c |   10 ++--------
 2 files changed, 4 insertions(+), 10 deletions(-)

diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 232c817452c0..8851875bcfd7 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -1326,8 +1326,8 @@ static void pnv_chip_core_realize(PnvChip *chip, Error **errp)
         object_property_set_int(OBJECT(pnv_core),
                                 pcc->core_pir(chip, core_hwid),
                                 "pir", &error_fatal);
-        object_property_add_const_link(OBJECT(pnv_core), "chip",
-                                       OBJECT(chip), &error_fatal);
+        object_property_set_link(OBJECT(pnv_core), OBJECT(chip), "chip",
+                                 &error_abort);
         object_property_set_bool(OBJECT(pnv_core), true, "realized",
                                  &error_fatal);
 
diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
index 61b3d3ce2250..5ab75bde6cc5 100644
--- a/hw/ppc/pnv_core.c
+++ b/hw/ppc/pnv_core.c
@@ -217,15 +217,8 @@ static void pnv_core_realize(DeviceState *dev, Error **errp)
     void *obj;
     int i, j;
     char name[32];
-    Object *chip;
 
-    chip = object_property_get_link(OBJECT(dev), "chip", &local_err);
-    if (!chip) {
-        error_propagate_prepend(errp, local_err,
-                                "required link 'chip' not found: ");
-        return;
-    }
-    pc->chip = PNV_CHIP(chip);
+    assert(pc->chip);
 
     pc->threads = g_new(PowerPCCPU *, cc->nr_threads);
     for (i = 0; i < cc->nr_threads; i++) {
@@ -297,6 +290,7 @@ static void pnv_core_unrealize(DeviceState *dev, Error **errp)
 
 static Property pnv_core_properties[] = {
     DEFINE_PROP_UINT32("pir", PnvCore, pir, 0),
+    DEFINE_PROP_LINK("chip", PnvCore, chip, TYPE_PNV_CHIP, PnvChip *),
     DEFINE_PROP_END_OF_LIST(),
 };
 

[PATCH v2 for-5.0 8/8] ppc/pnv: Link "chip" property to PnvXive::chip pointer

[Greg Kurz]

The XIVE object has both a pointer and a "chip" property pointing to the
chip object. Confusing bugs could arise if these ever go out of sync.

Change the property definition so that it explicitely sets the pointer.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/intc/pnv_xive.c |   13 +++----------
 hw/ppc/pnv.c       |    4 ++--
 2 files changed, 5 insertions(+), 12 deletions(-)

diff --git a/hw/intc/pnv_xive.c b/hw/intc/pnv_xive.c
index 6aa7aeed6f83..4e56c2e4689c 100644
--- a/hw/intc/pnv_xive.c
+++ b/hw/intc/pnv_xive.c
@@ -1675,17 +1675,8 @@ static void pnv_xive_realize(DeviceState *dev, Error **errp)
     XiveSource *xsrc = &xive->ipi_source;
     XiveENDSource *end_xsrc = &xive->end_source;
     Error *local_err = NULL;
-    Object *obj;
 
-    obj = object_property_get_link(OBJECT(dev), "chip", &local_err);
-    if (!obj) {
-        error_propagate(errp, local_err);
-        error_prepend(errp, "required link 'chip' not found: ");
-        return;
-    }
-
-    /* The PnvChip id identifies the XIVE interrupt controller. */
-    xive->chip = PNV_CHIP(obj);
+    assert(xive->chip);
 
     /*
      * The XiveSource and XiveENDSource objects are realized with the
@@ -1800,6 +1791,8 @@ static Property pnv_xive_properties[] = {
     DEFINE_PROP_UINT64("vc-bar", PnvXive, vc_base, 0),
     DEFINE_PROP_UINT64("pc-bar", PnvXive, pc_base, 0),
     DEFINE_PROP_UINT64("tm-bar", PnvXive, tm_base, 0),
+    /* The PnvChip id identifies the XIVE interrupt controller. */
+    DEFINE_PROP_LINK("chip", PnvXive, chip, TYPE_PNV_CHIP, PnvChip *),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 8851875bcfd7..d7130c3304f0 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -1088,8 +1088,6 @@ static void pnv_chip_power9_instance_init(Object *obj)
 
     object_initialize_child(obj, "xive", &chip9->xive, sizeof(chip9->xive),
                             TYPE_PNV_XIVE, &error_abort, NULL);
-    object_property_add_const_link(OBJECT(&chip9->xive), "chip", obj,
-                                   &error_abort);
 
     object_initialize_child(obj, "psi",  &chip9->psi, sizeof(chip9->psi),
                             TYPE_PNV9_PSI, &error_abort, NULL);
@@ -1171,6 +1169,8 @@ static void pnv_chip_power9_realize(DeviceState *dev, Error **errp)
                             "pc-bar", &error_fatal);
     object_property_set_int(OBJECT(&chip9->xive), PNV9_XIVE_TM_BASE(chip),
                             "tm-bar", &error_fatal);
+    object_property_set_link(OBJECT(&chip9->xive), OBJECT(chip), "chip",
+                             &error_abort);
     object_property_set_bool(OBJECT(&chip9->xive), true, "realized",
                              &local_err);
     if (local_err) {

Re: [PATCH v2 for-5.0 0/8] ppc: Consolidate QOM links and pointers to the same object

[David Gibson]

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

On Fri, Nov 15, 2019 at 04:55:21PM +0100, Greg Kurz wrote:
> There's a recurring pattern in the code where a const link is added to a
> newly instanciated object and the link is then used in the object's realize
> function to keep a pointer to the QOM entity which the link points to.
> 
> void create_obj_b(Object *obj_a)
> {
>     Object *obj_b;
> 
>     obj_b = object_new(TYPE_B);
>     object_property_add_const_link(obj_b, "link-to-a", obj_a, &error_abort);
>     object_property_set_bool(obj_b, true, "realized", &error_abort);
> }
> 
> void object_b_realize(DeviceState *dev, Error **errp)
> {
>     Object *obj_a;
> 
>     obj_a = object_property_get_link(OBJECT(dev), "link-to-a", errp);
>     if (!obj_a) {
>         return;
>     }
> 
>     obj_b->obj_a = A(obj_a); // If obj_b->obj_a is changed, the link property
>                              // still points to the original obj_a that was
>                              // passed to object_property_add_const_link()
> }
> 
> Confusing bugs could arise if the pointer and the link go out of sync for
> some reason. This can be avoided if the property is defined to directly use
> the pointer with the DEFINE_PROP_LINK() macro.
> 
> This series just does that for all occurences of the fragile pattern in
> the XIVE and PNV code.
> 
> Changes in v2:
> - use DEFINE_PROP_LINK() instead of object_property_add_link()
> - dropped public -> private changes in type definitions

Applied to ppc-for-5.0.

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

Re: [PATCH v2 for-5.0 0/8] ppc: Consolidate QOM links and pointers to the same object

[Markus Armbruster]

Greg Kurz <groug@kaod.org> writes:

> There's a recurring pattern in the code where a const link is added to a
> newly instanciated object and the link is then used in the object's realize
> function to keep a pointer to the QOM entity which the link points to.
>
> void create_obj_b(Object *obj_a)
> {
>     Object *obj_b;
>
>     obj_b = object_new(TYPE_B);
>     object_property_add_const_link(obj_b, "link-to-a", obj_a, &error_abort);
>     object_property_set_bool(obj_b, true, "realized", &error_abort);
> }
>
> void object_b_realize(DeviceState *dev, Error **errp)
> {
>     Object *obj_a;
>
>     obj_a = object_property_get_link(OBJECT(dev), "link-to-a", errp);
>     if (!obj_a) {
>         return;
>     }
>
>     obj_b->obj_a = A(obj_a); // If obj_b->obj_a is changed, the link property
>                              // still points to the original obj_a that was
>                              // passed to object_property_add_const_link()
> }
>
> Confusing bugs could arise if the pointer and the link go out of sync for
> some reason. This can be avoided if the property is defined to directly use
> the pointer with the DEFINE_PROP_LINK() macro.
>
> This series just does that for all occurences of the fragile pattern in
> the XIVE and PNV code.

Have you looked for the pattern elsewhere?

Re: [PATCH v2 for-5.0 0/8] ppc: Consolidate QOM links and pointers to the same object

[Cédric Le Goater]

On 18/11/2019 10:26, Markus Armbruster wrote:
> Greg Kurz <groug@kaod.org> writes:
> 
>> There's a recurring pattern in the code where a const link is added to a
>> newly instanciated object and the link is then used in the object's realize
>> function to keep a pointer to the QOM entity which the link points to.
>>
>> void create_obj_b(Object *obj_a)
>> {
>>     Object *obj_b;
>>
>>     obj_b = object_new(TYPE_B);
>>     object_property_add_const_link(obj_b, "link-to-a", obj_a, &error_abort);
>>     object_property_set_bool(obj_b, true, "realized", &error_abort);
>> }
>>
>> void object_b_realize(DeviceState *dev, Error **errp)
>> {
>>     Object *obj_a;
>>
>>     obj_a = object_property_get_link(OBJECT(dev), "link-to-a", errp);
>>     if (!obj_a) {
>>         return;
>>     }
>>
>>     obj_b->obj_a = A(obj_a); // If obj_b->obj_a is changed, the link property
>>                              // still points to the original obj_a that was
>>                              // passed to object_property_add_const_link()
>> }
>>
>> Confusing bugs could arise if the pointer and the link go out of sync for
>> some reason. This can be avoided if the property is defined to directly use
>> the pointer with the DEFINE_PROP_LINK() macro.
>>
>> This series just does that for all occurences of the fragile pattern in
>> the XIVE and PNV code.
> 
> Have you looked for the pattern elsewhere?

I can take care of the Aspeed machine. I followed the same pattern there.

C.

Re: [PATCH v2 for-5.0 0/8] ppc: Consolidate QOM links and pointers to the same object

[Greg Kurz]

On Mon, 18 Nov 2019 10:26:12 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Greg Kurz <groug@kaod.org> writes:
> 
> > There's a recurring pattern in the code where a const link is added to a
> > newly instanciated object and the link is then used in the object's realize
> > function to keep a pointer to the QOM entity which the link points to.
> >
> > void create_obj_b(Object *obj_a)
> > {
> >     Object *obj_b;
> >
> >     obj_b = object_new(TYPE_B);
> >     object_property_add_const_link(obj_b, "link-to-a", obj_a, &error_abort);
> >     object_property_set_bool(obj_b, true, "realized", &error_abort);
> > }
> >
> > void object_b_realize(DeviceState *dev, Error **errp)
> > {
> >     Object *obj_a;
> >
> >     obj_a = object_property_get_link(OBJECT(dev), "link-to-a", errp);
> >     if (!obj_a) {
> >         return;
> >     }
> >
> >     obj_b->obj_a = A(obj_a); // If obj_b->obj_a is changed, the link property
> >                              // still points to the original obj_a that was
> >                              // passed to object_property_add_const_link()
> > }
> >
> > Confusing bugs could arise if the pointer and the link go out of sync for
> > some reason. This can be avoided if the property is defined to directly use
> > the pointer with the DEFINE_PROP_LINK() macro.
> >
> > This series just does that for all occurences of the fragile pattern in
> > the XIVE and PNV code.
> 
> Have you looked for the pattern elsewhere?
> 

No I was focusing on the XICS/XIVE interrupt controller code for PPC machines
only. I'll have a broader look.