* [PATCH v3 0/4] ppc: reset the interrupt presenter from the CPU reset handler
@ 2019-10-22 7:22 Cédric Le Goater
2019-10-22 7:22 ` [PATCH v3 1/4] spapr: move CPU reset after presenter creation Cédric Le Goater
` (3 more replies)
0 siblings, 4 replies; 12+ messages in thread
From: Cédric Le Goater @ 2019-10-22 7:22 UTC (permalink / raw)
To: David Gibson; +Cc: Cédric Le Goater, qemu-ppc, qemu-devel, Greg Kurz
Hello,
On the sPAPR machine and PowerNV machine, the interrupt presenters are
created by a machine handler at the core level and are reseted
independently. This is not consistent and it raises issues when it
comes to handle hot-plugged CPUs. In that case, the presenters are not
reseted. This is less of an issue in XICS, although a zero MFFR could
be a concern, but in XIVE, the OS CAM line is not set and this breaks
the presenting algorithm. The current code has workarounds which need
a global cleanup.
Extend the sPAPR IRQ backend and the PowerNV Chip class with a new
cpu_intc_reset() handler called by the CPU reset handler and remove
the XiveTCTX reset handler which is now redundant.
Set the OS CAM line when the interrupt presenter of the sPAPR core is
reseted. This will also cover the case of hot-plugged CPUs.
Thanks,
C.
Changes in v3:
- Introduced a DeviceClass::reset for the CPU (Greg)
- add support for PowerNV
Changes in v2:
- removed property
- simplified reset handlers
Cédric Le Goater (3):
spapr: move CPU reset after presenter creation
ppc: reset the interrupt presenter from the CPU reset handler
spapr/xive: Set the OS CAM line at reset
Greg Kurz (1):
spapr_cpu_core: Implement DeviceClass::reset
include/hw/ppc/pnv.h | 1 +
include/hw/ppc/spapr_irq.h | 2 ++
include/hw/ppc/spapr_xive.h | 1 -
include/hw/ppc/xics.h | 1 +
include/hw/ppc/xive.h | 1 +
hw/intc/spapr_xive.c | 53 +++++++++++++++++--------------------
hw/intc/xics.c | 8 ++----
hw/intc/xics_spapr.c | 7 +++++
hw/intc/xive.c | 12 +--------
hw/ppc/pnv.c | 18 +++++++++++++
hw/ppc/pnv_core.c | 8 ++++++
hw/ppc/spapr_cpu_core.c | 38 +++++++++++++++++++-------
hw/ppc/spapr_irq.c | 14 ++++++++++
13 files changed, 107 insertions(+), 57 deletions(-)
--
2.21.0
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v3 1/4] spapr: move CPU reset after presenter creation
2019-10-22 7:22 [PATCH v3 0/4] ppc: reset the interrupt presenter from the CPU reset handler Cédric Le Goater
@ 2019-10-22 7:22 ` Cédric Le Goater
2019-10-22 8:53 ` Philippe Mathieu-Daudé
2019-10-22 9:53 ` Greg Kurz
2019-10-22 7:22 ` [PATCH v3 2/4] spapr_cpu_core: Implement DeviceClass::reset Cédric Le Goater
` (2 subsequent siblings)
3 siblings, 2 replies; 12+ messages in thread
From: Cédric Le Goater @ 2019-10-22 7:22 UTC (permalink / raw)
To: David Gibson; +Cc: Cédric Le Goater, qemu-ppc, qemu-devel, Greg Kurz
spapr_realize_vcpu() is modified to call the CPU reset only after the
the intc presenter has been created.
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
hw/ppc/spapr_cpu_core.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index 3e4302c7d596..992f00da6540 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -234,13 +234,16 @@ static void spapr_realize_vcpu(PowerPCCPU *cpu, SpaprMachineState *spapr,
cpu_ppc_set_vhyp(cpu, PPC_VIRTUAL_HYPERVISOR(spapr));
kvmppc_set_papr(cpu);
- qemu_register_reset(spapr_cpu_reset, cpu);
- spapr_cpu_reset(cpu);
-
if (spapr_irq_cpu_intc_create(spapr, cpu, &local_err) < 0) {
- goto error_unregister;
+ goto error_intc_create;
}
+ /*
+ * FIXME: Hot-plugged CPUs are not reseted. Do it at realize.
+ */
+ qemu_register_reset(spapr_cpu_reset, cpu);
+ spapr_cpu_reset(cpu);
+
if (!sc->pre_3_0_migration) {
vmstate_register(NULL, cs->cpu_index, &vmstate_spapr_cpu_state,
cpu->machine_data);
@@ -248,8 +251,7 @@ static void spapr_realize_vcpu(PowerPCCPU *cpu, SpaprMachineState *spapr,
return;
-error_unregister:
- qemu_unregister_reset(spapr_cpu_reset, cpu);
+error_intc_create:
cpu_remove_sync(CPU(cpu));
error:
error_propagate(errp, local_err);
--
2.21.0
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v3 2/4] spapr_cpu_core: Implement DeviceClass::reset
2019-10-22 7:22 [PATCH v3 0/4] ppc: reset the interrupt presenter from the CPU reset handler Cédric Le Goater
2019-10-22 7:22 ` [PATCH v3 1/4] spapr: move CPU reset after presenter creation Cédric Le Goater
@ 2019-10-22 7:22 ` Cédric Le Goater
2019-10-22 8:58 ` Philippe Mathieu-Daudé
2019-10-22 7:22 ` [PATCH v3 3/4] ppc: reset the interrupt presenter from the CPU reset handler Cédric Le Goater
2019-10-22 7:22 ` [PATCH v3 4/4] spapr/xive: Set the OS CAM line at reset Cédric Le Goater
3 siblings, 1 reply; 12+ messages in thread
From: Cédric Le Goater @ 2019-10-22 7:22 UTC (permalink / raw)
To: David Gibson; +Cc: Cédric Le Goater, qemu-ppc, qemu-devel, Greg Kurz
From: Greg Kurz <groug@kaod.org>
Since vCPUs aren't plugged into a bus, we manually register a reset
handler for each vCPU. We also call this handler at realize time
to ensure hot plugged vCPUs are reset before being exposed to the
guest. This results in vCPUs being reset twice at machine reset.
It doesn't break anything but it is slightly suboptimal and above
all confusing.
The hotplug path in device_set_realized() already knows how to reset
a hotplugged device if the device reset handler is present. Implement
one for sPAPR CPU cores that resets all vCPUs under a core.
While here rename spapr_cpu_reset() to spapr_reset_vcpu() for
consistency with spapr_realize_vcpu() and spapr_unrealize_vcpu().
Signed-off-by: Greg Kurz <groug@kaod.org>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
hw/ppc/spapr_cpu_core.c | 31 ++++++++++++++++++++++---------
1 file changed, 22 insertions(+), 9 deletions(-)
diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index 992f00da6540..5947e39b36ad 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -25,9 +25,8 @@
#include "sysemu/hw_accel.h"
#include "qemu/error-report.h"
-static void spapr_cpu_reset(void *opaque)
+static void spapr_reset_vcpu(PowerPCCPU *cpu)
{
- PowerPCCPU *cpu = opaque;
CPUState *cs = CPU(cpu);
CPUPPCState *env = &cpu->env;
PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
@@ -193,7 +192,6 @@ 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);
}
- qemu_unregister_reset(spapr_cpu_reset, cpu);
if (spapr_cpu_state(cpu)->icp) {
object_unparent(OBJECT(spapr_cpu_state(cpu)->icp));
}
@@ -204,12 +202,30 @@ static void spapr_unrealize_vcpu(PowerPCCPU *cpu, SpaprCpuCore *sc)
object_unparent(OBJECT(cpu));
}
+static void spapr_cpu_core_reset(DeviceState *dev)
+{
+ CPUCore *cc = CPU_CORE(dev);
+ SpaprCpuCore *sc = SPAPR_CPU_CORE(dev);
+ int i;
+
+ for (i = 0; i < cc->nr_threads; i++) {
+ spapr_reset_vcpu(sc->threads[i]);
+ }
+}
+
+static void spapr_cpu_core_reset_handler(void *opaque)
+{
+ spapr_cpu_core_reset(opaque);
+}
+
static void spapr_cpu_core_unrealize(DeviceState *dev, Error **errp)
{
SpaprCpuCore *sc = SPAPR_CPU_CORE(OBJECT(dev));
CPUCore *cc = CPU_CORE(dev);
int i;
+ qemu_unregister_reset(spapr_cpu_core_reset_handler, sc);
+
for (i = 0; i < cc->nr_threads; i++) {
spapr_unrealize_vcpu(sc->threads[i], sc);
}
@@ -238,12 +254,6 @@ static void spapr_realize_vcpu(PowerPCCPU *cpu, SpaprMachineState *spapr,
goto error_intc_create;
}
- /*
- * FIXME: Hot-plugged CPUs are not reseted. Do it at realize.
- */
- qemu_register_reset(spapr_cpu_reset, cpu);
- spapr_cpu_reset(cpu);
-
if (!sc->pre_3_0_migration) {
vmstate_register(NULL, cs->cpu_index, &vmstate_spapr_cpu_state,
cpu->machine_data);
@@ -338,6 +348,8 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
goto err_unrealize;
}
}
+
+ qemu_register_reset(spapr_cpu_core_reset_handler, sc);
return;
err_unrealize:
@@ -366,6 +378,7 @@ static void spapr_cpu_core_class_init(ObjectClass *oc, void *data)
dc->realize = spapr_cpu_core_realize;
dc->unrealize = spapr_cpu_core_unrealize;
+ dc->reset = spapr_cpu_core_reset;
dc->props = spapr_cpu_core_properties;
scc->cpu_type = data;
}
--
2.21.0
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v3 3/4] ppc: reset the interrupt presenter from the CPU reset handler
2019-10-22 7:22 [PATCH v3 0/4] ppc: reset the interrupt presenter from the CPU reset handler Cédric Le Goater
2019-10-22 7:22 ` [PATCH v3 1/4] spapr: move CPU reset after presenter creation Cédric Le Goater
2019-10-22 7:22 ` [PATCH v3 2/4] spapr_cpu_core: Implement DeviceClass::reset Cédric Le Goater
@ 2019-10-22 7:22 ` Cédric Le Goater
2019-10-22 9:08 ` Philippe Mathieu-Daudé
2019-10-22 11:19 ` Greg Kurz
2019-10-22 7:22 ` [PATCH v3 4/4] spapr/xive: Set the OS CAM line at reset Cédric Le Goater
3 siblings, 2 replies; 12+ messages in thread
From: Cédric Le Goater @ 2019-10-22 7:22 UTC (permalink / raw)
To: David Gibson; +Cc: Cédric Le Goater, qemu-ppc, qemu-devel, Greg Kurz
On the sPAPR machine and PowerNV machine, the interrupt presenters are
created by a machine handler at the core level and are reseted
independently. This is not consistent and it raises issues when it
comes to handle hot-plugged CPUs. In that case, the presenters are not
reseted. This is less of an issue in XICS, although a zero MFFR could
be a concern, but in XIVE, the OS CAM line is not set and this breaks
the presenting algorithm. The current code has workarounds which need
a global cleanup.
Extend the sPAPR IRQ backend and the PowerNV Chip class with a new
cpu_intc_reset() handler called by the CPU reset handler and remove
the XiveTCTX reset handler which is now redundant.
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
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 +
hw/intc/spapr_xive.c | 9 +++++++++
hw/intc/xics.c | 8 ++------
hw/intc/xics_spapr.c | 7 +++++++
hw/intc/xive.c | 12 +-----------
hw/ppc/pnv.c | 18 ++++++++++++++++++
hw/ppc/pnv_core.c | 8 ++++++++
hw/ppc/spapr_cpu_core.c | 5 ++++-
hw/ppc/spapr_irq.c | 14 ++++++++++++++
12 files changed, 68 insertions(+), 18 deletions(-)
diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
index 1cdbe55bf86c..2a780e633f23 100644
--- a/include/hw/ppc/pnv.h
+++ b/include/hw/ppc/pnv.h
@@ -111,6 +111,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);
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 5e150a667902..09232999b07e 100644
--- a/include/hw/ppc/spapr_irq.h
+++ b/include/hw/ppc/spapr_irq.h
@@ -52,6 +52,7 @@ typedef struct SpaprInterruptControllerClass {
*/
int (*cpu_intc_create)(SpaprInterruptController *intc,
PowerPCCPU *cpu, Error **errp);
+ void (*cpu_intc_reset)(SpaprInterruptController *intc, PowerPCCPU *cpu);
int (*claim_irq)(SpaprInterruptController *intc, int irq, bool lsi,
Error **errp);
void (*free_irq)(SpaprInterruptController *intc, int irq);
@@ -68,6 +69,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_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 1e6a9300eb2b..602173c12250 100644
--- a/include/hw/ppc/xics.h
+++ b/include/hw/ppc/xics.h
@@ -161,6 +161,7 @@ void icp_set_mfrr(ICPState *icp, uint8_t mfrr);
uint32_t icp_accept(ICPState *ss);
uint32_t icp_ipoll(ICPState *ss, uint32_t *mfrr);
void icp_eoi(ICPState *icp, uint32_t xirr);
+void icp_reset(ICPState *icp);
void ics_write_xive(ICSState *ics, int nr, int server,
uint8_t priority, uint8_t saved_priority);
diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
index fd3319bd3202..99381639f50c 100644
--- a/include/hw/ppc/xive.h
+++ b/include/hw/ppc/xive.h
@@ -415,6 +415,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);
static inline uint32_t xive_nvt_cam_line(uint8_t nvt_blk, uint32_t nvt_idx)
{
diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
index ba32d2cc5b0f..20a8d8285f64 100644
--- a/hw/intc/spapr_xive.c
+++ b/hw/intc/spapr_xive.c
@@ -553,6 +553,14 @@ static int spapr_xive_cpu_intc_create(SpaprInterruptController *intc,
return 0;
}
+static void spapr_xive_cpu_intc_reset(SpaprInterruptController *intc,
+ PowerPCCPU *cpu)
+{
+ XiveTCTX *tctx = spapr_cpu_state(cpu)->tctx;
+
+ xive_tctx_reset(tctx);
+}
+
static void spapr_xive_set_irq(SpaprInterruptController *intc, int irq, int val)
{
SpaprXive *xive = SPAPR_XIVE(intc);
@@ -697,6 +705,7 @@ static void spapr_xive_class_init(ObjectClass *klass, void *data)
sicc->activate = spapr_xive_activate;
sicc->deactivate = spapr_xive_deactivate;
sicc->cpu_intc_create = spapr_xive_cpu_intc_create;
+ sicc->cpu_intc_reset = spapr_xive_cpu_intc_reset;
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 b5ac408f7b74..6da05763f9db 100644
--- a/hw/intc/xics.c
+++ b/hw/intc/xics.c
@@ -274,10 +274,8 @@ static const VMStateDescription vmstate_icp_server = {
},
};
-static void icp_reset_handler(void *dev)
+void icp_reset(ICPState *icp)
{
- ICPState *icp = ICP(dev);
-
icp->xirr = 0;
icp->pending_priority = 0xff;
icp->mfrr = 0xff;
@@ -288,7 +286,7 @@ static void icp_reset_handler(void *dev)
if (kvm_irqchip_in_kernel()) {
Error *local_err = NULL;
- icp_set_kvm_state(ICP(dev), &local_err);
+ icp_set_kvm_state(icp, &local_err);
if (local_err) {
error_report_err(local_err);
}
@@ -351,7 +349,6 @@ static void icp_realize(DeviceState *dev, Error **errp)
}
}
- qemu_register_reset(icp_reset_handler, dev);
vmstate_register(NULL, icp->cs->cpu_index, &vmstate_icp_server, icp);
}
@@ -360,7 +357,6 @@ static void icp_unrealize(DeviceState *dev, Error **errp)
ICPState *icp = ICP(dev);
vmstate_unregister(NULL, &vmstate_icp_server, icp);
- qemu_unregister_reset(icp_reset_handler, dev);
}
static void icp_class_init(ObjectClass *klass, void *data)
diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
index 4f64b9a9fc66..7418fb9f370c 100644
--- a/hw/intc/xics_spapr.c
+++ b/hw/intc/xics_spapr.c
@@ -346,6 +346,12 @@ static int xics_spapr_cpu_intc_create(SpaprInterruptController *intc,
return 0;
}
+static void xics_spapr_cpu_intc_reset(SpaprInterruptController *intc,
+ PowerPCCPU *cpu)
+{
+ icp_reset(spapr_cpu_state(cpu)->icp);
+}
+
static int xics_spapr_claim_irq(SpaprInterruptController *intc, int irq,
bool lsi, Error **errp)
{
@@ -433,6 +439,7 @@ static void ics_spapr_class_init(ObjectClass *klass, void *data)
sicc->activate = xics_spapr_activate;
sicc->deactivate = xics_spapr_deactivate;
sicc->cpu_intc_create = xics_spapr_cpu_intc_create;
+ sicc->cpu_intc_reset = xics_spapr_cpu_intc_reset;
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 d420c6571e14..f066be5eb5e3 100644
--- a/hw/intc/xive.c
+++ b/hw/intc/xive.c
@@ -547,10 +547,8 @@ void xive_tctx_pic_print_info(XiveTCTX *tctx, Monitor *mon)
}
}
-static void xive_tctx_reset(void *dev)
+void xive_tctx_reset(XiveTCTX *tctx)
{
- XiveTCTX *tctx = XIVE_TCTX(dev);
-
memset(tctx->regs, 0, sizeof(tctx->regs));
/* Set some defaults */
@@ -607,13 +605,6 @@ static void xive_tctx_realize(DeviceState *dev, Error **errp)
return;
}
}
-
- qemu_register_reset(xive_tctx_reset, dev);
-}
-
-static void xive_tctx_unrealize(DeviceState *dev, Error **errp)
-{
- qemu_unregister_reset(xive_tctx_reset, dev);
}
static int vmstate_xive_tctx_pre_save(void *opaque)
@@ -668,7 +659,6 @@ static void xive_tctx_class_init(ObjectClass *klass, void *data)
dc->desc = "XIVE Interrupt Thread Context";
dc->realize = xive_tctx_realize;
- dc->unrealize = xive_tctx_unrealize;
dc->vmsd = &vmstate_xive_tctx;
/*
* Reason: part of XIVE interrupt controller, needs to be wired up
diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 7cf64b6d2533..4a51fb65a834 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -778,6 +778,13 @@ 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);
+
+ icp_reset(ICP(pnv_cpu->intc));
+}
+
/*
* 0:48 Reserved - Read as zeroes
* 49:52 Node ID
@@ -815,6 +822,13 @@ static void pnv_chip_power9_intc_create(PnvChip *chip, PowerPCCPU *cpu,
pnv_cpu->intc = obj;
}
+static void pnv_chip_power9_intc_reset(PnvChip *chip, PowerPCCPU *cpu)
+{
+ PnvCPUState *pnv_cpu = pnv_cpu_state(cpu);
+
+ xive_tctx_reset(XIVE_TCTX(pnv_cpu->intc));
+}
+
/*
* Allowed core identifiers on a POWER8 Processor Chip :
*
@@ -984,6 +998,7 @@ static void pnv_chip_power8e_class_init(ObjectClass *klass, void *data)
k->cores_mask = POWER8E_CORE_MASK;
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->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;
@@ -1003,6 +1018,7 @@ static void pnv_chip_power8_class_init(ObjectClass *klass, void *data)
k->cores_mask = POWER8_CORE_MASK;
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->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;
@@ -1022,6 +1038,7 @@ static void pnv_chip_power8nvl_class_init(ObjectClass *klass, void *data)
k->cores_mask = POWER8_CORE_MASK;
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->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;
@@ -1191,6 +1208,7 @@ static void pnv_chip_power9_class_init(ObjectClass *klass, void *data)
k->cores_mask = POWER9_CORE_MASK;
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->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 b1a7489e7abf..f36cb39dbf77 100644
--- a/hw/ppc/pnv_core.c
+++ b/hw/ppc/pnv_core.c
@@ -45,6 +45,8 @@ static void pnv_cpu_reset(void *opaque)
PowerPCCPU *cpu = opaque;
CPUState *cs = CPU(cpu);
CPUPPCState *env = &cpu->env;
+ PnvChipClass *pcc;
+ Object *chip;
cpu_reset(cs);
@@ -55,6 +57,10 @@ static void pnv_cpu_reset(void *opaque)
env->gpr[3] = PNV_FDT_ADDR;
env->nip = 0x10;
env->msr |= MSR_HVB; /* Hypervisor mode */
+
+ chip = object_property_get_link(OBJECT(cpu), "chip", &error_fatal);
+ pcc = PNV_CHIP_GET_CLASS(chip);
+ pcc->intc_reset(PNV_CHIP(chip), cpu);
}
/*
@@ -169,6 +175,8 @@ static void pnv_realize_vcpu(PowerPCCPU *cpu, PnvChip *chip, Error **errp)
Error *local_err = NULL;
PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip);
+ object_property_add_const_link(OBJECT(cpu), "chip",
+ OBJECT(chip), &error_fatal);
object_property_set_bool(OBJECT(cpu), true, "realized", &local_err);
if (local_err) {
error_propagate(errp, local_err);
diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index 5947e39b36ad..d2903c2d0f22 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -32,6 +32,7 @@ static void spapr_reset_vcpu(PowerPCCPU *cpu)
PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu);
target_ulong lpcr;
+ SpaprMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
cpu_reset(cs);
@@ -76,9 +77,11 @@ static void spapr_reset_vcpu(PowerPCCPU *cpu)
spapr_cpu->dtl_addr = 0;
spapr_cpu->dtl_size = 0;
- spapr_caps_cpu_apply(SPAPR_MACHINE(qdev_get_machine()), cpu);
+ spapr_caps_cpu_apply(spapr, cpu);
kvm_check_mmu(cpu, &error_fatal);
+
+ spapr_irq_cpu_intc_reset(spapr, cpu);
}
void spapr_cpu_set_entry_state(PowerPCCPU *cpu, target_ulong nip, target_ulong r3)
diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
index 234d1073e518..b941608b69ba 100644
--- a/hw/ppc/spapr_irq.c
+++ b/hw/ppc/spapr_irq.c
@@ -220,6 +220,20 @@ int spapr_irq_cpu_intc_create(SpaprMachineState *spapr,
return 0;
}
+void spapr_irq_cpu_intc_reset(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_reset(intc, cpu);
+ }
+ }
+}
+
static void spapr_set_irq(void *opaque, int irq, int level)
{
SpaprMachineState *spapr = SPAPR_MACHINE(opaque);
--
2.21.0
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v3 4/4] spapr/xive: Set the OS CAM line at reset
2019-10-22 7:22 [PATCH v3 0/4] ppc: reset the interrupt presenter from the CPU reset handler Cédric Le Goater
` (2 preceding siblings ...)
2019-10-22 7:22 ` [PATCH v3 3/4] ppc: reset the interrupt presenter from the CPU reset handler Cédric Le Goater
@ 2019-10-22 7:22 ` Cédric Le Goater
2019-10-22 11:27 ` Greg Kurz
3 siblings, 1 reply; 12+ messages in thread
From: Cédric Le Goater @ 2019-10-22 7:22 UTC (permalink / raw)
To: David Gibson; +Cc: Cédric Le Goater, qemu-ppc, qemu-devel, Greg Kurz
When a Virtual Processor is scheduled to run on a HW thread, the
hypervisor pushes its identifier in the OS CAM line. When running with
kernel_irqchip=off, QEMU needs to emulate the same behavior.
Set the OS CAM line when the interrupt presenter of the sPAPR core is
reseted. This will also cover the case of hot-plugged CPUs.
This change also has the benefit to remove the use of CPU_FOREACH()
which can be unsafe.
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
include/hw/ppc/spapr_xive.h | 1 -
hw/intc/spapr_xive.c | 48 +++++++++++++------------------------
2 files changed, 17 insertions(+), 32 deletions(-)
diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h
index d84bd5c229f0..742b7e834f2a 100644
--- a/include/hw/ppc/spapr_xive.h
+++ b/include/hw/ppc/spapr_xive.h
@@ -57,7 +57,6 @@ typedef struct SpaprXive {
void spapr_xive_pic_print_info(SpaprXive *xive, Monitor *mon);
void spapr_xive_hcall_init(SpaprMachineState *spapr);
-void spapr_xive_set_tctx_os_cam(XiveTCTX *tctx);
void spapr_xive_mmio_set_enabled(SpaprXive *xive, bool enable);
void spapr_xive_map_mmio(SpaprXive *xive);
diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
index 20a8d8285f64..d8e1291905c3 100644
--- a/hw/intc/spapr_xive.c
+++ b/hw/intc/spapr_xive.c
@@ -205,23 +205,6 @@ void spapr_xive_mmio_set_enabled(SpaprXive *xive, bool enable)
memory_region_set_enabled(&xive->end_source.esb_mmio, false);
}
-/*
- * When a Virtual Processor is scheduled to run on a HW thread, the
- * hypervisor pushes its identifier in the OS CAM line. Emulate the
- * same behavior under QEMU.
- */
-void spapr_xive_set_tctx_os_cam(XiveTCTX *tctx)
-{
- uint8_t nvt_blk;
- uint32_t nvt_idx;
- uint32_t nvt_cam;
-
- spapr_xive_cpu_to_nvt(POWERPC_CPU(tctx->cs), &nvt_blk, &nvt_idx);
-
- nvt_cam = cpu_to_be32(TM_QW1W2_VO | xive_nvt_cam_line(nvt_blk, nvt_idx));
- memcpy(&tctx->regs[TM_QW1_OS + TM_WORD2], &nvt_cam, 4);
-}
-
static void spapr_xive_end_reset(XiveEND *end)
{
memset(end, 0, sizeof(*end));
@@ -544,21 +527,32 @@ static int spapr_xive_cpu_intc_create(SpaprInterruptController *intc,
}
spapr_cpu->tctx = XIVE_TCTX(obj);
-
- /*
- * (TCG) Early setting the OS CAM line for hotplugged CPUs as they
- * don't beneficiate from the reset of the XIVE IRQ backend
- */
- spapr_xive_set_tctx_os_cam(spapr_cpu->tctx);
return 0;
}
+static void xive_tctx_set_os_cam(XiveTCTX *tctx, uint32_t os_cam)
+{
+ uint32_t qw1w2 = cpu_to_be32(TM_QW1W2_VO | os_cam);
+ memcpy(&tctx->regs[TM_QW1_OS + TM_WORD2], &qw1w2, 4);
+}
+
static void spapr_xive_cpu_intc_reset(SpaprInterruptController *intc,
PowerPCCPU *cpu)
{
XiveTCTX *tctx = spapr_cpu_state(cpu)->tctx;
+ uint8_t nvt_blk;
+ uint32_t nvt_idx;
xive_tctx_reset(tctx);
+
+ /*
+ * When a Virtual Processor is scheduled to run on a HW thread,
+ * the hypervisor pushes its identifier in the OS CAM line.
+ * Emulate the same behavior under QEMU.
+ */
+ spapr_xive_cpu_to_nvt(cpu, &nvt_blk, &nvt_idx);
+
+ xive_tctx_set_os_cam(tctx, xive_nvt_cam_line(nvt_blk, nvt_idx));
}
static void spapr_xive_set_irq(SpaprInterruptController *intc, int irq, int val)
@@ -651,14 +645,6 @@ static void spapr_xive_dt(SpaprInterruptController *intc, uint32_t nr_servers,
static int spapr_xive_activate(SpaprInterruptController *intc, Error **errp)
{
SpaprXive *xive = SPAPR_XIVE(intc);
- CPUState *cs;
-
- CPU_FOREACH(cs) {
- PowerPCCPU *cpu = POWERPC_CPU(cs);
-
- /* (TCG) Set the OS CAM line of the thread interrupt context. */
- spapr_xive_set_tctx_os_cam(spapr_cpu_state(cpu)->tctx);
- }
if (kvm_enabled()) {
int rc = spapr_irq_init_kvm(kvmppc_xive_connect, intc, errp);
--
2.21.0
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v3 1/4] spapr: move CPU reset after presenter creation
2019-10-22 7:22 ` [PATCH v3 1/4] spapr: move CPU reset after presenter creation Cédric Le Goater
@ 2019-10-22 8:53 ` Philippe Mathieu-Daudé
2019-10-22 9:53 ` Greg Kurz
1 sibling, 0 replies; 12+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-10-22 8:53 UTC (permalink / raw)
To: Cédric Le Goater, David Gibson; +Cc: qemu-ppc, qemu-devel, Greg Kurz
On 10/22/19 9:22 AM, Cédric Le Goater wrote:
> spapr_realize_vcpu() is modified to call the CPU reset only after the
> the intc presenter has been created.
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
> hw/ppc/spapr_cpu_core.c | 14 ++++++++------
> 1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 3e4302c7d596..992f00da6540 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -234,13 +234,16 @@ static void spapr_realize_vcpu(PowerPCCPU *cpu, SpaprMachineState *spapr,
> cpu_ppc_set_vhyp(cpu, PPC_VIRTUAL_HYPERVISOR(spapr));
> kvmppc_set_papr(cpu);
>
> - qemu_register_reset(spapr_cpu_reset, cpu);
> - spapr_cpu_reset(cpu);
> -
> if (spapr_irq_cpu_intc_create(spapr, cpu, &local_err) < 0) {
> - goto error_unregister;
> + goto error_intc_create;
> }
>
> + /*
> + * FIXME: Hot-plugged CPUs are not reseted. Do it at realize.
Typo: "reset" (past tense of reset is also reset).
> + */
> + qemu_register_reset(spapr_cpu_reset, cpu);
> + spapr_cpu_reset(cpu);
> +
> if (!sc->pre_3_0_migration) {
> vmstate_register(NULL, cs->cpu_index, &vmstate_spapr_cpu_state,
> cpu->machine_data);
> @@ -248,8 +251,7 @@ static void spapr_realize_vcpu(PowerPCCPU *cpu, SpaprMachineState *spapr,
>
> return;
>
> -error_unregister:
> - qemu_unregister_reset(spapr_cpu_reset, cpu);
> +error_intc_create:
> cpu_remove_sync(CPU(cpu));
> error:
> error_propagate(errp, local_err);
>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 2/4] spapr_cpu_core: Implement DeviceClass::reset
2019-10-22 7:22 ` [PATCH v3 2/4] spapr_cpu_core: Implement DeviceClass::reset Cédric Le Goater
@ 2019-10-22 8:58 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 12+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-10-22 8:58 UTC (permalink / raw)
To: Cédric Le Goater, David Gibson; +Cc: qemu-ppc, qemu-devel, Greg Kurz
On 10/22/19 9:22 AM, Cédric Le Goater wrote:
> From: Greg Kurz <groug@kaod.org>
>
> Since vCPUs aren't plugged into a bus, we manually register a reset
> handler for each vCPU. We also call this handler at realize time
> to ensure hot plugged vCPUs are reset before being exposed to the
> guest. This results in vCPUs being reset twice at machine reset.
> It doesn't break anything but it is slightly suboptimal and above
> all confusing.
>
> The hotplug path in device_set_realized() already knows how to reset
> a hotplugged device if the device reset handler is present. Implement
> one for sPAPR CPU cores that resets all vCPUs under a core.
>
> While here rename spapr_cpu_reset() to spapr_reset_vcpu() for
> consistency with spapr_realize_vcpu() and spapr_unrealize_vcpu().
>
> Signed-off-by: Greg Kurz <groug@kaod.org>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
> hw/ppc/spapr_cpu_core.c | 31 ++++++++++++++++++++++---------
> 1 file changed, 22 insertions(+), 9 deletions(-)
>
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 992f00da6540..5947e39b36ad 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -25,9 +25,8 @@
> #include "sysemu/hw_accel.h"
> #include "qemu/error-report.h"
>
> -static void spapr_cpu_reset(void *opaque)
> +static void spapr_reset_vcpu(PowerPCCPU *cpu)
> {
> - PowerPCCPU *cpu = opaque;
> CPUState *cs = CPU(cpu);
> CPUPPCState *env = &cpu->env;
> PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
> @@ -193,7 +192,6 @@ 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);
> }
> - qemu_unregister_reset(spapr_cpu_reset, cpu);
> if (spapr_cpu_state(cpu)->icp) {
> object_unparent(OBJECT(spapr_cpu_state(cpu)->icp));
> }
> @@ -204,12 +202,30 @@ static void spapr_unrealize_vcpu(PowerPCCPU *cpu, SpaprCpuCore *sc)
> object_unparent(OBJECT(cpu));
> }
>
> +static void spapr_cpu_core_reset(DeviceState *dev)
> +{
> + CPUCore *cc = CPU_CORE(dev);
> + SpaprCpuCore *sc = SPAPR_CPU_CORE(dev);
> + int i;
> +
> + for (i = 0; i < cc->nr_threads; i++) {
> + spapr_reset_vcpu(sc->threads[i]);
> + }
> +}
> +
> +static void spapr_cpu_core_reset_handler(void *opaque)
> +{
> + spapr_cpu_core_reset(opaque);
> +}
> +
> static void spapr_cpu_core_unrealize(DeviceState *dev, Error **errp)
> {
> SpaprCpuCore *sc = SPAPR_CPU_CORE(OBJECT(dev));
> CPUCore *cc = CPU_CORE(dev);
> int i;
>
> + qemu_unregister_reset(spapr_cpu_core_reset_handler, sc);
> +
> for (i = 0; i < cc->nr_threads; i++) {
> spapr_unrealize_vcpu(sc->threads[i], sc);
> }
> @@ -238,12 +254,6 @@ static void spapr_realize_vcpu(PowerPCCPU *cpu, SpaprMachineState *spapr,
> goto error_intc_create;
> }
>
> - /*
> - * FIXME: Hot-plugged CPUs are not reseted. Do it at realize.
> - */
> - qemu_register_reset(spapr_cpu_reset, cpu);
> - spapr_cpu_reset(cpu);
> -
> if (!sc->pre_3_0_migration) {
> vmstate_register(NULL, cs->cpu_index, &vmstate_spapr_cpu_state,
> cpu->machine_data);
> @@ -338,6 +348,8 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
> goto err_unrealize;
> }
> }
> +
> + qemu_register_reset(spapr_cpu_core_reset_handler, sc);
> return;
>
> err_unrealize:
> @@ -366,6 +378,7 @@ static void spapr_cpu_core_class_init(ObjectClass *oc, void *data)
>
> dc->realize = spapr_cpu_core_realize;
> dc->unrealize = spapr_cpu_core_unrealize;
> + dc->reset = spapr_cpu_core_reset;
> dc->props = spapr_cpu_core_properties;
> scc->cpu_type = data;
> }
>
Good cleanup!
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 3/4] ppc: reset the interrupt presenter from the CPU reset handler
2019-10-22 7:22 ` [PATCH v3 3/4] ppc: reset the interrupt presenter from the CPU reset handler Cédric Le Goater
@ 2019-10-22 9:08 ` Philippe Mathieu-Daudé
2019-10-22 11:43 ` Cédric Le Goater
2019-10-22 11:19 ` Greg Kurz
1 sibling, 1 reply; 12+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-10-22 9:08 UTC (permalink / raw)
To: Cédric Le Goater, David Gibson; +Cc: qemu-ppc, qemu-devel, Greg Kurz
Hi Cédric,
On 10/22/19 9:22 AM, Cédric Le Goater wrote:
> On the sPAPR machine and PowerNV machine, the interrupt presenters are
> created by a machine handler at the core level and are reseted
Typo "reset"
> independently. This is not consistent and it raises issues when it
> comes to handle hot-plugged CPUs. In that case, the presenters are not
> reseted. This is less of an issue in XICS, although a zero MFFR could
Same typo.
> be a concern, but in XIVE, the OS CAM line is not set and this breaks
> the presenting algorithm. The current code has workarounds which need
> a global cleanup.
>
> Extend the sPAPR IRQ backend and the PowerNV Chip class with a new
> cpu_intc_reset() handler called by the CPU reset handler and remove
> the XiveTCTX reset handler which is now redundant.
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
> 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 +
> hw/intc/spapr_xive.c | 9 +++++++++
> hw/intc/xics.c | 8 ++------
> hw/intc/xics_spapr.c | 7 +++++++
> hw/intc/xive.c | 12 +-----------
> hw/ppc/pnv.c | 18 ++++++++++++++++++
> hw/ppc/pnv_core.c | 8 ++++++++
> hw/ppc/spapr_cpu_core.c | 5 ++++-
> hw/ppc/spapr_irq.c | 14 ++++++++++++++
> 12 files changed, 68 insertions(+), 18 deletions(-)
>
> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
> index 1cdbe55bf86c..2a780e633f23 100644
> --- a/include/hw/ppc/pnv.h
> +++ b/include/hw/ppc/pnv.h
> @@ -111,6 +111,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);
> 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 5e150a667902..09232999b07e 100644
> --- a/include/hw/ppc/spapr_irq.h
> +++ b/include/hw/ppc/spapr_irq.h
> @@ -52,6 +52,7 @@ typedef struct SpaprInterruptControllerClass {
> */
> int (*cpu_intc_create)(SpaprInterruptController *intc,
> PowerPCCPU *cpu, Error **errp);
> + void (*cpu_intc_reset)(SpaprInterruptController *intc, PowerPCCPU *cpu);
> int (*claim_irq)(SpaprInterruptController *intc, int irq, bool lsi,
> Error **errp);
> void (*free_irq)(SpaprInterruptController *intc, int irq);
> @@ -68,6 +69,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_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 1e6a9300eb2b..602173c12250 100644
> --- a/include/hw/ppc/xics.h
> +++ b/include/hw/ppc/xics.h
> @@ -161,6 +161,7 @@ void icp_set_mfrr(ICPState *icp, uint8_t mfrr);
> uint32_t icp_accept(ICPState *ss);
> uint32_t icp_ipoll(ICPState *ss, uint32_t *mfrr);
> void icp_eoi(ICPState *icp, uint32_t xirr);
> +void icp_reset(ICPState *icp);
>
> void ics_write_xive(ICSState *ics, int nr, int server,
> uint8_t priority, uint8_t saved_priority);
> diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
> index fd3319bd3202..99381639f50c 100644
> --- a/include/hw/ppc/xive.h
> +++ b/include/hw/ppc/xive.h
> @@ -415,6 +415,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);
>
> static inline uint32_t xive_nvt_cam_line(uint8_t nvt_blk, uint32_t nvt_idx)
> {
> diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
> index ba32d2cc5b0f..20a8d8285f64 100644
> --- a/hw/intc/spapr_xive.c
> +++ b/hw/intc/spapr_xive.c
> @@ -553,6 +553,14 @@ static int spapr_xive_cpu_intc_create(SpaprInterruptController *intc,
> return 0;
> }
>
> +static void spapr_xive_cpu_intc_reset(SpaprInterruptController *intc,
> + PowerPCCPU *cpu)
> +{
> + XiveTCTX *tctx = spapr_cpu_state(cpu)->tctx;
> +
> + xive_tctx_reset(tctx);
> +}
> +
> static void spapr_xive_set_irq(SpaprInterruptController *intc, int irq, int val)
> {
> SpaprXive *xive = SPAPR_XIVE(intc);
> @@ -697,6 +705,7 @@ static void spapr_xive_class_init(ObjectClass *klass, void *data)
> sicc->activate = spapr_xive_activate;
> sicc->deactivate = spapr_xive_deactivate;
> sicc->cpu_intc_create = spapr_xive_cpu_intc_create;
> + sicc->cpu_intc_reset = spapr_xive_cpu_intc_reset;
> 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 b5ac408f7b74..6da05763f9db 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -274,10 +274,8 @@ static const VMStateDescription vmstate_icp_server = {
> },
> };
>
> -static void icp_reset_handler(void *dev)
> +void icp_reset(ICPState *icp)
> {
> - ICPState *icp = ICP(dev);
> -
> icp->xirr = 0;
> icp->pending_priority = 0xff;
> icp->mfrr = 0xff;
> @@ -288,7 +286,7 @@ static void icp_reset_handler(void *dev)
> if (kvm_irqchip_in_kernel()) {
> Error *local_err = NULL;
>
> - icp_set_kvm_state(ICP(dev), &local_err);
> + icp_set_kvm_state(icp, &local_err);
> if (local_err) {
> error_report_err(local_err);
> }
> @@ -351,7 +349,6 @@ static void icp_realize(DeviceState *dev, Error **errp)
> }
> }
>
> - qemu_register_reset(icp_reset_handler, dev);
> vmstate_register(NULL, icp->cs->cpu_index, &vmstate_icp_server, icp);
> }
>
> @@ -360,7 +357,6 @@ static void icp_unrealize(DeviceState *dev, Error **errp)
> ICPState *icp = ICP(dev);
>
> vmstate_unregister(NULL, &vmstate_icp_server, icp);
> - qemu_unregister_reset(icp_reset_handler, dev);
> }
>
> static void icp_class_init(ObjectClass *klass, void *data)
> diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
> index 4f64b9a9fc66..7418fb9f370c 100644
> --- a/hw/intc/xics_spapr.c
> +++ b/hw/intc/xics_spapr.c
> @@ -346,6 +346,12 @@ static int xics_spapr_cpu_intc_create(SpaprInterruptController *intc,
> return 0;
> }
>
> +static void xics_spapr_cpu_intc_reset(SpaprInterruptController *intc,
> + PowerPCCPU *cpu)
> +{
> + icp_reset(spapr_cpu_state(cpu)->icp);
> +}
> +
> static int xics_spapr_claim_irq(SpaprInterruptController *intc, int irq,
> bool lsi, Error **errp)
> {
> @@ -433,6 +439,7 @@ static void ics_spapr_class_init(ObjectClass *klass, void *data)
> sicc->activate = xics_spapr_activate;
> sicc->deactivate = xics_spapr_deactivate;
> sicc->cpu_intc_create = xics_spapr_cpu_intc_create;
> + sicc->cpu_intc_reset = xics_spapr_cpu_intc_reset;
> 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 d420c6571e14..f066be5eb5e3 100644
> --- a/hw/intc/xive.c
> +++ b/hw/intc/xive.c
> @@ -547,10 +547,8 @@ void xive_tctx_pic_print_info(XiveTCTX *tctx, Monitor *mon)
> }
> }
>
> -static void xive_tctx_reset(void *dev)
> +void xive_tctx_reset(XiveTCTX *tctx)
> {
> - XiveTCTX *tctx = XIVE_TCTX(dev);
> -
> memset(tctx->regs, 0, sizeof(tctx->regs));
>
> /* Set some defaults */
> @@ -607,13 +605,6 @@ static void xive_tctx_realize(DeviceState *dev, Error **errp)
> return;
> }
> }
> -
> - qemu_register_reset(xive_tctx_reset, dev);
> -}
> -
> -static void xive_tctx_unrealize(DeviceState *dev, Error **errp)
> -{
> - qemu_unregister_reset(xive_tctx_reset, dev);
> }
>
> static int vmstate_xive_tctx_pre_save(void *opaque)
> @@ -668,7 +659,6 @@ static void xive_tctx_class_init(ObjectClass *klass, void *data)
>
> dc->desc = "XIVE Interrupt Thread Context";
> dc->realize = xive_tctx_realize;
> - dc->unrealize = xive_tctx_unrealize;
> dc->vmsd = &vmstate_xive_tctx;
> /*
> * Reason: part of XIVE interrupt controller, needs to be wired up
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 7cf64b6d2533..4a51fb65a834 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -778,6 +778,13 @@ 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);
> +
> + icp_reset(ICP(pnv_cpu->intc));
> +}
> +
> /*
> * 0:48 Reserved - Read as zeroes
> * 49:52 Node ID
> @@ -815,6 +822,13 @@ static void pnv_chip_power9_intc_create(PnvChip *chip, PowerPCCPU *cpu,
> pnv_cpu->intc = obj;
> }
>
> +static void pnv_chip_power9_intc_reset(PnvChip *chip, PowerPCCPU *cpu)
> +{
> + PnvCPUState *pnv_cpu = pnv_cpu_state(cpu);
> +
> + xive_tctx_reset(XIVE_TCTX(pnv_cpu->intc));
> +}
> +
> /*
> * Allowed core identifiers on a POWER8 Processor Chip :
> *
> @@ -984,6 +998,7 @@ static void pnv_chip_power8e_class_init(ObjectClass *klass, void *data)
> k->cores_mask = POWER8E_CORE_MASK;
> 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->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;
> @@ -1003,6 +1018,7 @@ static void pnv_chip_power8_class_init(ObjectClass *klass, void *data)
> k->cores_mask = POWER8_CORE_MASK;
> 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->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;
> @@ -1022,6 +1038,7 @@ static void pnv_chip_power8nvl_class_init(ObjectClass *klass, void *data)
> k->cores_mask = POWER8_CORE_MASK;
> 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->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;
> @@ -1191,6 +1208,7 @@ static void pnv_chip_power9_class_init(ObjectClass *klass, void *data)
> k->cores_mask = POWER9_CORE_MASK;
> 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->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 b1a7489e7abf..f36cb39dbf77 100644
> --- a/hw/ppc/pnv_core.c
> +++ b/hw/ppc/pnv_core.c
> @@ -45,6 +45,8 @@ static void pnv_cpu_reset(void *opaque)
> PowerPCCPU *cpu = opaque;
> CPUState *cs = CPU(cpu);
> CPUPPCState *env = &cpu->env;
> + PnvChipClass *pcc;
> + Object *chip;
>
> cpu_reset(cs);
>
> @@ -55,6 +57,10 @@ static void pnv_cpu_reset(void *opaque)
> env->gpr[3] = PNV_FDT_ADDR;
> env->nip = 0x10;
> env->msr |= MSR_HVB; /* Hypervisor mode */
> +
> + chip = object_property_get_link(OBJECT(cpu), "chip", &error_fatal);
Why not add a PnvChip *chip field in PnvCore?
Then you only get it in realize() and it is available in reset().
Rest of your patch looks OK from a QOM PoV.
> + pcc = PNV_CHIP_GET_CLASS(chip);
> + pcc->intc_reset(PNV_CHIP(chip), cpu);
> }
>
> /*
> @@ -169,6 +175,8 @@ static void pnv_realize_vcpu(PowerPCCPU *cpu, PnvChip *chip, Error **errp)
> Error *local_err = NULL;
> PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip);
>
> + object_property_add_const_link(OBJECT(cpu), "chip",
> + OBJECT(chip), &error_fatal);
> object_property_set_bool(OBJECT(cpu), true, "realized", &local_err);
> if (local_err) {
> error_propagate(errp, local_err);
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 5947e39b36ad..d2903c2d0f22 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -32,6 +32,7 @@ static void spapr_reset_vcpu(PowerPCCPU *cpu)
> PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
> SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu);
> target_ulong lpcr;
> + SpaprMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
>
> cpu_reset(cs);
>
> @@ -76,9 +77,11 @@ static void spapr_reset_vcpu(PowerPCCPU *cpu)
> spapr_cpu->dtl_addr = 0;
> spapr_cpu->dtl_size = 0;
>
> - spapr_caps_cpu_apply(SPAPR_MACHINE(qdev_get_machine()), cpu);
> + spapr_caps_cpu_apply(spapr, cpu);
>
> kvm_check_mmu(cpu, &error_fatal);
> +
> + spapr_irq_cpu_intc_reset(spapr, cpu);
> }
>
> void spapr_cpu_set_entry_state(PowerPCCPU *cpu, target_ulong nip, target_ulong r3)
> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> index 234d1073e518..b941608b69ba 100644
> --- a/hw/ppc/spapr_irq.c
> +++ b/hw/ppc/spapr_irq.c
> @@ -220,6 +220,20 @@ int spapr_irq_cpu_intc_create(SpaprMachineState *spapr,
> return 0;
> }
>
> +void spapr_irq_cpu_intc_reset(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_reset(intc, cpu);
> + }
> + }
> +}
> +
> static void spapr_set_irq(void *opaque, int irq, int level)
> {
> SpaprMachineState *spapr = SPAPR_MACHINE(opaque);
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 1/4] spapr: move CPU reset after presenter creation
2019-10-22 7:22 ` [PATCH v3 1/4] spapr: move CPU reset after presenter creation Cédric Le Goater
2019-10-22 8:53 ` Philippe Mathieu-Daudé
@ 2019-10-22 9:53 ` Greg Kurz
1 sibling, 0 replies; 12+ messages in thread
From: Greg Kurz @ 2019-10-22 9:53 UTC (permalink / raw)
To: Cédric Le Goater; +Cc: qemu-ppc, qemu-devel, David Gibson
On Tue, 22 Oct 2019 09:22:43 +0200
Cédric Le Goater <clg@kaod.org> wrote:
> spapr_realize_vcpu() is modified to call the CPU reset only after the
> the intc presenter has been created.
>
Maybe indicate why this change is needed ?
Anyway,
Reviewed-by: Greg Kurz <groug@kaod.org>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
> hw/ppc/spapr_cpu_core.c | 14 ++++++++------
> 1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 3e4302c7d596..992f00da6540 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -234,13 +234,16 @@ static void spapr_realize_vcpu(PowerPCCPU *cpu, SpaprMachineState *spapr,
> cpu_ppc_set_vhyp(cpu, PPC_VIRTUAL_HYPERVISOR(spapr));
> kvmppc_set_papr(cpu);
>
> - qemu_register_reset(spapr_cpu_reset, cpu);
> - spapr_cpu_reset(cpu);
> -
> if (spapr_irq_cpu_intc_create(spapr, cpu, &local_err) < 0) {
> - goto error_unregister;
> + goto error_intc_create;
> }
>
> + /*
> + * FIXME: Hot-plugged CPUs are not reseted. Do it at realize.
> + */
> + qemu_register_reset(spapr_cpu_reset, cpu);
> + spapr_cpu_reset(cpu);
> +
> if (!sc->pre_3_0_migration) {
> vmstate_register(NULL, cs->cpu_index, &vmstate_spapr_cpu_state,
> cpu->machine_data);
> @@ -248,8 +251,7 @@ static void spapr_realize_vcpu(PowerPCCPU *cpu, SpaprMachineState *spapr,
>
> return;
>
> -error_unregister:
> - qemu_unregister_reset(spapr_cpu_reset, cpu);
> +error_intc_create:
> cpu_remove_sync(CPU(cpu));
> error:
> error_propagate(errp, local_err);
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 3/4] ppc: reset the interrupt presenter from the CPU reset handler
2019-10-22 7:22 ` [PATCH v3 3/4] ppc: reset the interrupt presenter from the CPU reset handler Cédric Le Goater
2019-10-22 9:08 ` Philippe Mathieu-Daudé
@ 2019-10-22 11:19 ` Greg Kurz
1 sibling, 0 replies; 12+ messages in thread
From: Greg Kurz @ 2019-10-22 11:19 UTC (permalink / raw)
To: Cédric Le Goater; +Cc: qemu-ppc, qemu-devel, David Gibson
On Tue, 22 Oct 2019 09:22:45 +0200
Cédric Le Goater <clg@kaod.org> wrote:
> On the sPAPR machine and PowerNV machine, the interrupt presenters are
> created by a machine handler at the core level and are reseted
> independently. This is not consistent and it raises issues when it
> comes to handle hot-plugged CPUs. In that case, the presenters are not
> reseted. This is less of an issue in XICS, although a zero MFFR could
> be a concern, but in XIVE, the OS CAM line is not set and this breaks
> the presenting algorithm. The current code has workarounds which need
> a global cleanup.
>
> Extend the sPAPR IRQ backend and the PowerNV Chip class with a new
> cpu_intc_reset() handler called by the CPU reset handler and remove
> the XiveTCTX reset handler which is now redundant.
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
Reviewed-by: Greg Kurz <groug@kaod.org>
> 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 +
> hw/intc/spapr_xive.c | 9 +++++++++
> hw/intc/xics.c | 8 ++------
> hw/intc/xics_spapr.c | 7 +++++++
> hw/intc/xive.c | 12 +-----------
> hw/ppc/pnv.c | 18 ++++++++++++++++++
> hw/ppc/pnv_core.c | 8 ++++++++
> hw/ppc/spapr_cpu_core.c | 5 ++++-
> hw/ppc/spapr_irq.c | 14 ++++++++++++++
> 12 files changed, 68 insertions(+), 18 deletions(-)
>
> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
> index 1cdbe55bf86c..2a780e633f23 100644
> --- a/include/hw/ppc/pnv.h
> +++ b/include/hw/ppc/pnv.h
> @@ -111,6 +111,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);
> 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 5e150a667902..09232999b07e 100644
> --- a/include/hw/ppc/spapr_irq.h
> +++ b/include/hw/ppc/spapr_irq.h
> @@ -52,6 +52,7 @@ typedef struct SpaprInterruptControllerClass {
> */
> int (*cpu_intc_create)(SpaprInterruptController *intc,
> PowerPCCPU *cpu, Error **errp);
> + void (*cpu_intc_reset)(SpaprInterruptController *intc, PowerPCCPU *cpu);
> int (*claim_irq)(SpaprInterruptController *intc, int irq, bool lsi,
> Error **errp);
> void (*free_irq)(SpaprInterruptController *intc, int irq);
> @@ -68,6 +69,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_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 1e6a9300eb2b..602173c12250 100644
> --- a/include/hw/ppc/xics.h
> +++ b/include/hw/ppc/xics.h
> @@ -161,6 +161,7 @@ void icp_set_mfrr(ICPState *icp, uint8_t mfrr);
> uint32_t icp_accept(ICPState *ss);
> uint32_t icp_ipoll(ICPState *ss, uint32_t *mfrr);
> void icp_eoi(ICPState *icp, uint32_t xirr);
> +void icp_reset(ICPState *icp);
>
> void ics_write_xive(ICSState *ics, int nr, int server,
> uint8_t priority, uint8_t saved_priority);
> diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
> index fd3319bd3202..99381639f50c 100644
> --- a/include/hw/ppc/xive.h
> +++ b/include/hw/ppc/xive.h
> @@ -415,6 +415,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);
>
> static inline uint32_t xive_nvt_cam_line(uint8_t nvt_blk, uint32_t nvt_idx)
> {
> diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
> index ba32d2cc5b0f..20a8d8285f64 100644
> --- a/hw/intc/spapr_xive.c
> +++ b/hw/intc/spapr_xive.c
> @@ -553,6 +553,14 @@ static int spapr_xive_cpu_intc_create(SpaprInterruptController *intc,
> return 0;
> }
>
> +static void spapr_xive_cpu_intc_reset(SpaprInterruptController *intc,
> + PowerPCCPU *cpu)
> +{
> + XiveTCTX *tctx = spapr_cpu_state(cpu)->tctx;
> +
> + xive_tctx_reset(tctx);
> +}
> +
> static void spapr_xive_set_irq(SpaprInterruptController *intc, int irq, int val)
> {
> SpaprXive *xive = SPAPR_XIVE(intc);
> @@ -697,6 +705,7 @@ static void spapr_xive_class_init(ObjectClass *klass, void *data)
> sicc->activate = spapr_xive_activate;
> sicc->deactivate = spapr_xive_deactivate;
> sicc->cpu_intc_create = spapr_xive_cpu_intc_create;
> + sicc->cpu_intc_reset = spapr_xive_cpu_intc_reset;
> 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 b5ac408f7b74..6da05763f9db 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -274,10 +274,8 @@ static const VMStateDescription vmstate_icp_server = {
> },
> };
>
> -static void icp_reset_handler(void *dev)
> +void icp_reset(ICPState *icp)
> {
> - ICPState *icp = ICP(dev);
> -
> icp->xirr = 0;
> icp->pending_priority = 0xff;
> icp->mfrr = 0xff;
> @@ -288,7 +286,7 @@ static void icp_reset_handler(void *dev)
> if (kvm_irqchip_in_kernel()) {
> Error *local_err = NULL;
>
> - icp_set_kvm_state(ICP(dev), &local_err);
> + icp_set_kvm_state(icp, &local_err);
> if (local_err) {
> error_report_err(local_err);
> }
> @@ -351,7 +349,6 @@ static void icp_realize(DeviceState *dev, Error **errp)
> }
> }
>
> - qemu_register_reset(icp_reset_handler, dev);
> vmstate_register(NULL, icp->cs->cpu_index, &vmstate_icp_server, icp);
> }
>
> @@ -360,7 +357,6 @@ static void icp_unrealize(DeviceState *dev, Error **errp)
> ICPState *icp = ICP(dev);
>
> vmstate_unregister(NULL, &vmstate_icp_server, icp);
> - qemu_unregister_reset(icp_reset_handler, dev);
> }
>
> static void icp_class_init(ObjectClass *klass, void *data)
> diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
> index 4f64b9a9fc66..7418fb9f370c 100644
> --- a/hw/intc/xics_spapr.c
> +++ b/hw/intc/xics_spapr.c
> @@ -346,6 +346,12 @@ static int xics_spapr_cpu_intc_create(SpaprInterruptController *intc,
> return 0;
> }
>
> +static void xics_spapr_cpu_intc_reset(SpaprInterruptController *intc,
> + PowerPCCPU *cpu)
> +{
> + icp_reset(spapr_cpu_state(cpu)->icp);
> +}
> +
> static int xics_spapr_claim_irq(SpaprInterruptController *intc, int irq,
> bool lsi, Error **errp)
> {
> @@ -433,6 +439,7 @@ static void ics_spapr_class_init(ObjectClass *klass, void *data)
> sicc->activate = xics_spapr_activate;
> sicc->deactivate = xics_spapr_deactivate;
> sicc->cpu_intc_create = xics_spapr_cpu_intc_create;
> + sicc->cpu_intc_reset = xics_spapr_cpu_intc_reset;
> 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 d420c6571e14..f066be5eb5e3 100644
> --- a/hw/intc/xive.c
> +++ b/hw/intc/xive.c
> @@ -547,10 +547,8 @@ void xive_tctx_pic_print_info(XiveTCTX *tctx, Monitor *mon)
> }
> }
>
> -static void xive_tctx_reset(void *dev)
> +void xive_tctx_reset(XiveTCTX *tctx)
> {
> - XiveTCTX *tctx = XIVE_TCTX(dev);
> -
> memset(tctx->regs, 0, sizeof(tctx->regs));
>
> /* Set some defaults */
> @@ -607,13 +605,6 @@ static void xive_tctx_realize(DeviceState *dev, Error **errp)
> return;
> }
> }
> -
> - qemu_register_reset(xive_tctx_reset, dev);
> -}
> -
> -static void xive_tctx_unrealize(DeviceState *dev, Error **errp)
> -{
> - qemu_unregister_reset(xive_tctx_reset, dev);
> }
>
> static int vmstate_xive_tctx_pre_save(void *opaque)
> @@ -668,7 +659,6 @@ static void xive_tctx_class_init(ObjectClass *klass, void *data)
>
> dc->desc = "XIVE Interrupt Thread Context";
> dc->realize = xive_tctx_realize;
> - dc->unrealize = xive_tctx_unrealize;
> dc->vmsd = &vmstate_xive_tctx;
> /*
> * Reason: part of XIVE interrupt controller, needs to be wired up
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 7cf64b6d2533..4a51fb65a834 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -778,6 +778,13 @@ 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);
> +
> + icp_reset(ICP(pnv_cpu->intc));
> +}
> +
> /*
> * 0:48 Reserved - Read as zeroes
> * 49:52 Node ID
> @@ -815,6 +822,13 @@ static void pnv_chip_power9_intc_create(PnvChip *chip, PowerPCCPU *cpu,
> pnv_cpu->intc = obj;
> }
>
> +static void pnv_chip_power9_intc_reset(PnvChip *chip, PowerPCCPU *cpu)
> +{
> + PnvCPUState *pnv_cpu = pnv_cpu_state(cpu);
> +
> + xive_tctx_reset(XIVE_TCTX(pnv_cpu->intc));
> +}
> +
> /*
> * Allowed core identifiers on a POWER8 Processor Chip :
> *
> @@ -984,6 +998,7 @@ static void pnv_chip_power8e_class_init(ObjectClass *klass, void *data)
> k->cores_mask = POWER8E_CORE_MASK;
> 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->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;
> @@ -1003,6 +1018,7 @@ static void pnv_chip_power8_class_init(ObjectClass *klass, void *data)
> k->cores_mask = POWER8_CORE_MASK;
> 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->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;
> @@ -1022,6 +1038,7 @@ static void pnv_chip_power8nvl_class_init(ObjectClass *klass, void *data)
> k->cores_mask = POWER8_CORE_MASK;
> 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->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;
> @@ -1191,6 +1208,7 @@ static void pnv_chip_power9_class_init(ObjectClass *klass, void *data)
> k->cores_mask = POWER9_CORE_MASK;
> 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->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 b1a7489e7abf..f36cb39dbf77 100644
> --- a/hw/ppc/pnv_core.c
> +++ b/hw/ppc/pnv_core.c
> @@ -45,6 +45,8 @@ static void pnv_cpu_reset(void *opaque)
> PowerPCCPU *cpu = opaque;
> CPUState *cs = CPU(cpu);
> CPUPPCState *env = &cpu->env;
> + PnvChipClass *pcc;
> + Object *chip;
>
> cpu_reset(cs);
>
> @@ -55,6 +57,10 @@ static void pnv_cpu_reset(void *opaque)
> env->gpr[3] = PNV_FDT_ADDR;
> env->nip = 0x10;
> env->msr |= MSR_HVB; /* Hypervisor mode */
> +
> + chip = object_property_get_link(OBJECT(cpu), "chip", &error_fatal);
> + pcc = PNV_CHIP_GET_CLASS(chip);
> + pcc->intc_reset(PNV_CHIP(chip), cpu);
> }
>
> /*
> @@ -169,6 +175,8 @@ static void pnv_realize_vcpu(PowerPCCPU *cpu, PnvChip *chip, Error **errp)
> Error *local_err = NULL;
> PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip);
>
> + object_property_add_const_link(OBJECT(cpu), "chip",
> + OBJECT(chip), &error_fatal);
> object_property_set_bool(OBJECT(cpu), true, "realized", &local_err);
> if (local_err) {
> error_propagate(errp, local_err);
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 5947e39b36ad..d2903c2d0f22 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -32,6 +32,7 @@ static void spapr_reset_vcpu(PowerPCCPU *cpu)
> PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
> SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu);
> target_ulong lpcr;
> + SpaprMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
>
> cpu_reset(cs);
>
> @@ -76,9 +77,11 @@ static void spapr_reset_vcpu(PowerPCCPU *cpu)
> spapr_cpu->dtl_addr = 0;
> spapr_cpu->dtl_size = 0;
>
> - spapr_caps_cpu_apply(SPAPR_MACHINE(qdev_get_machine()), cpu);
> + spapr_caps_cpu_apply(spapr, cpu);
>
> kvm_check_mmu(cpu, &error_fatal);
> +
> + spapr_irq_cpu_intc_reset(spapr, cpu);
> }
>
> void spapr_cpu_set_entry_state(PowerPCCPU *cpu, target_ulong nip, target_ulong r3)
> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> index 234d1073e518..b941608b69ba 100644
> --- a/hw/ppc/spapr_irq.c
> +++ b/hw/ppc/spapr_irq.c
> @@ -220,6 +220,20 @@ int spapr_irq_cpu_intc_create(SpaprMachineState *spapr,
> return 0;
> }
>
> +void spapr_irq_cpu_intc_reset(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_reset(intc, cpu);
> + }
> + }
> +}
> +
> static void spapr_set_irq(void *opaque, int irq, int level)
> {
> SpaprMachineState *spapr = SPAPR_MACHINE(opaque);
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 4/4] spapr/xive: Set the OS CAM line at reset
2019-10-22 7:22 ` [PATCH v3 4/4] spapr/xive: Set the OS CAM line at reset Cédric Le Goater
@ 2019-10-22 11:27 ` Greg Kurz
0 siblings, 0 replies; 12+ messages in thread
From: Greg Kurz @ 2019-10-22 11:27 UTC (permalink / raw)
To: Cédric Le Goater; +Cc: qemu-ppc, qemu-devel, David Gibson
On Tue, 22 Oct 2019 09:22:46 +0200
Cédric Le Goater <clg@kaod.org> wrote:
> When a Virtual Processor is scheduled to run on a HW thread, the
> hypervisor pushes its identifier in the OS CAM line. When running with
> kernel_irqchip=off, QEMU needs to emulate the same behavior.
>
> Set the OS CAM line when the interrupt presenter of the sPAPR core is
> reseted. This will also cover the case of hot-plugged CPUs.
>
> This change also has the benefit to remove the use of CPU_FOREACH()
> which can be unsafe.
>
CPU_FOREACH() is indeed an unsafe way to loop on presenters. There
are some other users in XICS and XIVE that do cause QEMU to crash.
I shall send fixes as soon as this series reaches ppc-for-4.2.
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
Reviewed-by: Greg Kurz <groug@kaod.org>
> include/hw/ppc/spapr_xive.h | 1 -
> hw/intc/spapr_xive.c | 48 +++++++++++++------------------------
> 2 files changed, 17 insertions(+), 32 deletions(-)
>
> diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h
> index d84bd5c229f0..742b7e834f2a 100644
> --- a/include/hw/ppc/spapr_xive.h
> +++ b/include/hw/ppc/spapr_xive.h
> @@ -57,7 +57,6 @@ typedef struct SpaprXive {
> void spapr_xive_pic_print_info(SpaprXive *xive, Monitor *mon);
>
> void spapr_xive_hcall_init(SpaprMachineState *spapr);
> -void spapr_xive_set_tctx_os_cam(XiveTCTX *tctx);
> void spapr_xive_mmio_set_enabled(SpaprXive *xive, bool enable);
> void spapr_xive_map_mmio(SpaprXive *xive);
>
> diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
> index 20a8d8285f64..d8e1291905c3 100644
> --- a/hw/intc/spapr_xive.c
> +++ b/hw/intc/spapr_xive.c
> @@ -205,23 +205,6 @@ void spapr_xive_mmio_set_enabled(SpaprXive *xive, bool enable)
> memory_region_set_enabled(&xive->end_source.esb_mmio, false);
> }
>
> -/*
> - * When a Virtual Processor is scheduled to run on a HW thread, the
> - * hypervisor pushes its identifier in the OS CAM line. Emulate the
> - * same behavior under QEMU.
> - */
> -void spapr_xive_set_tctx_os_cam(XiveTCTX *tctx)
> -{
> - uint8_t nvt_blk;
> - uint32_t nvt_idx;
> - uint32_t nvt_cam;
> -
> - spapr_xive_cpu_to_nvt(POWERPC_CPU(tctx->cs), &nvt_blk, &nvt_idx);
> -
> - nvt_cam = cpu_to_be32(TM_QW1W2_VO | xive_nvt_cam_line(nvt_blk, nvt_idx));
> - memcpy(&tctx->regs[TM_QW1_OS + TM_WORD2], &nvt_cam, 4);
> -}
> -
> static void spapr_xive_end_reset(XiveEND *end)
> {
> memset(end, 0, sizeof(*end));
> @@ -544,21 +527,32 @@ static int spapr_xive_cpu_intc_create(SpaprInterruptController *intc,
> }
>
> spapr_cpu->tctx = XIVE_TCTX(obj);
> -
> - /*
> - * (TCG) Early setting the OS CAM line for hotplugged CPUs as they
> - * don't beneficiate from the reset of the XIVE IRQ backend
> - */
> - spapr_xive_set_tctx_os_cam(spapr_cpu->tctx);
> return 0;
> }
>
> +static void xive_tctx_set_os_cam(XiveTCTX *tctx, uint32_t os_cam)
> +{
> + uint32_t qw1w2 = cpu_to_be32(TM_QW1W2_VO | os_cam);
> + memcpy(&tctx->regs[TM_QW1_OS + TM_WORD2], &qw1w2, 4);
> +}
> +
> static void spapr_xive_cpu_intc_reset(SpaprInterruptController *intc,
> PowerPCCPU *cpu)
> {
> XiveTCTX *tctx = spapr_cpu_state(cpu)->tctx;
> + uint8_t nvt_blk;
> + uint32_t nvt_idx;
>
> xive_tctx_reset(tctx);
> +
> + /*
> + * When a Virtual Processor is scheduled to run on a HW thread,
> + * the hypervisor pushes its identifier in the OS CAM line.
> + * Emulate the same behavior under QEMU.
> + */
> + spapr_xive_cpu_to_nvt(cpu, &nvt_blk, &nvt_idx);
> +
> + xive_tctx_set_os_cam(tctx, xive_nvt_cam_line(nvt_blk, nvt_idx));
> }
>
> static void spapr_xive_set_irq(SpaprInterruptController *intc, int irq, int val)
> @@ -651,14 +645,6 @@ static void spapr_xive_dt(SpaprInterruptController *intc, uint32_t nr_servers,
> static int spapr_xive_activate(SpaprInterruptController *intc, Error **errp)
> {
> SpaprXive *xive = SPAPR_XIVE(intc);
> - CPUState *cs;
> -
> - CPU_FOREACH(cs) {
> - PowerPCCPU *cpu = POWERPC_CPU(cs);
> -
> - /* (TCG) Set the OS CAM line of the thread interrupt context. */
> - spapr_xive_set_tctx_os_cam(spapr_cpu_state(cpu)->tctx);
> - }
>
> if (kvm_enabled()) {
> int rc = spapr_irq_init_kvm(kvmppc_xive_connect, intc, errp);
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 3/4] ppc: reset the interrupt presenter from the CPU reset handler
2019-10-22 9:08 ` Philippe Mathieu-Daudé
@ 2019-10-22 11:43 ` Cédric Le Goater
0 siblings, 0 replies; 12+ messages in thread
From: Cédric Le Goater @ 2019-10-22 11:43 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, David Gibson; +Cc: qemu-ppc, qemu-devel, Greg Kurz
On 22/10/2019 11:08, Philippe Mathieu-Daudé wrote:
> Hi Cédric,
>
> On 10/22/19 9:22 AM, Cédric Le Goater wrote:
>> On the sPAPR machine and PowerNV machine, the interrupt presenters are
>> created by a machine handler at the core level and are reseted
>
> Typo "reset"
>
>> independently. This is not consistent and it raises issues when it
>> comes to handle hot-plugged CPUs. In that case, the presenters are not
>> reseted. This is less of an issue in XICS, although a zero MFFR could
>
> Same typo.
>
>> be a concern, but in XIVE, the OS CAM line is not set and this breaks
>> the presenting algorithm. The current code has workarounds which need
>> a global cleanup.
>>
>> Extend the sPAPR IRQ backend and the PowerNV Chip class with a new
>> cpu_intc_reset() handler called by the CPU reset handler and remove
>> the XiveTCTX reset handler which is now redundant.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>> 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 +
>> hw/intc/spapr_xive.c | 9 +++++++++
>> hw/intc/xics.c | 8 ++------
>> hw/intc/xics_spapr.c | 7 +++++++
>> hw/intc/xive.c | 12 +-----------
>> hw/ppc/pnv.c | 18 ++++++++++++++++++
>> hw/ppc/pnv_core.c | 8 ++++++++
>> hw/ppc/spapr_cpu_core.c | 5 ++++-
>> hw/ppc/spapr_irq.c | 14 ++++++++++++++
>> 12 files changed, 68 insertions(+), 18 deletions(-)
>>
>> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
>> index 1cdbe55bf86c..2a780e633f23 100644
>> --- a/include/hw/ppc/pnv.h
>> +++ b/include/hw/ppc/pnv.h
>> @@ -111,6 +111,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);
>> 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 5e150a667902..09232999b07e 100644
>> --- a/include/hw/ppc/spapr_irq.h
>> +++ b/include/hw/ppc/spapr_irq.h
>> @@ -52,6 +52,7 @@ typedef struct SpaprInterruptControllerClass {
>> */
>> int (*cpu_intc_create)(SpaprInterruptController *intc,
>> PowerPCCPU *cpu, Error **errp);
>> + void (*cpu_intc_reset)(SpaprInterruptController *intc, PowerPCCPU *cpu);
>> int (*claim_irq)(SpaprInterruptController *intc, int irq, bool lsi,
>> Error **errp);
>> void (*free_irq)(SpaprInterruptController *intc, int irq);
>> @@ -68,6 +69,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_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 1e6a9300eb2b..602173c12250 100644
>> --- a/include/hw/ppc/xics.h
>> +++ b/include/hw/ppc/xics.h
>> @@ -161,6 +161,7 @@ void icp_set_mfrr(ICPState *icp, uint8_t mfrr);
>> uint32_t icp_accept(ICPState *ss);
>> uint32_t icp_ipoll(ICPState *ss, uint32_t *mfrr);
>> void icp_eoi(ICPState *icp, uint32_t xirr);
>> +void icp_reset(ICPState *icp);
>> void ics_write_xive(ICSState *ics, int nr, int server,
>> uint8_t priority, uint8_t saved_priority);
>> diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
>> index fd3319bd3202..99381639f50c 100644
>> --- a/include/hw/ppc/xive.h
>> +++ b/include/hw/ppc/xive.h
>> @@ -415,6 +415,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);
>> static inline uint32_t xive_nvt_cam_line(uint8_t nvt_blk, uint32_t nvt_idx)
>> {
>> diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
>> index ba32d2cc5b0f..20a8d8285f64 100644
>> --- a/hw/intc/spapr_xive.c
>> +++ b/hw/intc/spapr_xive.c
>> @@ -553,6 +553,14 @@ static int spapr_xive_cpu_intc_create(SpaprInterruptController *intc,
>> return 0;
>> }
>> +static void spapr_xive_cpu_intc_reset(SpaprInterruptController *intc,
>> + PowerPCCPU *cpu)
>> +{
>> + XiveTCTX *tctx = spapr_cpu_state(cpu)->tctx;
>> +
>> + xive_tctx_reset(tctx);
>> +}
>> +
>> static void spapr_xive_set_irq(SpaprInterruptController *intc, int irq, int val)
>> {
>> SpaprXive *xive = SPAPR_XIVE(intc);
>> @@ -697,6 +705,7 @@ static void spapr_xive_class_init(ObjectClass *klass, void *data)
>> sicc->activate = spapr_xive_activate;
>> sicc->deactivate = spapr_xive_deactivate;
>> sicc->cpu_intc_create = spapr_xive_cpu_intc_create;
>> + sicc->cpu_intc_reset = spapr_xive_cpu_intc_reset;
>> 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 b5ac408f7b74..6da05763f9db 100644
>> --- a/hw/intc/xics.c
>> +++ b/hw/intc/xics.c
>> @@ -274,10 +274,8 @@ static const VMStateDescription vmstate_icp_server = {
>> },
>> };
>> -static void icp_reset_handler(void *dev)
>> +void icp_reset(ICPState *icp)
>> {
>> - ICPState *icp = ICP(dev);
>> -
>> icp->xirr = 0;
>> icp->pending_priority = 0xff;
>> icp->mfrr = 0xff;
>> @@ -288,7 +286,7 @@ static void icp_reset_handler(void *dev)
>> if (kvm_irqchip_in_kernel()) {
>> Error *local_err = NULL;
>> - icp_set_kvm_state(ICP(dev), &local_err);
>> + icp_set_kvm_state(icp, &local_err);
>> if (local_err) {
>> error_report_err(local_err);
>> }
>> @@ -351,7 +349,6 @@ static void icp_realize(DeviceState *dev, Error **errp)
>> }
>> }
>> - qemu_register_reset(icp_reset_handler, dev);
>> vmstate_register(NULL, icp->cs->cpu_index, &vmstate_icp_server, icp);
>> }
>> @@ -360,7 +357,6 @@ static void icp_unrealize(DeviceState *dev, Error **errp)
>> ICPState *icp = ICP(dev);
>> vmstate_unregister(NULL, &vmstate_icp_server, icp);
>> - qemu_unregister_reset(icp_reset_handler, dev);
>> }
>> static void icp_class_init(ObjectClass *klass, void *data)
>> diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
>> index 4f64b9a9fc66..7418fb9f370c 100644
>> --- a/hw/intc/xics_spapr.c
>> +++ b/hw/intc/xics_spapr.c
>> @@ -346,6 +346,12 @@ static int xics_spapr_cpu_intc_create(SpaprInterruptController *intc,
>> return 0;
>> }
>> +static void xics_spapr_cpu_intc_reset(SpaprInterruptController *intc,
>> + PowerPCCPU *cpu)
>> +{
>> + icp_reset(spapr_cpu_state(cpu)->icp);
>> +}
>> +
>> static int xics_spapr_claim_irq(SpaprInterruptController *intc, int irq,
>> bool lsi, Error **errp)
>> {
>> @@ -433,6 +439,7 @@ static void ics_spapr_class_init(ObjectClass *klass, void *data)
>> sicc->activate = xics_spapr_activate;
>> sicc->deactivate = xics_spapr_deactivate;
>> sicc->cpu_intc_create = xics_spapr_cpu_intc_create;
>> + sicc->cpu_intc_reset = xics_spapr_cpu_intc_reset;
>> 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 d420c6571e14..f066be5eb5e3 100644
>> --- a/hw/intc/xive.c
>> +++ b/hw/intc/xive.c
>> @@ -547,10 +547,8 @@ void xive_tctx_pic_print_info(XiveTCTX *tctx, Monitor *mon)
>> }
>> }
>> -static void xive_tctx_reset(void *dev)
>> +void xive_tctx_reset(XiveTCTX *tctx)
>> {
>> - XiveTCTX *tctx = XIVE_TCTX(dev);
>> -
>> memset(tctx->regs, 0, sizeof(tctx->regs));
>> /* Set some defaults */
>> @@ -607,13 +605,6 @@ static void xive_tctx_realize(DeviceState *dev, Error **errp)
>> return;
>> }
>> }
>> -
>> - qemu_register_reset(xive_tctx_reset, dev);
>> -}
>> -
>> -static void xive_tctx_unrealize(DeviceState *dev, Error **errp)
>> -{
>> - qemu_unregister_reset(xive_tctx_reset, dev);
>> }
>> static int vmstate_xive_tctx_pre_save(void *opaque)
>> @@ -668,7 +659,6 @@ static void xive_tctx_class_init(ObjectClass *klass, void *data)
>> dc->desc = "XIVE Interrupt Thread Context";
>> dc->realize = xive_tctx_realize;
>> - dc->unrealize = xive_tctx_unrealize;
>> dc->vmsd = &vmstate_xive_tctx;
>> /*
>> * Reason: part of XIVE interrupt controller, needs to be wired up
>> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
>> index 7cf64b6d2533..4a51fb65a834 100644
>> --- a/hw/ppc/pnv.c
>> +++ b/hw/ppc/pnv.c
>> @@ -778,6 +778,13 @@ 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);
>> +
>> + icp_reset(ICP(pnv_cpu->intc));
>> +}
>> +
>> /*
>> * 0:48 Reserved - Read as zeroes
>> * 49:52 Node ID
>> @@ -815,6 +822,13 @@ static void pnv_chip_power9_intc_create(PnvChip *chip, PowerPCCPU *cpu,
>> pnv_cpu->intc = obj;
>> }
>> +static void pnv_chip_power9_intc_reset(PnvChip *chip, PowerPCCPU *cpu)
>> +{
>> + PnvCPUState *pnv_cpu = pnv_cpu_state(cpu);
>> +
>> + xive_tctx_reset(XIVE_TCTX(pnv_cpu->intc));
>> +}
>> +
>> /*
>> * Allowed core identifiers on a POWER8 Processor Chip :
>> *
>> @@ -984,6 +998,7 @@ static void pnv_chip_power8e_class_init(ObjectClass *klass, void *data)
>> k->cores_mask = POWER8E_CORE_MASK;
>> 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->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;
>> @@ -1003,6 +1018,7 @@ static void pnv_chip_power8_class_init(ObjectClass *klass, void *data)
>> k->cores_mask = POWER8_CORE_MASK;
>> 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->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;
>> @@ -1022,6 +1038,7 @@ static void pnv_chip_power8nvl_class_init(ObjectClass *klass, void *data)
>> k->cores_mask = POWER8_CORE_MASK;
>> 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->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;
>> @@ -1191,6 +1208,7 @@ static void pnv_chip_power9_class_init(ObjectClass *klass, void *data)
>> k->cores_mask = POWER9_CORE_MASK;
>> 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->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 b1a7489e7abf..f36cb39dbf77 100644
>> --- a/hw/ppc/pnv_core.c
>> +++ b/hw/ppc/pnv_core.c
>> @@ -45,6 +45,8 @@ static void pnv_cpu_reset(void *opaque)
>> PowerPCCPU *cpu = opaque;
>> CPUState *cs = CPU(cpu);
>> CPUPPCState *env = &cpu->env;
>> + PnvChipClass *pcc;
>> + Object *chip;
>> cpu_reset(cs);
>> @@ -55,6 +57,10 @@ static void pnv_cpu_reset(void *opaque)
>> env->gpr[3] = PNV_FDT_ADDR;
>> env->nip = 0x10;
>> env->msr |= MSR_HVB; /* Hypervisor mode */
>> +
>> + chip = object_property_get_link(OBJECT(cpu), "chip", &error_fatal);
>
> Why not add a PnvChip *chip field in PnvCore?
because this is a PowerPCCPU object and not a PnvCore.
Your remark is good and I think I will follow the same pattern for PowerNV
that Greg has introduced for sPAPR in patch 2.
Thanks,
C.
> Then you only get it in realize() and it is available in reset().
>
> Rest of your patch looks OK from a QOM PoV.
>
>> + pcc = PNV_CHIP_GET_CLASS(chip);
>> + pcc->intc_reset(PNV_CHIP(chip), cpu);
>> }
>> /*
>> @@ -169,6 +175,8 @@ static void pnv_realize_vcpu(PowerPCCPU *cpu, PnvChip *chip, Error **errp)
>> Error *local_err = NULL;
>> PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip);
>> + object_property_add_const_link(OBJECT(cpu), "chip",
>> + OBJECT(chip), &error_fatal);
>> object_property_set_bool(OBJECT(cpu), true, "realized", &local_err);
>> if (local_err) {
>> error_propagate(errp, local_err);
>> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
>> index 5947e39b36ad..d2903c2d0f22 100644
>> --- a/hw/ppc/spapr_cpu_core.c
>> +++ b/hw/ppc/spapr_cpu_core.c
>> @@ -32,6 +32,7 @@ static void spapr_reset_vcpu(PowerPCCPU *cpu)
>> PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
>> SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu);
>> target_ulong lpcr;
>> + SpaprMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
>> cpu_reset(cs);
>> @@ -76,9 +77,11 @@ static void spapr_reset_vcpu(PowerPCCPU *cpu)
>> spapr_cpu->dtl_addr = 0;
>> spapr_cpu->dtl_size = 0;
>> - spapr_caps_cpu_apply(SPAPR_MACHINE(qdev_get_machine()), cpu);
>> + spapr_caps_cpu_apply(spapr, cpu);
>> kvm_check_mmu(cpu, &error_fatal);
>> +
>> + spapr_irq_cpu_intc_reset(spapr, cpu);
>> }
>> void spapr_cpu_set_entry_state(PowerPCCPU *cpu, target_ulong nip, target_ulong r3)
>> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
>> index 234d1073e518..b941608b69ba 100644
>> --- a/hw/ppc/spapr_irq.c
>> +++ b/hw/ppc/spapr_irq.c
>> @@ -220,6 +220,20 @@ int spapr_irq_cpu_intc_create(SpaprMachineState *spapr,
>> return 0;
>> }
>> +void spapr_irq_cpu_intc_reset(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_reset(intc, cpu);
>> + }
>> + }
>> +}
>> +
>> static void spapr_set_irq(void *opaque, int irq, int level)
>> {
>> SpaprMachineState *spapr = SPAPR_MACHINE(opaque);
>>
>
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2019-10-22 11:45 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-22 7:22 [PATCH v3 0/4] ppc: reset the interrupt presenter from the CPU reset handler Cédric Le Goater
2019-10-22 7:22 ` [PATCH v3 1/4] spapr: move CPU reset after presenter creation Cédric Le Goater
2019-10-22 8:53 ` Philippe Mathieu-Daudé
2019-10-22 9:53 ` Greg Kurz
2019-10-22 7:22 ` [PATCH v3 2/4] spapr_cpu_core: Implement DeviceClass::reset Cédric Le Goater
2019-10-22 8:58 ` Philippe Mathieu-Daudé
2019-10-22 7:22 ` [PATCH v3 3/4] ppc: reset the interrupt presenter from the CPU reset handler Cédric Le Goater
2019-10-22 9:08 ` Philippe Mathieu-Daudé
2019-10-22 11:43 ` Cédric Le Goater
2019-10-22 11:19 ` Greg Kurz
2019-10-22 7:22 ` [PATCH v3 4/4] spapr/xive: Set the OS CAM line at reset Cédric Le Goater
2019-10-22 11:27 ` Greg Kurz
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.