Re: [Qemu-ppc] [PATCH v5 02/36] ppc/xive: add support for the LSI interrupt sources

[Cédric Le Goater]

>>>> +/*
>>>> + * 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 <address@hidden>
---
 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",