On 11/26/18 6:39 AM, David Gibson wrote: > On Fri, Nov 23, 2018 at 02:28:35PM +0100, Cédric Le Goater wrote: >> >>>>>> +/* >>>>>> + * 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 ? > > Uh.. not exactly what I had in mind, but close enough. > > [snip] > +/* >> + * 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) && > > Except that this block can go, since this function is no longer called > for LSIs.
It still can be through the ESB MMIOs, if the guest does a load on the trigger page. C. > >> + 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", >> >