[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 1/6] ppc: Add intc_destroy() handlers to SpaprInterruptContro
|
From: |
David Gibson |
|
Subject: |
Re: [PATCH 1/6] ppc: Add intc_destroy() handlers to SpaprInterruptController/PnvChip |
|
Date: |
Thu, 24 Oct 2019 13:50:25 +1100 |
|
User-agent: |
Mutt/1.12.1 (2019-06-15) |
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.
> ---
> 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
signature.asc
Description: PGP signature