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 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 > > >> Reviewed-by: Greg Kurz > > >> --- > > >> 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