>>>> +/* >>>> + * Returns whether the event notification should be forwarded. >>>> + */ >>>> +static bool xive_source_lsi_trigger(XiveSource *xsrc, uint32_t >>>> srcno) >>> >>> What exactly "trigger" means isn't entirely obvious for an LSI. Might >>> be clearer to have "lsi_assert" and "lsi_deassert" helpers instead. >> >> This is called only when the interrupt is asserted. So it is a >> simplified LSI trigger depending only on the 'P' bit. > > Yes, I see that. But the result is that while the MSI logic is > encapsulated in the MSI trigger function, this leaves the LSI logic > split across the trigger function and set_irq() itself. I think it > would be better to have assert and deassert helpers instead, which > handle both the trigger/notification and also the updating of the > ASSERTED bit.
Something like the xive_source_set_irq_lsi() below ? Thanks, C. Signed-off-by: Cédric Le Goater <c...@kaod.org> --- hw/intc/xive.c | 58 ++++++++++++++++++++++++++++++++++++++++++++------ include/hw/ppc/xive.h | 19 +++++++++++++++- 2 files changed, 70 insertions(+), 7 deletions(-) Index: qemu.git/include/hw/ppc/xive.h =================================================================== --- qemu.git.orig/include/hw/ppc/xive.h +++ qemu.git/include/hw/ppc/xive.h @@ -32,8 +32,9 @@ typedef struct XiveSource { /* IRQs */ uint32_t nr_irqs; qemu_irq *qirqs; + unsigned long *lsi_map; - /* PQ bits */ + /* PQ bits and LSI assertion bit */ uint8_t *status; /* ESB memory region */ @@ -89,6 +90,7 @@ static inline hwaddr xive_source_esb_mgm * When doing an EOI, the Q bit will indicate if the interrupt * needs to be re-triggered. */ +#define XIVE_STATUS_ASSERTED 0x4 /* Extra bit for LSI */ #define XIVE_ESB_VAL_P 0x2 #define XIVE_ESB_VAL_Q 0x1 @@ -127,4 +129,19 @@ static inline qemu_irq xive_source_qirq( return xsrc->qirqs[srcno]; } +static inline bool xive_source_irq_is_lsi(XiveSource *xsrc, uint32_t srcno) +{ + assert(srcno < xsrc->nr_irqs); + return test_bit(srcno, xsrc->lsi_map); +} + +static inline void xive_source_irq_set(XiveSource *xsrc, uint32_t srcno, + bool lsi) +{ + assert(srcno < xsrc->nr_irqs); + if (lsi) { + bitmap_set(xsrc->lsi_map, srcno, 1); + } +} + #endif /* PPC_XIVE_H */ Index: qemu.git/hw/intc/xive.c =================================================================== --- qemu.git.orig/hw/intc/xive.c +++ qemu.git/hw/intc/xive.c @@ -91,11 +91,35 @@ uint8_t xive_source_esb_set(XiveSource * /* * Returns whether the event notification should be forwarded. */ +static bool xive_source_set_irq_lsi(XiveSource *xsrc, uint32_t srcno, int val) +{ + if (!val) { + xsrc->status[srcno] &= ~XIVE_STATUS_ASSERTED; + return false; + } + + xsrc->status[srcno] |= XIVE_STATUS_ASSERTED; + return xive_source_esb_get(xsrc, srcno) == XIVE_ESB_RESET; +} + +/* + * Returns whether the event notification should be forwarded. + */ static bool xive_source_esb_trigger(XiveSource *xsrc, uint32_t srcno) { + bool notify; + assert(srcno < xsrc->nr_irqs); - return xive_esb_trigger(&xsrc->status[srcno]); + notify = xive_esb_trigger(&xsrc->status[srcno]); + + if (xive_source_irq_is_lsi(xsrc, srcno) && + xive_source_esb_get(xsrc, srcno) == XIVE_ESB_QUEUED) { + qemu_log_mask(LOG_GUEST_ERROR, + "XIVE: queued an event on LSI IRQ %d\n", srcno); + } + + return notify; } /* @@ -103,9 +127,22 @@ static bool xive_source_esb_trigger(Xive */ static bool xive_source_esb_eoi(XiveSource *xsrc, uint32_t srcno) { + bool notify; + assert(srcno < xsrc->nr_irqs); - return xive_esb_eoi(&xsrc->status[srcno]); + notify = xive_esb_eoi(&xsrc->status[srcno]); + + /* LSI sources do not set the Q bit but they can still be + * asserted, in which case we should forward a new event + * notification + */ + if (xive_source_irq_is_lsi(xsrc, srcno)) { + bool level = xsrc->status[srcno] & XIVE_STATUS_ASSERTED; + notify = xive_source_set_irq_lsi(xsrc, srcno, level); + } + + return notify; } /* @@ -268,8 +305,12 @@ static void xive_source_set_irq(void *op XiveSource *xsrc = XIVE_SOURCE(opaque); bool notify = false; - if (val) { - notify = xive_source_esb_trigger(xsrc, srcno); + if (xive_source_irq_is_lsi(xsrc, srcno)) { + notify = xive_source_set_irq_lsi(xsrc, srcno, val); + } else { + if (val) { + notify = xive_source_esb_trigger(xsrc, srcno); + } } /* Forward the source event notification for routing */ @@ -289,9 +330,11 @@ void xive_source_pic_print_info(XiveSour continue; } - monitor_printf(mon, " %08x %c%c\n", i + offset, + monitor_printf(mon, " %08x %s %c%c%c\n", i + offset, + xive_source_irq_is_lsi(xsrc, i) ? "LSI" : "MSI", pq & XIVE_ESB_VAL_P ? 'P' : '-', - pq & XIVE_ESB_VAL_Q ? 'Q' : '-'); + pq & XIVE_ESB_VAL_Q ? 'Q' : '-', + xsrc->status[i] & XIVE_STATUS_ASSERTED ? 'A' : ' '); } } @@ -299,6 +342,8 @@ static void xive_source_reset(DeviceStat { XiveSource *xsrc = XIVE_SOURCE(dev); + /* Do not clear the LSI bitmap */ + /* PQs are initialized to 0b01 which corresponds to "ints off" */ memset(xsrc->status, 0x1, xsrc->nr_irqs); } @@ -324,6 +369,7 @@ static void xive_source_realize(DeviceSt xsrc->nr_irqs); xsrc->status = g_malloc0(xsrc->nr_irqs); + xsrc->lsi_map = bitmap_new(xsrc->nr_irqs); memory_region_init_io(&xsrc->esb_mmio, OBJECT(xsrc), &xive_source_esb_ops, xsrc, "xive.esb",