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

[David Gibson]

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

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

signature.asc
Description: PGP signature