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