Re: [PATCH] ppc/spapr: Advertise StoreEOI for POWER10 compat guests
On 2/17/22 10:23, Cédric Le Goater wrote: On 2/17/22 12:28, Daniel Henrique Barboza wrote: On 2/14/22 11:11, Cédric Le Goater wrote: When an interrupt has been handled, the OS notifies the interrupt controller with a EOI sequence. On a POWER9 and POWER10 systems using the XIVE interrupt controller, this can be done with a load or a store operation on the ESB interrupt management page of the interrupt. The StoreEOI operation has less latency and improves interrupt handling performance but it was deactivated during the POWER9 DD2.0 timeframe because of ordering issues. POWER9 systems use the LoadEOI instead. POWER10 compat guests should have fixed the issue with Load-after-Store ordering and StoreEOI can be activated for them again. To maintain performance, this ordering is only enforced for the XIVE_ESB_SET_PQ_10 load operation. This operation can be used to disable temporarily an interrupt source. If StoreEOI is active, a source could be left enabled if the load and store operations come out of order. Add a check in our XIVE emulation model for Load-after-Store when StoreEOI is active. It should catch unreliable sequences. Other load operations should be fine without it. Signed-off-by: Cédric Le Goater --- Reviewed-by: Daniel Henrique Barboza Unfortunetaly, this patch breaks migration under TCG because the XIVE source flag is not updated on the target side. KVM is not impacted because the emulated sources are not used. This needs to be addressed in a v2. That said, even without this patch, TCG migration is broken. some CPUs on the receive side are stalled on CPU Hard LOCKUPs. QEMU 6.2 is impacted. So it has been a while :/ I've done a few tests and I can see Hard Lockups with TCG pseries migration, when using multiples CPUs (I used -smp 4 like you suggested in private), since at least QEMU v6.0.0. This is hardly surprising since TCG migration isn't something that we ever supported in a product or even in the community*. It would be good to understand why and get it fixed, but for now we can take a bit comfort in knowing that: - it has been broken for awhile (if ever worked). If this was a recent 7.0 regression we would need to solve it for this upcoming release; - single CPU TCG migration seems to be working fine, so we can count with this TCG migration scenario for testing. * I'm hoping David and Greg can push back on this if my assumption is wrong. Thanks, Daniel See below. C. [ 24.113608] watchdog: CPU 0 detected hard LOCKUP on other CPUs 1,3 [ 24.116534] watchdog: CPU 0 TB:15585461459, last SMP heartbeat TB:7394335409 (15998ms ago) [ 24.117840] watchdog: CPU 1 Hard LOCKUP [ 24.117956] watchdog: CPU 1 TB:15587843000, last heartbeat TB:5355690415 (19984ms ago) [ 24.117999] Modules linked in: [ 24.118387] irq event stamp: 341399 [ 24.118399] hardirqs last enabled at (341399): [] snooze_loop+0x9c/0x290 [ 24.118900] hardirqs last disabled at (341398): [] do_idle+0x12c/0x450 [ 24.118943] softirqs last enabled at (9798): [] __do_softirq+0x60c/0x678 [ 24.118971] softirqs last disabled at (9789): [] __irq_exit_rcu+0x158/0x1c0 [ 24.119127] CPU: 1 PID: 0 Comm: swapper/1 Not tainted 5.17.0-rc4-dirty #984 [ 24.119293] NIP: c0caea78 LR: c0caea38 CTR: c0cae990 [ 24.119315] REGS: c000fff43d60 TRAP: 0100 Not tainted (5.17.0-rc4-dirty) [ 24.119352] MSR: 8280b033 CR: 28000228 XER: 0006 [ 24.119554] CFAR: c0caea98 IRQMASK: 0 [ 24.119554] GPR00: c0caea2c c2bbbd80 c1c30b00 [ 24.119554] GPR04: 0006 c800 c1c7dc38 [ 24.119554] GPR08: c2b5d500 0003a115ef39 36d551ed [ 24.119554] GPR12: c0cae990 c000f300 [ 24.119554] GPR16: [ 24.119554] GPR20: c1b3a660 [ 24.119554] GPR24: c000ffa4fb48 00059d7c5070 c1c78e48 [ 24.119554] GPR28: c1b3a660 c15422e0 c15422e8 [ 24.119845] NIP [c0caea78] snooze_loop+0xe8/0x290 [ 24.119866] LR [c0caea38] snooze_loop+0xa8/0x290 [ 24.119998] Call Trace: [ 24.120029] [c2bbbd80] [c0caea2c] snooze_loop+0x9c/0x290 (unreliable) [ 24.120097] [c2bbbdc0] [c0cab730] cpuidle_enter_state+0x300/0x730 [ 24.120119] [c2bbbe30] [c0cabbfc] cpuidle_enter+0x4c/0x70 [ 24.120131] [c2bbbe70] [c0208d98] do_idle+0x328/0x450 [ 24.120141] [c2bbbf00] [c020926c] cpu_startup_entry+0x3c/0x40 [ 24.120150] [c2bbbf30] [c005e144] start_secondary+0x2a4/0x2b0 [ 24.120161] [c2bbbf90] [c000d054] start_secondary_prolog+0x10/0x14 [ 24.120238] Instruction
Re: [PATCH] ppc/spapr: Advertise StoreEOI for POWER10 compat guests
Unfortunately, this patch breaks migration under TCG because the XIVE
source flag is not updated on the target side. KVM is not impacted
because the emulated sources are not used. This needs to be addressed
in a v2.
That said, even without this patch, TCG migration is broken. some CPUs
on the receive side are stalled on CPU Hard LOCKUPs. QEMU 6.2 is impacted.
So it has been a while :/
Ouch. Guess we need to add TCG migration tests in the test workflow ...
Regarding the first issue with the new XIVE source flag, this routine
changes an object property after realize which is a no-no for migration :
void spapr_xive_enable_store_eoi(SpaprXive *xive, bool enable)
{
if (enable) {
xive->source.esb_flags |= XIVE_SRC_STORE_EOI;
} else {
xive->source.esb_flags &= ~XIVE_SRC_STORE_EOI;
}
}
I think we need a new SpaprXive state to represent the characteristic
of the source indirectly negotiated by CAS when the CPU is a POWER10.
we would use it to update xive->source.esb_flags at post_load time
after migration.
Or simply mimick CAS :
@@ -531,6 +531,14 @@ static int spapr_xive_post_load(SpaprInt
return kvmppc_xive_post_load(xive, version_id);
}
+PowerPCCPU *first_ppc_cpu = POWERPC_CPU(first_cpu);
+bool enable = ppc_check_compat(first_ppc_cpu, CPU_POWERPC_LOGICAL_3_10, 0,
+ first_ppc_cpu->compat_pvr);
+spapr_xive_enable_store_eoi(xive, enable);
+
return 0;
}
which has the benefit of being stateless.
Ideas ?
Thanks,
C.
Re: [PATCH] ppc/spapr: Advertise StoreEOI for POWER10 compat guests
On 2/17/22 10:23, Cédric Le Goater wrote: On 2/17/22 12:28, Daniel Henrique Barboza wrote: On 2/14/22 11:11, Cédric Le Goater wrote: When an interrupt has been handled, the OS notifies the interrupt controller with a EOI sequence. On a POWER9 and POWER10 systems using the XIVE interrupt controller, this can be done with a load or a store operation on the ESB interrupt management page of the interrupt. The StoreEOI operation has less latency and improves interrupt handling performance but it was deactivated during the POWER9 DD2.0 timeframe because of ordering issues. POWER9 systems use the LoadEOI instead. POWER10 compat guests should have fixed the issue with Load-after-Store ordering and StoreEOI can be activated for them again. To maintain performance, this ordering is only enforced for the XIVE_ESB_SET_PQ_10 load operation. This operation can be used to disable temporarily an interrupt source. If StoreEOI is active, a source could be left enabled if the load and store operations come out of order. Add a check in our XIVE emulation model for Load-after-Store when StoreEOI is active. It should catch unreliable sequences. Other load operations should be fine without it. Signed-off-by: Cédric Le Goater --- Reviewed-by: Daniel Henrique Barboza Unfortunetaly, this patch breaks migration under TCG because the XIVE source flag is not updated on the target side. KVM is not impacted because the emulated sources are not used. This needs to be addressed in a v2. That said, even without this patch, TCG migration is broken. some CPUs on the receive side are stalled on CPU Hard LOCKUPs. QEMU 6.2 is impacted. So it has been a while :/ Ouch. Guess we need to add TCG migration tests in the test workflow ... Daniel See below. C. [ 24.113608] watchdog: CPU 0 detected hard LOCKUP on other CPUs 1,3 [ 24.116534] watchdog: CPU 0 TB:15585461459, last SMP heartbeat TB:7394335409 (15998ms ago) [ 24.117840] watchdog: CPU 1 Hard LOCKUP [ 24.117956] watchdog: CPU 1 TB:15587843000, last heartbeat TB:5355690415 (19984ms ago) [ 24.117999] Modules linked in: [ 24.118387] irq event stamp: 341399 [ 24.118399] hardirqs last enabled at (341399): [] snooze_loop+0x9c/0x290 [ 24.118900] hardirqs last disabled at (341398): [] do_idle+0x12c/0x450 [ 24.118943] softirqs last enabled at (9798): [] __do_softirq+0x60c/0x678 [ 24.118971] softirqs last disabled at (9789): [] __irq_exit_rcu+0x158/0x1c0 [ 24.119127] CPU: 1 PID: 0 Comm: swapper/1 Not tainted 5.17.0-rc4-dirty #984 [ 24.119293] NIP: c0caea78 LR: c0caea38 CTR: c0cae990 [ 24.119315] REGS: c000fff43d60 TRAP: 0100 Not tainted (5.17.0-rc4-dirty) [ 24.119352] MSR: 8280b033 CR: 28000228 XER: 0006 [ 24.119554] CFAR: c0caea98 IRQMASK: 0 [ 24.119554] GPR00: c0caea2c c2bbbd80 c1c30b00 [ 24.119554] GPR04: 0006 c800 c1c7dc38 [ 24.119554] GPR08: c2b5d500 0003a115ef39 36d551ed [ 24.119554] GPR12: c0cae990 c000f300 [ 24.119554] GPR16: [ 24.119554] GPR20: c1b3a660 [ 24.119554] GPR24: c000ffa4fb48 00059d7c5070 c1c78e48 [ 24.119554] GPR28: c1b3a660 c15422e0 c15422e8 [ 24.119845] NIP [c0caea78] snooze_loop+0xe8/0x290 [ 24.119866] LR [c0caea38] snooze_loop+0xa8/0x290 [ 24.119998] Call Trace: [ 24.120029] [c2bbbd80] [c0caea2c] snooze_loop+0x9c/0x290 (unreliable) [ 24.120097] [c2bbbdc0] [c0cab730] cpuidle_enter_state+0x300/0x730 [ 24.120119] [c2bbbe30] [c0cabbfc] cpuidle_enter+0x4c/0x70 [ 24.120131] [c2bbbe70] [c0208d98] do_idle+0x328/0x450 [ 24.120141] [c2bbbf00] [c020926c] cpu_startup_entry+0x3c/0x40 [ 24.120150] [c2bbbf30] [c005e144] start_secondary+0x2a4/0x2b0 [ 24.120161] [c2bbbf90] [c000d054] start_secondary_prolog+0x10/0x14 [ 24.120238] Instruction dump: [ 24.120320] e9280080 e8c7d148 3ce20005 71290004 38e7d138 7d4a3214 4082003c 6000 [ 24.120357] 6000 6042 7c210b78 7b78 <8927000c> 2c09 41820010 7d2c42a6
Re: [PATCH] ppc/spapr: Advertise StoreEOI for POWER10 compat guests
On 2/17/22 12:28, Daniel Henrique Barboza wrote: On 2/14/22 11:11, Cédric Le Goater wrote: When an interrupt has been handled, the OS notifies the interrupt controller with a EOI sequence. On a POWER9 and POWER10 systems using the XIVE interrupt controller, this can be done with a load or a store operation on the ESB interrupt management page of the interrupt. The StoreEOI operation has less latency and improves interrupt handling performance but it was deactivated during the POWER9 DD2.0 timeframe because of ordering issues. POWER9 systems use the LoadEOI instead. POWER10 compat guests should have fixed the issue with Load-after-Store ordering and StoreEOI can be activated for them again. To maintain performance, this ordering is only enforced for the XIVE_ESB_SET_PQ_10 load operation. This operation can be used to disable temporarily an interrupt source. If StoreEOI is active, a source could be left enabled if the load and store operations come out of order. Add a check in our XIVE emulation model for Load-after-Store when StoreEOI is active. It should catch unreliable sequences. Other load operations should be fine without it. Signed-off-by: Cédric Le Goater --- Reviewed-by: Daniel Henrique Barboza Unfortunetaly, this patch breaks migration under TCG because the XIVE source flag is not updated on the target side. KVM is not impacted because the emulated sources are not used. This needs to be addressed in a v2. That said, even without this patch, TCG migration is broken. some CPUs on the receive side are stalled on CPU Hard LOCKUPs. QEMU 6.2 is impacted. So it has been a while :/ See below. C. [ 24.113608] watchdog: CPU 0 detected hard LOCKUP on other CPUs 1,3 [ 24.116534] watchdog: CPU 0 TB:15585461459, last SMP heartbeat TB:7394335409 (15998ms ago) [ 24.117840] watchdog: CPU 1 Hard LOCKUP [ 24.117956] watchdog: CPU 1 TB:15587843000, last heartbeat TB:5355690415 (19984ms ago) [ 24.117999] Modules linked in: [ 24.118387] irq event stamp: 341399 [ 24.118399] hardirqs last enabled at (341399): [] snooze_loop+0x9c/0x290 [ 24.118900] hardirqs last disabled at (341398): [] do_idle+0x12c/0x450 [ 24.118943] softirqs last enabled at (9798): [] __do_softirq+0x60c/0x678 [ 24.118971] softirqs last disabled at (9789): [] __irq_exit_rcu+0x158/0x1c0 [ 24.119127] CPU: 1 PID: 0 Comm: swapper/1 Not tainted 5.17.0-rc4-dirty #984 [ 24.119293] NIP: c0caea78 LR: c0caea38 CTR: c0cae990 [ 24.119315] REGS: c000fff43d60 TRAP: 0100 Not tainted (5.17.0-rc4-dirty) [ 24.119352] MSR: 8280b033 CR: 28000228 XER: 0006 [ 24.119554] CFAR: c0caea98 IRQMASK: 0 [ 24.119554] GPR00: c0caea2c c2bbbd80 c1c30b00 [ 24.119554] GPR04: 0006 c800 c1c7dc38 [ 24.119554] GPR08: c2b5d500 0003a115ef39 36d551ed [ 24.119554] GPR12: c0cae990 c000f300 [ 24.119554] GPR16: [ 24.119554] GPR20: c1b3a660 [ 24.119554] GPR24: c000ffa4fb48 00059d7c5070 c1c78e48 [ 24.119554] GPR28: c1b3a660 c15422e0 c15422e8 [ 24.119845] NIP [c0caea78] snooze_loop+0xe8/0x290 [ 24.119866] LR [c0caea38] snooze_loop+0xa8/0x290 [ 24.119998] Call Trace: [ 24.120029] [c2bbbd80] [c0caea2c] snooze_loop+0x9c/0x290 (unreliable) [ 24.120097] [c2bbbdc0] [c0cab730] cpuidle_enter_state+0x300/0x730 [ 24.120119] [c2bbbe30] [c0cabbfc] cpuidle_enter+0x4c/0x70 [ 24.120131] [c2bbbe70] [c0208d98] do_idle+0x328/0x450 [ 24.120141] [c2bbbf00] [c020926c] cpu_startup_entry+0x3c/0x40 [ 24.120150] [c2bbbf30] [c005e144] start_secondary+0x2a4/0x2b0 [ 24.120161] [c2bbbf90] [c000d054] start_secondary_prolog+0x10/0x14 [ 24.120238] Instruction dump: [ 24.120320] e9280080 e8c7d148 3ce20005 71290004 38e7d138 7d4a3214 4082003c 6000 [ 24.120357] 6000 6042 7c210b78 7b78 <8927000c> 2c09 41820010 7d2c42a6
Re: [PATCH] ppc/spapr: Advertise StoreEOI for POWER10 compat guests
On 2/14/22 11:11, Cédric Le Goater wrote:
When an interrupt has been handled, the OS notifies the interrupt
controller with a EOI sequence. On a POWER9 and POWER10 systems using
the XIVE interrupt controller, this can be done with a load or a store
operation on the ESB interrupt management page of the interrupt. The
StoreEOI operation has less latency and improves interrupt handling
performance but it was deactivated during the POWER9 DD2.0 timeframe
because of ordering issues. POWER9 systems use the LoadEOI instead.
POWER10 compat guests should have fixed the issue with
Load-after-Store ordering and StoreEOI can be activated for them
again.
To maintain performance, this ordering is only enforced for the
XIVE_ESB_SET_PQ_10 load operation. This operation can be used to
disable temporarily an interrupt source. If StoreEOI is active, a
source could be left enabled if the load and store operations come
out of order.
Add a check in our XIVE emulation model for Load-after-Store when
StoreEOI is active. It should catch unreliable sequences. Other load
operations should be fine without it.
Signed-off-by: Cédric Le Goater
---
Reviewed-by: Daniel Henrique Barboza
include/hw/ppc/spapr_xive.h | 1 +
include/hw/ppc/xive.h | 8
hw/intc/spapr_xive.c| 15 +++
hw/intc/spapr_xive_kvm.c| 15 +++
hw/intc/xive.c | 6 ++
hw/ppc/spapr_hcall.c| 7 +++
6 files changed, 52 insertions(+)
diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h
index b282960ad90d..9c247d8bf57d 100644
--- a/include/hw/ppc/spapr_xive.h
+++ b/include/hw/ppc/spapr_xive.h
@@ -73,6 +73,7 @@ void spapr_xive_map_mmio(SpaprXive *xive);
int spapr_xive_end_to_target(uint8_t end_blk, uint32_t end_idx,
uint32_t *out_server, uint8_t *out_prio);
+void spapr_xive_enable_store_eoi(SpaprXive *xive, bool enable);
/*
* KVM XIVE device helpers
diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
index 126e4e2c3a17..133f308c2792 100644
--- a/include/hw/ppc/xive.h
+++ b/include/hw/ppc/xive.h
@@ -285,6 +285,14 @@ uint8_t xive_esb_set(uint8_t *pq, uint8_t value);
#define XIVE_ESB_SET_PQ_10 0xe00 /* Load */
#define XIVE_ESB_SET_PQ_11 0xf00 /* Load */
+/*
+ * Load-after-store ordering
+ *
+ * Adding this offset to the load address will enforce
+ * load-after-store ordering. This is required to use with StoreEOI.
+ */
+#define XIVE_ESB_LD_ST_MO 0x40 /* Load-after-store ordering */
+
uint8_t xive_source_esb_get(XiveSource *xsrc, uint32_t srcno);
uint8_t xive_source_esb_set(XiveSource *xsrc, uint32_t srcno, uint8_t pq);
diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
index dc641cc604bf..0b8a246ad594 100644
--- a/hw/intc/spapr_xive.c
+++ b/hw/intc/spapr_xive.c
@@ -25,6 +25,7 @@
#include "hw/ppc/xive_regs.h"
#include "hw/qdev-properties.h"
#include "trace.h"
+#include "cpu-models.h"
/*
* XIVE Virtualization Controller BAR and Thread Managment BAR that we
@@ -1854,3 +1855,17 @@ void spapr_xive_hcall_init(SpaprMachineState *spapr)
spapr_register_hypercall(H_INT_SYNC, h_int_sync);
spapr_register_hypercall(H_INT_RESET, h_int_reset);
}
+
+/*
+ * Advertise StoreEOI for a P10 compat guest. OS is required to
+ * enforce load-after-store ordering.
+ */
+void spapr_xive_enable_store_eoi(SpaprXive *xive, bool enable)
+{
+if (enable) {
+xive->source.esb_flags |= XIVE_SRC_STORE_EOI;
+} else {
+xive->source.esb_flags &= ~XIVE_SRC_STORE_EOI;
+}
+
+}
diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c
index 61fe7bd2d322..bd023407bd7f 100644
--- a/hw/intc/spapr_xive_kvm.c
+++ b/hw/intc/spapr_xive_kvm.c
@@ -296,6 +296,21 @@ static uint64_t xive_esb_rw(XiveSource *xsrc, int srcno,
uint32_t offset,
static uint8_t xive_esb_read(XiveSource *xsrc, int srcno, uint32_t offset)
{
+/*
+ * The XIVE_ESB_SET_PQ_10 load operation is used to disable
+ * temporarily an interrupt source. If StoreEOI is active, a
+ * source could be left enabled if the load and store operations
+ * come out of order.
+ *
+ * As we don't know the characteristics of the host source
+ * interrupts (StoreEOI or not), enforce the load-after-store
+ * ordering always. The performance penalty will be very small for
+ * QEMU.
+ */
+if (offset == XIVE_ESB_SET_PQ_10) {
+offset |= XIVE_ESB_LD_ST_MO;
+}
+
return xive_esb_rw(xsrc, srcno, offset, 0, 0) & 0x3;
}
diff --git a/hw/intc/xive.c b/hw/intc/xive.c
index b8e4c7294d59..d62881873b1b 100644
--- a/hw/intc/xive.c
+++ b/hw/intc/xive.c
@@ -1024,6 +1024,12 @@ static uint64_t xive_source_esb_read(void *opaque,
hwaddr addr, unsigned size)
case XIVE_ESB_SET_PQ_01 ... XIVE_ESB_SET_PQ_01 + 0x0FF:
case XIVE_ESB_SET_PQ_10 ... XIVE_ESB_SET_PQ_10 + 0x0FF:
case XIVE_ESB_SET_PQ_11 ... XIVE_ESB_SET_PQ_11 + 0x0FF:
+if
Re: [PATCH] ppc/spapr: Advertise StoreEOI for POWER10 compat guests
On 2/17/22 04:51, Cédric Le Goater wrote:
[ adding a few people for the comments ]
On 2/16/22 23:17, Daniel Henrique Barboza wrote:
On 2/14/22 11:11, Cédric Le Goater wrote:
When an interrupt has been handled, the OS notifies the interrupt
controller with a EOI sequence. On a POWER9 and POWER10 systems using
nit: s/a EOI sequence/an EOI sequence
the XIVE interrupt controller, this can be done with a load or a store
operation on the ESB interrupt management page of the interrupt. The
StoreEOI operation has less latency and improves interrupt handling
performance but it was deactivated during the POWER9 DD2.0 timeframe
because of ordering issues. POWER9 systems use the LoadEOI instead.
POWER10 compat guests should have fixed the issue with
Load-after-Store ordering and StoreEOI can be activated for them
again.
To maintain performance, this ordering is only enforced for the
XIVE_ESB_SET_PQ_10 load operation. This operation can be used to
disable temporarily an interrupt source. If StoreEOI is active, a
source could be left enabled if the load and store operations come
out of order.
Add a check in our XIVE emulation model for Load-after-Store when
StoreEOI is active. It should catch unreliable sequences. Other load
operations should be fine without it.
Signed-off-by: Cédric Le Goater
---
include/hw/ppc/spapr_xive.h | 1 +
include/hw/ppc/xive.h | 8
hw/intc/spapr_xive.c | 15 +++
hw/intc/spapr_xive_kvm.c | 15 +++
hw/intc/xive.c | 6 ++
hw/ppc/spapr_hcall.c | 7 +++
6 files changed, 52 insertions(+)
diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h
index b282960ad90d..9c247d8bf57d 100644
--- a/include/hw/ppc/spapr_xive.h
+++ b/include/hw/ppc/spapr_xive.h
@@ -73,6 +73,7 @@ void spapr_xive_map_mmio(SpaprXive *xive);
int spapr_xive_end_to_target(uint8_t end_blk, uint32_t end_idx,
uint32_t *out_server, uint8_t *out_prio);
+void spapr_xive_enable_store_eoi(SpaprXive *xive, bool enable);
/*
* KVM XIVE device helpers
diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
index 126e4e2c3a17..133f308c2792 100644
--- a/include/hw/ppc/xive.h
+++ b/include/hw/ppc/xive.h
@@ -285,6 +285,14 @@ uint8_t xive_esb_set(uint8_t *pq, uint8_t value);
#define XIVE_ESB_SET_PQ_10 0xe00 /* Load */
#define XIVE_ESB_SET_PQ_11 0xf00 /* Load */
+/*
+ * Load-after-store ordering
+ *
+ * Adding this offset to the load address will enforce
+ * load-after-store ordering. This is required to use with StoreEOI.
+ */
+#define XIVE_ESB_LD_ST_MO 0x40 /* Load-after-store ordering */
+
uint8_t xive_source_esb_get(XiveSource *xsrc, uint32_t srcno);
uint8_t xive_source_esb_set(XiveSource *xsrc, uint32_t srcno, uint8_t pq);
diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
index dc641cc604bf..0b8a246ad594 100644
--- a/hw/intc/spapr_xive.c
+++ b/hw/intc/spapr_xive.c
@@ -25,6 +25,7 @@
#include "hw/ppc/xive_regs.h"
#include "hw/qdev-properties.h"
#include "trace.h"
+#include "cpu-models.h"
/*
* XIVE Virtualization Controller BAR and Thread Managment BAR that we
@@ -1854,3 +1855,17 @@ void spapr_xive_hcall_init(SpaprMachineState *spapr)
spapr_register_hypercall(H_INT_SYNC, h_int_sync);
spapr_register_hypercall(H_INT_RESET, h_int_reset);
}
+
+/*
+ * Advertise StoreEOI for a P10 compat guest. OS is required to
+ * enforce load-after-store ordering.
+ */
+void spapr_xive_enable_store_eoi(SpaprXive *xive, bool enable)
+{
+ if (enable) {
+ xive->source.esb_flags |= XIVE_SRC_STORE_EOI;
+ } else {
+ xive->source.esb_flags &= ~XIVE_SRC_STORE_EOI;
+ }
+
+}
diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c
index 61fe7bd2d322..bd023407bd7f 100644
--- a/hw/intc/spapr_xive_kvm.c
+++ b/hw/intc/spapr_xive_kvm.c
@@ -296,6 +296,21 @@ static uint64_t xive_esb_rw(XiveSource *xsrc, int srcno,
uint32_t offset,
static uint8_t xive_esb_read(XiveSource *xsrc, int srcno, uint32_t offset)
{
+ /*
+ * The XIVE_ESB_SET_PQ_10 load operation is used to disable
+ * temporarily an interrupt source. If StoreEOI is active, a
+ * source could be left enabled if the load and store operations
+ * come out of order.
+ *
+ * As we don't know the characteristics of the host source
+ * interrupts (StoreEOI or not), enforce the load-after-store
+ * ordering always. The performance penalty will be very small for
+ * QEMU.
+ */
+ if (offset == XIVE_ESB_SET_PQ_10) {
+ offset |= XIVE_ESB_LD_ST_MO;
+ }
+
return xive_esb_rw(xsrc, srcno, offset, 0, 0) & 0x3;
}
diff --git a/hw/intc/xive.c b/hw/intc/xive.c
index b8e4c7294d59..d62881873b1b 100644
--- a/hw/intc/xive.c
+++ b/hw/intc/xive.c
@@ -1024,6 +1024,12 @@ static uint64_t xive_source_esb_read(void *opaque,
hwaddr addr, unsigned size)
case XIVE_ESB_SET_PQ_01 ... XIVE_ESB_SET_PQ_01 + 0x0FF:
case XIVE_ESB_SET_PQ_10
Re: [PATCH] ppc/spapr: Advertise StoreEOI for POWER10 compat guests
[ adding a few people for the comments ]
On 2/16/22 23:17, Daniel Henrique Barboza wrote:
On 2/14/22 11:11, Cédric Le Goater wrote:
When an interrupt has been handled, the OS notifies the interrupt
controller with a EOI sequence. On a POWER9 and POWER10 systems using
nit: s/a EOI sequence/an EOI sequence
the XIVE interrupt controller, this can be done with a load or a store
operation on the ESB interrupt management page of the interrupt. The
StoreEOI operation has less latency and improves interrupt handling
performance but it was deactivated during the POWER9 DD2.0 timeframe
because of ordering issues. POWER9 systems use the LoadEOI instead.
POWER10 compat guests should have fixed the issue with
Load-after-Store ordering and StoreEOI can be activated for them
again.
To maintain performance, this ordering is only enforced for the
XIVE_ESB_SET_PQ_10 load operation. This operation can be used to
disable temporarily an interrupt source. If StoreEOI is active, a
source could be left enabled if the load and store operations come
out of order.
Add a check in our XIVE emulation model for Load-after-Store when
StoreEOI is active. It should catch unreliable sequences. Other load
operations should be fine without it.
Signed-off-by: Cédric Le Goater
---
include/hw/ppc/spapr_xive.h | 1 +
include/hw/ppc/xive.h | 8
hw/intc/spapr_xive.c | 15 +++
hw/intc/spapr_xive_kvm.c | 15 +++
hw/intc/xive.c | 6 ++
hw/ppc/spapr_hcall.c | 7 +++
6 files changed, 52 insertions(+)
diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h
index b282960ad90d..9c247d8bf57d 100644
--- a/include/hw/ppc/spapr_xive.h
+++ b/include/hw/ppc/spapr_xive.h
@@ -73,6 +73,7 @@ void spapr_xive_map_mmio(SpaprXive *xive);
int spapr_xive_end_to_target(uint8_t end_blk, uint32_t end_idx,
uint32_t *out_server, uint8_t *out_prio);
+void spapr_xive_enable_store_eoi(SpaprXive *xive, bool enable);
/*
* KVM XIVE device helpers
diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
index 126e4e2c3a17..133f308c2792 100644
--- a/include/hw/ppc/xive.h
+++ b/include/hw/ppc/xive.h
@@ -285,6 +285,14 @@ uint8_t xive_esb_set(uint8_t *pq, uint8_t value);
#define XIVE_ESB_SET_PQ_10 0xe00 /* Load */
#define XIVE_ESB_SET_PQ_11 0xf00 /* Load */
+/*
+ * Load-after-store ordering
+ *
+ * Adding this offset to the load address will enforce
+ * load-after-store ordering. This is required to use with StoreEOI.
+ */
+#define XIVE_ESB_LD_ST_MO 0x40 /* Load-after-store ordering */
+
uint8_t xive_source_esb_get(XiveSource *xsrc, uint32_t srcno);
uint8_t xive_source_esb_set(XiveSource *xsrc, uint32_t srcno, uint8_t pq);
diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
index dc641cc604bf..0b8a246ad594 100644
--- a/hw/intc/spapr_xive.c
+++ b/hw/intc/spapr_xive.c
@@ -25,6 +25,7 @@
#include "hw/ppc/xive_regs.h"
#include "hw/qdev-properties.h"
#include "trace.h"
+#include "cpu-models.h"
/*
* XIVE Virtualization Controller BAR and Thread Managment BAR that we
@@ -1854,3 +1855,17 @@ void spapr_xive_hcall_init(SpaprMachineState *spapr)
spapr_register_hypercall(H_INT_SYNC, h_int_sync);
spapr_register_hypercall(H_INT_RESET, h_int_reset);
}
+
+/*
+ * Advertise StoreEOI for a P10 compat guest. OS is required to
+ * enforce load-after-store ordering.
+ */
+void spapr_xive_enable_store_eoi(SpaprXive *xive, bool enable)
+{
+ if (enable) {
+ xive->source.esb_flags |= XIVE_SRC_STORE_EOI;
+ } else {
+ xive->source.esb_flags &= ~XIVE_SRC_STORE_EOI;
+ }
+
+}
diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c
index 61fe7bd2d322..bd023407bd7f 100644
--- a/hw/intc/spapr_xive_kvm.c
+++ b/hw/intc/spapr_xive_kvm.c
@@ -296,6 +296,21 @@ static uint64_t xive_esb_rw(XiveSource *xsrc, int srcno,
uint32_t offset,
static uint8_t xive_esb_read(XiveSource *xsrc, int srcno, uint32_t offset)
{
+ /*
+ * The XIVE_ESB_SET_PQ_10 load operation is used to disable
+ * temporarily an interrupt source. If StoreEOI is active, a
+ * source could be left enabled if the load and store operations
+ * come out of order.
+ *
+ * As we don't know the characteristics of the host source
+ * interrupts (StoreEOI or not), enforce the load-after-store
+ * ordering always. The performance penalty will be very small for
+ * QEMU.
+ */
+ if (offset == XIVE_ESB_SET_PQ_10) {
+ offset |= XIVE_ESB_LD_ST_MO;
+ }
+
return xive_esb_rw(xsrc, srcno, offset, 0, 0) & 0x3;
}
diff --git a/hw/intc/xive.c b/hw/intc/xive.c
index b8e4c7294d59..d62881873b1b 100644
--- a/hw/intc/xive.c
+++ b/hw/intc/xive.c
@@ -1024,6 +1024,12 @@ static uint64_t xive_source_esb_read(void *opaque,
hwaddr addr, unsigned size)
case XIVE_ESB_SET_PQ_01 ... XIVE_ESB_SET_PQ_01 + 0x0FF:
case XIVE_ESB_SET_PQ_10 ... XIVE_ESB_SET_PQ_10 + 0x0FF:
case
Re: [PATCH] ppc/spapr: Advertise StoreEOI for POWER10 compat guests
On 2/14/22 11:11, Cédric Le Goater wrote:
When an interrupt has been handled, the OS notifies the interrupt
controller with a EOI sequence. On a POWER9 and POWER10 systems using
nit: s/a EOI sequence/an EOI sequence
the XIVE interrupt controller, this can be done with a load or a store
operation on the ESB interrupt management page of the interrupt. The
StoreEOI operation has less latency and improves interrupt handling
performance but it was deactivated during the POWER9 DD2.0 timeframe
because of ordering issues. POWER9 systems use the LoadEOI instead.
POWER10 compat guests should have fixed the issue with
Load-after-Store ordering and StoreEOI can be activated for them
again.
To maintain performance, this ordering is only enforced for the
XIVE_ESB_SET_PQ_10 load operation. This operation can be used to
disable temporarily an interrupt source. If StoreEOI is active, a
source could be left enabled if the load and store operations come
out of order.
Add a check in our XIVE emulation model for Load-after-Store when
StoreEOI is active. It should catch unreliable sequences. Other load
operations should be fine without it.
Signed-off-by: Cédric Le Goater
---
include/hw/ppc/spapr_xive.h | 1 +
include/hw/ppc/xive.h | 8
hw/intc/spapr_xive.c| 15 +++
hw/intc/spapr_xive_kvm.c| 15 +++
hw/intc/xive.c | 6 ++
hw/ppc/spapr_hcall.c| 7 +++
6 files changed, 52 insertions(+)
diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h
index b282960ad90d..9c247d8bf57d 100644
--- a/include/hw/ppc/spapr_xive.h
+++ b/include/hw/ppc/spapr_xive.h
@@ -73,6 +73,7 @@ void spapr_xive_map_mmio(SpaprXive *xive);
int spapr_xive_end_to_target(uint8_t end_blk, uint32_t end_idx,
uint32_t *out_server, uint8_t *out_prio);
+void spapr_xive_enable_store_eoi(SpaprXive *xive, bool enable);
/*
* KVM XIVE device helpers
diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
index 126e4e2c3a17..133f308c2792 100644
--- a/include/hw/ppc/xive.h
+++ b/include/hw/ppc/xive.h
@@ -285,6 +285,14 @@ uint8_t xive_esb_set(uint8_t *pq, uint8_t value);
#define XIVE_ESB_SET_PQ_10 0xe00 /* Load */
#define XIVE_ESB_SET_PQ_11 0xf00 /* Load */
+/*
+ * Load-after-store ordering
+ *
+ * Adding this offset to the load address will enforce
+ * load-after-store ordering. This is required to use with StoreEOI.
+ */
+#define XIVE_ESB_LD_ST_MO 0x40 /* Load-after-store ordering */
+
uint8_t xive_source_esb_get(XiveSource *xsrc, uint32_t srcno);
uint8_t xive_source_esb_set(XiveSource *xsrc, uint32_t srcno, uint8_t pq);
diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
index dc641cc604bf..0b8a246ad594 100644
--- a/hw/intc/spapr_xive.c
+++ b/hw/intc/spapr_xive.c
@@ -25,6 +25,7 @@
#include "hw/ppc/xive_regs.h"
#include "hw/qdev-properties.h"
#include "trace.h"
+#include "cpu-models.h"
/*
* XIVE Virtualization Controller BAR and Thread Managment BAR that we
@@ -1854,3 +1855,17 @@ void spapr_xive_hcall_init(SpaprMachineState *spapr)
spapr_register_hypercall(H_INT_SYNC, h_int_sync);
spapr_register_hypercall(H_INT_RESET, h_int_reset);
}
+
+/*
+ * Advertise StoreEOI for a P10 compat guest. OS is required to
+ * enforce load-after-store ordering.
+ */
+void spapr_xive_enable_store_eoi(SpaprXive *xive, bool enable)
+{
+if (enable) {
+xive->source.esb_flags |= XIVE_SRC_STORE_EOI;
+} else {
+xive->source.esb_flags &= ~XIVE_SRC_STORE_EOI;
+}
+
+}
diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c
index 61fe7bd2d322..bd023407bd7f 100644
--- a/hw/intc/spapr_xive_kvm.c
+++ b/hw/intc/spapr_xive_kvm.c
@@ -296,6 +296,21 @@ static uint64_t xive_esb_rw(XiveSource *xsrc, int srcno,
uint32_t offset,
static uint8_t xive_esb_read(XiveSource *xsrc, int srcno, uint32_t offset)
{
+/*
+ * The XIVE_ESB_SET_PQ_10 load operation is used to disable
+ * temporarily an interrupt source. If StoreEOI is active, a
+ * source could be left enabled if the load and store operations
+ * come out of order.
+ *
+ * As we don't know the characteristics of the host source
+ * interrupts (StoreEOI or not), enforce the load-after-store
+ * ordering always. The performance penalty will be very small for
+ * QEMU.
+ */
+if (offset == XIVE_ESB_SET_PQ_10) {
+offset |= XIVE_ESB_LD_ST_MO;
+}
+
return xive_esb_rw(xsrc, srcno, offset, 0, 0) & 0x3;
}
diff --git a/hw/intc/xive.c b/hw/intc/xive.c
index b8e4c7294d59..d62881873b1b 100644
--- a/hw/intc/xive.c
+++ b/hw/intc/xive.c
@@ -1024,6 +1024,12 @@ static uint64_t xive_source_esb_read(void *opaque,
hwaddr addr, unsigned size)
case XIVE_ESB_SET_PQ_01 ... XIVE_ESB_SET_PQ_01 + 0x0FF:
case XIVE_ESB_SET_PQ_10 ... XIVE_ESB_SET_PQ_10 + 0x0FF:
case XIVE_ESB_SET_PQ_11 ... XIVE_ESB_SET_PQ_11 + 0x0FF:
+if
[PATCH] ppc/spapr: Advertise StoreEOI for POWER10 compat guests
When an interrupt has been handled, the OS notifies the interrupt
controller with a EOI sequence. On a POWER9 and POWER10 systems using
the XIVE interrupt controller, this can be done with a load or a store
operation on the ESB interrupt management page of the interrupt. The
StoreEOI operation has less latency and improves interrupt handling
performance but it was deactivated during the POWER9 DD2.0 timeframe
because of ordering issues. POWER9 systems use the LoadEOI instead.
POWER10 compat guests should have fixed the issue with
Load-after-Store ordering and StoreEOI can be activated for them
again.
To maintain performance, this ordering is only enforced for the
XIVE_ESB_SET_PQ_10 load operation. This operation can be used to
disable temporarily an interrupt source. If StoreEOI is active, a
source could be left enabled if the load and store operations come
out of order.
Add a check in our XIVE emulation model for Load-after-Store when
StoreEOI is active. It should catch unreliable sequences. Other load
operations should be fine without it.
Signed-off-by: Cédric Le Goater
---
include/hw/ppc/spapr_xive.h | 1 +
include/hw/ppc/xive.h | 8
hw/intc/spapr_xive.c| 15 +++
hw/intc/spapr_xive_kvm.c| 15 +++
hw/intc/xive.c | 6 ++
hw/ppc/spapr_hcall.c| 7 +++
6 files changed, 52 insertions(+)
diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h
index b282960ad90d..9c247d8bf57d 100644
--- a/include/hw/ppc/spapr_xive.h
+++ b/include/hw/ppc/spapr_xive.h
@@ -73,6 +73,7 @@ void spapr_xive_map_mmio(SpaprXive *xive);
int spapr_xive_end_to_target(uint8_t end_blk, uint32_t end_idx,
uint32_t *out_server, uint8_t *out_prio);
+void spapr_xive_enable_store_eoi(SpaprXive *xive, bool enable);
/*
* KVM XIVE device helpers
diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
index 126e4e2c3a17..133f308c2792 100644
--- a/include/hw/ppc/xive.h
+++ b/include/hw/ppc/xive.h
@@ -285,6 +285,14 @@ uint8_t xive_esb_set(uint8_t *pq, uint8_t value);
#define XIVE_ESB_SET_PQ_10 0xe00 /* Load */
#define XIVE_ESB_SET_PQ_11 0xf00 /* Load */
+/*
+ * Load-after-store ordering
+ *
+ * Adding this offset to the load address will enforce
+ * load-after-store ordering. This is required to use with StoreEOI.
+ */
+#define XIVE_ESB_LD_ST_MO 0x40 /* Load-after-store ordering */
+
uint8_t xive_source_esb_get(XiveSource *xsrc, uint32_t srcno);
uint8_t xive_source_esb_set(XiveSource *xsrc, uint32_t srcno, uint8_t pq);
diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
index dc641cc604bf..0b8a246ad594 100644
--- a/hw/intc/spapr_xive.c
+++ b/hw/intc/spapr_xive.c
@@ -25,6 +25,7 @@
#include "hw/ppc/xive_regs.h"
#include "hw/qdev-properties.h"
#include "trace.h"
+#include "cpu-models.h"
/*
* XIVE Virtualization Controller BAR and Thread Managment BAR that we
@@ -1854,3 +1855,17 @@ void spapr_xive_hcall_init(SpaprMachineState *spapr)
spapr_register_hypercall(H_INT_SYNC, h_int_sync);
spapr_register_hypercall(H_INT_RESET, h_int_reset);
}
+
+/*
+ * Advertise StoreEOI for a P10 compat guest. OS is required to
+ * enforce load-after-store ordering.
+ */
+void spapr_xive_enable_store_eoi(SpaprXive *xive, bool enable)
+{
+if (enable) {
+xive->source.esb_flags |= XIVE_SRC_STORE_EOI;
+} else {
+xive->source.esb_flags &= ~XIVE_SRC_STORE_EOI;
+}
+
+}
diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c
index 61fe7bd2d322..bd023407bd7f 100644
--- a/hw/intc/spapr_xive_kvm.c
+++ b/hw/intc/spapr_xive_kvm.c
@@ -296,6 +296,21 @@ static uint64_t xive_esb_rw(XiveSource *xsrc, int srcno,
uint32_t offset,
static uint8_t xive_esb_read(XiveSource *xsrc, int srcno, uint32_t offset)
{
+/*
+ * The XIVE_ESB_SET_PQ_10 load operation is used to disable
+ * temporarily an interrupt source. If StoreEOI is active, a
+ * source could be left enabled if the load and store operations
+ * come out of order.
+ *
+ * As we don't know the characteristics of the host source
+ * interrupts (StoreEOI or not), enforce the load-after-store
+ * ordering always. The performance penalty will be very small for
+ * QEMU.
+ */
+if (offset == XIVE_ESB_SET_PQ_10) {
+offset |= XIVE_ESB_LD_ST_MO;
+}
+
return xive_esb_rw(xsrc, srcno, offset, 0, 0) & 0x3;
}
diff --git a/hw/intc/xive.c b/hw/intc/xive.c
index b8e4c7294d59..d62881873b1b 100644
--- a/hw/intc/xive.c
+++ b/hw/intc/xive.c
@@ -1024,6 +1024,12 @@ static uint64_t xive_source_esb_read(void *opaque,
hwaddr addr, unsigned size)
case XIVE_ESB_SET_PQ_01 ... XIVE_ESB_SET_PQ_01 + 0x0FF:
case XIVE_ESB_SET_PQ_10 ... XIVE_ESB_SET_PQ_10 + 0x0FF:
case XIVE_ESB_SET_PQ_11 ... XIVE_ESB_SET_PQ_11 + 0x0FF:
+if (offset == XIVE_ESB_SET_PQ_10 &&
+xsrc->esb_flags & XIVE_SRC_STORE_EOI) {
+qemu_log_mask(LOG_GUEST_ERROR, "XIVE:
