[Qemu-devel] [PATCH 0/6] move interrupts from spapr to xics

[Qemu-devel] [PATCH 0/6] move interrupts from spapr to xics

[Alexey Kardashevskiy]

The existing interrupt allocation scheme in SPAPR assumes that
interrupts are allocated at the start time, continously and the config
will not change. However, there are cases when this is not going to work
such as:

1. migration - we will have to have an ability to choose interrupt
numbers for devices in the command line and this will create gaps in
interrupt space.

2. PCI hotplug - interrupts from unplugged device need to be returned
back to interrupt pool, otherwise we will quickly run out of interrupts if
we do PCI hotplug often.

First this was posted as "[PATCH 0/8] spapr: fix IOMMU and XICS/IRQs migration"
but since then we decided to migrate IOMMU in a different way and now it just
about interrupts.

This is also going to be used in spapr_pci's ibm,change-msi callback
to release interrupts and reallocate them again. At the moment spapr_pci
keeps a map of what it allocated for what device and it would be nice
to get rid of that mapping too and use this patchset instead.

Please comment. Thanks!


Alexey Kardashevskiy (6):
  xics: add flags for interrupts
  xics: add find_server
  xics: disable flags reset on xics reset
  spapr: move interrupt allocator to xics
  spapr: remove @next_irq
  xics: implement xics_ics_free()

 hw/intc/xics.c         | 150 +++++++++++++++++++++++++++++++++++++++++++++----
 hw/intc/xics_kvm.c     |   9 +--
 hw/ppc/spapr.c         |  70 +----------------------
 hw/ppc/spapr_events.c  |   2 +-
 hw/ppc/spapr_pci.c     |   6 +-
 hw/ppc/spapr_vio.c     |   2 +-
 include/hw/ppc/spapr.h |  11 ----
 include/hw/ppc/xics.h  |   9 ++-
 trace-events           |   6 ++
 9 files changed, 162 insertions(+), 103 deletions(-)

-- 
1.8.4.rc4

[Qemu-devel] [PATCH 1/6] xics: add flags for interrupts

[Alexey Kardashevskiy]

The existing interrupt allocation scheme in SPAPR assumes that
interrupts are allocated at the start time, continously and the config
will not change. However, there are cases when this is not going to work
such as:

1. migration - we will have to have an ability to choose interrupt
numbers for devices in the command line and this will create gaps in
interrupt space.

2. PCI hotplug - interrupts from unplugged device need to be returned
back to interrupt pool, otherwise we will quickly run out of interrupts.

This replaces a separate lslsi[] array with a byte in the ICSIRQState
struct and defines "LSI" and "MSI" flags. Neither of these flags set
signals that the descriptor is not allocated and not in use.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
 hw/intc/xics.c        | 21 ++++++++++++++-------
 hw/intc/xics_kvm.c    |  5 ++---
 include/hw/ppc/xics.h |  5 ++++-
 3 files changed, 20 insertions(+), 11 deletions(-)

diff --git a/hw/intc/xics.c b/hw/intc/xics.c
index 64aabe7..1f89a00 100644
--- a/hw/intc/xics.c
+++ b/hw/intc/xics.c
@@ -438,7 +438,7 @@ static void ics_set_irq(void *opaque, int srcno, int val)
 {
     ICSState *ics = (ICSState *)opaque;
 
-    if (ics->islsi[srcno]) {
+    if (ics->irqs[srcno].flags & XICS_FLAGS_LSI) {
         set_irq_lsi(ics, srcno, val);
     } else {
         set_irq_msi(ics, srcno, val);
@@ -475,7 +475,7 @@ static void ics_write_xive(ICSState *ics, int nr, int server,
 
     trace_xics_ics_write_xive(nr, srcno, server, priority);
 
-    if (ics->islsi[srcno]) {
+    if (ics->irqs[srcno].flags & XICS_FLAGS_LSI) {
         write_xive_lsi(ics, srcno);
     } else {
         write_xive_msi(ics, srcno);
@@ -497,7 +497,7 @@ static void ics_resend(ICSState *ics)
 
     for (i = 0; i < ics->nr_irqs; i++) {
         /* FIXME: filter by server#? */
-        if (ics->islsi[i]) {
+        if (ics->irqs[i].flags & XICS_FLAGS_LSI) {
             resend_lsi(ics, i);
         } else {
             resend_msi(ics, i);
@@ -512,7 +512,7 @@ static void ics_eoi(ICSState *ics, int nr)
 
     trace_xics_ics_eoi(nr);
 
-    if (ics->islsi[srcno]) {
+    if (ics->irqs[srcno].flags & XICS_FLAGS_LSI) {
         irq->status &= ~XICS_STATUS_SENT;
     }
 }
@@ -609,7 +609,6 @@ static void ics_realize(DeviceState *dev, Error **errp)
         return;
     }
     ics->irqs = g_malloc0(ics->nr_irqs * sizeof(ICSIRQState));
-    ics->islsi = g_malloc0(ics->nr_irqs * sizeof(bool));
     ics->qirqs = qemu_allocate_irqs(ics_set_irq, ics, ics->nr_irqs);
 }
 
@@ -646,11 +645,19 @@ qemu_irq xics_get_qirq(XICSState *icp, int irq)
     return icp->ics->qirqs[irq - icp->ics->offset];
 }
 
+static void ics_set_irq_type(ICSState *ics, int srcno, bool lsi)
+{
+    ics->irqs[srcno].flags |=
+        lsi ? XICS_FLAGS_LSI : XICS_FLAGS_MSI;
+}
+
 void xics_set_irq_type(XICSState *icp, int irq, bool lsi)
 {
-    assert(ics_valid_irq(icp->ics, irq));
+    ICSState *ics = icp->ics;
 
-    icp->ics->islsi[irq - icp->ics->offset] = lsi;
+    assert(ics_valid_irq(ics, irq));
+
+    ics_set_irq_type(ics, irq - ics->offset, lsi);
 }
 
 /*
diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
index 09476ae..456fc2c 100644
--- a/hw/intc/xics_kvm.c
+++ b/hw/intc/xics_kvm.c
@@ -224,7 +224,7 @@ static int ics_set_kvm_state(ICSState *ics, int version_id)
             state |= KVM_XICS_MASKED;
         }
 
-        if (ics->islsi[i]) {
+        if (ics->irqs[i].flags & XICS_FLAGS_LSI) {
             state |= KVM_XICS_LEVEL_SENSITIVE;
             if (irq->status & XICS_STATUS_ASSERTED) {
                 state |= KVM_XICS_PENDING;
@@ -253,7 +253,7 @@ static void ics_kvm_set_irq(void *opaque, int srcno, int val)
     int rc;
 
     args.irq = srcno + ics->offset;
-    if (!ics->islsi[srcno]) {
+    if (ics->irqs[srcno].flags & XICS_FLAGS_MSI) {
         if (!val) {
             return;
         }
@@ -290,7 +290,6 @@ static void ics_kvm_realize(DeviceState *dev, Error **errp)
         return;
     }
     ics->irqs = g_malloc0(ics->nr_irqs * sizeof(ICSIRQState));
-    ics->islsi = g_malloc0(ics->nr_irqs * sizeof(bool));
     ics->qirqs = qemu_allocate_irqs(ics_kvm_set_irq, ics, ics->nr_irqs);
 }
 
diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
index 0d7673d..aad48cf 100644
--- a/include/hw/ppc/xics.h
+++ b/include/hw/ppc/xics.h
@@ -136,7 +136,6 @@ struct ICSState {
     uint32_t nr_irqs;
     uint32_t offset;
     qemu_irq *qirqs;
-    bool *islsi;
     ICSIRQState *irqs;
     XICSState *icp;
 };
@@ -150,6 +149,10 @@ struct ICSIRQState {
 #define XICS_STATUS_REJECTED           0x4
 #define XICS_STATUS_MASKED_PENDING     0x8
     uint8_t status;
+/* @flags == 0 measn the interrupt is not allocated */
+#define XICS_FLAGS_LSI                 0x1
+#define XICS_FLAGS_MSI                 0x2
+    uint8_t flags;
 };
 
 qemu_irq xics_get_qirq(XICSState *icp, int irq);
-- 
1.8.4.rc4

[Qemu-devel] [PATCH 2/6] xics: add find_server

[Alexey Kardashevskiy]

PAPR allows having multiple interrupr servers.

This adds a server lookup function and makes use of it.

Since at the moment QEMU only supports a single server,
no change in behaviour is expected.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
 hw/intc/xics.c | 28 +++++++++++++++++++++++-----
 1 file changed, 23 insertions(+), 5 deletions(-)

diff --git a/hw/intc/xics.c b/hw/intc/xics.c
index 1f89a00..9314654 100644
--- a/hw/intc/xics.c
+++ b/hw/intc/xics.c
@@ -635,14 +635,30 @@ static const TypeInfo ics_info = {
 /*
  * Exported functions
  */
+static int xics_find_server(XICSState *icp, int irq)
+{
+    int server;
+
+    for (server = 0; server < icp->nr_servers; ++server) {
+        ICSState *ics = &icp->ics[server];
+        if (ics_valid_irq(ics, irq)) {
+            return server;
+        }
+    }
+
+    return -1;
+}
 
 qemu_irq xics_get_qirq(XICSState *icp, int irq)
 {
-    if (!ics_valid_irq(icp->ics, irq)) {
-        return NULL;
+    int server = xics_find_server(icp, irq);
+
+    if (server >= 0) {
+        ICSState *ics = &icp->ics[server];
+        return ics->qirqs[irq - ics->offset];
     }
 
-    return icp->ics->qirqs[irq - icp->ics->offset];
+    return NULL;
 }
 
 static void ics_set_irq_type(ICSState *ics, int srcno, bool lsi)
@@ -653,10 +669,12 @@ static void ics_set_irq_type(ICSState *ics, int srcno, bool lsi)
 
 void xics_set_irq_type(XICSState *icp, int irq, bool lsi)
 {
-    ICSState *ics = icp->ics;
+    int server = xics_find_server(icp, irq);
+    ICSState *ics;
 
-    assert(ics_valid_irq(ics, irq));
+    assert(server >= 0);
 
+    ics = &icp->ics[server];
     ics_set_irq_type(ics, irq - ics->offset, lsi);
 }
 
-- 
1.8.4.rc4

[Qemu-devel] [PATCH 3/6] xics: disable flags reset on xics reset

[Alexey Kardashevskiy]

Since islsi[] array has been merged into the ICSState struct,
we must not reset flags as they tell if the interrupt is in use.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
 hw/intc/xics.c     | 4 +++-
 hw/intc/xics_kvm.c | 4 +++-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/hw/intc/xics.c b/hw/intc/xics.c
index 9314654..7a64b2e 100644
--- a/hw/intc/xics.c
+++ b/hw/intc/xics.c
@@ -522,10 +522,12 @@ static void ics_reset(DeviceState *dev)
     ICSState *ics = ICS(dev);
     int i;
 
-    memset(ics->irqs, 0, sizeof(ICSIRQState) * ics->nr_irqs);
     for (i = 0; i < ics->nr_irqs; i++) {
+        ics->irqs[i].server = 0;
         ics->irqs[i].priority = 0xff;
         ics->irqs[i].saved_priority = 0xff;
+        ics->irqs[i].status = 0;
+        /* Do not reset @flags as IRQ might be allocated */
     }
 }
 
diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
index 456fc2c..a322593 100644
--- a/hw/intc/xics_kvm.c
+++ b/hw/intc/xics_kvm.c
@@ -272,10 +272,12 @@ static void ics_kvm_reset(DeviceState *dev)
     ICSState *ics = ICS(dev);
     int i;
 
-    memset(ics->irqs, 0, sizeof(ICSIRQState) * ics->nr_irqs);
     for (i = 0; i < ics->nr_irqs; i++) {
+        ics->irqs[i].server = 0;
         ics->irqs[i].priority = 0xff;
         ics->irqs[i].saved_priority = 0xff;
+        ics->irqs[i].status = 0;
+        /* Do not reset @flags as IRQ might be allocated */
     }
 
     ics_set_kvm_state(ics, 1);
-- 
1.8.4.rc4

[Qemu-devel] [PATCH 4/6] spapr: move interrupt allocator to xics

[Alexey Kardashevskiy]

The current allocator returns IRQ numbers from a pool and does not
support IRQs reuse in any form as it did not keep track of what it
previously returned, it only keeps the last returned IRQ. Some use
cases such as PCI hot(un)plug may require IRQ release and reallocation.

This moves an allocator from SPAPR to XICS.

This switches IRQ users to use new API.

This uses LSI/MSI flags to know if interrupt is allocated.

This removes xics_set_irq_type() as it is not used anymore.

The interrupt release function will be posted as a separate patch.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
 hw/intc/xics.c         | 85 +++++++++++++++++++++++++++++++++++++++++++++++---
 hw/ppc/spapr.c         | 67 ---------------------------------------
 hw/ppc/spapr_events.c  |  2 +-
 hw/ppc/spapr_pci.c     |  6 ++--
 hw/ppc/spapr_vio.c     |  2 +-
 include/hw/ppc/spapr.h | 10 ------
 include/hw/ppc/xics.h  |  3 +-
 trace-events           |  4 +++
 8 files changed, 91 insertions(+), 88 deletions(-)

diff --git a/hw/intc/xics.c b/hw/intc/xics.c
index 7a64b2e..faf304c 100644
--- a/hw/intc/xics.c
+++ b/hw/intc/xics.c
@@ -669,15 +669,90 @@ static void ics_set_irq_type(ICSState *ics, int srcno, bool lsi)
         lsi ? XICS_FLAGS_LSI : XICS_FLAGS_MSI;
 }
 
-void xics_set_irq_type(XICSState *icp, int irq, bool lsi)
+#define ICS_IRQ_FREE(ics, srcno)   \
+    (!((ics)->irqs[(srcno)].flags & (XICS_FLAGS_LSI | XICS_FLAGS_MSI)))
+
+static int ics_find_free_block(ICSState *ics, int num, int alignnum)
+{
+    int first, i;
+
+    for (first = 0; first < ics->nr_irqs; first += alignnum) {
+        if (num > (ics->nr_irqs - first)) {
+            return -1;
+        }
+        for (i = first; i < first + num; ++i) {
+            if (!ICS_IRQ_FREE(ics, i)) {
+                break;
+            }
+        }
+        if (i == (first + num)) {
+            return first;
+        }
+    }
+
+    return -1;
+}
+
+int xics_alloc(XICSState *icp, int server, int irq_hint, bool lsi)
 {
-    int server = xics_find_server(icp, irq);
-    ICSState *ics;
+    ICSState *ics = &icp->ics[server];
+    int irq;
 
-    assert(server >= 0);
+    if (irq_hint) {
+        assert(server == xics_find_server(icp, irq_hint));
+        if (!ICS_IRQ_FREE(ics, irq_hint - ics->offset)) {
+            trace_xics_alloc_failed_hint(server, irq_hint);
+            return -1;
+        }
+        irq = irq_hint;
+    } else {
+        irq = ics_find_free_block(ics, 1, 1);
+        if (irq < 0) {
+            trace_xics_alloc_failed_no_left(server);
+            return -1;
+        }
+        irq += ics->offset;
+    }
 
-    ics = &icp->ics[server];
     ics_set_irq_type(ics, irq - ics->offset, lsi);
+    trace_xics_alloc(server, irq);
+
+    return irq;
+}
+
+/*
+ * Allocate block of consequtive IRQs, returns a number of the first.
+ * If align==true, aligns the first IRQ number to num.
+ */
+int xics_alloc_block(XICSState *icp, int server, int num, bool lsi, bool align)
+{
+    int i, first = -1;
+    ICSState *ics = &icp->ics[server];
+
+    assert(server == 0);
+    /*
+     * MSIMesage::data is used for storing VIRQ so
+     * it has to be aligned to num to support multiple
+     * MSI vectors. MSI-X is not affected by this.
+     * The hint is used for the first IRQ, the rest should
+     * be allocated continuously.
+     */
+    if (align) {
+        assert((num == 1) || (num == 2) || (num == 4) ||
+               (num == 8) || (num == 16) || (num == 32));
+        first = ics_find_free_block(ics, num, num);
+    } else {
+        first = ics_find_free_block(ics, num, 1);
+    }
+
+    if (first > 0) {
+        for (i = first; i < first + num; ++i) {
+            ics_set_irq_type(ics, i, lsi);
+        }
+    }
+    trace_xics_alloc_block(server, first, num, lsi, align);
+
+    return first + ics->offset;
 }
 
 /*
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index a11e121..db21515 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -88,73 +88,6 @@
 
 sPAPREnvironment *spapr;
 
-int spapr_allocate_irq(int hint, bool lsi)
-{
-    int irq;
-
-    if (hint) {
-        irq = hint;
-        if (hint >= spapr->next_irq) {
-            spapr->next_irq = hint + 1;
-        }
-        /* FIXME: we should probably check for collisions somehow */
-    } else {
-        irq = spapr->next_irq++;
-    }
-
-    /* Configure irq type */
-    if (!xics_get_qirq(spapr->icp, irq)) {
-        return 0;
-    }
-
-    xics_set_irq_type(spapr->icp, irq, lsi);
-
-    return irq;
-}
-
-/*
- * Allocate block of consequtive IRQs, returns a number of the first.
- * If msi==true, aligns the first IRQ number to num.
- */
-int spapr_allocate_irq_block(int num, bool lsi, bool msi)
-{
-    int first = -1;
-    int i, hint = 0;
-
-    /*
-     * MSIMesage::data is used for storing VIRQ so
-     * it has to be aligned to num to support multiple
-     * MSI vectors. MSI-X is not affected by this.
-     * The hint is used for the first IRQ, the rest should
-     * be allocated continuously.
-     */
-    if (msi) {
-        assert((num == 1) || (num == 2) || (num == 4) ||
-               (num == 8) || (num == 16) || (num == 32));
-        hint = (spapr->next_irq + num - 1) & ~(num - 1);
-    }
-
-    for (i = 0; i < num; ++i) {
-        int irq;
-
-        irq = spapr_allocate_irq(hint, lsi);
-        if (!irq) {
-            return -1;
-        }
-
-        if (0 == i) {
-            first = irq;
-            hint = 0;
-        }
-
-        /* If the above doesn't create a consecutive block then that's
-         * an internal bug */
-        assert(irq == (first + i));
-    }
-
-    return first;
-}
-
 static XICSState *try_create_xics(const char *type, int nr_servers,
                                   int nr_irqs)
 {
diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
index 16fa49e..e605430 100644
--- a/hw/ppc/spapr_events.c
+++ b/hw/ppc/spapr_events.c
@@ -314,7 +314,7 @@ static void check_exception(PowerPCCPU *cpu, sPAPREnvironment *spapr,
 
 void spapr_events_init(sPAPREnvironment *spapr)
 {
-    spapr->epow_irq = spapr_allocate_msi(0);
+    spapr->epow_irq = xics_alloc_block(spapr->icp, 0, 1, false, false);
     spapr->epow_notifier.notify = spapr_powerdown_req;
     qemu_register_powerdown_notifier(&spapr->epow_notifier);
     spapr_rtas_register("check-exception", check_exception);
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index cbef095..4eaf364 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -343,8 +343,8 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu, sPAPREnvironment *spapr,
 
     /* There is no cached config, allocate MSIs */
     if (!phb->msi_table[ndev].nvec) {
-        irq = spapr_allocate_irq_block(req_num, false,
-                                       ret_intr_type == RTAS_TYPE_MSI);
+        irq = xics_alloc_block(spapr->icp, 0, req_num, false,
+                               ret_intr_type == RTAS_TYPE_MSI);
         if (irq < 0) {
             error_report("Cannot allocate MSIs for device#%d", ndev);
             rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
@@ -623,7 +623,7 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
     for (i = 0; i < PCI_NUM_PINS; i++) {
         uint32_t irq;
 
-        irq = spapr_allocate_lsi(0);
+        irq = xics_alloc_block(spapr->icp, 0, 1, true, false);
         if (!irq) {
             error_setg(errp, "spapr_allocate_lsi failed");
             return;
diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c
index 2ae06a3..713499e 100644
--- a/hw/ppc/spapr_vio.c
+++ b/hw/ppc/spapr_vio.c
@@ -449,7 +449,7 @@ static int spapr_vio_busdev_init(DeviceState *qdev)
         dev->qdev.id = id;
     }
 
-    dev->irq = spapr_allocate_msi(dev->irq);
+    dev->irq = xics_alloc(spapr->icp, 0, dev->irq, false);
     if (!dev->irq) {
         return -1;
     }
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 5fdac1e..feb241a 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -327,16 +327,6 @@ target_ulong spapr_hypercall(PowerPCCPU *cpu, target_ulong opcode,
 int spapr_allocate_irq(int hint, bool lsi);
 int spapr_allocate_irq_block(int num, bool lsi, bool msi);
 
-static inline int spapr_allocate_msi(int hint)
-{
-    return spapr_allocate_irq(hint, false);
-}
-
-static inline int spapr_allocate_lsi(int hint)
-{
-    return spapr_allocate_irq(hint, true);
-}
-
 /* RTAS return codes */
 #define RTAS_OUT_SUCCESS            0
 #define RTAS_OUT_NO_ERRORS_FOUND    1
diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
index aad48cf..0f01a21 100644
--- a/include/hw/ppc/xics.h
+++ b/include/hw/ppc/xics.h
@@ -156,7 +156,8 @@ struct ICSIRQState {
 };
 
 qemu_irq xics_get_qirq(XICSState *icp, int irq);
-void xics_set_irq_type(XICSState *icp, int irq, bool lsi);
+int xics_alloc(XICSState *icp, int server, int irq_hint, bool lsi);
+int xics_alloc_block(XICSState *icp, int server, int num, bool lsi, bool align);
 
 void xics_cpu_setup(XICSState *icp, PowerPCCPU *cpu);
 
diff --git a/trace-events b/trace-events
index a5218ba..8cf8fb2 100644
--- a/trace-events
+++ b/trace-events
@@ -1175,6 +1175,10 @@ xics_set_irq_lsi(int srcno, int nr) "set_irq_lsi: srcno %d [irq %#x]"
 xics_ics_write_xive(int nr, int srcno, int server, uint8_t priority) "ics_write_xive: irq %#x [src %d] server %#x prio %#x"
 xics_ics_reject(int nr, int srcno) "reject irq %#x [src %d]"
 xics_ics_eoi(int nr) "ics_eoi: irq %#x"
+xics_alloc(int server, int irq) "server#%d, irq %d"
+xics_alloc_failed_hint(int server, int irq) "server#%d, irq %d is already in use"
+xics_alloc_failed_no_left(int server) "server#%d, no irq left"
+xics_alloc_block(int server, int first, int num, bool lsi, int align) "server#%d, first irq %d, %d irqs, lsi=%d, alignnum %d"
 
 # hw/ppc/spapr_iommu.c
 spapr_iommu_put(uint64_t liobn, uint64_t ioba, uint64_t tce, uint64_t ret) "liobn=%"PRIx64" ioba=0x%"PRIx64" tce=0x%"PRIx64" ret=%"PRId64
-- 
1.8.4.rc4

[Qemu-devel] [PATCH 5/6] spapr: remove @next_irq

[Alexey Kardashevskiy]

This removes @next_irq from sPAPREnvironment which was used in old
IRQ allocator as XICS is now responsible for IRQs and keeps track of
allocated IRQs.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
 hw/ppc/spapr.c         | 3 +--
 include/hw/ppc/spapr.h | 1 -
 2 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index db21515..a680e90 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -754,7 +754,7 @@ static const VMStateDescription vmstate_spapr = {
     .minimum_version_id = 1,
     .minimum_version_id_old = 1,
     .fields      = (VMStateField []) {
-        VMSTATE_UINT32(next_irq, sPAPREnvironment),
+        VMSTATE_UNUSED(4), /* used to be @next_irq */
 
         /* RTC offset */
         VMSTATE_UINT64(rtc_offset, sPAPREnvironment),
@@ -1158,7 +1158,6 @@ static void ppc_spapr_init(QEMUMachineInitArgs *args)
     /* Set up Interrupt Controller before we create the VCPUs */
     spapr->icp = xics_system_init(smp_cpus * kvmppc_smt_threads() / smp_threads,
                                   XICS_IRQS);
-    spapr->next_irq = XICS_IRQ_BASE;
 
     /* init CPUs */
     if (cpu_model == NULL) {
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index feb241a..f8d7326 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -27,7 +27,6 @@ typedef struct sPAPREnvironment {
     long rtas_size;
     void *fdt_skel;
     target_ulong entry_point;
-    uint32_t next_irq;
     uint64_t rtc_offset;
     bool has_graphics;
 
-- 
1.8.4.rc4

[Qemu-devel] [PATCH 6/6] xics: implement xics_ics_free()

[Alexey Kardashevskiy]

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
 hw/intc/xics.c        | 24 ++++++++++++++++++++++++
 include/hw/ppc/xics.h |  1 +
 trace-events          |  2 ++
 3 files changed, 27 insertions(+)

diff --git a/hw/intc/xics.c b/hw/intc/xics.c
index faf304c..2316519 100644
--- a/hw/intc/xics.c
+++ b/hw/intc/xics.c
@@ -755,6 +755,30 @@ int xics_alloc_block(XICSState *icp, int server, int num, bool lsi, bool align)
     return first + ics->offset;
 }
 
+static void ics_free(ICSState *ics, int srcno, int num)
+{
+    int i;
+
+    for (i = srcno; i < srcno + num; ++i) {
+        if (ICS_IRQ_FREE(ics, i)) {
+            trace_xics_ics_free_warn(ics - ics->icp->ics, i + ics->offset);
+        }
+        memset(&ics->irqs[i], 0, sizeof(ICSIRQState));
+    }
+}
+
+void xics_free(XICSState *icp, int server, int irq, int num)
+{
+    ICSState *ics = &icp->ics[server];
+
+    assert(server == 0);
+
+    trace_xics_ics_free(ics - icp->ics, irq, num);
+    if (server >= 0) {
+        ics_free(ics, irq - ics->offset, num);
+    }
+}
+
 /*
  * Guest interfaces
  */
diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
index 0f01a21..e5d8f47 100644
--- a/include/hw/ppc/xics.h
+++ b/include/hw/ppc/xics.h
@@ -158,6 +158,7 @@ struct ICSIRQState {
 qemu_irq xics_get_qirq(XICSState *icp, int irq);
 int xics_alloc(XICSState *icp, int server, int irq_hint, bool lsi);
 int xics_alloc_block(XICSState *icp, int server, int num, bool lsi, bool align);
+void xics_free(XICSState *icp, int server, int irq, int num);
 
 void xics_cpu_setup(XICSState *icp, PowerPCCPU *cpu);
 
diff --git a/trace-events b/trace-events
index 8cf8fb2..5ca126c 100644
--- a/trace-events
+++ b/trace-events
@@ -1179,6 +1179,8 @@ xics_alloc(int server, int irq) "server#%d, irq %d"
 xics_alloc_failed_hint(int server, int irq) "server#%d, irq %d is already in use"
 xics_alloc_failed_no_left(int server) "server#%d, no irq left"
 xics_alloc_block(int server, int first, int num, bool lsi, int align) "server#%d, first irq %d, %d irqs, lsi=%d, alignnum %d"
+xics_ics_free(int server, int irq, int num) "server#%d, first irq %d, %d irqs"
+xics_ics_free_warn(int server, int irq) "server#%d, irq %d is already free"
 
 # hw/ppc/spapr_iommu.c
 spapr_iommu_put(uint64_t liobn, uint64_t ioba, uint64_t tce, uint64_t ret) "liobn=%"PRIx64" ioba=0x%"PRIx64" tce=0x%"PRIx64" ret=%"PRId64
-- 
1.8.4.rc4

Re: [Qemu-devel] [PATCH 1/6] xics: add flags for interrupts

[Mike Day]

>  
>      for (i = 0; i < ics->nr_irqs; i++) {
>          /* FIXME: filter by server#? */
> -        if (ics->islsi[i]) {
> +        if (ics->irqs[i].flags & XICS_FLAGS_LSI) {
>              resend_lsi(ics, i);

Not part of your patch, but I'm curious about this FIXME (there's an
identical FIXME in resend_msi). Has this proved to be a problem? With
these patches you could have many unallocated interrupts in array AFTER
the last allocated interrupt, correct? In that case, the loop would
continue beyond the last allocated interrupt for no purpose.  There are
a couple ways to mitigate this type of situation by using alternative
data structures to inform the loop traversal. I don't know if it is
worth the effort, though.


> +/* @flags == 0 measn the interrupt is not allocated */
> +#define XICS_FLAGS_LSI                 0x1
> +#define XICS_FLAGS_MSI                 0x2

(nit) typo in the above comment

Mike

-- 
Mike Day | "Endurance is a Virtue"

Re: [Qemu-devel] [PATCH 2/6] xics: add find_server

[Mike Day]

Alexey Kardashevskiy <aik@ozlabs.ru> writes:

> PAPR allows having multiple interrupr servers.
>

typo above in the commit log entry,

Mike
-- 
Mike Day | "Endurance is a Virtue"

Re: [Qemu-devel] [PATCH 1/6] xics: add flags for interrupts

[Alexander Graf]

On 05/07/2014 08:01 AM, Alexey Kardashevskiy wrote:
> The existing interrupt allocation scheme in SPAPR assumes that
> interrupts are allocated at the start time, continously and the config
> will not change. However, there are cases when this is not going to work
> such as:
>
> 1. migration - we will have to have an ability to choose interrupt
> numbers for devices in the command line and this will create gaps in
> interrupt space.
>
> 2. PCI hotplug - interrupts from unplugged device need to be returned
> back to interrupt pool, otherwise we will quickly run out of interrupts.
>
> This replaces a separate lslsi[] array with a byte in the ICSIRQState
> struct and defines "LSI" and "MSI" flags. Neither of these flags set
> signals that the descriptor is not allocated and not in use.
>
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
>   hw/intc/xics.c        | 21 ++++++++++++++-------
>   hw/intc/xics_kvm.c    |  5 ++---
>   include/hw/ppc/xics.h |  5 ++++-
>   3 files changed, 20 insertions(+), 11 deletions(-)
>
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index 64aabe7..1f89a00 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -438,7 +438,7 @@ static void ics_set_irq(void *opaque, int srcno, int val)
>   {
>       ICSState *ics = (ICSState *)opaque;
>   
> -    if (ics->islsi[srcno]) {
> +    if (ics->irqs[srcno].flags & XICS_FLAGS_LSI) {
>           set_irq_lsi(ics, srcno, val);
>       } else {
>           set_irq_msi(ics, srcno, val);
> @@ -475,7 +475,7 @@ static void ics_write_xive(ICSState *ics, int nr, int server,
>   
>       trace_xics_ics_write_xive(nr, srcno, server, priority);
>   
> -    if (ics->islsi[srcno]) {
> +    if (ics->irqs[srcno].flags & XICS_FLAGS_LSI) {
>           write_xive_lsi(ics, srcno);
>       } else {
>           write_xive_msi(ics, srcno);
> @@ -497,7 +497,7 @@ static void ics_resend(ICSState *ics)
>   
>       for (i = 0; i < ics->nr_irqs; i++) {
>           /* FIXME: filter by server#? */
> -        if (ics->islsi[i]) {
> +        if (ics->irqs[i].flags & XICS_FLAGS_LSI) {
>               resend_lsi(ics, i);
>           } else {
>               resend_msi(ics, i);
> @@ -512,7 +512,7 @@ static void ics_eoi(ICSState *ics, int nr)
>   
>       trace_xics_ics_eoi(nr);
>   
> -    if (ics->islsi[srcno]) {
> +    if (ics->irqs[srcno].flags & XICS_FLAGS_LSI) {
>           irq->status &= ~XICS_STATUS_SENT;
>       }
>   }
> @@ -609,7 +609,6 @@ static void ics_realize(DeviceState *dev, Error **errp)
>           return;
>       }
>       ics->irqs = g_malloc0(ics->nr_irqs * sizeof(ICSIRQState));
> -    ics->islsi = g_malloc0(ics->nr_irqs * sizeof(bool));
>       ics->qirqs = qemu_allocate_irqs(ics_set_irq, ics, ics->nr_irqs);
>   }
>   
> @@ -646,11 +645,19 @@ qemu_irq xics_get_qirq(XICSState *icp, int irq)
>       return icp->ics->qirqs[irq - icp->ics->offset];
>   }
>   
> +static void ics_set_irq_type(ICSState *ics, int srcno, bool lsi)
> +{
> +    ics->irqs[srcno].flags |=
> +        lsi ? XICS_FLAGS_LSI : XICS_FLAGS_MSI;
> +}
> +
>   void xics_set_irq_type(XICSState *icp, int irq, bool lsi)
>   {
> -    assert(ics_valid_irq(icp->ics, irq));
> +    ICSState *ics = icp->ics;
>   
> -    icp->ics->islsi[irq - icp->ics->offset] = lsi;
> +    assert(ics_valid_irq(ics, irq));
> +
> +    ics_set_irq_type(ics, irq - ics->offset, lsi);

What if this gets called with MSI first, then LSI on the same irq number?

>   }
>   
>   /*
> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> index 09476ae..456fc2c 100644
> --- a/hw/intc/xics_kvm.c
> +++ b/hw/intc/xics_kvm.c
> @@ -224,7 +224,7 @@ static int ics_set_kvm_state(ICSState *ics, int version_id)
>               state |= KVM_XICS_MASKED;
>           }
>   
> -        if (ics->islsi[i]) {
> +        if (ics->irqs[i].flags & XICS_FLAGS_LSI) {
>               state |= KVM_XICS_LEVEL_SENSITIVE;
>               if (irq->status & XICS_STATUS_ASSERTED) {
>                   state |= KVM_XICS_PENDING;
> @@ -253,7 +253,7 @@ static void ics_kvm_set_irq(void *opaque, int srcno, int val)
>       int rc;
>   
>       args.irq = srcno + ics->offset;
> -    if (!ics->islsi[srcno]) {
> +    if (ics->irqs[srcno].flags & XICS_FLAGS_MSI) {
>           if (!val) {
>               return;
>           }
> @@ -290,7 +290,6 @@ static void ics_kvm_realize(DeviceState *dev, Error **errp)
>           return;
>       }
>       ics->irqs = g_malloc0(ics->nr_irqs * sizeof(ICSIRQState));
> -    ics->islsi = g_malloc0(ics->nr_irqs * sizeof(bool));
>       ics->qirqs = qemu_allocate_irqs(ics_kvm_set_irq, ics, ics->nr_irqs);
>   }
>   
> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> index 0d7673d..aad48cf 100644
> --- a/include/hw/ppc/xics.h
> +++ b/include/hw/ppc/xics.h
> @@ -136,7 +136,6 @@ struct ICSState {
>       uint32_t nr_irqs;
>       uint32_t offset;
>       qemu_irq *qirqs;
> -    bool *islsi;
>       ICSIRQState *irqs;
>       XICSState *icp;
>   };
> @@ -150,6 +149,10 @@ struct ICSIRQState {
>   #define XICS_STATUS_REJECTED           0x4
>   #define XICS_STATUS_MASKED_PENDING     0x8
>       uint8_t status;
> +/* @flags == 0 measn the interrupt is not allocated */
> +#define XICS_FLAGS_LSI                 0x1
> +#define XICS_FLAGS_MSI                 0x2

Please define a mask for the interrupt type.


Alex

> +    uint8_t flags;
>   };
>   
>   qemu_irq xics_get_qirq(XICSState *icp, int irq);

Re: [Qemu-devel] [PATCH 3/6] xics: disable flags reset on xics reset

[Alexander Graf]

On 05/07/2014 08:01 AM, Alexey Kardashevskiy wrote:
> Since islsi[] array has been merged into the ICSState struct,
> we must not reset flags as they tell if the interrupt is in use.
>
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
>   hw/intc/xics.c     | 4 +++-
>   hw/intc/xics_kvm.c | 4 +++-
>   2 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index 9314654..7a64b2e 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -522,10 +522,12 @@ static void ics_reset(DeviceState *dev)
>       ICSState *ics = ICS(dev);
>       int i;
>   
> -    memset(ics->irqs, 0, sizeof(ICSIRQState) * ics->nr_irqs);
>       for (i = 0; i < ics->nr_irqs; i++) {
> +        ics->irqs[i].server = 0;
>           ics->irqs[i].priority = 0xff;
>           ics->irqs[i].saved_priority = 0xff;
> +        ics->irqs[i].status = 0;
> +        /* Do not reset @flags as IRQ might be allocated */

I think the code would be more obvious if you save the flags before you 
do the memset() - that would then have to move into the loop - and 
restore them afterwards.


Alex

>       }
>   }
>   
> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> index 456fc2c..a322593 100644
> --- a/hw/intc/xics_kvm.c
> +++ b/hw/intc/xics_kvm.c
> @@ -272,10 +272,12 @@ static void ics_kvm_reset(DeviceState *dev)
>       ICSState *ics = ICS(dev);
>       int i;
>   
> -    memset(ics->irqs, 0, sizeof(ICSIRQState) * ics->nr_irqs);
>       for (i = 0; i < ics->nr_irqs; i++) {
> +        ics->irqs[i].server = 0;
>           ics->irqs[i].priority = 0xff;
>           ics->irqs[i].saved_priority = 0xff;
> +        ics->irqs[i].status = 0;
> +        /* Do not reset @flags as IRQ might be allocated */
>       }
>   
>       ics_set_kvm_state(ics, 1);

Re: [Qemu-devel] [PATCH 6/6] xics: implement xics_ics_free()

[Alexander Graf]

On 05/07/2014 08:01 AM, Alexey Kardashevskiy wrote:
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
>   hw/intc/xics.c        | 24 ++++++++++++++++++++++++
>   include/hw/ppc/xics.h |  1 +
>   trace-events          |  2 ++
>   3 files changed, 27 insertions(+)
>
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index faf304c..2316519 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -755,6 +755,30 @@ int xics_alloc_block(XICSState *icp, int server, int num, bool lsi, bool align)
>       return first + ics->offset;
>   }
>   
> +static void ics_free(ICSState *ics, int srcno, int num)
> +{
> +    int i;
> +
> +    for (i = srcno; i < srcno + num; ++i) {
> +        if (ICS_IRQ_FREE(ics, i)) {
> +            trace_xics_ics_free_warn(ics - ics->icp->ics, i + ics->offset);
> +        }
> +        memset(&ics->irqs[i], 0, sizeof(ICSIRQState));

Why does this do something different from ics_reset?

Alex

> +    }
> +}
> +
> +void xics_free(XICSState *icp, int server, int irq, int num)
> +{
> +    ICSState *ics = &icp->ics[server];
> +
> +    assert(server == 0);
> +
> +    trace_xics_ics_free(ics - icp->ics, irq, num);
> +    if (server >= 0) {
> +        ics_free(ics, irq - ics->offset, num);
> +    }
> +}
> +
>   /*
>    * Guest interfaces
>    */
> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> index 0f01a21..e5d8f47 100644
> --- a/include/hw/ppc/xics.h
> +++ b/include/hw/ppc/xics.h
> @@ -158,6 +158,7 @@ struct ICSIRQState {
>   qemu_irq xics_get_qirq(XICSState *icp, int irq);
>   int xics_alloc(XICSState *icp, int server, int irq_hint, bool lsi);
>   int xics_alloc_block(XICSState *icp, int server, int num, bool lsi, bool align);
> +void xics_free(XICSState *icp, int server, int irq, int num);
>   
>   void xics_cpu_setup(XICSState *icp, PowerPCCPU *cpu);
>   
> diff --git a/trace-events b/trace-events
> index 8cf8fb2..5ca126c 100644
> --- a/trace-events
> +++ b/trace-events
> @@ -1179,6 +1179,8 @@ xics_alloc(int server, int irq) "server#%d, irq %d"
>   xics_alloc_failed_hint(int server, int irq) "server#%d, irq %d is already in use"
>   xics_alloc_failed_no_left(int server) "server#%d, no irq left"
>   xics_alloc_block(int server, int first, int num, bool lsi, int align) "server#%d, first irq %d, %d irqs, lsi=%d, alignnum %d"
> +xics_ics_free(int server, int irq, int num) "server#%d, first irq %d, %d irqs"
> +xics_ics_free_warn(int server, int irq) "server#%d, irq %d is already free"
>   
>   # hw/ppc/spapr_iommu.c
>   spapr_iommu_put(uint64_t liobn, uint64_t ioba, uint64_t tce, uint64_t ret) "liobn=%"PRIx64" ioba=0x%"PRIx64" tce=0x%"PRIx64" ret=%"PRId64

Re: [Qemu-devel] [Qemu-ppc] [PATCH 1/6] xics: add flags for interrupts

[Alexander Graf]

On 05/08/2014 01:52 PM, Alexander Graf wrote:
> On 05/07/2014 08:01 AM, Alexey Kardashevskiy wrote:
>> The existing interrupt allocation scheme in SPAPR assumes that
>> interrupts are allocated at the start time, continously and the config
>> will not change. However, there are cases when this is not going to work
>> such as:
>>
>> 1. migration - we will have to have an ability to choose interrupt
>> numbers for devices in the command line and this will create gaps in
>> interrupt space.
>>
>> 2. PCI hotplug - interrupts from unplugged device need to be returned
>> back to interrupt pool, otherwise we will quickly run out of interrupts.
>>
>> This replaces a separate lslsi[] array with a byte in the ICSIRQState
>> struct and defines "LSI" and "MSI" flags. Neither of these flags set
>> signals that the descriptor is not allocated and not in use.
>>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> ---
>>   hw/intc/xics.c        | 21 ++++++++++++++-------
>>   hw/intc/xics_kvm.c    |  5 ++---
>>   include/hw/ppc/xics.h |  5 ++++-
>>   3 files changed, 20 insertions(+), 11 deletions(-)
>>
>> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
>> index 64aabe7..1f89a00 100644
>> --- a/hw/intc/xics.c
>> +++ b/hw/intc/xics.c
>> @@ -438,7 +438,7 @@ static void ics_set_irq(void *opaque, int srcno, 
>> int val)
>>   {
>>       ICSState *ics = (ICSState *)opaque;
>>   -    if (ics->islsi[srcno]) {
>> +    if (ics->irqs[srcno].flags & XICS_FLAGS_LSI) {
>>           set_irq_lsi(ics, srcno, val);
>>       } else {
>>           set_irq_msi(ics, srcno, val);
>> @@ -475,7 +475,7 @@ static void ics_write_xive(ICSState *ics, int nr, 
>> int server,
>>         trace_xics_ics_write_xive(nr, srcno, server, priority);
>>   -    if (ics->islsi[srcno]) {
>> +    if (ics->irqs[srcno].flags & XICS_FLAGS_LSI) {
>>           write_xive_lsi(ics, srcno);
>>       } else {
>>           write_xive_msi(ics, srcno);
>> @@ -497,7 +497,7 @@ static void ics_resend(ICSState *ics)
>>         for (i = 0; i < ics->nr_irqs; i++) {
>>           /* FIXME: filter by server#? */
>> -        if (ics->islsi[i]) {
>> +        if (ics->irqs[i].flags & XICS_FLAGS_LSI) {
>>               resend_lsi(ics, i);
>>           } else {
>>               resend_msi(ics, i);
>> @@ -512,7 +512,7 @@ static void ics_eoi(ICSState *ics, int nr)
>>         trace_xics_ics_eoi(nr);
>>   -    if (ics->islsi[srcno]) {
>> +    if (ics->irqs[srcno].flags & XICS_FLAGS_LSI) {
>>           irq->status &= ~XICS_STATUS_SENT;
>>       }
>>   }
>> @@ -609,7 +609,6 @@ static void ics_realize(DeviceState *dev, Error 
>> **errp)
>>           return;
>>       }
>>       ics->irqs = g_malloc0(ics->nr_irqs * sizeof(ICSIRQState));
>> -    ics->islsi = g_malloc0(ics->nr_irqs * sizeof(bool));
>>       ics->qirqs = qemu_allocate_irqs(ics_set_irq, ics, ics->nr_irqs);
>>   }
>>   @@ -646,11 +645,19 @@ qemu_irq xics_get_qirq(XICSState *icp, int irq)
>>       return icp->ics->qirqs[irq - icp->ics->offset];
>>   }
>>   +static void ics_set_irq_type(ICSState *ics, int srcno, bool lsi)
>> +{
>> +    ics->irqs[srcno].flags |=
>> +        lsi ? XICS_FLAGS_LSI : XICS_FLAGS_MSI;
>> +}
>> +
>>   void xics_set_irq_type(XICSState *icp, int irq, bool lsi)
>>   {
>> -    assert(ics_valid_irq(icp->ics, irq));
>> +    ICSState *ics = icp->ics;
>>   -    icp->ics->islsi[irq - icp->ics->offset] = lsi;
>> +    assert(ics_valid_irq(ics, irq));
>> +
>> +    ics_set_irq_type(ics, irq - ics->offset, lsi);
>
> What if this gets called with MSI first, then LSI on the same irq number?

Following your code I think this never happens. But to ensure it doesn't 
add an assert(!(ics->irqs[srcno].flags & XICS_FLAGS_IRQTYPE)) in 
ircs_set_irq_type() maybe?


Alex

Re: [Qemu-devel] [PATCH 6/6] xics: implement xics_ics_free()

[Alexey Kardashevskiy]

On 05/08/2014 10:07 PM, Alexander Graf wrote:
> On 05/07/2014 08:01 AM, Alexey Kardashevskiy wrote:
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> ---
>>   hw/intc/xics.c        | 24 ++++++++++++++++++++++++
>>   include/hw/ppc/xics.h |  1 +
>>   trace-events          |  2 ++
>>   3 files changed, 27 insertions(+)
>>
>> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
>> index faf304c..2316519 100644
>> --- a/hw/intc/xics.c
>> +++ b/hw/intc/xics.c
>> @@ -755,6 +755,30 @@ int xics_alloc_block(XICSState *icp, int server, int
>> num, bool lsi, bool align)
>>       return first + ics->offset;
>>   }
>>   +static void ics_free(ICSState *ics, int srcno, int num)
>> +{
>> +    int i;
>> +
>> +    for (i = srcno; i < srcno + num; ++i) {
>> +        if (ICS_IRQ_FREE(ics, i)) {
>> +            trace_xics_ics_free_warn(ics - ics->icp->ics, i + ics->offset);
>> +        }
>> +        memset(&ics->irqs[i], 0, sizeof(ICSIRQState));
> 
> Why does this do something different from ics_reset?


ics_reset() resets the interrupt state which still remains allocated and
assigned to some device. ics_free() clears the descriptor and this
interrupt may be now returned if someone asks for a new interrupt (pci
hotplug, intx->msi switch).


> Alex
> 
>> +    }
>> +}
>> +
>> +void xics_free(XICSState *icp, int server, int irq, int num)
>> +{
>> +    ICSState *ics = &icp->ics[server];
>> +
>> +    assert(server == 0);
>> +
>> +    trace_xics_ics_free(ics - icp->ics, irq, num);
>> +    if (server >= 0) {
>> +        ics_free(ics, irq - ics->offset, num);
>> +    }
>> +}
>> +
>>   /*
>>    * Guest interfaces
>>    */
>> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
>> index 0f01a21..e5d8f47 100644
>> --- a/include/hw/ppc/xics.h
>> +++ b/include/hw/ppc/xics.h
>> @@ -158,6 +158,7 @@ struct ICSIRQState {
>>   qemu_irq xics_get_qirq(XICSState *icp, int irq);
>>   int xics_alloc(XICSState *icp, int server, int irq_hint, bool lsi);
>>   int xics_alloc_block(XICSState *icp, int server, int num, bool lsi,
>> bool align);
>> +void xics_free(XICSState *icp, int server, int irq, int num);
>>     void xics_cpu_setup(XICSState *icp, PowerPCCPU *cpu);
>>   diff --git a/trace-events b/trace-events
>> index 8cf8fb2..5ca126c 100644
>> --- a/trace-events
>> +++ b/trace-events
>> @@ -1179,6 +1179,8 @@ xics_alloc(int server, int irq) "server#%d, irq %d"
>>   xics_alloc_failed_hint(int server, int irq) "server#%d, irq %d is
>> already in use"
>>   xics_alloc_failed_no_left(int server) "server#%d, no irq left"
>>   xics_alloc_block(int server, int first, int num, bool lsi, int align)
>> "server#%d, first irq %d, %d irqs, lsi=%d, alignnum %d"
>> +xics_ics_free(int server, int irq, int num) "server#%d, first irq %d, %d
>> irqs"
>> +xics_ics_free_warn(int server, int irq) "server#%d, irq %d is already free"
>>     # hw/ppc/spapr_iommu.c
>>   spapr_iommu_put(uint64_t liobn, uint64_t ioba, uint64_t tce, uint64_t
>> ret) "liobn=%"PRIx64" ioba=0x%"PRIx64" tce=0x%"PRIx64" ret=%"PRId64
> 


-- 
Alexey

Re: [Qemu-devel] [PATCH 1/6] xics: add flags for interrupts

[Alexey Kardashevskiy]

On 05/07/2014 11:09 PM, Mike Day wrote:
> 
>>  
>>      for (i = 0; i < ics->nr_irqs; i++) {
>>          /* FIXME: filter by server#? */
>> -        if (ics->islsi[i]) {
>> +        if (ics->irqs[i].flags & XICS_FLAGS_LSI) {
>>              resend_lsi(ics, i);
> 
> Not part of your patch, but I'm curious about this FIXME (there's an
> identical FIXME in resend_msi). Has this proved to be a problem?

ics_resend -> resend_lsi() -> icp_irq() and the last one delivers interrupt
to the correct server. I should probably remove that FIXME or I am missing
something here.

> With
> these patches you could have many unallocated interrupts in array AFTER
> the last allocated interrupt, correct?

I would have unallocated interrupts before this patch too.

> In that case, the loop would
> continue beyond the last allocated interrupt for no purpose. 

For LSI we check the type, if it is not, it is assumed MSI and then
resend_msi() would check for XICS_STATUS_REJECTED which is not set for
non-allocated interrupt so we are safe.

But since I can tell now if it is allocated or not, I will fix it to call
resend_msi() on only if irq is allocated as MSI.

> There are
> a couple ways to mitigate this type of situation by using alternative
> data structures to inform the loop traversal. I don't know if it is
> worth the effort, though.

Here I lost you :)

btw I just realized that in patch#2 it should be xics_find_source instead
of xics_find_server. There are many interrupt servers already and just one
interrupt source (we could have many like one per PHB or something like
that but we are not there yet), this is what I meant.


> 
> 
>> +/* @flags == 0 measn the interrupt is not allocated */
>> +#define XICS_FLAGS_LSI                 0x1
>> +#define XICS_FLAGS_MSI                 0x2
> 
> (nit) typo in the above comment
> 
> Mike
> 


-- 
Alexey

Re: [Qemu-devel] [PATCH 1/6] xics: add flags for interrupts

[Mike Day]

On Thu, May 8, 2014 at 11:12 PM, Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
>> There are
>> a couple ways to mitigate this type of situation by using alternative
>> data structures to inform the loop traversal. I don't know if it is
>> worth the effort, though.
>
> Here I lost you :)

If I read the code correctly, the problem I'm wondering  about is that
the loop will waste time traversing the array  when there are only
unallocated interrupts from the current index to the end. For example,
if the interrupt array is 256 entries and the highest index that is
allocated is 16, the outside loop will traverse all 256 entries while
it should have exited after the 16th.

To mitigate this you could keep a shadow index of the current highest
allocated index and check for that in the outside loop. Or you could
maintain a shadow linked list that only includes allocated array
entries and just traverse that list. Each element on the list would be
an allocated entry in the interrupt array.

> btw I just realized that in patch#2 it should be xics_find_source instead
> of xics_find_server. There are many interrupt servers already and just one
> interrupt source (we could have many like one per PHB or something like
> that but we are not there yet), this is what I meant.