* [PATCH 0/2] spapr: interrupt presenter fixes
@ 2019-10-17 14:42 Cédric Le Goater
2019-10-17 14:42 ` [PATCH 1/2] spapr: Introduce a interrupt presenter reset handler Cédric Le Goater
2019-10-17 14:42 ` [PATCH 2/2] spapr/xive: Set the OS CAM line at reset Cédric Le Goater
0 siblings, 2 replies; 14+ messages in thread
From: Cédric Le Goater @ 2019-10-17 14:42 UTC (permalink / raw)
To: David Gibson; +Cc: Cédric Le Goater, qemu-ppc, qemu-devel, Greg Kurz
Hello,
First add a cpu_intc_reset() handler to reset interrupt presenters
which are not today and Introduce a 'os-cam' property which will be
used to set the OS CAM line at reset.
Thanks,
C.
Cédric Le Goater (2):
spapr: Introduce a interrupt presenter reset handler
spapr/xive: Set the OS CAM line at reset
include/hw/ppc/spapr_irq.h | 4 ++++
include/hw/ppc/spapr_xive.h | 1 -
include/hw/ppc/xics.h | 1 +
include/hw/ppc/xive.h | 5 ++++-
hw/intc/spapr_xive.c | 37 ++++++++++++-------------------------
hw/intc/xics.c | 5 +++++
hw/intc/xics_spapr.c | 8 ++++++++
hw/intc/xive.c | 33 +++++++++++++++++++++++++++++----
hw/ppc/pnv.c | 3 ++-
hw/ppc/spapr_cpu_core.c | 8 ++++++--
hw/ppc/spapr_irq.c | 21 +++++++++++++++++++++
11 files changed, 92 insertions(+), 34 deletions(-)
--
2.21.0
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/2] spapr: Introduce a interrupt presenter reset handler
2019-10-17 14:42 [PATCH 0/2] spapr: interrupt presenter fixes Cédric Le Goater
@ 2019-10-17 14:42 ` Cédric Le Goater
2019-10-18 2:47 ` David Gibson
` (2 more replies)
2019-10-17 14:42 ` [PATCH 2/2] spapr/xive: Set the OS CAM line at reset Cédric Le Goater
1 sibling, 3 replies; 14+ messages in thread
From: Cédric Le Goater @ 2019-10-17 14:42 UTC (permalink / raw)
To: David Gibson; +Cc: Cédric Le Goater, qemu-ppc, qemu-devel, Greg Kurz
The interrupt presenters are not reseted today. Extend the sPAPR IRQ
backend with a new cpu_intc_reset() handler which will be called by
the CPU reset handler.
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>
---
include/hw/ppc/spapr_irq.h | 4 ++++
include/hw/ppc/xics.h | 1 +
include/hw/ppc/xive.h | 1 +
hw/intc/spapr_xive.c | 8 ++++++++
hw/intc/xics.c | 5 +++++
hw/intc/xics_spapr.c | 8 ++++++++
hw/intc/xive.c | 11 ++++++++---
hw/ppc/spapr_cpu_core.c | 8 ++++++--
hw/ppc/spapr_irq.c | 21 +++++++++++++++++++++
9 files changed, 62 insertions(+), 5 deletions(-)
diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
index 5e150a667902..78327496c102 100644
--- a/include/hw/ppc/spapr_irq.h
+++ b/include/hw/ppc/spapr_irq.h
@@ -52,6 +52,8 @@ typedef struct SpaprInterruptControllerClass {
*/
int (*cpu_intc_create)(SpaprInterruptController *intc,
PowerPCCPU *cpu, Error **errp);
+ int (*cpu_intc_reset)(SpaprInterruptController *intc, PowerPCCPU *cpu,
+ Error **errp);
int (*claim_irq)(SpaprInterruptController *intc, int irq, bool lsi,
Error **errp);
void (*free_irq)(SpaprInterruptController *intc, int irq);
@@ -68,6 +70,8 @@ void spapr_irq_update_active_intc(SpaprMachineState *spapr);
int spapr_irq_cpu_intc_create(SpaprMachineState *spapr,
PowerPCCPU *cpu, Error **errp);
+int spapr_irq_cpu_intc_reset(SpaprMachineState *spapr,
+ PowerPCCPU *cpu, Error **errp);
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..0c3acf1a4192 100644
--- a/hw/intc/spapr_xive.c
+++ b/hw/intc/spapr_xive.c
@@ -553,6 +553,13 @@ static int spapr_xive_cpu_intc_create(SpaprInterruptController *intc,
return 0;
}
+static int spapr_xive_cpu_intc_reset(SpaprInterruptController *intc,
+ PowerPCCPU *cpu, Error **errp)
+{
+ xive_tctx_reset(spapr_cpu_state(cpu)->tctx);
+ return 0;
+}
+
static void spapr_xive_set_irq(SpaprInterruptController *intc, int irq, int val)
{
SpaprXive *xive = SPAPR_XIVE(intc);
@@ -697,6 +704,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..652771d6a5a5 100644
--- a/hw/intc/xics.c
+++ b/hw/intc/xics.c
@@ -295,6 +295,11 @@ static void icp_reset_handler(void *dev)
}
}
+void icp_reset(ICPState *icp)
+{
+ icp_reset_handler(icp);
+}
+
static void icp_realize(DeviceState *dev, Error **errp)
{
ICPState *icp = ICP(dev);
diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
index 4f64b9a9fc66..c0b2a576effe 100644
--- a/hw/intc/xics_spapr.c
+++ b/hw/intc/xics_spapr.c
@@ -346,6 +346,13 @@ static int xics_spapr_cpu_intc_create(SpaprInterruptController *intc,
return 0;
}
+static int xics_spapr_cpu_intc_reset(SpaprInterruptController *intc,
+ PowerPCCPU *cpu, Error **errp)
+{
+ icp_reset(spapr_cpu_state(cpu)->icp);
+ return 0;
+}
+
static int xics_spapr_claim_irq(SpaprInterruptController *intc, int irq,
bool lsi, Error **errp)
{
@@ -433,6 +440,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..0ae3f9b1efe4 100644
--- a/hw/intc/xive.c
+++ b/hw/intc/xive.c
@@ -547,7 +547,7 @@ void xive_tctx_pic_print_info(XiveTCTX *tctx, Monitor *mon)
}
}
-static void xive_tctx_reset(void *dev)
+static void xive_tctx_reset_handler(void *dev)
{
XiveTCTX *tctx = XIVE_TCTX(dev);
@@ -568,6 +568,11 @@ static void xive_tctx_reset(void *dev)
ipb_to_pipr(tctx->regs[TM_QW3_HV_PHYS + TM_IPB]);
}
+void xive_tctx_reset(XiveTCTX *tctx)
+{
+ xive_tctx_reset_handler(tctx);
+}
+
static void xive_tctx_realize(DeviceState *dev, Error **errp)
{
XiveTCTX *tctx = XIVE_TCTX(dev);
@@ -608,12 +613,12 @@ static void xive_tctx_realize(DeviceState *dev, Error **errp)
}
}
- qemu_register_reset(xive_tctx_reset, dev);
+ qemu_register_reset(xive_tctx_reset_handler, dev);
}
static void xive_tctx_unrealize(DeviceState *dev, Error **errp)
{
- qemu_unregister_reset(xive_tctx_reset, dev);
+ qemu_unregister_reset(xive_tctx_reset_handler, dev);
}
static int vmstate_xive_tctx_pre_save(void *opaque)
diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index 3e4302c7d596..416aa75e5fba 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -33,6 +33,7 @@ static void spapr_cpu_reset(void *opaque)
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);
@@ -77,9 +78,11 @@ static void spapr_cpu_reset(void *opaque)
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, &error_fatal);
}
void spapr_cpu_set_entry_state(PowerPCCPU *cpu, target_ulong nip, target_ulong r3)
@@ -235,12 +238,13 @@ static void spapr_realize_vcpu(PowerPCCPU *cpu, SpaprMachineState *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;
}
+ spapr_cpu_reset(cpu);
+
if (!sc->pre_3_0_migration) {
vmstate_register(NULL, cs->cpu_index, &vmstate_spapr_cpu_state,
cpu->machine_data);
diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
index bb91c61fa000..5d2b64029cd5 100644
--- a/hw/ppc/spapr_irq.c
+++ b/hw/ppc/spapr_irq.c
@@ -220,6 +220,27 @@ int spapr_irq_cpu_intc_create(SpaprMachineState *spapr,
return 0;
}
+int spapr_irq_cpu_intc_reset(SpaprMachineState *spapr,
+ PowerPCCPU *cpu, Error **errp)
+{
+ SpaprInterruptController *intcs[] = ALL_INTCS(spapr);
+ int i;
+ int rc;
+
+ for (i = 0; i < ARRAY_SIZE(intcs); i++) {
+ SpaprInterruptController *intc = intcs[i];
+ if (intc) {
+ SpaprInterruptControllerClass *sicc = SPAPR_INTC_GET_CLASS(intc);
+ rc = sicc->cpu_intc_reset(intc, cpu, errp);
+ if (rc < 0) {
+ return rc;
+ }
+ }
+ }
+
+ return 0;
+}
+
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] 14+ messages in thread
* [PATCH 2/2] spapr/xive: Set the OS CAM line at reset
2019-10-17 14:42 [PATCH 0/2] spapr: interrupt presenter fixes Cédric Le Goater
2019-10-17 14:42 ` [PATCH 1/2] spapr: Introduce a interrupt presenter reset handler Cédric Le Goater
@ 2019-10-17 14:42 ` Cédric Le Goater
2019-10-18 3:55 ` David Gibson
2019-10-18 8:07 ` Greg Kurz
1 sibling, 2 replies; 14+ messages in thread
From: Cédric Le Goater @ 2019-10-17 14:42 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 in
TCG or kernel_irqchip=off, QEMU needs to emulate the same behavior.
Introduce a 'os-cam' property which will be used to set the OS CAM
line at reset and remove the spapr_xive_set_tctx_os_cam() calls which
are done when the XIVE interrupt controller are activated.
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 -
include/hw/ppc/xive.h | 4 +++-
hw/intc/spapr_xive.c | 31 +++++--------------------------
hw/intc/xive.c | 22 +++++++++++++++++++++-
hw/ppc/pnv.c | 3 ++-
5 files changed, 31 insertions(+), 30 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/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
index 99381639f50c..e273069c25a9 100644
--- a/include/hw/ppc/xive.h
+++ b/include/hw/ppc/xive.h
@@ -319,6 +319,7 @@ typedef struct XiveTCTX {
qemu_irq os_output;
uint8_t regs[XIVE_TM_RING_COUNT * XIVE_TM_RING_SIZE];
+ uint32_t os_cam;
} XiveTCTX;
/*
@@ -414,7 +415,8 @@ void xive_tctx_tm_write(XiveTCTX *tctx, hwaddr offset, uint64_t value,
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);
+Object *xive_tctx_create(Object *cpu, XiveRouter *xrtr, uint32_t os_cam,
+ 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 0c3acf1a4192..71f138512a1c 100644
--- a/hw/intc/spapr_xive.c
+++ b/hw/intc/spapr_xive.c
@@ -205,21 +205,13 @@ 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)
+static uint32_t spapr_xive_get_os_cam(PowerPCCPU *cpu)
{
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);
+ spapr_xive_cpu_to_nvt(cpu, &nvt_blk, &nvt_idx);
+ return xive_nvt_cam_line(nvt_blk, nvt_idx);
}
static void spapr_xive_end_reset(XiveEND *end)
@@ -537,19 +529,14 @@ static int spapr_xive_cpu_intc_create(SpaprInterruptController *intc,
SpaprXive *xive = SPAPR_XIVE(intc);
Object *obj;
SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu);
+ uint32_t os_cam = spapr_xive_get_os_cam(cpu);
- obj = xive_tctx_create(OBJECT(cpu), XIVE_ROUTER(xive), errp);
+ obj = xive_tctx_create(OBJECT(cpu), XIVE_ROUTER(xive), os_cam, errp);
if (!obj) {
return -1;
}
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;
}
@@ -650,14 +637,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);
diff --git a/hw/intc/xive.c b/hw/intc/xive.c
index 0ae3f9b1efe4..be4f2c974178 100644
--- a/hw/intc/xive.c
+++ b/hw/intc/xive.c
@@ -566,6 +566,18 @@ static void xive_tctx_reset_handler(void *dev)
ipb_to_pipr(tctx->regs[TM_QW1_OS + TM_IPB]);
tctx->regs[TM_QW3_HV_PHYS + TM_PIPR] =
ipb_to_pipr(tctx->regs[TM_QW3_HV_PHYS + TM_IPB]);
+
+ /*
+ * (TCG) Set the OS CAM line of the thread interrupt context.
+ *
+ * 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.
+ */
+ if (tctx->os_cam) {
+ uint32_t qw1w2 = cpu_to_be32(TM_QW1W2_VO | tctx->os_cam);
+ memcpy(&tctx->regs[TM_QW1_OS + TM_WORD2], &qw1w2, 4);
+ }
}
void xive_tctx_reset(XiveTCTX *tctx)
@@ -667,11 +679,17 @@ static const VMStateDescription vmstate_xive_tctx = {
},
};
+static Property xive_tctx_properties[] = {
+ DEFINE_PROP_UINT32("os-cam", XiveTCTX, os_cam, 0),
+ DEFINE_PROP_END_OF_LIST(),
+};
+
static void xive_tctx_class_init(ObjectClass *klass, void *data)
{
DeviceClass *dc = DEVICE_CLASS(klass);
dc->desc = "XIVE Interrupt Thread Context";
+ dc->props = xive_tctx_properties;
dc->realize = xive_tctx_realize;
dc->unrealize = xive_tctx_unrealize;
dc->vmsd = &vmstate_xive_tctx;
@@ -689,7 +707,8 @@ static const TypeInfo xive_tctx_info = {
.class_init = xive_tctx_class_init,
};
-Object *xive_tctx_create(Object *cpu, XiveRouter *xrtr, Error **errp)
+Object *xive_tctx_create(Object *cpu, XiveRouter *xrtr, uint32_t os_cam,
+ Error **errp)
{
Error *local_err = NULL;
Object *obj;
@@ -698,6 +717,7 @@ Object *xive_tctx_create(Object *cpu, XiveRouter *xrtr, Error **errp)
object_property_add_child(cpu, TYPE_XIVE_TCTX, obj, &error_abort);
object_unref(obj);
object_property_add_const_link(obj, "cpu", cpu, &error_abort);
+ object_property_set_int(obj, os_cam, "os-cam", &local_err);
object_property_set_bool(obj, true, "realized", &local_err);
if (local_err) {
goto error;
diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 7cf64b6d2533..99c06842573e 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -806,7 +806,8 @@ static void pnv_chip_power9_intc_create(PnvChip *chip, PowerPCCPU *cpu,
* controller object is initialized afterwards. Hopefully, it's
* only used at runtime.
*/
- obj = xive_tctx_create(OBJECT(cpu), XIVE_ROUTER(&chip9->xive), &local_err);
+ obj = xive_tctx_create(OBJECT(cpu), XIVE_ROUTER(&chip9->xive), 0,
+ &local_err);
if (local_err) {
error_propagate(errp, local_err);
return;
--
2.21.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] spapr: Introduce a interrupt presenter reset handler
2019-10-17 14:42 ` [PATCH 1/2] spapr: Introduce a interrupt presenter reset handler Cédric Le Goater
@ 2019-10-18 2:47 ` David Gibson
2019-10-18 9:41 ` Greg Kurz
2019-10-18 9:44 ` Cédric Le Goater
2019-10-18 6:16 ` Cédric Le Goater
2019-10-18 7:46 ` Greg Kurz
2 siblings, 2 replies; 14+ messages in thread
From: David Gibson @ 2019-10-18 2:47 UTC (permalink / raw)
To: Cédric Le Goater; +Cc: qemu-ppc, qemu-devel, Greg Kurz
[-- Attachment #1: Type: text/plain, Size: 10042 bytes --]
On Thu, Oct 17, 2019 at 04:42:40PM +0200, Cédric Le Goater wrote:
> The interrupt presenters are not reseted today.
I don't think that's accurate. We register reset handlers for both
ICP and TCTX already. We might not be resetting in quite the right
order, but this will need a clearer description of what's changing.
Also, with this patch as is, I think we'll reset twice (once from the
registered handler, once via the cpu).
> Extend the sPAPR IRQ
> backend with a new cpu_intc_reset() handler which will be called by
> the CPU reset handler.
>
> 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>
> ---
> include/hw/ppc/spapr_irq.h | 4 ++++
> include/hw/ppc/xics.h | 1 +
> include/hw/ppc/xive.h | 1 +
> hw/intc/spapr_xive.c | 8 ++++++++
> hw/intc/xics.c | 5 +++++
> hw/intc/xics_spapr.c | 8 ++++++++
> hw/intc/xive.c | 11 ++++++++---
> hw/ppc/spapr_cpu_core.c | 8 ++++++--
> hw/ppc/spapr_irq.c | 21 +++++++++++++++++++++
> 9 files changed, 62 insertions(+), 5 deletions(-)
>
> diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
> index 5e150a667902..78327496c102 100644
> --- a/include/hw/ppc/spapr_irq.h
> +++ b/include/hw/ppc/spapr_irq.h
> @@ -52,6 +52,8 @@ typedef struct SpaprInterruptControllerClass {
> */
> int (*cpu_intc_create)(SpaprInterruptController *intc,
> PowerPCCPU *cpu, Error **errp);
> + int (*cpu_intc_reset)(SpaprInterruptController *intc, PowerPCCPU *cpu,
> + Error **errp);
> int (*claim_irq)(SpaprInterruptController *intc, int irq, bool lsi,
> Error **errp);
> void (*free_irq)(SpaprInterruptController *intc, int irq);
> @@ -68,6 +70,8 @@ void spapr_irq_update_active_intc(SpaprMachineState *spapr);
>
> int spapr_irq_cpu_intc_create(SpaprMachineState *spapr,
> PowerPCCPU *cpu, Error **errp);
> +int spapr_irq_cpu_intc_reset(SpaprMachineState *spapr,
> + PowerPCCPU *cpu, Error **errp);
> 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..0c3acf1a4192 100644
> --- a/hw/intc/spapr_xive.c
> +++ b/hw/intc/spapr_xive.c
> @@ -553,6 +553,13 @@ static int spapr_xive_cpu_intc_create(SpaprInterruptController *intc,
> return 0;
> }
>
> +static int spapr_xive_cpu_intc_reset(SpaprInterruptController *intc,
> + PowerPCCPU *cpu, Error **errp)
> +{
> + xive_tctx_reset(spapr_cpu_state(cpu)->tctx);
> + return 0;
> +}
> +
> static void spapr_xive_set_irq(SpaprInterruptController *intc, int irq, int val)
> {
> SpaprXive *xive = SPAPR_XIVE(intc);
> @@ -697,6 +704,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..652771d6a5a5 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -295,6 +295,11 @@ static void icp_reset_handler(void *dev)
> }
> }
>
> +void icp_reset(ICPState *icp)
> +{
> + icp_reset_handler(icp);
> +}
> +
> static void icp_realize(DeviceState *dev, Error **errp)
> {
> ICPState *icp = ICP(dev);
> diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
> index 4f64b9a9fc66..c0b2a576effe 100644
> --- a/hw/intc/xics_spapr.c
> +++ b/hw/intc/xics_spapr.c
> @@ -346,6 +346,13 @@ static int xics_spapr_cpu_intc_create(SpaprInterruptController *intc,
> return 0;
> }
>
> +static int xics_spapr_cpu_intc_reset(SpaprInterruptController *intc,
> + PowerPCCPU *cpu, Error **errp)
> +{
> + icp_reset(spapr_cpu_state(cpu)->icp);
> + return 0;
> +}
> +
> static int xics_spapr_claim_irq(SpaprInterruptController *intc, int irq,
> bool lsi, Error **errp)
> {
> @@ -433,6 +440,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..0ae3f9b1efe4 100644
> --- a/hw/intc/xive.c
> +++ b/hw/intc/xive.c
> @@ -547,7 +547,7 @@ void xive_tctx_pic_print_info(XiveTCTX *tctx, Monitor *mon)
> }
> }
>
> -static void xive_tctx_reset(void *dev)
> +static void xive_tctx_reset_handler(void *dev)
> {
> XiveTCTX *tctx = XIVE_TCTX(dev);
>
> @@ -568,6 +568,11 @@ static void xive_tctx_reset(void *dev)
> ipb_to_pipr(tctx->regs[TM_QW3_HV_PHYS + TM_IPB]);
> }
>
> +void xive_tctx_reset(XiveTCTX *tctx)
> +{
> + xive_tctx_reset_handler(tctx);
> +}
> +
> static void xive_tctx_realize(DeviceState *dev, Error **errp)
> {
> XiveTCTX *tctx = XIVE_TCTX(dev);
> @@ -608,12 +613,12 @@ static void xive_tctx_realize(DeviceState *dev, Error **errp)
> }
> }
>
> - qemu_register_reset(xive_tctx_reset, dev);
> + qemu_register_reset(xive_tctx_reset_handler, dev);
> }
>
> static void xive_tctx_unrealize(DeviceState *dev, Error **errp)
> {
> - qemu_unregister_reset(xive_tctx_reset, dev);
> + qemu_unregister_reset(xive_tctx_reset_handler, dev);
> }
>
> static int vmstate_xive_tctx_pre_save(void *opaque)
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 3e4302c7d596..416aa75e5fba 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -33,6 +33,7 @@ static void spapr_cpu_reset(void *opaque)
> 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);
>
> @@ -77,9 +78,11 @@ static void spapr_cpu_reset(void *opaque)
> 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, &error_fatal);
> }
>
> void spapr_cpu_set_entry_state(PowerPCCPU *cpu, target_ulong nip, target_ulong r3)
> @@ -235,12 +238,13 @@ static void spapr_realize_vcpu(PowerPCCPU *cpu, SpaprMachineState *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;
> }
>
> + spapr_cpu_reset(cpu);
> +
> if (!sc->pre_3_0_migration) {
> vmstate_register(NULL, cs->cpu_index, &vmstate_spapr_cpu_state,
> cpu->machine_data);
> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> index bb91c61fa000..5d2b64029cd5 100644
> --- a/hw/ppc/spapr_irq.c
> +++ b/hw/ppc/spapr_irq.c
> @@ -220,6 +220,27 @@ int spapr_irq_cpu_intc_create(SpaprMachineState *spapr,
> return 0;
> }
>
> +int spapr_irq_cpu_intc_reset(SpaprMachineState *spapr,
> + PowerPCCPU *cpu, Error **errp)
> +{
> + SpaprInterruptController *intcs[] = ALL_INTCS(spapr);
> + int i;
> + int rc;
> +
> + for (i = 0; i < ARRAY_SIZE(intcs); i++) {
> + SpaprInterruptController *intc = intcs[i];
> + if (intc) {
> + SpaprInterruptControllerClass *sicc = SPAPR_INTC_GET_CLASS(intc);
> + rc = sicc->cpu_intc_reset(intc, cpu, errp);
> + if (rc < 0) {
> + return rc;
> + }
> + }
> + }
> +
> + return 0;
> +}
> +
> static void spapr_set_irq(void *opaque, int irq, int level)
> {
> SpaprMachineState *spapr = SPAPR_MACHINE(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] 14+ messages in thread
* Re: [PATCH 2/2] spapr/xive: Set the OS CAM line at reset
2019-10-17 14:42 ` [PATCH 2/2] spapr/xive: Set the OS CAM line at reset Cédric Le Goater
@ 2019-10-18 3:55 ` David Gibson
2019-10-18 9:42 ` Cédric Le Goater
2019-10-18 8:07 ` Greg Kurz
1 sibling, 1 reply; 14+ messages in thread
From: David Gibson @ 2019-10-18 3:55 UTC (permalink / raw)
To: Cédric Le Goater; +Cc: qemu-ppc, qemu-devel, Greg Kurz
[-- Attachment #1: Type: text/plain, Size: 8459 bytes --]
On Thu, Oct 17, 2019 at 04:42:41PM +0200, Cédric Le Goater 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 in
> TCG or kernel_irqchip=off, QEMU needs to emulate the same behavior.
>
> Introduce a 'os-cam' property which will be used to set the OS CAM
> line at reset and remove the spapr_xive_set_tctx_os_cam() calls which
> are done when the XIVE interrupt controller are activated.
I'm not immediately seeing the advantage of doing this via a property,
rather than poking it from the PAPR code which already knows the right
values.
Also, let me check my understanding:
IIUC, on powernv the OS (running in HV mode) can alter the OS CAM
lines for itself and/or its guests, but for pseries they're fixed in
place. Is that right?
> 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 -
> include/hw/ppc/xive.h | 4 +++-
> hw/intc/spapr_xive.c | 31 +++++--------------------------
> hw/intc/xive.c | 22 +++++++++++++++++++++-
> hw/ppc/pnv.c | 3 ++-
> 5 files changed, 31 insertions(+), 30 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/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
> index 99381639f50c..e273069c25a9 100644
> --- a/include/hw/ppc/xive.h
> +++ b/include/hw/ppc/xive.h
> @@ -319,6 +319,7 @@ typedef struct XiveTCTX {
> qemu_irq os_output;
>
> uint8_t regs[XIVE_TM_RING_COUNT * XIVE_TM_RING_SIZE];
> + uint32_t os_cam;
> } XiveTCTX;
>
> /*
> @@ -414,7 +415,8 @@ void xive_tctx_tm_write(XiveTCTX *tctx, hwaddr offset, uint64_t value,
> 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);
> +Object *xive_tctx_create(Object *cpu, XiveRouter *xrtr, uint32_t os_cam,
> + 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 0c3acf1a4192..71f138512a1c 100644
> --- a/hw/intc/spapr_xive.c
> +++ b/hw/intc/spapr_xive.c
> @@ -205,21 +205,13 @@ 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)
> +static uint32_t spapr_xive_get_os_cam(PowerPCCPU *cpu)
> {
> 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);
> + spapr_xive_cpu_to_nvt(cpu, &nvt_blk, &nvt_idx);
> + return xive_nvt_cam_line(nvt_blk, nvt_idx);
> }
>
> static void spapr_xive_end_reset(XiveEND *end)
> @@ -537,19 +529,14 @@ static int spapr_xive_cpu_intc_create(SpaprInterruptController *intc,
> SpaprXive *xive = SPAPR_XIVE(intc);
> Object *obj;
> SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu);
> + uint32_t os_cam = spapr_xive_get_os_cam(cpu);
>
> - obj = xive_tctx_create(OBJECT(cpu), XIVE_ROUTER(xive), errp);
> + obj = xive_tctx_create(OBJECT(cpu), XIVE_ROUTER(xive), os_cam, errp);
> if (!obj) {
> return -1;
> }
>
> 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;
> }
>
> @@ -650,14 +637,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);
> diff --git a/hw/intc/xive.c b/hw/intc/xive.c
> index 0ae3f9b1efe4..be4f2c974178 100644
> --- a/hw/intc/xive.c
> +++ b/hw/intc/xive.c
> @@ -566,6 +566,18 @@ static void xive_tctx_reset_handler(void *dev)
> ipb_to_pipr(tctx->regs[TM_QW1_OS + TM_IPB]);
> tctx->regs[TM_QW3_HV_PHYS + TM_PIPR] =
> ipb_to_pipr(tctx->regs[TM_QW3_HV_PHYS + TM_IPB]);
> +
> + /*
> + * (TCG) Set the OS CAM line of the thread interrupt context.
> + *
> + * 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.
> + */
> + if (tctx->os_cam) {
> + uint32_t qw1w2 = cpu_to_be32(TM_QW1W2_VO | tctx->os_cam);
> + memcpy(&tctx->regs[TM_QW1_OS + TM_WORD2], &qw1w2, 4);
> + }
> }
>
> void xive_tctx_reset(XiveTCTX *tctx)
> @@ -667,11 +679,17 @@ static const VMStateDescription vmstate_xive_tctx = {
> },
> };
>
> +static Property xive_tctx_properties[] = {
> + DEFINE_PROP_UINT32("os-cam", XiveTCTX, os_cam, 0),
> + DEFINE_PROP_END_OF_LIST(),
> +};
> +
> static void xive_tctx_class_init(ObjectClass *klass, void *data)
> {
> DeviceClass *dc = DEVICE_CLASS(klass);
>
> dc->desc = "XIVE Interrupt Thread Context";
> + dc->props = xive_tctx_properties;
> dc->realize = xive_tctx_realize;
> dc->unrealize = xive_tctx_unrealize;
> dc->vmsd = &vmstate_xive_tctx;
> @@ -689,7 +707,8 @@ static const TypeInfo xive_tctx_info = {
> .class_init = xive_tctx_class_init,
> };
>
> -Object *xive_tctx_create(Object *cpu, XiveRouter *xrtr, Error **errp)
> +Object *xive_tctx_create(Object *cpu, XiveRouter *xrtr, uint32_t os_cam,
> + Error **errp)
> {
> Error *local_err = NULL;
> Object *obj;
> @@ -698,6 +717,7 @@ Object *xive_tctx_create(Object *cpu, XiveRouter *xrtr, Error **errp)
> object_property_add_child(cpu, TYPE_XIVE_TCTX, obj, &error_abort);
> object_unref(obj);
> object_property_add_const_link(obj, "cpu", cpu, &error_abort);
> + object_property_set_int(obj, os_cam, "os-cam", &local_err);
> object_property_set_bool(obj, true, "realized", &local_err);
> if (local_err) {
> goto error;
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 7cf64b6d2533..99c06842573e 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -806,7 +806,8 @@ static void pnv_chip_power9_intc_create(PnvChip *chip, PowerPCCPU *cpu,
> * controller object is initialized afterwards. Hopefully, it's
> * only used at runtime.
> */
> - obj = xive_tctx_create(OBJECT(cpu), XIVE_ROUTER(&chip9->xive), &local_err);
> + obj = xive_tctx_create(OBJECT(cpu), XIVE_ROUTER(&chip9->xive), 0,
> + &local_err);
> if (local_err) {
> error_propagate(errp, local_err);
> return;
--
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] 14+ messages in thread
* Re: [PATCH 1/2] spapr: Introduce a interrupt presenter reset handler
2019-10-17 14:42 ` [PATCH 1/2] spapr: Introduce a interrupt presenter reset handler Cédric Le Goater
2019-10-18 2:47 ` David Gibson
@ 2019-10-18 6:16 ` Cédric Le Goater
2019-10-18 7:46 ` Greg Kurz
2 siblings, 0 replies; 14+ messages in thread
From: Cédric Le Goater @ 2019-10-18 6:16 UTC (permalink / raw)
To: David Gibson; +Cc: qemu-ppc, qemu-devel, Greg Kurz
On 17/10/2019 16:42, Cédric Le Goater wrote:
> The interrupt presenters are not reseted today.
I should have added :
... when CPU are hot-plugged.
> Extend the sPAPR IRQ
> backend with a new cpu_intc_reset() handler which will be called by
> the CPU reset handler.
>
> 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>
> ---
> include/hw/ppc/spapr_irq.h | 4 ++++
> include/hw/ppc/xics.h | 1 +
> include/hw/ppc/xive.h | 1 +
> hw/intc/spapr_xive.c | 8 ++++++++
> hw/intc/xics.c | 5 +++++
> hw/intc/xics_spapr.c | 8 ++++++++
> hw/intc/xive.c | 11 ++++++++---
> hw/ppc/spapr_cpu_core.c | 8 ++++++--
> hw/ppc/spapr_irq.c | 21 +++++++++++++++++++++
> 9 files changed, 62 insertions(+), 5 deletions(-)
>
> diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
> index 5e150a667902..78327496c102 100644
> --- a/include/hw/ppc/spapr_irq.h
> +++ b/include/hw/ppc/spapr_irq.h
> @@ -52,6 +52,8 @@ typedef struct SpaprInterruptControllerClass {
> */
> int (*cpu_intc_create)(SpaprInterruptController *intc,
> PowerPCCPU *cpu, Error **errp);
> + int (*cpu_intc_reset)(SpaprInterruptController *intc, PowerPCCPU *cpu,
> + Error **errp);
> int (*claim_irq)(SpaprInterruptController *intc, int irq, bool lsi,
> Error **errp);
> void (*free_irq)(SpaprInterruptController *intc, int irq);
> @@ -68,6 +70,8 @@ void spapr_irq_update_active_intc(SpaprMachineState *spapr);
>
> int spapr_irq_cpu_intc_create(SpaprMachineState *spapr,
> PowerPCCPU *cpu, Error **errp);
> +int spapr_irq_cpu_intc_reset(SpaprMachineState *spapr,
> + PowerPCCPU *cpu, Error **errp);
> 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..0c3acf1a4192 100644
> --- a/hw/intc/spapr_xive.c
> +++ b/hw/intc/spapr_xive.c
> @@ -553,6 +553,13 @@ static int spapr_xive_cpu_intc_create(SpaprInterruptController *intc,
> return 0;
> }
>
> +static int spapr_xive_cpu_intc_reset(SpaprInterruptController *intc,
> + PowerPCCPU *cpu, Error **errp)
> +{
> + xive_tctx_reset(spapr_cpu_state(cpu)->tctx);
> + return 0;
> +}
> +
> static void spapr_xive_set_irq(SpaprInterruptController *intc, int irq, int val)
> {
> SpaprXive *xive = SPAPR_XIVE(intc);
> @@ -697,6 +704,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..652771d6a5a5 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -295,6 +295,11 @@ static void icp_reset_handler(void *dev)
> }
> }
>
> +void icp_reset(ICPState *icp)
> +{
> + icp_reset_handler(icp);
> +}
> +
> static void icp_realize(DeviceState *dev, Error **errp)
> {
> ICPState *icp = ICP(dev);
> diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
> index 4f64b9a9fc66..c0b2a576effe 100644
> --- a/hw/intc/xics_spapr.c
> +++ b/hw/intc/xics_spapr.c
> @@ -346,6 +346,13 @@ static int xics_spapr_cpu_intc_create(SpaprInterruptController *intc,
> return 0;
> }
>
> +static int xics_spapr_cpu_intc_reset(SpaprInterruptController *intc,
> + PowerPCCPU *cpu, Error **errp)
> +{
> + icp_reset(spapr_cpu_state(cpu)->icp);
> + return 0;
> +}
> +
> static int xics_spapr_claim_irq(SpaprInterruptController *intc, int irq,
> bool lsi, Error **errp)
> {
> @@ -433,6 +440,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..0ae3f9b1efe4 100644
> --- a/hw/intc/xive.c
> +++ b/hw/intc/xive.c
> @@ -547,7 +547,7 @@ void xive_tctx_pic_print_info(XiveTCTX *tctx, Monitor *mon)
> }
> }
>
> -static void xive_tctx_reset(void *dev)
> +static void xive_tctx_reset_handler(void *dev)
> {
> XiveTCTX *tctx = XIVE_TCTX(dev);
>
> @@ -568,6 +568,11 @@ static void xive_tctx_reset(void *dev)
> ipb_to_pipr(tctx->regs[TM_QW3_HV_PHYS + TM_IPB]);
> }
>
> +void xive_tctx_reset(XiveTCTX *tctx)
> +{
> + xive_tctx_reset_handler(tctx);
> +}
> +
> static void xive_tctx_realize(DeviceState *dev, Error **errp)
> {
> XiveTCTX *tctx = XIVE_TCTX(dev);
> @@ -608,12 +613,12 @@ static void xive_tctx_realize(DeviceState *dev, Error **errp)
> }
> }
>
> - qemu_register_reset(xive_tctx_reset, dev);
> + qemu_register_reset(xive_tctx_reset_handler, dev);
> }
>
> static void xive_tctx_unrealize(DeviceState *dev, Error **errp)
> {
> - qemu_unregister_reset(xive_tctx_reset, dev);
> + qemu_unregister_reset(xive_tctx_reset_handler, dev);
> }
>
> static int vmstate_xive_tctx_pre_save(void *opaque)
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 3e4302c7d596..416aa75e5fba 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -33,6 +33,7 @@ static void spapr_cpu_reset(void *opaque)
> 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);
>
> @@ -77,9 +78,11 @@ static void spapr_cpu_reset(void *opaque)
> 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, &error_fatal);
> }
>
> void spapr_cpu_set_entry_state(PowerPCCPU *cpu, target_ulong nip, target_ulong r3)
> @@ -235,12 +238,13 @@ static void spapr_realize_vcpu(PowerPCCPU *cpu, SpaprMachineState *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;
> }
>
> + spapr_cpu_reset(cpu);
> +
> if (!sc->pre_3_0_migration) {
> vmstate_register(NULL, cs->cpu_index, &vmstate_spapr_cpu_state,
> cpu->machine_data);
> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> index bb91c61fa000..5d2b64029cd5 100644
> --- a/hw/ppc/spapr_irq.c
> +++ b/hw/ppc/spapr_irq.c
> @@ -220,6 +220,27 @@ int spapr_irq_cpu_intc_create(SpaprMachineState *spapr,
> return 0;
> }
>
> +int spapr_irq_cpu_intc_reset(SpaprMachineState *spapr,
> + PowerPCCPU *cpu, Error **errp)
> +{
> + SpaprInterruptController *intcs[] = ALL_INTCS(spapr);
> + int i;
> + int rc;
> +
> + for (i = 0; i < ARRAY_SIZE(intcs); i++) {
> + SpaprInterruptController *intc = intcs[i];
> + if (intc) {
> + SpaprInterruptControllerClass *sicc = SPAPR_INTC_GET_CLASS(intc);
> + rc = sicc->cpu_intc_reset(intc, cpu, errp);
> + if (rc < 0) {
> + return rc;
> + }
> + }
> + }
> +
> + return 0;
> +}
> +
> static void spapr_set_irq(void *opaque, int irq, int level)
> {
> SpaprMachineState *spapr = SPAPR_MACHINE(opaque);
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] spapr: Introduce a interrupt presenter reset handler
2019-10-17 14:42 ` [PATCH 1/2] spapr: Introduce a interrupt presenter reset handler Cédric Le Goater
2019-10-18 2:47 ` David Gibson
2019-10-18 6:16 ` Cédric Le Goater
@ 2019-10-18 7:46 ` Greg Kurz
2019-10-18 8:26 ` Cédric Le Goater
2 siblings, 1 reply; 14+ messages in thread
From: Greg Kurz @ 2019-10-18 7:46 UTC (permalink / raw)
To: Cédric Le Goater; +Cc: qemu-ppc, qemu-devel, David Gibson
On Thu, 17 Oct 2019 16:42:40 +0200
Cédric Le Goater <clg@kaod.org> wrote:
> The interrupt presenters are not reseted today. Extend the sPAPR IRQ
> backend with a new cpu_intc_reset() handler which will be called by
> the CPU reset handler.
>
> 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>
> ---
> include/hw/ppc/spapr_irq.h | 4 ++++
> include/hw/ppc/xics.h | 1 +
> include/hw/ppc/xive.h | 1 +
> hw/intc/spapr_xive.c | 8 ++++++++
> hw/intc/xics.c | 5 +++++
> hw/intc/xics_spapr.c | 8 ++++++++
> hw/intc/xive.c | 11 ++++++++---
> hw/ppc/spapr_cpu_core.c | 8 ++++++--
> hw/ppc/spapr_irq.c | 21 +++++++++++++++++++++
> 9 files changed, 62 insertions(+), 5 deletions(-)
>
> diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
> index 5e150a667902..78327496c102 100644
> --- a/include/hw/ppc/spapr_irq.h
> +++ b/include/hw/ppc/spapr_irq.h
> @@ -52,6 +52,8 @@ typedef struct SpaprInterruptControllerClass {
> */
> int (*cpu_intc_create)(SpaprInterruptController *intc,
> PowerPCCPU *cpu, Error **errp);
> + int (*cpu_intc_reset)(SpaprInterruptController *intc, PowerPCCPU *cpu,
> + Error **errp);
Looking at the rest of the patch, it seems that we don't need error
reporting. I suggest you make this void and drop the errp parameter.
> int (*claim_irq)(SpaprInterruptController *intc, int irq, bool lsi,
> Error **errp);
> void (*free_irq)(SpaprInterruptController *intc, int irq);
> @@ -68,6 +70,8 @@ void spapr_irq_update_active_intc(SpaprMachineState *spapr);
>
> int spapr_irq_cpu_intc_create(SpaprMachineState *spapr,
> PowerPCCPU *cpu, Error **errp);
> +int spapr_irq_cpu_intc_reset(SpaprMachineState *spapr,
> + PowerPCCPU *cpu, Error **errp);
> 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..0c3acf1a4192 100644
> --- a/hw/intc/spapr_xive.c
> +++ b/hw/intc/spapr_xive.c
> @@ -553,6 +553,13 @@ static int spapr_xive_cpu_intc_create(SpaprInterruptController *intc,
> return 0;
> }
>
> +static int spapr_xive_cpu_intc_reset(SpaprInterruptController *intc,
> + PowerPCCPU *cpu, Error **errp)
> +{
> + xive_tctx_reset(spapr_cpu_state(cpu)->tctx);
> + return 0;
> +}
> +
> static void spapr_xive_set_irq(SpaprInterruptController *intc, int irq, int val)
> {
> SpaprXive *xive = SPAPR_XIVE(intc);
> @@ -697,6 +704,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..652771d6a5a5 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -295,6 +295,11 @@ static void icp_reset_handler(void *dev)
> }
> }
>
> +void icp_reset(ICPState *icp)
> +{
> + icp_reset_handler(icp);
> +}
> +
> static void icp_realize(DeviceState *dev, Error **errp)
> {
> ICPState *icp = ICP(dev);
> diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
> index 4f64b9a9fc66..c0b2a576effe 100644
> --- a/hw/intc/xics_spapr.c
> +++ b/hw/intc/xics_spapr.c
> @@ -346,6 +346,13 @@ static int xics_spapr_cpu_intc_create(SpaprInterruptController *intc,
> return 0;
> }
>
> +static int xics_spapr_cpu_intc_reset(SpaprInterruptController *intc,
> + PowerPCCPU *cpu, Error **errp)
> +{
> + icp_reset(spapr_cpu_state(cpu)->icp);
> + return 0;
> +}
> +
> static int xics_spapr_claim_irq(SpaprInterruptController *intc, int irq,
> bool lsi, Error **errp)
> {
> @@ -433,6 +440,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..0ae3f9b1efe4 100644
> --- a/hw/intc/xive.c
> +++ b/hw/intc/xive.c
> @@ -547,7 +547,7 @@ void xive_tctx_pic_print_info(XiveTCTX *tctx, Monitor *mon)
> }
> }
>
> -static void xive_tctx_reset(void *dev)
> +static void xive_tctx_reset_handler(void *dev)
> {
> XiveTCTX *tctx = XIVE_TCTX(dev);
>
> @@ -568,6 +568,11 @@ static void xive_tctx_reset(void *dev)
> ipb_to_pipr(tctx->regs[TM_QW3_HV_PHYS + TM_IPB]);
> }
>
> +void xive_tctx_reset(XiveTCTX *tctx)
> +{
> + xive_tctx_reset_handler(tctx);
> +}
> +
> static void xive_tctx_realize(DeviceState *dev, Error **errp)
> {
> XiveTCTX *tctx = XIVE_TCTX(dev);
> @@ -608,12 +613,12 @@ static void xive_tctx_realize(DeviceState *dev, Error **errp)
> }
> }
>
> - qemu_register_reset(xive_tctx_reset, dev);
> + qemu_register_reset(xive_tctx_reset_handler, dev);
> }
>
> static void xive_tctx_unrealize(DeviceState *dev, Error **errp)
> {
> - qemu_unregister_reset(xive_tctx_reset, dev);
> + qemu_unregister_reset(xive_tctx_reset_handler, dev);
> }
>
> static int vmstate_xive_tctx_pre_save(void *opaque)
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 3e4302c7d596..416aa75e5fba 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -33,6 +33,7 @@ static void spapr_cpu_reset(void *opaque)
> 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);
>
> @@ -77,9 +78,11 @@ static void spapr_cpu_reset(void *opaque)
> 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, &error_fatal);
So now, interrupt presenters are reset twice: from here and
from qemu_devices_reset(). Not a big deal, but if you add
something equivalent in the pnv machine, then the presenters
no longer need to call qemu_register_reset() at all.
> }
>
> void spapr_cpu_set_entry_state(PowerPCCPU *cpu, target_ulong nip, target_ulong r3)
> @@ -235,12 +238,13 @@ static void spapr_realize_vcpu(PowerPCCPU *cpu, SpaprMachineState *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;
> }
>
> + spapr_cpu_reset(cpu);
> +
> if (!sc->pre_3_0_migration) {
> vmstate_register(NULL, cs->cpu_index, &vmstate_spapr_cpu_state,
> cpu->machine_data);
> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> index bb91c61fa000..5d2b64029cd5 100644
> --- a/hw/ppc/spapr_irq.c
> +++ b/hw/ppc/spapr_irq.c
> @@ -220,6 +220,27 @@ int spapr_irq_cpu_intc_create(SpaprMachineState *spapr,
> return 0;
> }
>
> +int spapr_irq_cpu_intc_reset(SpaprMachineState *spapr,
> + PowerPCCPU *cpu, Error **errp)
> +{
> + SpaprInterruptController *intcs[] = ALL_INTCS(spapr);
> + int i;
> + int rc;
> +
> + for (i = 0; i < ARRAY_SIZE(intcs); i++) {
> + SpaprInterruptController *intc = intcs[i];
> + if (intc) {
> + SpaprInterruptControllerClass *sicc = SPAPR_INTC_GET_CLASS(intc);
> + rc = sicc->cpu_intc_reset(intc, cpu, errp);
> + if (rc < 0) {
> + return rc;
> + }
> + }
> + }
> +
> + return 0;
> +}
> +
> static void spapr_set_irq(void *opaque, int irq, int level)
> {
> SpaprMachineState *spapr = SPAPR_MACHINE(opaque);
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] spapr/xive: Set the OS CAM line at reset
2019-10-17 14:42 ` [PATCH 2/2] spapr/xive: Set the OS CAM line at reset Cédric Le Goater
2019-10-18 3:55 ` David Gibson
@ 2019-10-18 8:07 ` Greg Kurz
2019-10-18 8:29 ` Cédric Le Goater
1 sibling, 1 reply; 14+ messages in thread
From: Greg Kurz @ 2019-10-18 8:07 UTC (permalink / raw)
To: Cédric Le Goater; +Cc: qemu-ppc, qemu-devel, David Gibson
On Thu, 17 Oct 2019 16:42:41 +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 in
> TCG or kernel_irqchip=off, QEMU needs to emulate the same behavior.
>
This is only related to kernel_irqchip=off, which is always the case
when running in TCG actually. Maybe rephrase to "When not running with
an in-kernel irqchip, QEMU needs..." ?
> Introduce a 'os-cam' property which will be used to set the OS CAM
> line at reset and remove the spapr_xive_set_tctx_os_cam() calls which
> are done when the XIVE interrupt controller are activated.
>
Since OS CAM is constant, I guess it is ok to make it a property.
Alternatively, you could pass it as an extra parameter to
xive_tctx_reset().
> This change also has the benefit to remove the use of CPU_FOREACH()
> which can be unsafe.
>
Nice !
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
> include/hw/ppc/spapr_xive.h | 1 -
> include/hw/ppc/xive.h | 4 +++-
> hw/intc/spapr_xive.c | 31 +++++--------------------------
> hw/intc/xive.c | 22 +++++++++++++++++++++-
> hw/ppc/pnv.c | 3 ++-
> 5 files changed, 31 insertions(+), 30 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/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
> index 99381639f50c..e273069c25a9 100644
> --- a/include/hw/ppc/xive.h
> +++ b/include/hw/ppc/xive.h
> @@ -319,6 +319,7 @@ typedef struct XiveTCTX {
> qemu_irq os_output;
>
> uint8_t regs[XIVE_TM_RING_COUNT * XIVE_TM_RING_SIZE];
> + uint32_t os_cam;
> } XiveTCTX;
>
> /*
> @@ -414,7 +415,8 @@ void xive_tctx_tm_write(XiveTCTX *tctx, hwaddr offset, uint64_t value,
> 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);
> +Object *xive_tctx_create(Object *cpu, XiveRouter *xrtr, uint32_t os_cam,
> + 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 0c3acf1a4192..71f138512a1c 100644
> --- a/hw/intc/spapr_xive.c
> +++ b/hw/intc/spapr_xive.c
> @@ -205,21 +205,13 @@ 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)
> +static uint32_t spapr_xive_get_os_cam(PowerPCCPU *cpu)
> {
> 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);
> + spapr_xive_cpu_to_nvt(cpu, &nvt_blk, &nvt_idx);
> + return xive_nvt_cam_line(nvt_blk, nvt_idx);
> }
>
> static void spapr_xive_end_reset(XiveEND *end)
> @@ -537,19 +529,14 @@ static int spapr_xive_cpu_intc_create(SpaprInterruptController *intc,
> SpaprXive *xive = SPAPR_XIVE(intc);
> Object *obj;
> SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu);
> + uint32_t os_cam = spapr_xive_get_os_cam(cpu);
>
> - obj = xive_tctx_create(OBJECT(cpu), XIVE_ROUTER(xive), errp);
> + obj = xive_tctx_create(OBJECT(cpu), XIVE_ROUTER(xive), os_cam, errp);
> if (!obj) {
> return -1;
> }
>
> 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;
> }
>
> @@ -650,14 +637,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);
> diff --git a/hw/intc/xive.c b/hw/intc/xive.c
> index 0ae3f9b1efe4..be4f2c974178 100644
> --- a/hw/intc/xive.c
> +++ b/hw/intc/xive.c
> @@ -566,6 +566,18 @@ static void xive_tctx_reset_handler(void *dev)
> ipb_to_pipr(tctx->regs[TM_QW1_OS + TM_IPB]);
> tctx->regs[TM_QW3_HV_PHYS + TM_PIPR] =
> ipb_to_pipr(tctx->regs[TM_QW3_HV_PHYS + TM_IPB]);
> +
> + /*
> + * (TCG) Set the OS CAM line of the thread interrupt context.
As per my remark above, this shouldn't mention TCG but rather
kernel-irqchip=off.
> + *
> + * 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.
> + */
> + if (tctx->os_cam) {
> + uint32_t qw1w2 = cpu_to_be32(TM_QW1W2_VO | tctx->os_cam);
> + memcpy(&tctx->regs[TM_QW1_OS + TM_WORD2], &qw1w2, 4);
> + }
> }
>
> void xive_tctx_reset(XiveTCTX *tctx)
> @@ -667,11 +679,17 @@ static const VMStateDescription vmstate_xive_tctx = {
> },
> };
>
> +static Property xive_tctx_properties[] = {
> + DEFINE_PROP_UINT32("os-cam", XiveTCTX, os_cam, 0),
> + DEFINE_PROP_END_OF_LIST(),
> +};
> +
> static void xive_tctx_class_init(ObjectClass *klass, void *data)
> {
> DeviceClass *dc = DEVICE_CLASS(klass);
>
> dc->desc = "XIVE Interrupt Thread Context";
> + dc->props = xive_tctx_properties;
> dc->realize = xive_tctx_realize;
> dc->unrealize = xive_tctx_unrealize;
> dc->vmsd = &vmstate_xive_tctx;
> @@ -689,7 +707,8 @@ static const TypeInfo xive_tctx_info = {
> .class_init = xive_tctx_class_init,
> };
>
> -Object *xive_tctx_create(Object *cpu, XiveRouter *xrtr, Error **errp)
> +Object *xive_tctx_create(Object *cpu, XiveRouter *xrtr, uint32_t os_cam,
> + Error **errp)
> {
> Error *local_err = NULL;
> Object *obj;
> @@ -698,6 +717,7 @@ Object *xive_tctx_create(Object *cpu, XiveRouter *xrtr, Error **errp)
> object_property_add_child(cpu, TYPE_XIVE_TCTX, obj, &error_abort);
> object_unref(obj);
> object_property_add_const_link(obj, "cpu", cpu, &error_abort);
> + object_property_set_int(obj, os_cam, "os-cam", &local_err);
> object_property_set_bool(obj, true, "realized", &local_err);
> if (local_err) {
> goto error;
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 7cf64b6d2533..99c06842573e 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -806,7 +806,8 @@ static void pnv_chip_power9_intc_create(PnvChip *chip, PowerPCCPU *cpu,
> * controller object is initialized afterwards. Hopefully, it's
> * only used at runtime.
> */
> - obj = xive_tctx_create(OBJECT(cpu), XIVE_ROUTER(&chip9->xive), &local_err);
> + obj = xive_tctx_create(OBJECT(cpu), XIVE_ROUTER(&chip9->xive), 0,
> + &local_err);
> if (local_err) {
> error_propagate(errp, local_err);
> return;
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] spapr: Introduce a interrupt presenter reset handler
2019-10-18 7:46 ` Greg Kurz
@ 2019-10-18 8:26 ` Cédric Le Goater
0 siblings, 0 replies; 14+ messages in thread
From: Cédric Le Goater @ 2019-10-18 8:26 UTC (permalink / raw)
To: Greg Kurz; +Cc: qemu-ppc, qemu-devel, David Gibson
On 18/10/2019 09:46, Greg Kurz wrote:
> On Thu, 17 Oct 2019 16:42:40 +0200
> Cédric Le Goater <clg@kaod.org> wrote:
>
>> The interrupt presenters are not reseted today. Extend the sPAPR IRQ
>> backend with a new cpu_intc_reset() handler which will be called by
>> the CPU reset handler.
>>
>> 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>
>> ---
>> include/hw/ppc/spapr_irq.h | 4 ++++
>> include/hw/ppc/xics.h | 1 +
>> include/hw/ppc/xive.h | 1 +
>> hw/intc/spapr_xive.c | 8 ++++++++
>> hw/intc/xics.c | 5 +++++
>> hw/intc/xics_spapr.c | 8 ++++++++
>> hw/intc/xive.c | 11 ++++++++---
>> hw/ppc/spapr_cpu_core.c | 8 ++++++--
>> hw/ppc/spapr_irq.c | 21 +++++++++++++++++++++
>> 9 files changed, 62 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
>> index 5e150a667902..78327496c102 100644
>> --- a/include/hw/ppc/spapr_irq.h
>> +++ b/include/hw/ppc/spapr_irq.h
>> @@ -52,6 +52,8 @@ typedef struct SpaprInterruptControllerClass {
>> */
>> int (*cpu_intc_create)(SpaprInterruptController *intc,
>> PowerPCCPU *cpu, Error **errp);
>> + int (*cpu_intc_reset)(SpaprInterruptController *intc, PowerPCCPU *cpu,
>> + Error **errp);
>
> Looking at the rest of the patch, it seems that we don't need error
> reporting. I suggest you make this void and drop the errp parameter.
yes. we can drop the error on that path.
>
>> int (*claim_irq)(SpaprInterruptController *intc, int irq, bool lsi,
>> Error **errp);
>> void (*free_irq)(SpaprInterruptController *intc, int irq);
>> @@ -68,6 +70,8 @@ void spapr_irq_update_active_intc(SpaprMachineState *spapr);
>>
>> int spapr_irq_cpu_intc_create(SpaprMachineState *spapr,
>> PowerPCCPU *cpu, Error **errp);
>> +int spapr_irq_cpu_intc_reset(SpaprMachineState *spapr,
>> + PowerPCCPU *cpu, Error **errp);
>> 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..0c3acf1a4192 100644
>> --- a/hw/intc/spapr_xive.c
>> +++ b/hw/intc/spapr_xive.c
>> @@ -553,6 +553,13 @@ static int spapr_xive_cpu_intc_create(SpaprInterruptController *intc,
>> return 0;
>> }
>>
>> +static int spapr_xive_cpu_intc_reset(SpaprInterruptController *intc,
>> + PowerPCCPU *cpu, Error **errp)
>> +{
>> + xive_tctx_reset(spapr_cpu_state(cpu)->tctx);
>> + return 0;
>> +}
>> +
>> static void spapr_xive_set_irq(SpaprInterruptController *intc, int irq, int val)
>> {
>> SpaprXive *xive = SPAPR_XIVE(intc);
>> @@ -697,6 +704,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..652771d6a5a5 100644
>> --- a/hw/intc/xics.c
>> +++ b/hw/intc/xics.c
>> @@ -295,6 +295,11 @@ static void icp_reset_handler(void *dev)
>> }
>> }
>>
>> +void icp_reset(ICPState *icp)
>> +{
>> + icp_reset_handler(icp);
>> +}
>> +
>> static void icp_realize(DeviceState *dev, Error **errp)
>> {
>> ICPState *icp = ICP(dev);
>> diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
>> index 4f64b9a9fc66..c0b2a576effe 100644
>> --- a/hw/intc/xics_spapr.c
>> +++ b/hw/intc/xics_spapr.c
>> @@ -346,6 +346,13 @@ static int xics_spapr_cpu_intc_create(SpaprInterruptController *intc,
>> return 0;
>> }
>>
>> +static int xics_spapr_cpu_intc_reset(SpaprInterruptController *intc,
>> + PowerPCCPU *cpu, Error **errp)
>> +{
>> + icp_reset(spapr_cpu_state(cpu)->icp);
>> + return 0;
>> +}
>> +
>> static int xics_spapr_claim_irq(SpaprInterruptController *intc, int irq,
>> bool lsi, Error **errp)
>> {
>> @@ -433,6 +440,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..0ae3f9b1efe4 100644
>> --- a/hw/intc/xive.c
>> +++ b/hw/intc/xive.c
>> @@ -547,7 +547,7 @@ void xive_tctx_pic_print_info(XiveTCTX *tctx, Monitor *mon)
>> }
>> }
>>
>> -static void xive_tctx_reset(void *dev)
>> +static void xive_tctx_reset_handler(void *dev)
>> {
>> XiveTCTX *tctx = XIVE_TCTX(dev);
>>
>> @@ -568,6 +568,11 @@ static void xive_tctx_reset(void *dev)
>> ipb_to_pipr(tctx->regs[TM_QW3_HV_PHYS + TM_IPB]);
>> }
>>
>> +void xive_tctx_reset(XiveTCTX *tctx)
>> +{
>> + xive_tctx_reset_handler(tctx);
>> +}
>> +
>> static void xive_tctx_realize(DeviceState *dev, Error **errp)
>> {
>> XiveTCTX *tctx = XIVE_TCTX(dev);
>> @@ -608,12 +613,12 @@ static void xive_tctx_realize(DeviceState *dev, Error **errp)
>> }
>> }
>>
>> - qemu_register_reset(xive_tctx_reset, dev);
>> + qemu_register_reset(xive_tctx_reset_handler, dev);
>> }
>>
>> static void xive_tctx_unrealize(DeviceState *dev, Error **errp)
>> {
>> - qemu_unregister_reset(xive_tctx_reset, dev);
>> + qemu_unregister_reset(xive_tctx_reset_handler, dev);
>> }
>>
>> static int vmstate_xive_tctx_pre_save(void *opaque)
>> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
>> index 3e4302c7d596..416aa75e5fba 100644
>> --- a/hw/ppc/spapr_cpu_core.c
>> +++ b/hw/ppc/spapr_cpu_core.c
>> @@ -33,6 +33,7 @@ static void spapr_cpu_reset(void *opaque)
>> 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);
>>
>> @@ -77,9 +78,11 @@ static void spapr_cpu_reset(void *opaque)
>> 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, &error_fatal);
>
> So now, interrupt presenters are reset twice: from here and
> from qemu_devices_reset(). Not a big deal, but if you add
> something equivalent in the pnv machine, then the presenters
> no longer need to call qemu_register_reset() at all.
indeed. We could remove the reset handler in the presenter model.
>> }
>>
>> void spapr_cpu_set_entry_state(PowerPCCPU *cpu, target_ulong nip, target_ulong r3)
>> @@ -235,12 +238,13 @@ static void spapr_realize_vcpu(PowerPCCPU *cpu, SpaprMachineState *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;
>> }
>>
>> + spapr_cpu_reset(cpu);
>> +
We need to fix that reset one day. I will add a comment to say it is for
hotplug.
C.
>> if (!sc->pre_3_0_migration) {
>> vmstate_register(NULL, cs->cpu_index, &vmstate_spapr_cpu_state,
>> cpu->machine_data);
>> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
>> index bb91c61fa000..5d2b64029cd5 100644
>> --- a/hw/ppc/spapr_irq.c
>> +++ b/hw/ppc/spapr_irq.c
>> @@ -220,6 +220,27 @@ int spapr_irq_cpu_intc_create(SpaprMachineState *spapr,
>> return 0;
>> }
>>
>> +int spapr_irq_cpu_intc_reset(SpaprMachineState *spapr,
>> + PowerPCCPU *cpu, Error **errp)
>> +{
>> + SpaprInterruptController *intcs[] = ALL_INTCS(spapr);
>> + int i;
>> + int rc;
>> +
>> + for (i = 0; i < ARRAY_SIZE(intcs); i++) {
>> + SpaprInterruptController *intc = intcs[i];
>> + if (intc) {
>> + SpaprInterruptControllerClass *sicc = SPAPR_INTC_GET_CLASS(intc);
>> + rc = sicc->cpu_intc_reset(intc, cpu, errp);
>> + if (rc < 0) {
>> + return rc;
>> + }
>> + }
>> + }
>> +
>> + return 0;
>> +}
>> +
>> static void spapr_set_irq(void *opaque, int irq, int level)
>> {
>> SpaprMachineState *spapr = SPAPR_MACHINE(opaque);
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] spapr/xive: Set the OS CAM line at reset
2019-10-18 8:07 ` Greg Kurz
@ 2019-10-18 8:29 ` Cédric Le Goater
0 siblings, 0 replies; 14+ messages in thread
From: Cédric Le Goater @ 2019-10-18 8:29 UTC (permalink / raw)
To: Greg Kurz; +Cc: qemu-ppc, qemu-devel, David Gibson
On 18/10/2019 10:07, Greg Kurz wrote:
> On Thu, 17 Oct 2019 16:42:41 +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 in
>> TCG or kernel_irqchip=off, QEMU needs to emulate the same behavior.
>>
>
> This is only related to kernel_irqchip=off, which is always the case
> when running in TCG actually. Maybe rephrase to "When not running with
> an in-kernel irqchip, QEMU needs..." ?
yes.
>> Introduce a 'os-cam' property which will be used to set the OS CAM
>> line at reset and remove the spapr_xive_set_tctx_os_cam() calls which
>> are done when the XIVE interrupt controller are activated.
>>
>
> Since OS CAM is constant, I guess it is ok to make it a property.
> Alternatively, you could pass it as an extra parameter to
> xive_tctx_reset().
indeed. We have all we need to do that. I will wait for some feedback.
>> This change also has the benefit to remove the use of CPU_FOREACH()
>> which can be unsafe.
>>
>
> Nice !
>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>> include/hw/ppc/spapr_xive.h | 1 -
>> include/hw/ppc/xive.h | 4 +++-
>> hw/intc/spapr_xive.c | 31 +++++--------------------------
>> hw/intc/xive.c | 22 +++++++++++++++++++++-
>> hw/ppc/pnv.c | 3 ++-
>> 5 files changed, 31 insertions(+), 30 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/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
>> index 99381639f50c..e273069c25a9 100644
>> --- a/include/hw/ppc/xive.h
>> +++ b/include/hw/ppc/xive.h
>> @@ -319,6 +319,7 @@ typedef struct XiveTCTX {
>> qemu_irq os_output;
>>
>> uint8_t regs[XIVE_TM_RING_COUNT * XIVE_TM_RING_SIZE];
>> + uint32_t os_cam;
>> } XiveTCTX;
>>
>> /*
>> @@ -414,7 +415,8 @@ void xive_tctx_tm_write(XiveTCTX *tctx, hwaddr offset, uint64_t value,
>> 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);
>> +Object *xive_tctx_create(Object *cpu, XiveRouter *xrtr, uint32_t os_cam,
>> + 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 0c3acf1a4192..71f138512a1c 100644
>> --- a/hw/intc/spapr_xive.c
>> +++ b/hw/intc/spapr_xive.c
>> @@ -205,21 +205,13 @@ 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)
>> +static uint32_t spapr_xive_get_os_cam(PowerPCCPU *cpu)
>> {
>> 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);
>> + spapr_xive_cpu_to_nvt(cpu, &nvt_blk, &nvt_idx);
>> + return xive_nvt_cam_line(nvt_blk, nvt_idx);
>> }
>>
>> static void spapr_xive_end_reset(XiveEND *end)
>> @@ -537,19 +529,14 @@ static int spapr_xive_cpu_intc_create(SpaprInterruptController *intc,
>> SpaprXive *xive = SPAPR_XIVE(intc);
>> Object *obj;
>> SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu);
>> + uint32_t os_cam = spapr_xive_get_os_cam(cpu);
>>
>> - obj = xive_tctx_create(OBJECT(cpu), XIVE_ROUTER(xive), errp);
>> + obj = xive_tctx_create(OBJECT(cpu), XIVE_ROUTER(xive), os_cam, errp);
>> if (!obj) {
>> return -1;
>> }
>>
>> 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;
>> }
>>
>> @@ -650,14 +637,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);
>> diff --git a/hw/intc/xive.c b/hw/intc/xive.c
>> index 0ae3f9b1efe4..be4f2c974178 100644
>> --- a/hw/intc/xive.c
>> +++ b/hw/intc/xive.c
>> @@ -566,6 +566,18 @@ static void xive_tctx_reset_handler(void *dev)
>> ipb_to_pipr(tctx->regs[TM_QW1_OS + TM_IPB]);
>> tctx->regs[TM_QW3_HV_PHYS + TM_PIPR] =
>> ipb_to_pipr(tctx->regs[TM_QW3_HV_PHYS + TM_IPB]);
>> +
>> + /*
>> + * (TCG) Set the OS CAM line of the thread interrupt context.
>
> As per my remark above, this shouldn't mention TCG but rather
> kernel-irqchip=off.
ok.
Thanks,
C.
>
>> + *
>> + * 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.
>> + */
>> + if (tctx->os_cam) {
>> + uint32_t qw1w2 = cpu_to_be32(TM_QW1W2_VO | tctx->os_cam);
>> + memcpy(&tctx->regs[TM_QW1_OS + TM_WORD2], &qw1w2, 4);
>> + }
>> }
>>
>> void xive_tctx_reset(XiveTCTX *tctx)
>> @@ -667,11 +679,17 @@ static const VMStateDescription vmstate_xive_tctx = {
>> },
>> };
>>
>> +static Property xive_tctx_properties[] = {
>> + DEFINE_PROP_UINT32("os-cam", XiveTCTX, os_cam, 0),
>> + DEFINE_PROP_END_OF_LIST(),
>> +};
>> +
>> static void xive_tctx_class_init(ObjectClass *klass, void *data)
>> {
>> DeviceClass *dc = DEVICE_CLASS(klass);
>>
>> dc->desc = "XIVE Interrupt Thread Context";
>> + dc->props = xive_tctx_properties;
>> dc->realize = xive_tctx_realize;
>> dc->unrealize = xive_tctx_unrealize;
>> dc->vmsd = &vmstate_xive_tctx;
>> @@ -689,7 +707,8 @@ static const TypeInfo xive_tctx_info = {
>> .class_init = xive_tctx_class_init,
>> };
>>
>> -Object *xive_tctx_create(Object *cpu, XiveRouter *xrtr, Error **errp)
>> +Object *xive_tctx_create(Object *cpu, XiveRouter *xrtr, uint32_t os_cam,
>> + Error **errp)
>> {
>> Error *local_err = NULL;
>> Object *obj;
>> @@ -698,6 +717,7 @@ Object *xive_tctx_create(Object *cpu, XiveRouter *xrtr, Error **errp)
>> object_property_add_child(cpu, TYPE_XIVE_TCTX, obj, &error_abort);
>> object_unref(obj);
>> object_property_add_const_link(obj, "cpu", cpu, &error_abort);
>> + object_property_set_int(obj, os_cam, "os-cam", &local_err);
>> object_property_set_bool(obj, true, "realized", &local_err);
>> if (local_err) {
>> goto error;
>> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
>> index 7cf64b6d2533..99c06842573e 100644
>> --- a/hw/ppc/pnv.c
>> +++ b/hw/ppc/pnv.c
>> @@ -806,7 +806,8 @@ static void pnv_chip_power9_intc_create(PnvChip *chip, PowerPCCPU *cpu,
>> * controller object is initialized afterwards. Hopefully, it's
>> * only used at runtime.
>> */
>> - obj = xive_tctx_create(OBJECT(cpu), XIVE_ROUTER(&chip9->xive), &local_err);
>> + obj = xive_tctx_create(OBJECT(cpu), XIVE_ROUTER(&chip9->xive), 0,
>> + &local_err);
>> if (local_err) {
>> error_propagate(errp, local_err);
>> return;
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] spapr: Introduce a interrupt presenter reset handler
2019-10-18 2:47 ` David Gibson
@ 2019-10-18 9:41 ` Greg Kurz
2019-10-18 9:44 ` Cédric Le Goater
1 sibling, 0 replies; 14+ messages in thread
From: Greg Kurz @ 2019-10-18 9:41 UTC (permalink / raw)
To: David Gibson; +Cc: qemu-ppc, Cédric Le Goater, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 10661 bytes --]
On Fri, 18 Oct 2019 13:47:07 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:
> On Thu, Oct 17, 2019 at 04:42:40PM +0200, Cédric Le Goater wrote:
> > The interrupt presenters are not reseted today.
>
> I don't think that's accurate. We register reset handlers for both
> ICP and TCTX already. We might not be resetting in quite the right
> order, but this will need a clearer description of what's changing.
>
> Also, with this patch as is, I think we'll reset twice (once from the
> registered handler, once via the cpu).
>
That makes three at first boot since we also reset the CPU during
realize :) But yes, if we go that way, we should probably change
the pnv machine to do the same and stop registering reset handlers.
> > Extend the sPAPR IRQ
> > backend with a new cpu_intc_reset() handler which will be called by
> > the CPU reset handler.
> >
> > 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>
> > ---
> > include/hw/ppc/spapr_irq.h | 4 ++++
> > include/hw/ppc/xics.h | 1 +
> > include/hw/ppc/xive.h | 1 +
> > hw/intc/spapr_xive.c | 8 ++++++++
> > hw/intc/xics.c | 5 +++++
> > hw/intc/xics_spapr.c | 8 ++++++++
> > hw/intc/xive.c | 11 ++++++++---
> > hw/ppc/spapr_cpu_core.c | 8 ++++++--
> > hw/ppc/spapr_irq.c | 21 +++++++++++++++++++++
> > 9 files changed, 62 insertions(+), 5 deletions(-)
> >
> > diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
> > index 5e150a667902..78327496c102 100644
> > --- a/include/hw/ppc/spapr_irq.h
> > +++ b/include/hw/ppc/spapr_irq.h
> > @@ -52,6 +52,8 @@ typedef struct SpaprInterruptControllerClass {
> > */
> > int (*cpu_intc_create)(SpaprInterruptController *intc,
> > PowerPCCPU *cpu, Error **errp);
> > + int (*cpu_intc_reset)(SpaprInterruptController *intc, PowerPCCPU *cpu,
> > + Error **errp);
> > int (*claim_irq)(SpaprInterruptController *intc, int irq, bool lsi,
> > Error **errp);
> > void (*free_irq)(SpaprInterruptController *intc, int irq);
> > @@ -68,6 +70,8 @@ void spapr_irq_update_active_intc(SpaprMachineState *spapr);
> >
> > int spapr_irq_cpu_intc_create(SpaprMachineState *spapr,
> > PowerPCCPU *cpu, Error **errp);
> > +int spapr_irq_cpu_intc_reset(SpaprMachineState *spapr,
> > + PowerPCCPU *cpu, Error **errp);
> > 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..0c3acf1a4192 100644
> > --- a/hw/intc/spapr_xive.c
> > +++ b/hw/intc/spapr_xive.c
> > @@ -553,6 +553,13 @@ static int spapr_xive_cpu_intc_create(SpaprInterruptController *intc,
> > return 0;
> > }
> >
> > +static int spapr_xive_cpu_intc_reset(SpaprInterruptController *intc,
> > + PowerPCCPU *cpu, Error **errp)
> > +{
> > + xive_tctx_reset(spapr_cpu_state(cpu)->tctx);
> > + return 0;
> > +}
> > +
> > static void spapr_xive_set_irq(SpaprInterruptController *intc, int irq, int val)
> > {
> > SpaprXive *xive = SPAPR_XIVE(intc);
> > @@ -697,6 +704,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..652771d6a5a5 100644
> > --- a/hw/intc/xics.c
> > +++ b/hw/intc/xics.c
> > @@ -295,6 +295,11 @@ static void icp_reset_handler(void *dev)
> > }
> > }
> >
> > +void icp_reset(ICPState *icp)
> > +{
> > + icp_reset_handler(icp);
> > +}
> > +
> > static void icp_realize(DeviceState *dev, Error **errp)
> > {
> > ICPState *icp = ICP(dev);
> > diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
> > index 4f64b9a9fc66..c0b2a576effe 100644
> > --- a/hw/intc/xics_spapr.c
> > +++ b/hw/intc/xics_spapr.c
> > @@ -346,6 +346,13 @@ static int xics_spapr_cpu_intc_create(SpaprInterruptController *intc,
> > return 0;
> > }
> >
> > +static int xics_spapr_cpu_intc_reset(SpaprInterruptController *intc,
> > + PowerPCCPU *cpu, Error **errp)
> > +{
> > + icp_reset(spapr_cpu_state(cpu)->icp);
> > + return 0;
> > +}
> > +
> > static int xics_spapr_claim_irq(SpaprInterruptController *intc, int irq,
> > bool lsi, Error **errp)
> > {
> > @@ -433,6 +440,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..0ae3f9b1efe4 100644
> > --- a/hw/intc/xive.c
> > +++ b/hw/intc/xive.c
> > @@ -547,7 +547,7 @@ void xive_tctx_pic_print_info(XiveTCTX *tctx, Monitor *mon)
> > }
> > }
> >
> > -static void xive_tctx_reset(void *dev)
> > +static void xive_tctx_reset_handler(void *dev)
> > {
> > XiveTCTX *tctx = XIVE_TCTX(dev);
> >
> > @@ -568,6 +568,11 @@ static void xive_tctx_reset(void *dev)
> > ipb_to_pipr(tctx->regs[TM_QW3_HV_PHYS + TM_IPB]);
> > }
> >
> > +void xive_tctx_reset(XiveTCTX *tctx)
> > +{
> > + xive_tctx_reset_handler(tctx);
> > +}
> > +
> > static void xive_tctx_realize(DeviceState *dev, Error **errp)
> > {
> > XiveTCTX *tctx = XIVE_TCTX(dev);
> > @@ -608,12 +613,12 @@ static void xive_tctx_realize(DeviceState *dev, Error **errp)
> > }
> > }
> >
> > - qemu_register_reset(xive_tctx_reset, dev);
> > + qemu_register_reset(xive_tctx_reset_handler, dev);
> > }
> >
> > static void xive_tctx_unrealize(DeviceState *dev, Error **errp)
> > {
> > - qemu_unregister_reset(xive_tctx_reset, dev);
> > + qemu_unregister_reset(xive_tctx_reset_handler, dev);
> > }
> >
> > static int vmstate_xive_tctx_pre_save(void *opaque)
> > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > index 3e4302c7d596..416aa75e5fba 100644
> > --- a/hw/ppc/spapr_cpu_core.c
> > +++ b/hw/ppc/spapr_cpu_core.c
> > @@ -33,6 +33,7 @@ static void spapr_cpu_reset(void *opaque)
> > 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);
> >
> > @@ -77,9 +78,11 @@ static void spapr_cpu_reset(void *opaque)
> > 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, &error_fatal);
> > }
> >
> > void spapr_cpu_set_entry_state(PowerPCCPU *cpu, target_ulong nip, target_ulong r3)
> > @@ -235,12 +238,13 @@ static void spapr_realize_vcpu(PowerPCCPU *cpu, SpaprMachineState *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;
> > }
> >
> > + spapr_cpu_reset(cpu);
> > +
> > if (!sc->pre_3_0_migration) {
> > vmstate_register(NULL, cs->cpu_index, &vmstate_spapr_cpu_state,
> > cpu->machine_data);
> > diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> > index bb91c61fa000..5d2b64029cd5 100644
> > --- a/hw/ppc/spapr_irq.c
> > +++ b/hw/ppc/spapr_irq.c
> > @@ -220,6 +220,27 @@ int spapr_irq_cpu_intc_create(SpaprMachineState *spapr,
> > return 0;
> > }
> >
> > +int spapr_irq_cpu_intc_reset(SpaprMachineState *spapr,
> > + PowerPCCPU *cpu, Error **errp)
> > +{
> > + SpaprInterruptController *intcs[] = ALL_INTCS(spapr);
> > + int i;
> > + int rc;
> > +
> > + for (i = 0; i < ARRAY_SIZE(intcs); i++) {
> > + SpaprInterruptController *intc = intcs[i];
> > + if (intc) {
> > + SpaprInterruptControllerClass *sicc = SPAPR_INTC_GET_CLASS(intc);
> > + rc = sicc->cpu_intc_reset(intc, cpu, errp);
> > + if (rc < 0) {
> > + return rc;
> > + }
> > + }
> > + }
> > +
> > + return 0;
> > +}
> > +
> > static void spapr_set_irq(void *opaque, int irq, int level)
> > {
> > SpaprMachineState *spapr = SPAPR_MACHINE(opaque);
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] spapr/xive: Set the OS CAM line at reset
2019-10-18 3:55 ` David Gibson
@ 2019-10-18 9:42 ` Cédric Le Goater
2019-10-18 10:41 ` Cédric Le Goater
0 siblings, 1 reply; 14+ messages in thread
From: Cédric Le Goater @ 2019-10-18 9:42 UTC (permalink / raw)
To: David Gibson; +Cc: qemu-ppc, qemu-devel, Greg Kurz
On 18/10/2019 05:55, David Gibson wrote:
> On Thu, Oct 17, 2019 at 04:42:41PM +0200, Cédric Le Goater 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 in
>> TCG or kernel_irqchip=off, QEMU needs to emulate the same behavior.
>>
>> Introduce a 'os-cam' property which will be used to set the OS CAM
>> line at reset and remove the spapr_xive_set_tctx_os_cam() calls which
>> are done when the XIVE interrupt controller are activated.
>
> I'm not immediately seeing the advantage of doing this via a property,
> rather than poking it from the PAPR code which already knows the right
> values.
we can simplify by passing the OS CAM line value as a parameter of the
xive_tctx_reset routine, as suggested by Greg.
>
> Also, let me check my understanding:
> IIUC, on powernv the OS (running in HV mode) can alter the OS CAM
> lines for itself
OPAL only sets the VT bit in the HW cam line.
Linux PowerNV sets the POOL CAM line.
> and/or its guests,
KVM sets the OS CAM line when a vCPU is scheduled to run.
> but for pseries they're fixed in place. Is that right?
QEMU emulates KVM and sets the OS CAM line to a value similar to what KVM
would use. We can consider this value a reset constant.
C.
>> 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 -
>> include/hw/ppc/xive.h | 4 +++-
>> hw/intc/spapr_xive.c | 31 +++++--------------------------
>> hw/intc/xive.c | 22 +++++++++++++++++++++-
>> hw/ppc/pnv.c | 3 ++-
>> 5 files changed, 31 insertions(+), 30 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/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
>> index 99381639f50c..e273069c25a9 100644
>> --- a/include/hw/ppc/xive.h
>> +++ b/include/hw/ppc/xive.h
>> @@ -319,6 +319,7 @@ typedef struct XiveTCTX {
>> qemu_irq os_output;
>>
>> uint8_t regs[XIVE_TM_RING_COUNT * XIVE_TM_RING_SIZE];
>> + uint32_t os_cam;
>> } XiveTCTX;
>>
>> /*
>> @@ -414,7 +415,8 @@ void xive_tctx_tm_write(XiveTCTX *tctx, hwaddr offset, uint64_t value,
>> 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);
>> +Object *xive_tctx_create(Object *cpu, XiveRouter *xrtr, uint32_t os_cam,
>> + 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 0c3acf1a4192..71f138512a1c 100644
>> --- a/hw/intc/spapr_xive.c
>> +++ b/hw/intc/spapr_xive.c
>> @@ -205,21 +205,13 @@ 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)
>> +static uint32_t spapr_xive_get_os_cam(PowerPCCPU *cpu)
>> {
>> 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);
>> + spapr_xive_cpu_to_nvt(cpu, &nvt_blk, &nvt_idx);
>> + return xive_nvt_cam_line(nvt_blk, nvt_idx);
>> }
>>
>> static void spapr_xive_end_reset(XiveEND *end)
>> @@ -537,19 +529,14 @@ static int spapr_xive_cpu_intc_create(SpaprInterruptController *intc,
>> SpaprXive *xive = SPAPR_XIVE(intc);
>> Object *obj;
>> SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu);
>> + uint32_t os_cam = spapr_xive_get_os_cam(cpu);
>>
>> - obj = xive_tctx_create(OBJECT(cpu), XIVE_ROUTER(xive), errp);
>> + obj = xive_tctx_create(OBJECT(cpu), XIVE_ROUTER(xive), os_cam, errp);
>> if (!obj) {
>> return -1;
>> }
>>
>> 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;
>> }
>>
>> @@ -650,14 +637,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);
>> diff --git a/hw/intc/xive.c b/hw/intc/xive.c
>> index 0ae3f9b1efe4..be4f2c974178 100644
>> --- a/hw/intc/xive.c
>> +++ b/hw/intc/xive.c
>> @@ -566,6 +566,18 @@ static void xive_tctx_reset_handler(void *dev)
>> ipb_to_pipr(tctx->regs[TM_QW1_OS + TM_IPB]);
>> tctx->regs[TM_QW3_HV_PHYS + TM_PIPR] =
>> ipb_to_pipr(tctx->regs[TM_QW3_HV_PHYS + TM_IPB]);
>> +
>> + /*
>> + * (TCG) Set the OS CAM line of the thread interrupt context.
>> + *
>> + * 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.
>> + */
>> + if (tctx->os_cam) {
>> + uint32_t qw1w2 = cpu_to_be32(TM_QW1W2_VO | tctx->os_cam);
>> + memcpy(&tctx->regs[TM_QW1_OS + TM_WORD2], &qw1w2, 4);
>> + }
>> }
>>
>> void xive_tctx_reset(XiveTCTX *tctx)
>> @@ -667,11 +679,17 @@ static const VMStateDescription vmstate_xive_tctx = {
>> },
>> };
>>
>> +static Property xive_tctx_properties[] = {
>> + DEFINE_PROP_UINT32("os-cam", XiveTCTX, os_cam, 0),
>> + DEFINE_PROP_END_OF_LIST(),
>> +};
>> +
>> static void xive_tctx_class_init(ObjectClass *klass, void *data)
>> {
>> DeviceClass *dc = DEVICE_CLASS(klass);
>>
>> dc->desc = "XIVE Interrupt Thread Context";
>> + dc->props = xive_tctx_properties;
>> dc->realize = xive_tctx_realize;
>> dc->unrealize = xive_tctx_unrealize;
>> dc->vmsd = &vmstate_xive_tctx;
>> @@ -689,7 +707,8 @@ static const TypeInfo xive_tctx_info = {
>> .class_init = xive_tctx_class_init,
>> };
>>
>> -Object *xive_tctx_create(Object *cpu, XiveRouter *xrtr, Error **errp)
>> +Object *xive_tctx_create(Object *cpu, XiveRouter *xrtr, uint32_t os_cam,
>> + Error **errp)
>> {
>> Error *local_err = NULL;
>> Object *obj;
>> @@ -698,6 +717,7 @@ Object *xive_tctx_create(Object *cpu, XiveRouter *xrtr, Error **errp)
>> object_property_add_child(cpu, TYPE_XIVE_TCTX, obj, &error_abort);
>> object_unref(obj);
>> object_property_add_const_link(obj, "cpu", cpu, &error_abort);
>> + object_property_set_int(obj, os_cam, "os-cam", &local_err);
>> object_property_set_bool(obj, true, "realized", &local_err);
>> if (local_err) {
>> goto error;
>> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
>> index 7cf64b6d2533..99c06842573e 100644
>> --- a/hw/ppc/pnv.c
>> +++ b/hw/ppc/pnv.c
>> @@ -806,7 +806,8 @@ static void pnv_chip_power9_intc_create(PnvChip *chip, PowerPCCPU *cpu,
>> * controller object is initialized afterwards. Hopefully, it's
>> * only used at runtime.
>> */
>> - obj = xive_tctx_create(OBJECT(cpu), XIVE_ROUTER(&chip9->xive), &local_err);
>> + obj = xive_tctx_create(OBJECT(cpu), XIVE_ROUTER(&chip9->xive), 0,
>> + &local_err);
>> if (local_err) {
>> error_propagate(errp, local_err);
>> return;
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] spapr: Introduce a interrupt presenter reset handler
2019-10-18 2:47 ` David Gibson
2019-10-18 9:41 ` Greg Kurz
@ 2019-10-18 9:44 ` Cédric Le Goater
1 sibling, 0 replies; 14+ messages in thread
From: Cédric Le Goater @ 2019-10-18 9:44 UTC (permalink / raw)
To: David Gibson; +Cc: qemu-ppc, qemu-devel, Greg Kurz
On 18/10/2019 04:47, David Gibson wrote:
> On Thu, Oct 17, 2019 at 04:42:40PM +0200, Cédric Le Goater wrote:
>> The interrupt presenters are not reseted today.
>
> I don't think that's accurate. We register reset handlers for both
> ICP and TCTX already. We might not be resetting in quite the right
> order, but this will need a clearer description of what's changing.
>
> Also, with this patch as is, I think we'll reset twice (once from the
> registered handler, once via the cpu).
Unless the CPU is hotplugged, in which case it is only called once
in the realize handler ...
C.
>
>> Extend the sPAPR IRQ
>> backend with a new cpu_intc_reset() handler which will be called by
>> the CPU reset handler.
>>
>> 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>
>> ---
>> include/hw/ppc/spapr_irq.h | 4 ++++
>> include/hw/ppc/xics.h | 1 +
>> include/hw/ppc/xive.h | 1 +
>> hw/intc/spapr_xive.c | 8 ++++++++
>> hw/intc/xics.c | 5 +++++
>> hw/intc/xics_spapr.c | 8 ++++++++
>> hw/intc/xive.c | 11 ++++++++---
>> hw/ppc/spapr_cpu_core.c | 8 ++++++--
>> hw/ppc/spapr_irq.c | 21 +++++++++++++++++++++
>> 9 files changed, 62 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
>> index 5e150a667902..78327496c102 100644
>> --- a/include/hw/ppc/spapr_irq.h
>> +++ b/include/hw/ppc/spapr_irq.h
>> @@ -52,6 +52,8 @@ typedef struct SpaprInterruptControllerClass {
>> */
>> int (*cpu_intc_create)(SpaprInterruptController *intc,
>> PowerPCCPU *cpu, Error **errp);
>> + int (*cpu_intc_reset)(SpaprInterruptController *intc, PowerPCCPU *cpu,
>> + Error **errp);
>> int (*claim_irq)(SpaprInterruptController *intc, int irq, bool lsi,
>> Error **errp);
>> void (*free_irq)(SpaprInterruptController *intc, int irq);
>> @@ -68,6 +70,8 @@ void spapr_irq_update_active_intc(SpaprMachineState *spapr);
>>
>> int spapr_irq_cpu_intc_create(SpaprMachineState *spapr,
>> PowerPCCPU *cpu, Error **errp);
>> +int spapr_irq_cpu_intc_reset(SpaprMachineState *spapr,
>> + PowerPCCPU *cpu, Error **errp);
>> 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..0c3acf1a4192 100644
>> --- a/hw/intc/spapr_xive.c
>> +++ b/hw/intc/spapr_xive.c
>> @@ -553,6 +553,13 @@ static int spapr_xive_cpu_intc_create(SpaprInterruptController *intc,
>> return 0;
>> }
>>
>> +static int spapr_xive_cpu_intc_reset(SpaprInterruptController *intc,
>> + PowerPCCPU *cpu, Error **errp)
>> +{
>> + xive_tctx_reset(spapr_cpu_state(cpu)->tctx);
>> + return 0;
>> +}
>> +
>> static void spapr_xive_set_irq(SpaprInterruptController *intc, int irq, int val)
>> {
>> SpaprXive *xive = SPAPR_XIVE(intc);
>> @@ -697,6 +704,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..652771d6a5a5 100644
>> --- a/hw/intc/xics.c
>> +++ b/hw/intc/xics.c
>> @@ -295,6 +295,11 @@ static void icp_reset_handler(void *dev)
>> }
>> }
>>
>> +void icp_reset(ICPState *icp)
>> +{
>> + icp_reset_handler(icp);
>> +}
>> +
>> static void icp_realize(DeviceState *dev, Error **errp)
>> {
>> ICPState *icp = ICP(dev);
>> diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
>> index 4f64b9a9fc66..c0b2a576effe 100644
>> --- a/hw/intc/xics_spapr.c
>> +++ b/hw/intc/xics_spapr.c
>> @@ -346,6 +346,13 @@ static int xics_spapr_cpu_intc_create(SpaprInterruptController *intc,
>> return 0;
>> }
>>
>> +static int xics_spapr_cpu_intc_reset(SpaprInterruptController *intc,
>> + PowerPCCPU *cpu, Error **errp)
>> +{
>> + icp_reset(spapr_cpu_state(cpu)->icp);
>> + return 0;
>> +}
>> +
>> static int xics_spapr_claim_irq(SpaprInterruptController *intc, int irq,
>> bool lsi, Error **errp)
>> {
>> @@ -433,6 +440,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..0ae3f9b1efe4 100644
>> --- a/hw/intc/xive.c
>> +++ b/hw/intc/xive.c
>> @@ -547,7 +547,7 @@ void xive_tctx_pic_print_info(XiveTCTX *tctx, Monitor *mon)
>> }
>> }
>>
>> -static void xive_tctx_reset(void *dev)
>> +static void xive_tctx_reset_handler(void *dev)
>> {
>> XiveTCTX *tctx = XIVE_TCTX(dev);
>>
>> @@ -568,6 +568,11 @@ static void xive_tctx_reset(void *dev)
>> ipb_to_pipr(tctx->regs[TM_QW3_HV_PHYS + TM_IPB]);
>> }
>>
>> +void xive_tctx_reset(XiveTCTX *tctx)
>> +{
>> + xive_tctx_reset_handler(tctx);
>> +}
>> +
>> static void xive_tctx_realize(DeviceState *dev, Error **errp)
>> {
>> XiveTCTX *tctx = XIVE_TCTX(dev);
>> @@ -608,12 +613,12 @@ static void xive_tctx_realize(DeviceState *dev, Error **errp)
>> }
>> }
>>
>> - qemu_register_reset(xive_tctx_reset, dev);
>> + qemu_register_reset(xive_tctx_reset_handler, dev);
>> }
>>
>> static void xive_tctx_unrealize(DeviceState *dev, Error **errp)
>> {
>> - qemu_unregister_reset(xive_tctx_reset, dev);
>> + qemu_unregister_reset(xive_tctx_reset_handler, dev);
>> }
>>
>> static int vmstate_xive_tctx_pre_save(void *opaque)
>> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
>> index 3e4302c7d596..416aa75e5fba 100644
>> --- a/hw/ppc/spapr_cpu_core.c
>> +++ b/hw/ppc/spapr_cpu_core.c
>> @@ -33,6 +33,7 @@ static void spapr_cpu_reset(void *opaque)
>> 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);
>>
>> @@ -77,9 +78,11 @@ static void spapr_cpu_reset(void *opaque)
>> 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, &error_fatal);
>> }
>>
>> void spapr_cpu_set_entry_state(PowerPCCPU *cpu, target_ulong nip, target_ulong r3)
>> @@ -235,12 +238,13 @@ static void spapr_realize_vcpu(PowerPCCPU *cpu, SpaprMachineState *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;
>> }
>>
>> + spapr_cpu_reset(cpu);
>> +
>> if (!sc->pre_3_0_migration) {
>> vmstate_register(NULL, cs->cpu_index, &vmstate_spapr_cpu_state,
>> cpu->machine_data);
>> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
>> index bb91c61fa000..5d2b64029cd5 100644
>> --- a/hw/ppc/spapr_irq.c
>> +++ b/hw/ppc/spapr_irq.c
>> @@ -220,6 +220,27 @@ int spapr_irq_cpu_intc_create(SpaprMachineState *spapr,
>> return 0;
>> }
>>
>> +int spapr_irq_cpu_intc_reset(SpaprMachineState *spapr,
>> + PowerPCCPU *cpu, Error **errp)
>> +{
>> + SpaprInterruptController *intcs[] = ALL_INTCS(spapr);
>> + int i;
>> + int rc;
>> +
>> + for (i = 0; i < ARRAY_SIZE(intcs); i++) {
>> + SpaprInterruptController *intc = intcs[i];
>> + if (intc) {
>> + SpaprInterruptControllerClass *sicc = SPAPR_INTC_GET_CLASS(intc);
>> + rc = sicc->cpu_intc_reset(intc, cpu, errp);
>> + if (rc < 0) {
>> + return rc;
>> + }
>> + }
>> + }
>> +
>> + return 0;
>> +}
>> +
>> static void spapr_set_irq(void *opaque, int irq, int level)
>> {
>> SpaprMachineState *spapr = SPAPR_MACHINE(opaque);
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] spapr/xive: Set the OS CAM line at reset
2019-10-18 9:42 ` Cédric Le Goater
@ 2019-10-18 10:41 ` Cédric Le Goater
0 siblings, 0 replies; 14+ messages in thread
From: Cédric Le Goater @ 2019-10-18 10:41 UTC (permalink / raw)
To: David Gibson; +Cc: qemu-ppc, qemu-devel, Greg Kurz
On 18/10/2019 11:42, Cédric Le Goater wrote:
> On 18/10/2019 05:55, David Gibson wrote:
>> On Thu, Oct 17, 2019 at 04:42:41PM +0200, Cédric Le Goater 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 in
>>> TCG or kernel_irqchip=off, QEMU needs to emulate the same behavior.
>>>
>>> Introduce a 'os-cam' property which will be used to set the OS CAM
>>> line at reset and remove the spapr_xive_set_tctx_os_cam() calls which
>>> are done when the XIVE interrupt controller are activated.
>>
>> I'm not immediately seeing the advantage of doing this via a property,
>> rather than poking it from the PAPR code which already knows the right
>> values.
>
> we can simplify by passing the OS CAM line value as a parameter of the
> xive_tctx_reset routine, as suggested by Greg.
and if we remove the reset handlers from XiveTCTX and rely only on the
CPU reset handler to reset the presenter.
C.
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2019-10-18 10:42 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-17 14:42 [PATCH 0/2] spapr: interrupt presenter fixes Cédric Le Goater
2019-10-17 14:42 ` [PATCH 1/2] spapr: Introduce a interrupt presenter reset handler Cédric Le Goater
2019-10-18 2:47 ` David Gibson
2019-10-18 9:41 ` Greg Kurz
2019-10-18 9:44 ` Cédric Le Goater
2019-10-18 6:16 ` Cédric Le Goater
2019-10-18 7:46 ` Greg Kurz
2019-10-18 8:26 ` Cédric Le Goater
2019-10-17 14:42 ` [PATCH 2/2] spapr/xive: Set the OS CAM line at reset Cédric Le Goater
2019-10-18 3:55 ` David Gibson
2019-10-18 9:42 ` Cédric Le Goater
2019-10-18 10:41 ` Cédric Le Goater
2019-10-18 8:07 ` Greg Kurz
2019-10-18 8:29 ` Cédric Le Goater
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.