From 0990ce6a2e900d0bdda7f3ecdc991746f63551fb Mon Sep 17 00:00:00 2001 From: Greg Kurz Date: Thu, 24 Oct 2019 16:27:22 +0200 Subject: ppc: Add intc_destroy() handlers to SpaprInterruptController/PnvChip 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(). Also NULLify the presenter pointers to avoid having stale pointers around. This will allow to reliably check if a vCPU has a valid presenter. Signed-off-by: Greg Kurz Message-Id: <157192724208.3146912.7254684777515287626.stgit@bahia.lan> Signed-off-by: David Gibson Signed-off-by: Laurent Vivier --- hw/intc/spapr_xive.c | 10 ++++++++++ hw/intc/xics.c | 5 +++++ hw/intc/xics_spapr.c | 10 ++++++++++ hw/intc/xive.c | 5 +++++ 4 files changed, 30 insertions(+) (limited to 'hw/intc') diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c index d8e1291..9cb8d38 100644 --- a/hw/intc/spapr_xive.c +++ b/hw/intc/spapr_xive.c @@ -555,6 +555,15 @@ 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) +{ + SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu); + + xive_tctx_destroy(spapr_cpu->tctx); + spapr_cpu->tctx = NULL; +} + static void spapr_xive_set_irq(SpaprInterruptController *intc, int irq, int val) { SpaprXive *xive = SPAPR_XIVE(intc); @@ -692,6 +701,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 6da0576..935f325 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 7418fb9..b3705da 100644 --- a/hw/intc/xics_spapr.c +++ b/hw/intc/xics_spapr.c @@ -352,6 +352,15 @@ 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) +{ + SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu); + + icp_destroy(spapr_cpu->icp); + spapr_cpu->icp = NULL; +} + static int xics_spapr_claim_irq(SpaprInterruptController *intc, int irq, bool lsi, Error **errp) { @@ -440,6 +449,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 f066be5..38257aa 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 */ -- cgit v1.1 From 35886de140b7ff781b775d2da5e7475e8a8cb4c6 Mon Sep 17 00:00:00 2001 From: Greg Kurz Date: Thu, 24 Oct 2019 16:27:27 +0200 Subject: xive, xics: Fix reference counting on CPU objects When a VCPU gets connected to the XIVE interrupt controller, we add a const link targetting the CPU object to the TCTX object. Similar links are added to the ICP object when using the XICS interrupt controller. As explained in : * The caller must ensure that @target stays alive as long as * this property exists. In the case @target is a child of @obj, * this will be the case. Otherwise, the caller is responsible for * taking a reference. We're in the latter case for both XICS and XIVE. Add the missing calls to object_ref() and object_unref(). This doesn't fix any known issue because the life cycle of the TCTX or ICP happens to be shorter than the one of the CPU or XICS fabric, but better safe than sorry. Signed-off-by: Greg Kurz Reviewed-by: David Gibson Message-Id: <157192724770.3146912.15400869269097231255.stgit@bahia.lan> Signed-off-by: David Gibson Signed-off-by: Laurent Vivier --- hw/intc/xics.c | 8 +++++++- hw/intc/xive.c | 6 +++++- 2 files changed, 12 insertions(+), 2 deletions(-) (limited to 'hw/intc') diff --git a/hw/intc/xics.c b/hw/intc/xics.c index 935f325..5f74607 100644 --- a/hw/intc/xics.c +++ b/hw/intc/xics.c @@ -388,8 +388,10 @@ Object *icp_create(Object *cpu, const char *type, XICSFabric *xi, Error **errp) obj = object_new(type); object_property_add_child(cpu, type, obj, &error_abort); object_unref(obj); + object_ref(OBJECT(xi)); object_property_add_const_link(obj, ICP_PROP_XICS, OBJECT(xi), &error_abort); + object_ref(cpu); object_property_add_const_link(obj, ICP_PROP_CPU, cpu, &error_abort); object_property_set_bool(obj, true, "realized", &local_err); if (local_err) { @@ -403,7 +405,11 @@ Object *icp_create(Object *cpu, const char *type, XICSFabric *xi, Error **errp) void icp_destroy(ICPState *icp) { - object_unparent(OBJECT(icp)); + Object *obj = OBJECT(icp); + + object_unref(object_property_get_link(obj, ICP_PROP_CPU, &error_abort)); + object_unref(object_property_get_link(obj, ICP_PROP_XICS, &error_abort)); + object_unparent(obj); } /* diff --git a/hw/intc/xive.c b/hw/intc/xive.c index 38257aa..952a461 100644 --- a/hw/intc/xive.c +++ b/hw/intc/xive.c @@ -682,6 +682,7 @@ Object *xive_tctx_create(Object *cpu, XiveRouter *xrtr, Error **errp) obj = object_new(TYPE_XIVE_TCTX); object_property_add_child(cpu, TYPE_XIVE_TCTX, obj, &error_abort); object_unref(obj); + object_ref(cpu); object_property_add_const_link(obj, "cpu", cpu, &error_abort); object_property_set_bool(obj, true, "realized", &local_err); if (local_err) { @@ -698,7 +699,10 @@ error: void xive_tctx_destroy(XiveTCTX *tctx) { - object_unparent(OBJECT(tctx)); + Object *obj = OBJECT(tctx); + + object_unref(object_property_get_link(obj, "cpu", &error_abort)); + object_unparent(obj); } /* -- cgit v1.1 From 0a83b47055246d3942084f03fc54731c4fb9b731 Mon Sep 17 00:00:00 2001 From: Greg Kurz Date: Thu, 24 Oct 2019 16:27:33 +0200 Subject: ppc: Skip partially initialized vCPUs in 'info pic' CPU_FOREACH() can race with vCPU hotplug/unplug on sPAPR machines, ie. we may try to print out info about a vCPU with a NULL presenter pointer. Check that in order to prevent QEMU from crashing. Signed-off-by: Greg Kurz Message-Id: <157192725327.3146912.12047076483178652551.stgit@bahia.lan> Signed-off-by: David Gibson Signed-off-by: Laurent Vivier --- hw/intc/xics.c | 11 ++++++++++- hw/intc/xive.c | 11 ++++++++++- 2 files changed, 20 insertions(+), 2 deletions(-) (limited to 'hw/intc') diff --git a/hw/intc/xics.c b/hw/intc/xics.c index 5f74607..e7ac9ba 100644 --- a/hw/intc/xics.c +++ b/hw/intc/xics.c @@ -44,7 +44,16 @@ void icp_pic_print_info(ICPState *icp, Monitor *mon) { - int cpu_index = icp->cs ? icp->cs->cpu_index : -1; + int cpu_index; + + /* Skip partially initialized vCPUs. This can happen on sPAPR when vCPUs + * are hot plugged or unplugged. + */ + if (!icp) { + return; + } + + cpu_index = icp->cs ? icp->cs->cpu_index : -1; if (!icp->output) { return; diff --git a/hw/intc/xive.c b/hw/intc/xive.c index 952a461..75dce82 100644 --- a/hw/intc/xive.c +++ b/hw/intc/xive.c @@ -523,9 +523,18 @@ static const char * const xive_tctx_ring_names[] = { void xive_tctx_pic_print_info(XiveTCTX *tctx, Monitor *mon) { - int cpu_index = tctx->cs ? tctx->cs->cpu_index : -1; + int cpu_index; int i; + /* Skip partially initialized vCPUs. This can happen on sPAPR when vCPUs + * are hot plugged or unplugged. + */ + if (!tctx) { + return; + } + + cpu_index = tctx->cs ? tctx->cs->cpu_index : -1; + if (kvm_irqchip_in_kernel()) { Error *local_err = NULL; -- cgit v1.1