Message ID | 20200220081506.31209-1-clg@kaod.org (mailing list archive) |
---|---|
State | Accepted |
Commit | b1f9be9392f090f08e4ad9e2c68963aeff03bd67 |
Headers | show |
Series | powerpc/xive: Enforce load-after-store ordering when StoreEOI is active | expand |
Context | Check | Description |
---|---|---|
snowpatch_ozlabs/apply_patch | warning | Failed to apply on branch powerpc/merge (65b2623f395a4e25ab3ff4cff1c9c7623619a22d) |
snowpatch_ozlabs/apply_patch | warning | Failed to apply on branch powerpc/next (71c3a888cbcaf453aecf8d2f8fb003271d28073f) |
snowpatch_ozlabs/apply_patch | warning | Failed to apply on branch linus/master (ca7e1fd1026c5af6a533b4b5447e1d2f153e28f2) |
snowpatch_ozlabs/apply_patch | warning | Failed to apply on branch powerpc/fixes (9eb425b2e04e0e3006adffea5bf5f227a896f128) |
snowpatch_ozlabs/apply_patch | warning | Failed to apply on branch linux-next (f4aba10148cd290bbbf4d0efae0e9789a13c2778) |
snowpatch_ozlabs/apply_patch | fail | Failed to apply to any branch |
I am still slowly wrapping my head around XIVE and it's interaction with KVM but from what I can see this looks good and is needed so we can enable StoreEOI support in future so: Reviewed-by: Alistair Popple <alistair@popple.id.au> On Thursday, 20 February 2020 7:15:06 PM AEST Cédric Le Goater wrote: > When an interrupt has been handled, the OS notifies the interrupt > controller with a EOI sequence. On a POWER9 system 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. We use the LoadEOI today but we plan to > reactivate StoreEOI in future architectures. > > There is usually no need to enforce ordering between ESB load and > store operations as they should lead to the same result. E.g. a store > trigger and a load EOI can be executed in any order. Assuming the > interrupt state is PQ=10, a store trigger followed by a load EOI will > return a Q bit. In the reverse order, it will create a new interrupt > trigger from HW. In both cases, the handler processing interrupts is > notified. > > In some cases, the XIVE_ESB_SET_PQ_10 load operation is used to > disable temporarily the interrupt source (mask/unmask). When the > source is reenabled, the OS can detect if interrupts were received > while the source was disabled and reinject them. This process needs > special care when StoreEOI is activated. The ESB load and store > operations should be correctly ordered because a XIVE_ESB_STORE_EOI > operation could leave the source enabled if it has not completed > before the loads. > > For those cases, we enforce Load-after-Store ordering with a special > load operation offset. To avoid performance impact, this ordering is > only enforced when really needed, that is when interrupt sources are > temporarily disabled with the XIVE_ESB_SET_PQ_10 load. It should not > be needed for other loads. > > Signed-off-by: Cédric Le Goater <clg@kaod.org> > --- > arch/powerpc/include/asm/xive-regs.h | 8 ++++++++ > arch/powerpc/kvm/book3s_xive_native.c | 6 ++++++ > arch/powerpc/kvm/book3s_xive_template.c | 3 +++ > arch/powerpc/sysdev/xive/common.c | 3 +++ > arch/powerpc/kvm/book3s_hv_rmhandlers.S | 5 +++++ > 5 files changed, 25 insertions(+) > > diff --git a/arch/powerpc/include/asm/xive-regs.h > b/arch/powerpc/include/asm/xive-regs.h index f2dfcd50a2d3..b1996fbae59a > 100644 > --- a/arch/powerpc/include/asm/xive-regs.h > +++ b/arch/powerpc/include/asm/xive-regs.h > @@ -37,6 +37,14 @@ > #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 StoreEOI. > + */ > +#define XIVE_ESB_LD_ST_MO 0x40 /* Load-after-store ordering */ > + > #define XIVE_ESB_VAL_P 0x2 > #define XIVE_ESB_VAL_Q 0x1 > > diff --git a/arch/powerpc/kvm/book3s_xive_native.c > b/arch/powerpc/kvm/book3s_xive_native.c index d83adb1e1490..c80b6a447efd > 100644 > --- a/arch/powerpc/kvm/book3s_xive_native.c > +++ b/arch/powerpc/kvm/book3s_xive_native.c > @@ -31,6 +31,12 @@ static u8 xive_vm_esb_load(struct xive_irq_data *xd, u32 > offset) { > u64 val; > > + /* > + * The KVM XIVE native device does not use the XIVE_ESB_SET_PQ_10 > + * load operation, so there is no need to enforce load-after-store > + * ordering. > + */ > + > if (xd->flags & XIVE_IRQ_FLAG_SHIFT_BUG) > offset |= offset << 4; > > diff --git a/arch/powerpc/kvm/book3s_xive_template.c > b/arch/powerpc/kvm/book3s_xive_template.c index a8a900ace1e6..4ad3c0279458 > 100644 > --- a/arch/powerpc/kvm/book3s_xive_template.c > +++ b/arch/powerpc/kvm/book3s_xive_template.c > @@ -58,6 +58,9 @@ static u8 GLUE(X_PFX,esb_load)(struct xive_irq_data *xd, > u32 offset) { > u64 val; > > + if (offset == XIVE_ESB_SET_PQ_10 && xd->flags & XIVE_IRQ_FLAG_STORE_EOI) > + offset |= XIVE_ESB_LD_ST_MO; > + > if (xd->flags & XIVE_IRQ_FLAG_SHIFT_BUG) > offset |= offset << 4; > > diff --git a/arch/powerpc/sysdev/xive/common.c > b/arch/powerpc/sysdev/xive/common.c index f5fadbd2533a..0dc421bb494f 100644 > --- a/arch/powerpc/sysdev/xive/common.c > +++ b/arch/powerpc/sysdev/xive/common.c > @@ -202,6 +202,9 @@ static notrace u8 xive_esb_read(struct xive_irq_data > *xd, u32 offset) { > u64 val; > > + if (offset == XIVE_ESB_SET_PQ_10 && xd->flags & XIVE_IRQ_FLAG_STORE_EOI) > + offset |= XIVE_ESB_LD_ST_MO; > + > /* Handle HW errata */ > if (xd->flags & XIVE_IRQ_FLAG_SHIFT_BUG) > offset |= offset << 4; > diff --git a/arch/powerpc/kvm/book3s_hv_rmhandlers.S > b/arch/powerpc/kvm/book3s_hv_rmhandlers.S index e11017897eb0..abe132ff2346 > 100644 > --- a/arch/powerpc/kvm/book3s_hv_rmhandlers.S > +++ b/arch/powerpc/kvm/book3s_hv_rmhandlers.S > @@ -2911,6 +2911,11 @@ kvm_cede_exit: > beq 4f > li r0, 0 > stb r0, VCPU_CEDED(r9) > + /* > + * The escalation interrupts are special as we don't EOI them. > + * There is no need to use the load-after-store ordering offset > + * to set PQ to 10 as we won't use StoreEOI. > + */ > li r6, XIVE_ESB_SET_PQ_10 > b 5f > 4: li r0, 1
On 5/6/20 1:34 AM, Alistair Popple wrote: > I am still slowly wrapping my head around XIVE and it's interaction with KVM > but from what I can see this looks good and is needed so we can enable > StoreEOI support in future so: > > Reviewed-by: Alistair Popple <alistair@popple.id.au> > > On Thursday, 20 February 2020 7:15:06 PM AEST Cédric Le Goater wrote: >> When an interrupt has been handled, the OS notifies the interrupt >> controller with a EOI sequence. On a POWER9 system 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. We use the LoadEOI today but we plan to >> reactivate StoreEOI in future architectures. StoreEOI was considered broken on P9, and never used, but we have to prepare ground for the IBM hypervisor which will activate StoreEOI without negotiation on the new architecture. Thanks, C. >> >> There is usually no need to enforce ordering between ESB load and >> store operations as they should lead to the same result. E.g. a store >> trigger and a load EOI can be executed in any order. Assuming the >> interrupt state is PQ=10, a store trigger followed by a load EOI will >> return a Q bit. In the reverse order, it will create a new interrupt >> trigger from HW. In both cases, the handler processing interrupts is >> notified. >> >> In some cases, the XIVE_ESB_SET_PQ_10 load operation is used to >> disable temporarily the interrupt source (mask/unmask). When the >> source is reenabled, the OS can detect if interrupts were received >> while the source was disabled and reinject them. This process needs >> special care when StoreEOI is activated. The ESB load and store >> operations should be correctly ordered because a XIVE_ESB_STORE_EOI >> operation could leave the source enabled if it has not completed >> before the loads. >> >> For those cases, we enforce Load-after-Store ordering with a special >> load operation offset. To avoid performance impact, this ordering is >> only enforced when really needed, that is when interrupt sources are >> temporarily disabled with the XIVE_ESB_SET_PQ_10 load. It should not >> be needed for other loads. >> >> Signed-off-by: Cédric Le Goater <clg@kaod.org> >> --- >> arch/powerpc/include/asm/xive-regs.h | 8 ++++++++ >> arch/powerpc/kvm/book3s_xive_native.c | 6 ++++++ >> arch/powerpc/kvm/book3s_xive_template.c | 3 +++ >> arch/powerpc/sysdev/xive/common.c | 3 +++ >> arch/powerpc/kvm/book3s_hv_rmhandlers.S | 5 +++++ >> 5 files changed, 25 insertions(+) >> >> diff --git a/arch/powerpc/include/asm/xive-regs.h >> b/arch/powerpc/include/asm/xive-regs.h index f2dfcd50a2d3..b1996fbae59a >> 100644 >> --- a/arch/powerpc/include/asm/xive-regs.h >> +++ b/arch/powerpc/include/asm/xive-regs.h >> @@ -37,6 +37,14 @@ >> #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 StoreEOI. >> + */ >> +#define XIVE_ESB_LD_ST_MO 0x40 /* Load-after-store ordering */ >> + >> #define XIVE_ESB_VAL_P 0x2 >> #define XIVE_ESB_VAL_Q 0x1 >> >> diff --git a/arch/powerpc/kvm/book3s_xive_native.c >> b/arch/powerpc/kvm/book3s_xive_native.c index d83adb1e1490..c80b6a447efd >> 100644 >> --- a/arch/powerpc/kvm/book3s_xive_native.c >> +++ b/arch/powerpc/kvm/book3s_xive_native.c >> @@ -31,6 +31,12 @@ static u8 xive_vm_esb_load(struct xive_irq_data *xd, u32 >> offset) { >> u64 val; >> >> + /* >> + * The KVM XIVE native device does not use the XIVE_ESB_SET_PQ_10 >> + * load operation, so there is no need to enforce load-after-store >> + * ordering. >> + */ >> + >> if (xd->flags & XIVE_IRQ_FLAG_SHIFT_BUG) >> offset |= offset << 4; >> >> diff --git a/arch/powerpc/kvm/book3s_xive_template.c >> b/arch/powerpc/kvm/book3s_xive_template.c index a8a900ace1e6..4ad3c0279458 >> 100644 >> --- a/arch/powerpc/kvm/book3s_xive_template.c >> +++ b/arch/powerpc/kvm/book3s_xive_template.c >> @@ -58,6 +58,9 @@ static u8 GLUE(X_PFX,esb_load)(struct xive_irq_data *xd, >> u32 offset) { >> u64 val; >> >> + if (offset == XIVE_ESB_SET_PQ_10 && xd->flags & XIVE_IRQ_FLAG_STORE_EOI) >> + offset |= XIVE_ESB_LD_ST_MO; >> + >> if (xd->flags & XIVE_IRQ_FLAG_SHIFT_BUG) >> offset |= offset << 4; >> >> diff --git a/arch/powerpc/sysdev/xive/common.c >> b/arch/powerpc/sysdev/xive/common.c index f5fadbd2533a..0dc421bb494f 100644 >> --- a/arch/powerpc/sysdev/xive/common.c >> +++ b/arch/powerpc/sysdev/xive/common.c >> @@ -202,6 +202,9 @@ static notrace u8 xive_esb_read(struct xive_irq_data >> *xd, u32 offset) { >> u64 val; >> >> + if (offset == XIVE_ESB_SET_PQ_10 && xd->flags & XIVE_IRQ_FLAG_STORE_EOI) >> + offset |= XIVE_ESB_LD_ST_MO; >> + >> /* Handle HW errata */ >> if (xd->flags & XIVE_IRQ_FLAG_SHIFT_BUG) >> offset |= offset << 4; >> diff --git a/arch/powerpc/kvm/book3s_hv_rmhandlers.S >> b/arch/powerpc/kvm/book3s_hv_rmhandlers.S index e11017897eb0..abe132ff2346 >> 100644 >> --- a/arch/powerpc/kvm/book3s_hv_rmhandlers.S >> +++ b/arch/powerpc/kvm/book3s_hv_rmhandlers.S >> @@ -2911,6 +2911,11 @@ kvm_cede_exit: >> beq 4f >> li r0, 0 >> stb r0, VCPU_CEDED(r9) >> + /* >> + * The escalation interrupts are special as we don't EOI them. >> + * There is no need to use the load-after-store ordering offset >> + * to set PQ to 10 as we won't use StoreEOI. >> + */ >> li r6, XIVE_ESB_SET_PQ_10 >> b 5f >> 4: li r0, 1 > > > >
On Thu, 2020-02-20 at 08:15:06 UTC, =?UTF-8?q?C=C3=A9dric=20Le=20Goater?= wrote: > When an interrupt has been handled, the OS notifies the interrupt > controller with a EOI sequence. On a POWER9 system 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. We use the LoadEOI today but we plan to > reactivate StoreEOI in future architectures. > > There is usually no need to enforce ordering between ESB load and > store operations as they should lead to the same result. E.g. a store > trigger and a load EOI can be executed in any order. Assuming the > interrupt state is PQ=10, a store trigger followed by a load EOI will > return a Q bit. In the reverse order, it will create a new interrupt > trigger from HW. In both cases, the handler processing interrupts is > notified. > > In some cases, the XIVE_ESB_SET_PQ_10 load operation is used to > disable temporarily the interrupt source (mask/unmask). When the > source is reenabled, the OS can detect if interrupts were received > while the source was disabled and reinject them. This process needs > special care when StoreEOI is activated. The ESB load and store > operations should be correctly ordered because a XIVE_ESB_STORE_EOI > operation could leave the source enabled if it has not completed > before the loads. > > For those cases, we enforce Load-after-Store ordering with a special > load operation offset. To avoid performance impact, this ordering is > only enforced when really needed, that is when interrupt sources are > temporarily disabled with the XIVE_ESB_SET_PQ_10 load. It should not > be needed for other loads. > > Signed-off-by: Cédric Le Goater <clg@kaod.org> Applied to powerpc topic/ppc-kvm, thanks. https://git.kernel.org/powerpc/c/b1f9be9392f090f08e4ad9e2c68963aeff03bd67 cheers
diff --git a/arch/powerpc/include/asm/xive-regs.h b/arch/powerpc/include/asm/xive-regs.h index f2dfcd50a2d3..b1996fbae59a 100644 --- a/arch/powerpc/include/asm/xive-regs.h +++ b/arch/powerpc/include/asm/xive-regs.h @@ -37,6 +37,14 @@ #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 StoreEOI. + */ +#define XIVE_ESB_LD_ST_MO 0x40 /* Load-after-store ordering */ + #define XIVE_ESB_VAL_P 0x2 #define XIVE_ESB_VAL_Q 0x1 diff --git a/arch/powerpc/kvm/book3s_xive_native.c b/arch/powerpc/kvm/book3s_xive_native.c index d83adb1e1490..c80b6a447efd 100644 --- a/arch/powerpc/kvm/book3s_xive_native.c +++ b/arch/powerpc/kvm/book3s_xive_native.c @@ -31,6 +31,12 @@ static u8 xive_vm_esb_load(struct xive_irq_data *xd, u32 offset) { u64 val; + /* + * The KVM XIVE native device does not use the XIVE_ESB_SET_PQ_10 + * load operation, so there is no need to enforce load-after-store + * ordering. + */ + if (xd->flags & XIVE_IRQ_FLAG_SHIFT_BUG) offset |= offset << 4; diff --git a/arch/powerpc/kvm/book3s_xive_template.c b/arch/powerpc/kvm/book3s_xive_template.c index a8a900ace1e6..4ad3c0279458 100644 --- a/arch/powerpc/kvm/book3s_xive_template.c +++ b/arch/powerpc/kvm/book3s_xive_template.c @@ -58,6 +58,9 @@ static u8 GLUE(X_PFX,esb_load)(struct xive_irq_data *xd, u32 offset) { u64 val; + if (offset == XIVE_ESB_SET_PQ_10 && xd->flags & XIVE_IRQ_FLAG_STORE_EOI) + offset |= XIVE_ESB_LD_ST_MO; + if (xd->flags & XIVE_IRQ_FLAG_SHIFT_BUG) offset |= offset << 4; diff --git a/arch/powerpc/sysdev/xive/common.c b/arch/powerpc/sysdev/xive/common.c index f5fadbd2533a..0dc421bb494f 100644 --- a/arch/powerpc/sysdev/xive/common.c +++ b/arch/powerpc/sysdev/xive/common.c @@ -202,6 +202,9 @@ static notrace u8 xive_esb_read(struct xive_irq_data *xd, u32 offset) { u64 val; + if (offset == XIVE_ESB_SET_PQ_10 && xd->flags & XIVE_IRQ_FLAG_STORE_EOI) + offset |= XIVE_ESB_LD_ST_MO; + /* Handle HW errata */ if (xd->flags & XIVE_IRQ_FLAG_SHIFT_BUG) offset |= offset << 4; diff --git a/arch/powerpc/kvm/book3s_hv_rmhandlers.S b/arch/powerpc/kvm/book3s_hv_rmhandlers.S index e11017897eb0..abe132ff2346 100644 --- a/arch/powerpc/kvm/book3s_hv_rmhandlers.S +++ b/arch/powerpc/kvm/book3s_hv_rmhandlers.S @@ -2911,6 +2911,11 @@ kvm_cede_exit: beq 4f li r0, 0 stb r0, VCPU_CEDED(r9) + /* + * The escalation interrupts are special as we don't EOI them. + * There is no need to use the load-after-store ordering offset + * to set PQ to 10 as we won't use StoreEOI. + */ li r6, XIVE_ESB_SET_PQ_10 b 5f 4: li r0, 1
When an interrupt has been handled, the OS notifies the interrupt controller with a EOI sequence. On a POWER9 system 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. We use the LoadEOI today but we plan to reactivate StoreEOI in future architectures. There is usually no need to enforce ordering between ESB load and store operations as they should lead to the same result. E.g. a store trigger and a load EOI can be executed in any order. Assuming the interrupt state is PQ=10, a store trigger followed by a load EOI will return a Q bit. In the reverse order, it will create a new interrupt trigger from HW. In both cases, the handler processing interrupts is notified. In some cases, the XIVE_ESB_SET_PQ_10 load operation is used to disable temporarily the interrupt source (mask/unmask). When the source is reenabled, the OS can detect if interrupts were received while the source was disabled and reinject them. This process needs special care when StoreEOI is activated. The ESB load and store operations should be correctly ordered because a XIVE_ESB_STORE_EOI operation could leave the source enabled if it has not completed before the loads. For those cases, we enforce Load-after-Store ordering with a special load operation offset. To avoid performance impact, this ordering is only enforced when really needed, that is when interrupt sources are temporarily disabled with the XIVE_ESB_SET_PQ_10 load. It should not be needed for other loads. Signed-off-by: Cédric Le Goater <clg@kaod.org> --- arch/powerpc/include/asm/xive-regs.h | 8 ++++++++ arch/powerpc/kvm/book3s_xive_native.c | 6 ++++++ arch/powerpc/kvm/book3s_xive_template.c | 3 +++ arch/powerpc/sysdev/xive/common.c | 3 +++ arch/powerpc/kvm/book3s_hv_rmhandlers.S | 5 +++++ 5 files changed, 25 insertions(+)