Re: [PATCH v5 4/7] ppc/pnv: Add a PnvChip pointer to PnvCore

[Greg Kurz]

On Thu, 24 Oct 2019 11:57:05 +0200
Cédric Le Goater <address@hidden> wrote:

> On 24/10/2019 04:38, David Gibson wrote:
> > On Tue, Oct 22, 2019 at 06:38:09PM +0200, Cédric Le Goater wrote:
> >> We will use it to reset the interrupt presenter from the CPU reset
> >> handler.
> >>
> >> Signed-off-by: Cédric Le Goater <address@hidden>
> >> Reviewed-by: Greg Kurz <address@hidden>
> >> ---
> >>  include/hw/ppc/pnv_core.h | 3 +++
> >>  hw/ppc/pnv_core.c         | 3 ++-
> >>  2 files changed, 5 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/include/hw/ppc/pnv_core.h b/include/hw/ppc/pnv_core.h
> >> index bfbd2ec42aa6..55eee95104da 100644
> >> --- a/include/hw/ppc/pnv_core.h
> >> +++ b/include/hw/ppc/pnv_core.h
> >> @@ -31,6 +31,8 @@
> >>  #define PNV_CORE_GET_CLASS(obj) \
> >>       OBJECT_GET_CLASS(PnvCoreClass, (obj), TYPE_PNV_CORE)
> >>  
> >> +typedef struct PnvChip PnvChip;
> >> +
> >>  typedef struct PnvCore {
> >>      /*< private >*/
> >>      CPUCore parent_obj;
> >> @@ -38,6 +40,7 @@ typedef struct PnvCore {
> >>      /*< public >*/
> >>      PowerPCCPU **threads;
> >>      uint32_t pir;
> >> +    PnvChip *chip;
> > 
> > I don't love having this as a redundant encoding of the information
> > already in the property, since it raises the possibility of confusing
> > bugs if they ever got out of sync.
> 
> Indeed.
> 
> > It's not a huge deal, but it would be nice to at least to at least
> > consider either a) grabbing the property everywhere you need it (if
> > there aren't too many places) 
> 
> We need the chip at core creation and core reset to call the 
> interrupt chip handlers. These are not hot path but the pointer
> seemed practical.
> 

FWIW this is also used at core destruction in this patch:

[1/3] ppc: Add intc_destroy() handlers to SpaprInterruptController/PnvChip
https://patchwork.ozlabs.org/patch/1183158/

> > or b) customizing the property
> > definition so it's written directly into that field.
> 
> OK. That is better than just a link.
> 

I think David is suggesting to use object_property_add_link()
instead of object_property_add_const_link() actually. Something
like that:

diff --git a/include/hw/ppc/pnv_core.h b/include/hw/ppc/pnv_core.h
index 55eee95104da..fce6d8d9b31b 100644
--- a/include/hw/ppc/pnv_core.h
+++ b/include/hw/ppc/pnv_core.h
@@ -36,11 +36,11 @@ typedef struct PnvChip PnvChip;
 typedef struct PnvCore {
     /*< private >*/
     CPUCore parent_obj;
+    PnvChip *chip;
 
     /*< public >*/
     PowerPCCPU **threads;
     uint32_t pir;
-    PnvChip *chip;
 
     MemoryRegion xscom_regs;
 } PnvCore;
diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 68cc3c81aa75..90449d33e422 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -1312,8 +1312,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..8562a9961845 100644
--- a/hw/ppc/pnv_core.c
+++ b/hw/ppc/pnv_core.c
@@ -217,15 +217,6 @@ 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);
 
     pc->threads = g_new(PowerPCCPU *, cc->nr_threads);
     for (i = 0; i < cc->nr_threads; i++) {
@@ -323,6 +314,14 @@ static void pnv_core_class_init(ObjectClass *oc, void 
*data)
     dc->props = pnv_core_properties;
 }
 
+static void pnv_core_instance_init(Object *obj)
+{
+    object_property_add_link(obj, "chip", TYPE_PNV_CHIP,
+                             (Object **) &PNV_CORE(obj)->chip,
+                             qdev_prop_allow_set_link_before_realize,
+                             0, &error_abort);
+}
+
 #define DEFINE_PNV_CORE_TYPE(family, cpu_model) \
     {                                           \
         .parent = TYPE_PNV_CORE,                \
@@ -335,6 +334,7 @@ static const TypeInfo pnv_core_infos[] = {
         .name           = TYPE_PNV_CORE,
         .parent         = TYPE_CPU_CORE,
         .instance_size  = sizeof(PnvCore),
+        .instance_init  = pnv_core_instance_init,
         .class_size     = sizeof(PnvCoreClass),
         .class_init = pnv_core_class_init,
         .abstract       = true,
----

> C. 
> 
> > 
> >>  
> >>      MemoryRegion xscom_regs;
> >>  } PnvCore;
> >> diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
> >> index 9f981a4940e6..cc17bbfed829 100644
> >> --- a/hw/ppc/pnv_core.c
> >> +++ b/hw/ppc/pnv_core.c
> >> @@ -222,6 +222,7 @@ static void pnv_core_realize(DeviceState *dev, Error 
> >> **errp)
> >>                                  "required link 'chip' not found: ");
> >>          return;
> >>      }
> >> +    pc->chip = PNV_CHIP(chip);
> >>  
> >>      pc->threads = g_new(PowerPCCPU *, cc->nr_threads);
> >>      for (i = 0; i < cc->nr_threads; i++) {
> >> @@ -243,7 +244,7 @@ static void pnv_core_realize(DeviceState *dev, Error 
> >> **errp)
> >>      }
> >>  
> >>      for (j = 0; j < cc->nr_threads; j++) {
> >> -        pnv_realize_vcpu(pc->threads[j], PNV_CHIP(chip), &local_err);
> >> +        pnv_realize_vcpu(pc->threads[j], pc->chip, &local_err);
> >>          if (local_err) {
> >>              goto err;
> >>          }
> > 
>