| Message ID | 20190918160645.25126-25-clg@kaod.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ppc/pnv: add XIVE support for KVM guests | expand |
On Wed, Sep 18, 2019 at 06:06:44PM +0200, Cédric Le Goater wrote: > The trigger definition is used for triggers both for HW source > interrupts, PHB, PSI, as well as for rerouting interrupts between > Interrupt Controller. > > HW source controllers set bit0 of word0 to ‘0’ as they provide EAS > information (EAS block + EAS index) in the 8 byte data and not END > information, and bit1 of word0 to ‘1’ to signal that the state bit > check has been performed. > > Introduce these new trigger bits and rename the XIVE_SRCNO macros in > XIVE_EAS to reflect better the nature of the data. This is breaking > the notification for the PSI model which will be fixed in the next > patch. > > Signed-off-by: Cédric Le Goater <clg@kaod.org> > --- > include/hw/ppc/xive_regs.h | 24 +++++++++++++++++++++--- > hw/intc/pnv_xive.c | 16 ++++++++++++---- > hw/intc/xive.c | 4 ++-- > 3 files changed, 35 insertions(+), 9 deletions(-) > > diff --git a/include/hw/ppc/xive_regs.h b/include/hw/ppc/xive_regs.h > index dd42c33cef35..83a2f2cc1318 100644 > --- a/include/hw/ppc/xive_regs.h > +++ b/include/hw/ppc/xive_regs.h > @@ -22,9 +22,27 @@ > /* > * Interrupt source number encoding on PowerBUS > */ > -#define XIVE_SRCNO_BLOCK(srcno) (((srcno) >> 28) & 0xf) > -#define XIVE_SRCNO_INDEX(srcno) ((srcno) & 0x0fffffff) > -#define XIVE_SRCNO(blk, idx) ((uint32_t)(blk) << 28 | (idx)) > +/* > + * Trigger data definition > + * > + * The trigger definition is used for triggers both for HW source > + * interrupts (PHB, PSI), as well as for rerouting interrupts between > + * Interrupt Controller. > + * > + * HW source controllers set bit0 of word0 to ‘0’ as they provide EAS > + * information (EAS block + EAS index) in the 8 byte data and not END > + * information, and bit1 of word0 to ‘1’ to signal that the state bit > + * check has been performed. > + */ > +#define XIVE_TRIGGER_END PPC_BIT(0) > +#define XIVE_TRIGGER_EAS PPC_BIT(1) These names seem really confusing in light of the comment above. HW source controllers set the XIVE_TRIGGER_END bit in order to indicate that they're providing EAS info, not END info?? And also set the XIVE_TRIGGER_EAS bit to indicate something different? Also, the comment implies that HW source controllers set both bits, but I'm not really sure if that's what you meant. Expanding on what non-HW source controllers might set could be helpful, too.. > + > +/* > + * QEMU macros to manipulate the trigger payload in native endian > + */ > +#define XIVE_EAS_BLOCK(n) (((n) >> 28) & 0xf) > +#define XIVE_EAS_INDEX(n) ((n) & 0x0fffffff) > +#define XIVE_EAS(blk, idx) ((uint32_t)(blk) << 28 | (idx)) > > #define TM_SHIFT 16 > > diff --git a/hw/intc/pnv_xive.c b/hw/intc/pnv_xive.c > index 4c1fa024cdf5..61af3f23000f 100644 > --- a/hw/intc/pnv_xive.c > +++ b/hw/intc/pnv_xive.c > @@ -385,7 +385,7 @@ static int pnv_xive_get_eas(XiveRouter *xrtr, uint8_t blk, uint32_t idx, > * EAT lookups should be local to the IC > */ > if (pnv_xive_block_id(xive) != blk) { > - xive_error(xive, "VST: EAS %x is remote !?", XIVE_SRCNO(blk, idx)); > + xive_error(xive, "VST: EAS %x is remote !?", XIVE_EAS(blk, idx)); > return -1; > } > > @@ -502,7 +502,7 @@ static void pnv_xive_notify(XiveNotifier *xn, uint32_t srcno) > PnvXive *xive = PNV_XIVE(xn); > uint8_t blk = pnv_xive_block_id(xive); > > - xive_router_notify(xn, XIVE_SRCNO(blk, srcno)); > + xive_router_notify(xn, XIVE_EAS(blk, srcno)); > } > > /* > @@ -1287,12 +1287,20 @@ static const MemoryRegionOps pnv_xive_ic_reg_ops = { > > static void pnv_xive_ic_hw_trigger(PnvXive *xive, hwaddr addr, uint64_t val) > { > + uint8_t blk = XIVE_EAS_BLOCK(val); > + uint32_t idx = XIVE_EAS_INDEX(val); > + > /* > * Forward the source event notification directly to the Router. > * The source interrupt number should already be correctly encoded > * with the chip block id by the sending device (PHB, PSI). > */ > - xive_router_notify(XIVE_NOTIFIER(xive), val); > + if (val & XIVE_TRIGGER_EAS) { > + xive_router_notify(XIVE_NOTIFIER(xive), XIVE_EAS(blk, idx)); > + } else { > + xive_error(xive, "IC: END trigger at @0x%"HWADDR_PRIx" data 0x%"PRIx64, > + addr, val); > + } > } > > static void pnv_xive_ic_notify_write(void *opaque, hwaddr addr, uint64_t val, > @@ -1683,7 +1691,7 @@ void pnv_xive_pic_print_info(PnvXive *xive, Monitor *mon) > XiveRouter *xrtr = XIVE_ROUTER(xive); > uint8_t blk = pnv_xive_block_id(xive); > uint8_t chip_id = xive->chip->chip_id; > - uint32_t srcno0 = XIVE_SRCNO(blk, 0); > + uint32_t srcno0 = XIVE_EAS(blk, 0); > uint32_t nr_ipis = pnv_xive_nr_ipis(xive, blk); > XiveEAS eas; > XiveEND end; > diff --git a/hw/intc/xive.c b/hw/intc/xive.c > index 07b7c3586c12..6702f32be601 100644 > --- a/hw/intc/xive.c > +++ b/hw/intc/xive.c > @@ -1652,8 +1652,8 @@ do_escalation: > void xive_router_notify(XiveNotifier *xn, uint32_t lisn) > { > XiveRouter *xrtr = XIVE_ROUTER(xn); > - uint8_t eas_blk = XIVE_SRCNO_BLOCK(lisn); > - uint32_t eas_idx = XIVE_SRCNO_INDEX(lisn); > + uint8_t eas_blk = XIVE_EAS_BLOCK(lisn); > + uint32_t eas_idx = XIVE_EAS_INDEX(lisn); > XiveEAS eas; > > /* EAS cache lookup */
On 03/10/2019 04:41, David Gibson wrote: > On Wed, Sep 18, 2019 at 06:06:44PM +0200, Cédric Le Goater wrote: >> The trigger definition is used for triggers both for HW source >> interrupts, PHB, PSI, as well as for rerouting interrupts between >> Interrupt Controller. >> >> HW source controllers set bit0 of word0 to ‘0’ as they provide EAS >> information (EAS block + EAS index) in the 8 byte data and not END >> information, and bit1 of word0 to ‘1’ to signal that the state bit >> check has been performed. >> >> Introduce these new trigger bits and rename the XIVE_SRCNO macros in >> XIVE_EAS to reflect better the nature of the data. This is breaking >> the notification for the PSI model which will be fixed in the next >> patch. >> >> Signed-off-by: Cédric Le Goater <clg@kaod.org> >> --- >> include/hw/ppc/xive_regs.h | 24 +++++++++++++++++++++--- >> hw/intc/pnv_xive.c | 16 ++++++++++++---- >> hw/intc/xive.c | 4 ++-- >> 3 files changed, 35 insertions(+), 9 deletions(-) >> >> diff --git a/include/hw/ppc/xive_regs.h b/include/hw/ppc/xive_regs.h >> index dd42c33cef35..83a2f2cc1318 100644 >> --- a/include/hw/ppc/xive_regs.h >> +++ b/include/hw/ppc/xive_regs.h >> @@ -22,9 +22,27 @@ >> /* >> * Interrupt source number encoding on PowerBUS >> */ >> -#define XIVE_SRCNO_BLOCK(srcno) (((srcno) >> 28) & 0xf) >> -#define XIVE_SRCNO_INDEX(srcno) ((srcno) & 0x0fffffff) >> -#define XIVE_SRCNO(blk, idx) ((uint32_t)(blk) << 28 | (idx)) >> +/* >> + * Trigger data definition >> + * >> + * The trigger definition is used for triggers both for HW source >> + * interrupts (PHB, PSI), as well as for rerouting interrupts between >> + * Interrupt Controller. >> + * >> + * HW source controllers set bit0 of word0 to ‘0’ as they provide EAS >> + * information (EAS block + EAS index) in the 8 byte data and not END >> + * information, and bit1 of word0 to ‘1’ to signal that the state bit >> + * check has been performed. >> + */ >> +#define XIVE_TRIGGER_END PPC_BIT(0) >> +#define XIVE_TRIGGER_EAS PPC_BIT(1) > > These names seem really confusing in light of the comment above. HW > source controllers set the XIVE_TRIGGER_END bit in order to indicate > that they're providing EAS info, not END info?? The END bit defines an END trigger, which is a forward trigger between ICs. In that case, the remote IC needs to do a lookup of an END and it needs an EAS containing an END index. An END trigger, bit0 of word0 set to '1', is defined as : 01234567 01234567 01234567 01234567 W0 E=1 1P--BLOC END IDX W1 E=1 M END DATA An EAS is defined as : 01234567 01234567 01234567 01234567 W0 V---BLOC END IDX W1 M END DATA Same layout as you can see. There is an extra 'PQ' bit, bit1 of word0 set to '1', signaling that the PQ bits have been checked. But that bit is unused in the initial EAS definition. > And also set the XIVE_TRIGGER_EAS bit to indicate something different? That's where the confusion comes from. I have called it 'EAS' because the trigger data in that case contains an EAS index which the IC needs to look for. This is the format the HW devices use to perform the trigger. An EAS trigger, bit0 of word0 set to '0', is defined as : 01234567 01234567 01234567 01234567 W0 E=0 0P------ -------- -------- -------- W1 E=0 BLOC EAS INDEX There is also a 'PQ' bit, bit1 of word0 to '1', signaling that the PQ bits have been checked. > Also, the comment implies that HW source controllers set both bits, > but I'm not really sure if that's what you meant. END trigger starts with 0b11 (IC <-> IC) EAS trigger starts with 0b01 (HW -> IC) > Expanding on what non-HW source controllers might set could be helpful > too .. what do you mean ? this is all HW ! guests ? C. > >> + >> +/* >> + * QEMU macros to manipulate the trigger payload in native endian >> + */ >> +#define XIVE_EAS_BLOCK(n) (((n) >> 28) & 0xf) >> +#define XIVE_EAS_INDEX(n) ((n) & 0x0fffffff) >> +#define XIVE_EAS(blk, idx) ((uint32_t)(blk) << 28 | (idx)) >> >> #define TM_SHIFT 16 >> >> diff --git a/hw/intc/pnv_xive.c b/hw/intc/pnv_xive.c >> index 4c1fa024cdf5..61af3f23000f 100644 >> --- a/hw/intc/pnv_xive.c >> +++ b/hw/intc/pnv_xive.c >> @@ -385,7 +385,7 @@ static int pnv_xive_get_eas(XiveRouter *xrtr, uint8_t blk, uint32_t idx, >> * EAT lookups should be local to the IC >> */ >> if (pnv_xive_block_id(xive) != blk) { >> - xive_error(xive, "VST: EAS %x is remote !?", XIVE_SRCNO(blk, idx)); >> + xive_error(xive, "VST: EAS %x is remote !?", XIVE_EAS(blk, idx)); >> return -1; >> } >> >> @@ -502,7 +502,7 @@ static void pnv_xive_notify(XiveNotifier *xn, uint32_t srcno) >> PnvXive *xive = PNV_XIVE(xn); >> uint8_t blk = pnv_xive_block_id(xive); >> >> - xive_router_notify(xn, XIVE_SRCNO(blk, srcno)); >> + xive_router_notify(xn, XIVE_EAS(blk, srcno)); >> } >> >> /* >> @@ -1287,12 +1287,20 @@ static const MemoryRegionOps pnv_xive_ic_reg_ops = { >> >> static void pnv_xive_ic_hw_trigger(PnvXive *xive, hwaddr addr, uint64_t val) >> { >> + uint8_t blk = XIVE_EAS_BLOCK(val); >> + uint32_t idx = XIVE_EAS_INDEX(val); >> + >> /* >> * Forward the source event notification directly to the Router. >> * The source interrupt number should already be correctly encoded >> * with the chip block id by the sending device (PHB, PSI). >> */ >> - xive_router_notify(XIVE_NOTIFIER(xive), val); >> + if (val & XIVE_TRIGGER_EAS) { >> + xive_router_notify(XIVE_NOTIFIER(xive), XIVE_EAS(blk, idx)); >> + } else { >> + xive_error(xive, "IC: END trigger at @0x%"HWADDR_PRIx" data 0x%"PRIx64, >> + addr, val); >> + } >> } >> >> static void pnv_xive_ic_notify_write(void *opaque, hwaddr addr, uint64_t val, >> @@ -1683,7 +1691,7 @@ void pnv_xive_pic_print_info(PnvXive *xive, Monitor *mon) >> XiveRouter *xrtr = XIVE_ROUTER(xive); >> uint8_t blk = pnv_xive_block_id(xive); >> uint8_t chip_id = xive->chip->chip_id; >> - uint32_t srcno0 = XIVE_SRCNO(blk, 0); >> + uint32_t srcno0 = XIVE_EAS(blk, 0); >> uint32_t nr_ipis = pnv_xive_nr_ipis(xive, blk); >> XiveEAS eas; >> XiveEND end; >> diff --git a/hw/intc/xive.c b/hw/intc/xive.c >> index 07b7c3586c12..6702f32be601 100644 >> --- a/hw/intc/xive.c >> +++ b/hw/intc/xive.c >> @@ -1652,8 +1652,8 @@ do_escalation: >> void xive_router_notify(XiveNotifier *xn, uint32_t lisn) >> { >> XiveRouter *xrtr = XIVE_ROUTER(xn); >> - uint8_t eas_blk = XIVE_SRCNO_BLOCK(lisn); >> - uint32_t eas_idx = XIVE_SRCNO_INDEX(lisn); >> + uint8_t eas_blk = XIVE_EAS_BLOCK(lisn); >> + uint32_t eas_idx = XIVE_EAS_INDEX(lisn); >> XiveEAS eas; >> >> /* EAS cache lookup */ >
diff --git a/include/hw/ppc/xive_regs.h b/include/hw/ppc/xive_regs.h index dd42c33cef35..83a2f2cc1318 100644 --- a/include/hw/ppc/xive_regs.h +++ b/include/hw/ppc/xive_regs.h @@ -22,9 +22,27 @@ /* * Interrupt source number encoding on PowerBUS */ -#define XIVE_SRCNO_BLOCK(srcno) (((srcno) >> 28) & 0xf) -#define XIVE_SRCNO_INDEX(srcno) ((srcno) & 0x0fffffff) -#define XIVE_SRCNO(blk, idx) ((uint32_t)(blk) << 28 | (idx)) +/* + * Trigger data definition + * + * The trigger definition is used for triggers both for HW source + * interrupts (PHB, PSI), as well as for rerouting interrupts between + * Interrupt Controller. + * + * HW source controllers set bit0 of word0 to ‘0’ as they provide EAS + * information (EAS block + EAS index) in the 8 byte data and not END + * information, and bit1 of word0 to ‘1’ to signal that the state bit + * check has been performed. + */ +#define XIVE_TRIGGER_END PPC_BIT(0) +#define XIVE_TRIGGER_EAS PPC_BIT(1) + +/* + * QEMU macros to manipulate the trigger payload in native endian + */ +#define XIVE_EAS_BLOCK(n) (((n) >> 28) & 0xf) +#define XIVE_EAS_INDEX(n) ((n) & 0x0fffffff) +#define XIVE_EAS(blk, idx) ((uint32_t)(blk) << 28 | (idx)) #define TM_SHIFT 16 diff --git a/hw/intc/pnv_xive.c b/hw/intc/pnv_xive.c index 4c1fa024cdf5..61af3f23000f 100644 --- a/hw/intc/pnv_xive.c +++ b/hw/intc/pnv_xive.c @@ -385,7 +385,7 @@ static int pnv_xive_get_eas(XiveRouter *xrtr, uint8_t blk, uint32_t idx, * EAT lookups should be local to the IC */ if (pnv_xive_block_id(xive) != blk) { - xive_error(xive, "VST: EAS %x is remote !?", XIVE_SRCNO(blk, idx)); + xive_error(xive, "VST: EAS %x is remote !?", XIVE_EAS(blk, idx)); return -1; } @@ -502,7 +502,7 @@ static void pnv_xive_notify(XiveNotifier *xn, uint32_t srcno) PnvXive *xive = PNV_XIVE(xn); uint8_t blk = pnv_xive_block_id(xive); - xive_router_notify(xn, XIVE_SRCNO(blk, srcno)); + xive_router_notify(xn, XIVE_EAS(blk, srcno)); } /* @@ -1287,12 +1287,20 @@ static const MemoryRegionOps pnv_xive_ic_reg_ops = { static void pnv_xive_ic_hw_trigger(PnvXive *xive, hwaddr addr, uint64_t val) { + uint8_t blk = XIVE_EAS_BLOCK(val); + uint32_t idx = XIVE_EAS_INDEX(val); + /* * Forward the source event notification directly to the Router. * The source interrupt number should already be correctly encoded * with the chip block id by the sending device (PHB, PSI). */ - xive_router_notify(XIVE_NOTIFIER(xive), val); + if (val & XIVE_TRIGGER_EAS) { + xive_router_notify(XIVE_NOTIFIER(xive), XIVE_EAS(blk, idx)); + } else { + xive_error(xive, "IC: END trigger at @0x%"HWADDR_PRIx" data 0x%"PRIx64, + addr, val); + } } static void pnv_xive_ic_notify_write(void *opaque, hwaddr addr, uint64_t val, @@ -1683,7 +1691,7 @@ void pnv_xive_pic_print_info(PnvXive *xive, Monitor *mon) XiveRouter *xrtr = XIVE_ROUTER(xive); uint8_t blk = pnv_xive_block_id(xive); uint8_t chip_id = xive->chip->chip_id; - uint32_t srcno0 = XIVE_SRCNO(blk, 0); + uint32_t srcno0 = XIVE_EAS(blk, 0); uint32_t nr_ipis = pnv_xive_nr_ipis(xive, blk); XiveEAS eas; XiveEND end; diff --git a/hw/intc/xive.c b/hw/intc/xive.c index 07b7c3586c12..6702f32be601 100644 --- a/hw/intc/xive.c +++ b/hw/intc/xive.c @@ -1652,8 +1652,8 @@ do_escalation: void xive_router_notify(XiveNotifier *xn, uint32_t lisn) { XiveRouter *xrtr = XIVE_ROUTER(xn); - uint8_t eas_blk = XIVE_SRCNO_BLOCK(lisn); - uint32_t eas_idx = XIVE_SRCNO_INDEX(lisn); + uint8_t eas_blk = XIVE_EAS_BLOCK(lisn); + uint32_t eas_idx = XIVE_EAS_INDEX(lisn); XiveEAS eas; /* EAS cache lookup */
The trigger definition is used for triggers both for HW source interrupts, PHB, PSI, as well as for rerouting interrupts between Interrupt Controller. HW source controllers set bit0 of word0 to ‘0’ as they provide EAS information (EAS block + EAS index) in the 8 byte data and not END information, and bit1 of word0 to ‘1’ to signal that the state bit check has been performed. Introduce these new trigger bits and rename the XIVE_SRCNO macros in XIVE_EAS to reflect better the nature of the data. This is breaking the notification for the PSI model which will be fixed in the next patch. Signed-off-by: Cédric Le Goater <clg@kaod.org> --- include/hw/ppc/xive_regs.h | 24 +++++++++++++++++++++--- hw/intc/pnv_xive.c | 16 ++++++++++++---- hw/intc/xive.c | 4 ++-- 3 files changed, 35 insertions(+), 9 deletions(-)