ppc: support for the XIVE interrupt controller (POWER9)

Each XIVE interrupt source is associated with a two bit state machine called an Event State Buffer (ESB) : the first bit "P" means that an interrupt is "pending" and waiting for an EOI and the bit "Q" (queued) means a new interrupt was triggered while another was still pending.

When an event is triggered, the associated interrupt state bits are fetched and modified and forwarded to the virtualization engine of the controller doing the routing. These can also be controlled by MMIO, to trigger events or turn off the sources for instance. See code for more details on the states and transitions.

On a sPAPR machine, the OS will obtain the address of the MMIO page of the ESB entry associated with a source and its characteristic using the H_INT_GET_SOURCE_INFO hcall. On PowerNV, a similar OPAL call is used.

The xive_source_notify() routine is in charge forwarding the source event notification to the routing engine. It will be filled later on.

Signed-off-by Cédric Le Goater <[email protected]>

---
Changes since v2:

- added support for Store EOI
- added support for two page MMIO setting like on KVM

 default-configs/ppc64-softmmu.mak |   1 +
 hw/intc/Makefile.objs             |   1 +
 hw/intc/xive.c                    | 335 ++++++++++++++++++++++++++++++++++++++
 include/hw/ppc/xive.h             | 130 +++++++++++++++
 4 files changed, 467 insertions(+)
 create mode 100644 hw/intc/xive.c
 create mode 100644 include/hw/ppc/xive.h

default-configs/ppc64-softmmu.mak +1 -0

index b94af6c7c62a..c6d13e757977 100644
@@ -16,4 +16,5 @@ CONFIG_VIRTIO_VGA=y
 CONFIG_XICS=$(CONFIG_PSERIES)
 CONFIG_XICS_SPAPR=$(CONFIG_PSERIES)
 CONFIG_XICS_KVM=$(call land,$(CONFIG_PSERIES),$(CONFIG_KVM))
+CONFIG_XIVE=$(CONFIG_PSERIES)
 CONFIG_MEM_HOTPLUG=y

hw/intc/Makefile.objs +1 -0

index 0e9963f5eecc..72a46ed91c31 100644
@@ -37,6 +37,7 @@ obj-$(CONFIG_SH4) += sh_intc.o
 obj-$(CONFIG_XICS) += xics.o
 obj-$(CONFIG_XICS_SPAPR) += xics_spapr.o
 obj-$(CONFIG_XICS_KVM) += xics_kvm.o
+obj-$(CONFIG_XIVE) += xive.o
 obj-$(CONFIG_POWERNV) += xics_pnv.o
 obj-$(CONFIG_ALLWINNER_A10_PIC) += allwinner-a10-pic.o
 obj-$(CONFIG_S390_FLIC) += s390_flic.o

hw/intc/xive.c new file +98 -0

new file mode 100644
index 000000000000..c70578759d02
@@ -0,0 +1,335 @@
+/*
+ * QEMU PowerPC XIVE interrupt controller model
+ *
+ * Copyright (c) 2017-2018, IBM Corporation.
+ *
+ * This code is licensed under the GPL version 2 or later. See the
+ * COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/log.h"
+#include "qapi/error.h"
+#include "target/ppc/cpu.h"
+#include "sysemu/cpus.h"
+#include "sysemu/dma.h"
+#include "monitor/monitor.h"
+#include "hw/ppc/xive.h"
+
+/*
+ * XIVE Interrupt Source
+ */
+
+uint8_t xive_source_pq_get(XiveSource *xsrc, uint32_t srcno)
+{
+    uint32_t byte = srcno / 4;
+    uint32_t bit  = (srcno % 4) * 2;
+
+    assert(byte < xsrc->sbe_size);
+
+    return (xsrc->sbe[byte] >> bit) & 0x3;
+}
+
+uint8_t xive_source_pq_set(XiveSource *xsrc, uint32_t srcno, uint8_t pq)
+{
+    uint32_t byte = srcno / 4;
+    uint32_t bit  = (srcno % 4) * 2;
+    uint8_t old, new;
+
+    assert(byte < xsrc->sbe_size);
+
+    old = xsrc->sbe[byte];
+
+    new = xsrc->sbe[byte] & ~(0x3 << bit);
+    new |= (pq & 0x3) << bit;
+
+    xsrc->sbe[byte] = new;
+
+    return (old >> bit) & 0x3;
+}
+
+static bool xive_source_pq_eoi(XiveSource *xsrc, uint32_t srcno)
+{
+    uint8_t old_pq = xive_source_pq_get(xsrc, srcno);
+
+    switch (old_pq) {
+    case XIVE_ESB_RESET:
+        xive_source_pq_set(xsrc, srcno, XIVE_ESB_RESET);
+        return false;
+    case XIVE_ESB_PENDING:
+        xive_source_pq_set(xsrc, srcno, XIVE_ESB_RESET);
+        return false;
+    case XIVE_ESB_QUEUED:
+        xive_source_pq_set(xsrc, srcno, XIVE_ESB_PENDING);
+        return true;
+    case XIVE_ESB_OFF:
+        xive_source_pq_set(xsrc, srcno, XIVE_ESB_OFF);
+        return false;
+    default:
+         g_assert_not_reached();
+    }
+}
+
+/*
+ * Returns whether the event notification should be forwarded.
+ */
+static bool xive_source_pq_trigger(XiveSource *xsrc, uint32_t srcno)
+{
+    uint8_t old_pq = xive_source_pq_get(xsrc, srcno);
+
+    switch (old_pq) {
+    case XIVE_ESB_RESET:
+        xive_source_pq_set(xsrc, srcno, XIVE_ESB_PENDING);
+        return true;
+    case XIVE_ESB_PENDING:
+        xive_source_pq_set(xsrc, srcno, XIVE_ESB_QUEUED);
+        return false;
+    case XIVE_ESB_QUEUED:
+        xive_source_pq_set(xsrc, srcno, XIVE_ESB_QUEUED);
+        return false;
+    case XIVE_ESB_OFF:
+        xive_source_pq_set(xsrc, srcno, XIVE_ESB_OFF);
+        return false;
+    default:
+         g_assert_not_reached();
+    }
+}
+
+/*

hw/intc/xive.c new file +7 -0

+    uint32_t srcno = addr >> xsrc->esb_shift;
+    uint64_t ret = -1;
+
+    if (xive_source_esb_2page(xsrc) && xive_source_is_trigger_page(addr)) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "XIVE: invalid load on IRQ %d trigger page at "
+                      "0x%"HWADDR_PRIx"\n", srcno, addr);

hw/intc/xive.c new file +53 -0

+
+        break;
+
+    case XIVE_ESB_GET:
+        ret = xive_source_pq_get(xsrc, srcno);
+        break;
+
+    case XIVE_ESB_SET_PQ_00:
+    case XIVE_ESB_SET_PQ_01:
+    case XIVE_ESB_SET_PQ_10:
+    case XIVE_ESB_SET_PQ_11:
+        ret = xive_source_pq_set(xsrc, srcno, (offset >> 8) & 0x3);
+        break;
+    default:
+        qemu_log_mask(LOG_GUEST_ERROR, "XIVE: invalid ESB addr %d\n", offset);
+    }
+
+    return ret;
+}
+
+static void xive_source_esb_write(void *opaque, hwaddr addr,
+                                 uint64_t value, unsigned size)
+{
+    XiveSource *xsrc = XIVE_SOURCE(opaque);
+    uint32_t offset = addr & 0xF00;
+    uint32_t srcno = addr >> xsrc->esb_shift;
+    bool notify = false;
+
+    switch (offset) {
+    case 0:
+        notify = xive_source_pq_trigger(xsrc, srcno);
+        break;
+
+    case XIVE_ESB_STORE_EOI:
+        if (xive_source_is_trigger_page(addr)) {
+            qemu_log_mask(LOG_GUEST_ERROR,
+                          "XIVE: invalid store on IRQ %d trigger page at "
+                          "0x%"HWADDR_PRIx"\n", srcno, addr);
+            return;
+        }
+
+        if (!(xsrc->esb_flags & XIVE_SRC_STORE_EOI)) {
+            qemu_log_mask(LOG_GUEST_ERROR,
+                          "XIVE: invalid Store EOI for IRQ %d\n", srcno);
+            return;
+        }
+
+        /* If the Q bit is set, we should forward a new source event
+         * notification
+         */
+        notify = xive_source_pq_eoi(xsrc, srcno);
+        break;
+

hw/intc/xive.c new file +100 -0

+}
+
+static const MemoryRegionOps xive_source_esb_ops = {
+    .read = xive_source_esb_read,
+    .write = xive_source_esb_write,
+    .endianness = DEVICE_BIG_ENDIAN,
+    .valid = {
+        .min_access_size = 8,
+        .max_access_size = 8,
+    },
+    .impl = {
+        .min_access_size = 8,
+        .max_access_size = 8,
+    },
+};
+
+static void xive_source_set_irq(void *opaque, int srcno, int val)
+{
+    XiveSource *xsrc = XIVE_SOURCE(opaque);
+    bool notify = false;
+
+    if (val) {
+        notify = xive_source_pq_trigger(xsrc, srcno);
+    }
+
+    /* Forward the source event notification for routing */
+    if (notify) {
+        xive_source_notify(xsrc, srcno);
+    }
+}
+
+void xive_source_pic_print_info(XiveSource *xsrc, Monitor *mon)
+{
+    int i;
+
+    monitor_printf(mon, "XIVE Source %6x ..%6x\n",
+                   xsrc->offset, xsrc->offset + xsrc->nr_irqs - 1);
+    for (i = 0; i < xsrc->nr_irqs; i++) {
+        uint8_t pq = xive_source_pq_get(xsrc, i);
+        uint32_t lisn = i  + xsrc->offset;
+
+        if (pq == XIVE_ESB_OFF) {
+            continue;
+        }
+
+        monitor_printf(mon, "  %4x %c%c\n", lisn,
+                       pq & XIVE_ESB_VAL_P ? 'P' : '-',
+                       pq & XIVE_ESB_VAL_Q ? 'Q' : '-');
+    }
+}
+
+static void xive_source_reset(DeviceState *dev)
+{
+    XiveSource *xsrc = XIVE_SOURCE(dev);
+
+    /* SBEs are initialized to 0b01 which corresponds to "ints off" */
+    memset(xsrc->sbe, 0x55, xsrc->sbe_size);
+}
+
+static void xive_source_realize(DeviceState *dev, Error **errp)
+{
+    XiveSource *xsrc = XIVE_SOURCE(dev);
+
+    if (!xsrc->nr_irqs) {
+        error_setg(errp, "Number of interrupt needs to be greater than 0");
+        return;
+    }
+
+    if (xsrc->esb_shift != XIVE_ESB_4K &&
+        xsrc->esb_shift != XIVE_ESB_4K_2PAGE &&
+        xsrc->esb_shift != XIVE_ESB_64K &&
+        xsrc->esb_shift != XIVE_ESB_64K_2PAGE) {
+        error_setg(errp, "Invalid ESB shift setting");
+        return;
+    }
+
+    xsrc->qirqs = qemu_allocate_irqs(xive_source_set_irq, xsrc,
+                                     xsrc->nr_irqs);
+
+    /* Allocate the SBEs (State Bit Entry). 2 bits, so 4 entries per byte */
+    xsrc->sbe_size = DIV_ROUND_UP(xsrc->nr_irqs, 4);
+    xsrc->sbe = g_malloc0(xsrc->sbe_size);
+
+    /* TODO: H_INT_ESB support, which removing the ESB MMIOs */
+
+    memory_region_init_io(&xsrc->esb_mmio, OBJECT(xsrc),
+                          &xive_source_esb_ops, xsrc, "xive.esb",
+                          (1ull << xsrc->esb_shift) * xsrc->nr_irqs);
+    sysbus_init_mmio(SYS_BUS_DEVICE(dev), &xsrc->esb_mmio);
+}
+
+static const VMStateDescription vmstate_xive_source = {
+    .name = TYPE_XIVE_SOURCE,
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32_EQUAL(nr_irqs, XiveSource, NULL),
+        VMSTATE_VBUFFER_UINT32(sbe, XiveSource, 1, NULL, sbe_size),
+        VMSTATE_END_OF_LIST()
+    },

hw/intc/xive.c new file +28 -0

+    DEFINE_PROP_UINT32("shift", XiveSource, esb_shift, XIVE_ESB_64K_2PAGE),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void xive_source_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->realize = xive_source_realize;
+    dc->reset = xive_source_reset;
+    dc->props = xive_source_properties;
+    dc->desc = "XIVE interrupt source";
+    dc->vmsd = &vmstate_xive_source;
+}
+
+static const TypeInfo xive_source_info = {
+    .name          = TYPE_XIVE_SOURCE,
+    .parent        = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(XiveSource),
+    .class_init    = xive_source_class_init,
+};
+
+static void xive_register_types(void)
+{
+    type_register_static(&xive_source_info);
+}
+
+type_init(xive_register_types)

include/hw/ppc/xive.h new file +44 -0

new file mode 100644
index 000000000000..d92a50519edf
@@ -0,0 +1,130 @@
+/*
+ * QEMU PowerPC XIVE interrupt controller model
+ *
+ * Copyright (c) 2017-2018, IBM Corporation.
+ *
+ * This code is licensed under the GPL version 2 or later. See the
+ * COPYING file in the top-level directory.
+ */
+
+#ifndef PPC_XIVE_H
+#define PPC_XIVE_H
+
+#include "hw/sysbus.h"
+
+/*
+ * XIVE Interrupt Source
+ */
+
+#define TYPE_XIVE_SOURCE "xive-source"
+#define XIVE_SOURCE(obj) OBJECT_CHECK(XiveSource, (obj), TYPE_XIVE_SOURCE)
+
+/*
+ * XIVE Source Interrupt source characteristics, which define how the
+ * ESB are controlled.
+ */
+#define XIVE_SRC_H_INT_ESB     0x1 /* ESB managed with hcall H_INT_ESB */
+#define XIVE_SRC_STORE_EOI     0x4 /* Store EOI supported */
+
+typedef struct XiveSource {
+    SysBusDevice parent;
+
+    /* IRQs */
+    uint32_t     nr_irqs;
+    uint32_t     offset;
+    qemu_irq     *qirqs;
+
+    /* PQ bits */
+    uint8_t      *sbe;
+    uint32_t     sbe_size;
+
+    /* ESB memory region */
+    uint64_t     esb_flags;
+    hwaddr       esb_base;
+    uint32_t     esb_shift;

include/hw/ppc/xive.h new file +74 -0

+
+static inline bool xive_source_esb_2page(XiveSource *xsrc)
+{
+    return xsrc->esb_shift == XIVE_ESB_64K_2PAGE;
+}
+
+static inline hwaddr xive_source_esb_base(XiveSource *xsrc, uint32_t srcno)
+{
+    assert(srcno < xsrc->nr_irqs);
+    return xsrc->esb_base + (1ull << xsrc->esb_shift) * srcno;
+}
+
+/* The trigger page is always the first/even page */
+#define xive_source_esb_trigger xive_source_esb_base
+
+/* In a two pages ESB MMIO setting, the odd page is for management */
+static inline hwaddr xive_source_esb_mgmt(XiveSource *xsrc, int srcno)
+{
+    hwaddr addr = xive_source_esb_base(xsrc, srcno);
+
+    if (xive_source_esb_2page(xsrc)) {
+        addr += (1 << (xsrc->esb_shift - 1));
+    }
+
+    return addr;
+}
+
+/*
+ * Each interrupt source has a 2-bit state machine called ESB which
+ * can be controlled by MMIO. It's made of 2 bits, P and Q. P
+ * indicates that an interrupt is pending (has been sent to a queue
+ * and is waiting for an EOI). Q indicates that the interrupt has been
+ * triggered while pending.
+ *
+ * This acts as a coalescing mechanism in order to guarantee
+ * that a given interrupt only occurs at most once in a queue.
+ *
+ * When doing an EOI, the Q bit will indicate if the interrupt
+ * needs to be re-triggered.
+ */
+#define XIVE_ESB_VAL_P        0x2
+#define XIVE_ESB_VAL_Q        0x1
+
+#define XIVE_ESB_RESET        0x0
+#define XIVE_ESB_PENDING      XIVE_ESB_VAL_P
+#define XIVE_ESB_QUEUED       (XIVE_ESB_VAL_P | XIVE_ESB_VAL_Q)
+#define XIVE_ESB_OFF          XIVE_ESB_VAL_Q
+
+/*
+ * "magic" Event State Buffer (ESB) MMIO offsets.
+ *
+ * The following offsets into the ESB MMIO allow to read or
+ * manipulate the PQ bits. They must be used with an 8-bytes
+ * load instruction. They all return the previous state of the
+ * interrupt (atomically).
+ *
+ * Additionally, some ESB pages support doing an EOI via a
+ * store at 0 and some ESBs support doing a trigger via a
+ * separate trigger page.
+ */
+#define XIVE_ESB_STORE_EOI      0x400 /* Store */
+#define XIVE_ESB_LOAD_EOI       0x000 /* Load */
+#define XIVE_ESB_GET            0x800 /* Load */
+#define XIVE_ESB_SET_PQ_00      0xc00 /* Load */
+#define XIVE_ESB_SET_PQ_01      0xd00 /* Load */
+#define XIVE_ESB_SET_PQ_10      0xe00 /* Load */
+#define XIVE_ESB_SET_PQ_11      0xf00 /* Load */
+
+uint8_t xive_source_pq_get(XiveSource *xsrc, uint32_t srcno);
+uint8_t xive_source_pq_set(XiveSource *xsrc, uint32_t srcno, uint8_t pq);
+
+void xive_source_pic_print_info(XiveSource *xsrc, Monitor *mon);
+
+#endif /* PPC_XIVE_H */

Quoted 19 lines · Cédric Le Goater

Each XIVE interrupt source is associated with a two bit state machine called an Event State Buffer (ESB) : the first bit "P" means that an interrupt is "pending" and waiting for an EOI and the bit "Q" (queued) means a new interrupt was triggered while another was still pending.

When an event is triggered, the associated interrupt state bits are fetched and modified and forwarded to the virtualization engine of the controller doing the routing. These can also be controlled by MMIO, to trigger events or turn off the sources for instance. See code for more details on the states and transitions.

On a sPAPR machine, the OS will obtain the address of the MMIO page of the ESB entry associated with a source and its characteristic using the H_INT_GET_SOURCE_INFO hcall. On PowerNV, a similar OPAL call is used.

The xive_source_notify() routine is in charge forwarding the source event notification to the routing engine. It will be filled later on.

Quoted 6 lines

Signed-off-by Cédric Le Goater <[email protected]>

---
Changes since v2:

- added support for Store EOI
- added support for two page MMIO setting like on KVM

Looks generally sane to me, though I have a few queries.

Quoted 140 lines

 default-configs/ppc64-softmmu.mak |   1 +
 hw/intc/Makefile.objs             |   1 +
 hw/intc/xive.c                    | 335 ++++++++++++++++++++++++++++++++++++++
 include/hw/ppc/xive.h             | 130 +++++++++++++++
 4 files changed, 467 insertions(+)
 create mode 100644 hw/intc/xive.c
 create mode 100644 include/hw/ppc/xive.h

default-configs/ppc64-softmmu.mak +1 -0

index b94af6c7c62a..c6d13e757977 100644
@@ -16,4 +16,5 @@ CONFIG_VIRTIO_VGA=y
 CONFIG_XICS=$(CONFIG_PSERIES)
 CONFIG_XICS_SPAPR=$(CONFIG_PSERIES)
 CONFIG_XICS_KVM=$(call land,$(CONFIG_PSERIES),$(CONFIG_KVM))
+CONFIG_XIVE=$(CONFIG_PSERIES)
 CONFIG_MEM_HOTPLUG=y

hw/intc/Makefile.objs +1 -0

index 0e9963f5eecc..72a46ed91c31 100644
@@ -37,6 +37,7 @@ obj-$(CONFIG_SH4) += sh_intc.o
 obj-$(CONFIG_XICS) += xics.o
 obj-$(CONFIG_XICS_SPAPR) += xics_spapr.o
 obj-$(CONFIG_XICS_KVM) += xics_kvm.o
+obj-$(CONFIG_XIVE) += xive.o
 obj-$(CONFIG_POWERNV) += xics_pnv.o
 obj-$(CONFIG_ALLWINNER_A10_PIC) += allwinner-a10-pic.o
 obj-$(CONFIG_S390_FLIC) += s390_flic.o

hw/intc/xive.c new file +103 -0

new file mode 100644
index 000000000000..c70578759d02
@@ -0,0 +1,335 @@
+/*
+ * QEMU PowerPC XIVE interrupt controller model
+ *
+ * Copyright (c) 2017-2018, IBM Corporation.
+ *
+ * This code is licensed under the GPL version 2 or later. See the
+ * COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/log.h"
+#include "qapi/error.h"
+#include "target/ppc/cpu.h"
+#include "sysemu/cpus.h"
+#include "sysemu/dma.h"
+#include "monitor/monitor.h"
+#include "hw/ppc/xive.h"
+
+/*
+ * XIVE Interrupt Source
+ */
+
+uint8_t xive_source_pq_get(XiveSource *xsrc, uint32_t srcno)
+{
+    uint32_t byte = srcno / 4;
+    uint32_t bit  = (srcno % 4) * 2;
+
+    assert(byte < xsrc->sbe_size);
+
+    return (xsrc->sbe[byte] >> bit) & 0x3;
+}
+
+uint8_t xive_source_pq_set(XiveSource *xsrc, uint32_t srcno, uint8_t pq)
+{
+    uint32_t byte = srcno / 4;
+    uint32_t bit  = (srcno % 4) * 2;
+    uint8_t old, new;
+
+    assert(byte < xsrc->sbe_size);
+
+    old = xsrc->sbe[byte];
+
+    new = xsrc->sbe[byte] & ~(0x3 << bit);
+    new |= (pq & 0x3) << bit;
+
+    xsrc->sbe[byte] = new;
+
+    return (old >> bit) & 0x3;
+}
+
+static bool xive_source_pq_eoi(XiveSource *xsrc, uint32_t srcno)
+{
+    uint8_t old_pq = xive_source_pq_get(xsrc, srcno);
+
+    switch (old_pq) {
+    case XIVE_ESB_RESET:
+        xive_source_pq_set(xsrc, srcno, XIVE_ESB_RESET);
+        return false;
+    case XIVE_ESB_PENDING:
+        xive_source_pq_set(xsrc, srcno, XIVE_ESB_RESET);
+        return false;
+    case XIVE_ESB_QUEUED:
+        xive_source_pq_set(xsrc, srcno, XIVE_ESB_PENDING);
+        return true;
+    case XIVE_ESB_OFF:
+        xive_source_pq_set(xsrc, srcno, XIVE_ESB_OFF);
+        return false;
+    default:
+         g_assert_not_reached();
+    }
+}
+
+/*
+ * Returns whether the event notification should be forwarded.
+ */
+static bool xive_source_pq_trigger(XiveSource *xsrc, uint32_t srcno)
+{
+    uint8_t old_pq = xive_source_pq_get(xsrc, srcno);
+
+    switch (old_pq) {
+    case XIVE_ESB_RESET:
+        xive_source_pq_set(xsrc, srcno, XIVE_ESB_PENDING);
+        return true;
+    case XIVE_ESB_PENDING:
+        xive_source_pq_set(xsrc, srcno, XIVE_ESB_QUEUED);
+        return false;
+    case XIVE_ESB_QUEUED:
+        xive_source_pq_set(xsrc, srcno, XIVE_ESB_QUEUED);
+        return false;
+    case XIVE_ESB_OFF:
+        xive_source_pq_set(xsrc, srcno, XIVE_ESB_OFF);
+        return false;
+    default:
+         g_assert_not_reached();
+    }
+}
+
+/*
+ * Forward the source event notification to the associated XiveFabric,
+ * the device owning the sources.
+ */
+static void xive_source_notify(XiveSource *xsrc, int srcno)
+{

hw/intc/xive.c line 108

Quoted 5 lines

hw/intc/xive.c new file +5 -0

+
+}
+
+/* In a two pages ESB MMIO setting, even page is the trigger page, odd
+ * page is for management */

Can I understand from this that the result from this function is only meaningful in 2-pages mode?

hw/intc/xive.c line 111

Quoted 3 lines

hw/intc/xive.c new file +3 -0

+static inline bool xive_source_is_trigger_page(hwaddr addr)
+{
+    return !((addr >> 16) & 1);

Later on you seem to have both 4k and 64k variants list, but here you hardcode 64k. Is that a problem?

hw/intc/xive.c line 117

Quoted 6 lines

hw/intc/xive.c new file +6 -0

+}
+
+static uint64_t xive_source_esb_read(void *opaque, hwaddr addr, unsigned size)
+{
+    XiveSource *xsrc = XIVE_SOURCE(opaque);
+    uint32_t offset = addr & 0xF00;

You ignore the low bits of the address entirely, so effective you have a 256 byte range that's all aliases of the same register. Is that intentional?

Quoted 12 lines

hw/intc/xive.c new file +12 -0

+    uint32_t srcno = addr >> xsrc->esb_shift;
+    uint64_t ret = -1;
+
+    if (xive_source_esb_2page(xsrc) && xive_source_is_trigger_page(addr)) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "XIVE: invalid load on IRQ %d trigger page at "
+                      "0x%"HWADDR_PRIx"\n", srcno, addr);
+        return -1;
+    }
+
+    switch (offset) {
+    case XIVE_ESB_LOAD_EOI:

hw/intc/xive.c line 134

Quoted 5 lines

hw/intc/xive.c new file +5 -0

+        /*
+         * Load EOI is not the default source setting under QEMU, but
+         * this is what HW uses currently.
+         */
+        ret = xive_source_pq_eoi(xsrc, srcno);

You're implicitly casting a bool return value into a u64 here, is that intentional?

Quoted 58 lines

hw/intc/xive.c new file +58 -0

+
+        break;
+
+    case XIVE_ESB_GET:
+        ret = xive_source_pq_get(xsrc, srcno);
+        break;
+
+    case XIVE_ESB_SET_PQ_00:
+    case XIVE_ESB_SET_PQ_01:
+    case XIVE_ESB_SET_PQ_10:
+    case XIVE_ESB_SET_PQ_11:
+        ret = xive_source_pq_set(xsrc, srcno, (offset >> 8) & 0x3);
+        break;
+    default:
+        qemu_log_mask(LOG_GUEST_ERROR, "XIVE: invalid ESB addr %d\n", offset);
+    }
+
+    return ret;
+}
+
+static void xive_source_esb_write(void *opaque, hwaddr addr,
+                                 uint64_t value, unsigned size)
+{
+    XiveSource *xsrc = XIVE_SOURCE(opaque);
+    uint32_t offset = addr & 0xF00;
+    uint32_t srcno = addr >> xsrc->esb_shift;
+    bool notify = false;
+
+    switch (offset) {
+    case 0:
+        notify = xive_source_pq_trigger(xsrc, srcno);
+        break;
+
+    case XIVE_ESB_STORE_EOI:
+        if (xive_source_is_trigger_page(addr)) {
+            qemu_log_mask(LOG_GUEST_ERROR,
+                          "XIVE: invalid store on IRQ %d trigger page at "
+                          "0x%"HWADDR_PRIx"\n", srcno, addr);
+            return;
+        }
+
+        if (!(xsrc->esb_flags & XIVE_SRC_STORE_EOI)) {
+            qemu_log_mask(LOG_GUEST_ERROR,
+                          "XIVE: invalid Store EOI for IRQ %d\n", srcno);
+            return;
+        }
+
+        /* If the Q bit is set, we should forward a new source event
+         * notification
+         */
+        notify = xive_source_pq_eoi(xsrc, srcno);
+        break;
+
+    default:
+        qemu_log_mask(LOG_GUEST_ERROR, "XIVE: invalid ESB write addr %d\n",
+                      offset);
+        return;
+    }

hw/intc/xive.c line 197

Quoted 5 lines

hw/intc/xive.c new file +5 -0

+
+    /* Forward the source event notification for routing */
+    if (notify) {
+        xive_source_notify(xsrc, srcno);
+    }

EOI via this path calls notify, but the one via the read path doesn't. Is that correct?

Quoted 106 lines

hw/intc/xive.c new file +105 -0

+}
+
+static const MemoryRegionOps xive_source_esb_ops = {
+    .read = xive_source_esb_read,
+    .write = xive_source_esb_write,
+    .endianness = DEVICE_BIG_ENDIAN,
+    .valid = {
+        .min_access_size = 8,
+        .max_access_size = 8,
+    },
+    .impl = {
+        .min_access_size = 8,
+        .max_access_size = 8,
+    },
+};
+
+static void xive_source_set_irq(void *opaque, int srcno, int val)
+{
+    XiveSource *xsrc = XIVE_SOURCE(opaque);
+    bool notify = false;
+
+    if (val) {
+        notify = xive_source_pq_trigger(xsrc, srcno);
+    }
+
+    /* Forward the source event notification for routing */
+    if (notify) {
+        xive_source_notify(xsrc, srcno);
+    }
+}
+
+void xive_source_pic_print_info(XiveSource *xsrc, Monitor *mon)
+{
+    int i;
+
+    monitor_printf(mon, "XIVE Source %6x ..%6x\n",
+                   xsrc->offset, xsrc->offset + xsrc->nr_irqs - 1);
+    for (i = 0; i < xsrc->nr_irqs; i++) {
+        uint8_t pq = xive_source_pq_get(xsrc, i);
+        uint32_t lisn = i  + xsrc->offset;
+
+        if (pq == XIVE_ESB_OFF) {
+            continue;
+        }
+
+        monitor_printf(mon, "  %4x %c%c\n", lisn,
+                       pq & XIVE_ESB_VAL_P ? 'P' : '-',
+                       pq & XIVE_ESB_VAL_Q ? 'Q' : '-');
+    }
+}
+
+static void xive_source_reset(DeviceState *dev)
+{
+    XiveSource *xsrc = XIVE_SOURCE(dev);
+
+    /* SBEs are initialized to 0b01 which corresponds to "ints off" */
+    memset(xsrc->sbe, 0x55, xsrc->sbe_size);
+}
+
+static void xive_source_realize(DeviceState *dev, Error **errp)
+{
+    XiveSource *xsrc = XIVE_SOURCE(dev);
+
+    if (!xsrc->nr_irqs) {
+        error_setg(errp, "Number of interrupt needs to be greater than 0");
+        return;
+    }
+
+    if (xsrc->esb_shift != XIVE_ESB_4K &&
+        xsrc->esb_shift != XIVE_ESB_4K_2PAGE &&
+        xsrc->esb_shift != XIVE_ESB_64K &&
+        xsrc->esb_shift != XIVE_ESB_64K_2PAGE) {
+        error_setg(errp, "Invalid ESB shift setting");
+        return;
+    }
+
+    xsrc->qirqs = qemu_allocate_irqs(xive_source_set_irq, xsrc,
+                                     xsrc->nr_irqs);
+
+    /* Allocate the SBEs (State Bit Entry). 2 bits, so 4 entries per byte */
+    xsrc->sbe_size = DIV_ROUND_UP(xsrc->nr_irqs, 4);
+    xsrc->sbe = g_malloc0(xsrc->sbe_size);
+
+    /* TODO: H_INT_ESB support, which removing the ESB MMIOs */
+
+    memory_region_init_io(&xsrc->esb_mmio, OBJECT(xsrc),
+                          &xive_source_esb_ops, xsrc, "xive.esb",
+                          (1ull << xsrc->esb_shift) * xsrc->nr_irqs);
+    sysbus_init_mmio(SYS_BUS_DEVICE(dev), &xsrc->esb_mmio);
+}
+
+static const VMStateDescription vmstate_xive_source = {
+    .name = TYPE_XIVE_SOURCE,
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32_EQUAL(nr_irqs, XiveSource, NULL),
+        VMSTATE_VBUFFER_UINT32(sbe, XiveSource, 1, NULL, sbe_size),
+        VMSTATE_END_OF_LIST()
+    },
+};
+
+/*
+ * The default XIVE interrupt source setting for ESB MMIO is two 64k
+ * pages without Store EOI. This is in sync with KVM.

hw/intc/xive.c line 307

Quoted 5 lines

hw/intc/xive.c new file +5 -0

+ */
+static Property xive_source_properties[] = {
+    DEFINE_PROP_UINT64("flags", XiveSource, esb_flags, 0),
+    DEFINE_PROP_UINT32("nr-irqs", XiveSource, nr_irqs, 0),
+    DEFINE_PROP_UINT64("bar", XiveSource, esb_base, 0),

Isn't this redundant with however the base address is handled through the SysBusDevice stuff (I forget the details)?

Quoted 82 lines

hw/intc/xive.c new file +28 -0

+    DEFINE_PROP_UINT32("shift", XiveSource, esb_shift, XIVE_ESB_64K_2PAGE),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void xive_source_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->realize = xive_source_realize;
+    dc->reset = xive_source_reset;
+    dc->props = xive_source_properties;
+    dc->desc = "XIVE interrupt source";
+    dc->vmsd = &vmstate_xive_source;
+}
+
+static const TypeInfo xive_source_info = {
+    .name          = TYPE_XIVE_SOURCE,
+    .parent        = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(XiveSource),
+    .class_init    = xive_source_class_init,
+};
+
+static void xive_register_types(void)
+{
+    type_register_static(&xive_source_info);
+}
+
+type_init(xive_register_types)

include/hw/ppc/xive.h new file +48 -0

new file mode 100644
index 000000000000..d92a50519edf
@@ -0,0 +1,130 @@
+/*
+ * QEMU PowerPC XIVE interrupt controller model
+ *
+ * Copyright (c) 2017-2018, IBM Corporation.
+ *
+ * This code is licensed under the GPL version 2 or later. See the
+ * COPYING file in the top-level directory.
+ */
+
+#ifndef PPC_XIVE_H
+#define PPC_XIVE_H
+
+#include "hw/sysbus.h"
+
+/*
+ * XIVE Interrupt Source
+ */
+
+#define TYPE_XIVE_SOURCE "xive-source"
+#define XIVE_SOURCE(obj) OBJECT_CHECK(XiveSource, (obj), TYPE_XIVE_SOURCE)
+
+/*
+ * XIVE Source Interrupt source characteristics, which define how the
+ * ESB are controlled.
+ */
+#define XIVE_SRC_H_INT_ESB     0x1 /* ESB managed with hcall H_INT_ESB */
+#define XIVE_SRC_STORE_EOI     0x4 /* Store EOI supported */
+
+typedef struct XiveSource {
+    SysBusDevice parent;
+
+    /* IRQs */
+    uint32_t     nr_irqs;
+    uint32_t     offset;
+    qemu_irq     *qirqs;
+
+    /* PQ bits */
+    uint8_t      *sbe;
+    uint32_t     sbe_size;
+
+    /* ESB memory region */
+    uint64_t     esb_flags;
+    hwaddr       esb_base;
+    uint32_t     esb_shift;
+    MemoryRegion esb_mmio;
+} XiveSource;
+
+/*

include/hw/ppc/xive.h line 54

Quoted 6 lines

include/hw/ppc/xive.h new file +6 -0

+ * ESB MMIO setting. Can be one page, for both source triggering and
+ * source management, or two different pages. See below for magic
+ * values.
+ */
+#define XIVE_ESB_4K          12 /* PSI HB */
+#define XIVE_ESB_4K_2PAGE    17

Should this be 13 instead of 17?

include/hw/ppc/xive.h line 56

Quoted 2 lines

include/hw/ppc/xive.h new file +2 -0

+#define XIVE_ESB_64K         16
+#define XIVE_ESB_64K_2PAGE   17

(Also, who the hell comes up with a brand new PIC and decides to have *4* different interface variants. But that's not your problem, I realise).

Quoted 74 lines

include/hw/ppc/xive.h new file +74 -0

+
+static inline bool xive_source_esb_2page(XiveSource *xsrc)
+{
+    return xsrc->esb_shift == XIVE_ESB_64K_2PAGE;
+}
+
+static inline hwaddr xive_source_esb_base(XiveSource *xsrc, uint32_t srcno)
+{
+    assert(srcno < xsrc->nr_irqs);
+    return xsrc->esb_base + (1ull << xsrc->esb_shift) * srcno;
+}
+
+/* The trigger page is always the first/even page */
+#define xive_source_esb_trigger xive_source_esb_base
+
+/* In a two pages ESB MMIO setting, the odd page is for management */
+static inline hwaddr xive_source_esb_mgmt(XiveSource *xsrc, int srcno)
+{
+    hwaddr addr = xive_source_esb_base(xsrc, srcno);
+
+    if (xive_source_esb_2page(xsrc)) {
+        addr += (1 << (xsrc->esb_shift - 1));
+    }
+
+    return addr;
+}
+
+/*
+ * Each interrupt source has a 2-bit state machine called ESB which
+ * can be controlled by MMIO. It's made of 2 bits, P and Q. P
+ * indicates that an interrupt is pending (has been sent to a queue
+ * and is waiting for an EOI). Q indicates that the interrupt has been
+ * triggered while pending.
+ *
+ * This acts as a coalescing mechanism in order to guarantee
+ * that a given interrupt only occurs at most once in a queue.
+ *
+ * When doing an EOI, the Q bit will indicate if the interrupt
+ * needs to be re-triggered.
+ */
+#define XIVE_ESB_VAL_P        0x2
+#define XIVE_ESB_VAL_Q        0x1
+
+#define XIVE_ESB_RESET        0x0
+#define XIVE_ESB_PENDING      XIVE_ESB_VAL_P
+#define XIVE_ESB_QUEUED       (XIVE_ESB_VAL_P | XIVE_ESB_VAL_Q)
+#define XIVE_ESB_OFF          XIVE_ESB_VAL_Q
+
+/*
+ * "magic" Event State Buffer (ESB) MMIO offsets.
+ *
+ * The following offsets into the ESB MMIO allow to read or
+ * manipulate the PQ bits. They must be used with an 8-bytes
+ * load instruction. They all return the previous state of the
+ * interrupt (atomically).
+ *
+ * Additionally, some ESB pages support doing an EOI via a
+ * store at 0 and some ESBs support doing a trigger via a
+ * separate trigger page.
+ */
+#define XIVE_ESB_STORE_EOI      0x400 /* Store */
+#define XIVE_ESB_LOAD_EOI       0x000 /* Load */
+#define XIVE_ESB_GET            0x800 /* Load */
+#define XIVE_ESB_SET_PQ_00      0xc00 /* Load */
+#define XIVE_ESB_SET_PQ_01      0xd00 /* Load */
+#define XIVE_ESB_SET_PQ_10      0xe00 /* Load */
+#define XIVE_ESB_SET_PQ_11      0xf00 /* Load */
+
+uint8_t xive_source_pq_get(XiveSource *xsrc, uint32_t srcno);
+uint8_t xive_source_pq_set(XiveSource *xsrc, uint32_t srcno, uint8_t pq);
+
+void xive_source_pic_print_info(XiveSource *xsrc, Monitor *mon);
+
+#endif /* PPC_XIVE_H */

Quoted 169 lines · David Gibson

Quoted 19 lines · Cédric Le Goater

Each XIVE interrupt source is associated with a two bit state machine called an Event State Buffer (ESB) : the first bit "P" means that an interrupt is "pending" and waiting for an EOI and the bit "Q" (queued) means a new interrupt was triggered while another was still pending.

When an event is triggered, the associated interrupt state bits are fetched and modified and forwarded to the virtualization engine of the controller doing the routing. These can also be controlled by MMIO, to trigger events or turn off the sources for instance. See code for more details on the states and transitions.

On a sPAPR machine, the OS will obtain the address of the MMIO page of the ESB entry associated with a source and its characteristic using the H_INT_GET_SOURCE_INFO hcall. On PowerNV, a similar OPAL call is used.

The xive_source_notify() routine is in charge forwarding the source event notification to the routing engine. It will be filled later on.

Quoted 6 lines

Signed-off-by Cédric Le Goater <[email protected]>

---
Changes since v2:

- added support for Store EOI
- added support for two page MMIO setting like on KVM

Looks generally sane to me, though I have a few queries.

Quoted 140 lines

 default-configs/ppc64-softmmu.mak |   1 +
 hw/intc/Makefile.objs             |   1 +
 hw/intc/xive.c                    | 335 ++++++++++++++++++++++++++++++++++++++
 include/hw/ppc/xive.h             | 130 +++++++++++++++
 4 files changed, 467 insertions(+)
 create mode 100644 hw/intc/xive.c
 create mode 100644 include/hw/ppc/xive.h

default-configs/ppc64-softmmu.mak +1 -0

index b94af6c7c62a..c6d13e757977 100644
@@ -16,4 +16,5 @@ CONFIG_VIRTIO_VGA=y
 CONFIG_XICS=$(CONFIG_PSERIES)
 CONFIG_XICS_SPAPR=$(CONFIG_PSERIES)
 CONFIG_XICS_KVM=$(call land,$(CONFIG_PSERIES),$(CONFIG_KVM))
+CONFIG_XIVE=$(CONFIG_PSERIES)
 CONFIG_MEM_HOTPLUG=y

hw/intc/Makefile.objs +1 -0

index 0e9963f5eecc..72a46ed91c31 100644
@@ -37,6 +37,7 @@ obj-$(CONFIG_SH4) += sh_intc.o
 obj-$(CONFIG_XICS) += xics.o
 obj-$(CONFIG_XICS_SPAPR) += xics_spapr.o
 obj-$(CONFIG_XICS_KVM) += xics_kvm.o
+obj-$(CONFIG_XIVE) += xive.o
 obj-$(CONFIG_POWERNV) += xics_pnv.o
 obj-$(CONFIG_ALLWINNER_A10_PIC) += allwinner-a10-pic.o
 obj-$(CONFIG_S390_FLIC) += s390_flic.o

hw/intc/xive.c new file +103 -0

new file mode 100644
index 000000000000..c70578759d02
@@ -0,0 +1,335 @@
+/*
+ * QEMU PowerPC XIVE interrupt controller model
+ *
+ * Copyright (c) 2017-2018, IBM Corporation.
+ *
+ * This code is licensed under the GPL version 2 or later. See the
+ * COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/log.h"
+#include "qapi/error.h"
+#include "target/ppc/cpu.h"
+#include "sysemu/cpus.h"
+#include "sysemu/dma.h"
+#include "monitor/monitor.h"
+#include "hw/ppc/xive.h"
+
+/*
+ * XIVE Interrupt Source
+ */
+
+uint8_t xive_source_pq_get(XiveSource *xsrc, uint32_t srcno)
+{
+    uint32_t byte = srcno / 4;
+    uint32_t bit  = (srcno % 4) * 2;
+
+    assert(byte < xsrc->sbe_size);
+
+    return (xsrc->sbe[byte] >> bit) & 0x3;
+}
+
+uint8_t xive_source_pq_set(XiveSource *xsrc, uint32_t srcno, uint8_t pq)
+{
+    uint32_t byte = srcno / 4;
+    uint32_t bit  = (srcno % 4) * 2;
+    uint8_t old, new;
+
+    assert(byte < xsrc->sbe_size);
+
+    old = xsrc->sbe[byte];
+
+    new = xsrc->sbe[byte] & ~(0x3 << bit);
+    new |= (pq & 0x3) << bit;
+
+    xsrc->sbe[byte] = new;
+
+    return (old >> bit) & 0x3;
+}
+
+static bool xive_source_pq_eoi(XiveSource *xsrc, uint32_t srcno)
+{
+    uint8_t old_pq = xive_source_pq_get(xsrc, srcno);
+
+    switch (old_pq) {
+    case XIVE_ESB_RESET:
+        xive_source_pq_set(xsrc, srcno, XIVE_ESB_RESET);
+        return false;
+    case XIVE_ESB_PENDING:
+        xive_source_pq_set(xsrc, srcno, XIVE_ESB_RESET);
+        return false;
+    case XIVE_ESB_QUEUED:
+        xive_source_pq_set(xsrc, srcno, XIVE_ESB_PENDING);
+        return true;
+    case XIVE_ESB_OFF:
+        xive_source_pq_set(xsrc, srcno, XIVE_ESB_OFF);
+        return false;
+    default:
+         g_assert_not_reached();
+    }
+}
+
+/*
+ * Returns whether the event notification should be forwarded.
+ */
+static bool xive_source_pq_trigger(XiveSource *xsrc, uint32_t srcno)
+{
+    uint8_t old_pq = xive_source_pq_get(xsrc, srcno);
+
+    switch (old_pq) {
+    case XIVE_ESB_RESET:
+        xive_source_pq_set(xsrc, srcno, XIVE_ESB_PENDING);
+        return true;
+    case XIVE_ESB_PENDING:
+        xive_source_pq_set(xsrc, srcno, XIVE_ESB_QUEUED);
+        return false;
+    case XIVE_ESB_QUEUED:
+        xive_source_pq_set(xsrc, srcno, XIVE_ESB_QUEUED);
+        return false;
+    case XIVE_ESB_OFF:
+        xive_source_pq_set(xsrc, srcno, XIVE_ESB_OFF);
+        return false;
+    default:
+         g_assert_not_reached();
+    }
+}
+
+/*
+ * Forward the source event notification to the associated XiveFabric,
+ * the device owning the sources.
+ */
+static void xive_source_notify(XiveSource *xsrc, int srcno)
+{

Quoted 8 lines

hw/intc/xive.c line 108

Quoted 5 lines

hw/intc/xive.c new file +5 -0

+
+}
+
+/* In a two pages ESB MMIO setting, even page is the trigger page, odd
+ * page is for management */

Can I understand from this that the result from this function is only meaningful in 2-pages mode?

yes. May be I should rename it to xive_source_is_even_page() ?

Quoted 6 lines

hw/intc/xive.c line 111

Quoted 3 lines

hw/intc/xive.c new file +3 -0

+static inline bool xive_source_is_trigger_page(hwaddr addr)
+{
+    return !((addr >> 16) & 1);

Later on you seem to have both 4k and 64k variants list, but here you hardcode 64k. Is that a problem?

oups. This should be :

	(addr >> (xsrc->esb_shift - 1))

I did the tests with the spapr guest which uses 64k ESB MMIO pages. The check is only significant in a 2 pages setting.

Quoted 10 lines

hw/intc/xive.c line 117

Quoted 6 lines

hw/intc/xive.c new file +6 -0

+}
+
+static uint64_t xive_source_esb_read(void *opaque, hwaddr addr, unsigned size)
+{
+    XiveSource *xsrc = XIVE_SOURCE(opaque);
+    uint32_t offset = addr & 0xF00;

You ignore the low bits of the address entirely, so effective you have a 256 byte range that's all aliases of the same register. Is that intentional?

yes but it's not entirely correct. The exact ranges are :

Load Store

0x000 .. 0x3FF EOI and return 0|1 Trigger
0x400 .. 0x7FF EOI and return 0|1 EOI
0x800 .. 0xBFF return PQ undefined
0xC00 .. 0xCFF return PQ and PQ=00 PQ=00
0xD00 .. 0xDFF return PQ and PQ=01 PQ=01
0xE00 .. 0xDFF return PQ and PQ=10 PQ=10
0xF00 .. 0xDFF return PQ and PQ=11 PQ=11

There is room for some improvements.

The trigger page in a two pages ESB MMIO settings only triggers on stores.

Quoted 12 lines

Quoted 12 lines

hw/intc/xive.c new file +12 -0

+    uint32_t srcno = addr >> xsrc->esb_shift;
+    uint64_t ret = -1;
+
+    if (xive_source_esb_2page(xsrc) && xive_source_is_trigger_page(addr)) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "XIVE: invalid load on IRQ %d trigger page at "
+                      "0x%"HWADDR_PRIx"\n", srcno, addr);
+        return -1;
+    }
+
+    switch (offset) {
+    case XIVE_ESB_LOAD_EOI:

Quoted 8 lines

hw/intc/xive.c line 134

Quoted 5 lines

hw/intc/xive.c new file +5 -0

+        /*
+         * Load EOI is not the default source setting under QEMU, but
+         * this is what HW uses currently.
+         */
+        ret = xive_source_pq_eoi(xsrc, srcno);

You're implicitly casting a bool return value into a u64 here, is that intentional?

yes. is that bad ? This is what the load is supposed to return in the transition algo.

Quoted 58 lines

Quoted 58 lines

hw/intc/xive.c new file +58 -0

+
+        break;
+
+    case XIVE_ESB_GET:
+        ret = xive_source_pq_get(xsrc, srcno);
+        break;
+
+    case XIVE_ESB_SET_PQ_00:
+    case XIVE_ESB_SET_PQ_01:
+    case XIVE_ESB_SET_PQ_10:
+    case XIVE_ESB_SET_PQ_11:
+        ret = xive_source_pq_set(xsrc, srcno, (offset >> 8) & 0x3);
+        break;
+    default:
+        qemu_log_mask(LOG_GUEST_ERROR, "XIVE: invalid ESB addr %d\n", offset);
+    }
+
+    return ret;
+}
+
+static void xive_source_esb_write(void *opaque, hwaddr addr,
+                                 uint64_t value, unsigned size)
+{
+    XiveSource *xsrc = XIVE_SOURCE(opaque);
+    uint32_t offset = addr & 0xF00;
+    uint32_t srcno = addr >> xsrc->esb_shift;
+    bool notify = false;
+
+    switch (offset) {
+    case 0:
+        notify = xive_source_pq_trigger(xsrc, srcno);
+        break;
+
+    case XIVE_ESB_STORE_EOI:
+        if (xive_source_is_trigger_page(addr)) {
+            qemu_log_mask(LOG_GUEST_ERROR,
+                          "XIVE: invalid store on IRQ %d trigger page at "
+                          "0x%"HWADDR_PRIx"\n", srcno, addr);
+            return;
+        }
+
+        if (!(xsrc->esb_flags & XIVE_SRC_STORE_EOI)) {
+            qemu_log_mask(LOG_GUEST_ERROR,
+                          "XIVE: invalid Store EOI for IRQ %d\n", srcno);
+            return;
+        }
+
+        /* If the Q bit is set, we should forward a new source event
+         * notification
+         */
+        notify = xive_source_pq_eoi(xsrc, srcno);
+        break;
+
+    default:
+        qemu_log_mask(LOG_GUEST_ERROR, "XIVE: invalid ESB write addr %d\n",
+                      offset);
+        return;
+    }

Quoted 8 lines

hw/intc/xive.c line 197

Quoted 5 lines

hw/intc/xive.c new file +5 -0

+
+    /* Forward the source event notification for routing */
+    if (notify) {
+        xive_source_notify(xsrc, srcno);
+    }

EOI via this path calls notify, but the one via the read path doesn't. Is that correct?

No. I have given attention to the one page ESB MMIO setting + Store EOI in the emulated mode and not enough to the two pages ESB MMIO setting. This is a late change to be compatible with KVM. I will fix.

Quoted 106 lines

Quoted 106 lines

hw/intc/xive.c new file +105 -0

+}
+
+static const MemoryRegionOps xive_source_esb_ops = {
+    .read = xive_source_esb_read,
+    .write = xive_source_esb_write,
+    .endianness = DEVICE_BIG_ENDIAN,
+    .valid = {
+        .min_access_size = 8,
+        .max_access_size = 8,
+    },
+    .impl = {
+        .min_access_size = 8,
+        .max_access_size = 8,
+    },
+};
+
+static void xive_source_set_irq(void *opaque, int srcno, int val)
+{
+    XiveSource *xsrc = XIVE_SOURCE(opaque);
+    bool notify = false;
+
+    if (val) {
+        notify = xive_source_pq_trigger(xsrc, srcno);
+    }
+
+    /* Forward the source event notification for routing */
+    if (notify) {
+        xive_source_notify(xsrc, srcno);
+    }
+}
+
+void xive_source_pic_print_info(XiveSource *xsrc, Monitor *mon)
+{
+    int i;
+
+    monitor_printf(mon, "XIVE Source %6x ..%6x\n",
+                   xsrc->offset, xsrc->offset + xsrc->nr_irqs - 1);
+    for (i = 0; i < xsrc->nr_irqs; i++) {
+        uint8_t pq = xive_source_pq_get(xsrc, i);
+        uint32_t lisn = i  + xsrc->offset;
+
+        if (pq == XIVE_ESB_OFF) {
+            continue;
+        }
+
+        monitor_printf(mon, "  %4x %c%c\n", lisn,
+                       pq & XIVE_ESB_VAL_P ? 'P' : '-',
+                       pq & XIVE_ESB_VAL_Q ? 'Q' : '-');
+    }
+}
+
+static void xive_source_reset(DeviceState *dev)
+{
+    XiveSource *xsrc = XIVE_SOURCE(dev);
+
+    /* SBEs are initialized to 0b01 which corresponds to "ints off" */
+    memset(xsrc->sbe, 0x55, xsrc->sbe_size);
+}
+
+static void xive_source_realize(DeviceState *dev, Error **errp)
+{
+    XiveSource *xsrc = XIVE_SOURCE(dev);
+
+    if (!xsrc->nr_irqs) {
+        error_setg(errp, "Number of interrupt needs to be greater than 0");
+        return;
+    }
+
+    if (xsrc->esb_shift != XIVE_ESB_4K &&
+        xsrc->esb_shift != XIVE_ESB_4K_2PAGE &&
+        xsrc->esb_shift != XIVE_ESB_64K &&
+        xsrc->esb_shift != XIVE_ESB_64K_2PAGE) {
+        error_setg(errp, "Invalid ESB shift setting");
+        return;
+    }
+
+    xsrc->qirqs = qemu_allocate_irqs(xive_source_set_irq, xsrc,
+                                     xsrc->nr_irqs);
+
+    /* Allocate the SBEs (State Bit Entry). 2 bits, so 4 entries per byte */
+    xsrc->sbe_size = DIV_ROUND_UP(xsrc->nr_irqs, 4);
+    xsrc->sbe = g_malloc0(xsrc->sbe_size);
+
+    /* TODO: H_INT_ESB support, which removing the ESB MMIOs */
+
+    memory_region_init_io(&xsrc->esb_mmio, OBJECT(xsrc),
+                          &xive_source_esb_ops, xsrc, "xive.esb",
+                          (1ull << xsrc->esb_shift) * xsrc->nr_irqs);
+    sysbus_init_mmio(SYS_BUS_DEVICE(dev), &xsrc->esb_mmio);
+}
+
+static const VMStateDescription vmstate_xive_source = {
+    .name = TYPE_XIVE_SOURCE,
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32_EQUAL(nr_irqs, XiveSource, NULL),
+        VMSTATE_VBUFFER_UINT32(sbe, XiveSource, 1, NULL, sbe_size),
+        VMSTATE_END_OF_LIST()
+    },
+};
+
+/*
+ * The default XIVE interrupt source setting for ESB MMIO is two 64k
+ * pages without Store EOI. This is in sync with KVM.

Quoted 8 lines

hw/intc/xive.c line 307

Quoted 5 lines

hw/intc/xive.c new file +5 -0

+ */
+static Property xive_source_properties[] = {
+    DEFINE_PROP_UINT64("flags", XiveSource, esb_flags, 0),
+    DEFINE_PROP_UINT32("nr-irqs", XiveSource, nr_irqs, 0),
+    DEFINE_PROP_UINT64("bar", XiveSource, esb_base, 0),

Isn't this redundant with however the base address is handled through the SysBusDevice stuff (I forget the details)?

Storing the ESB MMIO base address under the XiveSource object is convenient later on in the h_int_get_source_info hcall which make use of the helpers :

hwaddr xive_source_esb_trigger(XiveSource *xsrc, uint32_t srcno) hwaddr xive_source_esb_mgmt(XiveSource *xsrc, int srcno)

But it is only used in that place. So we could just store the ESB MMIO base address under the sPAPRXive controller. This makes some sense in the design, as we have to inform KVM of this address with a KVM device ioctl.

Quoted 82 lines

Quoted 82 lines

hw/intc/xive.c new file +28 -0

+    DEFINE_PROP_UINT32("shift", XiveSource, esb_shift, XIVE_ESB_64K_2PAGE),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void xive_source_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->realize = xive_source_realize;
+    dc->reset = xive_source_reset;
+    dc->props = xive_source_properties;
+    dc->desc = "XIVE interrupt source";
+    dc->vmsd = &vmstate_xive_source;
+}
+
+static const TypeInfo xive_source_info = {
+    .name          = TYPE_XIVE_SOURCE,
+    .parent        = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(XiveSource),
+    .class_init    = xive_source_class_init,
+};
+
+static void xive_register_types(void)
+{
+    type_register_static(&xive_source_info);
+}
+
+type_init(xive_register_types)

include/hw/ppc/xive.h new file +48 -0

new file mode 100644
index 000000000000..d92a50519edf
@@ -0,0 +1,130 @@
+/*
+ * QEMU PowerPC XIVE interrupt controller model
+ *
+ * Copyright (c) 2017-2018, IBM Corporation.
+ *
+ * This code is licensed under the GPL version 2 or later. See the
+ * COPYING file in the top-level directory.
+ */
+
+#ifndef PPC_XIVE_H
+#define PPC_XIVE_H
+
+#include "hw/sysbus.h"
+
+/*
+ * XIVE Interrupt Source
+ */
+
+#define TYPE_XIVE_SOURCE "xive-source"
+#define XIVE_SOURCE(obj) OBJECT_CHECK(XiveSource, (obj), TYPE_XIVE_SOURCE)
+
+/*
+ * XIVE Source Interrupt source characteristics, which define how the
+ * ESB are controlled.
+ */
+#define XIVE_SRC_H_INT_ESB     0x1 /* ESB managed with hcall H_INT_ESB */
+#define XIVE_SRC_STORE_EOI     0x4 /* Store EOI supported */
+
+typedef struct XiveSource {
+    SysBusDevice parent;
+
+    /* IRQs */
+    uint32_t     nr_irqs;
+    uint32_t     offset;
+    qemu_irq     *qirqs;
+
+    /* PQ bits */
+    uint8_t      *sbe;
+    uint32_t     sbe_size;
+
+    /* ESB memory region */
+    uint64_t     esb_flags;
+    hwaddr       esb_base;
+    uint32_t     esb_shift;
+    MemoryRegion esb_mmio;
+} XiveSource;
+
+/*

Quoted 8 lines

include/hw/ppc/xive.h line 54

Quoted 6 lines

include/hw/ppc/xive.h new file +6 -0

+ * ESB MMIO setting. Can be one page, for both source triggering and
+ * source management, or two different pages. See below for magic
+ * values.
+ */
+#define XIVE_ESB_4K          12 /* PSI HB */
+#define XIVE_ESB_4K_2PAGE    17

Should this be 13 instead of 17?

oups. obviously this is not used.

Quoted 6 lines

include/hw/ppc/xive.h line 56

Quoted 2 lines

include/hw/ppc/xive.h new file +2 -0

+#define XIVE_ESB_64K         16
+#define XIVE_ESB_64K_2PAGE   17

(Also, who the hell comes up with a brand new PIC and decides to have *4* different interface variants. But that's not your problem, I realise).

HW constraints on the different controllers which need to expose sources : PSI, PHB4. The internal sources of the XIVE interrupt controller can be configured to use 4K or 64K but I doubt the 4k will be ever used.

Quoted 179 lines · Cédric Le Goater

Quoted 177 lines · David Gibson

Quoted 19 lines · Cédric Le Goater

Each XIVE interrupt source is associated with a two bit state machine called an Event State Buffer (ESB) : the first bit "P" means that an interrupt is "pending" and waiting for an EOI and the bit "Q" (queued) means a new interrupt was triggered while another was still pending.

When an event is triggered, the associated interrupt state bits are fetched and modified and forwarded to the virtualization engine of the controller doing the routing. These can also be controlled by MMIO, to trigger events or turn off the sources for instance. See code for more details on the states and transitions.

On a sPAPR machine, the OS will obtain the address of the MMIO page of the ESB entry associated with a source and its characteristic using the H_INT_GET_SOURCE_INFO hcall. On PowerNV, a similar OPAL call is used.

The xive_source_notify() routine is in charge forwarding the source event notification to the routing engine. It will be filled later on.

Quoted 6 lines

Signed-off-by Cédric Le Goater <[email protected]>

---
Changes since v2:

- added support for Store EOI
- added support for two page MMIO setting like on KVM

Looks generally sane to me, though I have a few queries.

Quoted 135 lines

 default-configs/ppc64-softmmu.mak |   1 +
 hw/intc/Makefile.objs             |   1 +
 hw/intc/xive.c                    | 335 ++++++++++++++++++++++++++++++++++++++
 include/hw/ppc/xive.h             | 130 +++++++++++++++
 4 files changed, 467 insertions(+)
 create mode 100644 hw/intc/xive.c
 create mode 100644 include/hw/ppc/xive.h

default-configs/ppc64-softmmu.mak +1 -0

index b94af6c7c62a..c6d13e757977 100644
@@ -16,4 +16,5 @@ CONFIG_VIRTIO_VGA=y
 CONFIG_XICS=$(CONFIG_PSERIES)
 CONFIG_XICS_SPAPR=$(CONFIG_PSERIES)
 CONFIG_XICS_KVM=$(call land,$(CONFIG_PSERIES),$(CONFIG_KVM))
+CONFIG_XIVE=$(CONFIG_PSERIES)
 CONFIG_MEM_HOTPLUG=y

hw/intc/Makefile.objs +1 -0

index 0e9963f5eecc..72a46ed91c31 100644
@@ -37,6 +37,7 @@ obj-$(CONFIG_SH4) += sh_intc.o
 obj-$(CONFIG_XICS) += xics.o
 obj-$(CONFIG_XICS_SPAPR) += xics_spapr.o
 obj-$(CONFIG_XICS_KVM) += xics_kvm.o
+obj-$(CONFIG_XIVE) += xive.o
 obj-$(CONFIG_POWERNV) += xics_pnv.o
 obj-$(CONFIG_ALLWINNER_A10_PIC) += allwinner-a10-pic.o
 obj-$(CONFIG_S390_FLIC) += s390_flic.o

hw/intc/xive.c new file +98 -0

new file mode 100644
index 000000000000..c70578759d02
@@ -0,0 +1,335 @@
+/*
+ * QEMU PowerPC XIVE interrupt controller model
+ *
+ * Copyright (c) 2017-2018, IBM Corporation.
+ *
+ * This code is licensed under the GPL version 2 or later. See the
+ * COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/log.h"
+#include "qapi/error.h"
+#include "target/ppc/cpu.h"
+#include "sysemu/cpus.h"
+#include "sysemu/dma.h"
+#include "monitor/monitor.h"
+#include "hw/ppc/xive.h"
+
+/*
+ * XIVE Interrupt Source
+ */
+
+uint8_t xive_source_pq_get(XiveSource *xsrc, uint32_t srcno)
+{
+    uint32_t byte = srcno / 4;
+    uint32_t bit  = (srcno % 4) * 2;
+
+    assert(byte < xsrc->sbe_size);
+
+    return (xsrc->sbe[byte] >> bit) & 0x3;
+}
+
+uint8_t xive_source_pq_set(XiveSource *xsrc, uint32_t srcno, uint8_t pq)
+{
+    uint32_t byte = srcno / 4;
+    uint32_t bit  = (srcno % 4) * 2;
+    uint8_t old, new;
+
+    assert(byte < xsrc->sbe_size);
+
+    old = xsrc->sbe[byte];
+
+    new = xsrc->sbe[byte] & ~(0x3 << bit);
+    new |= (pq & 0x3) << bit;
+
+    xsrc->sbe[byte] = new;
+
+    return (old >> bit) & 0x3;
+}
+
+static bool xive_source_pq_eoi(XiveSource *xsrc, uint32_t srcno)
+{
+    uint8_t old_pq = xive_source_pq_get(xsrc, srcno);
+
+    switch (old_pq) {
+    case XIVE_ESB_RESET:
+        xive_source_pq_set(xsrc, srcno, XIVE_ESB_RESET);
+        return false;
+    case XIVE_ESB_PENDING:
+        xive_source_pq_set(xsrc, srcno, XIVE_ESB_RESET);
+        return false;
+    case XIVE_ESB_QUEUED:
+        xive_source_pq_set(xsrc, srcno, XIVE_ESB_PENDING);
+        return true;
+    case XIVE_ESB_OFF:
+        xive_source_pq_set(xsrc, srcno, XIVE_ESB_OFF);
+        return false;
+    default:
+         g_assert_not_reached();
+    }
+}
+
+/*
+ * Returns whether the event notification should be forwarded.
+ */
+static bool xive_source_pq_trigger(XiveSource *xsrc, uint32_t srcno)
+{
+    uint8_t old_pq = xive_source_pq_get(xsrc, srcno);
+
+    switch (old_pq) {
+    case XIVE_ESB_RESET:
+        xive_source_pq_set(xsrc, srcno, XIVE_ESB_PENDING);
+        return true;
+    case XIVE_ESB_PENDING:
+        xive_source_pq_set(xsrc, srcno, XIVE_ESB_QUEUED);
+        return false;
+    case XIVE_ESB_QUEUED:
+        xive_source_pq_set(xsrc, srcno, XIVE_ESB_QUEUED);
+        return false;
+    case XIVE_ESB_OFF:
+        xive_source_pq_set(xsrc, srcno, XIVE_ESB_OFF);
+        return false;
+    default:
+         g_assert_not_reached();
+    }
+}
+
+/*

hw/intc/xive.c line 108

Quoted 10 lines

hw/intc/xive.c new file +10 -0

+ * Forward the source event notification to the associated XiveFabric,
+ * the device owning the sources.
+ */
+static void xive_source_notify(XiveSource *xsrc, int srcno)
+{
+
+}
+
+/* In a two pages ESB MMIO setting, even page is the trigger page, odd
+ * page is for management */

Can I understand from this that the result from this function is only meaningful in 2-pages mode?

Quoted 1 line

yes. May be I should rename it to xive_source_is_even_page() ?

Seems very long winded. Maybe keep the name but have it check both whether it's an even page and also whether you're in 2pages mode.

Quoted 8 lines

Quoted 7 lines

hw/intc/xive.c line 111

Quoted 4 lines

hw/intc/xive.c new file +3 -0

+static inline bool xive_source_is_trigger_page(hwaddr addr)
+{
+    return !((addr >> 16) & 1);

Later on you seem to have both 4k and 64k variants list, but here you hardcode 64k. Is that a problem?

Quoted 6 lines

oups. This should be :

	(addr >> (xsrc->esb_shift - 1))

I did the tests with the spapr guest which uses 64k ESB MMIO pages. The check is only significant in a 2 pages setting.

Ok.

Quoted 23 lines

Quoted 10 lines

hw/intc/xive.c line 117

Quoted 6 lines

hw/intc/xive.c new file +6 -0

+}
+
+static uint64_t xive_source_esb_read(void *opaque, hwaddr addr, unsigned size)
+{
+    XiveSource *xsrc = XIVE_SOURCE(opaque);
+    uint32_t offset = addr & 0xF00;

You ignore the low bits of the address entirely, so effective you have a 256 byte range that's all aliases of the same register. Is that intentional?

yes but it's not entirely correct. The exact ranges are :

Load Store

0x000 .. 0x3FF EOI and return 0|1 Trigger
0x400 .. 0x7FF EOI and return 0|1 EOI
0x800 .. 0xBFF return PQ undefined
0xC00 .. 0xCFF return PQ and PQ=00 PQ=00
0xD00 .. 0xDFF return PQ and PQ=01 PQ=01
0xE00 .. 0xDFF return PQ and PQ=10 PQ=10
0xF00 .. 0xDFF return PQ and PQ=11 PQ=11

Quoted 3 lines

There is room for some improvements.

The trigger page in a two pages ESB MMIO settings only triggers on stores.

Ok.

Quoted 22 lines

Quoted 13 lines

Quoted 13 lines

hw/intc/xive.c new file +12 -0

+    uint32_t srcno = addr >> xsrc->esb_shift;
+    uint64_t ret = -1;
+
+    if (xive_source_esb_2page(xsrc) && xive_source_is_trigger_page(addr)) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "XIVE: invalid load on IRQ %d trigger page at "
+                      "0x%"HWADDR_PRIx"\n", srcno, addr);
+        return -1;
+    }
+
+    switch (offset) {
+    case XIVE_ESB_LOAD_EOI:

Quoted 8 lines

hw/intc/xive.c line 134

Quoted 5 lines

hw/intc/xive.c new file +5 -0

+        /*
+         * Load EOI is not the default source setting under QEMU, but
+         * this is what HW uses currently.
+         */
+        ret = xive_source_pq_eoi(xsrc, srcno);

You're implicitly casting a bool return value into a u64 here, is that intentional?

Quoted 2 lines

yes. is that bad ? This is what the load is supposed to return in the transition algo.

No, that's fine, as long as just using the LSB in your return is correct. Just making sure I understood.

Quoted 67 lines

Quoted 58 lines

Quoted 58 lines

hw/intc/xive.c new file +58 -0

+
+        break;
+
+    case XIVE_ESB_GET:
+        ret = xive_source_pq_get(xsrc, srcno);
+        break;
+
+    case XIVE_ESB_SET_PQ_00:
+    case XIVE_ESB_SET_PQ_01:
+    case XIVE_ESB_SET_PQ_10:
+    case XIVE_ESB_SET_PQ_11:
+        ret = xive_source_pq_set(xsrc, srcno, (offset >> 8) & 0x3);
+        break;
+    default:
+        qemu_log_mask(LOG_GUEST_ERROR, "XIVE: invalid ESB addr %d\n", offset);
+    }
+
+    return ret;
+}
+
+static void xive_source_esb_write(void *opaque, hwaddr addr,
+                                 uint64_t value, unsigned size)
+{
+    XiveSource *xsrc = XIVE_SOURCE(opaque);
+    uint32_t offset = addr & 0xF00;
+    uint32_t srcno = addr >> xsrc->esb_shift;
+    bool notify = false;
+
+    switch (offset) {
+    case 0:
+        notify = xive_source_pq_trigger(xsrc, srcno);
+        break;
+
+    case XIVE_ESB_STORE_EOI:
+        if (xive_source_is_trigger_page(addr)) {
+            qemu_log_mask(LOG_GUEST_ERROR,
+                          "XIVE: invalid store on IRQ %d trigger page at "
+                          "0x%"HWADDR_PRIx"\n", srcno, addr);
+            return;
+        }
+
+        if (!(xsrc->esb_flags & XIVE_SRC_STORE_EOI)) {
+            qemu_log_mask(LOG_GUEST_ERROR,
+                          "XIVE: invalid Store EOI for IRQ %d\n", srcno);
+            return;
+        }
+
+        /* If the Q bit is set, we should forward a new source event
+         * notification
+         */
+        notify = xive_source_pq_eoi(xsrc, srcno);
+        break;
+
+    default:
+        qemu_log_mask(LOG_GUEST_ERROR, "XIVE: invalid ESB write addr %d\n",
+                      offset);
+        return;
+    }

Quoted 8 lines

hw/intc/xive.c line 197

Quoted 5 lines

hw/intc/xive.c new file +5 -0

+
+    /* Forward the source event notification for routing */
+    if (notify) {
+        xive_source_notify(xsrc, srcno);
+    }

EOI via this path calls notify, but the one via the read path doesn't. Is that correct?

Quoted 3 lines

No. I have given attention to the one page ESB MMIO setting + Store EOI in the emulated mode and not enough to the two pages ESB MMIO setting. This is a late change to be compatible with KVM. I will fix.

Ok.

Quoted 122 lines

Quoted 106 lines

Quoted 106 lines

hw/intc/xive.c new file +105 -0

+}
+
+static const MemoryRegionOps xive_source_esb_ops = {
+    .read = xive_source_esb_read,
+    .write = xive_source_esb_write,
+    .endianness = DEVICE_BIG_ENDIAN,
+    .valid = {
+        .min_access_size = 8,
+        .max_access_size = 8,
+    },
+    .impl = {
+        .min_access_size = 8,
+        .max_access_size = 8,
+    },
+};
+
+static void xive_source_set_irq(void *opaque, int srcno, int val)
+{
+    XiveSource *xsrc = XIVE_SOURCE(opaque);
+    bool notify = false;
+
+    if (val) {
+        notify = xive_source_pq_trigger(xsrc, srcno);
+    }
+
+    /* Forward the source event notification for routing */
+    if (notify) {
+        xive_source_notify(xsrc, srcno);
+    }
+}
+
+void xive_source_pic_print_info(XiveSource *xsrc, Monitor *mon)
+{
+    int i;
+
+    monitor_printf(mon, "XIVE Source %6x ..%6x\n",
+                   xsrc->offset, xsrc->offset + xsrc->nr_irqs - 1);
+    for (i = 0; i < xsrc->nr_irqs; i++) {
+        uint8_t pq = xive_source_pq_get(xsrc, i);
+        uint32_t lisn = i  + xsrc->offset;
+
+        if (pq == XIVE_ESB_OFF) {
+            continue;
+        }
+
+        monitor_printf(mon, "  %4x %c%c\n", lisn,
+                       pq & XIVE_ESB_VAL_P ? 'P' : '-',
+                       pq & XIVE_ESB_VAL_Q ? 'Q' : '-');
+    }
+}
+
+static void xive_source_reset(DeviceState *dev)
+{
+    XiveSource *xsrc = XIVE_SOURCE(dev);
+
+    /* SBEs are initialized to 0b01 which corresponds to "ints off" */
+    memset(xsrc->sbe, 0x55, xsrc->sbe_size);
+}
+
+static void xive_source_realize(DeviceState *dev, Error **errp)
+{
+    XiveSource *xsrc = XIVE_SOURCE(dev);
+
+    if (!xsrc->nr_irqs) {
+        error_setg(errp, "Number of interrupt needs to be greater than 0");
+        return;
+    }
+
+    if (xsrc->esb_shift != XIVE_ESB_4K &&
+        xsrc->esb_shift != XIVE_ESB_4K_2PAGE &&
+        xsrc->esb_shift != XIVE_ESB_64K &&
+        xsrc->esb_shift != XIVE_ESB_64K_2PAGE) {
+        error_setg(errp, "Invalid ESB shift setting");
+        return;
+    }
+
+    xsrc->qirqs = qemu_allocate_irqs(xive_source_set_irq, xsrc,
+                                     xsrc->nr_irqs);
+
+    /* Allocate the SBEs (State Bit Entry). 2 bits, so 4 entries per byte */
+    xsrc->sbe_size = DIV_ROUND_UP(xsrc->nr_irqs, 4);
+    xsrc->sbe = g_malloc0(xsrc->sbe_size);
+
+    /* TODO: H_INT_ESB support, which removing the ESB MMIOs */
+
+    memory_region_init_io(&xsrc->esb_mmio, OBJECT(xsrc),
+                          &xive_source_esb_ops, xsrc, "xive.esb",
+                          (1ull << xsrc->esb_shift) * xsrc->nr_irqs);
+    sysbus_init_mmio(SYS_BUS_DEVICE(dev), &xsrc->esb_mmio);
+}
+
+static const VMStateDescription vmstate_xive_source = {
+    .name = TYPE_XIVE_SOURCE,
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32_EQUAL(nr_irqs, XiveSource, NULL),
+        VMSTATE_VBUFFER_UINT32(sbe, XiveSource, 1, NULL, sbe_size),
+        VMSTATE_END_OF_LIST()
+    },
+};
+
+/*
+ * The default XIVE interrupt source setting for ESB MMIO is two 64k
+ * pages without Store EOI. This is in sync with KVM.

Quoted 8 lines

hw/intc/xive.c line 307

Quoted 5 lines

hw/intc/xive.c new file +5 -0

+ */
+static Property xive_source_properties[] = {
+    DEFINE_PROP_UINT64("flags", XiveSource, esb_flags, 0),
+    DEFINE_PROP_UINT32("nr-irqs", XiveSource, nr_irqs, 0),
+    DEFINE_PROP_UINT64("bar", XiveSource, esb_base, 0),

Isn't this redundant with however the base address is handled through the SysBusDevice stuff (I forget the details)?

Storing the ESB MMIO base address under the XiveSource object is convenient later on in the h_int_get_source_info hcall which make use of the helpers :

hwaddr xive_source_esb_trigger(XiveSource *xsrc, uint32_t srcno) hwaddr xive_source_esb_mgmt(XiveSource *xsrc, int srcno)

Quoted 4 lines

But it is only used in that place. So we could just store the ESB MMIO base address under the sPAPRXive controller. This makes some sense in the design, as we have to inform KVM of this address with a KVM device ioctl.

Well.. I really dislike the idea that you could change the actual MMIO mapping address in one place, but other bits of code would still think it was mapped somewhere else.

Quoted 101 lines

Quoted 83 lines

Quoted 83 lines

hw/intc/xive.c new file +28 -0

+    DEFINE_PROP_UINT32("shift", XiveSource, esb_shift, XIVE_ESB_64K_2PAGE),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void xive_source_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->realize = xive_source_realize;
+    dc->reset = xive_source_reset;
+    dc->props = xive_source_properties;
+    dc->desc = "XIVE interrupt source";
+    dc->vmsd = &vmstate_xive_source;
+}
+
+static const TypeInfo xive_source_info = {
+    .name          = TYPE_XIVE_SOURCE,
+    .parent        = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(XiveSource),
+    .class_init    = xive_source_class_init,
+};
+
+static void xive_register_types(void)
+{
+    type_register_static(&xive_source_info);
+}
+
+type_init(xive_register_types)

include/hw/ppc/xive.h new file +48 -0

new file mode 100644
index 000000000000..d92a50519edf
@@ -0,0 +1,130 @@
+/*
+ * QEMU PowerPC XIVE interrupt controller model
+ *
+ * Copyright (c) 2017-2018, IBM Corporation.
+ *
+ * This code is licensed under the GPL version 2 or later. See the
+ * COPYING file in the top-level directory.
+ */
+
+#ifndef PPC_XIVE_H
+#define PPC_XIVE_H
+
+#include "hw/sysbus.h"
+
+/*
+ * XIVE Interrupt Source
+ */
+
+#define TYPE_XIVE_SOURCE "xive-source"
+#define XIVE_SOURCE(obj) OBJECT_CHECK(XiveSource, (obj), TYPE_XIVE_SOURCE)
+
+/*
+ * XIVE Source Interrupt source characteristics, which define how the
+ * ESB are controlled.
+ */
+#define XIVE_SRC_H_INT_ESB     0x1 /* ESB managed with hcall H_INT_ESB */
+#define XIVE_SRC_STORE_EOI     0x4 /* Store EOI supported */
+
+typedef struct XiveSource {
+    SysBusDevice parent;
+
+    /* IRQs */
+    uint32_t     nr_irqs;
+    uint32_t     offset;
+    qemu_irq     *qirqs;
+
+    /* PQ bits */
+    uint8_t      *sbe;
+    uint32_t     sbe_size;
+
+    /* ESB memory region */
+    uint64_t     esb_flags;
+    hwaddr       esb_base;
+    uint32_t     esb_shift;
+    MemoryRegion esb_mmio;
+} XiveSource;
+
+/*

Quoted 8 lines

include/hw/ppc/xive.h line 54

Quoted 6 lines

include/hw/ppc/xive.h new file +6 -0

+ * ESB MMIO setting. Can be one page, for both source triggering and
+ * source management, or two different pages. See below for magic
+ * values.
+ */
+#define XIVE_ESB_4K          12 /* PSI HB */
+#define XIVE_ESB_4K_2PAGE    17

Should this be 13 instead of 17?

oups. obviously this is not used.

Quoted 6 lines

include/hw/ppc/xive.h line 56

Quoted 2 lines

include/hw/ppc/xive.h new file +2 -0

+#define XIVE_ESB_64K         16
+#define XIVE_ESB_64K_2PAGE   17

(Also, who the hell comes up with a brand new PIC and decides to have *4* different interface variants. But that's not your problem, I realise).

Quoted 4 lines

HW constraints on the different controllers which need to expose sources : PSI, PHB4. The internal sources of the XIVE interrupt controller can be configured to use 4K or 64K but I doubt the 4k will be ever used.

Sure, the hardware is different, but *why* is it different. This is a brand new design, you'd think they could come up with one variant that works for all the cases.

Quoted 30 lines · David Gibson

Quoted 29 lines · Cédric Le Goater

Quoted 28 lines · David Gibson

Quoted 19 lines · Cédric Le Goater

Each XIVE interrupt source is associated with a two bit state machine called an Event State Buffer (ESB) : the first bit "P" means that an interrupt is "pending" and waiting for an EOI and the bit "Q" (queued) means a new interrupt was triggered while another was still pending.

When an event is triggered, the associated interrupt state bits are fetched and modified and forwarded to the virtualization engine of the controller doing the routing. These can also be controlled by MMIO, to trigger events or turn off the sources for instance. See code for more details on the states and transitions.

On a sPAPR machine, the OS will obtain the address of the MMIO page of the ESB entry associated with a source and its characteristic using the H_INT_GET_SOURCE_INFO hcall. On PowerNV, a similar OPAL call is used.

The xive_source_notify() routine is in charge forwarding the source event notification to the routing engine. It will be filled later on.

Quoted 6 lines

Signed-off-by Cédric Le Goater <[email protected]>

---
Changes since v2:

- added support for Store EOI
- added support for two page MMIO setting like on KVM

Looks generally sane to me, though I have a few queries.

Quoted 18 lines

Quoted 18 lines

Quoted 10 lines

hw/intc/xive.c line 307

Quoted 7 lines

+ * The default XIVE interrupt source setting for ESB MMIO is two 64k
+ * pages without Store EOI. This is in sync with KVM.
+ */

hw/intc/xive.c new file +4 -0

+static Property xive_source_properties[] = {
+    DEFINE_PROP_UINT64("flags", XiveSource, esb_flags, 0),
+    DEFINE_PROP_UINT32("nr-irqs", XiveSource, nr_irqs, 0),
+    DEFINE_PROP_UINT64("bar", XiveSource, esb_base, 0),

Isn't this redundant with however the base address is handled through the SysBusDevice stuff (I forget the details)?

Storing the ESB MMIO base address under the XiveSource object is convenient later on in the h_int_get_source_info hcall which make use of the helpers :

hwaddr xive_source_esb_trigger(XiveSource *xsrc, uint32_t srcno) hwaddr xive_source_esb_mgmt(XiveSource *xsrc, int srcno)

Quoted 8 lines

Quoted 4 lines

But it is only used in that place. So we could just store the ESB MMIO base address under the sPAPRXive controller. This makes some sense in the design, as we have to inform KVM of this address with a KVM device ioctl.

Well.. I really dislike the idea that you could change the actual MMIO mapping address in one place, but other bits of code would still think it was mapped somewhere else.

OK. I think that my last proposal of removing the ESB MMIO address from the source and letting the owning device, the sPAPR Xive controller in this case, but this is the same for PoweNV or PSI HB, handle the mapping goes in the direction you want to take ?

It does looks better in the overall XIVE model and the XiveSource object have no need of this address.

The 'sent' status of the LSI interrupt source is modeled with the 'P' bit of the ESB and the assertion status of the source is maintained in an array under the main sPAPRXive object. The type of the source is stored in the same array for practical reasons.

Signed-off-by Cédric Le Goater <[email protected]>

---
 hw/intc/xive.c        | 54 +++++++++++++++++++++++++++++++++++++++++++++++----
 include/hw/ppc/xive.h | 16 +++++++++++++++
 2 files changed, 66 insertions(+), 4 deletions(-)

hw/intc/xive.c +50 -4

index c70578759d02..060976077dd7 100644
@@ -104,6 +104,21 @@ static void xive_source_notify(XiveSource *xsrc, int srcno)
 
 }
 
+/*
+ * LSI interrupt sources use the P bit and a custom assertion flag
+ */
+static bool xive_source_lsi_trigger(XiveSource *xsrc, uint32_t srcno)
+{
+    uint8_t old_pq = xive_source_pq_get(xsrc, srcno);
+
+    if  (old_pq == XIVE_ESB_RESET &&
+         xsrc->status[srcno] & XIVE_STATUS_ASSERTED) {
+        xive_source_pq_set(xsrc, srcno, XIVE_ESB_PENDING);
+        return true;
+    }
+    return false;
+}
+
 /* In a two pages ESB MMIO setting, even page is the trigger page, odd
  * page is for management */
 static inline bool xive_source_is_trigger_page(hwaddr addr)

@@ -133,6 +148,13 @@ static uint64_t xive_source_esb_read(void *opaque, hwaddr addr, unsigned size)
          */
         ret = xive_source_pq_eoi(xsrc, srcno);
 
+        /* If the LSI source is still asserted, forward a new source
+         * event notification */
+        if (xive_source_irq_is_lsi(xsrc, srcno)) {
+            if (xive_source_lsi_trigger(xsrc, srcno)) {
+                xive_source_notify(xsrc, srcno);
+            }
+        }
         break;
 
     case XIVE_ESB_GET:

@@ -183,6 +205,14 @@ static void xive_source_esb_write(void *opaque, hwaddr addr,
          * notification
          */
         notify = xive_source_pq_eoi(xsrc, srcno);
+
+        /* LSI sources do not set the Q bit but they can still be
+         * asserted, in which case we should forward a new source
+         * event notification
+         */
+        if (xive_source_irq_is_lsi(xsrc, srcno)) {
+            notify = xive_source_lsi_trigger(xsrc, srcno);
+        }
         break;
 
     default:

@@ -216,8 +246,17 @@ static void xive_source_set_irq(void *opaque, int srcno, int val)
     XiveSource *xsrc = XIVE_SOURCE(opaque);
     bool notify = false;
 
-    if (val) {
-        notify = xive_source_pq_trigger(xsrc, srcno);
+    if (xive_source_irq_is_lsi(xsrc, srcno)) {
+        if (val) {
+            xsrc->status[srcno] |= XIVE_STATUS_ASSERTED;
+        } else {
+            xsrc->status[srcno] &= ~XIVE_STATUS_ASSERTED;
+        }
+        notify = xive_source_lsi_trigger(xsrc, srcno);
+    } else {
+        if (val) {
+            notify = xive_source_pq_trigger(xsrc, srcno);
+        }
     }
 
     /* Forward the source event notification for routing */

@@ -234,13 +273,13 @@ void xive_source_pic_print_info(XiveSource *xsrc, Monitor *mon)
                    xsrc->offset, xsrc->offset + xsrc->nr_irqs - 1);
     for (i = 0; i < xsrc->nr_irqs; i++) {
         uint8_t pq = xive_source_pq_get(xsrc, i);
-        uint32_t lisn = i  + xsrc->offset;
 
         if (pq == XIVE_ESB_OFF) {
             continue;
         }
 
-        monitor_printf(mon, "  %4x %c%c\n", lisn,
+        monitor_printf(mon, "  %4x %s %c%c\n", i + xsrc->offset,
+                       xive_source_irq_is_lsi(xsrc, i) ? "LSI" : "MSI",
                        pq & XIVE_ESB_VAL_P ? 'P' : '-',
                        pq & XIVE_ESB_VAL_Q ? 'Q' : '-');
     }

@@ -249,6 +288,12 @@ void xive_source_pic_print_info(XiveSource *xsrc, Monitor *mon)
 static void xive_source_reset(DeviceState *dev)
 {
     XiveSource *xsrc = XIVE_SOURCE(dev);
+    int i;
+
+    /* Keep the IRQ type */
+    for (i = 0; i < xsrc->nr_irqs; i++) {
+        xsrc->status[i] &= ~XIVE_STATUS_ASSERTED;
+    }
 
     /* SBEs are initialized to 0b01 which corresponds to "ints off" */
     memset(xsrc->sbe, 0x55, xsrc->sbe_size);

@@ -273,6 +318,7 @@ static void xive_source_realize(DeviceState *dev, Error **errp)
 
     xsrc->qirqs = qemu_allocate_irqs(xive_source_set_irq, xsrc,
                                      xsrc->nr_irqs);
+    xsrc->status = g_malloc0(xsrc->nr_irqs);
 
     /* Allocate the SBEs (State Bit Entry). 2 bits, so 4 entries per byte */
     xsrc->sbe_size = DIV_ROUND_UP(xsrc->nr_irqs, 4);

include/hw/ppc/xive.h +0 -0

index d92a50519edf..0b76dd278d9b 100644

include/hw/ppc/xive.h +13 -0

@@ -127,4 +130,17 @@ uint8_t xive_source_pq_set(XiveSource *xsrc, uint32_t srcno, uint8_t pq);
 
 void xive_source_pic_print_info(XiveSource *xsrc, Monitor *mon);
 
+static inline bool xive_source_irq_is_lsi(XiveSource *xsrc, uint32_t srcno)
+{
+    assert(srcno < xsrc->nr_irqs);
+    return xsrc->status[srcno] & XIVE_STATUS_LSI;
+}
+
+static inline void xive_source_irq_set(XiveSource *xsrc, uint32_t srcno,
+                                       bool lsi)
+{
+    assert(srcno < xsrc->nr_irqs);
+    xsrc->status[srcno] |= lsi ? XIVE_STATUS_LSI : 0;
+}
+
 #endif /* PPC_XIVE_H */

Quoted 50 lines · Cédric Le Goater

The 'sent' status of the LSI interrupt source is modeled with the 'P' bit of the ESB and the assertion status of the source is maintained in an array under the main sPAPRXive object. The type of the source is stored in the same array for practical reasons.

Signed-off-by Cédric Le Goater <[email protected]>

---
 hw/intc/xive.c        | 54 +++++++++++++++++++++++++++++++++++++++++++++++----
 include/hw/ppc/xive.h | 16 +++++++++++++++
 2 files changed, 66 insertions(+), 4 deletions(-)

hw/intc/xive.c +22 -0

index c70578759d02..060976077dd7 100644
@@ -104,6 +104,21 @@ static void xive_source_notify(XiveSource *xsrc, int srcno)
 
 }
 
+/*
+ * LSI interrupt sources use the P bit and a custom assertion flag
+ */
+static bool xive_source_lsi_trigger(XiveSource *xsrc, uint32_t srcno)
+{
+    uint8_t old_pq = xive_source_pq_get(xsrc, srcno);
+
+    if  (old_pq == XIVE_ESB_RESET &&
+         xsrc->status[srcno] & XIVE_STATUS_ASSERTED) {
+        xive_source_pq_set(xsrc, srcno, XIVE_ESB_PENDING);
+        return true;
+    }
+    return false;
+}
+
 /* In a two pages ESB MMIO setting, even page is the trigger page, odd
  * page is for management */
 static inline bool xive_source_is_trigger_page(hwaddr addr)

@@ -133,6 +148,13 @@ static uint64_t xive_source_esb_read(void *opaque, hwaddr addr, unsigned size)
          */
         ret = xive_source_pq_eoi(xsrc, srcno);
 
+        /* If the LSI source is still asserted, forward a new source
+         * event notification */
+        if (xive_source_irq_is_lsi(xsrc, srcno)) {
+            if (xive_source_lsi_trigger(xsrc, srcno)) {
+                xive_source_notify(xsrc, srcno);
+            }
+        }
         break;
 

hw/intc/xive.c static void xive_source_esb_write(void *opaque, hwaddr addr, · line 215

Quoted 13 lines

hw/intc/xive.c +8 -0

     case XIVE_ESB_GET:

@@ -183,6 +205,14 @@ static void xive_source_esb_write(void *opaque, hwaddr addr,
          * notification
          */
         notify = xive_source_pq_eoi(xsrc, srcno);
+
+        /* LSI sources do not set the Q bit but they can still be
+         * asserted, in which case we should forward a new source
+         * event notification
+         */
+        if (xive_source_irq_is_lsi(xsrc, srcno)) {
+            notify = xive_source_lsi_trigger(xsrc, srcno);
+        }

Quoted 67 lines

hw/intc/xive.c +20 -4

         break;
 
     default:

@@ -216,8 +246,17 @@ static void xive_source_set_irq(void *opaque, int srcno, int val)
     XiveSource *xsrc = XIVE_SOURCE(opaque);
     bool notify = false;
 
-    if (val) {
-        notify = xive_source_pq_trigger(xsrc, srcno);
+    if (xive_source_irq_is_lsi(xsrc, srcno)) {
+        if (val) {
+            xsrc->status[srcno] |= XIVE_STATUS_ASSERTED;
+        } else {
+            xsrc->status[srcno] &= ~XIVE_STATUS_ASSERTED;
+        }
+        notify = xive_source_lsi_trigger(xsrc, srcno);
+    } else {
+        if (val) {
+            notify = xive_source_pq_trigger(xsrc, srcno);
+        }
     }
 
     /* Forward the source event notification for routing */

@@ -234,13 +273,13 @@ void xive_source_pic_print_info(XiveSource *xsrc, Monitor *mon)
                    xsrc->offset, xsrc->offset + xsrc->nr_irqs - 1);
     for (i = 0; i < xsrc->nr_irqs; i++) {
         uint8_t pq = xive_source_pq_get(xsrc, i);
-        uint32_t lisn = i  + xsrc->offset;
 
         if (pq == XIVE_ESB_OFF) {
             continue;
         }
 
-        monitor_printf(mon, "  %4x %c%c\n", lisn,
+        monitor_printf(mon, "  %4x %s %c%c\n", i + xsrc->offset,
+                       xive_source_irq_is_lsi(xsrc, i) ? "LSI" : "MSI",
                        pq & XIVE_ESB_VAL_P ? 'P' : '-',
                        pq & XIVE_ESB_VAL_Q ? 'Q' : '-');
     }

@@ -249,6 +288,12 @@ void xive_source_pic_print_info(XiveSource *xsrc, Monitor *mon)
 static void xive_source_reset(DeviceState *dev)
 {
     XiveSource *xsrc = XIVE_SOURCE(dev);
+    int i;
+
+    /* Keep the IRQ type */
+    for (i = 0; i < xsrc->nr_irqs; i++) {
+        xsrc->status[i] &= ~XIVE_STATUS_ASSERTED;
+    }
 
     /* SBEs are initialized to 0b01 which corresponds to "ints off" */
     memset(xsrc->sbe, 0x55, xsrc->sbe_size);

@@ -273,6 +318,7 @@ static void xive_source_realize(DeviceState *dev, Error **errp)
 
     xsrc->qirqs = qemu_allocate_irqs(xive_source_set_irq, xsrc,
                                      xsrc->nr_irqs);
+    xsrc->status = g_malloc0(xsrc->nr_irqs);
 
     /* Allocate the SBEs (State Bit Entry). 2 bits, so 4 entries per byte */
     xsrc->sbe_size = DIV_ROUND_UP(xsrc->nr_irqs, 4);

include/hw/ppc/xive.h +0 -0

index d92a50519edf..0b76dd278d9b 100644
@@ -33,6 +33,9 @@ typedef struct XiveSource {
     uint32_t     nr_irqs;
     uint32_t     offset;

include/hw/ppc/xive.h typedef struct XiveSource { · line 38

Quoted 4 lines

include/hw/ppc/xive.h +3 -0

     qemu_irq     *qirqs;
+#define XIVE_STATUS_LSI         0x1
+#define XIVE_STATUS_ASSERTED    0x2
+    uint8_t      *status;

I don't love the idea of mixing configuration information (STATUS_LSI) with runtime state information (ASSERTED) in the same field. Any reason not to have these as parallel bitmaps.

Come to that.. is there a compelling reason to allow any individual irq to be marked LSI or MSI, rather than using separate XiveSource objects for MSIs and LSIs?

include/hw/ppc/xive.h typedef struct XiveSource { · line 41

Quoted 2 lines

include/hw/ppc/xive.h +0 -0

     /* PQ bits */
     uint8_t      *sbe;

.. and come to that is there a reason to keep the ASSERTED bit in a separate array from sbe? AFAICT the actual 2-bit-per-irq layout is never exposed to the guests.

Or, even re-use the Q bit for asserted in LSIs (but report it as always 0 in the register read/write path).

Quoted 18 lines

include/hw/ppc/xive.h +13 -0

@@ -127,4 +130,17 @@ uint8_t xive_source_pq_set(XiveSource *xsrc, uint32_t srcno, uint8_t pq);
 
 void xive_source_pic_print_info(XiveSource *xsrc, Monitor *mon);
 
+static inline bool xive_source_irq_is_lsi(XiveSource *xsrc, uint32_t srcno)
+{
+    assert(srcno < xsrc->nr_irqs);
+    return xsrc->status[srcno] & XIVE_STATUS_LSI;
+}
+
+static inline void xive_source_irq_set(XiveSource *xsrc, uint32_t srcno,
+                                       bool lsi)
+{
+    assert(srcno < xsrc->nr_irqs);
+    xsrc->status[srcno] |= lsi ? XIVE_STATUS_LSI : 0;
+}
+
 #endif /* PPC_XIVE_H */

Quoted 51 lines · David Gibson

Quoted 50 lines · Cédric Le Goater

The 'sent' status of the LSI interrupt source is modeled with the 'P' bit of the ESB and the assertion status of the source is maintained in an array under the main sPAPRXive object. The type of the source is stored in the same array for practical reasons.

Signed-off-by Cédric Le Goater <[email protected]>

---
 hw/intc/xive.c        | 54 +++++++++++++++++++++++++++++++++++++++++++++++----
 include/hw/ppc/xive.h | 16 +++++++++++++++
 2 files changed, 66 insertions(+), 4 deletions(-)

hw/intc/xive.c +22 -0

index c70578759d02..060976077dd7 100644
@@ -104,6 +104,21 @@ static void xive_source_notify(XiveSource *xsrc, int srcno)
 
 }
 
+/*
+ * LSI interrupt sources use the P bit and a custom assertion flag
+ */
+static bool xive_source_lsi_trigger(XiveSource *xsrc, uint32_t srcno)
+{
+    uint8_t old_pq = xive_source_pq_get(xsrc, srcno);
+
+    if  (old_pq == XIVE_ESB_RESET &&
+         xsrc->status[srcno] & XIVE_STATUS_ASSERTED) {
+        xive_source_pq_set(xsrc, srcno, XIVE_ESB_PENDING);
+        return true;
+    }
+    return false;
+}
+
 /* In a two pages ESB MMIO setting, even page is the trigger page, odd
  * page is for management */
 static inline bool xive_source_is_trigger_page(hwaddr addr)

@@ -133,6 +148,13 @@ static uint64_t xive_source_esb_read(void *opaque, hwaddr addr, unsigned size)
          */
         ret = xive_source_pq_eoi(xsrc, srcno);
 
+        /* If the LSI source is still asserted, forward a new source
+         * event notification */
+        if (xive_source_irq_is_lsi(xsrc, srcno)) {
+            if (xive_source_lsi_trigger(xsrc, srcno)) {
+                xive_source_notify(xsrc, srcno);
+            }
+        }
         break;
 

Quoted 5 lines

Quoted 5 lines

hw/intc/xive.c +0 -0

     case XIVE_ESB_GET:

@@ -183,6 +205,14 @@ static void xive_source_esb_write(void *opaque, hwaddr addr,
          * notification
          */
         notify = xive_source_pq_eoi(xsrc, srcno);

Quoted 8 lines

hw/intc/xive.c static void xive_source_esb_write(void *opaque, hwaddr addr, · line 215

Quoted 8 lines

hw/intc/xive.c +8 -0

+
+        /* LSI sources do not set the Q bit but they can still be
+         * asserted, in which case we should forward a new source
+         * event notification
+         */
+        if (xive_source_irq_is_lsi(xsrc, srcno)) {
+            notify = xive_source_lsi_trigger(xsrc, srcno);
+        }

FYI, I have moved that common test under xive_source_pq_eoi()

Quoted 67 lines

Quoted 67 lines

hw/intc/xive.c +20 -4

         break;
 
     default:

@@ -216,8 +246,17 @@ static void xive_source_set_irq(void *opaque, int srcno, int val)
     XiveSource *xsrc = XIVE_SOURCE(opaque);
     bool notify = false;
 
-    if (val) {
-        notify = xive_source_pq_trigger(xsrc, srcno);
+    if (xive_source_irq_is_lsi(xsrc, srcno)) {
+        if (val) {
+            xsrc->status[srcno] |= XIVE_STATUS_ASSERTED;
+        } else {
+            xsrc->status[srcno] &= ~XIVE_STATUS_ASSERTED;
+        }
+        notify = xive_source_lsi_trigger(xsrc, srcno);
+    } else {
+        if (val) {
+            notify = xive_source_pq_trigger(xsrc, srcno);
+        }
     }
 
     /* Forward the source event notification for routing */

@@ -234,13 +273,13 @@ void xive_source_pic_print_info(XiveSource *xsrc, Monitor *mon)
                    xsrc->offset, xsrc->offset + xsrc->nr_irqs - 1);
     for (i = 0; i < xsrc->nr_irqs; i++) {
         uint8_t pq = xive_source_pq_get(xsrc, i);
-        uint32_t lisn = i  + xsrc->offset;
 
         if (pq == XIVE_ESB_OFF) {
             continue;
         }
 
-        monitor_printf(mon, "  %4x %c%c\n", lisn,
+        monitor_printf(mon, "  %4x %s %c%c\n", i + xsrc->offset,
+                       xive_source_irq_is_lsi(xsrc, i) ? "LSI" : "MSI",
                        pq & XIVE_ESB_VAL_P ? 'P' : '-',
                        pq & XIVE_ESB_VAL_Q ? 'Q' : '-');
     }

@@ -249,6 +288,12 @@ void xive_source_pic_print_info(XiveSource *xsrc, Monitor *mon)
 static void xive_source_reset(DeviceState *dev)
 {
     XiveSource *xsrc = XIVE_SOURCE(dev);
+    int i;
+
+    /* Keep the IRQ type */
+    for (i = 0; i < xsrc->nr_irqs; i++) {
+        xsrc->status[i] &= ~XIVE_STATUS_ASSERTED;
+    }
 
     /* SBEs are initialized to 0b01 which corresponds to "ints off" */
     memset(xsrc->sbe, 0x55, xsrc->sbe_size);

@@ -273,6 +318,7 @@ static void xive_source_realize(DeviceState *dev, Error **errp)
 
     xsrc->qirqs = qemu_allocate_irqs(xive_source_set_irq, xsrc,
                                      xsrc->nr_irqs);
+    xsrc->status = g_malloc0(xsrc->nr_irqs);
 
     /* Allocate the SBEs (State Bit Entry). 2 bits, so 4 entries per byte */
     xsrc->sbe_size = DIV_ROUND_UP(xsrc->nr_irqs, 4);

include/hw/ppc/xive.h +0 -0

index d92a50519edf..0b76dd278d9b 100644
@@ -33,6 +33,9 @@ typedef struct XiveSource {
     uint32_t     nr_irqs;
     uint32_t     offset;

Quoted 8 lines

include/hw/ppc/xive.h typedef struct XiveSource { · line 38

Quoted 4 lines

include/hw/ppc/xive.h +3 -0

     qemu_irq     *qirqs;
+#define XIVE_STATUS_LSI         0x1
+#define XIVE_STATUS_ASSERTED    0x2
+    uint8_t      *status;

I don't love the idea of mixing configuration information (STATUS_LSI) with runtime state information (ASSERTED) in the same field. Any reason not to have these as parallel bitmaps.

none. I can change that.

Quoted 3 lines

Come to that.. is there a compelling reason to allow any individual irq to be marked LSI or MSI, rather than using separate XiveSource objects for MSIs and LSIs?

yes. I would have preferred two distinct interrupt source objects but this is to be compatible with XICS, which uses only one. If we want to be able to change interrupt mode, the IRQ number space should be organized in the exact same way. Or we should change XICS also.

Also, the change (a bitmap) is really small.

Quoted 6 lines

include/hw/ppc/xive.h typedef struct XiveSource { · line 41

Quoted 2 lines

include/hw/ppc/xive.h +0 -0

     /* PQ bits */
     uint8_t      *sbe;

.. and come to that is there a reason to keep the ASSERTED bit in a separate array from sbe? AFAICT the actual 2-bit-per-irq layout is never exposed to the guests.

indeed. we always use the xive_source_pq_get/set() helpers to manipulate the PQ bits. So we could add an extra bit for the ASSERT without too much changes. Could also we put the type there or would you still prefer a bitmap ?

Quoted 2 lines

Or, even re-use the Q bit for asserted in LSIs (but report it as always 0 in the register read/write path).

I would prefer to add extra status bits. It is easier to debug.

Thanks,

C.

Quoted 19 lines

Quoted 18 lines

include/hw/ppc/xive.h +13 -0

@@ -127,4 +130,17 @@ uint8_t xive_source_pq_set(XiveSource *xsrc, uint32_t srcno, uint8_t pq);
 
 void xive_source_pic_print_info(XiveSource *xsrc, Monitor *mon);
 
+static inline bool xive_source_irq_is_lsi(XiveSource *xsrc, uint32_t srcno)
+{
+    assert(srcno < xsrc->nr_irqs);
+    return xsrc->status[srcno] & XIVE_STATUS_LSI;
+}
+
+static inline void xive_source_irq_set(XiveSource *xsrc, uint32_t srcno,
+                                       bool lsi)
+{
+    assert(srcno < xsrc->nr_irqs);
+    xsrc->status[srcno] |= lsi ? XIVE_STATUS_LSI : 0;
+}
+
 #endif /* PPC_XIVE_H */

Quoted 52 lines · Cédric Le Goater

Quoted 51 lines · David Gibson

Quoted 50 lines · Cédric Le Goater

The 'sent' status of the LSI interrupt source is modeled with the 'P' bit of the ESB and the assertion status of the source is maintained in an array under the main sPAPRXive object. The type of the source is stored in the same array for practical reasons.

Signed-off-by Cédric Le Goater <[email protected]>

---
 hw/intc/xive.c        | 54 +++++++++++++++++++++++++++++++++++++++++++++++----
 include/hw/ppc/xive.h | 16 +++++++++++++++
 2 files changed, 66 insertions(+), 4 deletions(-)

hw/intc/xive.c +22 -0

index c70578759d02..060976077dd7 100644
@@ -104,6 +104,21 @@ static void xive_source_notify(XiveSource *xsrc, int srcno)
 
 }
 
+/*
+ * LSI interrupt sources use the P bit and a custom assertion flag
+ */
+static bool xive_source_lsi_trigger(XiveSource *xsrc, uint32_t srcno)
+{
+    uint8_t old_pq = xive_source_pq_get(xsrc, srcno);
+
+    if  (old_pq == XIVE_ESB_RESET &&
+         xsrc->status[srcno] & XIVE_STATUS_ASSERTED) {
+        xive_source_pq_set(xsrc, srcno, XIVE_ESB_PENDING);
+        return true;
+    }
+    return false;
+}
+
 /* In a two pages ESB MMIO setting, even page is the trigger page, odd
  * page is for management */
 static inline bool xive_source_is_trigger_page(hwaddr addr)

@@ -133,6 +148,13 @@ static uint64_t xive_source_esb_read(void *opaque, hwaddr addr, unsigned size)
          */
         ret = xive_source_pq_eoi(xsrc, srcno);
 
+        /* If the LSI source is still asserted, forward a new source
+         * event notification */
+        if (xive_source_irq_is_lsi(xsrc, srcno)) {
+            if (xive_source_lsi_trigger(xsrc, srcno)) {
+                xive_source_notify(xsrc, srcno);
+            }
+        }
         break;
 

Quoted 15 lines

Quoted 5 lines

Quoted 5 lines

hw/intc/xive.c +0 -0

     case XIVE_ESB_GET:

@@ -183,6 +205,14 @@ static void xive_source_esb_write(void *opaque, hwaddr addr,
          * notification
          */
         notify = xive_source_pq_eoi(xsrc, srcno);

Quoted 8 lines

hw/intc/xive.c static void xive_source_esb_write(void *opaque, hwaddr addr, · line 215

Quoted 8 lines

hw/intc/xive.c +8 -0

+
+        /* LSI sources do not set the Q bit but they can still be
+         * asserted, in which case we should forward a new source
+         * event notification
+         */
+        if (xive_source_irq_is_lsi(xsrc, srcno)) {
+            notify = xive_source_lsi_trigger(xsrc, srcno);
+        }

FYI, I have moved that common test under xive_source_pq_eoi()

Ok.

Quoted 76 lines

Quoted 67 lines

Quoted 67 lines

hw/intc/xive.c +20 -4

         break;
 
     default:

@@ -216,8 +246,17 @@ static void xive_source_set_irq(void *opaque, int srcno, int val)
     XiveSource *xsrc = XIVE_SOURCE(opaque);
     bool notify = false;
 
-    if (val) {
-        notify = xive_source_pq_trigger(xsrc, srcno);
+    if (xive_source_irq_is_lsi(xsrc, srcno)) {
+        if (val) {
+            xsrc->status[srcno] |= XIVE_STATUS_ASSERTED;
+        } else {
+            xsrc->status[srcno] &= ~XIVE_STATUS_ASSERTED;
+        }
+        notify = xive_source_lsi_trigger(xsrc, srcno);
+    } else {
+        if (val) {
+            notify = xive_source_pq_trigger(xsrc, srcno);
+        }
     }
 
     /* Forward the source event notification for routing */

@@ -234,13 +273,13 @@ void xive_source_pic_print_info(XiveSource *xsrc, Monitor *mon)
                    xsrc->offset, xsrc->offset + xsrc->nr_irqs - 1);
     for (i = 0; i < xsrc->nr_irqs; i++) {
         uint8_t pq = xive_source_pq_get(xsrc, i);
-        uint32_t lisn = i  + xsrc->offset;
 
         if (pq == XIVE_ESB_OFF) {
             continue;
         }
 
-        monitor_printf(mon, "  %4x %c%c\n", lisn,
+        monitor_printf(mon, "  %4x %s %c%c\n", i + xsrc->offset,
+                       xive_source_irq_is_lsi(xsrc, i) ? "LSI" : "MSI",
                        pq & XIVE_ESB_VAL_P ? 'P' : '-',
                        pq & XIVE_ESB_VAL_Q ? 'Q' : '-');
     }

@@ -249,6 +288,12 @@ void xive_source_pic_print_info(XiveSource *xsrc, Monitor *mon)
 static void xive_source_reset(DeviceState *dev)
 {
     XiveSource *xsrc = XIVE_SOURCE(dev);
+    int i;
+
+    /* Keep the IRQ type */
+    for (i = 0; i < xsrc->nr_irqs; i++) {
+        xsrc->status[i] &= ~XIVE_STATUS_ASSERTED;
+    }
 
     /* SBEs are initialized to 0b01 which corresponds to "ints off" */
     memset(xsrc->sbe, 0x55, xsrc->sbe_size);

@@ -273,6 +318,7 @@ static void xive_source_realize(DeviceState *dev, Error **errp)
 
     xsrc->qirqs = qemu_allocate_irqs(xive_source_set_irq, xsrc,
                                      xsrc->nr_irqs);
+    xsrc->status = g_malloc0(xsrc->nr_irqs);
 
     /* Allocate the SBEs (State Bit Entry). 2 bits, so 4 entries per byte */
     xsrc->sbe_size = DIV_ROUND_UP(xsrc->nr_irqs, 4);

include/hw/ppc/xive.h +0 -0

index d92a50519edf..0b76dd278d9b 100644
@@ -33,6 +33,9 @@ typedef struct XiveSource {
     uint32_t     nr_irqs;
     uint32_t     offset;

Quoted 8 lines

include/hw/ppc/xive.h typedef struct XiveSource { · line 38

Quoted 4 lines

include/hw/ppc/xive.h +3 -0

     qemu_irq     *qirqs;
+#define XIVE_STATUS_LSI         0x1
+#define XIVE_STATUS_ASSERTED    0x2
+    uint8_t      *status;

I don't love the idea of mixing configuration information (STATUS_LSI) with runtime state information (ASSERTED) in the same field. Any reason not to have these as parallel bitmaps.

Quoted 1 line

none. I can change that.

Ok.

Quoted 9 lines

Quoted 3 lines

Come to that.. is there a compelling reason to allow any individual irq to be marked LSI or MSI, rather than using separate XiveSource objects for MSIs and LSIs?

yes. I would have preferred two distinct interrupt source objects but this is to be compatible with XICS, which uses only one. If we want to be able to change interrupt mode, the IRQ number space should be organized in the exact same way. Or we should change XICS also.

Quoted 1 line

Also, the change (a bitmap) is really small.

Hrm, but since XIVE supports thousands of irqs, it could be quite a large bitmap.

It's not impossible - in fact, not really even that hard - to change the existing irq layout on xics. It does need a new machine type variant, of course.

Quoted 7 lines

Quoted 6 lines

include/hw/ppc/xive.h typedef struct XiveSource { · line 41

Quoted 2 lines

include/hw/ppc/xive.h +0 -0

     /* PQ bits */
     uint8_t      *sbe;

.. and come to that is there a reason to keep the ASSERTED bit in a separate array from sbe? AFAICT the actual 2-bit-per-irq layout is never exposed to the guests.

Quoted 4 lines

indeed. we always use the xive_source_pq_get/set() helpers to manipulate the PQ bits. So we could add an extra bit for the ASSERT without too much changes. Could also we put the type there or would you still prefer a bitmap ?

I'd prefer the type (config information) be separate from the P, Q, ASSERTED bits (state information).

Quoted 29 lines

Quoted 2 lines

Or, even re-use the Q bit for asserted in LSIs (but report it as always 0 in the register read/write path).

I would prefer to add extra status bits. It is easier to debug.

Thanks,

C.

Quoted 19 lines

Quoted 18 lines

include/hw/ppc/xive.h +13 -0

@@ -127,4 +130,17 @@ uint8_t xive_source_pq_set(XiveSource *xsrc, uint32_t srcno, uint8_t pq);
 
 void xive_source_pic_print_info(XiveSource *xsrc, Monitor *mon);
 
+static inline bool xive_source_irq_is_lsi(XiveSource *xsrc, uint32_t srcno)
+{
+    assert(srcno < xsrc->nr_irqs);
+    return xsrc->status[srcno] & XIVE_STATUS_LSI;
+}
+
+static inline void xive_source_irq_set(XiveSource *xsrc, uint32_t srcno,
+                                       bool lsi)
+{
+    assert(srcno < xsrc->nr_irqs);
+    xsrc->status[srcno] |= lsi ? XIVE_STATUS_LSI : 0;
+}
+
 #endif /* PPC_XIVE_H */

Quoted 160 lines · David Gibson

Quoted 52 lines · Cédric Le Goater

Quoted 51 lines · David Gibson

Quoted 50 lines · Cédric Le Goater

The 'sent' status of the LSI interrupt source is modeled with the 'P' bit of the ESB and the assertion status of the source is maintained in an array under the main sPAPRXive object. The type of the source is stored in the same array for practical reasons.

Signed-off-by Cédric Le Goater <[email protected]>

---
 hw/intc/xive.c        | 54 +++++++++++++++++++++++++++++++++++++++++++++++----
 include/hw/ppc/xive.h | 16 +++++++++++++++
 2 files changed, 66 insertions(+), 4 deletions(-)

hw/intc/xive.c +22 -0

index c70578759d02..060976077dd7 100644
@@ -104,6 +104,21 @@ static void xive_source_notify(XiveSource *xsrc, int srcno)
 
 }
 
+/*
+ * LSI interrupt sources use the P bit and a custom assertion flag
+ */
+static bool xive_source_lsi_trigger(XiveSource *xsrc, uint32_t srcno)
+{
+    uint8_t old_pq = xive_source_pq_get(xsrc, srcno);
+
+    if  (old_pq == XIVE_ESB_RESET &&
+         xsrc->status[srcno] & XIVE_STATUS_ASSERTED) {
+        xive_source_pq_set(xsrc, srcno, XIVE_ESB_PENDING);
+        return true;
+    }
+    return false;
+}
+
 /* In a two pages ESB MMIO setting, even page is the trigger page, odd
  * page is for management */
 static inline bool xive_source_is_trigger_page(hwaddr addr)

@@ -133,6 +148,13 @@ static uint64_t xive_source_esb_read(void *opaque, hwaddr addr, unsigned size)
          */
         ret = xive_source_pq_eoi(xsrc, srcno);
 
+        /* If the LSI source is still asserted, forward a new source
+         * event notification */
+        if (xive_source_irq_is_lsi(xsrc, srcno)) {
+            if (xive_source_lsi_trigger(xsrc, srcno)) {
+                xive_source_notify(xsrc, srcno);
+            }
+        }
         break;
 

Quoted 15 lines

Quoted 5 lines

Quoted 5 lines

hw/intc/xive.c +0 -0

     case XIVE_ESB_GET:

@@ -183,6 +205,14 @@ static void xive_source_esb_write(void *opaque, hwaddr addr,
          * notification
          */
         notify = xive_source_pq_eoi(xsrc, srcno);

Quoted 8 lines

hw/intc/xive.c static void xive_source_esb_write(void *opaque, hwaddr addr, · line 215

Quoted 8 lines

hw/intc/xive.c +8 -0

+
+        /* LSI sources do not set the Q bit but they can still be
+         * asserted, in which case we should forward a new source
+         * event notification
+         */
+        if (xive_source_irq_is_lsi(xsrc, srcno)) {
+            notify = xive_source_lsi_trigger(xsrc, srcno);
+        }

FYI, I have moved that common test under xive_source_pq_eoi()

Ok.

Quoted 76 lines

Quoted 67 lines

Quoted 67 lines

hw/intc/xive.c +20 -4

         break;
 
     default:

@@ -216,8 +246,17 @@ static void xive_source_set_irq(void *opaque, int srcno, int val)
     XiveSource *xsrc = XIVE_SOURCE(opaque);
     bool notify = false;
 
-    if (val) {
-        notify = xive_source_pq_trigger(xsrc, srcno);
+    if (xive_source_irq_is_lsi(xsrc, srcno)) {
+        if (val) {
+            xsrc->status[srcno] |= XIVE_STATUS_ASSERTED;
+        } else {
+            xsrc->status[srcno] &= ~XIVE_STATUS_ASSERTED;
+        }
+        notify = xive_source_lsi_trigger(xsrc, srcno);
+    } else {
+        if (val) {
+            notify = xive_source_pq_trigger(xsrc, srcno);
+        }
     }
 
     /* Forward the source event notification for routing */

@@ -234,13 +273,13 @@ void xive_source_pic_print_info(XiveSource *xsrc, Monitor *mon)
                    xsrc->offset, xsrc->offset + xsrc->nr_irqs - 1);
     for (i = 0; i < xsrc->nr_irqs; i++) {
         uint8_t pq = xive_source_pq_get(xsrc, i);
-        uint32_t lisn = i  + xsrc->offset;
 
         if (pq == XIVE_ESB_OFF) {
             continue;
         }
 
-        monitor_printf(mon, "  %4x %c%c\n", lisn,
+        monitor_printf(mon, "  %4x %s %c%c\n", i + xsrc->offset,
+                       xive_source_irq_is_lsi(xsrc, i) ? "LSI" : "MSI",
                        pq & XIVE_ESB_VAL_P ? 'P' : '-',
                        pq & XIVE_ESB_VAL_Q ? 'Q' : '-');
     }

@@ -249,6 +288,12 @@ void xive_source_pic_print_info(XiveSource *xsrc, Monitor *mon)
 static void xive_source_reset(DeviceState *dev)
 {
     XiveSource *xsrc = XIVE_SOURCE(dev);
+    int i;
+
+    /* Keep the IRQ type */
+    for (i = 0; i < xsrc->nr_irqs; i++) {
+        xsrc->status[i] &= ~XIVE_STATUS_ASSERTED;
+    }
 
     /* SBEs are initialized to 0b01 which corresponds to "ints off" */
     memset(xsrc->sbe, 0x55, xsrc->sbe_size);

@@ -273,6 +318,7 @@ static void xive_source_realize(DeviceState *dev, Error **errp)
 
     xsrc->qirqs = qemu_allocate_irqs(xive_source_set_irq, xsrc,
                                      xsrc->nr_irqs);
+    xsrc->status = g_malloc0(xsrc->nr_irqs);
 
     /* Allocate the SBEs (State Bit Entry). 2 bits, so 4 entries per byte */
     xsrc->sbe_size = DIV_ROUND_UP(xsrc->nr_irqs, 4);

include/hw/ppc/xive.h +0 -0

index d92a50519edf..0b76dd278d9b 100644
@@ -33,6 +33,9 @@ typedef struct XiveSource {
     uint32_t     nr_irqs;
     uint32_t     offset;

Quoted 8 lines

include/hw/ppc/xive.h typedef struct XiveSource { · line 38

Quoted 4 lines

include/hw/ppc/xive.h +3 -0

     qemu_irq     *qirqs;
+#define XIVE_STATUS_LSI         0x1
+#define XIVE_STATUS_ASSERTED    0x2
+    uint8_t      *status;

I don't love the idea of mixing configuration information (STATUS_LSI) with runtime state information (ASSERTED) in the same field. Any reason not to have these as parallel bitmaps.

Quoted 1 line

none. I can change that.

Ok.

Quoted 9 lines

Quoted 3 lines

Come to that.. is there a compelling reason to allow any individual irq to be marked LSI or MSI, rather than using separate XiveSource objects for MSIs and LSIs?

yes. I would have preferred two distinct interrupt source objects but this is to be compatible with XICS, which uses only one. If we want to be able to change interrupt mode, the IRQ number space should be organized in the exact same way. Or we should change XICS also.

Quoted 4 lines

Quoted 1 line

Also, the change (a bitmap) is really small.

Hrm, but since XIVE supports thousands of irqs, it could be quite a large bitmap.

Yes. The change is small, not the bitmap.

Quoted 3 lines

It's not impossible - in fact, not really even that hard - to change the existing irq layout on xics. It does need a new machine type variant, of course.

I did some work on that topic a while ago :

	https://patchwork.ozlabs.org/cover/836782/

But we stopped exploring the idea. May be it was not the good approach. The PHBs LSIs would benefit from such a split though.

Quoted 12 lines

Quoted 11 lines

Quoted 6 lines

include/hw/ppc/xive.h typedef struct XiveSource { · line 41

Quoted 2 lines

include/hw/ppc/xive.h +0 -0

     /* PQ bits */
     uint8_t      *sbe;

.. and come to that is there a reason to keep the ASSERTED bit in a separate array from sbe? AFAICT the actual 2-bit-per-irq layout is never exposed to the guests.

indeed. we always use the xive_source_pq_get/set() helpers to manipulate the PQ bits. So we could add an extra bit for the ASSERT without too much changes. Could also we put the type there or would you still prefer a bitmap ?

Quoted 2 lines

I'd prefer the type (config information) be separate from the P, Q, ASSERTED bits (state information).

ok. So I will use the 'uint8_t *status' for P, Q, ASSERT, which leaves 5 bits available, but I don't think it is really worth the pain to optimize the size.

The sbe array will disappear and we will have
a bitmap for the type.

Thanks,

C.

Quoted 30 lines

Quoted 29 lines

Quoted 2 lines

Or, even re-use the Q bit for asserted in LSIs (but report it as always 0 in the register read/write path).

I would prefer to add extra status bits. It is easier to debug.

Thanks,

C.

Quoted 19 lines

Quoted 18 lines

include/hw/ppc/xive.h +13 -0

@@ -127,4 +130,17 @@ uint8_t xive_source_pq_set(XiveSource *xsrc, uint32_t srcno, uint8_t pq);
 
 void xive_source_pic_print_info(XiveSource *xsrc, Monitor *mon);
 
+static inline bool xive_source_irq_is_lsi(XiveSource *xsrc, uint32_t srcno)
+{
+    assert(srcno < xsrc->nr_irqs);
+    return xsrc->status[srcno] & XIVE_STATUS_LSI;
+}
+
+static inline void xive_source_irq_set(XiveSource *xsrc, uint32_t srcno,
+                                       bool lsi)
+{
+    assert(srcno < xsrc->nr_irqs);
+    xsrc->status[srcno] |= lsi ? XIVE_STATUS_LSI : 0;
+}
+
 #endif /* PPC_XIVE_H */