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.
> + 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",
>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
