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 <[email protected]> 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 <[email protected]> > > >> Reviewed-by: Greg Kurz <[email protected]> > > >> --- > > >> 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
