We will use it to reset the interrupt presenter from the CPU reset
handler.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
Reviewed-by: Greg Kurz <groug@kaod.org>
---
 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;
 
     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;
         }
-- 
2.21.0

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 <clg@kaod.org>
> Reviewed-by: Greg Kurz <groug@kaod.org>
> ---
>  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.

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) or b) customizing the property
definition so it's written directly into that field.

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

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

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 <clg@kaod.org>
>> Reviewed-by: Greg Kurz <groug@kaod.org>
>> ---
>>  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.

> or b) customizing the property
> definition so it's written directly into that field.

OK. That is better than just a link.

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

On Thu, 24 Oct 2019 11:57:05 +0200
Cédric Le Goater <clg@kaod.org> 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 <clg@kaod.org>
> >> Reviewed-by: Greg Kurz <groug@kaod.org>
> >> ---
> >>  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;
> >>          }
> > 
> 

On Thu, Oct 24, 2019 at 06:48:23PM +0200, Greg Kurz wrote:
> On Thu, 24 Oct 2019 11:57:05 +0200
> Cédric Le Goater <clg@kaod.org> 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 <clg@kaod.org>
> > >> Reviewed-by: Greg Kurz <groug@kaod.org>
> > >> ---
> > >>  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:

TBH, I hadn't looked into the mechanics of how to do this that
closely.  Change below looks pretty reasonable, though.

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

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

On Thu, Oct 24, 2019 at 11:57:05AM +0200, Cédric Le Goater 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 <clg@kaod.org>
> >> Reviewed-by: Greg Kurz <groug@kaod.org>
> >> ---
> >>  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.
> 
> > or b) customizing the property
> > definition so it's written directly into that field.
> 
> OK. That is better than just a link.

I guess.  If there are only two non hot path callers, it seems like it
might be simpler to just pull it out of the property at those places.

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

On Thu, 24 Oct 2019 13:38:12 +1100
David Gibson <david@gibson.dropbear.id.au> 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 <clg@kaod.org>
> > Reviewed-by: Greg Kurz <groug@kaod.org>
> > ---
> >  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.
> 

Ouch, we also have this pattern in xive_tctx_realize(). The XiveTCXT
object has both a "cpu" property and a pointer to the vCPU.

> 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) or b) customizing the property
> definition so it's written directly into that field.
> 

The pointer to the vCPU is used among other things to get the
value of the PIR, which is needed by the presenting logic to
match physical CAM lines. This is a _hot_ path so it's probably
better to go for b).

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

On Thu, Oct 24, 2019 at 07:30:30PM +0200, Greg Kurz wrote:
> On Thu, 24 Oct 2019 13:38:12 +1100
> David Gibson <david@gibson.dropbear.id.au> 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 <clg@kaod.org>
> > > Reviewed-by: Greg Kurz <groug@kaod.org>
> > > ---
> > >  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.
> > 
> 
> Ouch, we also have this pattern in xive_tctx_realize(). The XiveTCXT
> object has both a "cpu" property and a pointer to the vCPU.
> 
> > 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) or b) customizing the property
> > definition so it's written directly into that field.
> > 
> 
> The pointer to the vCPU is used among other things to get the
> value of the PIR, which is needed by the presenting logic to
> match physical CAM lines. This is a _hot_ path so it's probably
> better to go for b).

Agreed.

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

On 10/22/19 6:38 PM, 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 <clg@kaod.org>
> Reviewed-by: Greg Kurz <groug@kaod.org>
> ---
>   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;
>   
>       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;
>           }
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>