[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 1/6] ppc: Add intc_destroy() handlers to SpaprInterruptContro
|
From: |
Greg Kurz |
|
Subject: |
Re: [PATCH 1/6] ppc: Add intc_destroy() handlers to SpaprInterruptController/PnvChip |
|
Date: |
Thu, 24 Oct 2019 09:20:14 +0200 |
On Thu, 24 Oct 2019 13:50:25 +1100
David Gibson <address@hidden> 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 <address@hidden>
>
> 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)
> > {
> >
>
pgpCLAVnB5dxr.pgp
Description: OpenPGP digital signature
[PATCH 5/6] spapr: Don't use CPU_FOREACH() in 'info pic', Greg Kurz, 2019/10/23