On Thu, 24 Oct 2019 13:50:25 +1100 David Gibson wrote: > 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. > Sure, better safe than sorry :) > > --- > > 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) > > { > > >