| Message ID | 1497399565.2897.23.camel@kernel.crashing.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 25642705b2359a705784bbbf1655c25a8f8efde2 |
| Headers | show |
Benjamin Herrenschmidt <benh@kernel.crashing.org> writes: > Architecturally we should apply a 0x400 offset for these. Not doing > it will break future HW implementations. Can you elaborate a bit? You're changing a write to 0x0 to be a write to 0x400, which at face value appears like it breaks something, or is already broken? cheers > diff --git a/arch/powerpc/include/asm/xive.h b/arch/powerpc/include/asm/xive.h > index c8a822a..c23ff43 100644 > --- a/arch/powerpc/include/asm/xive.h > +++ b/arch/powerpc/include/asm/xive.h > @@ -94,11 +94,13 @@ struct xive_q { > * store at 0 and some ESBs support doing a trigger via a > * separate trigger page. > */ > -#define XIVE_ESB_GET 0x800 > -#define XIVE_ESB_SET_PQ_00 0xc00 > -#define XIVE_ESB_SET_PQ_01 0xd00 > -#define XIVE_ESB_SET_PQ_10 0xe00 > -#define XIVE_ESB_SET_PQ_11 0xf00 > +#define XIVE_ESB_STORE_EOI 0x400 /* Store */ > +#define XIVE_ESB_LOAD_EOI 0x000 /* Load */ > +#define XIVE_ESB_GET 0x800 /* Load */ > +#define XIVE_ESB_SET_PQ_00 0xc00 /* Load */ > +#define XIVE_ESB_SET_PQ_01 0xd00 /* Load */ > +#define XIVE_ESB_SET_PQ_10 0xe00 /* Load */ > +#define XIVE_ESB_SET_PQ_11 0xf00 /* Load */ > > #define XIVE_ESB_VAL_P 0x2 > #define XIVE_ESB_VAL_Q 0x1 > diff --git a/arch/powerpc/kvm/book3s_xive_template.c b/arch/powerpc/kvm/book3s_xive_template.c > index 023a311..4636ca6 100644 > --- a/arch/powerpc/kvm/book3s_xive_template.c > +++ b/arch/powerpc/kvm/book3s_xive_template.c > @@ -69,7 +69,7 @@ static void GLUE(X_PFX,source_eoi)(u32 hw_irq, struct xive_irq_data *xd) > { > /* If the XIVE supports the new "store EOI facility, use it */ > if (xd->flags & XIVE_IRQ_FLAG_STORE_EOI) > - __x_writeq(0, __x_eoi_page(xd)); > + __x_writeq(0, __x_eoi_page(xd) + XIVE_ESB_STORE_EOI); > else if (hw_irq && xd->flags & XIVE_IRQ_FLAG_EOI_FW) { > opal_int_eoi(hw_irq); > } else { > @@ -89,7 +89,7 @@ static void GLUE(X_PFX,source_eoi)(u32 hw_irq, struct xive_irq_data *xd) > * properly. > */ > if (xd->flags & XIVE_IRQ_FLAG_LSI) > - __x_readq(__x_eoi_page(xd)); > + __x_readq(__x_eoi_page(xd) + XIVE_ESB_LOAD_EOI); > else { > eoi_val = GLUE(X_PFX,esb_load)(xd, XIVE_ESB_SET_PQ_00); > > diff --git a/arch/powerpc/sysdev/xive/common.c b/arch/powerpc/sysdev/xive/common.c > index 9138250..8f5e303 100644 > --- a/arch/powerpc/sysdev/xive/common.c > +++ b/arch/powerpc/sysdev/xive/common.c > @@ -297,7 +297,7 @@ void xive_do_source_eoi(u32 hw_irq, struct xive_irq_data *xd) > { > /* If the XIVE supports the new "store EOI facility, use it */ > if (xd->flags & XIVE_IRQ_FLAG_STORE_EOI) > - out_be64(xd->eoi_mmio, 0); > + out_be64(xd->eoi_mmio + XIVE_ESB_STORE_EOI, 0); > else if (hw_irq && xd->flags & XIVE_IRQ_FLAG_EOI_FW) { > /* > * The FW told us to call it. This happens for some
On Wed, 2017-06-14 at 14:44 +1000, Michael Ellerman wrote: > Benjamin Herrenschmidt <benh@kernel.crashing.org> writes: > > > Architecturally we should apply a 0x400 offset for these. Not doing > > it will break future HW implementations. > > Can you elaborate a bit? > > You're changing a write to 0x0 to be a write to 0x400, which at face > value appears like it breaks something, or is already broken? The code is broken today, I didn't read the spec properly. We never had a proper DD2 model to test until recently. The offset of 0 is supposed to remain for "triggers" though not all sources support both trigger and store EOI, and in P9 specifically, some sources will treat 0 as a store EOI :-) But future chips will not. So this makes us use the properly architected offset which should work always. Cheers, Ben. > > cheers > > > diff --git a/arch/powerpc/include/asm/xive.h b/arch/powerpc/include/asm/xive.h > > index c8a822a..c23ff43 100644 > > --- a/arch/powerpc/include/asm/xive.h > > +++ b/arch/powerpc/include/asm/xive.h > > @@ -94,11 +94,13 @@ struct xive_q { > > * store at 0 and some ESBs support doing a trigger via a > > * separate trigger page. > > */ > > -#define XIVE_ESB_GET 0x800 > > -#define XIVE_ESB_SET_PQ_00 0xc00 > > -#define XIVE_ESB_SET_PQ_01 0xd00 > > -#define XIVE_ESB_SET_PQ_10 0xe00 > > -#define XIVE_ESB_SET_PQ_11 0xf00 > > +#define XIVE_ESB_STORE_EOI 0x400 /* Store */ > > +#define XIVE_ESB_LOAD_EOI 0x000 /* Load */ > > +#define XIVE_ESB_GET 0x800 /* Load */ > > +#define XIVE_ESB_SET_PQ_00 0xc00 /* Load */ > > +#define XIVE_ESB_SET_PQ_01 0xd00 /* Load */ > > +#define XIVE_ESB_SET_PQ_10 0xe00 /* Load */ > > +#define XIVE_ESB_SET_PQ_11 0xf00 /* Load */ > > > > #define XIVE_ESB_VAL_P 0x2 > > #define XIVE_ESB_VAL_Q 0x1 > > diff --git a/arch/powerpc/kvm/book3s_xive_template.c b/arch/powerpc/kvm/book3s_xive_template.c > > index 023a311..4636ca6 100644 > > --- a/arch/powerpc/kvm/book3s_xive_template.c > > +++ b/arch/powerpc/kvm/book3s_xive_template.c > > @@ -69,7 +69,7 @@ static void GLUE(X_PFX,source_eoi)(u32 hw_irq, struct xive_irq_data *xd) > > { > > /* If the XIVE supports the new "store EOI facility, use it */ > > if (xd->flags & XIVE_IRQ_FLAG_STORE_EOI) > > - __x_writeq(0, __x_eoi_page(xd)); > > + __x_writeq(0, __x_eoi_page(xd) + XIVE_ESB_STORE_EOI); > > else if (hw_irq && xd->flags & XIVE_IRQ_FLAG_EOI_FW) { > > opal_int_eoi(hw_irq); > > } else { > > @@ -89,7 +89,7 @@ static void GLUE(X_PFX,source_eoi)(u32 hw_irq, struct xive_irq_data *xd) > > * properly. > > */ > > if (xd->flags & XIVE_IRQ_FLAG_LSI) > > - __x_readq(__x_eoi_page(xd)); > > + __x_readq(__x_eoi_page(xd) + XIVE_ESB_LOAD_EOI); > > else { > > eoi_val = GLUE(X_PFX,esb_load)(xd, XIVE_ESB_SET_PQ_00); > > > > diff --git a/arch/powerpc/sysdev/xive/common.c b/arch/powerpc/sysdev/xive/common.c > > index 9138250..8f5e303 100644 > > --- a/arch/powerpc/sysdev/xive/common.c > > +++ b/arch/powerpc/sysdev/xive/common.c > > @@ -297,7 +297,7 @@ void xive_do_source_eoi(u32 hw_irq, struct xive_irq_data *xd) > > { > > /* If the XIVE supports the new "store EOI facility, use it */ > > if (xd->flags & XIVE_IRQ_FLAG_STORE_EOI) > > - out_be64(xd->eoi_mmio, 0); > > + out_be64(xd->eoi_mmio + XIVE_ESB_STORE_EOI, 0); > > else if (hw_irq && xd->flags & XIVE_IRQ_FLAG_EOI_FW) { > > /* > > * The FW told us to call it. This happens for some
On Wed, 2017-06-14 at 00:19:25 UTC, Benjamin Herrenschmidt wrote: > Architecturally we should apply a 0x400 offset for these. Not doing > it will break future HW implementations. > > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> Applied to powerpc fixes, thanks. https://git.kernel.org/powerpc/c/25642705b2359a705784bbbf1655c2 cheers
diff --git a/arch/powerpc/include/asm/xive.h b/arch/powerpc/include/asm/xive.h index c8a822a..c23ff43 100644 --- a/arch/powerpc/include/asm/xive.h +++ b/arch/powerpc/include/asm/xive.h @@ -94,11 +94,13 @@ struct xive_q { * store at 0 and some ESBs support doing a trigger via a * separate trigger page. */ -#define XIVE_ESB_GET 0x800 -#define XIVE_ESB_SET_PQ_00 0xc00 -#define XIVE_ESB_SET_PQ_01 0xd00 -#define XIVE_ESB_SET_PQ_10 0xe00 -#define XIVE_ESB_SET_PQ_11 0xf00 +#define XIVE_ESB_STORE_EOI 0x400 /* Store */ +#define XIVE_ESB_LOAD_EOI 0x000 /* Load */ +#define XIVE_ESB_GET 0x800 /* Load */ +#define XIVE_ESB_SET_PQ_00 0xc00 /* Load */ +#define XIVE_ESB_SET_PQ_01 0xd00 /* Load */ +#define XIVE_ESB_SET_PQ_10 0xe00 /* Load */ +#define XIVE_ESB_SET_PQ_11 0xf00 /* Load */ #define XIVE_ESB_VAL_P 0x2 #define XIVE_ESB_VAL_Q 0x1 diff --git a/arch/powerpc/kvm/book3s_xive_template.c b/arch/powerpc/kvm/book3s_xive_template.c index 023a311..4636ca6 100644 --- a/arch/powerpc/kvm/book3s_xive_template.c +++ b/arch/powerpc/kvm/book3s_xive_template.c @@ -69,7 +69,7 @@ static void GLUE(X_PFX,source_eoi)(u32 hw_irq, struct xive_irq_data *xd) { /* If the XIVE supports the new "store EOI facility, use it */ if (xd->flags & XIVE_IRQ_FLAG_STORE_EOI) - __x_writeq(0, __x_eoi_page(xd)); + __x_writeq(0, __x_eoi_page(xd) + XIVE_ESB_STORE_EOI); else if (hw_irq && xd->flags & XIVE_IRQ_FLAG_EOI_FW) { opal_int_eoi(hw_irq); } else { @@ -89,7 +89,7 @@ static void GLUE(X_PFX,source_eoi)(u32 hw_irq, struct xive_irq_data *xd) * properly. */ if (xd->flags & XIVE_IRQ_FLAG_LSI) - __x_readq(__x_eoi_page(xd)); + __x_readq(__x_eoi_page(xd) + XIVE_ESB_LOAD_EOI); else { eoi_val = GLUE(X_PFX,esb_load)(xd, XIVE_ESB_SET_PQ_00); diff --git a/arch/powerpc/sysdev/xive/common.c b/arch/powerpc/sysdev/xive/common.c index 9138250..8f5e303 100644 --- a/arch/powerpc/sysdev/xive/common.c +++ b/arch/powerpc/sysdev/xive/common.c @@ -297,7 +297,7 @@ void xive_do_source_eoi(u32 hw_irq, struct xive_irq_data *xd) { /* If the XIVE supports the new "store EOI facility, use it */ if (xd->flags & XIVE_IRQ_FLAG_STORE_EOI) - out_be64(xd->eoi_mmio, 0); + out_be64(xd->eoi_mmio + XIVE_ESB_STORE_EOI, 0); else if (hw_irq && xd->flags & XIVE_IRQ_FLAG_EOI_FW) { /* * The FW told us to call it. This happens for some
Architecturally we should apply a 0x400 offset for these. Not doing it will break future HW implementations. Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> --- arch/powerpc/include/asm/xive.h | 12 +++++++----- arch/powerpc/kvm/book3s_xive_template.c | 4 ++-- arch/powerpc/sysdev/xive/common.c | 2 +- 3 files changed, 10 insertions(+), 8 deletions(-)