* [PATCH 0/6] ppc: Reparent the interrupt presenter
@ 2019-10-23 14:51 Greg Kurz
2019-10-23 14:51 ` [PATCH 1/6] ppc: Add intc_destroy() handlers to SpaprInterruptController/PnvChip Greg Kurz
` (5 more replies)
0 siblings, 6 replies; 22+ messages in thread
From: Greg Kurz @ 2019-10-23 14:51 UTC (permalink / raw)
To: David Gibson; +Cc: Paolo Bonzini, qemu-ppc, Cédric Le Goater, qemu-devel
The interrupt presenters are currently parented to their associated
VCPU, and we rely on CPU_FOREACH() when we need to perform a specific
task with them. Like exposing their state with 'info pic', or finding
the target VCPU for an interrupt when using the XIVE controller.
We recently realized that the latter could crash QEMU because CPU_FOREACH()
can race with CPU hotplug. This got fixed by checking the presenter pointer
under the CPU was set (commit 627fa61746f7) but this is still fragile. And
we still can crash QEMU with 'info pic' while doing CPU hotplug/unplug:
With XIVE:
Thread 1 "qemu-system-ppc" received signal SIGSEGV, Segmentation fault.
0x00000001003d2848 in xive_tctx_pic_print_info (tctx=0x101ae5280,
mon=0x7fffffffe180) at /home/greg/Work/qemu/qemu-spapr/hw/intc/xive.c:526
526 int cpu_index = tctx->cs ? tctx->cs->cpu_index : -1;
(gdb) p tctx
$1 = (XiveTCTX *) 0x101ae5280
(gdb) p tctx->cs
$2 = (CPUState *) 0x2057512020203a5d
(gdb) p tctx->cs->cpu_index
Cannot access memory at address 0x205751202020bead
With XICS:
Thread 1 "qemu-system-ppc" received signal SIGSEGV, Segmentation fault.
0x00000001003cc39c in icp_pic_print_info (icp=0x10244ccf0, mon=0x7fffffffe940)
at /home/greg/Work/qemu/qemu-spapr/hw/intc/xics.c:47
47 int cpu_index = icp->cs ? icp->cs->cpu_index : -1;
(gdb) p icp
$1 = (ICPState *) 0x10244ccf0
(gdb) p icp->cs
$2 = (CPUState *) 0x524958203220
(gdb) p icp->cs->cpu_index
Cannot access memory at address 0x52495820b670
This series fixes the issue globally by moving the presenter objects under
the interrupt controller and to loop on them with object_child_foreach()
instead of CPU_FOREACH().
It is based on Cédric Le Goater's series:
[v5,0/7] ppc: reset the interrupt presenter from the CPU reset handler
https://patchwork.ozlabs.org/cover/1181522/
--
Greg
---
Greg Kurz (6):
ppc: Add intc_destroy() handlers to SpaprInterruptController/PnvChip
xive, xics: Fix reference counting on CPU objects
ppc: Reparent presenter objects to the interrupt controller object
qom: Add object_child_foreach_type() helper function
spapr: Don't use CPU_FOREACH() in 'info pic'
xive: Don't use CPU_FOREACH() to perform CAM line matching
hw/intc/spapr_xive.c | 19 ++++---
hw/intc/xics.c | 30 ++++++++++-
hw/intc/xics_spapr.c | 21 +++++--
hw/intc/xive.c | 125 ++++++++++++++++++++++++++++++--------------
hw/ppc/pnv.c | 28 +++++++++-
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 | 4 +
include/hw/ppc/xive.h | 3 +
include/qom/object.h | 35 ++++++++++++
qom/object.c | 30 ++++++++---
14 files changed, 251 insertions(+), 75 deletions(-)
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 1/6] ppc: Add intc_destroy() handlers to SpaprInterruptController/PnvChip
2019-10-23 14:51 [PATCH 0/6] ppc: Reparent the interrupt presenter Greg Kurz
@ 2019-10-23 14:51 ` Greg Kurz
2019-10-24 2:50 ` David Gibson
2019-10-23 14:52 ` [PATCH 2/6] xive, xics: Fix reference counting on CPU objects Greg Kurz
` (4 subsequent siblings)
5 siblings, 1 reply; 22+ messages in thread
From: Greg Kurz @ 2019-10-23 14:51 UTC (permalink / raw)
To: David Gibson; +Cc: Paolo Bonzini, qemu-ppc, Cédric Le Goater, qemu-devel
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 <groug@kaod.org>
---
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)
{
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 2/6] xive, xics: Fix reference counting on CPU objects
2019-10-23 14:51 [PATCH 0/6] ppc: Reparent the interrupt presenter Greg Kurz
2019-10-23 14:51 ` [PATCH 1/6] ppc: Add intc_destroy() handlers to SpaprInterruptController/PnvChip Greg Kurz
@ 2019-10-23 14:52 ` Greg Kurz
2019-10-24 2:50 ` David Gibson
2019-10-23 14:52 ` [PATCH 3/6] ppc: Reparent presenter objects to the interrupt controller object Greg Kurz
` (3 subsequent siblings)
5 siblings, 1 reply; 22+ messages in thread
From: Greg Kurz @ 2019-10-23 14:52 UTC (permalink / raw)
To: David Gibson; +Cc: Paolo Bonzini, qemu-ppc, Cédric Le Goater, qemu-devel
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 <qom/object.h>:
* 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 <groug@kaod.org>
---
hw/intc/xics.c | 8 +++++++-
hw/intc/xive.c | 6 +++++-
2 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/hw/intc/xics.c b/hw/intc/xics.c
index 935f325749cb..5f746079be46 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 38257aa02083..952a461d5329 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);
}
/*
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 3/6] ppc: Reparent presenter objects to the interrupt controller object
2019-10-23 14:51 [PATCH 0/6] ppc: Reparent the interrupt presenter Greg Kurz
2019-10-23 14:51 ` [PATCH 1/6] ppc: Add intc_destroy() handlers to SpaprInterruptController/PnvChip Greg Kurz
2019-10-23 14:52 ` [PATCH 2/6] xive, xics: Fix reference counting on CPU objects Greg Kurz
@ 2019-10-23 14:52 ` Greg Kurz
2019-10-24 2:58 ` David Gibson
2019-10-23 14:52 ` [PATCH 4/6] qom: Add object_child_foreach_type() helper function Greg Kurz
` (2 subsequent siblings)
5 siblings, 1 reply; 22+ messages in thread
From: Greg Kurz @ 2019-10-23 14:52 UTC (permalink / raw)
To: David Gibson; +Cc: Paolo Bonzini, qemu-ppc, Cédric Le Goater, qemu-devel
Each VCPU is associated to a presenter object within the interrupt
controller, ie. TCTX for XIVE and ICP for XICS, but our current
models put these objects below the VCPU, and we rely on CPU_FOREACH()
to do anything that involves presenters.
This recently bit us with the CAM line matching logic in XIVE because
CPU_FOREACH() can race with CPU hotplug and we ended up considering a
VCPU that wasn't associated to a TCTX object yet. Other users of
CPU_FOREACH() are 'info pic' for both XICS and XIVE. It is again very
easy to crash QEMU with concurrent VCPU hotplug/unplug and 'info pic'.
Reparent the presenter objects to the corresponding interrupt controller
object, ie. XIVE router or ICS, to make it clear they are not some extra
data hanging from the CPU but internal XIVE or XICS entities. The CPU
object now needs to explicitely take a reference on the presenter to
ensure its pointer remains valid until unrealize time.
This will allow to get rid of CPU_FOREACH() and ease further improvements
to the XIVE model.
This change doesn't impact section ids and is thus transparent to
migration.
Signed-off-by: Greg Kurz <groug@kaod.org>
---
hw/intc/spapr_xive.c | 6 +++++-
hw/intc/xics.c | 7 +++++--
hw/intc/xics_spapr.c | 8 ++++++--
hw/intc/xive.c | 4 +++-
hw/ppc/pnv.c | 17 +++++++++++++----
include/hw/ppc/xics.h | 2 +-
6 files changed, 33 insertions(+), 11 deletions(-)
diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
index b09cc48bcb61..d74ee71e76b4 100644
--- a/hw/intc/spapr_xive.c
+++ b/hw/intc/spapr_xive.c
@@ -526,6 +526,7 @@ static int spapr_xive_cpu_intc_create(SpaprInterruptController *intc,
return -1;
}
+ object_ref(obj);
spapr_cpu->tctx = XIVE_TCTX(obj);
return 0;
}
@@ -558,7 +559,10 @@ static void spapr_xive_cpu_intc_reset(SpaprInterruptController *intc,
static void spapr_xive_cpu_intc_destroy(SpaprInterruptController *intc,
PowerPCCPU *cpu)
{
- xive_tctx_destroy(spapr_cpu_state(cpu)->tctx);
+ XiveTCTX *tctx = spapr_cpu_state(cpu)->tctx;
+
+ object_unref(OBJECT(tctx));
+ xive_tctx_destroy(tctx);
}
static void spapr_xive_set_irq(SpaprInterruptController *intc, int irq, int val)
diff --git a/hw/intc/xics.c b/hw/intc/xics.c
index 5f746079be46..d5e4db668a4b 100644
--- a/hw/intc/xics.c
+++ b/hw/intc/xics.c
@@ -380,13 +380,16 @@ static const TypeInfo icp_info = {
.class_size = sizeof(ICPStateClass),
};
-Object *icp_create(Object *cpu, const char *type, XICSFabric *xi, Error **errp)
+Object *icp_create(Object *cpu, const char *type, ICSState *ics, XICSFabric *xi,
+ Error **errp)
{
Error *local_err = NULL;
+ g_autofree char *name = NULL;
Object *obj;
obj = object_new(type);
- object_property_add_child(cpu, type, obj, &error_abort);
+ name = g_strdup_printf("%s[%d]", type, CPU(cpu)->cpu_index);
+ object_property_add_child(OBJECT(ics), name, obj, &error_abort);
object_unref(obj);
object_ref(OBJECT(xi));
object_property_add_const_link(obj, ICP_PROP_XICS, OBJECT(xi),
diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
index 5977d1bdb73f..080ed73aad64 100644
--- a/hw/intc/xics_spapr.c
+++ b/hw/intc/xics_spapr.c
@@ -337,11 +337,12 @@ static int xics_spapr_cpu_intc_create(SpaprInterruptController *intc,
Object *obj;
SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu);
- obj = icp_create(OBJECT(cpu), TYPE_ICP, ics->xics, errp);
+ obj = icp_create(OBJECT(cpu), TYPE_ICP, ics, ics->xics, errp);
if (!obj) {
return -1;
}
+ object_ref(obj);
spapr_cpu->icp = ICP(obj);
return 0;
}
@@ -355,7 +356,10 @@ static void xics_spapr_cpu_intc_reset(SpaprInterruptController *intc,
static void xics_spapr_cpu_intc_destroy(SpaprInterruptController *intc,
PowerPCCPU *cpu)
{
- icp_destroy(spapr_cpu_state(cpu)->icp);
+ ICPState *icp = spapr_cpu_state(cpu)->icp;
+
+ object_unref(OBJECT(icp));
+ icp_destroy(icp);
}
static int xics_spapr_claim_irq(SpaprInterruptController *intc, int irq,
diff --git a/hw/intc/xive.c b/hw/intc/xive.c
index 952a461d5329..8d2da4a11163 100644
--- a/hw/intc/xive.c
+++ b/hw/intc/xive.c
@@ -677,10 +677,12 @@ static const TypeInfo xive_tctx_info = {
Object *xive_tctx_create(Object *cpu, XiveRouter *xrtr, Error **errp)
{
Error *local_err = NULL;
+ g_autofree char *name = NULL;
Object *obj;
obj = object_new(TYPE_XIVE_TCTX);
- object_property_add_child(cpu, TYPE_XIVE_TCTX, obj, &error_abort);
+ name = g_strdup_printf(TYPE_XIVE_TCTX "[%d]", CPU(cpu)->cpu_index);
+ object_property_add_child(OBJECT(xrtr), name, obj, &error_abort);
object_unref(obj);
object_ref(cpu);
object_property_add_const_link(obj, "cpu", cpu, &error_abort);
diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index bd17c3536dd5..cbeabf98bff6 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -767,14 +767,16 @@ static void pnv_chip_power8_intc_create(PnvChip *chip, PowerPCCPU *cpu,
Error *local_err = NULL;
Object *obj;
PnvCPUState *pnv_cpu = pnv_cpu_state(cpu);
+ Pnv8Chip *chip8 = PNV8_CHIP(chip);
- obj = icp_create(OBJECT(cpu), TYPE_PNV_ICP, XICS_FABRIC(qdev_get_machine()),
- &local_err);
+ obj = icp_create(OBJECT(cpu), TYPE_PNV_ICP, &chip8->psi.ics,
+ XICS_FABRIC(qdev_get_machine()), &local_err);
if (local_err) {
error_propagate(errp, local_err);
return;
}
+ object_ref(obj);
pnv_cpu->intc = obj;
}
@@ -788,7 +790,10 @@ static void pnv_chip_power8_intc_reset(PnvChip *chip, PowerPCCPU *cpu)
static void pnv_chip_power8_intc_destroy(PnvChip *chip, PowerPCCPU *cpu)
{
- icp_destroy(ICP(pnv_cpu_state(cpu)->intc));
+ Object *intc = pnv_cpu_state(cpu)->intc;
+
+ object_unref(intc);
+ icp_destroy(ICP(intc));
}
/*
@@ -825,6 +830,7 @@ static void pnv_chip_power9_intc_create(PnvChip *chip, PowerPCCPU *cpu,
return;
}
+ object_ref(obj);
pnv_cpu->intc = obj;
}
@@ -837,7 +843,10 @@ static void pnv_chip_power9_intc_reset(PnvChip *chip, PowerPCCPU *cpu)
static void pnv_chip_power9_intc_destroy(PnvChip *chip, PowerPCCPU *cpu)
{
- xive_tctx_destroy(XIVE_TCTX(pnv_cpu_state(cpu)->intc));
+ Object *intc = pnv_cpu_state(cpu)->intc;
+
+ object_unref(intc);
+ xive_tctx_destroy(XIVE_TCTX(intc));
}
/*
diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
index 48a75aa4ab75..f4827e748fd8 100644
--- a/include/hw/ppc/xics.h
+++ b/include/hw/ppc/xics.h
@@ -179,7 +179,7 @@ void ics_pic_print_info(ICSState *ics, Monitor *mon);
void ics_resend(ICSState *ics);
void icp_resend(ICPState *ss);
-Object *icp_create(Object *cpu, const char *type, XICSFabric *xi,
+Object *icp_create(Object *cpu, const char *type, ICSState *ics, XICSFabric *xi,
Error **errp);
void icp_destroy(ICPState *icp);
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 4/6] qom: Add object_child_foreach_type() helper function
2019-10-23 14:51 [PATCH 0/6] ppc: Reparent the interrupt presenter Greg Kurz
` (2 preceding siblings ...)
2019-10-23 14:52 ` [PATCH 3/6] ppc: Reparent presenter objects to the interrupt controller object Greg Kurz
@ 2019-10-23 14:52 ` Greg Kurz
2019-10-24 2:59 ` David Gibson
2019-10-23 14:52 ` [PATCH 5/6] spapr: Don't use CPU_FOREACH() in 'info pic' Greg Kurz
2019-10-23 14:52 ` [PATCH 6/6] xive: Don't use CPU_FOREACH() to perform CAM line matching Greg Kurz
5 siblings, 1 reply; 22+ messages in thread
From: Greg Kurz @ 2019-10-23 14:52 UTC (permalink / raw)
To: David Gibson; +Cc: Paolo Bonzini, qemu-ppc, Cédric Le Goater, qemu-devel
Calling a function for children of a certain type is a recurring pattern
in the QEMU code base. In order to avoid the need to setup the same boiler
plate again and again, introduce a variant of object_child_foreach() that
only considers children of the given type.
Signed-off-by: Greg Kurz <groug@kaod.org>
---
include/qom/object.h | 35 +++++++++++++++++++++++++++++++++++
qom/object.c | 30 +++++++++++++++++++++++-------
2 files changed, 58 insertions(+), 7 deletions(-)
diff --git a/include/qom/object.h b/include/qom/object.h
index 128d00c77fd6..e9e3c2eae8ed 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -1728,6 +1728,41 @@ int object_child_foreach(Object *obj, int (*fn)(Object *child, void *opaque),
int object_child_foreach_recursive(Object *obj,
int (*fn)(Object *child, void *opaque),
void *opaque);
+
+/**
+ * object_child_foreach_type:
+ * @obj: the object whose children will be navigated
+ * @typename: the type of child objects to consider
+ * @fn: the iterator function to be called
+ * @opaque: an opaque value that will be passed to the iterator
+ *
+ * This is similar to object_child_foreach, but it only calls @fn for
+ * child objects of the give @typename.
+ *
+ * Returns: The last value returned by @fn, or 0 if there is no child of
+ * the given @typename.
+ */
+int object_child_foreach_type(Object *obj, const char *typename,
+ int (*fn)(Object *child, void *opaque),
+ void *opaque);
+
+/**
+ * object_child_foreach_recursive_type:
+ * @obj: the object whose children will be navigated
+ * @typename: the type of child objects to consider
+ * @fn: the iterator function to be called
+ * @opaque: an opaque value that will be passed to the iterator
+ *
+ * This is similar to object_child_foreach_recursive, but it only calls
+ * @fn for child objects of the give @typename.
+ *
+ * Returns: The last value returned by @fn, or 0 if there is no child of
+ * the given @typename.
+ */
+int object_child_foreach_recursive_type(Object *obj, const char *typename,
+ int (*fn)(Object *child, void *opaque),
+ void *opaque);
+
/**
* container_get:
* @root: root of the #path, e.g., object_get_root()
diff --git a/qom/object.c b/qom/object.c
index 6fa9c619fac4..a2dec1261ff7 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -986,7 +986,7 @@ void object_class_foreach(void (*fn)(ObjectClass *klass, void *opaque),
enumerating_types = false;
}
-static int do_object_child_foreach(Object *obj,
+static int do_object_child_foreach(Object *obj, const char *typename,
int (*fn)(Object *child, void *opaque),
void *opaque, bool recurse)
{
@@ -999,12 +999,14 @@ static int do_object_child_foreach(Object *obj,
if (object_property_is_child(prop)) {
Object *child = prop->opaque;
- ret = fn(child, opaque);
- if (ret != 0) {
- break;
+ if (!typename || object_dynamic_cast(child, typename)) {
+ ret = fn(child, opaque);
+ if (ret != 0) {
+ break;
+ }
}
if (recurse) {
- do_object_child_foreach(child, fn, opaque, true);
+ do_object_child_foreach(child, typename, fn, opaque, true);
}
}
}
@@ -1014,14 +1016,28 @@ static int do_object_child_foreach(Object *obj,
int object_child_foreach(Object *obj, int (*fn)(Object *child, void *opaque),
void *opaque)
{
- return do_object_child_foreach(obj, fn, opaque, false);
+ return do_object_child_foreach(obj, NULL, fn, opaque, false);
}
int object_child_foreach_recursive(Object *obj,
int (*fn)(Object *child, void *opaque),
void *opaque)
{
- return do_object_child_foreach(obj, fn, opaque, true);
+ return do_object_child_foreach(obj, NULL, fn, opaque, true);
+}
+
+int object_child_foreach_type(Object *obj, const char *typename,
+ int (*fn)(Object *child, void *opaque),
+ void *opaque)
+{
+ return do_object_child_foreach(obj, typename, fn, opaque, false);
+}
+
+int object_child_foreach_recursive_type(Object *obj, const char *typename,
+ int (*fn)(Object *child, void *opaque),
+ void *opaque)
+{
+ return do_object_child_foreach(obj, typename, fn, opaque, true);
}
static void object_class_get_list_tramp(ObjectClass *klass, void *opaque)
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 5/6] spapr: Don't use CPU_FOREACH() in 'info pic'
2019-10-23 14:51 [PATCH 0/6] ppc: Reparent the interrupt presenter Greg Kurz
` (3 preceding siblings ...)
2019-10-23 14:52 ` [PATCH 4/6] qom: Add object_child_foreach_type() helper function Greg Kurz
@ 2019-10-23 14:52 ` Greg Kurz
2019-10-24 3:02 ` David Gibson
2019-10-23 14:52 ` [PATCH 6/6] xive: Don't use CPU_FOREACH() to perform CAM line matching Greg Kurz
5 siblings, 1 reply; 22+ messages in thread
From: Greg Kurz @ 2019-10-23 14:52 UTC (permalink / raw)
To: David Gibson; +Cc: Paolo Bonzini, qemu-ppc, Cédric Le Goater, qemu-devel
Now that presenter objects are parented to the interrupt controller, stop
relying on CPU_FOREACH() which can race with CPU hotplug and crash QEMU.
Signed-off-by: Greg Kurz <groug@kaod.org>
---
hw/intc/spapr_xive.c | 8 +-------
hw/intc/xics.c | 12 ++++++++++++
hw/intc/xics_spapr.c | 8 +-------
hw/intc/xive.c | 12 ++++++++++++
include/hw/ppc/xics.h | 1 +
include/hw/ppc/xive.h | 2 ++
6 files changed, 29 insertions(+), 14 deletions(-)
diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
index d74ee71e76b4..05763a58cf5d 100644
--- a/hw/intc/spapr_xive.c
+++ b/hw/intc/spapr_xive.c
@@ -579,14 +579,8 @@ static void spapr_xive_set_irq(SpaprInterruptController *intc, int irq, int val)
static void spapr_xive_print_info(SpaprInterruptController *intc, Monitor *mon)
{
SpaprXive *xive = SPAPR_XIVE(intc);
- CPUState *cs;
-
- CPU_FOREACH(cs) {
- PowerPCCPU *cpu = POWERPC_CPU(cs);
-
- xive_tctx_pic_print_info(spapr_cpu_state(cpu)->tctx, mon);
- }
+ xive_presenter_print_info(XIVE_ROUTER(intc), mon);
spapr_xive_pic_print_info(xive, mon);
}
diff --git a/hw/intc/xics.c b/hw/intc/xics.c
index d5e4db668a4b..6e820c4851f3 100644
--- a/hw/intc/xics.c
+++ b/hw/intc/xics.c
@@ -88,6 +88,18 @@ void ics_pic_print_info(ICSState *ics, Monitor *mon)
}
}
+static int do_ics_pic_print_icp_infos(Object *child, void *opaque)
+{
+ icp_pic_print_info(ICP(child), opaque);
+ return 0;
+}
+
+void ics_pic_print_icp_infos(ICSState *ics, const char *type, Monitor *mon)
+{
+ object_child_foreach_type(OBJECT(ics), type, do_ics_pic_print_icp_infos,
+ mon);
+}
+
/*
* ICP: Presentation layer
*/
diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
index 080ed73aad64..7624d693c8da 100644
--- a/hw/intc/xics_spapr.c
+++ b/hw/intc/xics_spapr.c
@@ -400,14 +400,8 @@ static void xics_spapr_set_irq(SpaprInterruptController *intc, int irq, int val)
static void xics_spapr_print_info(SpaprInterruptController *intc, Monitor *mon)
{
ICSState *ics = ICS_SPAPR(intc);
- CPUState *cs;
-
- CPU_FOREACH(cs) {
- PowerPCCPU *cpu = POWERPC_CPU(cs);
-
- icp_pic_print_info(spapr_cpu_state(cpu)->icp, mon);
- }
+ ics_pic_print_icp_infos(ics, TYPE_ICP, mon);
ics_pic_print_info(ics, mon);
}
diff --git a/hw/intc/xive.c b/hw/intc/xive.c
index 8d2da4a11163..40ce43152456 100644
--- a/hw/intc/xive.c
+++ b/hw/intc/xive.c
@@ -547,6 +547,18 @@ void xive_tctx_pic_print_info(XiveTCTX *tctx, Monitor *mon)
}
}
+static int do_xive_presenter_print_info(Object *child, void *opaque)
+{
+ xive_tctx_pic_print_info(XIVE_TCTX(child), opaque);
+ return 0;
+}
+
+void xive_presenter_print_info(XiveRouter *xrtr, Monitor *mon)
+{
+ object_child_foreach_type(OBJECT(xrtr), TYPE_XIVE_TCTX,
+ do_xive_presenter_print_info, mon);
+}
+
void xive_tctx_reset(XiveTCTX *tctx)
{
memset(tctx->regs, 0, sizeof(tctx->regs));
diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
index f4827e748fd8..4de1f421c997 100644
--- a/include/hw/ppc/xics.h
+++ b/include/hw/ppc/xics.h
@@ -175,6 +175,7 @@ static inline bool ics_irq_free(ICSState *ics, uint32_t srcno)
void ics_set_irq_type(ICSState *ics, int srcno, bool lsi);
void icp_pic_print_info(ICPState *icp, Monitor *mon);
void ics_pic_print_info(ICSState *ics, Monitor *mon);
+void ics_pic_print_icp_infos(ICSState *ics, const char *type, Monitor *mon);
void ics_resend(ICSState *ics);
void icp_resend(ICPState *ss);
diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
index 8fd439ec9bba..14690428a0aa 100644
--- a/include/hw/ppc/xive.h
+++ b/include/hw/ppc/xive.h
@@ -367,6 +367,8 @@ int xive_router_write_nvt(XiveRouter *xrtr, uint8_t nvt_blk, uint32_t nvt_idx,
XiveTCTX *xive_router_get_tctx(XiveRouter *xrtr, CPUState *cs);
void xive_router_notify(XiveNotifier *xn, uint32_t lisn);
+void xive_presenter_print_info(XiveRouter *xrtr, Monitor *mon);
+
/*
* XIVE END ESBs
*/
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 6/6] xive: Don't use CPU_FOREACH() to perform CAM line matching
2019-10-23 14:51 [PATCH 0/6] ppc: Reparent the interrupt presenter Greg Kurz
` (4 preceding siblings ...)
2019-10-23 14:52 ` [PATCH 5/6] spapr: Don't use CPU_FOREACH() in 'info pic' Greg Kurz
@ 2019-10-23 14:52 ` Greg Kurz
2019-10-24 3:05 ` David Gibson
5 siblings, 1 reply; 22+ messages in thread
From: Greg Kurz @ 2019-10-23 14:52 UTC (permalink / raw)
To: David Gibson; +Cc: Paolo Bonzini, qemu-ppc, Cédric Le Goater, qemu-devel
Now that the TCTX objects are children of the XIVE router, stop
using CPU_FOREACH() when looking for a matching VCPU target.
Signed-off-by: Greg Kurz <groug@kaod.org>
---
hw/intc/xive.c | 100 +++++++++++++++++++++++++++++++++++---------------------
1 file changed, 62 insertions(+), 38 deletions(-)
diff --git a/hw/intc/xive.c b/hw/intc/xive.c
index 40ce43152456..ec5e7d0ee39a 100644
--- a/hw/intc/xive.c
+++ b/hw/intc/xive.c
@@ -1403,55 +1403,79 @@ typedef struct XiveTCTXMatch {
uint8_t ring;
} XiveTCTXMatch;
-static bool xive_presenter_match(XiveRouter *xrtr, uint8_t format,
- uint8_t nvt_blk, uint32_t nvt_idx,
- bool cam_ignore, uint8_t priority,
- uint32_t logic_serv, XiveTCTXMatch *match)
+typedef struct XivePresenterMatch {
+ uint8_t format;
+ uint8_t nvt_blk;
+ uint32_t nvt_idx;
+ bool cam_ignore;
+ uint8_t priority;
+ uint32_t logic_serv;
+ XiveTCTXMatch *match;
+ int count;
+} XivePresenterMatch;
+
+static int do_xive_presenter_match(Object *child, void *opaque)
{
- CPUState *cs;
+ XiveTCTX *tctx = XIVE_TCTX(child);
+ XivePresenterMatch *xpm = opaque;
+ int ring;
/*
* TODO (PowerNV): handle chip_id overwrite of block field for
* hardwired CAM compares
*/
- CPU_FOREACH(cs) {
- XiveTCTX *tctx = xive_router_get_tctx(xrtr, cs);
- int ring;
+ /*
+ * HW checks that the CPU is enabled in the Physical Thread
+ * Enable Register (PTER).
+ */
- /*
- * Skip partially initialized vCPUs. This can happen when
- * vCPUs are hotplugged.
- */
- if (!tctx) {
- continue;
+ /*
+ * Check the thread context CAM lines and record matches. We
+ * will handle CPU exception delivery later
+ */
+ ring = xive_presenter_tctx_match(tctx, xpm->format, xpm->nvt_blk,
+ xpm->nvt_idx, xpm->cam_ignore,
+ xpm->logic_serv);
+
+ /*
+ * Save the context and follow on to catch duplicates, that we
+ * don't support yet.
+ */
+ if (ring != -1) {
+ if (xpm->match->tctx) {
+ qemu_log_mask(LOG_GUEST_ERROR, "XIVE: already found a thread "
+ "context NVT %x/%x\n", xpm->nvt_blk, xpm->nvt_idx);
+ return -1;
}
- /*
- * HW checks that the CPU is enabled in the Physical Thread
- * Enable Register (PTER).
- */
+ xpm->match->ring = ring;
+ xpm->match->tctx = tctx;
+ xpm->count++;
+ }
- /*
- * Check the thread context CAM lines and record matches. We
- * will handle CPU exception delivery later
- */
- ring = xive_presenter_tctx_match(tctx, format, nvt_blk, nvt_idx,
- cam_ignore, logic_serv);
- /*
- * Save the context and follow on to catch duplicates, that we
- * don't support yet.
- */
- if (ring != -1) {
- if (match->tctx) {
- qemu_log_mask(LOG_GUEST_ERROR, "XIVE: already found a thread "
- "context NVT %x/%x\n", nvt_blk, nvt_idx);
- return false;
- }
-
- match->ring = ring;
- match->tctx = tctx;
- }
+ return 0;
+}
+
+static bool xive_presenter_match(XiveRouter *xrtr, uint8_t format,
+ uint8_t nvt_blk, uint32_t nvt_idx,
+ bool cam_ignore, uint8_t priority,
+ uint32_t logic_serv, XiveTCTXMatch *match)
+{
+ XivePresenterMatch xpm = {
+ .format = format,
+ .nvt_blk = nvt_blk,
+ .nvt_idx = nvt_idx,
+ .cam_ignore = cam_ignore,
+ .priority = priority,
+ .logic_serv = logic_serv,
+ .match = match,
+ .count = 0,
+ };
+
+ if (object_child_foreach_type(OBJECT(xrtr), TYPE_XIVE_TCTX,
+ do_xive_presenter_match, &xpm) < 0) {
+ return false;
}
if (!match->tctx) {
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH 1/6] ppc: Add intc_destroy() handlers to SpaprInterruptController/PnvChip
2019-10-23 14:51 ` [PATCH 1/6] ppc: Add intc_destroy() handlers to SpaprInterruptController/PnvChip Greg Kurz
@ 2019-10-24 2:50 ` David Gibson
2019-10-24 7:20 ` Greg Kurz
0 siblings, 1 reply; 22+ messages in thread
From: David Gibson @ 2019-10-24 2:50 UTC (permalink / raw)
To: Greg Kurz; +Cc: Paolo Bonzini, qemu-ppc, Cédric Le Goater, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 13600 bytes --]
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 <groug@kaod.org>
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/6] xive, xics: Fix reference counting on CPU objects
2019-10-23 14:52 ` [PATCH 2/6] xive, xics: Fix reference counting on CPU objects Greg Kurz
@ 2019-10-24 2:50 ` David Gibson
0 siblings, 0 replies; 22+ messages in thread
From: David Gibson @ 2019-10-24 2:50 UTC (permalink / raw)
To: Greg Kurz; +Cc: Paolo Bonzini, qemu-ppc, Cédric Le Goater, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 3312 bytes --]
On Wed, Oct 23, 2019 at 04:52:05PM +0200, Greg Kurz wrote:
> 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 <qom/object.h>:
>
> * 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 <groug@kaod.org>
LGTM
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> ---
> hw/intc/xics.c | 8 +++++++-
> hw/intc/xive.c | 6 +++++-
> 2 files changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index 935f325749cb..5f746079be46 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 38257aa02083..952a461d5329 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);
> }
>
> /*
>
--
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 3/6] ppc: Reparent presenter objects to the interrupt controller object
2019-10-23 14:52 ` [PATCH 3/6] ppc: Reparent presenter objects to the interrupt controller object Greg Kurz
@ 2019-10-24 2:58 ` David Gibson
2019-10-24 9:04 ` Greg Kurz
0 siblings, 1 reply; 22+ messages in thread
From: David Gibson @ 2019-10-24 2:58 UTC (permalink / raw)
To: Greg Kurz; +Cc: Paolo Bonzini, qemu-ppc, Cédric Le Goater, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 9052 bytes --]
On Wed, Oct 23, 2019 at 04:52:10PM +0200, Greg Kurz wrote:
> Each VCPU is associated to a presenter object within the interrupt
> controller, ie. TCTX for XIVE and ICP for XICS, but our current
> models put these objects below the VCPU, and we rely on CPU_FOREACH()
> to do anything that involves presenters.
>
> This recently bit us with the CAM line matching logic in XIVE because
> CPU_FOREACH() can race with CPU hotplug and we ended up considering a
> VCPU that wasn't associated to a TCTX object yet. Other users of
> CPU_FOREACH() are 'info pic' for both XICS and XIVE. It is again very
> easy to crash QEMU with concurrent VCPU hotplug/unplug and 'info pic'.
>
> Reparent the presenter objects to the corresponding interrupt controller
> object, ie. XIVE router or ICS, to make it clear they are not some extra
> data hanging from the CPU but internal XIVE or XICS entities. The CPU
> object now needs to explicitely take a reference on the presenter to
> ensure its pointer remains valid until unrealize time.
>
> This will allow to get rid of CPU_FOREACH() and ease further improvements
> to the XIVE model.
>
> This change doesn't impact section ids and is thus transparent to
> migration.
>
> Signed-off-by: Greg Kurz <groug@kaod.org>
Urgh. I see why we want something like this, but reparenting the
presenters to the source modules (particularly for XICS) makes me
uncomfortable.
AFAICT the association here is *purely* about managing lifetimes, not
because those ICPs inherently have something to do with those ICSes.
Same with XIVE, although since they'll be on the same chip there's a
bit more logic to it.
For spapr we kinda-sorta treat the (single) ICS or XiveRouter object
as the "master" of the interrupt controller. But that's not really
accurate to the hardware, so I don't really want to push that way of
looking at it any further than it already is.
If we could make the presenters children of the machine (spapr) or
chip (pnv) that might make more sense?
I'm also not totally convinced that having the presenter as a child of
the cpu is inherently bad. Yes we have bugs now, but maybe the right
fix *is* actually to do the NULL check on every CPU_FOREACH().
For comparison, the lapic on x86 machines is a child of the cpu, and I
believe they do have NULL checks on cpu->apic_state in various places
they use CPU_FOREACH().
> ---
> hw/intc/spapr_xive.c | 6 +++++-
> hw/intc/xics.c | 7 +++++--
> hw/intc/xics_spapr.c | 8 ++++++--
> hw/intc/xive.c | 4 +++-
> hw/ppc/pnv.c | 17 +++++++++++++----
> include/hw/ppc/xics.h | 2 +-
> 6 files changed, 33 insertions(+), 11 deletions(-)
>
> diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
> index b09cc48bcb61..d74ee71e76b4 100644
> --- a/hw/intc/spapr_xive.c
> +++ b/hw/intc/spapr_xive.c
> @@ -526,6 +526,7 @@ static int spapr_xive_cpu_intc_create(SpaprInterruptController *intc,
> return -1;
> }
>
> + object_ref(obj);
> spapr_cpu->tctx = XIVE_TCTX(obj);
> return 0;
> }
> @@ -558,7 +559,10 @@ static void spapr_xive_cpu_intc_reset(SpaprInterruptController *intc,
> static void spapr_xive_cpu_intc_destroy(SpaprInterruptController *intc,
> PowerPCCPU *cpu)
> {
> - xive_tctx_destroy(spapr_cpu_state(cpu)->tctx);
> + XiveTCTX *tctx = spapr_cpu_state(cpu)->tctx;
> +
> + object_unref(OBJECT(tctx));
> + xive_tctx_destroy(tctx);
> }
>
> static void spapr_xive_set_irq(SpaprInterruptController *intc, int irq, int val)
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index 5f746079be46..d5e4db668a4b 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -380,13 +380,16 @@ static const TypeInfo icp_info = {
> .class_size = sizeof(ICPStateClass),
> };
>
> -Object *icp_create(Object *cpu, const char *type, XICSFabric *xi, Error **errp)
> +Object *icp_create(Object *cpu, const char *type, ICSState *ics, XICSFabric *xi,
> + Error **errp)
> {
> Error *local_err = NULL;
> + g_autofree char *name = NULL;
> Object *obj;
>
> obj = object_new(type);
> - object_property_add_child(cpu, type, obj, &error_abort);
> + name = g_strdup_printf("%s[%d]", type, CPU(cpu)->cpu_index);
> + object_property_add_child(OBJECT(ics), name, obj, &error_abort);
> object_unref(obj);
> object_ref(OBJECT(xi));
> object_property_add_const_link(obj, ICP_PROP_XICS, OBJECT(xi),
> diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
> index 5977d1bdb73f..080ed73aad64 100644
> --- a/hw/intc/xics_spapr.c
> +++ b/hw/intc/xics_spapr.c
> @@ -337,11 +337,12 @@ static int xics_spapr_cpu_intc_create(SpaprInterruptController *intc,
> Object *obj;
> SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu);
>
> - obj = icp_create(OBJECT(cpu), TYPE_ICP, ics->xics, errp);
> + obj = icp_create(OBJECT(cpu), TYPE_ICP, ics, ics->xics, errp);
> if (!obj) {
> return -1;
> }
>
> + object_ref(obj);
> spapr_cpu->icp = ICP(obj);
> return 0;
> }
> @@ -355,7 +356,10 @@ static void xics_spapr_cpu_intc_reset(SpaprInterruptController *intc,
> static void xics_spapr_cpu_intc_destroy(SpaprInterruptController *intc,
> PowerPCCPU *cpu)
> {
> - icp_destroy(spapr_cpu_state(cpu)->icp);
> + ICPState *icp = spapr_cpu_state(cpu)->icp;
> +
> + object_unref(OBJECT(icp));
> + icp_destroy(icp);
> }
>
> static int xics_spapr_claim_irq(SpaprInterruptController *intc, int irq,
> diff --git a/hw/intc/xive.c b/hw/intc/xive.c
> index 952a461d5329..8d2da4a11163 100644
> --- a/hw/intc/xive.c
> +++ b/hw/intc/xive.c
> @@ -677,10 +677,12 @@ static const TypeInfo xive_tctx_info = {
> Object *xive_tctx_create(Object *cpu, XiveRouter *xrtr, Error **errp)
> {
> Error *local_err = NULL;
> + g_autofree char *name = NULL;
> Object *obj;
>
> obj = object_new(TYPE_XIVE_TCTX);
> - object_property_add_child(cpu, TYPE_XIVE_TCTX, obj, &error_abort);
> + name = g_strdup_printf(TYPE_XIVE_TCTX "[%d]", CPU(cpu)->cpu_index);
> + object_property_add_child(OBJECT(xrtr), name, obj, &error_abort);
> object_unref(obj);
> object_ref(cpu);
> object_property_add_const_link(obj, "cpu", cpu, &error_abort);
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index bd17c3536dd5..cbeabf98bff6 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -767,14 +767,16 @@ static void pnv_chip_power8_intc_create(PnvChip *chip, PowerPCCPU *cpu,
> Error *local_err = NULL;
> Object *obj;
> PnvCPUState *pnv_cpu = pnv_cpu_state(cpu);
> + Pnv8Chip *chip8 = PNV8_CHIP(chip);
>
> - obj = icp_create(OBJECT(cpu), TYPE_PNV_ICP, XICS_FABRIC(qdev_get_machine()),
> - &local_err);
> + obj = icp_create(OBJECT(cpu), TYPE_PNV_ICP, &chip8->psi.ics,
> + XICS_FABRIC(qdev_get_machine()), &local_err);
Here, the association of the ICPs with the PSI ICS is particularly arbitrary.
> if (local_err) {
> error_propagate(errp, local_err);
> return;
> }
>
> + object_ref(obj);
> pnv_cpu->intc = obj;
> }
>
> @@ -788,7 +790,10 @@ static void pnv_chip_power8_intc_reset(PnvChip *chip, PowerPCCPU *cpu)
>
> static void pnv_chip_power8_intc_destroy(PnvChip *chip, PowerPCCPU *cpu)
> {
> - icp_destroy(ICP(pnv_cpu_state(cpu)->intc));
> + Object *intc = pnv_cpu_state(cpu)->intc;
> +
> + object_unref(intc);
> + icp_destroy(ICP(intc));
> }
>
> /*
> @@ -825,6 +830,7 @@ static void pnv_chip_power9_intc_create(PnvChip *chip, PowerPCCPU *cpu,
> return;
> }
>
> + object_ref(obj);
> pnv_cpu->intc = obj;
> }
>
> @@ -837,7 +843,10 @@ static void pnv_chip_power9_intc_reset(PnvChip *chip, PowerPCCPU *cpu)
>
> static void pnv_chip_power9_intc_destroy(PnvChip *chip, PowerPCCPU *cpu)
> {
> - xive_tctx_destroy(XIVE_TCTX(pnv_cpu_state(cpu)->intc));
> + Object *intc = pnv_cpu_state(cpu)->intc;
> +
> + object_unref(intc);
> + xive_tctx_destroy(XIVE_TCTX(intc));
> }
>
> /*
> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> index 48a75aa4ab75..f4827e748fd8 100644
> --- a/include/hw/ppc/xics.h
> +++ b/include/hw/ppc/xics.h
> @@ -179,7 +179,7 @@ void ics_pic_print_info(ICSState *ics, Monitor *mon);
> void ics_resend(ICSState *ics);
> void icp_resend(ICPState *ss);
>
> -Object *icp_create(Object *cpu, const char *type, XICSFabric *xi,
> +Object *icp_create(Object *cpu, const char *type, ICSState *ics, XICSFabric *xi,
> Error **errp);
> void icp_destroy(ICPState *icp);
>
>
--
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 4/6] qom: Add object_child_foreach_type() helper function
2019-10-23 14:52 ` [PATCH 4/6] qom: Add object_child_foreach_type() helper function Greg Kurz
@ 2019-10-24 2:59 ` David Gibson
2019-10-24 3:07 ` David Gibson
0 siblings, 1 reply; 22+ messages in thread
From: David Gibson @ 2019-10-24 2:59 UTC (permalink / raw)
To: Greg Kurz; +Cc: Paolo Bonzini, qemu-ppc, Cédric Le Goater, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 5488 bytes --]
On Wed, Oct 23, 2019 at 04:52:16PM +0200, Greg Kurz wrote:
> Calling a function for children of a certain type is a recurring pattern
> in the QEMU code base. In order to avoid the need to setup the same boiler
> plate again and again, introduce a variant of object_child_foreach() that
> only considers children of the given type.
>
> Signed-off-by: Greg Kurz <groug@kaod.org>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> ---
> include/qom/object.h | 35 +++++++++++++++++++++++++++++++++++
> qom/object.c | 30 +++++++++++++++++++++++-------
> 2 files changed, 58 insertions(+), 7 deletions(-)
>
> diff --git a/include/qom/object.h b/include/qom/object.h
> index 128d00c77fd6..e9e3c2eae8ed 100644
> --- a/include/qom/object.h
> +++ b/include/qom/object.h
> @@ -1728,6 +1728,41 @@ int object_child_foreach(Object *obj, int (*fn)(Object *child, void *opaque),
> int object_child_foreach_recursive(Object *obj,
> int (*fn)(Object *child, void *opaque),
> void *opaque);
> +
> +/**
> + * object_child_foreach_type:
> + * @obj: the object whose children will be navigated
> + * @typename: the type of child objects to consider
> + * @fn: the iterator function to be called
> + * @opaque: an opaque value that will be passed to the iterator
> + *
> + * This is similar to object_child_foreach, but it only calls @fn for
> + * child objects of the give @typename.
> + *
> + * Returns: The last value returned by @fn, or 0 if there is no child of
> + * the given @typename.
> + */
> +int object_child_foreach_type(Object *obj, const char *typename,
> + int (*fn)(Object *child, void *opaque),
> + void *opaque);
> +
> +/**
> + * object_child_foreach_recursive_type:
> + * @obj: the object whose children will be navigated
> + * @typename: the type of child objects to consider
> + * @fn: the iterator function to be called
> + * @opaque: an opaque value that will be passed to the iterator
> + *
> + * This is similar to object_child_foreach_recursive, but it only calls
> + * @fn for child objects of the give @typename.
> + *
> + * Returns: The last value returned by @fn, or 0 if there is no child of
> + * the given @typename.
> + */
> +int object_child_foreach_recursive_type(Object *obj, const char *typename,
> + int (*fn)(Object *child, void *opaque),
> + void *opaque);
> +
> /**
> * container_get:
> * @root: root of the #path, e.g., object_get_root()
> diff --git a/qom/object.c b/qom/object.c
> index 6fa9c619fac4..a2dec1261ff7 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -986,7 +986,7 @@ void object_class_foreach(void (*fn)(ObjectClass *klass, void *opaque),
> enumerating_types = false;
> }
>
> -static int do_object_child_foreach(Object *obj,
> +static int do_object_child_foreach(Object *obj, const char *typename,
> int (*fn)(Object *child, void *opaque),
> void *opaque, bool recurse)
> {
> @@ -999,12 +999,14 @@ static int do_object_child_foreach(Object *obj,
> if (object_property_is_child(prop)) {
> Object *child = prop->opaque;
>
> - ret = fn(child, opaque);
> - if (ret != 0) {
> - break;
> + if (!typename || object_dynamic_cast(child, typename)) {
> + ret = fn(child, opaque);
> + if (ret != 0) {
> + break;
> + }
> }
> if (recurse) {
> - do_object_child_foreach(child, fn, opaque, true);
> + do_object_child_foreach(child, typename, fn, opaque, true);
> }
> }
> }
> @@ -1014,14 +1016,28 @@ static int do_object_child_foreach(Object *obj,
> int object_child_foreach(Object *obj, int (*fn)(Object *child, void *opaque),
> void *opaque)
> {
> - return do_object_child_foreach(obj, fn, opaque, false);
> + return do_object_child_foreach(obj, NULL, fn, opaque, false);
> }
>
> int object_child_foreach_recursive(Object *obj,
> int (*fn)(Object *child, void *opaque),
> void *opaque)
> {
> - return do_object_child_foreach(obj, fn, opaque, true);
> + return do_object_child_foreach(obj, NULL, fn, opaque, true);
> +}
> +
> +int object_child_foreach_type(Object *obj, const char *typename,
> + int (*fn)(Object *child, void *opaque),
> + void *opaque)
> +{
> + return do_object_child_foreach(obj, typename, fn, opaque, false);
> +}
> +
> +int object_child_foreach_recursive_type(Object *obj, const char *typename,
> + int (*fn)(Object *child, void *opaque),
> + void *opaque)
> +{
> + return do_object_child_foreach(obj, typename, fn, opaque, true);
> }
>
> static void object_class_get_list_tramp(ObjectClass *klass, void *opaque)
>
--
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 5/6] spapr: Don't use CPU_FOREACH() in 'info pic'
2019-10-23 14:52 ` [PATCH 5/6] spapr: Don't use CPU_FOREACH() in 'info pic' Greg Kurz
@ 2019-10-24 3:02 ` David Gibson
2019-10-24 9:28 ` Greg Kurz
0 siblings, 1 reply; 22+ messages in thread
From: David Gibson @ 2019-10-24 3:02 UTC (permalink / raw)
To: Greg Kurz; +Cc: Paolo Bonzini, qemu-ppc, Cédric Le Goater, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 5271 bytes --]
On Wed, Oct 23, 2019 at 04:52:21PM +0200, Greg Kurz wrote:
> Now that presenter objects are parented to the interrupt controller, stop
> relying on CPU_FOREACH() which can race with CPU hotplug and crash QEMU.
>
> Signed-off-by: Greg Kurz <groug@kaod.org>
So.. we might be able to go further than this. Having the
TYPE_INTERRUPT_STATS_PROVIDER interrupt on the machine is actually an
spapr and pnv oddity. In most cases that interface is on the various
components of the interrupt controller directly. hmp_info_irq() scans
the whole QOM tree looking for everything with the interface to
produce the info pic output.
It would be nice if we can do the same for xics and xive. The tricky
bit is that we do have the possibility of both, in which case the
individual components would need to know if they're currently "active"
and minimize their output if so.
> ---
> hw/intc/spapr_xive.c | 8 +-------
> hw/intc/xics.c | 12 ++++++++++++
> hw/intc/xics_spapr.c | 8 +-------
> hw/intc/xive.c | 12 ++++++++++++
> include/hw/ppc/xics.h | 1 +
> include/hw/ppc/xive.h | 2 ++
> 6 files changed, 29 insertions(+), 14 deletions(-)
>
> diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
> index d74ee71e76b4..05763a58cf5d 100644
> --- a/hw/intc/spapr_xive.c
> +++ b/hw/intc/spapr_xive.c
> @@ -579,14 +579,8 @@ static void spapr_xive_set_irq(SpaprInterruptController *intc, int irq, int val)
> static void spapr_xive_print_info(SpaprInterruptController *intc, Monitor *mon)
> {
> SpaprXive *xive = SPAPR_XIVE(intc);
> - CPUState *cs;
> -
> - CPU_FOREACH(cs) {
> - PowerPCCPU *cpu = POWERPC_CPU(cs);
> -
> - xive_tctx_pic_print_info(spapr_cpu_state(cpu)->tctx, mon);
> - }
>
> + xive_presenter_print_info(XIVE_ROUTER(intc), mon);
> spapr_xive_pic_print_info(xive, mon);
> }
>
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index d5e4db668a4b..6e820c4851f3 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -88,6 +88,18 @@ void ics_pic_print_info(ICSState *ics, Monitor *mon)
> }
> }
>
> +static int do_ics_pic_print_icp_infos(Object *child, void *opaque)
> +{
> + icp_pic_print_info(ICP(child), opaque);
> + return 0;
> +}
> +
> +void ics_pic_print_icp_infos(ICSState *ics, const char *type, Monitor *mon)
> +{
> + object_child_foreach_type(OBJECT(ics), type, do_ics_pic_print_icp_infos,
> + mon);
> +}
> +
> /*
> * ICP: Presentation layer
> */
> diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
> index 080ed73aad64..7624d693c8da 100644
> --- a/hw/intc/xics_spapr.c
> +++ b/hw/intc/xics_spapr.c
> @@ -400,14 +400,8 @@ static void xics_spapr_set_irq(SpaprInterruptController *intc, int irq, int val)
> static void xics_spapr_print_info(SpaprInterruptController *intc, Monitor *mon)
> {
> ICSState *ics = ICS_SPAPR(intc);
> - CPUState *cs;
> -
> - CPU_FOREACH(cs) {
> - PowerPCCPU *cpu = POWERPC_CPU(cs);
> -
> - icp_pic_print_info(spapr_cpu_state(cpu)->icp, mon);
> - }
>
> + ics_pic_print_icp_infos(ics, TYPE_ICP, mon);
> ics_pic_print_info(ics, mon);
> }
>
> diff --git a/hw/intc/xive.c b/hw/intc/xive.c
> index 8d2da4a11163..40ce43152456 100644
> --- a/hw/intc/xive.c
> +++ b/hw/intc/xive.c
> @@ -547,6 +547,18 @@ void xive_tctx_pic_print_info(XiveTCTX *tctx, Monitor *mon)
> }
> }
>
> +static int do_xive_presenter_print_info(Object *child, void *opaque)
> +{
> + xive_tctx_pic_print_info(XIVE_TCTX(child), opaque);
> + return 0;
> +}
> +
> +void xive_presenter_print_info(XiveRouter *xrtr, Monitor *mon)
> +{
> + object_child_foreach_type(OBJECT(xrtr), TYPE_XIVE_TCTX,
> + do_xive_presenter_print_info, mon);
> +}
> +
> void xive_tctx_reset(XiveTCTX *tctx)
> {
> memset(tctx->regs, 0, sizeof(tctx->regs));
> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> index f4827e748fd8..4de1f421c997 100644
> --- a/include/hw/ppc/xics.h
> +++ b/include/hw/ppc/xics.h
> @@ -175,6 +175,7 @@ static inline bool ics_irq_free(ICSState *ics, uint32_t srcno)
> void ics_set_irq_type(ICSState *ics, int srcno, bool lsi);
> void icp_pic_print_info(ICPState *icp, Monitor *mon);
> void ics_pic_print_info(ICSState *ics, Monitor *mon);
> +void ics_pic_print_icp_infos(ICSState *ics, const char *type, Monitor *mon);
>
> void ics_resend(ICSState *ics);
> void icp_resend(ICPState *ss);
> diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
> index 8fd439ec9bba..14690428a0aa 100644
> --- a/include/hw/ppc/xive.h
> +++ b/include/hw/ppc/xive.h
> @@ -367,6 +367,8 @@ int xive_router_write_nvt(XiveRouter *xrtr, uint8_t nvt_blk, uint32_t nvt_idx,
> XiveTCTX *xive_router_get_tctx(XiveRouter *xrtr, CPUState *cs);
> void xive_router_notify(XiveNotifier *xn, uint32_t lisn);
>
> +void xive_presenter_print_info(XiveRouter *xrtr, Monitor *mon);
> +
> /*
> * XIVE END ESBs
> */
>
--
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 6/6] xive: Don't use CPU_FOREACH() to perform CAM line matching
2019-10-23 14:52 ` [PATCH 6/6] xive: Don't use CPU_FOREACH() to perform CAM line matching Greg Kurz
@ 2019-10-24 3:05 ` David Gibson
2019-10-24 12:33 ` Greg Kurz
0 siblings, 1 reply; 22+ messages in thread
From: David Gibson @ 2019-10-24 3:05 UTC (permalink / raw)
To: Greg Kurz; +Cc: Paolo Bonzini, qemu-ppc, Cédric Le Goater, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 5169 bytes --]
On Wed, Oct 23, 2019 at 04:52:27PM +0200, Greg Kurz wrote:
> Now that the TCTX objects are children of the XIVE router, stop
> using CPU_FOREACH() when looking for a matching VCPU target.
>
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
> hw/intc/xive.c | 100 +++++++++++++++++++++++++++++++++++---------------------
> 1 file changed, 62 insertions(+), 38 deletions(-)
>
> diff --git a/hw/intc/xive.c b/hw/intc/xive.c
> index 40ce43152456..ec5e7d0ee39a 100644
> --- a/hw/intc/xive.c
> +++ b/hw/intc/xive.c
> @@ -1403,55 +1403,79 @@ typedef struct XiveTCTXMatch {
> uint8_t ring;
> } XiveTCTXMatch;
>
> -static bool xive_presenter_match(XiveRouter *xrtr, uint8_t format,
> - uint8_t nvt_blk, uint32_t nvt_idx,
> - bool cam_ignore, uint8_t priority,
> - uint32_t logic_serv, XiveTCTXMatch *match)
> +typedef struct XivePresenterMatch {
> + uint8_t format;
> + uint8_t nvt_blk;
> + uint32_t nvt_idx;
> + bool cam_ignore;
> + uint8_t priority;
> + uint32_t logic_serv;
> + XiveTCTXMatch *match;
> + int count;
> +} XivePresenterMatch;
> +
> +static int do_xive_presenter_match(Object *child, void *opaque)
> {
> - CPUState *cs;
> + XiveTCTX *tctx = XIVE_TCTX(child);
> + XivePresenterMatch *xpm = opaque;
> + int ring;
>
> /*
> * TODO (PowerNV): handle chip_id overwrite of block field for
> * hardwired CAM compares
> */
>
> - CPU_FOREACH(cs) {
> - XiveTCTX *tctx = xive_router_get_tctx(xrtr, cs);
> - int ring;
> + /*
> + * HW checks that the CPU is enabled in the Physical Thread
> + * Enable Register (PTER).
> + */
>
> - /*
> - * Skip partially initialized vCPUs. This can happen when
> - * vCPUs are hotplugged.
> - */
> - if (!tctx) {
> - continue;
> + /*
> + * Check the thread context CAM lines and record matches. We
> + * will handle CPU exception delivery later
> + */
> + ring = xive_presenter_tctx_match(tctx, xpm->format, xpm->nvt_blk,
> + xpm->nvt_idx, xpm->cam_ignore,
> + xpm->logic_serv);
> +
> + /*
> + * Save the context and follow on to catch duplicates, that we
> + * don't support yet.
> + */
> + if (ring != -1) {
> + if (xpm->match->tctx) {
> + qemu_log_mask(LOG_GUEST_ERROR, "XIVE: already found a thread "
> + "context NVT %x/%x\n", xpm->nvt_blk, xpm->nvt_idx);
> + return -1;
> }
>
> - /*
> - * HW checks that the CPU is enabled in the Physical Thread
> - * Enable Register (PTER).
> - */
> + xpm->match->ring = ring;
> + xpm->match->tctx = tctx;
> + xpm->count++;
> + }
>
> - /*
> - * Check the thread context CAM lines and record matches. We
> - * will handle CPU exception delivery later
> - */
> - ring = xive_presenter_tctx_match(tctx, format, nvt_blk, nvt_idx,
> - cam_ignore, logic_serv);
> - /*
> - * Save the context and follow on to catch duplicates, that we
> - * don't support yet.
> - */
> - if (ring != -1) {
> - if (match->tctx) {
> - qemu_log_mask(LOG_GUEST_ERROR, "XIVE: already found a thread "
> - "context NVT %x/%x\n", nvt_blk, nvt_idx);
> - return false;
> - }
> -
> - match->ring = ring;
> - match->tctx = tctx;
> - }
> + return 0;
> +}
> +
> +static bool xive_presenter_match(XiveRouter *xrtr, uint8_t format,
> + uint8_t nvt_blk, uint32_t nvt_idx,
> + bool cam_ignore, uint8_t priority,
> + uint32_t logic_serv, XiveTCTXMatch *match)
> +{
> + XivePresenterMatch xpm = {
> + .format = format,
> + .nvt_blk = nvt_blk,
> + .nvt_idx = nvt_idx,
> + .cam_ignore = cam_ignore,
> + .priority = priority,
> + .logic_serv = logic_serv,
> + .match = match,
> + .count = 0,
> + };
> +
> + if (object_child_foreach_type(OBJECT(xrtr), TYPE_XIVE_TCTX,
> + do_xive_presenter_match, &xpm) < 0) {
> + return false;
Hrm... xive_presenter_match() is potentially a pretty hot path, it's
called on every interrupt delivery - especially since we don't have a
usable KVM irq chip for Boston machines. I'm concerned that using
something as heavyweight as object_child_foreach() might have a
noticeable performance impact.
> }
>
> if (!match->tctx) {
>
--
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 4/6] qom: Add object_child_foreach_type() helper function
2019-10-24 2:59 ` David Gibson
@ 2019-10-24 3:07 ` David Gibson
2019-10-24 9:20 ` Greg Kurz
0 siblings, 1 reply; 22+ messages in thread
From: David Gibson @ 2019-10-24 3:07 UTC (permalink / raw)
To: Greg Kurz; +Cc: Paolo Bonzini, qemu-ppc, Cédric Le Goater, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 6126 bytes --]
On Thu, Oct 24, 2019 at 01:59:03PM +1100, David Gibson wrote:
> On Wed, Oct 23, 2019 at 04:52:16PM +0200, Greg Kurz wrote:
> > Calling a function for children of a certain type is a recurring pattern
> > in the QEMU code base. In order to avoid the need to setup the same boiler
> > plate again and again, introduce a variant of object_child_foreach() that
> > only considers children of the given type.
> >
> > Signed-off-by: Greg Kurz <groug@kaod.org>
>
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Actually.. a couuple of caveats on that.
Reading it again the name is potentially misleading it's "for each
object of given type" not "for each type" So maybe
"object_child_foreach_of_type()".
Also, having created these, using them to simplify hmp_info_irq() and
hmp_info_pic() seems like a good idea.
>
> > ---
> > include/qom/object.h | 35 +++++++++++++++++++++++++++++++++++
> > qom/object.c | 30 +++++++++++++++++++++++-------
> > 2 files changed, 58 insertions(+), 7 deletions(-)
> >
> > diff --git a/include/qom/object.h b/include/qom/object.h
> > index 128d00c77fd6..e9e3c2eae8ed 100644
> > --- a/include/qom/object.h
> > +++ b/include/qom/object.h
> > @@ -1728,6 +1728,41 @@ int object_child_foreach(Object *obj, int (*fn)(Object *child, void *opaque),
> > int object_child_foreach_recursive(Object *obj,
> > int (*fn)(Object *child, void *opaque),
> > void *opaque);
> > +
> > +/**
> > + * object_child_foreach_type:
> > + * @obj: the object whose children will be navigated
> > + * @typename: the type of child objects to consider
> > + * @fn: the iterator function to be called
> > + * @opaque: an opaque value that will be passed to the iterator
> > + *
> > + * This is similar to object_child_foreach, but it only calls @fn for
> > + * child objects of the give @typename.
> > + *
> > + * Returns: The last value returned by @fn, or 0 if there is no child of
> > + * the given @typename.
> > + */
> > +int object_child_foreach_type(Object *obj, const char *typename,
> > + int (*fn)(Object *child, void *opaque),
> > + void *opaque);
> > +
> > +/**
> > + * object_child_foreach_recursive_type:
> > + * @obj: the object whose children will be navigated
> > + * @typename: the type of child objects to consider
> > + * @fn: the iterator function to be called
> > + * @opaque: an opaque value that will be passed to the iterator
> > + *
> > + * This is similar to object_child_foreach_recursive, but it only calls
> > + * @fn for child objects of the give @typename.
> > + *
> > + * Returns: The last value returned by @fn, or 0 if there is no child of
> > + * the given @typename.
> > + */
> > +int object_child_foreach_recursive_type(Object *obj, const char *typename,
> > + int (*fn)(Object *child, void *opaque),
> > + void *opaque);
> > +
> > /**
> > * container_get:
> > * @root: root of the #path, e.g., object_get_root()
> > diff --git a/qom/object.c b/qom/object.c
> > index 6fa9c619fac4..a2dec1261ff7 100644
> > --- a/qom/object.c
> > +++ b/qom/object.c
> > @@ -986,7 +986,7 @@ void object_class_foreach(void (*fn)(ObjectClass *klass, void *opaque),
> > enumerating_types = false;
> > }
> >
> > -static int do_object_child_foreach(Object *obj,
> > +static int do_object_child_foreach(Object *obj, const char *typename,
> > int (*fn)(Object *child, void *opaque),
> > void *opaque, bool recurse)
> > {
> > @@ -999,12 +999,14 @@ static int do_object_child_foreach(Object *obj,
> > if (object_property_is_child(prop)) {
> > Object *child = prop->opaque;
> >
> > - ret = fn(child, opaque);
> > - if (ret != 0) {
> > - break;
> > + if (!typename || object_dynamic_cast(child, typename)) {
> > + ret = fn(child, opaque);
> > + if (ret != 0) {
> > + break;
> > + }
> > }
> > if (recurse) {
> > - do_object_child_foreach(child, fn, opaque, true);
> > + do_object_child_foreach(child, typename, fn, opaque, true);
> > }
> > }
> > }
> > @@ -1014,14 +1016,28 @@ static int do_object_child_foreach(Object *obj,
> > int object_child_foreach(Object *obj, int (*fn)(Object *child, void *opaque),
> > void *opaque)
> > {
> > - return do_object_child_foreach(obj, fn, opaque, false);
> > + return do_object_child_foreach(obj, NULL, fn, opaque, false);
> > }
> >
> > int object_child_foreach_recursive(Object *obj,
> > int (*fn)(Object *child, void *opaque),
> > void *opaque)
> > {
> > - return do_object_child_foreach(obj, fn, opaque, true);
> > + return do_object_child_foreach(obj, NULL, fn, opaque, true);
> > +}
> > +
> > +int object_child_foreach_type(Object *obj, const char *typename,
> > + int (*fn)(Object *child, void *opaque),
> > + void *opaque)
> > +{
> > + return do_object_child_foreach(obj, typename, fn, opaque, false);
> > +}
> > +
> > +int object_child_foreach_recursive_type(Object *obj, const char *typename,
> > + int (*fn)(Object *child, void *opaque),
> > + void *opaque)
> > +{
> > + return do_object_child_foreach(obj, typename, fn, opaque, true);
> > }
> >
> > static void object_class_get_list_tramp(ObjectClass *klass, void *opaque)
> >
>
--
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/6] ppc: Add intc_destroy() handlers to SpaprInterruptController/PnvChip
2019-10-24 2:50 ` David Gibson
@ 2019-10-24 7:20 ` Greg Kurz
0 siblings, 0 replies; 22+ messages in thread
From: Greg Kurz @ 2019-10-24 7:20 UTC (permalink / raw)
To: David Gibson; +Cc: Paolo Bonzini, qemu-ppc, Cédric Le Goater, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 14170 bytes --]
On Thu, 24 Oct 2019 13:50:25 +1100
David Gibson <david@gibson.dropbear.id.au> 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 <groug@kaod.org>
>
> 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)
> > {
> >
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 3/6] ppc: Reparent presenter objects to the interrupt controller object
2019-10-24 2:58 ` David Gibson
@ 2019-10-24 9:04 ` Greg Kurz
2019-10-27 16:57 ` David Gibson
0 siblings, 1 reply; 22+ messages in thread
From: Greg Kurz @ 2019-10-24 9:04 UTC (permalink / raw)
To: David Gibson; +Cc: Paolo Bonzini, qemu-ppc, Cédric Le Goater, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 10258 bytes --]
On Thu, 24 Oct 2019 13:58:41 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:
> On Wed, Oct 23, 2019 at 04:52:10PM +0200, Greg Kurz wrote:
> > Each VCPU is associated to a presenter object within the interrupt
> > controller, ie. TCTX for XIVE and ICP for XICS, but our current
> > models put these objects below the VCPU, and we rely on CPU_FOREACH()
> > to do anything that involves presenters.
> >
> > This recently bit us with the CAM line matching logic in XIVE because
> > CPU_FOREACH() can race with CPU hotplug and we ended up considering a
> > VCPU that wasn't associated to a TCTX object yet. Other users of
> > CPU_FOREACH() are 'info pic' for both XICS and XIVE. It is again very
> > easy to crash QEMU with concurrent VCPU hotplug/unplug and 'info pic'.
> >
> > Reparent the presenter objects to the corresponding interrupt controller
> > object, ie. XIVE router or ICS, to make it clear they are not some extra
> > data hanging from the CPU but internal XIVE or XICS entities. The CPU
> > object now needs to explicitely take a reference on the presenter to
> > ensure its pointer remains valid until unrealize time.
> >
> > This will allow to get rid of CPU_FOREACH() and ease further improvements
> > to the XIVE model.
> >
> > This change doesn't impact section ids and is thus transparent to
> > migration.
> >
> > Signed-off-by: Greg Kurz <groug@kaod.org>
>
>
> Urgh. I see why we want something like this, but reparenting the
> presenters to the source modules (particularly for XICS) makes me
> uncomfortable.
>
> AFAICT the association here is *purely* about managing lifetimes, not
> because those ICPs inherently have something to do with those ICSes.
> Same with XIVE, although since they'll be on the same chip there's a
> bit more logic to it.
>
I did it this way for XIVE because they are indeed on the same chip,
but I agree it is questionable for XICS.
> For spapr we kinda-sorta treat the (single) ICS or XiveRouter object
> as the "master" of the interrupt controller. But that's not really
Agreed for XICS. On the other side, the XIVE controller chip really has
a routing sub-engine (IVRE) and a presenter sub-engine (IVPE), and the
TCTXs do reside in an SRAM within the IVPE. The XiveRouter type might
be better named XiveController, and each instance to expose a XiveRouter
and a XivePresenter interface. We have one per chip for PNV and only a
single one for sPAPR.
> accurate to the hardware, so I don't really want to push that way of
> looking at it any further than it already is.
>
> If we could make the presenters children of the machine (spapr) or
> chip (pnv) that might make more sense?
>
It probably makes sense for XICS, not sure this makes things clearer
for XIVE.
> I'm also not totally convinced that having the presenter as a child of
> the cpu is inherently bad. Yes we have bugs now, but maybe the right
> fix *is* actually to do the NULL check on every CPU_FOREACH().
>
> For comparison, the lapic on x86 machines is a child of the cpu, and I
> believe they do have NULL checks on cpu->apic_state in various places
> they use CPU_FOREACH().
>
I didn't want to go that way because I was finding CPU_FOREACH() to
be fragile since all users are broken, but I can revisit that. Maybe
worth consolidating the logic in a PRESENTER_FOREACH() macro in order
to avoid future breakage with CPU_FOREACH() ?
> > ---
> > hw/intc/spapr_xive.c | 6 +++++-
> > hw/intc/xics.c | 7 +++++--
> > hw/intc/xics_spapr.c | 8 ++++++--
> > hw/intc/xive.c | 4 +++-
> > hw/ppc/pnv.c | 17 +++++++++++++----
> > include/hw/ppc/xics.h | 2 +-
> > 6 files changed, 33 insertions(+), 11 deletions(-)
> >
> > diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
> > index b09cc48bcb61..d74ee71e76b4 100644
> > --- a/hw/intc/spapr_xive.c
> > +++ b/hw/intc/spapr_xive.c
> > @@ -526,6 +526,7 @@ static int spapr_xive_cpu_intc_create(SpaprInterruptController *intc,
> > return -1;
> > }
> >
> > + object_ref(obj);
> > spapr_cpu->tctx = XIVE_TCTX(obj);
> > return 0;
> > }
> > @@ -558,7 +559,10 @@ static void spapr_xive_cpu_intc_reset(SpaprInterruptController *intc,
> > static void spapr_xive_cpu_intc_destroy(SpaprInterruptController *intc,
> > PowerPCCPU *cpu)
> > {
> > - xive_tctx_destroy(spapr_cpu_state(cpu)->tctx);
> > + XiveTCTX *tctx = spapr_cpu_state(cpu)->tctx;
> > +
> > + object_unref(OBJECT(tctx));
> > + xive_tctx_destroy(tctx);
> > }
> >
> > static void spapr_xive_set_irq(SpaprInterruptController *intc, int irq, int val)
> > diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> > index 5f746079be46..d5e4db668a4b 100644
> > --- a/hw/intc/xics.c
> > +++ b/hw/intc/xics.c
> > @@ -380,13 +380,16 @@ static const TypeInfo icp_info = {
> > .class_size = sizeof(ICPStateClass),
> > };
> >
> > -Object *icp_create(Object *cpu, const char *type, XICSFabric *xi, Error **errp)
> > +Object *icp_create(Object *cpu, const char *type, ICSState *ics, XICSFabric *xi,
> > + Error **errp)
> > {
> > Error *local_err = NULL;
> > + g_autofree char *name = NULL;
> > Object *obj;
> >
> > obj = object_new(type);
> > - object_property_add_child(cpu, type, obj, &error_abort);
> > + name = g_strdup_printf("%s[%d]", type, CPU(cpu)->cpu_index);
> > + object_property_add_child(OBJECT(ics), name, obj, &error_abort);
> > object_unref(obj);
> > object_ref(OBJECT(xi));
> > object_property_add_const_link(obj, ICP_PROP_XICS, OBJECT(xi),
> > diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
> > index 5977d1bdb73f..080ed73aad64 100644
> > --- a/hw/intc/xics_spapr.c
> > +++ b/hw/intc/xics_spapr.c
> > @@ -337,11 +337,12 @@ static int xics_spapr_cpu_intc_create(SpaprInterruptController *intc,
> > Object *obj;
> > SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu);
> >
> > - obj = icp_create(OBJECT(cpu), TYPE_ICP, ics->xics, errp);
> > + obj = icp_create(OBJECT(cpu), TYPE_ICP, ics, ics->xics, errp);
> > if (!obj) {
> > return -1;
> > }
> >
> > + object_ref(obj);
> > spapr_cpu->icp = ICP(obj);
> > return 0;
> > }
> > @@ -355,7 +356,10 @@ static void xics_spapr_cpu_intc_reset(SpaprInterruptController *intc,
> > static void xics_spapr_cpu_intc_destroy(SpaprInterruptController *intc,
> > PowerPCCPU *cpu)
> > {
> > - icp_destroy(spapr_cpu_state(cpu)->icp);
> > + ICPState *icp = spapr_cpu_state(cpu)->icp;
> > +
> > + object_unref(OBJECT(icp));
> > + icp_destroy(icp);
> > }
> >
> > static int xics_spapr_claim_irq(SpaprInterruptController *intc, int irq,
> > diff --git a/hw/intc/xive.c b/hw/intc/xive.c
> > index 952a461d5329..8d2da4a11163 100644
> > --- a/hw/intc/xive.c
> > +++ b/hw/intc/xive.c
> > @@ -677,10 +677,12 @@ static const TypeInfo xive_tctx_info = {
> > Object *xive_tctx_create(Object *cpu, XiveRouter *xrtr, Error **errp)
> > {
> > Error *local_err = NULL;
> > + g_autofree char *name = NULL;
> > Object *obj;
> >
> > obj = object_new(TYPE_XIVE_TCTX);
> > - object_property_add_child(cpu, TYPE_XIVE_TCTX, obj, &error_abort);
> > + name = g_strdup_printf(TYPE_XIVE_TCTX "[%d]", CPU(cpu)->cpu_index);
> > + object_property_add_child(OBJECT(xrtr), name, obj, &error_abort);
> > object_unref(obj);
> > object_ref(cpu);
> > object_property_add_const_link(obj, "cpu", cpu, &error_abort);
> > diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> > index bd17c3536dd5..cbeabf98bff6 100644
> > --- a/hw/ppc/pnv.c
> > +++ b/hw/ppc/pnv.c
> > @@ -767,14 +767,16 @@ static void pnv_chip_power8_intc_create(PnvChip *chip, PowerPCCPU *cpu,
> > Error *local_err = NULL;
> > Object *obj;
> > PnvCPUState *pnv_cpu = pnv_cpu_state(cpu);
> > + Pnv8Chip *chip8 = PNV8_CHIP(chip);
> >
> > - obj = icp_create(OBJECT(cpu), TYPE_PNV_ICP, XICS_FABRIC(qdev_get_machine()),
> > - &local_err);
> > + obj = icp_create(OBJECT(cpu), TYPE_PNV_ICP, &chip8->psi.ics,
> > + XICS_FABRIC(qdev_get_machine()), &local_err);
>
> Here, the association of the ICPs with the PSI ICS is particularly arbitrary.
>
> > if (local_err) {
> > error_propagate(errp, local_err);
> > return;
> > }
> >
> > + object_ref(obj);
> > pnv_cpu->intc = obj;
> > }
> >
> > @@ -788,7 +790,10 @@ static void pnv_chip_power8_intc_reset(PnvChip *chip, PowerPCCPU *cpu)
> >
> > static void pnv_chip_power8_intc_destroy(PnvChip *chip, PowerPCCPU *cpu)
> > {
> > - icp_destroy(ICP(pnv_cpu_state(cpu)->intc));
> > + Object *intc = pnv_cpu_state(cpu)->intc;
> > +
> > + object_unref(intc);
> > + icp_destroy(ICP(intc));
> > }
> >
> > /*
> > @@ -825,6 +830,7 @@ static void pnv_chip_power9_intc_create(PnvChip *chip, PowerPCCPU *cpu,
> > return;
> > }
> >
> > + object_ref(obj);
> > pnv_cpu->intc = obj;
> > }
> >
> > @@ -837,7 +843,10 @@ static void pnv_chip_power9_intc_reset(PnvChip *chip, PowerPCCPU *cpu)
> >
> > static void pnv_chip_power9_intc_destroy(PnvChip *chip, PowerPCCPU *cpu)
> > {
> > - xive_tctx_destroy(XIVE_TCTX(pnv_cpu_state(cpu)->intc));
> > + Object *intc = pnv_cpu_state(cpu)->intc;
> > +
> > + object_unref(intc);
> > + xive_tctx_destroy(XIVE_TCTX(intc));
> > }
> >
> > /*
> > diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> > index 48a75aa4ab75..f4827e748fd8 100644
> > --- a/include/hw/ppc/xics.h
> > +++ b/include/hw/ppc/xics.h
> > @@ -179,7 +179,7 @@ void ics_pic_print_info(ICSState *ics, Monitor *mon);
> > void ics_resend(ICSState *ics);
> > void icp_resend(ICPState *ss);
> >
> > -Object *icp_create(Object *cpu, const char *type, XICSFabric *xi,
> > +Object *icp_create(Object *cpu, const char *type, ICSState *ics, XICSFabric *xi,
> > Error **errp);
> > void icp_destroy(ICPState *icp);
> >
> >
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 4/6] qom: Add object_child_foreach_type() helper function
2019-10-24 3:07 ` David Gibson
@ 2019-10-24 9:20 ` Greg Kurz
0 siblings, 0 replies; 22+ messages in thread
From: Greg Kurz @ 2019-10-24 9:20 UTC (permalink / raw)
To: David Gibson; +Cc: Paolo Bonzini, qemu-ppc, Cédric Le Goater, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 6321 bytes --]
On Thu, 24 Oct 2019 14:07:15 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:
> On Thu, Oct 24, 2019 at 01:59:03PM +1100, David Gibson wrote:
> > On Wed, Oct 23, 2019 at 04:52:16PM +0200, Greg Kurz wrote:
> > > Calling a function for children of a certain type is a recurring pattern
> > > in the QEMU code base. In order to avoid the need to setup the same boiler
> > > plate again and again, introduce a variant of object_child_foreach() that
> > > only considers children of the given type.
> > >
> > > Signed-off-by: Greg Kurz <groug@kaod.org>
> >
> > Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
>
> Actually.. a couuple of caveats on that.
>
> Reading it again the name is potentially misleading it's "for each
> object of given type" not "for each type" So maybe
> "object_child_foreach_of_type()".
>
Makes sense.
> Also, having created these, using them to simplify hmp_info_irq() and
> hmp_info_pic() seems like a good idea.
>
> >
> > > ---
> > > include/qom/object.h | 35 +++++++++++++++++++++++++++++++++++
> > > qom/object.c | 30 +++++++++++++++++++++++-------
> > > 2 files changed, 58 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/include/qom/object.h b/include/qom/object.h
> > > index 128d00c77fd6..e9e3c2eae8ed 100644
> > > --- a/include/qom/object.h
> > > +++ b/include/qom/object.h
> > > @@ -1728,6 +1728,41 @@ int object_child_foreach(Object *obj, int (*fn)(Object *child, void *opaque),
> > > int object_child_foreach_recursive(Object *obj,
> > > int (*fn)(Object *child, void *opaque),
> > > void *opaque);
> > > +
> > > +/**
> > > + * object_child_foreach_type:
> > > + * @obj: the object whose children will be navigated
> > > + * @typename: the type of child objects to consider
> > > + * @fn: the iterator function to be called
> > > + * @opaque: an opaque value that will be passed to the iterator
> > > + *
> > > + * This is similar to object_child_foreach, but it only calls @fn for
> > > + * child objects of the give @typename.
> > > + *
> > > + * Returns: The last value returned by @fn, or 0 if there is no child of
> > > + * the given @typename.
> > > + */
> > > +int object_child_foreach_type(Object *obj, const char *typename,
> > > + int (*fn)(Object *child, void *opaque),
> > > + void *opaque);
> > > +
> > > +/**
> > > + * object_child_foreach_recursive_type:
> > > + * @obj: the object whose children will be navigated
> > > + * @typename: the type of child objects to consider
> > > + * @fn: the iterator function to be called
> > > + * @opaque: an opaque value that will be passed to the iterator
> > > + *
> > > + * This is similar to object_child_foreach_recursive, but it only calls
> > > + * @fn for child objects of the give @typename.
> > > + *
> > > + * Returns: The last value returned by @fn, or 0 if there is no child of
> > > + * the given @typename.
> > > + */
> > > +int object_child_foreach_recursive_type(Object *obj, const char *typename,
> > > + int (*fn)(Object *child, void *opaque),
> > > + void *opaque);
> > > +
> > > /**
> > > * container_get:
> > > * @root: root of the #path, e.g., object_get_root()
> > > diff --git a/qom/object.c b/qom/object.c
> > > index 6fa9c619fac4..a2dec1261ff7 100644
> > > --- a/qom/object.c
> > > +++ b/qom/object.c
> > > @@ -986,7 +986,7 @@ void object_class_foreach(void (*fn)(ObjectClass *klass, void *opaque),
> > > enumerating_types = false;
> > > }
> > >
> > > -static int do_object_child_foreach(Object *obj,
> > > +static int do_object_child_foreach(Object *obj, const char *typename,
> > > int (*fn)(Object *child, void *opaque),
> > > void *opaque, bool recurse)
> > > {
> > > @@ -999,12 +999,14 @@ static int do_object_child_foreach(Object *obj,
> > > if (object_property_is_child(prop)) {
> > > Object *child = prop->opaque;
> > >
> > > - ret = fn(child, opaque);
> > > - if (ret != 0) {
> > > - break;
> > > + if (!typename || object_dynamic_cast(child, typename)) {
> > > + ret = fn(child, opaque);
> > > + if (ret != 0) {
> > > + break;
> > > + }
> > > }
> > > if (recurse) {
> > > - do_object_child_foreach(child, fn, opaque, true);
> > > + do_object_child_foreach(child, typename, fn, opaque, true);
> > > }
> > > }
> > > }
> > > @@ -1014,14 +1016,28 @@ static int do_object_child_foreach(Object *obj,
> > > int object_child_foreach(Object *obj, int (*fn)(Object *child, void *opaque),
> > > void *opaque)
> > > {
> > > - return do_object_child_foreach(obj, fn, opaque, false);
> > > + return do_object_child_foreach(obj, NULL, fn, opaque, false);
> > > }
> > >
> > > int object_child_foreach_recursive(Object *obj,
> > > int (*fn)(Object *child, void *opaque),
> > > void *opaque)
> > > {
> > > - return do_object_child_foreach(obj, fn, opaque, true);
> > > + return do_object_child_foreach(obj, NULL, fn, opaque, true);
> > > +}
> > > +
> > > +int object_child_foreach_type(Object *obj, const char *typename,
> > > + int (*fn)(Object *child, void *opaque),
> > > + void *opaque)
> > > +{
> > > + return do_object_child_foreach(obj, typename, fn, opaque, false);
> > > +}
> > > +
> > > +int object_child_foreach_recursive_type(Object *obj, const char *typename,
> > > + int (*fn)(Object *child, void *opaque),
> > > + void *opaque)
> > > +{
> > > + return do_object_child_foreach(obj, typename, fn, opaque, true);
> > > }
> > >
> > > static void object_class_get_list_tramp(ObjectClass *klass, void *opaque)
> > >
> >
>
>
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 5/6] spapr: Don't use CPU_FOREACH() in 'info pic'
2019-10-24 3:02 ` David Gibson
@ 2019-10-24 9:28 ` Greg Kurz
2019-10-27 17:01 ` David Gibson
0 siblings, 1 reply; 22+ messages in thread
From: Greg Kurz @ 2019-10-24 9:28 UTC (permalink / raw)
To: David Gibson; +Cc: Paolo Bonzini, qemu-ppc, Cédric Le Goater, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 5603 bytes --]
On Thu, 24 Oct 2019 14:02:31 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:
> On Wed, Oct 23, 2019 at 04:52:21PM +0200, Greg Kurz wrote:
> > Now that presenter objects are parented to the interrupt controller, stop
> > relying on CPU_FOREACH() which can race with CPU hotplug and crash QEMU.
> >
> > Signed-off-by: Greg Kurz <groug@kaod.org>
>
> So.. we might be able to go further than this. Having the
> TYPE_INTERRUPT_STATS_PROVIDER interrupt on the machine is actually an
> spapr and pnv oddity. In most cases that interface is on the various
> components of the interrupt controller directly. hmp_info_irq() scans
> the whole QOM tree looking for everything with the interface to
> produce the info pic output.
>
> It would be nice if we can do the same for xics and xive. The tricky
> bit is that we do have the possibility of both, in which case the
> individual components would need to know if they're currently "active"
> and minimize their output if so.
>
Yes but this looks like 4.3 material. If we want to fix this for 4.2,
I'm now thinking it might be safer to keep CPU_FOREACH() and check the
state.
> > ---
> > hw/intc/spapr_xive.c | 8 +-------
> > hw/intc/xics.c | 12 ++++++++++++
> > hw/intc/xics_spapr.c | 8 +-------
> > hw/intc/xive.c | 12 ++++++++++++
> > include/hw/ppc/xics.h | 1 +
> > include/hw/ppc/xive.h | 2 ++
> > 6 files changed, 29 insertions(+), 14 deletions(-)
> >
> > diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
> > index d74ee71e76b4..05763a58cf5d 100644
> > --- a/hw/intc/spapr_xive.c
> > +++ b/hw/intc/spapr_xive.c
> > @@ -579,14 +579,8 @@ static void spapr_xive_set_irq(SpaprInterruptController *intc, int irq, int val)
> > static void spapr_xive_print_info(SpaprInterruptController *intc, Monitor *mon)
> > {
> > SpaprXive *xive = SPAPR_XIVE(intc);
> > - CPUState *cs;
> > -
> > - CPU_FOREACH(cs) {
> > - PowerPCCPU *cpu = POWERPC_CPU(cs);
> > -
> > - xive_tctx_pic_print_info(spapr_cpu_state(cpu)->tctx, mon);
> > - }
> >
> > + xive_presenter_print_info(XIVE_ROUTER(intc), mon);
> > spapr_xive_pic_print_info(xive, mon);
> > }
> >
> > diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> > index d5e4db668a4b..6e820c4851f3 100644
> > --- a/hw/intc/xics.c
> > +++ b/hw/intc/xics.c
> > @@ -88,6 +88,18 @@ void ics_pic_print_info(ICSState *ics, Monitor *mon)
> > }
> > }
> >
> > +static int do_ics_pic_print_icp_infos(Object *child, void *opaque)
> > +{
> > + icp_pic_print_info(ICP(child), opaque);
> > + return 0;
> > +}
> > +
> > +void ics_pic_print_icp_infos(ICSState *ics, const char *type, Monitor *mon)
> > +{
> > + object_child_foreach_type(OBJECT(ics), type, do_ics_pic_print_icp_infos,
> > + mon);
> > +}
> > +
> > /*
> > * ICP: Presentation layer
> > */
> > diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
> > index 080ed73aad64..7624d693c8da 100644
> > --- a/hw/intc/xics_spapr.c
> > +++ b/hw/intc/xics_spapr.c
> > @@ -400,14 +400,8 @@ static void xics_spapr_set_irq(SpaprInterruptController *intc, int irq, int val)
> > static void xics_spapr_print_info(SpaprInterruptController *intc, Monitor *mon)
> > {
> > ICSState *ics = ICS_SPAPR(intc);
> > - CPUState *cs;
> > -
> > - CPU_FOREACH(cs) {
> > - PowerPCCPU *cpu = POWERPC_CPU(cs);
> > -
> > - icp_pic_print_info(spapr_cpu_state(cpu)->icp, mon);
> > - }
> >
> > + ics_pic_print_icp_infos(ics, TYPE_ICP, mon);
> > ics_pic_print_info(ics, mon);
> > }
> >
> > diff --git a/hw/intc/xive.c b/hw/intc/xive.c
> > index 8d2da4a11163..40ce43152456 100644
> > --- a/hw/intc/xive.c
> > +++ b/hw/intc/xive.c
> > @@ -547,6 +547,18 @@ void xive_tctx_pic_print_info(XiveTCTX *tctx, Monitor *mon)
> > }
> > }
> >
> > +static int do_xive_presenter_print_info(Object *child, void *opaque)
> > +{
> > + xive_tctx_pic_print_info(XIVE_TCTX(child), opaque);
> > + return 0;
> > +}
> > +
> > +void xive_presenter_print_info(XiveRouter *xrtr, Monitor *mon)
> > +{
> > + object_child_foreach_type(OBJECT(xrtr), TYPE_XIVE_TCTX,
> > + do_xive_presenter_print_info, mon);
> > +}
> > +
> > void xive_tctx_reset(XiveTCTX *tctx)
> > {
> > memset(tctx->regs, 0, sizeof(tctx->regs));
> > diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> > index f4827e748fd8..4de1f421c997 100644
> > --- a/include/hw/ppc/xics.h
> > +++ b/include/hw/ppc/xics.h
> > @@ -175,6 +175,7 @@ static inline bool ics_irq_free(ICSState *ics, uint32_t srcno)
> > void ics_set_irq_type(ICSState *ics, int srcno, bool lsi);
> > void icp_pic_print_info(ICPState *icp, Monitor *mon);
> > void ics_pic_print_info(ICSState *ics, Monitor *mon);
> > +void ics_pic_print_icp_infos(ICSState *ics, const char *type, Monitor *mon);
> >
> > void ics_resend(ICSState *ics);
> > void icp_resend(ICPState *ss);
> > diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
> > index 8fd439ec9bba..14690428a0aa 100644
> > --- a/include/hw/ppc/xive.h
> > +++ b/include/hw/ppc/xive.h
> > @@ -367,6 +367,8 @@ int xive_router_write_nvt(XiveRouter *xrtr, uint8_t nvt_blk, uint32_t nvt_idx,
> > XiveTCTX *xive_router_get_tctx(XiveRouter *xrtr, CPUState *cs);
> > void xive_router_notify(XiveNotifier *xn, uint32_t lisn);
> >
> > +void xive_presenter_print_info(XiveRouter *xrtr, Monitor *mon);
> > +
> > /*
> > * XIVE END ESBs
> > */
> >
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 6/6] xive: Don't use CPU_FOREACH() to perform CAM line matching
2019-10-24 3:05 ` David Gibson
@ 2019-10-24 12:33 ` Greg Kurz
2019-10-27 17:03 ` David Gibson
0 siblings, 1 reply; 22+ messages in thread
From: Greg Kurz @ 2019-10-24 12:33 UTC (permalink / raw)
To: David Gibson; +Cc: Paolo Bonzini, qemu-ppc, Cédric Le Goater, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 5913 bytes --]
On Thu, 24 Oct 2019 14:05:36 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:
> On Wed, Oct 23, 2019 at 04:52:27PM +0200, Greg Kurz wrote:
> > Now that the TCTX objects are children of the XIVE router, stop
> > using CPU_FOREACH() when looking for a matching VCPU target.
> >
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> > hw/intc/xive.c | 100 +++++++++++++++++++++++++++++++++++---------------------
> > 1 file changed, 62 insertions(+), 38 deletions(-)
> >
> > diff --git a/hw/intc/xive.c b/hw/intc/xive.c
> > index 40ce43152456..ec5e7d0ee39a 100644
> > --- a/hw/intc/xive.c
> > +++ b/hw/intc/xive.c
> > @@ -1403,55 +1403,79 @@ typedef struct XiveTCTXMatch {
> > uint8_t ring;
> > } XiveTCTXMatch;
> >
> > -static bool xive_presenter_match(XiveRouter *xrtr, uint8_t format,
> > - uint8_t nvt_blk, uint32_t nvt_idx,
> > - bool cam_ignore, uint8_t priority,
> > - uint32_t logic_serv, XiveTCTXMatch *match)
> > +typedef struct XivePresenterMatch {
> > + uint8_t format;
> > + uint8_t nvt_blk;
> > + uint32_t nvt_idx;
> > + bool cam_ignore;
> > + uint8_t priority;
> > + uint32_t logic_serv;
> > + XiveTCTXMatch *match;
> > + int count;
> > +} XivePresenterMatch;
> > +
> > +static int do_xive_presenter_match(Object *child, void *opaque)
> > {
> > - CPUState *cs;
> > + XiveTCTX *tctx = XIVE_TCTX(child);
> > + XivePresenterMatch *xpm = opaque;
> > + int ring;
> >
> > /*
> > * TODO (PowerNV): handle chip_id overwrite of block field for
> > * hardwired CAM compares
> > */
> >
> > - CPU_FOREACH(cs) {
> > - XiveTCTX *tctx = xive_router_get_tctx(xrtr, cs);
> > - int ring;
> > + /*
> > + * HW checks that the CPU is enabled in the Physical Thread
> > + * Enable Register (PTER).
> > + */
> >
> > - /*
> > - * Skip partially initialized vCPUs. This can happen when
> > - * vCPUs are hotplugged.
> > - */
> > - if (!tctx) {
> > - continue;
> > + /*
> > + * Check the thread context CAM lines and record matches. We
> > + * will handle CPU exception delivery later
> > + */
> > + ring = xive_presenter_tctx_match(tctx, xpm->format, xpm->nvt_blk,
> > + xpm->nvt_idx, xpm->cam_ignore,
> > + xpm->logic_serv);
> > +
> > + /*
> > + * Save the context and follow on to catch duplicates, that we
> > + * don't support yet.
> > + */
> > + if (ring != -1) {
> > + if (xpm->match->tctx) {
> > + qemu_log_mask(LOG_GUEST_ERROR, "XIVE: already found a thread "
> > + "context NVT %x/%x\n", xpm->nvt_blk, xpm->nvt_idx);
> > + return -1;
> > }
> >
> > - /*
> > - * HW checks that the CPU is enabled in the Physical Thread
> > - * Enable Register (PTER).
> > - */
> > + xpm->match->ring = ring;
> > + xpm->match->tctx = tctx;
> > + xpm->count++;
> > + }
> >
> > - /*
> > - * Check the thread context CAM lines and record matches. We
> > - * will handle CPU exception delivery later
> > - */
> > - ring = xive_presenter_tctx_match(tctx, format, nvt_blk, nvt_idx,
> > - cam_ignore, logic_serv);
> > - /*
> > - * Save the context and follow on to catch duplicates, that we
> > - * don't support yet.
> > - */
> > - if (ring != -1) {
> > - if (match->tctx) {
> > - qemu_log_mask(LOG_GUEST_ERROR, "XIVE: already found a thread "
> > - "context NVT %x/%x\n", nvt_blk, nvt_idx);
> > - return false;
> > - }
> > -
> > - match->ring = ring;
> > - match->tctx = tctx;
> > - }
> > + return 0;
> > +}
> > +
> > +static bool xive_presenter_match(XiveRouter *xrtr, uint8_t format,
> > + uint8_t nvt_blk, uint32_t nvt_idx,
> > + bool cam_ignore, uint8_t priority,
> > + uint32_t logic_serv, XiveTCTXMatch *match)
> > +{
> > + XivePresenterMatch xpm = {
> > + .format = format,
> > + .nvt_blk = nvt_blk,
> > + .nvt_idx = nvt_idx,
> > + .cam_ignore = cam_ignore,
> > + .priority = priority,
> > + .logic_serv = logic_serv,
> > + .match = match,
> > + .count = 0,
> > + };
> > +
> > + if (object_child_foreach_type(OBJECT(xrtr), TYPE_XIVE_TCTX,
> > + do_xive_presenter_match, &xpm) < 0) {
> > + return false;
>
> Hrm... xive_presenter_match() is potentially a pretty hot path, it's
> called on every interrupt delivery - especially since we don't have a
> usable KVM irq chip for Boston machines. I'm concerned that using
> something as heavyweight as object_child_foreach() might have a
> noticeable performance impact.
>
Well, the XiveRouter _only_ has 3 extra children (XiveSource,
XiveENDSource and TIMA) but indeed object_child_foreach() might
cost more than QTAILQ_FOREACH_RCU(). A possible option could be
to have a QTAILQ of presenters under the machine for sPAPR or
under the chip for PNV, in order to avoid the need to filter out
VCPUs that we don't want to consider, ie. partly realized with
sPAPR and from another chip with PNV.
But as said in another mail, the safer for 4.2 is probably to
fix the CPU_FOREACH() users, which is already the case here for
sPAPR.
> > }
> >
> > if (!match->tctx) {
> >
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 3/6] ppc: Reparent presenter objects to the interrupt controller object
2019-10-24 9:04 ` Greg Kurz
@ 2019-10-27 16:57 ` David Gibson
0 siblings, 0 replies; 22+ messages in thread
From: David Gibson @ 2019-10-27 16:57 UTC (permalink / raw)
To: Greg Kurz; +Cc: Paolo Bonzini, qemu-ppc, Cédric Le Goater, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 4043 bytes --]
On Thu, Oct 24, 2019 at 11:04:53AM +0200, Greg Kurz wrote:
> On Thu, 24 Oct 2019 13:58:41 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
>
> > On Wed, Oct 23, 2019 at 04:52:10PM +0200, Greg Kurz wrote:
> > > Each VCPU is associated to a presenter object within the interrupt
> > > controller, ie. TCTX for XIVE and ICP for XICS, but our current
> > > models put these objects below the VCPU, and we rely on CPU_FOREACH()
> > > to do anything that involves presenters.
> > >
> > > This recently bit us with the CAM line matching logic in XIVE because
> > > CPU_FOREACH() can race with CPU hotplug and we ended up considering a
> > > VCPU that wasn't associated to a TCTX object yet. Other users of
> > > CPU_FOREACH() are 'info pic' for both XICS and XIVE. It is again very
> > > easy to crash QEMU with concurrent VCPU hotplug/unplug and 'info pic'.
> > >
> > > Reparent the presenter objects to the corresponding interrupt controller
> > > object, ie. XIVE router or ICS, to make it clear they are not some extra
> > > data hanging from the CPU but internal XIVE or XICS entities. The CPU
> > > object now needs to explicitely take a reference on the presenter to
> > > ensure its pointer remains valid until unrealize time.
> > >
> > > This will allow to get rid of CPU_FOREACH() and ease further improvements
> > > to the XIVE model.
> > >
> > > This change doesn't impact section ids and is thus transparent to
> > > migration.
> > >
> > > Signed-off-by: Greg Kurz <groug@kaod.org>
> >
> >
> > Urgh. I see why we want something like this, but reparenting the
> > presenters to the source modules (particularly for XICS) makes me
> > uncomfortable.
> >
> > AFAICT the association here is *purely* about managing lifetimes, not
> > because those ICPs inherently have something to do with those ICSes.
> > Same with XIVE, although since they'll be on the same chip there's a
> > bit more logic to it.
> >
>
> I did it this way for XIVE because they are indeed on the same chip,
> but I agree it is questionable for XICS.
>
> > For spapr we kinda-sorta treat the (single) ICS or XiveRouter object
> > as the "master" of the interrupt controller. But that's not really
>
> Agreed for XICS. On the other side, the XIVE controller chip really has
> a routing sub-engine (IVRE) and a presenter sub-engine (IVPE), and the
> TCTXs do reside in an SRAM within the IVPE. The XiveRouter type might
> be better named XiveController, and each instance to expose a XiveRouter
> and a XivePresenter interface. We have one per chip for PNV and only a
> single one for sPAPR.
Yes, that sounds like a reasonable approach for XIVE.
>
> > accurate to the hardware, so I don't really want to push that way of
> > looking at it any further than it already is.
> >
> > If we could make the presenters children of the machine (spapr) or
> > chip (pnv) that might make more sense?
> >
>
> It probably makes sense for XICS, not sure this makes things clearer
> for XIVE.
>
> > I'm also not totally convinced that having the presenter as a child of
> > the cpu is inherently bad. Yes we have bugs now, but maybe the right
> > fix *is* actually to do the NULL check on every CPU_FOREACH().
> >
> > For comparison, the lapic on x86 machines is a child of the cpu, and I
> > believe they do have NULL checks on cpu->apic_state in various places
> > they use CPU_FOREACH().
> >
>
> I didn't want to go that way because I was finding CPU_FOREACH() to
> be fragile since all users are broken,
Hm, ok. There aren't that many existing users though, right?
> but I can revisit that. Maybe
> worth consolidating the logic in a PRESENTER_FOREACH() macro in order
> to avoid future breakage with CPU_FOREACH() ?
That sounds worth considering at least, yes.
--
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 5/6] spapr: Don't use CPU_FOREACH() in 'info pic'
2019-10-24 9:28 ` Greg Kurz
@ 2019-10-27 17:01 ` David Gibson
0 siblings, 0 replies; 22+ messages in thread
From: David Gibson @ 2019-10-27 17:01 UTC (permalink / raw)
To: Greg Kurz; +Cc: Paolo Bonzini, qemu-ppc, Cédric Le Goater, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 6210 bytes --]
On Thu, Oct 24, 2019 at 11:28:45AM +0200, Greg Kurz wrote:
> On Thu, 24 Oct 2019 14:02:31 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
>
> > On Wed, Oct 23, 2019 at 04:52:21PM +0200, Greg Kurz wrote:
> > > Now that presenter objects are parented to the interrupt controller, stop
> > > relying on CPU_FOREACH() which can race with CPU hotplug and crash QEMU.
> > >
> > > Signed-off-by: Greg Kurz <groug@kaod.org>
> >
> > So.. we might be able to go further than this. Having the
> > TYPE_INTERRUPT_STATS_PROVIDER interrupt on the machine is actually an
> > spapr and pnv oddity. In most cases that interface is on the various
> > components of the interrupt controller directly. hmp_info_irq() scans
> > the whole QOM tree looking for everything with the interface to
> > produce the info pic output.
> >
> > It would be nice if we can do the same for xics and xive. The tricky
> > bit is that we do have the possibility of both, in which case the
> > individual components would need to know if they're currently "active"
> > and minimize their output if so.
> >
>
> Yes but this looks like 4.3 material. If we want to fix this for 4.2,
> I'm now thinking it might be safer to keep CPU_FOREACH() and check the
> state.
Yeah, for 4.2 that sounds like a good idea.
>
> > > ---
> > > hw/intc/spapr_xive.c | 8 +-------
> > > hw/intc/xics.c | 12 ++++++++++++
> > > hw/intc/xics_spapr.c | 8 +-------
> > > hw/intc/xive.c | 12 ++++++++++++
> > > include/hw/ppc/xics.h | 1 +
> > > include/hw/ppc/xive.h | 2 ++
> > > 6 files changed, 29 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
> > > index d74ee71e76b4..05763a58cf5d 100644
> > > --- a/hw/intc/spapr_xive.c
> > > +++ b/hw/intc/spapr_xive.c
> > > @@ -579,14 +579,8 @@ static void spapr_xive_set_irq(SpaprInterruptController *intc, int irq, int val)
> > > static void spapr_xive_print_info(SpaprInterruptController *intc, Monitor *mon)
> > > {
> > > SpaprXive *xive = SPAPR_XIVE(intc);
> > > - CPUState *cs;
> > > -
> > > - CPU_FOREACH(cs) {
> > > - PowerPCCPU *cpu = POWERPC_CPU(cs);
> > > -
> > > - xive_tctx_pic_print_info(spapr_cpu_state(cpu)->tctx, mon);
> > > - }
> > >
> > > + xive_presenter_print_info(XIVE_ROUTER(intc), mon);
> > > spapr_xive_pic_print_info(xive, mon);
> > > }
> > >
> > > diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> > > index d5e4db668a4b..6e820c4851f3 100644
> > > --- a/hw/intc/xics.c
> > > +++ b/hw/intc/xics.c
> > > @@ -88,6 +88,18 @@ void ics_pic_print_info(ICSState *ics, Monitor *mon)
> > > }
> > > }
> > >
> > > +static int do_ics_pic_print_icp_infos(Object *child, void *opaque)
> > > +{
> > > + icp_pic_print_info(ICP(child), opaque);
> > > + return 0;
> > > +}
> > > +
> > > +void ics_pic_print_icp_infos(ICSState *ics, const char *type, Monitor *mon)
> > > +{
> > > + object_child_foreach_type(OBJECT(ics), type, do_ics_pic_print_icp_infos,
> > > + mon);
> > > +}
> > > +
> > > /*
> > > * ICP: Presentation layer
> > > */
> > > diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
> > > index 080ed73aad64..7624d693c8da 100644
> > > --- a/hw/intc/xics_spapr.c
> > > +++ b/hw/intc/xics_spapr.c
> > > @@ -400,14 +400,8 @@ static void xics_spapr_set_irq(SpaprInterruptController *intc, int irq, int val)
> > > static void xics_spapr_print_info(SpaprInterruptController *intc, Monitor *mon)
> > > {
> > > ICSState *ics = ICS_SPAPR(intc);
> > > - CPUState *cs;
> > > -
> > > - CPU_FOREACH(cs) {
> > > - PowerPCCPU *cpu = POWERPC_CPU(cs);
> > > -
> > > - icp_pic_print_info(spapr_cpu_state(cpu)->icp, mon);
> > > - }
> > >
> > > + ics_pic_print_icp_infos(ics, TYPE_ICP, mon);
> > > ics_pic_print_info(ics, mon);
> > > }
> > >
> > > diff --git a/hw/intc/xive.c b/hw/intc/xive.c
> > > index 8d2da4a11163..40ce43152456 100644
> > > --- a/hw/intc/xive.c
> > > +++ b/hw/intc/xive.c
> > > @@ -547,6 +547,18 @@ void xive_tctx_pic_print_info(XiveTCTX *tctx, Monitor *mon)
> > > }
> > > }
> > >
> > > +static int do_xive_presenter_print_info(Object *child, void *opaque)
> > > +{
> > > + xive_tctx_pic_print_info(XIVE_TCTX(child), opaque);
> > > + return 0;
> > > +}
> > > +
> > > +void xive_presenter_print_info(XiveRouter *xrtr, Monitor *mon)
> > > +{
> > > + object_child_foreach_type(OBJECT(xrtr), TYPE_XIVE_TCTX,
> > > + do_xive_presenter_print_info, mon);
> > > +}
> > > +
> > > void xive_tctx_reset(XiveTCTX *tctx)
> > > {
> > > memset(tctx->regs, 0, sizeof(tctx->regs));
> > > diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> > > index f4827e748fd8..4de1f421c997 100644
> > > --- a/include/hw/ppc/xics.h
> > > +++ b/include/hw/ppc/xics.h
> > > @@ -175,6 +175,7 @@ static inline bool ics_irq_free(ICSState *ics, uint32_t srcno)
> > > void ics_set_irq_type(ICSState *ics, int srcno, bool lsi);
> > > void icp_pic_print_info(ICPState *icp, Monitor *mon);
> > > void ics_pic_print_info(ICSState *ics, Monitor *mon);
> > > +void ics_pic_print_icp_infos(ICSState *ics, const char *type, Monitor *mon);
> > >
> > > void ics_resend(ICSState *ics);
> > > void icp_resend(ICPState *ss);
> > > diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
> > > index 8fd439ec9bba..14690428a0aa 100644
> > > --- a/include/hw/ppc/xive.h
> > > +++ b/include/hw/ppc/xive.h
> > > @@ -367,6 +367,8 @@ int xive_router_write_nvt(XiveRouter *xrtr, uint8_t nvt_blk, uint32_t nvt_idx,
> > > XiveTCTX *xive_router_get_tctx(XiveRouter *xrtr, CPUState *cs);
> > > void xive_router_notify(XiveNotifier *xn, uint32_t lisn);
> > >
> > > +void xive_presenter_print_info(XiveRouter *xrtr, Monitor *mon);
> > > +
> > > /*
> > > * XIVE END ESBs
> > > */
> > >
> >
>
--
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 6/6] xive: Don't use CPU_FOREACH() to perform CAM line matching
2019-10-24 12:33 ` Greg Kurz
@ 2019-10-27 17:03 ` David Gibson
0 siblings, 0 replies; 22+ messages in thread
From: David Gibson @ 2019-10-27 17:03 UTC (permalink / raw)
To: Greg Kurz; +Cc: Paolo Bonzini, qemu-ppc, Cédric Le Goater, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 6740 bytes --]
On Thu, Oct 24, 2019 at 02:33:27PM +0200, Greg Kurz wrote:
> On Thu, 24 Oct 2019 14:05:36 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
>
> > On Wed, Oct 23, 2019 at 04:52:27PM +0200, Greg Kurz wrote:
> > > Now that the TCTX objects are children of the XIVE router, stop
> > > using CPU_FOREACH() when looking for a matching VCPU target.
> > >
> > > Signed-off-by: Greg Kurz <groug@kaod.org>
> > > ---
> > > hw/intc/xive.c | 100 +++++++++++++++++++++++++++++++++++---------------------
> > > 1 file changed, 62 insertions(+), 38 deletions(-)
> > >
> > > diff --git a/hw/intc/xive.c b/hw/intc/xive.c
> > > index 40ce43152456..ec5e7d0ee39a 100644
> > > --- a/hw/intc/xive.c
> > > +++ b/hw/intc/xive.c
> > > @@ -1403,55 +1403,79 @@ typedef struct XiveTCTXMatch {
> > > uint8_t ring;
> > > } XiveTCTXMatch;
> > >
> > > -static bool xive_presenter_match(XiveRouter *xrtr, uint8_t format,
> > > - uint8_t nvt_blk, uint32_t nvt_idx,
> > > - bool cam_ignore, uint8_t priority,
> > > - uint32_t logic_serv, XiveTCTXMatch *match)
> > > +typedef struct XivePresenterMatch {
> > > + uint8_t format;
> > > + uint8_t nvt_blk;
> > > + uint32_t nvt_idx;
> > > + bool cam_ignore;
> > > + uint8_t priority;
> > > + uint32_t logic_serv;
> > > + XiveTCTXMatch *match;
> > > + int count;
> > > +} XivePresenterMatch;
> > > +
> > > +static int do_xive_presenter_match(Object *child, void *opaque)
> > > {
> > > - CPUState *cs;
> > > + XiveTCTX *tctx = XIVE_TCTX(child);
> > > + XivePresenterMatch *xpm = opaque;
> > > + int ring;
> > >
> > > /*
> > > * TODO (PowerNV): handle chip_id overwrite of block field for
> > > * hardwired CAM compares
> > > */
> > >
> > > - CPU_FOREACH(cs) {
> > > - XiveTCTX *tctx = xive_router_get_tctx(xrtr, cs);
> > > - int ring;
> > > + /*
> > > + * HW checks that the CPU is enabled in the Physical Thread
> > > + * Enable Register (PTER).
> > > + */
> > >
> > > - /*
> > > - * Skip partially initialized vCPUs. This can happen when
> > > - * vCPUs are hotplugged.
> > > - */
> > > - if (!tctx) {
> > > - continue;
> > > + /*
> > > + * Check the thread context CAM lines and record matches. We
> > > + * will handle CPU exception delivery later
> > > + */
> > > + ring = xive_presenter_tctx_match(tctx, xpm->format, xpm->nvt_blk,
> > > + xpm->nvt_idx, xpm->cam_ignore,
> > > + xpm->logic_serv);
> > > +
> > > + /*
> > > + * Save the context and follow on to catch duplicates, that we
> > > + * don't support yet.
> > > + */
> > > + if (ring != -1) {
> > > + if (xpm->match->tctx) {
> > > + qemu_log_mask(LOG_GUEST_ERROR, "XIVE: already found a thread "
> > > + "context NVT %x/%x\n", xpm->nvt_blk, xpm->nvt_idx);
> > > + return -1;
> > > }
> > >
> > > - /*
> > > - * HW checks that the CPU is enabled in the Physical Thread
> > > - * Enable Register (PTER).
> > > - */
> > > + xpm->match->ring = ring;
> > > + xpm->match->tctx = tctx;
> > > + xpm->count++;
> > > + }
> > >
> > > - /*
> > > - * Check the thread context CAM lines and record matches. We
> > > - * will handle CPU exception delivery later
> > > - */
> > > - ring = xive_presenter_tctx_match(tctx, format, nvt_blk, nvt_idx,
> > > - cam_ignore, logic_serv);
> > > - /*
> > > - * Save the context and follow on to catch duplicates, that we
> > > - * don't support yet.
> > > - */
> > > - if (ring != -1) {
> > > - if (match->tctx) {
> > > - qemu_log_mask(LOG_GUEST_ERROR, "XIVE: already found a thread "
> > > - "context NVT %x/%x\n", nvt_blk, nvt_idx);
> > > - return false;
> > > - }
> > > -
> > > - match->ring = ring;
> > > - match->tctx = tctx;
> > > - }
> > > + return 0;
> > > +}
> > > +
> > > +static bool xive_presenter_match(XiveRouter *xrtr, uint8_t format,
> > > + uint8_t nvt_blk, uint32_t nvt_idx,
> > > + bool cam_ignore, uint8_t priority,
> > > + uint32_t logic_serv, XiveTCTXMatch *match)
> > > +{
> > > + XivePresenterMatch xpm = {
> > > + .format = format,
> > > + .nvt_blk = nvt_blk,
> > > + .nvt_idx = nvt_idx,
> > > + .cam_ignore = cam_ignore,
> > > + .priority = priority,
> > > + .logic_serv = logic_serv,
> > > + .match = match,
> > > + .count = 0,
> > > + };
> > > +
> > > + if (object_child_foreach_type(OBJECT(xrtr), TYPE_XIVE_TCTX,
> > > + do_xive_presenter_match, &xpm) < 0) {
> > > + return false;
> >
> > Hrm... xive_presenter_match() is potentially a pretty hot path, it's
> > called on every interrupt delivery - especially since we don't have a
> > usable KVM irq chip for Boston machines. I'm concerned that using
> > something as heavyweight as object_child_foreach() might have a
> > noticeable performance impact.
> >
>
> Well, the XiveRouter _only_ has 3 extra children (XiveSource,
> XiveENDSource and TIMA) but indeed object_child_foreach() might
> cost more than QTAILQ_FOREACH_RCU().
Right, it's not so much the redundant children, but whatever we have
to do to delve into the children data structure I'm concerned about
here.
> A possible option could be
> to have a QTAILQ of presenters under the machine for sPAPR or
> under the chip for PNV, in order to avoid the need to filter out
> VCPUs that we don't want to consider, ie. partly realized with
> sPAPR and from another chip with PNV.
If we can manage it, I'd actually suggest a plain old array/vector of
them, rather than a QTAILQ.
> But as said in another mail, the safer for 4.2 is probably to
> fix the CPU_FOREACH() users, which is already the case here for
> sPAPR.
>
> > > }
> > >
> > > if (!match->tctx) {
> > >
> >
>
--
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2019-10-27 17:37 UTC | newest]
Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-23 14:51 [PATCH 0/6] ppc: Reparent the interrupt presenter Greg Kurz
2019-10-23 14:51 ` [PATCH 1/6] ppc: Add intc_destroy() handlers to SpaprInterruptController/PnvChip Greg Kurz
2019-10-24 2:50 ` David Gibson
2019-10-24 7:20 ` Greg Kurz
2019-10-23 14:52 ` [PATCH 2/6] xive, xics: Fix reference counting on CPU objects Greg Kurz
2019-10-24 2:50 ` David Gibson
2019-10-23 14:52 ` [PATCH 3/6] ppc: Reparent presenter objects to the interrupt controller object Greg Kurz
2019-10-24 2:58 ` David Gibson
2019-10-24 9:04 ` Greg Kurz
2019-10-27 16:57 ` David Gibson
2019-10-23 14:52 ` [PATCH 4/6] qom: Add object_child_foreach_type() helper function Greg Kurz
2019-10-24 2:59 ` David Gibson
2019-10-24 3:07 ` David Gibson
2019-10-24 9:20 ` Greg Kurz
2019-10-23 14:52 ` [PATCH 5/6] spapr: Don't use CPU_FOREACH() in 'info pic' Greg Kurz
2019-10-24 3:02 ` David Gibson
2019-10-24 9:28 ` Greg Kurz
2019-10-27 17:01 ` David Gibson
2019-10-23 14:52 ` [PATCH 6/6] xive: Don't use CPU_FOREACH() to perform CAM line matching Greg Kurz
2019-10-24 3:05 ` David Gibson
2019-10-24 12:33 ` Greg Kurz
2019-10-27 17:03 ` David Gibson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).