[Qemu-devel] [PATCH v3 0/3] spapr: introduce a fixed IRQ number space and an IRQ controller backend

[Qemu-devel] [PATCH v3 0/3] spapr: introduce a fixed IRQ number space and an IRQ controller backend

[Cédric Le Goater]

Here is a proposal for a new IRQ number space layout using static
numbers and a bitmap allocator for the MSIs. The previous layout is
kept for compatibly in machines raising the 'legacy_irq_allocation' flag.

The patchset also introduces a sPAPR IRQ interface which offers the
possibility to provide different IRQ controller backend to the sPAPR
machine. This is preparing ground for the new XIVE controller.

Thanks,

C.

Changes since v2 :

 - renamed 'xics_legacy' to 'legacy_irq_allocation'
 - introduced the sPAPR IRQ backend interface
 - increase the size of the IRQ number space for newer machines

Changes since v1 :

 - removed block allocation
 - spaced the IRQ offsets 
 - check for overlaps when allocating VIO irqs
 - removed 'Error *' arg from spapr_irq_msi_init() 


Cédric Le Goater (3):
  spapr: introduce a fixed IRQ number space
  spapr: introduce a IRQ controller backend to the machine
  spapr: increase the size of the IRQ number space

 include/hw/ppc/spapr.h     |  14 +--
 include/hw/ppc/spapr_irq.h |  55 +++++++++
 hw/ppc/spapr.c             | 201 +++++-------------------------
 hw/ppc/spapr_events.c      |  12 +-
 hw/ppc/spapr_irq.c         | 297 +++++++++++++++++++++++++++++++++++++++++++++
 hw/ppc/spapr_pci.c         |  29 ++++-
 hw/ppc/spapr_vio.c         |  19 ++-
 hw/ppc/Makefile.objs       |   2 +-
 8 files changed, 438 insertions(+), 191 deletions(-)
 create mode 100644 include/hw/ppc/spapr_irq.h
 create mode 100644 hw/ppc/spapr_irq.c

-- 
2.13.6

[Qemu-devel] [PATCH v3 1/3] spapr: introduce a fixed IRQ number space

[Cédric Le Goater]

This proposal introduces a new IRQ number space layout using static
numbers for all devices and a bitmap allocator for the MSI IRQ numbers
which are negotiated by the guest at runtime.

The previous layout is kept in pre-3.0 machines raising the
'legacy_irq_allocation' machine class flag.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 include/hw/ppc/spapr.h     |  5 +++++
 include/hw/ppc/spapr_irq.h | 30 +++++++++++++++++++++++++
 hw/ppc/spapr.c             | 31 +++++++++++++++++++++++++
 hw/ppc/spapr_events.c      | 12 ++++++++--
 hw/ppc/spapr_irq.c         | 56 ++++++++++++++++++++++++++++++++++++++++++++++
 hw/ppc/spapr_pci.c         | 29 +++++++++++++++++++-----
 hw/ppc/spapr_vio.c         | 19 ++++++++++++----
 hw/ppc/Makefile.objs       |  2 +-
 8 files changed, 171 insertions(+), 13 deletions(-)
 create mode 100644 include/hw/ppc/spapr_irq.h
 create mode 100644 hw/ppc/spapr_irq.c

diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 8a9142244f12..b60f84c3adfc 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -7,6 +7,7 @@
 #include "hw/ppc/spapr_drc.h"
 #include "hw/mem/pc-dimm.h"
 #include "hw/ppc/spapr_ovec.h"
+#include "hw/ppc/spapr_irq.h"
 
 struct VIOsPAPRBus;
 struct sPAPRPHBState;
@@ -98,6 +99,8 @@ struct sPAPRMachineClass {
     bool dr_lmb_enabled;       /* enable dynamic-reconfig/hotplug of LMBs */
     bool use_ohci_by_default;  /* use USB-OHCI instead of XHCI */
     bool pre_2_10_has_unused_icps;
+    bool legacy_irq_allocation;
+
     void (*phb_placement)(sPAPRMachineState *spapr, uint32_t index,
                           uint64_t *buid, hwaddr *pio, 
                           hwaddr *mmio32, hwaddr *mmio64,
@@ -164,6 +167,8 @@ struct sPAPRMachineState {
     char *kvm_type;
 
     const char *icp_type;
+    int32_t irq_map_nr;
+    unsigned long *irq_map;
 
     bool cmd_line_caps[SPAPR_CAP_NUM];
     sPAPRCapabilities def, eff, mig;
diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
new file mode 100644
index 000000000000..ac5cdc44e5d8
--- /dev/null
+++ b/include/hw/ppc/spapr_irq.h
@@ -0,0 +1,30 @@
+/*
+ * QEMU PowerPC sPAPR IRQ backend definitions
+ *
+ * Copyright (c) 2018, IBM Corporation.
+ *
+ * This code is licensed under the GPL version 2 or later. See the
+ * COPYING file in the top-level directory.
+ */
+
+#ifndef HW_SPAPR_IRQ_H
+#define HW_SPAPR_IRQ_H
+
+/*
+ * IRQ range offsets per device type
+ */
+#define SPAPR_IRQ_EPOW       0x1000  /* XICS_IRQ_BASE offset */
+#define SPAPR_IRQ_HOTPLUG    0x1001
+#define SPAPR_IRQ_VIO        0x1100  /* 256 VIO devices */
+#define SPAPR_IRQ_PCI_LSI    0x1200  /* 32+ PHBs devices */
+
+#define SPAPR_IRQ_MSI        0x1300  /* Offset of the dynamic range covered
+                                      * by the bitmap allocator */
+
+void spapr_irq_msi_init(sPAPRMachineState *spapr, uint32_t nr_msis);
+int spapr_irq_msi_alloc(sPAPRMachineState *spapr, uint32_t num, bool align,
+                        Error **errp);
+void spapr_irq_msi_free(sPAPRMachineState *spapr, int irq, uint32_t num);
+void spapr_irq_msi_reset(sPAPRMachineState *spapr);
+
+#endif
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 78186500e917..5a55b4f45e02 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -188,6 +188,11 @@ static void xics_system_init(MachineState *machine, int nr_irqs, Error **errp)
     sPAPRMachineState *spapr = SPAPR_MACHINE(machine);
     Error *local_err = NULL;
 
+    /* Initialize the MSI IRQ allocator. */
+    if (!SPAPR_MACHINE_GET_CLASS(spapr)->legacy_irq_allocation) {
+        spapr_irq_msi_init(spapr, XICS_IRQ_BASE + nr_irqs - SPAPR_IRQ_MSI);
+    }
+
     if (kvm_enabled()) {
         if (machine_kernel_irqchip_allowed(machine) &&
             !xics_kvm_init(spapr, &local_err)) {
@@ -1635,6 +1640,10 @@ static void spapr_machine_reset(void)
         ppc_set_compat(first_ppc_cpu, spapr->max_compat_pvr, &error_fatal);
     }
 
+    if (!SPAPR_MACHINE_GET_CLASS(spapr)->legacy_irq_allocation) {
+        spapr_irq_msi_reset(spapr);
+    }
+
     qemu_devices_reset();
 
     /* DRC reset may cause a device to be unplugged. This will cause troubles
@@ -1909,6 +1918,24 @@ static const VMStateDescription vmstate_spapr_patb_entry = {
     },
 };
 
+static bool spapr_irq_map_needed(void *opaque)
+{
+    sPAPRMachineState *spapr = opaque;
+
+    return spapr->irq_map && !bitmap_empty(spapr->irq_map, spapr->irq_map_nr);
+}
+
+static const VMStateDescription vmstate_spapr_irq_map = {
+    .name = "spapr_irq_map",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = spapr_irq_map_needed,
+    .fields = (VMStateField[]) {
+        VMSTATE_BITMAP(irq_map, sPAPRMachineState, 0, irq_map_nr),
+        VMSTATE_END_OF_LIST()
+    },
+};
+
 static const VMStateDescription vmstate_spapr = {
     .name = "spapr",
     .version_id = 3,
@@ -1936,6 +1963,7 @@ static const VMStateDescription vmstate_spapr = {
         &vmstate_spapr_cap_cfpc,
         &vmstate_spapr_cap_sbbc,
         &vmstate_spapr_cap_ibs,
+        &vmstate_spapr_irq_map,
         NULL
     }
 };
@@ -4103,8 +4131,11 @@ static void spapr_machine_2_12_instance_options(MachineState *machine)
 
 static void spapr_machine_2_12_class_options(MachineClass *mc)
 {
+    sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(mc);
+
     spapr_machine_3_0_class_options(mc);
     SET_MACHINE_COMPAT(mc, SPAPR_COMPAT_2_12);
+    smc->legacy_irq_allocation = true;
 }
 
 DEFINE_SPAPR_MACHINE(2_12, "2.12", false);
diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
index e4f5946a2188..cab950d33446 100644
--- a/hw/ppc/spapr_events.c
+++ b/hw/ppc/spapr_events.c
@@ -709,7 +709,11 @@ void spapr_events_init(sPAPRMachineState *spapr)
 {
     int epow_irq;
 
-    epow_irq = spapr_irq_findone(spapr, &error_fatal);
+    if (SPAPR_MACHINE_GET_CLASS(spapr)->legacy_irq_allocation) {
+        epow_irq = spapr_irq_findone(spapr, &error_fatal);
+    } else {
+        epow_irq = SPAPR_IRQ_EPOW;
+    }
 
     spapr_irq_claim(spapr, epow_irq, false, &error_fatal);
 
@@ -731,7 +735,11 @@ void spapr_events_init(sPAPRMachineState *spapr)
     if (spapr->use_hotplug_event_source) {
         int hp_irq;
 
-        hp_irq = spapr_irq_findone(spapr, &error_fatal);
+        if (SPAPR_MACHINE_GET_CLASS(spapr)->legacy_irq_allocation) {
+            hp_irq = spapr_irq_findone(spapr, &error_fatal);
+        } else {
+            hp_irq = SPAPR_IRQ_HOTPLUG;
+        }
 
         spapr_irq_claim(spapr, hp_irq, false, &error_fatal);
 
diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
new file mode 100644
index 000000000000..24e9c1d4433c
--- /dev/null
+++ b/hw/ppc/spapr_irq.c
@@ -0,0 +1,56 @@
+/*
+ * QEMU PowerPC sPAPR IRQ interface
+ *
+ * Copyright (c) 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 "qemu/error-report.h"
+#include "qapi/error.h"
+#include "hw/ppc/spapr.h"
+#include "hw/ppc/xics.h"
+
+void spapr_irq_msi_init(sPAPRMachineState *spapr, uint32_t nr_msis)
+{
+    spapr->irq_map_nr = nr_msis;
+    spapr->irq_map = bitmap_new(spapr->irq_map_nr);
+}
+
+int spapr_irq_msi_alloc(sPAPRMachineState *spapr, uint32_t num, bool align,
+                        Error **errp)
+{
+    int irq;
+
+    /*
+     * The 'align_mask' parameter of bitmap_find_next_zero_area()
+     * should be one less than a power of 2; 0 means no
+     * alignment. Adapt the 'align' value of the former allocator
+     * to fit the requirements of bitmap_find_next_zero_area()
+     */
+    align -= 1;
+
+    irq = bitmap_find_next_zero_area(spapr->irq_map, spapr->irq_map_nr, 0, num,
+                                     align);
+    if (irq == spapr->irq_map_nr) {
+        error_setg(errp, "can't find a free %d-IRQ block", num);
+        return -1;
+    }
+
+    bitmap_set(spapr->irq_map, irq, num);
+
+    return irq + SPAPR_IRQ_MSI;
+}
+
+void spapr_irq_msi_free(sPAPRMachineState *spapr, int irq, uint32_t num)
+{
+    bitmap_clear(spapr->irq_map, irq - SPAPR_IRQ_MSI, num);
+}
+
+void spapr_irq_msi_reset(sPAPRMachineState *spapr)
+{
+    bitmap_clear(spapr->irq_map, 0, spapr->irq_map_nr);
+}
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 497b896c7d24..cba5340f4bad 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -334,6 +334,9 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
             return;
         }
 
+        if (!SPAPR_MACHINE_GET_CLASS(spapr)->legacy_irq_allocation) {
+            spapr_irq_msi_free(spapr, msi->first_irq, msi->num);
+        }
         spapr_irq_free(spapr, msi->first_irq, msi->num);
         if (msi_present(pdev)) {
             spapr_msi_setmsg(pdev, 0, false, 0, 0);
@@ -372,7 +375,13 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
     }
 
     /* Allocate MSIs */
-    irq = spapr_irq_find(spapr, req_num, ret_intr_type == RTAS_TYPE_MSI, &err);
+    if (SPAPR_MACHINE_GET_CLASS(spapr)->legacy_irq_allocation) {
+        irq = spapr_irq_find(spapr, req_num, ret_intr_type == RTAS_TYPE_MSI,
+                             &err);
+    } else {
+        irq = spapr_irq_msi_alloc(spapr, req_num,
+                                  ret_intr_type == RTAS_TYPE_MSI, &err);
+    }
     if (err) {
         error_reportf_err(err, "Can't allocate MSIs for device %x: ",
                           config_addr);
@@ -392,6 +401,9 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
 
     /* Release previous MSIs */
     if (msi) {
+        if (!SPAPR_MACHINE_GET_CLASS(spapr)->legacy_irq_allocation) {
+            spapr_irq_msi_free(spapr, msi->first_irq, msi->num);
+        }
         spapr_irq_free(spapr, msi->first_irq, msi->num);
         g_hash_table_remove(phb->msi, &config_addr);
     }
@@ -1708,11 +1720,15 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
         uint32_t irq;
         Error *local_err = NULL;
 
-        irq = spapr_irq_findone(spapr, &local_err);
-        if (local_err) {
-            error_propagate(errp, local_err);
-            error_prepend(errp, "can't allocate LSIs: ");
-            return;
+        if (SPAPR_MACHINE_GET_CLASS(spapr)->legacy_irq_allocation) {
+            irq = spapr_irq_findone(spapr, &local_err);
+            if (local_err) {
+                error_propagate(errp, local_err);
+                error_prepend(errp, "can't allocate LSIs: ");
+                return;
+            }
+        } else {
+            irq = SPAPR_IRQ_PCI_LSI + sphb->index * PCI_NUM_PINS + i;
         }
 
         spapr_irq_claim(spapr, irq, true, &local_err);
@@ -2123,6 +2139,7 @@ int spapr_populate_pci_dt(sPAPRPHBState *phb,
     _FDT(fdt_setprop(fdt, bus_off, "ranges", &ranges, sizeof_ranges));
     _FDT(fdt_setprop(fdt, bus_off, "reg", &bus_reg, sizeof(bus_reg)));
     _FDT(fdt_setprop_cell(fdt, bus_off, "ibm,pci-config-space-type", 0x1));
+    /* TODO: fine tune the total count of allocatable MSIs per PHB */
     _FDT(fdt_setprop_cell(fdt, bus_off, "ibm,pe-total-#msi", XICS_IRQS_SPAPR));
 
     /* Dynamic DMA window */
diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c
index daf85130b5ef..2ecee3e8d0ed 100644
--- a/hw/ppc/spapr_vio.c
+++ b/hw/ppc/spapr_vio.c
@@ -436,6 +436,9 @@ static void spapr_vio_busdev_reset(DeviceState *qdev)
     }
 }
 
+/* TODO : poor VIO device indexing ... */
+static uint32_t vio_index;
+
 static void spapr_vio_busdev_realize(DeviceState *qdev, Error **errp)
 {
     sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
@@ -476,10 +479,18 @@ static void spapr_vio_busdev_realize(DeviceState *qdev, Error **errp)
     }
 
     if (!dev->irq) {
-        dev->irq = spapr_irq_findone(spapr, &local_err);
-        if (local_err) {
-            error_propagate(errp, local_err);
-            return;
+        if (SPAPR_MACHINE_GET_CLASS(spapr)->legacy_irq_allocation) {
+            dev->irq = spapr_irq_findone(spapr, &local_err);
+            if (local_err) {
+                error_propagate(errp, local_err);
+                return;
+            }
+        } else {
+            dev->irq = SPAPR_IRQ_VIO + vio_index++;
+            if (dev->irq == SPAPR_IRQ_PCI_LSI) {
+                error_setg(errp, "Too many VIO devices");
+                return;
+            }
         }
     }
 
diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
index 86d82a6ec3ac..4fe3b7804d43 100644
--- a/hw/ppc/Makefile.objs
+++ b/hw/ppc/Makefile.objs
@@ -4,7 +4,7 @@ obj-y += ppc.o ppc_booke.o fdt.o
 obj-$(CONFIG_PSERIES) += spapr.o spapr_caps.o spapr_vio.o spapr_events.o
 obj-$(CONFIG_PSERIES) += spapr_hcall.o spapr_iommu.o spapr_rtas.o
 obj-$(CONFIG_PSERIES) += spapr_pci.o spapr_rtc.o spapr_drc.o spapr_rng.o
-obj-$(CONFIG_PSERIES) += spapr_cpu_core.o spapr_ovec.o
+obj-$(CONFIG_PSERIES) += spapr_cpu_core.o spapr_ovec.o spapr_irq.o
 # IBM PowerNV
 obj-$(CONFIG_POWERNV) += pnv.o pnv_xscom.o pnv_core.o pnv_lpc.o pnv_psi.o pnv_occ.o pnv_bmc.o
 ifeq ($(CONFIG_PCI)$(CONFIG_PSERIES)$(CONFIG_LINUX), yyy)
-- 
2.13.6

[Qemu-devel] [PATCH v3 2/3] spapr: introduce a IRQ controller backend to the machine

[Cédric Le Goater]

This proposal moves all the related IRQ routines of the sPAPR machine
behind a sPAPR IRQ backend interface 'spapr_irq' to prepare for future
changes. First of which will be to increase the size of the IRQ number
space, then, will follow a new backend for the POWER9 XIVE IRQ controller.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 include/hw/ppc/spapr.h     |   9 +-
 include/hw/ppc/spapr_irq.h |  24 +++++
 hw/ppc/spapr.c             | 179 +----------------------------------
 hw/ppc/spapr_irq.c         | 231 +++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 260 insertions(+), 183 deletions(-)

diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index b60f84c3adfc..acc2c01e1123 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -107,6 +107,7 @@ struct sPAPRMachineClass {
                           unsigned n_dma, uint32_t *liobns, Error **errp);
     sPAPRResizeHPT resize_hpt_default;
     sPAPRCapabilities default_caps;
+    sPAPRIrq *irq;
 };
 
 /**
@@ -777,14 +778,6 @@ int spapr_get_vcpu_id(PowerPCCPU *cpu);
 void spapr_set_vcpu_id(PowerPCCPU *cpu, int cpu_index, Error **errp);
 PowerPCCPU *spapr_find_cpu(int vcpu_id);
 
-int spapr_irq_find(sPAPRMachineState *spapr, int num, bool align,
-                   Error **errp);
-#define spapr_irq_findone(spapr, errp) spapr_irq_find(spapr, 1, false, errp)
-int spapr_irq_claim(sPAPRMachineState *spapr, int irq, bool lsi, Error **errp);
-void spapr_irq_free(sPAPRMachineState *spapr, int irq, int num);
-qemu_irq spapr_qirq(sPAPRMachineState *spapr, int irq);
-
-
 int spapr_caps_pre_load(void *opaque);
 int spapr_caps_pre_save(void *opaque);
 
diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
index ac5cdc44e5d8..f2ae0ac0e6cb 100644
--- a/include/hw/ppc/spapr_irq.h
+++ b/include/hw/ppc/spapr_irq.h
@@ -27,4 +27,28 @@ int spapr_irq_msi_alloc(sPAPRMachineState *spapr, uint32_t num, bool align,
 void spapr_irq_msi_free(sPAPRMachineState *spapr, int irq, uint32_t num);
 void spapr_irq_msi_reset(sPAPRMachineState *spapr);
 
+typedef struct sPAPRIrq {
+    uint32_t    nr_irqs;
+
+    void (*init)(sPAPRMachineState *spapr, Error **errp);
+    int (*claim)(sPAPRMachineState *spapr, int irq, bool lsi, Error **errp);
+    void (*free)(sPAPRMachineState *spapr, int irq, int num);
+    qemu_irq (*qirq)(sPAPRMachineState *spapr, int irq);
+    void (*print_info)(sPAPRMachineState *spapr, Monitor *mon);
+} sPAPRIrq;
+
+extern sPAPRIrq spapr_irq_xics;
+
+int spapr_irq_claim(sPAPRMachineState *spapr, int irq, bool lsi, Error **errp);
+void spapr_irq_free(sPAPRMachineState *spapr, int irq, int num);
+qemu_irq spapr_qirq(sPAPRMachineState *spapr, int irq);
+
+
+/*
+ * XICS legacy routines
+ */
+int spapr_irq_find(sPAPRMachineState *spapr, int num, bool align, Error **errp);
+#define spapr_irq_findone(spapr, errp) spapr_irq_find(spapr, 1, false, errp)
+
+
 #endif
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 5a55b4f45e02..ec21df432380 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -116,33 +116,6 @@ static bool spapr_is_thread0_in_vcore(sPAPRMachineState *spapr,
     return spapr_get_vcpu_id(cpu) % spapr->vsmt == 0;
 }
 
-static ICSState *spapr_ics_create(sPAPRMachineState *spapr,
-                                  const char *type_ics,
-                                  int nr_irqs, Error **errp)
-{
-    Error *local_err = NULL;
-    Object *obj;
-
-    obj = object_new(type_ics);
-    object_property_add_child(OBJECT(spapr), "ics", obj, &error_abort);
-    object_property_add_const_link(obj, ICS_PROP_XICS, OBJECT(spapr),
-                                   &error_abort);
-    object_property_set_int(obj, nr_irqs, "nr-irqs", &local_err);
-    if (local_err) {
-        goto error;
-    }
-    object_property_set_bool(obj, true, "realized", &local_err);
-    if (local_err) {
-        goto error;
-    }
-
-    return ICS_SIMPLE(obj);
-
-error:
-    error_propagate(errp, local_err);
-    return NULL;
-}
-
 static bool pre_2_10_vmstate_dummy_icp_needed(void *opaque)
 {
     /* Dummy entries correspond to unused ICPState objects in older QEMUs,
@@ -183,43 +156,6 @@ static int xics_max_server_number(sPAPRMachineState *spapr)
     return DIV_ROUND_UP(max_cpus * spapr->vsmt, smp_threads);
 }
 
-static void xics_system_init(MachineState *machine, int nr_irqs, Error **errp)
-{
-    sPAPRMachineState *spapr = SPAPR_MACHINE(machine);
-    Error *local_err = NULL;
-
-    /* Initialize the MSI IRQ allocator. */
-    if (!SPAPR_MACHINE_GET_CLASS(spapr)->legacy_irq_allocation) {
-        spapr_irq_msi_init(spapr, XICS_IRQ_BASE + nr_irqs - SPAPR_IRQ_MSI);
-    }
-
-    if (kvm_enabled()) {
-        if (machine_kernel_irqchip_allowed(machine) &&
-            !xics_kvm_init(spapr, &local_err)) {
-            spapr->icp_type = TYPE_KVM_ICP;
-            spapr->ics = spapr_ics_create(spapr, TYPE_ICS_KVM, nr_irqs,
-                                          &local_err);
-        }
-        if (machine_kernel_irqchip_required(machine) && !spapr->ics) {
-            error_prepend(&local_err,
-                          "kernel_irqchip requested but unavailable: ");
-            goto error;
-        }
-        error_free(local_err);
-        local_err = NULL;
-    }
-
-    if (!spapr->ics) {
-        xics_spapr_init(spapr);
-        spapr->icp_type = TYPE_ICP;
-        spapr->ics = spapr_ics_create(spapr, TYPE_ICS_SIMPLE, nr_irqs,
-                                      &local_err);
-    }
-
-error:
-    error_propagate(errp, local_err);
-}
-
 static int spapr_fixup_cpu_smt_dt(void *fdt, int offset, PowerPCCPU *cpu,
                                   int smt_threads)
 {
@@ -2617,7 +2553,7 @@ static void spapr_machine_init(MachineState *machine)
     load_limit = MIN(spapr->rma_size, RTAS_MAX_ADDR) - FW_OVERHEAD;
 
     /* Set up Interrupt Controller before we create the VCPUs */
-    xics_system_init(machine, XICS_IRQS_SPAPR, &error_fatal);
+    smc->irq->init(spapr, &error_fatal);
 
     /* Set up containers for ibm,client-architecture-support negotiated options
      */
@@ -3820,121 +3756,13 @@ static ICPState *spapr_icp_get(XICSFabric *xi, int vcpu_id)
     return cpu ? ICP(cpu->intc) : NULL;
 }
 
-#define ICS_IRQ_FREE(ics, srcno)   \
-    (!((ics)->irqs[(srcno)].flags & (XICS_FLAGS_IRQ_MASK)))
-
-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 spapr_irq_find(sPAPRMachineState *spapr, int num, bool align, Error **errp)
-{
-    ICSState *ics = spapr->ics;
-    int first = -1;
-
-    assert(ics);
-
-    /*
-     * 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) {
-        error_setg(errp, "can't find a free %d-IRQ block", num);
-        return -1;
-    }
-
-    return first + ics->offset;
-}
-
-int spapr_irq_claim(sPAPRMachineState *spapr, int irq, bool lsi, Error **errp)
-{
-    ICSState *ics = spapr->ics;
-
-    assert(ics);
-
-    if (!ics_valid_irq(ics, irq)) {
-        error_setg(errp, "IRQ %d is invalid", irq);
-        return -1;
-    }
-
-    if (!ICS_IRQ_FREE(ics, irq - ics->offset)) {
-        error_setg(errp, "IRQ %d is not free", irq);
-        return -1;
-    }
-
-    ics_set_irq_type(ics, irq - ics->offset, lsi);
-    return 0;
-}
-
-void spapr_irq_free(sPAPRMachineState *spapr, int irq, int num)
-{
-    ICSState *ics = spapr->ics;
-    int srcno = irq - ics->offset;
-    int i;
-
-    if (ics_valid_irq(ics, irq)) {
-        trace_spapr_irq_free(0, irq, num);
-        for (i = srcno; i < srcno + num; ++i) {
-            if (ICS_IRQ_FREE(ics, i)) {
-                trace_spapr_irq_free_warn(0, i + ics->offset);
-            }
-            memset(&ics->irqs[i], 0, sizeof(ICSIRQState));
-        }
-    }
-}
-
-qemu_irq spapr_qirq(sPAPRMachineState *spapr, int irq)
-{
-    ICSState *ics = spapr->ics;
-
-    if (ics_valid_irq(ics, irq)) {
-        return ics->qirqs[irq - ics->offset];
-    }
-
-    return NULL;
-}
-
 static void spapr_pic_print_info(InterruptStatsProvider *obj,
                                  Monitor *mon)
 {
     sPAPRMachineState *spapr = SPAPR_MACHINE(obj);
-    CPUState *cs;
-
-    CPU_FOREACH(cs) {
-        PowerPCCPU *cpu = POWERPC_CPU(cs);
-
-        icp_pic_print_info(ICP(cpu->intc), mon);
-    }
+    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
 
-    ics_pic_print_info(spapr->ics, mon);
+    smc->irq->print_info(spapr, mon);
 }
 
 int spapr_get_vcpu_id(PowerPCCPU *cpu)
@@ -4044,6 +3872,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
     smc->default_caps.caps[SPAPR_CAP_SBBC] = SPAPR_CAP_BROKEN;
     smc->default_caps.caps[SPAPR_CAP_IBS] = SPAPR_CAP_BROKEN;
     spapr_caps_add_properties(smc, &error_abort);
+    smc->irq = &spapr_irq_xics;
 }
 
 static const TypeInfo spapr_machine_info = {
diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
index 24e9c1d4433c..e50a512a2d9c 100644
--- a/hw/ppc/spapr_irq.c
+++ b/hw/ppc/spapr_irq.c
@@ -13,6 +13,9 @@
 #include "qapi/error.h"
 #include "hw/ppc/spapr.h"
 #include "hw/ppc/xics.h"
+#include "sysemu/kvm.h"
+
+#include "trace.h"
 
 void spapr_irq_msi_init(sPAPRMachineState *spapr, uint32_t nr_msis)
 {
@@ -54,3 +57,231 @@ void spapr_irq_msi_reset(sPAPRMachineState *spapr)
 {
     bitmap_clear(spapr->irq_map, 0, spapr->irq_map_nr);
 }
+
+
+/*
+ * XICS IRQ backend.
+ */
+
+static ICSState *spapr_ics_create(sPAPRMachineState *spapr,
+                                  const char *type_ics,
+                                  int nr_irqs, Error **errp)
+{
+    Error *local_err = NULL;
+    Object *obj;
+
+    obj = object_new(type_ics);
+    object_property_add_child(OBJECT(spapr), "ics", obj, &error_abort);
+    object_property_add_const_link(obj, ICS_PROP_XICS, OBJECT(spapr),
+                                   &error_abort);
+    object_property_set_int(obj, nr_irqs, "nr-irqs", &local_err);
+    if (local_err) {
+        goto error;
+    }
+    object_property_set_bool(obj, true, "realized", &local_err);
+    if (local_err) {
+        goto error;
+    }
+
+    return ICS_SIMPLE(obj);
+
+error:
+    error_propagate(errp, local_err);
+    return NULL;
+}
+
+static void spapr_irq_init_xics(sPAPRMachineState *spapr, Error **errp)
+{
+    MachineState *machine = MACHINE(spapr);
+    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
+    int nr_irqs = smc->irq->nr_irqs;
+    Error *local_err = NULL;
+
+    /* Initialize the MSI IRQ allocator. */
+    if (!SPAPR_MACHINE_GET_CLASS(spapr)->legacy_irq_allocation) {
+        spapr_irq_msi_init(spapr, XICS_IRQ_BASE + nr_irqs - SPAPR_IRQ_MSI);
+    }
+
+    if (kvm_enabled()) {
+        if (machine_kernel_irqchip_allowed(machine) &&
+            !xics_kvm_init(spapr, &local_err)) {
+            spapr->icp_type = TYPE_KVM_ICP;
+            spapr->ics = spapr_ics_create(spapr, TYPE_ICS_KVM, nr_irqs,
+                                          &local_err);
+        }
+        if (machine_kernel_irqchip_required(machine) && !spapr->ics) {
+            error_prepend(&local_err,
+                          "kernel_irqchip requested but unavailable: ");
+            goto error;
+        }
+        error_free(local_err);
+        local_err = NULL;
+    }
+
+    if (!spapr->ics) {
+        xics_spapr_init(spapr);
+        spapr->icp_type = TYPE_ICP;
+        spapr->ics = spapr_ics_create(spapr, TYPE_ICS_SIMPLE, nr_irqs,
+                                      &local_err);
+    }
+
+error:
+    error_propagate(errp, local_err);
+}
+
+#define ICS_IRQ_FREE(ics, srcno)   \
+    (!((ics)->irqs[(srcno)].flags & (XICS_FLAGS_IRQ_MASK)))
+
+static int spapr_irq_claim_xics(sPAPRMachineState *spapr, int irq, bool lsi,
+                                Error **errp)
+{
+    ICSState *ics = spapr->ics;
+
+    assert(ics);
+
+    if (!ics_valid_irq(ics, irq)) {
+        error_setg(errp, "IRQ %d is invalid", irq);
+        return -1;
+    }
+
+    if (!ICS_IRQ_FREE(ics, irq - ics->offset)) {
+        error_setg(errp, "IRQ %d is not free", irq);
+        return -1;
+    }
+
+    ics_set_irq_type(ics, irq - ics->offset, lsi);
+    return 0;
+}
+
+static void spapr_irq_free_xics(sPAPRMachineState *spapr, int irq, int num)
+{
+    ICSState *ics = spapr->ics;
+    uint32_t srcno = irq - ics->offset;
+    int i;
+
+    if (ics_valid_irq(ics, irq)) {
+        trace_spapr_irq_free(0, irq, num);
+        for (i = srcno; i < srcno + num; ++i) {
+            if (ICS_IRQ_FREE(ics, i)) {
+                trace_spapr_irq_free_warn(0, i);
+            }
+            memset(&ics->irqs[i], 0, sizeof(ICSIRQState));
+        }
+    }
+}
+
+static qemu_irq spapr_qirq_xics(sPAPRMachineState *spapr, int irq)
+{
+    ICSState *ics = spapr->ics;
+    uint32_t srcno = irq - ics->offset;
+
+    if (ics_valid_irq(ics, irq)) {
+        return ics->qirqs[srcno];
+    }
+
+    return NULL;
+}
+
+static void spapr_irq_print_info_xics(sPAPRMachineState *spapr,
+                                      Monitor *mon)
+{
+    CPUState *cs;
+
+    CPU_FOREACH(cs) {
+        PowerPCCPU *cpu = POWERPC_CPU(cs);
+
+        icp_pic_print_info(ICP(cpu->intc), mon);
+    }
+
+    ics_pic_print_info(spapr->ics, mon);
+}
+
+sPAPRIrq spapr_irq_xics = {
+    .nr_irqs     = XICS_IRQS_SPAPR,
+
+    .init        = spapr_irq_init_xics,
+    .claim       = spapr_irq_claim_xics,
+    .free        = spapr_irq_free_xics,
+    .qirq        = spapr_qirq_xics,
+    .print_info  = spapr_irq_print_info_xics,
+};
+
+/*
+ * sPAPR IRQ frontend routines for devices
+ */
+
+int spapr_irq_claim(sPAPRMachineState *spapr, int irq, bool lsi, Error **errp)
+{
+    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
+
+    return smc->irq->claim(spapr, irq, lsi, errp);
+}
+
+void spapr_irq_free(sPAPRMachineState *spapr, int irq, int num)
+{
+    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
+
+    smc->irq->free(spapr, irq, num);
+}
+
+qemu_irq spapr_qirq(sPAPRMachineState *spapr, int irq)
+{
+    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
+
+    return smc->irq->qirq(spapr, irq);
+}
+
+/*
+ * XICS legacy routines - to deprecate one day
+ */
+
+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 spapr_irq_find(sPAPRMachineState *spapr, int num, bool align, Error **errp)
+{
+    ICSState *ics = spapr->ics;
+    int first = -1;
+
+    assert(ics);
+
+    /*
+     * 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) {
+        error_setg(errp, "can't find a free %d-IRQ block", num);
+        return -1;
+    }
+
+    return first + ics->offset;
+}
-- 
2.13.6

[Qemu-devel] [PATCH v3 3/3] spapr: increase the size of the IRQ number space

[Cédric Le Goater]

The new layout using static IRQ number does not leave much space to
the dynamic MSI range, only 0x100 IRQ numbers. Increase the total
number of IRQS for newer machines and introduce a legacy XICS backend
for pre-3.0 machines to maintain compatibility.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 include/hw/ppc/spapr_irq.h |  1 +
 hw/ppc/spapr.c             |  1 +
 hw/ppc/spapr_irq.c         | 12 +++++++++++-
 3 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
index f2ae0ac0e6cb..f02544934c93 100644
--- a/include/hw/ppc/spapr_irq.h
+++ b/include/hw/ppc/spapr_irq.h
@@ -38,6 +38,7 @@ typedef struct sPAPRIrq {
 } sPAPRIrq;
 
 extern sPAPRIrq spapr_irq_xics;
+extern sPAPRIrq spapr_irq_xics_legacy;
 
 int spapr_irq_claim(sPAPRMachineState *spapr, int irq, bool lsi, Error **errp);
 void spapr_irq_free(sPAPRMachineState *spapr, int irq, int num);
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index ec21df432380..fb3ba90eaefa 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3964,6 +3964,7 @@ static void spapr_machine_2_12_class_options(MachineClass *mc)
 
     spapr_machine_3_0_class_options(mc);
     SET_MACHINE_COMPAT(mc, SPAPR_COMPAT_2_12);
+    smc->irq = &spapr_irq_xics_legacy;
     smc->legacy_irq_allocation = true;
 }
 
diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
index e50a512a2d9c..603541f175bd 100644
--- a/hw/ppc/spapr_irq.c
+++ b/hw/ppc/spapr_irq.c
@@ -197,7 +197,7 @@ static void spapr_irq_print_info_xics(sPAPRMachineState *spapr,
 }
 
 sPAPRIrq spapr_irq_xics = {
-    .nr_irqs     = XICS_IRQS_SPAPR,
+    .nr_irqs     = 0x1000,
 
     .init        = spapr_irq_init_xics,
     .claim       = spapr_irq_claim_xics,
@@ -285,3 +285,13 @@ int spapr_irq_find(sPAPRMachineState *spapr, int num, bool align, Error **errp)
 
     return first + ics->offset;
 }
+
+sPAPRIrq spapr_irq_xics_legacy = {
+    .nr_irqs     = XICS_IRQS_SPAPR,
+
+    .init        = spapr_irq_init_xics,
+    .claim       = spapr_irq_claim_xics,
+    .free        = spapr_irq_free_xics,
+    .qirq        = spapr_qirq_xics,
+    .print_info  = spapr_irq_print_info_xics,
+};
-- 
2.13.6

[Qemu-devel] [PATCH v3 3/3 bonus] spapr: remove the XICS header from the machine and device models

[Cédric Le Goater]

The XICS routines are now completely hidden behind the sPAPR IRQ
interface. There is no need to include the header files anymore.

The only remaining direct use of XICS is done by the sPAPR cores which
need to create the IRQ presenter. This part will be removed when the
XIVE backend is introduced.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 include/hw/ppc/spapr.h     | 2 +-
 include/hw/ppc/spapr_irq.h | 2 ++
 hw/ppc/spapr.c             | 1 -
 hw/ppc/spapr_cpu_core.c    | 1 +
 hw/ppc/spapr_vio.c         | 1 -
 5 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index acc2c01e1123..190d7f04d8d0 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -3,7 +3,6 @@
 
 #include "sysemu/dma.h"
 #include "hw/boards.h"
-#include "hw/ppc/xics.h"
 #include "hw/ppc/spapr_drc.h"
 #include "hw/mem/pc-dimm.h"
 #include "hw/ppc/spapr_ovec.h"
@@ -15,6 +14,7 @@ struct sPAPRNVRAM;
 typedef struct sPAPREventLogEntry sPAPREventLogEntry;
 typedef struct sPAPREventSource sPAPREventSource;
 typedef struct sPAPRPendingHPT sPAPRPendingHPT;
+typedef struct ICSState ICSState;
 
 #define HPTE64_V_HPTE_DIRTY     0x0000000000000040ULL
 #define SPAPR_ENTRY_POINT       0x100
diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
index f02544934c93..0c2ea54ba0a0 100644
--- a/include/hw/ppc/spapr_irq.h
+++ b/include/hw/ppc/spapr_irq.h
@@ -21,6 +21,8 @@
 #define SPAPR_IRQ_MSI        0x1300  /* Offset of the dynamic range covered
                                       * by the bitmap allocator */
 
+typedef struct sPAPRMachineState sPAPRMachineState;
+
 void spapr_irq_msi_init(sPAPRMachineState *spapr, uint32_t nr_msis);
 int spapr_irq_msi_alloc(sPAPRMachineState *spapr, uint32_t num, bool align,
                         Error **errp);
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index fb3ba90eaefa..73f7342edb82 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -54,7 +54,6 @@
 #include "hw/ppc/spapr.h"
 #include "hw/ppc/spapr_vio.h"
 #include "hw/pci-host/spapr.h"
-#include "hw/ppc/xics.h"
 #include "hw/pci/msi.h"
 
 #include "hw/pci/pci.h"
diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index 56fe403fc540..aedb15dd2083 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -11,6 +11,7 @@
 #include "hw/ppc/spapr_cpu_core.h"
 #include "target/ppc/cpu.h"
 #include "hw/ppc/spapr.h"
+#include "hw/ppc/xics.h" /* for icp_create() - to be removed */
 #include "hw/boards.h"
 #include "qapi/error.h"
 #include "sysemu/cpus.h"
diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c
index 2ecee3e8d0ed..b8a80bcc91ca 100644
--- a/hw/ppc/spapr_vio.c
+++ b/hw/ppc/spapr_vio.c
@@ -37,7 +37,6 @@
 
 #include "hw/ppc/spapr.h"
 #include "hw/ppc/spapr_vio.h"
-#include "hw/ppc/xics.h"
 #include "hw/ppc/fdt.h"
 #include "trace.h"
 
-- 
2.13.6

Re: [Qemu-devel] [PATCH v3 1/3] spapr: introduce a fixed IRQ number space

[Cédric Le Goater]

> --- a/hw/ppc/spapr_vio.c
> +++ b/hw/ppc/spapr_vio.c
> @@ -436,6 +436,9 @@ static void spapr_vio_busdev_reset(DeviceState *qdev)
>      }
>  }
>  
> +/* TODO : poor VIO device indexing ... */
> +static uint32_t vio_index;

I think we could also use (dev->reg & 0xff) as an index for 
the VIO devices.

The unit address of the virtual IOA is simply allocated using 
an increment of bus->next_reg, next_reg being initialized at
0x71000000.

I did not see any restrictions in the PAPR specs or in QEMU 
that would break the above.

C.


>  static void spapr_vio_busdev_realize(DeviceState *qdev, Error **errp)
>  {
>      sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> @@ -476,10 +479,18 @@ static void spapr_vio_busdev_realize(DeviceState *qdev, Error **errp)
>      }
>  
>      if (!dev->irq) {
> -        dev->irq = spapr_irq_findone(spapr, &local_err);
> -        if (local_err) {
> -            error_propagate(errp, local_err);
> -            return;
> +        if (SPAPR_MACHINE_GET_CLASS(spapr)->legacy_irq_allocation) {
> +            dev->irq = spapr_irq_findone(spapr, &local_err);
> +            if (local_err) {
> +                error_propagate(errp, local_err);
> +                return;
> +            }
> +        } else {
> +            dev->irq = SPAPR_IRQ_VIO + vio_index++;
> +            if (dev->irq == SPAPR_IRQ_PCI_LSI) {
> +                error_setg(errp, "Too many VIO devices");
> +                return;
> +            }

 

Re: [Qemu-devel] [PATCH v3 1/3] spapr: introduce a fixed IRQ number space

[Cédric Le Goater]

On 07/02/2018 12:03 PM, Cédric Le Goater wrote:
>> --- a/hw/ppc/spapr_vio.c
>> +++ b/hw/ppc/spapr_vio.c
>> @@ -436,6 +436,9 @@ static void spapr_vio_busdev_reset(DeviceState *qdev)
>>      }
>>  }
>>  
>> +/* TODO : poor VIO device indexing ... */
>> +static uint32_t vio_index;
> 
> I think we could also use (dev->reg & 0xff) as an index for 
> the VIO devices.
> 
> The unit address of the virtual IOA is simply allocated using 
> an increment of bus->next_reg, next_reg being initialized at
> 0x71000000.
> 
> I did not see any restrictions in the PAPR specs or in QEMU 
> that would break the above.

That was until I discovered this macro : 

  #define DEFINE_SPAPR_PROPERTIES(type, field)           \
        DEFINE_PROP_UINT32("reg", type, field.reg, -1)


so 'reg' could have any value. We can not use it ...

C.

Re: [Qemu-devel] [PATCH v3 1/3] spapr: introduce a fixed IRQ number space

[Cédric Le Goater]

On 07/02/2018 01:11 PM, Cédric Le Goater wrote:
> On 07/02/2018 12:03 PM, Cédric Le Goater wrote:
>>> --- a/hw/ppc/spapr_vio.c
>>> +++ b/hw/ppc/spapr_vio.c
>>> @@ -436,6 +436,9 @@ static void spapr_vio_busdev_reset(DeviceState *qdev)
>>>      }
>>>  }
>>>  
>>> +/* TODO : poor VIO device indexing ... */
>>> +static uint32_t vio_index;
>>
>> I think we could also use (dev->reg & 0xff) as an index for 
>> the VIO devices.
>>
>> The unit address of the virtual IOA is simply allocated using 
>> an increment of bus->next_reg, next_reg being initialized at
>> 0x71000000.
>>
>> I did not see any restrictions in the PAPR specs or in QEMU 
>> that would break the above.
> 
> That was until I discovered this macro : 
> 
>   #define DEFINE_SPAPR_PROPERTIES(type, field)           \
>         DEFINE_PROP_UINT32("reg", type, field.reg, -1)
>  
> so 'reg' could have any value. We can not use it ...

Would moving vio_index under the bus and incrementing it each time
a VIO device is created be acceptable ? 

It does look like an allocator but I really don't know what else to 
propose :/ See below.

Thanks,

C.


>From 35d206c8768498538ebc74764c65f7a8d59caa33 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?C=C3=A9dric=20Le=20Goater?= <clg@kaod.org>
Date: Tue, 3 Jul 2018 17:17:23 +0200
Subject: [PATCH] spapr/vio: introduce a device index
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

To introduce a static layout of IRQ numbers, we need a clear identifier
for all devices a sPAPR machine can use. The VIO devices have a "reg"
property which can be set to any value by the user (or libvirt) and we
can not rely on it. Add an "index" attribute defined by the bus when
the device is created.

The number of VIO devices is limited to 100 to fit the IRQ range.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 include/hw/ppc/spapr_vio.h |  2 ++
 hw/ppc/spapr_vio.c         | 13 ++++++++++---
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/include/hw/ppc/spapr_vio.h b/include/hw/ppc/spapr_vio.h
index e8b006d18f4e..d95abeffe963 100644
--- a/include/hw/ppc/spapr_vio.h
+++ b/include/hw/ppc/spapr_vio.h
@@ -60,6 +60,7 @@ typedef struct VIOsPAPRDeviceClass {
 
 struct VIOsPAPRDevice {
     DeviceState qdev;
+    uint32_t index;
     uint32_t reg;
     uint32_t irq;
     uint64_t signal_state;
@@ -75,6 +76,7 @@ struct VIOsPAPRDevice {
 
 struct VIOsPAPRBus {
     BusState bus;
+    uint32_t next_index;
     uint32_t next_reg;
 };
 
diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c
index be9af71437cc..4665422c11d2 100644
--- a/hw/ppc/spapr_vio.c
+++ b/hw/ppc/spapr_vio.c
@@ -445,14 +445,23 @@ static void spapr_vio_busdev_reset(DeviceState *qdev)
     }
 }
 
+#define SPAPR_VIO_MAX_DEVICES 0x100
+
 static void spapr_vio_busdev_realize(DeviceState *qdev, Error **errp)
 {
     sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
-    VIOsPAPRDevice *dev = (VIOsPAPRDevice *)qdev;
+    VIOsPAPRBus *bus = SPAPR_VIO_BUS(qdev_get_parent_bus(qdev));
+    VIOsPAPRDevice *dev = VIO_SPAPR_DEVICE(qdev);
     VIOsPAPRDeviceClass *pc = VIO_SPAPR_DEVICE_GET_CLASS(dev);
     char *id;
     Error *local_err = NULL;
 
+    if (bus->next_index == SPAPR_VIO_MAX_DEVICES) {
+        error_setg(errp, "Maximum number of VIO devices reached");
+        return;
+    }
+    dev->index = bus->next_index++;
+
     if (dev->reg != -1) {
         /*
          * Explicitly assigned address, just verify that no-one else
@@ -471,8 +480,6 @@ static void spapr_vio_busdev_realize(DeviceState *qdev, Error **errp)
         }
     } else {
         /* Need to assign an address */
-        VIOsPAPRBus *bus = SPAPR_VIO_BUS(dev->qdev.parent_bus);
-
         do {
             dev->reg = bus->next_reg++;
         } while (reg_conflict(dev));
-- 
2.13.6

Re: [Qemu-devel] [PATCH v3 1/3] spapr: introduce a fixed IRQ number space

[Greg Kurz]

On Tue, 3 Jul 2018 17:19:56 +0200
Cédric Le Goater <clg@kaod.org> wrote:

> On 07/02/2018 01:11 PM, Cédric Le Goater wrote:
> > On 07/02/2018 12:03 PM, Cédric Le Goater wrote:  
> >>> --- a/hw/ppc/spapr_vio.c
> >>> +++ b/hw/ppc/spapr_vio.c
> >>> @@ -436,6 +436,9 @@ static void spapr_vio_busdev_reset(DeviceState *qdev)
> >>>      }
> >>>  }
> >>>  
> >>> +/* TODO : poor VIO device indexing ... */
> >>> +static uint32_t vio_index;  
> >>
> >> I think we could also use (dev->reg & 0xff) as an index for 
> >> the VIO devices.
> >>
> >> The unit address of the virtual IOA is simply allocated using 
> >> an increment of bus->next_reg, next_reg being initialized at
> >> 0x71000000.
> >>
> >> I did not see any restrictions in the PAPR specs or in QEMU 
> >> that would break the above.  
> > 
> > That was until I discovered this macro : 
> > 
> >   #define DEFINE_SPAPR_PROPERTIES(type, field)           \
> >         DEFINE_PROP_UINT32("reg", type, field.reg, -1)
> >  
> > so 'reg' could have any value. We can not use it ...  
> 
> Would moving vio_index under the bus and incrementing it each time
> a VIO device is created be acceptable ? 
> 
> It does look like an allocator but I really don't know what else to 
> propose :/ See below.
> 
> Thanks,
> 
> C.
> 
> 
> From 35d206c8768498538ebc74764c65f7a8d59caa33 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?C=C3=A9dric=20Le=20Goater?= <clg@kaod.org>
> Date: Tue, 3 Jul 2018 17:17:23 +0200
> Subject: [PATCH] spapr/vio: introduce a device index
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
> 
> To introduce a static layout of IRQ numbers, we need a clear identifier
> for all devices a sPAPR machine can use. The VIO devices have a "reg"
> property which can be set to any value by the user (or libvirt) and we
> can not rely on it. Add an "index" attribute defined by the bus when
> the device is created.
> 
> The number of VIO devices is limited to 100 to fit the IRQ range.
                                         0x100

> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---

The patch looks ok to me.

>  include/hw/ppc/spapr_vio.h |  2 ++
>  hw/ppc/spapr_vio.c         | 13 ++++++++++---
>  2 files changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/include/hw/ppc/spapr_vio.h b/include/hw/ppc/spapr_vio.h
> index e8b006d18f4e..d95abeffe963 100644
> --- a/include/hw/ppc/spapr_vio.h
> +++ b/include/hw/ppc/spapr_vio.h
> @@ -60,6 +60,7 @@ typedef struct VIOsPAPRDeviceClass {
>  
>  struct VIOsPAPRDevice {
>      DeviceState qdev;
> +    uint32_t index;
>      uint32_t reg;
>      uint32_t irq;
>      uint64_t signal_state;
> @@ -75,6 +76,7 @@ struct VIOsPAPRDevice {
>  
>  struct VIOsPAPRBus {
>      BusState bus;
> +    uint32_t next_index;
>      uint32_t next_reg;
>  };
>  
> diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c
> index be9af71437cc..4665422c11d2 100644
> --- a/hw/ppc/spapr_vio.c
> +++ b/hw/ppc/spapr_vio.c
> @@ -445,14 +445,23 @@ static void spapr_vio_busdev_reset(DeviceState *qdev)
>      }
>  }
>  
> +#define SPAPR_VIO_MAX_DEVICES 0x100
> +
>  static void spapr_vio_busdev_realize(DeviceState *qdev, Error **errp)
>  {
>      sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> -    VIOsPAPRDevice *dev = (VIOsPAPRDevice *)qdev;
> +    VIOsPAPRBus *bus = SPAPR_VIO_BUS(qdev_get_parent_bus(qdev));
> +    VIOsPAPRDevice *dev = VIO_SPAPR_DEVICE(qdev);
>      VIOsPAPRDeviceClass *pc = VIO_SPAPR_DEVICE_GET_CLASS(dev);
>      char *id;
>      Error *local_err = NULL;
>  
> +    if (bus->next_index == SPAPR_VIO_MAX_DEVICES) {
> +        error_setg(errp, "Maximum number of VIO devices reached");
> +        return;
> +    }
> +    dev->index = bus->next_index++;
> +
>      if (dev->reg != -1) {
>          /*
>           * Explicitly assigned address, just verify that no-one else
> @@ -471,8 +480,6 @@ static void spapr_vio_busdev_realize(DeviceState *qdev, Error **errp)
>          }
>      } else {
>          /* Need to assign an address */
> -        VIOsPAPRBus *bus = SPAPR_VIO_BUS(dev->qdev.parent_bus);
> -
>          do {
>              dev->reg = bus->next_reg++;
>          } while (reg_conflict(dev));

Re: [Qemu-devel] [PATCH v3 1/3] spapr: introduce a fixed IRQ number space

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2297 bytes --]

On Tue, Jul 03, 2018 at 05:19:56PM +0200, Cédric Le Goater wrote:
> On 07/02/2018 01:11 PM, Cédric Le Goater wrote:
> > On 07/02/2018 12:03 PM, Cédric Le Goater wrote:
> >>> --- a/hw/ppc/spapr_vio.c
> >>> +++ b/hw/ppc/spapr_vio.c
> >>> @@ -436,6 +436,9 @@ static void spapr_vio_busdev_reset(DeviceState *qdev)
> >>>      }
> >>>  }
> >>>  
> >>> +/* TODO : poor VIO device indexing ... */
> >>> +static uint32_t vio_index;
> >>
> >> I think we could also use (dev->reg & 0xff) as an index for 
> >> the VIO devices.
> >>
> >> The unit address of the virtual IOA is simply allocated using 
> >> an increment of bus->next_reg, next_reg being initialized at
> >> 0x71000000.
> >>
> >> I did not see any restrictions in the PAPR specs or in QEMU 
> >> that would break the above.
> > 
> > That was until I discovered this macro : 
> > 
> >   #define DEFINE_SPAPR_PROPERTIES(type, field)           \
> >         DEFINE_PROP_UINT32("reg", type, field.reg, -1)
> >  
> > so 'reg' could have any value. We can not use it ...
> 
> Would moving vio_index under the bus and incrementing it each time
> a VIO device is created be acceptable ? 

Not really, no.

> It does look like an allocator but I really don't know what else to 
> propose :/ See below.

Not only is it a stealth allocator, it also means we have two
different unique ids for VIO devices - the 'reg' and this new index.
That sounds like a recipe for confusion.

I think we can do better.  I had a look at how these are allocated and
it seems to be this:

In qemu:
	VIO devices start at reg=0x71000000, and just increment by one
	from there.

In libvirt:
	VIO net devices start at reg=0x1000
	VIO scsi devices start at reg=0x2000
	VIO nvram devices start at reg=0x3000
	VIO vty devices start at reg=0x30000000
	    and increment by 0x1000 each type

So we could go for say:
	irq = (reg & 0xf) ^ ((reg >> 12) & 0xf);

Obviously it's easily to construct cases where that will result in
collisions, but I don't think it'll happen for anyone not going out of
there way to make it happen.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH v3 1/3] spapr: introduce a fixed IRQ number space

[Cédric Le Goater]

On 07/06/2018 07:44 AM, David Gibson wrote:
> On Tue, Jul 03, 2018 at 05:19:56PM +0200, Cédric Le Goater wrote:
>> On 07/02/2018 01:11 PM, Cédric Le Goater wrote:
>>> On 07/02/2018 12:03 PM, Cédric Le Goater wrote:
>>>>> --- a/hw/ppc/spapr_vio.c
>>>>> +++ b/hw/ppc/spapr_vio.c
>>>>> @@ -436,6 +436,9 @@ static void spapr_vio_busdev_reset(DeviceState *qdev)
>>>>>      }
>>>>>  }
>>>>>  
>>>>> +/* TODO : poor VIO device indexing ... */
>>>>> +static uint32_t vio_index;
>>>>
>>>> I think we could also use (dev->reg & 0xff) as an index for 
>>>> the VIO devices.
>>>>
>>>> The unit address of the virtual IOA is simply allocated using 
>>>> an increment of bus->next_reg, next_reg being initialized at
>>>> 0x71000000.
>>>>
>>>> I did not see any restrictions in the PAPR specs or in QEMU 
>>>> that would break the above.
>>>
>>> That was until I discovered this macro : 
>>>
>>>   #define DEFINE_SPAPR_PROPERTIES(type, field)           \
>>>         DEFINE_PROP_UINT32("reg", type, field.reg, -1)
>>>  
>>> so 'reg' could have any value. We can not use it ...
>>
>> Would moving vio_index under the bus and incrementing it each time
>> a VIO device is created be acceptable ? 
> 
> Not really, no.
> 
>> It does look like an allocator but I really don't know what else to 
>> propose :/ See below.
> 
> Not only is it a stealth allocator, it also means we have two
> different unique ids for VIO devices - the 'reg' and this new index.
> That sounds like a recipe for confusion.

I agree.

> I think we can do better.  I had a look at how these are allocated and
> it seems to be this:
> 
> In qemu:
> 	VIO devices start at reg=0x71000000, and just increment by one
> 	from there.
> 
> In libvirt:
> 	VIO net devices start at reg=0x1000
> 	VIO scsi devices start at reg=0x2000
> 	VIO nvram devices start at reg=0x3000
> 	VIO vty devices start at reg=0x30000000
> 	    and increment by 0x1000 each type
> 
> So we could go for say:
> 	irq = (reg & 0xf) ^ ((reg >> 12) & 0xf);

We should use 0xff in the formula above to cover 256 devices. So we are 
looking for an index in the bit ranges [0-7] or [12-19]. Ok, I didn't 
dare to do that but let's go that way. I will send a v4 before going.

> Obviously it's easily to construct cases where that will result in
> collisions, but I don't think it'll happen for anyone not going out of
> there way to make it happen.

We should also deprecate the "reg" property in 3.1.

C.
 

Re: [Qemu-devel] [PATCH v3 1/3] spapr: introduce a fixed IRQ number space

[Cédric Le Goater]

On 07/06/2018 07:44 AM, David Gibson wrote:
> On Tue, Jul 03, 2018 at 05:19:56PM +0200, Cédric Le Goater wrote:
>> On 07/02/2018 01:11 PM, Cédric Le Goater wrote:
>>> On 07/02/2018 12:03 PM, Cédric Le Goater wrote:
>>>>> --- a/hw/ppc/spapr_vio.c
>>>>> +++ b/hw/ppc/spapr_vio.c
>>>>> @@ -436,6 +436,9 @@ static void spapr_vio_busdev_reset(DeviceState *qdev)
>>>>>      }
>>>>>  }
>>>>>  
>>>>> +/* TODO : poor VIO device indexing ... */
>>>>> +static uint32_t vio_index;
>>>>
>>>> I think we could also use (dev->reg & 0xff) as an index for 
>>>> the VIO devices.
>>>>
>>>> The unit address of the virtual IOA is simply allocated using 
>>>> an increment of bus->next_reg, next_reg being initialized at
>>>> 0x71000000.
>>>>
>>>> I did not see any restrictions in the PAPR specs or in QEMU 
>>>> that would break the above.
>>>
>>> That was until I discovered this macro : 
>>>
>>>   #define DEFINE_SPAPR_PROPERTIES(type, field)           \
>>>         DEFINE_PROP_UINT32("reg", type, field.reg, -1)
>>>  
>>> so 'reg' could have any value. We can not use it ...
>>
>> Would moving vio_index under the bus and incrementing it each time
>> a VIO device is created be acceptable ? 
> 
> Not really, no.
> 
>> It does look like an allocator but I really don't know what else to 
>> propose :/ See below.
> 
> Not only is it a stealth allocator, it also means we have two
> different unique ids for VIO devices - the 'reg' and this new index.
> That sounds like a recipe for confusion.
> 
> I think we can do better.  I had a look at how these are allocated and
> it seems to be this:
> 
> In qemu:
> 	VIO devices start at reg=0x71000000, and just increment by one
> 	from there.
> 
> In libvirt:
> 	VIO net devices start at reg=0x1000
> 	VIO scsi devices start at reg=0x2000
> 	VIO nvram devices start at reg=0x3000

but a default VIO nvram device is always created by the machine. Here is 
a typical /vdevice layout :

  drwxr-xr-x. 2 root root 0 Jul  2 04:22 /proc/device-tree/vdevice/nvram@71000000
  drwxr-xr-x. 2 root root 0 Jul  2 04:22 /proc/device-tree/vdevice/vty@30000000

which is going to have collisions.

Should we set the "register" property of the defaut nvram device to some 
high value ? the sPAPR platform expects to always have a nvram device:

    R1–8.1–1.

      Platforms must implement at least 8 KB of Non-Volatile Memory. 
      The actual amount is platform dependent and must allow for 4 KB 
      for the OS. Platforms must provide an additional 4 KB for each 
      installed OS beyond the first.

So we can not remove it. 

The vty devices are dependent on the chardev backends. We are fine on that
side.

Thanks,

C.

> 	VIO vty devices start at reg=0x30000000
> 	    and increment by 0x1000 each type
> 
> So we could go for say:
> 	irq = (reg & 0xf) ^ ((reg >> 12) & 0xf);
> 
> Obviously it's easily to construct cases where that will result in
> collisions, but I don't think it'll happen for anyone not going out of
> there way to make it happen.
> 

Re: [Qemu-devel] [Qemu-ppc] [PATCH v3 1/3] spapr: introduce a fixed IRQ number space

[Cédric Le Goater]

On 07/06/2018 09:40 AM, Cédric Le Goater wrote:
> On 07/06/2018 07:44 AM, David Gibson wrote:
>> On Tue, Jul 03, 2018 at 05:19:56PM +0200, Cédric Le Goater wrote:
>>> On 07/02/2018 01:11 PM, Cédric Le Goater wrote:
>>>> On 07/02/2018 12:03 PM, Cédric Le Goater wrote:
>>>>>> --- a/hw/ppc/spapr_vio.c
>>>>>> +++ b/hw/ppc/spapr_vio.c
>>>>>> @@ -436,6 +436,9 @@ static void spapr_vio_busdev_reset(DeviceState *qdev)
>>>>>>      }
>>>>>>  }
>>>>>>  
>>>>>> +/* TODO : poor VIO device indexing ... */
>>>>>> +static uint32_t vio_index;
>>>>>
>>>>> I think we could also use (dev->reg & 0xff) as an index for 
>>>>> the VIO devices.
>>>>>
>>>>> The unit address of the virtual IOA is simply allocated using 
>>>>> an increment of bus->next_reg, next_reg being initialized at
>>>>> 0x71000000.
>>>>>
>>>>> I did not see any restrictions in the PAPR specs or in QEMU 
>>>>> that would break the above.
>>>>
>>>> That was until I discovered this macro : 
>>>>
>>>>   #define DEFINE_SPAPR_PROPERTIES(type, field)           \
>>>>         DEFINE_PROP_UINT32("reg", type, field.reg, -1)
>>>>  
>>>> so 'reg' could have any value. We can not use it ...
>>>
>>> Would moving vio_index under the bus and incrementing it each time
>>> a VIO device is created be acceptable ? 
>>
>> Not really, no.
>>
>>> It does look like an allocator but I really don't know what else to 
>>> propose :/ See below.
>>
>> Not only is it a stealth allocator, it also means we have two
>> different unique ids for VIO devices - the 'reg' and this new index.
>> That sounds like a recipe for confusion.
>>
>> I think we can do better.  I had a look at how these are allocated and
>> it seems to be this:
>>
>> In qemu:
>> 	VIO devices start at reg=0x71000000, and just increment by one
>> 	from there.
>>
>> In libvirt:
>> 	VIO net devices start at reg=0x1000
>> 	VIO scsi devices start at reg=0x2000
>> 	VIO nvram devices start at reg=0x3000
> 
> but a default VIO nvram device is always created by the machine. Here is 
> a typical /vdevice layout :
> 
>   drwxr-xr-x. 2 root root 0 Jul  2 04:22 /proc/device-tree/vdevice/nvram@71000000
>   drwxr-xr-x. 2 root root 0 Jul  2 04:22 /proc/device-tree/vdevice/vty@30000000
> 
> which is going to have collisions.

maybe we could split the VIO index number space and use [0x0 - 0x7f] for 
the user defined  "reg" values and [0x80-0xff] the values allocated by the
QEMU VIO model, 0x71000000 and above ? 

C. 

 
> 
> Should we set the "register" property of the defaut nvram device to some 
> high value ? the sPAPR platform expects to always have a nvram device:
> 
>     R1–8.1–1.
> 
>       Platforms must implement at least 8 KB of Non-Volatile Memory. 
>       The actual amount is platform dependent and must allow for 4 KB 
>       for the OS. Platforms must provide an additional 4 KB for each 
>       installed OS beyond the first.
> 
> So we can not remove it. 
> 
> The vty devices are dependent on the chardev backends. We are fine on that
> side.
> 
> Thanks,
> 
> C.
> 
>> 	VIO vty devices start at reg=0x30000000
>> 	    and increment by 0x1000 each type
>>
>> So we could go for say:
>> 	irq = (reg & 0xf) ^ ((reg >> 12) & 0xf);
>>
>> Obviously it's easily to construct cases where that will result in
>> collisions, but I don't think it'll happen for anyone not going out of
>> there way to make it happen.
>>
> 
>