On Wed, Oct 23, 2019 at 04:51:59PM +0200, Greg Kurz wrote: > SpaprInterruptControllerClass and PnvChipClass have an intc_create() method > that calls the appropriate routine, ie. icp_create() or xive_tctx_create(), > to establish the link between the VCPU and the presenter component of the > interrupt controller during realize. > > There aren't any symmetrical call to be called when the VCPU gets unrealized > though. It is assumed that object_unparent() is the only thing to do. > > This is questionable because the parenting logic around the CPU and > presenter objects is really an implementation detail of the interrupt > controller. It shouldn't be open-coded in the machine code. > > Fix this by adding an intc_destroy() method that undoes what was done in > intc_create(). > > Signed-off-by: Greg Kurz I think it would be good to set the pointers in spapr_cpu_state() to NULL at the point of destroy. It probably won't matter, since this happens in a path where the cpu's about to go away. But still, leaving a stale pointer around a moment more than necessary makes me anxious. > --- > hw/intc/spapr_xive.c | 7 +++++++ > hw/intc/xics.c | 5 +++++ > hw/intc/xics_spapr.c | 7 +++++++ > hw/intc/xive.c | 5 +++++ > hw/ppc/pnv.c | 15 +++++++++++++++ > hw/ppc/pnv_core.c | 7 ++++--- > hw/ppc/spapr_cpu_core.c | 7 +------ > hw/ppc/spapr_irq.c | 14 ++++++++++++++ > include/hw/ppc/pnv.h | 1 + > include/hw/ppc/spapr_irq.h | 2 ++ > include/hw/ppc/xics.h | 1 + > include/hw/ppc/xive.h | 1 + > 12 files changed, 63 insertions(+), 9 deletions(-) > > diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c > index d8e1291905c3..b09cc48bcb61 100644 > --- a/hw/intc/spapr_xive.c > +++ b/hw/intc/spapr_xive.c > @@ -555,6 +555,12 @@ static void spapr_xive_cpu_intc_reset(SpaprInterruptController *intc, > xive_tctx_set_os_cam(tctx, xive_nvt_cam_line(nvt_blk, nvt_idx)); > } > > +static void spapr_xive_cpu_intc_destroy(SpaprInterruptController *intc, > + PowerPCCPU *cpu) > +{ > + xive_tctx_destroy(spapr_cpu_state(cpu)->tctx); > +} > + > static void spapr_xive_set_irq(SpaprInterruptController *intc, int irq, int val) > { > SpaprXive *xive = SPAPR_XIVE(intc); > @@ -692,6 +698,7 @@ static void spapr_xive_class_init(ObjectClass *klass, void *data) > sicc->deactivate = spapr_xive_deactivate; > sicc->cpu_intc_create = spapr_xive_cpu_intc_create; > sicc->cpu_intc_reset = spapr_xive_cpu_intc_reset; > + sicc->cpu_intc_destroy = spapr_xive_cpu_intc_destroy; > sicc->claim_irq = spapr_xive_claim_irq; > sicc->free_irq = spapr_xive_free_irq; > sicc->set_irq = spapr_xive_set_irq; > diff --git a/hw/intc/xics.c b/hw/intc/xics.c > index 6da05763f9db..935f325749cb 100644 > --- a/hw/intc/xics.c > +++ b/hw/intc/xics.c > @@ -401,6 +401,11 @@ Object *icp_create(Object *cpu, const char *type, XICSFabric *xi, Error **errp) > return obj; > } > > +void icp_destroy(ICPState *icp) > +{ > + object_unparent(OBJECT(icp)); > +} > + > /* > * ICS: Source layer > */ > diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c > index 7418fb9f370c..5977d1bdb73f 100644 > --- a/hw/intc/xics_spapr.c > +++ b/hw/intc/xics_spapr.c > @@ -352,6 +352,12 @@ static void xics_spapr_cpu_intc_reset(SpaprInterruptController *intc, > icp_reset(spapr_cpu_state(cpu)->icp); > } > > +static void xics_spapr_cpu_intc_destroy(SpaprInterruptController *intc, > + PowerPCCPU *cpu) > +{ > + icp_destroy(spapr_cpu_state(cpu)->icp); > +} > + > static int xics_spapr_claim_irq(SpaprInterruptController *intc, int irq, > bool lsi, Error **errp) > { > @@ -440,6 +446,7 @@ static void ics_spapr_class_init(ObjectClass *klass, void *data) > sicc->deactivate = xics_spapr_deactivate; > sicc->cpu_intc_create = xics_spapr_cpu_intc_create; > sicc->cpu_intc_reset = xics_spapr_cpu_intc_reset; > + sicc->cpu_intc_destroy = xics_spapr_cpu_intc_destroy; > sicc->claim_irq = xics_spapr_claim_irq; > sicc->free_irq = xics_spapr_free_irq; > sicc->set_irq = xics_spapr_set_irq; > diff --git a/hw/intc/xive.c b/hw/intc/xive.c > index f066be5eb5e3..38257aa02083 100644 > --- a/hw/intc/xive.c > +++ b/hw/intc/xive.c > @@ -696,6 +696,11 @@ error: > return NULL; > } > > +void xive_tctx_destroy(XiveTCTX *tctx) > +{ > + object_unparent(OBJECT(tctx)); > +} > + > /* > * XIVE ESB helpers > */ > diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c > index 4a51fb65a834..bd17c3536dd5 100644 > --- a/hw/ppc/pnv.c > +++ b/hw/ppc/pnv.c > @@ -778,6 +778,7 @@ static void pnv_chip_power8_intc_create(PnvChip *chip, PowerPCCPU *cpu, > pnv_cpu->intc = obj; > } > > + > static void pnv_chip_power8_intc_reset(PnvChip *chip, PowerPCCPU *cpu) > { > PnvCPUState *pnv_cpu = pnv_cpu_state(cpu); > @@ -785,6 +786,11 @@ static void pnv_chip_power8_intc_reset(PnvChip *chip, PowerPCCPU *cpu) > icp_reset(ICP(pnv_cpu->intc)); > } > > +static void pnv_chip_power8_intc_destroy(PnvChip *chip, PowerPCCPU *cpu) > +{ > + icp_destroy(ICP(pnv_cpu_state(cpu)->intc)); > +} > + > /* > * 0:48 Reserved - Read as zeroes > * 49:52 Node ID > @@ -829,6 +835,11 @@ static void pnv_chip_power9_intc_reset(PnvChip *chip, PowerPCCPU *cpu) > xive_tctx_reset(XIVE_TCTX(pnv_cpu->intc)); > } > > +static void pnv_chip_power9_intc_destroy(PnvChip *chip, PowerPCCPU *cpu) > +{ > + xive_tctx_destroy(XIVE_TCTX(pnv_cpu_state(cpu)->intc)); > +} > + > /* > * Allowed core identifiers on a POWER8 Processor Chip : > * > @@ -999,6 +1010,7 @@ static void pnv_chip_power8e_class_init(ObjectClass *klass, void *data) > k->core_pir = pnv_chip_core_pir_p8; > k->intc_create = pnv_chip_power8_intc_create; > k->intc_reset = pnv_chip_power8_intc_reset; > + k->intc_destroy = pnv_chip_power8_intc_destroy; > k->isa_create = pnv_chip_power8_isa_create; > k->dt_populate = pnv_chip_power8_dt_populate; > k->pic_print_info = pnv_chip_power8_pic_print_info; > @@ -1019,6 +1031,7 @@ static void pnv_chip_power8_class_init(ObjectClass *klass, void *data) > k->core_pir = pnv_chip_core_pir_p8; > k->intc_create = pnv_chip_power8_intc_create; > k->intc_reset = pnv_chip_power8_intc_reset; > + k->intc_destroy = pnv_chip_power8_intc_destroy; > k->isa_create = pnv_chip_power8_isa_create; > k->dt_populate = pnv_chip_power8_dt_populate; > k->pic_print_info = pnv_chip_power8_pic_print_info; > @@ -1039,6 +1052,7 @@ static void pnv_chip_power8nvl_class_init(ObjectClass *klass, void *data) > k->core_pir = pnv_chip_core_pir_p8; > k->intc_create = pnv_chip_power8_intc_create; > k->intc_reset = pnv_chip_power8_intc_reset; > + k->intc_destroy = pnv_chip_power8_intc_destroy; > k->isa_create = pnv_chip_power8nvl_isa_create; > k->dt_populate = pnv_chip_power8_dt_populate; > k->pic_print_info = pnv_chip_power8_pic_print_info; > @@ -1209,6 +1223,7 @@ static void pnv_chip_power9_class_init(ObjectClass *klass, void *data) > k->core_pir = pnv_chip_core_pir_p9; > k->intc_create = pnv_chip_power9_intc_create; > k->intc_reset = pnv_chip_power9_intc_reset; > + k->intc_destroy = pnv_chip_power9_intc_destroy; > k->isa_create = pnv_chip_power9_isa_create; > k->dt_populate = pnv_chip_power9_dt_populate; > k->pic_print_info = pnv_chip_power9_pic_print_info; > diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c > index 328ad07c8d06..a66c4b471407 100644 > --- a/hw/ppc/pnv_core.c > +++ b/hw/ppc/pnv_core.c > @@ -270,11 +270,12 @@ err: > error_propagate(errp, local_err); > } > > -static void pnv_core_cpu_unrealize(PowerPCCPU *cpu) > +static void pnv_core_cpu_unrealize(PowerPCCPU *cpu, PnvChip *chip) > { > PnvCPUState *pnv_cpu = pnv_cpu_state(cpu); > + PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip); > > - object_unparent(OBJECT(pnv_cpu_state(cpu)->intc)); > + pcc->intc_destroy(chip, cpu); > cpu_remove_sync(CPU(cpu)); > cpu->machine_data = NULL; > g_free(pnv_cpu); > @@ -290,7 +291,7 @@ static void pnv_core_unrealize(DeviceState *dev, Error **errp) > qemu_unregister_reset(pnv_core_reset, pc); > > for (i = 0; i < cc->nr_threads; i++) { > - pnv_core_cpu_unrealize(pc->threads[i]); > + pnv_core_cpu_unrealize(pc->threads[i], pc->chip); > } > g_free(pc->threads); > } > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c > index ef7b27a66d56..8339c4c0f86b 100644 > --- a/hw/ppc/spapr_cpu_core.c > +++ b/hw/ppc/spapr_cpu_core.c > @@ -195,12 +195,7 @@ static void spapr_unrealize_vcpu(PowerPCCPU *cpu, SpaprCpuCore *sc) > if (!sc->pre_3_0_migration) { > vmstate_unregister(NULL, &vmstate_spapr_cpu_state, cpu->machine_data); > } > - if (spapr_cpu_state(cpu)->icp) { > - object_unparent(OBJECT(spapr_cpu_state(cpu)->icp)); > - } > - if (spapr_cpu_state(cpu)->tctx) { > - object_unparent(OBJECT(spapr_cpu_state(cpu)->tctx)); > - } > + spapr_irq_cpu_intc_destroy(SPAPR_MACHINE(qdev_get_machine()), cpu); > cpu_remove_sync(CPU(cpu)); > object_unparent(OBJECT(cpu)); > } > diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c > index b941608b69ba..168044be853a 100644 > --- a/hw/ppc/spapr_irq.c > +++ b/hw/ppc/spapr_irq.c > @@ -234,6 +234,20 @@ void spapr_irq_cpu_intc_reset(SpaprMachineState *spapr, PowerPCCPU *cpu) > } > } > > +void spapr_irq_cpu_intc_destroy(SpaprMachineState *spapr, PowerPCCPU *cpu) > +{ > + SpaprInterruptController *intcs[] = ALL_INTCS(spapr); > + int i; > + > + for (i = 0; i < ARRAY_SIZE(intcs); i++) { > + SpaprInterruptController *intc = intcs[i]; > + if (intc) { > + SpaprInterruptControllerClass *sicc = SPAPR_INTC_GET_CLASS(intc); > + sicc->cpu_intc_destroy(intc, cpu); > + } > + } > +} > + > static void spapr_set_irq(void *opaque, int irq, int level) > { > SpaprMachineState *spapr = SPAPR_MACHINE(opaque); > diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h > index 2a780e633f23..0b4c722e6b48 100644 > --- a/include/hw/ppc/pnv.h > +++ b/include/hw/ppc/pnv.h > @@ -112,6 +112,7 @@ typedef struct PnvChipClass { > uint32_t (*core_pir)(PnvChip *chip, uint32_t core_id); > void (*intc_create)(PnvChip *chip, PowerPCCPU *cpu, Error **errp); > void (*intc_reset)(PnvChip *chip, PowerPCCPU *cpu); > + void (*intc_destroy)(PnvChip *chip, PowerPCCPU *cpu); > ISABus *(*isa_create)(PnvChip *chip, Error **errp); > void (*dt_populate)(PnvChip *chip, void *fdt); > void (*pic_print_info)(PnvChip *chip, Monitor *mon); > diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h > index 09232999b07e..ff814d13de37 100644 > --- a/include/hw/ppc/spapr_irq.h > +++ b/include/hw/ppc/spapr_irq.h > @@ -53,6 +53,7 @@ typedef struct SpaprInterruptControllerClass { > int (*cpu_intc_create)(SpaprInterruptController *intc, > PowerPCCPU *cpu, Error **errp); > void (*cpu_intc_reset)(SpaprInterruptController *intc, PowerPCCPU *cpu); > + void (*cpu_intc_destroy)(SpaprInterruptController *intc, PowerPCCPU *cpu); > int (*claim_irq)(SpaprInterruptController *intc, int irq, bool lsi, > Error **errp); > void (*free_irq)(SpaprInterruptController *intc, int irq); > @@ -70,6 +71,7 @@ void spapr_irq_update_active_intc(SpaprMachineState *spapr); > int spapr_irq_cpu_intc_create(SpaprMachineState *spapr, > PowerPCCPU *cpu, Error **errp); > void spapr_irq_cpu_intc_reset(SpaprMachineState *spapr, PowerPCCPU *cpu); > +void spapr_irq_cpu_intc_destroy(SpaprMachineState *spapr, PowerPCCPU *cpu); > void spapr_irq_print_info(SpaprMachineState *spapr, Monitor *mon); > void spapr_irq_dt(SpaprMachineState *spapr, uint32_t nr_servers, > void *fdt, uint32_t phandle); > diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h > index 602173c12250..48a75aa4ab75 100644 > --- a/include/hw/ppc/xics.h > +++ b/include/hw/ppc/xics.h > @@ -181,6 +181,7 @@ void icp_resend(ICPState *ss); > > Object *icp_create(Object *cpu, const char *type, XICSFabric *xi, > Error **errp); > +void icp_destroy(ICPState *icp); > > /* KVM */ > void icp_get_kvm_state(ICPState *icp); > diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h > index 99381639f50c..8fd439ec9bba 100644 > --- a/include/hw/ppc/xive.h > +++ b/include/hw/ppc/xive.h > @@ -416,6 +416,7 @@ uint64_t xive_tctx_tm_read(XiveTCTX *tctx, hwaddr offset, unsigned size); > void xive_tctx_pic_print_info(XiveTCTX *tctx, Monitor *mon); > Object *xive_tctx_create(Object *cpu, XiveRouter *xrtr, Error **errp); > void xive_tctx_reset(XiveTCTX *tctx); > +void xive_tctx_destroy(XiveTCTX *tctx); > > static inline uint32_t xive_nvt_cam_line(uint8_t nvt_blk, uint32_t nvt_idx) > { > -- 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