* [Qemu-devel] [PATCH 0/4] spapr: generic IRQ frontend
@ 2018-05-18 16:44 Cédric Le Goater
2018-05-18 16:44 ` [Qemu-devel] [PATCH 1/4] spapr: remove irq_hint parameter from spapr_irq_alloc() Cédric Le Goater
` (3 more replies)
0 siblings, 4 replies; 32+ messages in thread
From: Cédric Le Goater @ 2018-05-18 16:44 UTC (permalink / raw)
To: qemu-ppc
Cc: qemu-devel, David Gibson, Greg Kurz, Alexey Kardashevskiy,
Cédric Le Goater
Hello,
This proposal moves all the related IRQ routines of the sPAPR machine
behind a class interface to prepare for future changes in the IRQ
controller model. First of which is a reorganization of the IRQ number
space layout and a second, coming later, will be to integrate the
support for the new POWER9 XIVE IRQ controller.
The new interface defines a set of fixed IRQ number ranges, for each
IRQ type, in which devices allocate the IRQ numbers they need
depending on a unique device index. Here is the layout :
RANGES DEVICES
0x0000 - 0x0FFF Reserved for future use (IPI = 2)
0x1000 - 0x1000 1 EPOW
0x1001 - 0x1001 1 HOTPLUG
0x1002 - 0x10FF unused
0x1100 - 0x11FF 256 VIO devices (1 IRQ each)
0x1200 - 0x1283 32 PCI LSI devices (4 IRQs each)
0x1284 - 0x13FF unused
0x1400 - 0x17FF PCI MSI device 1 (1024 IRQs each)
0x1800 - 0x1BFF PCI MSI device 1
0x1c00 - 0x1FFF PCI MSI device 2
0x2000 .... not allocated. Need to increase NR_IRQS
Each device model is modified to take the new interface into account
using the IRQ range/type definitions and a device index. A part from
the VIO devices, lacking an id, the changes are relatively simple.
The MSI range is a bit more complex to handle as the IRQS are dynamically
allocated by the guest OS. In consequence, we use a bitmap allocator
under the machine for these.
The XICS IRQ number space is increased to 4K, which gives three MSI
ranges of 1K for the PHBs. The XICS source IRQ numbers still have the
4K offset.
Thanks,
C.
Cédric Le Goater (4):
spapr: remove irq_hint parameter from spapr_irq_alloc()
sparp_pci: simplify how the PCI LSIs are allocated
spapr: introduce a generic IRQ frontend to the machine
spapr: introduce a new IRQ backend using fixed IRQ number ranges
include/hw/ppc/spapr.h | 13 +-
include/hw/ppc/spapr_irq.h | 58 ++++++
hw/ppc/spapr.c | 208 +++------------------
hw/ppc/spapr_events.c | 8 +-
hw/ppc/spapr_irq.c | 451 +++++++++++++++++++++++++++++++++++++++++++++
hw/ppc/spapr_pci.c | 40 ++--
hw/ppc/spapr_vio.c | 5 +-
hw/ppc/Makefile.objs | 2 +-
8 files changed, 571 insertions(+), 214 deletions(-)
create mode 100644 include/hw/ppc/spapr_irq.h
create mode 100644 hw/ppc/spapr_irq.c
--
2.13.6
^ permalink raw reply [flat|nested] 32+ messages in thread
* [Qemu-devel] [PATCH 1/4] spapr: remove irq_hint parameter from spapr_irq_alloc()
2018-05-18 16:44 [Qemu-devel] [PATCH 0/4] spapr: generic IRQ frontend Cédric Le Goater
@ 2018-05-18 16:44 ` Cédric Le Goater
2018-05-25 14:02 ` Greg Kurz
2018-06-02 9:10 ` Cédric Le Goater
2018-05-18 16:44 ` [Qemu-devel] [PATCH 2/4] sparp_pci: simplify how the PCI LSIs are allocated Cédric Le Goater
` (2 subsequent siblings)
3 siblings, 2 replies; 32+ messages in thread
From: Cédric Le Goater @ 2018-05-18 16:44 UTC (permalink / raw)
To: qemu-ppc
Cc: qemu-devel, David Gibson, Greg Kurz, Alexey Kardashevskiy,
Cédric Le Goater
This IRQ number hint can possibly be used by the VIO devices if the
"irq" property is defined on the command line but it seems it is never
the case. It is not used in libvirt for instance. So, let's remove it
to simplify future changes.
Nevertheless, this is a compatibbility breakage that will be addressed
by the subsequent patches which introduce IRQ number allocator
handlers per machine version.
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
include/hw/ppc/spapr.h | 3 +--
hw/ppc/spapr.c | 21 ++++++---------------
hw/ppc/spapr_events.c | 7 +++----
hw/ppc/spapr_vio.c | 2 +-
4 files changed, 11 insertions(+), 22 deletions(-)
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index d60b7c6d7a8b..2cfdfdd67eaf 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -773,8 +773,7 @@ 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_alloc(sPAPRMachineState *spapr, int irq_hint, bool lsi,
- Error **errp);
+int spapr_irq_alloc(sPAPRMachineState *spapr, bool lsi, Error **errp);
int spapr_irq_alloc_block(sPAPRMachineState *spapr, int num, bool lsi,
bool align, 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 32ab3c43b6c0..05a924a5f2da 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3798,28 +3798,19 @@ static void spapr_irq_set_lsi(sPAPRMachineState *spapr, int irq, bool lsi)
ics_set_irq_type(spapr->ics, irq - spapr->ics->offset, lsi);
}
-int spapr_irq_alloc(sPAPRMachineState *spapr, int irq_hint, bool lsi,
- Error **errp)
+int spapr_irq_alloc(sPAPRMachineState *spapr, bool lsi, Error **errp)
{
ICSState *ics = spapr->ics;
int irq;
assert(ics);
- if (irq_hint) {
- if (!ICS_IRQ_FREE(ics, irq_hint - ics->offset)) {
- error_setg(errp, "can't allocate IRQ %d: already in use", irq_hint);
- return -1;
- }
- irq = irq_hint;
- } else {
- irq = ics_find_free_block(ics, 1, 1);
- if (irq < 0) {
- error_setg(errp, "can't allocate IRQ: no IRQ left");
- return -1;
- }
- irq += ics->offset;
+ irq = ics_find_free_block(ics, 1, 1);
+ if (irq < 0) {
+ error_setg(errp, "can't allocate IRQ: no IRQ left");
+ return -1;
}
+ irq += ics->offset;
spapr_irq_set_lsi(spapr, irq, lsi);
trace_spapr_irq_alloc(irq);
diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
index 86836f0626dc..64a67439beac 100644
--- a/hw/ppc/spapr_events.c
+++ b/hw/ppc/spapr_events.c
@@ -712,8 +712,7 @@ void spapr_events_init(sPAPRMachineState *spapr)
spapr->event_sources = spapr_event_sources_new();
spapr_event_sources_register(spapr->event_sources, EVENT_CLASS_EPOW,
- spapr_irq_alloc(spapr, 0, false,
- &error_fatal));
+ spapr_irq_alloc(spapr, false, &error_fatal));
/* NOTE: if machine supports modern/dedicated hotplug event source,
* we add it to the device-tree unconditionally. This means we may
@@ -725,8 +724,8 @@ void spapr_events_init(sPAPRMachineState *spapr)
*/
if (spapr->use_hotplug_event_source) {
spapr_event_sources_register(spapr->event_sources, EVENT_CLASS_HOT_PLUG,
- spapr_irq_alloc(spapr, 0, false,
- &error_fatal));
+ spapr_irq_alloc(spapr, false,
+ &error_fatal));
}
spapr->epow_notifier.notify = spapr_powerdown_req;
diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c
index 472dd6f33a96..cc064f64fccf 100644
--- a/hw/ppc/spapr_vio.c
+++ b/hw/ppc/spapr_vio.c
@@ -455,7 +455,7 @@ static void spapr_vio_busdev_realize(DeviceState *qdev, Error **errp)
dev->qdev.id = id;
}
- dev->irq = spapr_irq_alloc(spapr, dev->irq, false, &local_err);
+ dev->irq = spapr_irq_alloc(spapr, false, &local_err);
if (local_err) {
error_propagate(errp, local_err);
return;
--
2.13.6
^ permalink raw reply related [flat|nested] 32+ messages in thread
* [Qemu-devel] [PATCH 2/4] sparp_pci: simplify how the PCI LSIs are allocated
2018-05-18 16:44 [Qemu-devel] [PATCH 0/4] spapr: generic IRQ frontend Cédric Le Goater
2018-05-18 16:44 ` [Qemu-devel] [PATCH 1/4] spapr: remove irq_hint parameter from spapr_irq_alloc() Cédric Le Goater
@ 2018-05-18 16:44 ` Cédric Le Goater
2018-05-26 9:40 ` Greg Kurz
2018-05-18 16:44 ` [Qemu-devel] [PATCH 3/4] spapr: introduce a generic IRQ frontend to the machine Cédric Le Goater
2018-05-18 16:44 ` [Qemu-devel] [PATCH 4/4] spapr: introduce a new IRQ backend using fixed IRQ number ranges Cédric Le Goater
3 siblings, 1 reply; 32+ messages in thread
From: Cédric Le Goater @ 2018-05-18 16:44 UTC (permalink / raw)
To: qemu-ppc
Cc: qemu-devel, David Gibson, Greg Kurz, Alexey Kardashevskiy,
Cédric Le Goater
PCI LSIs are today allocated one by one using the IRQ alloc_block
routine. Change the code sequence to first allocate a PCI_NUM_PINS
block. It will help us providing a generic IRQ framework to the
machine.
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
hw/ppc/spapr_pci.c | 21 ++++++++++-----------
1 file changed, 10 insertions(+), 11 deletions(-)
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 39a14980d397..4fd97ffe4c6e 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1546,6 +1546,8 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
sPAPRTCETable *tcet;
const unsigned windows_supported =
sphb->ddw_enabled ? SPAPR_PCI_DMA_MAX_WINDOWS : 1;
+ uint32_t irq;
+ Error *local_err = NULL;
if (!spapr) {
error_setg(errp, TYPE_SPAPR_PCI_HOST_BRIDGE " needs a pseries machine");
@@ -1694,18 +1696,15 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
QLIST_INSERT_HEAD(&spapr->phbs, sphb, list);
/* Initialize the LSI table */
- for (i = 0; i < PCI_NUM_PINS; i++) {
- uint32_t irq;
- Error *local_err = NULL;
-
- irq = spapr_irq_alloc_block(spapr, 1, true, false, &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
- error_prepend(errp, "can't allocate LSIs: ");
- return;
- }
+ irq = spapr_irq_alloc_block(spapr, PCI_NUM_PINS, true, false, &local_err);
+ if (local_err) {
+ error_propagate(errp, local_err);
+ error_prepend(errp, "can't allocate LSIs: ");
+ return;
+ }
- sphb->lsi_table[i].irq = irq;
+ for (i = 0; i < PCI_NUM_PINS; i++) {
+ sphb->lsi_table[i].irq = irq + i;
}
/* allocate connectors for child PCI devices */
--
2.13.6
^ permalink raw reply related [flat|nested] 32+ messages in thread
* [Qemu-devel] [PATCH 3/4] spapr: introduce a generic IRQ frontend to the machine
2018-05-18 16:44 [Qemu-devel] [PATCH 0/4] spapr: generic IRQ frontend Cédric Le Goater
2018-05-18 16:44 ` [Qemu-devel] [PATCH 1/4] spapr: remove irq_hint parameter from spapr_irq_alloc() Cédric Le Goater
2018-05-18 16:44 ` [Qemu-devel] [PATCH 2/4] sparp_pci: simplify how the PCI LSIs are allocated Cédric Le Goater
@ 2018-05-18 16:44 ` Cédric Le Goater
2018-05-28 14:27 ` Greg Kurz
2018-06-13 5:00 ` David Gibson
2018-05-18 16:44 ` [Qemu-devel] [PATCH 4/4] spapr: introduce a new IRQ backend using fixed IRQ number ranges Cédric Le Goater
3 siblings, 2 replies; 32+ messages in thread
From: Cédric Le Goater @ 2018-05-18 16:44 UTC (permalink / raw)
To: qemu-ppc
Cc: qemu-devel, David Gibson, Greg Kurz, Alexey Kardashevskiy,
Cédric Le Goater
This proposal moves all the related IRQ routines of the sPAPR machine
behind a class interface to prepare for future changes in the IRQ
controller model. First of which is a reorganization of the IRQ number
space layout and a second, coming later, will be to integrate the
support for the new POWER9 XIVE IRQ controller.
The new interface defines a set of fixed IRQ number ranges, for each
IRQ type, in which devices allocate the IRQ numbers they need
depending on a unique device index. Here is the layout :
SPAPR_IRQ_IPI 0x0 /* 1 IRQ per CPU */
SPAPR_IRQ_EPOW 0x1000 /* 1 IRQ per device */
SPAPR_IRQ_HOTPLUG 0x1001 /* 1 IRQ per device */
SPAPR_IRQ_VIO 0x1100 /* 1 IRQ per device */
SPAPR_IRQ_PCI_LSI 0x1200 /* 4 IRQs per device */
SPAPR_IRQ_PCI_MSI 0x1400 /* 1K IRQs per device */
The IPI range is reserved for future use when XIVE support
comes in.
The routines of this interface encompass the previous needs and the
new ones and seem complex but the provided IRQ backend should
implement what we have today without any functional changes.
Each device model is modified to take the new interface into account
using the IRQ range/type definitions and a device index. A part from
the VIO devices, lacking an id, the changes are relatively simple.
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
include/hw/ppc/spapr.h | 10 +-
include/hw/ppc/spapr_irq.h | 46 +++++++++
hw/ppc/spapr.c | 177 +---------------------------------
hw/ppc/spapr_events.c | 7 +-
hw/ppc/spapr_irq.c | 233 +++++++++++++++++++++++++++++++++++++++++++++
hw/ppc/spapr_pci.c | 21 +++-
hw/ppc/spapr_vio.c | 5 +-
hw/ppc/Makefile.objs | 2 +-
8 files changed, 308 insertions(+), 193 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 2cfdfdd67eaf..4eb212b16a51 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -3,10 +3,10 @@
#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"
+#include "hw/ppc/spapr_irq.h"
struct VIOsPAPRBus;
struct sPAPRPHBState;
@@ -104,6 +104,7 @@ struct sPAPRMachineClass {
unsigned n_dma, uint32_t *liobns, Error **errp);
sPAPRResizeHPT resize_hpt_default;
sPAPRCapabilities default_caps;
+ sPAPRIrq *irq;
};
/**
@@ -773,13 +774,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_alloc(sPAPRMachineState *spapr, bool lsi, Error **errp);
-int spapr_irq_alloc_block(sPAPRMachineState *spapr, int num, bool lsi,
- bool align, 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
new file mode 100644
index 000000000000..caf4c33d4cec
--- /dev/null
+++ b/include/hw/ppc/spapr_irq.h
@@ -0,0 +1,46 @@
+/*
+ * 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
+
+#include "hw/ppc/xics.h"
+
+/*
+ * IRQ ranges
+ */
+#define SPAPR_IRQ_IPI 0x0 /* 1 IRQ per CPU */
+#define SPAPR_IRQ_EPOW 0x1000 /* 1 IRQ per device */
+#define SPAPR_IRQ_HOTPLUG 0x1001 /* 1 IRQ per device */
+#define SPAPR_IRQ_VIO 0x1100 /* 1 IRQ per device */
+#define SPAPR_IRQ_PCI_LSI 0x1200 /* 4 IRQs per device */
+#define SPAPR_IRQ_PCI_MSI 0x1400 /* 1K IRQs per device covered by
+ * a bitmap allocator */
+
+typedef struct sPAPRIrq {
+ uint32_t nr_irqs;
+
+ void (*init)(sPAPRMachineState *spapr, Error **errp);
+ int (*alloc)(sPAPRMachineState *spapr, uint32_t range, uint32_t index,
+ Error **errp);
+ int (*alloc_block)(sPAPRMachineState *spapr, uint32_t range,
+ uint32_t index, int num, bool align, Error **errp);
+ void (*free)(sPAPRMachineState *spapr, int irq, int num, Error **errp);
+ qemu_irq (*qirq)(sPAPRMachineState *spapr, int irq);
+} sPAPRIrq;
+
+extern sPAPRIrq spapr_irq_default;
+
+int spapr_irq_alloc(sPAPRMachineState *spapr, uint32_t range, uint32_t index,
+ Error **errp);
+int spapr_irq_alloc_block(sPAPRMachineState *spapr, uint32_t range,
+ uint32_t index, int num, bool align, Error **errp);
+void spapr_irq_free(sPAPRMachineState *spapr, int irq, int num, Error **errp);
+qemu_irq spapr_qirq(sPAPRMachineState *spapr, int irq);
+
+#endif /* HW_SPAPR_IRQ_H */
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 05a924a5f2da..09f095d73eae 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,32 +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);
-
- if (kvm_enabled()) {
- if (machine_kernel_irqchip_allowed(machine) &&
- !xics_kvm_init(spapr, errp)) {
- spapr->icp_type = TYPE_KVM_ICP;
- spapr->ics = spapr_ics_create(spapr, TYPE_ICS_KVM, nr_irqs, errp);
- }
- if (machine_kernel_irqchip_required(machine) && !spapr->ics) {
- error_prepend(errp, "kernel_irqchip requested but unavailable: ");
- return;
- }
- }
-
- if (!spapr->ics) {
- xics_spapr_init(spapr);
- spapr->icp_type = TYPE_ICP;
- spapr->ics = spapr_ics_create(spapr, TYPE_ICS_SIMPLE, nr_irqs, errp);
- if (!spapr->ics) {
- return;
- }
- }
-}
-
static int spapr_fixup_cpu_smt_dt(void *fdt, int offset, PowerPCCPU *cpu,
int smt_threads)
{
@@ -2580,7 +2527,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
*/
@@ -3766,127 +3713,6 @@ 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;
-}
-
-/*
- * Allocate the IRQ number and set the IRQ type, LSI or MSI
- */
-static void spapr_irq_set_lsi(sPAPRMachineState *spapr, int irq, bool lsi)
-{
- ics_set_irq_type(spapr->ics, irq - spapr->ics->offset, lsi);
-}
-
-int spapr_irq_alloc(sPAPRMachineState *spapr, bool lsi, Error **errp)
-{
- ICSState *ics = spapr->ics;
- int irq;
-
- assert(ics);
-
- irq = ics_find_free_block(ics, 1, 1);
- if (irq < 0) {
- error_setg(errp, "can't allocate IRQ: no IRQ left");
- return -1;
- }
- irq += ics->offset;
-
- spapr_irq_set_lsi(spapr, irq, lsi);
- trace_spapr_irq_alloc(irq);
-
- return irq;
-}
-
-/*
- * Allocate block of consecutive IRQs, and return the number of the first IRQ in
- * the block. If align==true, aligns the first IRQ number to num.
- */
-int spapr_irq_alloc_block(sPAPRMachineState *spapr, int num, bool lsi,
- bool align, Error **errp)
-{
- ICSState *ics = spapr->ics;
- int i, 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;
- }
-
- first += ics->offset;
- for (i = first; i < first + num; ++i) {
- spapr_irq_set_lsi(spapr, i, lsi);
- }
-
- trace_spapr_irq_alloc_block(first, num, lsi, align);
-
- return first;
-}
-
-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)
{
@@ -4007,6 +3833,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_default;
}
static const TypeInfo spapr_machine_info = {
diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
index 64a67439beac..e457c5f18189 100644
--- a/hw/ppc/spapr_events.c
+++ b/hw/ppc/spapr_events.c
@@ -712,7 +712,8 @@ void spapr_events_init(sPAPRMachineState *spapr)
spapr->event_sources = spapr_event_sources_new();
spapr_event_sources_register(spapr->event_sources, EVENT_CLASS_EPOW,
- spapr_irq_alloc(spapr, false, &error_fatal));
+ spapr_irq_alloc(spapr, SPAPR_IRQ_EPOW, 0,
+ &error_fatal));
/* NOTE: if machine supports modern/dedicated hotplug event source,
* we add it to the device-tree unconditionally. This means we may
@@ -724,8 +725,8 @@ void spapr_events_init(sPAPRMachineState *spapr)
*/
if (spapr->use_hotplug_event_source) {
spapr_event_sources_register(spapr->event_sources, EVENT_CLASS_HOT_PLUG,
- spapr_irq_alloc(spapr, false,
- &error_fatal));
+ spapr_irq_alloc(spapr, SPAPR_IRQ_HOTPLUG,
+ 0, &error_fatal));
}
spapr->epow_notifier.notify = spapr_powerdown_req;
diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
new file mode 100644
index 000000000000..ff6cb1aafd25
--- /dev/null
+++ b/hw/ppc/spapr_irq.c
@@ -0,0 +1,233 @@
+/*
+ * QEMU PowerPC sPAPR IRQ backend
+ *
+ * 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 "qapi/error.h"
+#include "hw/pci/pci.h"
+#include "hw/ppc/spapr.h"
+#include "sysemu/kvm.h"
+#include "trace.h"
+
+/*
+ * Legacy XICS IRQ backend.
+ *
+ * The device IRQ 'range' is used to identify LSIs, and the device
+ * 'index' is unused
+ */
+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_2_12(sPAPRMachineState *spapr, Error **errp)
+{
+ MachineState *machine = MACHINE(spapr);
+ sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(machine);
+ uint32_t nr_irqs = smc->irq->nr_irqs;
+
+ if (kvm_enabled()) {
+ if (machine_kernel_irqchip_allowed(machine) &&
+ !xics_kvm_init(spapr, errp)) {
+ spapr->icp_type = TYPE_KVM_ICP;
+ spapr->ics = spapr_ics_create(spapr, TYPE_ICS_KVM, nr_irqs, errp);
+ }
+ if (machine_kernel_irqchip_required(machine) && !spapr->ics) {
+ error_prepend(errp, "kernel_irqchip requested but unavailable: ");
+ return;
+ }
+ }
+
+ if (!spapr->ics) {
+ xics_spapr_init(spapr);
+ spapr->icp_type = TYPE_ICP;
+ spapr->ics = spapr_ics_create(spapr, TYPE_ICS_SIMPLE, nr_irqs, errp);
+ if (!spapr->ics) {
+ return;
+ }
+ }
+}
+
+#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;
+}
+
+static int spapr_irq_alloc_2_12(sPAPRMachineState *spapr,
+ uint32_t range, uint32_t index, Error **errp)
+{
+ ICSState *ics = spapr->ics;
+ bool lsi = (range == SPAPR_IRQ_PCI_LSI);
+ int srcno;
+
+ assert(ics);
+
+ srcno = ics_find_free_block(ics, 1, 1);
+ if (srcno < 0) {
+ error_setg(errp, "can't allocate IRQ: no IRQ left");
+ return -1;
+ }
+
+ ics_set_irq_type(ics, srcno, lsi);
+ trace_spapr_irq_alloc(srcno);
+
+ return ics->offset + srcno;
+}
+
+/*
+ * Allocate block of consecutive IRQs, and return the number of the first IRQ in
+ * the block. If align==true, aligns the first IRQ number to num.
+ */
+static int spapr_irq_alloc_block_2_12(sPAPRMachineState *spapr, uint32_t range,
+ uint32_t index, int num, bool align,
+ Error **errp)
+{
+ ICSState *ics = spapr->ics;
+ bool lsi = (range == SPAPR_IRQ_PCI_LSI);
+ int i, srcno;
+
+ assert(ics);
+
+ /*
+ * MSIMessage::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.
+ */
+ if (align) {
+ assert((num == 1) || (num == 2) || (num == 4) ||
+ (num == 8) || (num == 16) || (num == 32));
+ srcno = ics_find_free_block(ics, num, num);
+ } else {
+ srcno = ics_find_free_block(ics, num, 1);
+ }
+
+ if (srcno < 0) {
+ error_setg(errp, "can't find a free %d-IRQ block", num);
+ return -1;
+ }
+
+ for (i = srcno; i < srcno + num; ++i) {
+ ics_set_irq_type(ics, i, lsi);
+ }
+
+ trace_spapr_irq_alloc_block(srcno, num, lsi, align);
+
+ return ics->offset + srcno;
+}
+
+static void spapr_irq_free_2_12(sPAPRMachineState *spapr, int irq, int num,
+ Error **errp)
+{
+ 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_2_12(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;
+}
+
+sPAPRIrq spapr_irq_default = {
+ .nr_irqs = XICS_IRQS_SPAPR,
+ .init = spapr_irq_init_2_12,
+ .alloc = spapr_irq_alloc_2_12,
+ .alloc_block = spapr_irq_alloc_block_2_12,
+ .free = spapr_irq_free_2_12,
+ .qirq = spapr_qirq_2_12,
+};
+
+int spapr_irq_alloc(sPAPRMachineState *spapr, uint32_t range, uint32_t index,
+ Error **errp)
+{
+ sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
+
+ return smc->irq->alloc(spapr, range, index, errp);
+}
+
+int spapr_irq_alloc_block(sPAPRMachineState *spapr, uint32_t range,
+ uint32_t index, int num, bool align, Error **errp)
+{
+ sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
+
+ return smc->irq->alloc_block(spapr, range, index, num, align, errp);
+}
+
+void spapr_irq_free(sPAPRMachineState *spapr, int irq, int num,
+ Error **errp)
+{
+ sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
+
+ smc->irq->free(spapr, irq, num, errp);
+}
+
+qemu_irq spapr_qirq(sPAPRMachineState *spapr, int irq)
+{
+ sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
+
+ return smc->irq->qirq(spapr, irq);
+}
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 4fd97ffe4c6e..cca4169fa10b 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -333,7 +333,12 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
return;
}
- spapr_irq_free(spapr, msi->first_irq, msi->num);
+ spapr_irq_free(spapr, msi->first_irq, msi->num, &err);
+ if (err) {
+ error_reportf_err(err, "Can't remove MSIs for device %x: ",
+ config_addr);
+ rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
+ }
if (msi_present(pdev)) {
spapr_msi_setmsg(pdev, 0, false, 0, 0);
}
@@ -371,8 +376,8 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
}
/* Allocate MSIs */
- irq = spapr_irq_alloc_block(spapr, req_num, false,
- ret_intr_type == RTAS_TYPE_MSI, &err);
+ irq = spapr_irq_alloc_block(spapr, SPAPR_IRQ_PCI_MSI, phb->index, req_num,
+ ret_intr_type == RTAS_TYPE_MSI, &err);
if (err) {
error_reportf_err(err, "Can't allocate MSIs for device %x: ",
config_addr);
@@ -382,7 +387,11 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
/* Release previous MSIs */
if (msi) {
- spapr_irq_free(spapr, msi->first_irq, msi->num);
+ spapr_irq_free(spapr, msi->first_irq, msi->num, &err);
+ if (err) {
+ error_reportf_err(err, "Can't remove MSIs for device %x: ",
+ config_addr);
+ }
g_hash_table_remove(phb->msi, &config_addr);
}
@@ -1696,7 +1705,8 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
QLIST_INSERT_HEAD(&spapr->phbs, sphb, list);
/* Initialize the LSI table */
- irq = spapr_irq_alloc_block(spapr, PCI_NUM_PINS, true, false, &local_err);
+ irq = spapr_irq_alloc_block(spapr, SPAPR_IRQ_PCI_LSI, sphb->index,
+ PCI_NUM_PINS, false, &local_err);
if (local_err) {
error_propagate(errp, local_err);
error_prepend(errp, "can't allocate LSIs: ");
@@ -2112,6 +2122,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: fix 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 cc064f64fccf..7ec69a29d806 100644
--- a/hw/ppc/spapr_vio.c
+++ b/hw/ppc/spapr_vio.c
@@ -416,6 +416,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());
@@ -455,7 +458,7 @@ static void spapr_vio_busdev_realize(DeviceState *qdev, Error **errp)
dev->qdev.id = id;
}
- dev->irq = spapr_irq_alloc(spapr, false, &local_err);
+ dev->irq = spapr_irq_alloc(spapr, SPAPR_IRQ_VIO, vio_index++, &local_err);
if (local_err) {
error_propagate(errp, local_err);
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
^ permalink raw reply related [flat|nested] 32+ messages in thread
* [Qemu-devel] [PATCH 4/4] spapr: introduce a new IRQ backend using fixed IRQ number ranges
2018-05-18 16:44 [Qemu-devel] [PATCH 0/4] spapr: generic IRQ frontend Cédric Le Goater
` (2 preceding siblings ...)
2018-05-18 16:44 ` [Qemu-devel] [PATCH 3/4] spapr: introduce a generic IRQ frontend to the machine Cédric Le Goater
@ 2018-05-18 16:44 ` Cédric Le Goater
2018-05-28 15:18 ` Greg Kurz
3 siblings, 1 reply; 32+ messages in thread
From: Cédric Le Goater @ 2018-05-18 16:44 UTC (permalink / raw)
To: qemu-ppc
Cc: qemu-devel, David Gibson, Greg Kurz, Alexey Kardashevskiy,
Cédric Le Goater
The proposed layout of the IRQ number space is organized as follow :
RANGES DEVICES
0x0000 - 0x0FFF Reserved for future use (IPI = 2)
0x1000 - 0x1000 1 EPOW
0x1001 - 0x1001 1 HOTPLUG
0x1002 - 0x10FF unused
0x1100 - 0x11FF 256 VIO devices (1 IRQ each)
0x1200 - 0x1283 32 PCI LSI devices (4 IRQs each)
0x1284 - 0x13FF unused
0x1400 - 0x17FF PCI MSI device 1 (1024 IRQs each)
0x1800 - 0x1BFF PCI MSI device 1
0x1c00 - 0x1FFF PCI MSI device 2
0x2000 .... not allocated. Need to increase NR_IRQS
The MSI range is a bit more complex to handle as the IRQS are dynamically
allocated by the guest OS. In consequence, we use a bitmap allocator
under the machine for these.
The XICS IRQ number space is increased to 4K, which gives three MSI
ranges of 1K for the PHBs. The XICS source IRQ numbers still have the
4K offset.
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
include/hw/ppc/spapr.h | 2 +
include/hw/ppc/spapr_irq.h | 12 +++
hw/ppc/spapr.c | 22 +++++
hw/ppc/spapr_irq.c | 220 ++++++++++++++++++++++++++++++++++++++++++++-
4 files changed, 255 insertions(+), 1 deletion(-)
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 4eb212b16a51..fcc1b1c1451d 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -165,6 +165,8 @@ struct sPAPRMachineState {
char *kvm_type;
MemoryHotplugState hotplug_memory;
+ int32_t nr_irqs;
+ unsigned long *irq_map;
const char *icp_type;
bool cmd_line_caps[SPAPR_CAP_NUM];
diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
index caf4c33d4cec..d1af4c4d11ba 100644
--- a/include/hw/ppc/spapr_irq.h
+++ b/include/hw/ppc/spapr_irq.h
@@ -22,8 +22,16 @@
#define SPAPR_IRQ_PCI_MSI 0x1400 /* 1K IRQs per device covered by
* a bitmap allocator */
+typedef struct sPAPRPIrqRange {
+ const char *name;
+ uint32_t offset;
+ uint32_t width;
+ uint32_t max_index;
+} sPAPRPIrqRange;
+
typedef struct sPAPRIrq {
uint32_t nr_irqs;
+ const sPAPRPIrqRange *ranges;
void (*init)(sPAPRMachineState *spapr, Error **errp);
int (*alloc)(sPAPRMachineState *spapr, uint32_t range, uint32_t index,
@@ -35,6 +43,10 @@ typedef struct sPAPRIrq {
} sPAPRIrq;
extern sPAPRIrq spapr_irq_default;
+extern sPAPRIrq spapr_irq_2_12;
+
+const sPAPRPIrqRange *spapr_irq_get_range(sPAPRMachineState *spapr,
+ uint32_t offset);
int spapr_irq_alloc(sPAPRMachineState *spapr, uint32_t range, uint32_t index,
Error **errp);
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 09f095d73eae..f2ebd6f20414 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1848,6 +1848,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->nr_irqs);
+}
+
+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, nr_irqs),
+ VMSTATE_END_OF_LIST()
+ },
+};
+
static const VMStateDescription vmstate_spapr = {
.name = "spapr",
.version_id = 3,
@@ -1875,6 +1893,7 @@ static const VMStateDescription vmstate_spapr = {
&vmstate_spapr_cap_cfpc,
&vmstate_spapr_cap_sbbc,
&vmstate_spapr_cap_ibs,
+ &vmstate_spapr_irq_map,
NULL
}
};
@@ -3916,7 +3935,10 @@ 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_2_13_class_options(mc);
+ smc->irq = &spapr_irq_2_12;
SET_MACHINE_COMPAT(mc, SPAPR_COMPAT_2_12);
}
diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
index ff6cb1aafd25..bfffb1467336 100644
--- a/hw/ppc/spapr_irq.c
+++ b/hw/ppc/spapr_irq.c
@@ -192,7 +192,7 @@ static qemu_irq spapr_qirq_2_12(sPAPRMachineState *spapr, int irq)
return NULL;
}
-sPAPRIrq spapr_irq_default = {
+sPAPRIrq spapr_irq_2_12 = {
.nr_irqs = XICS_IRQS_SPAPR,
.init = spapr_irq_init_2_12,
.alloc = spapr_irq_alloc_2_12,
@@ -201,6 +201,224 @@ sPAPRIrq spapr_irq_default = {
.qirq = spapr_qirq_2_12,
};
+/*
+ * IRQ range helpers for new IRQ backends
+ */
+const sPAPRPIrqRange *spapr_irq_get_range(sPAPRMachineState *spapr,
+ uint32_t offset)
+{
+ sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
+ const sPAPRPIrqRange *range = smc->irq->ranges;
+
+ if (range) {
+ while (range->name && range->offset != offset) {
+ range++;
+ }
+
+ if (!range->name) {
+ return NULL;
+ }
+ }
+
+ return range;
+}
+
+static int spapr_irq_get_base(sPAPRMachineState *spapr, uint32_t offset,
+ uint32_t index, Error **errp)
+{
+ const sPAPRPIrqRange *range = spapr_irq_get_range(spapr, offset);
+
+ if (!range) {
+ error_setg(errp, "Invalid IRQ range %x", offset);
+ return -1;
+ }
+
+ if (index > range->max_index) {
+ error_setg(errp, "Index %d too big for IRQ range %s", index,
+ range->name);
+ return -1;
+ }
+
+ return range->offset + index * range->width;
+}
+
+static int spapr_irq_range_alloc(sPAPRMachineState *spapr,
+ uint32_t range, uint32_t index, Error **errp)
+{
+ return spapr_irq_get_base(spapr, range, index, errp);
+}
+
+static int spapr_irq_range_alloc_msi(sPAPRMachineState *spapr, uint32_t range,
+ uint32_t index, int num, bool align,
+ Error **errp)
+{
+ int msi_base = spapr_irq_get_base(spapr, SPAPR_IRQ_PCI_MSI, index, 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->nr_irqs,
+ msi_base, num, align);
+ if (irq == spapr->nr_irqs) {
+ error_setg(errp, "can't find a free MSI %d-IRQ block", num);
+ return -1;
+ }
+
+ bitmap_set(spapr->irq_map, irq, num);
+ return irq;
+}
+
+static void spapr_irq_range_free_msi(sPAPRMachineState *spapr, int irq, int num)
+{
+ bitmap_clear(spapr->irq_map, irq, num);
+}
+
+/*
+ * New XICS IRQ backend
+ *
+ * using the IRQ ranges and device indexes
+ */
+static void spapr_irq_init_2_13(sPAPRMachineState *spapr, Error **errp)
+{
+ MachineState *machine = MACHINE(spapr);
+ sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(machine);
+
+ spapr_irq_init_2_12(spapr, errp);
+
+ /*
+ * Initialize the MSI IRQ allocator. The full XICS IRQ number
+ * space is covered even though the bottow IRQ numbers below the
+ * XICS source number offset (4K) are unused and that only MSI IRQ
+ * numbers can be allocated. We does waste some bytes but it makes
+ * things easier. We will optimize later.
+ */
+ spapr->nr_irqs = smc->irq->nr_irqs + spapr->ics->offset;
+ spapr->irq_map = bitmap_new(spapr->nr_irqs);
+}
+
+static int spapr_irq_alloc_2_13(sPAPRMachineState *spapr,
+ uint32_t range, uint32_t index, Error **errp)
+{
+ ICSState *ics = spapr->ics;
+ bool lsi = (range == SPAPR_IRQ_PCI_LSI);
+ int irq = spapr_irq_range_alloc(spapr, range, index, errp);
+
+ if (irq < 0) {
+ return irq;
+ }
+
+ /* Update the IRQState in the XICS source */
+ ics_set_irq_type(ics, irq - ics->offset, lsi);
+
+ return irq;
+}
+
+static int spapr_irq_alloc_block_2_13(sPAPRMachineState *spapr, uint32_t range,
+ uint32_t index, int num, bool align,
+ Error **errp)
+{
+ ICSState *ics = spapr->ics;
+ bool lsi = (range == SPAPR_IRQ_PCI_LSI);
+ int irq;
+ int i;
+
+ if (range == SPAPR_IRQ_PCI_MSI) {
+ irq = spapr_irq_range_alloc_msi(spapr, range, index, num, align, errp);
+ } else {
+ /* TODO: check IRQ range width vs. required block size */
+ irq = spapr_irq_range_alloc(spapr, range, index, errp);
+ }
+
+ if (irq < 0) {
+ return irq;
+ }
+
+ /* Update the IRQState in the XICS source */
+ for (i = irq; i < irq + num; ++i) {
+ ics_set_irq_type(ics, i - ics->offset, lsi);
+ }
+
+ return irq;
+}
+
+static void spapr_irq_free_2_13(sPAPRMachineState *spapr, int irq, int num,
+ Error **errp)
+{
+ ICSState *ics = spapr->ics;
+ int msi_base = spapr_irq_get_base(spapr, SPAPR_IRQ_PCI_MSI, 0, NULL);
+ int i;
+
+ /* Any IRQ below MSI base should not be freed */
+ if (irq < msi_base) {
+ error_setg(errp, "IRQs %x-%x can not be freed", irq, irq + num);
+ return;
+ }
+
+ spapr_irq_range_free_msi(spapr, irq, num);
+
+ /* Clear out the IRQState from the XICS source */
+ for (i = irq; i < irq + num; ++i) {
+ if (ics_valid_irq(ics, i)) {
+ uint32_t srcno = i - ics->offset;
+ memset(&ics->irqs[srcno], 0, sizeof(ICSIRQState));
+ }
+ }
+}
+
+static qemu_irq spapr_qirq_2_13(sPAPRMachineState *spapr, int irq)
+{
+ return spapr_qirq_2_12(spapr, irq);
+}
+
+/*
+ * RANGES DEVICES
+ *
+ * 0x0000 - 0x0FFF Reserved (IPI = 2)
+ *
+ * 0x1000 - 0x1000 1 EPOW
+ * 0x1001 - 0x1001 1 HOTPLUG
+ * 0x1002 - 0x10FF unused
+ * 0x1100 - 0x11FF 256 VIO devices (1 IRQ each)
+ * 0x1200 - 0x1283 32 PCI LSI devices (4 IRQs each)
+ * 0x1284 - 0x13FF unused
+ * 0x1400 - 0x17FF PCI MSI device 1 (1024 IRQs each)
+ * 0x1800 - 0x1BFF PCI MSI device 1
+ * 0x1c00 - 0x1FFF PCI MSI device 2
+ * 0x2000 .... not allocated. Need to increase NR_IRQS
+ */
+static const sPAPRPIrqRange spapr_irq_ranges_2_13[] = {
+ /* "IPI" Not used */
+ { "EPOW", SPAPR_IRQ_EPOW, 1, 0 },
+ { "HOTPLUG", SPAPR_IRQ_HOTPLUG, 1, 0 },
+ { "VIO", SPAPR_IRQ_VIO, 1, 0xFF },
+ { "PCI LSI", SPAPR_IRQ_PCI_LSI, PCI_NUM_PINS, 0x1F },
+ { "PCI MSI", SPAPR_IRQ_PCI_MSI, 0x400, 0x1F },
+ { NULL, 0, 0, 0 },
+};
+
+/*
+ * Increase the XICS IRQ number space to 4K. It gives us 3 possible
+ * MSI ranges for the PHBs. The XICS Source IRQ numbers still have the
+ * 4K offset.
+ */
+#define SPAPR_NR_IRQS_2_13 0x1000
+
+sPAPRIrq spapr_irq_default = {
+ .nr_irqs = SPAPR_NR_IRQS_2_13,
+ .init = spapr_irq_init_2_13,
+ .ranges = spapr_irq_ranges_2_13,
+ .alloc = spapr_irq_alloc_2_13,
+ .alloc_block = spapr_irq_alloc_block_2_13,
+ .free = spapr_irq_free_2_13,
+ .qirq = spapr_qirq_2_13,
+};
+
int spapr_irq_alloc(sPAPRMachineState *spapr, uint32_t range, uint32_t index,
Error **errp)
{
--
2.13.6
^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [Qemu-devel] [PATCH 1/4] spapr: remove irq_hint parameter from spapr_irq_alloc()
2018-05-18 16:44 ` [Qemu-devel] [PATCH 1/4] spapr: remove irq_hint parameter from spapr_irq_alloc() Cédric Le Goater
@ 2018-05-25 14:02 ` Greg Kurz
2018-05-28 6:17 ` Thomas Huth
2018-06-02 9:10 ` Cédric Le Goater
1 sibling, 1 reply; 32+ messages in thread
From: Greg Kurz @ 2018-05-25 14:02 UTC (permalink / raw)
To: Cédric Le Goater
Cc: qemu-ppc, qemu-devel, David Gibson, Alexey Kardashevskiy, Thomas Huth
On Fri, 18 May 2018 18:44:02 +0200
Cédric Le Goater <clg@kaod.org> wrote:
> This IRQ number hint can possibly be used by the VIO devices if the
> "irq" property is defined on the command line but it seems it is never
> the case. It is not used in libvirt for instance. So, let's remove it
> to simplify future changes.
>
Setting an irq manually looks a bit anachronistic. I doubt anyone would
do that nowadays, and the patch does a nice cleanup. So this looks like
a good idea.
> Nevertheless, this is a compatibbility breakage that will be addressed
^^
typo
> by the subsequent patches which introduce IRQ number allocator
> handlers per machine version.
>
Indeed, this silently breaks the "irq" property of VIO devices, and I
don't quite see how the other patches address this specific breakage.
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
> include/hw/ppc/spapr.h | 3 +--
> hw/ppc/spapr.c | 21 ++++++---------------
> hw/ppc/spapr_events.c | 7 +++----
> hw/ppc/spapr_vio.c | 2 +-
> 4 files changed, 11 insertions(+), 22 deletions(-)
>
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index d60b7c6d7a8b..2cfdfdd67eaf 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -773,8 +773,7 @@ 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_alloc(sPAPRMachineState *spapr, int irq_hint, bool lsi,
> - Error **errp);
> +int spapr_irq_alloc(sPAPRMachineState *spapr, bool lsi, Error **errp);
> int spapr_irq_alloc_block(sPAPRMachineState *spapr, int num, bool lsi,
> bool align, 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 32ab3c43b6c0..05a924a5f2da 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3798,28 +3798,19 @@ static void spapr_irq_set_lsi(sPAPRMachineState *spapr, int irq, bool lsi)
> ics_set_irq_type(spapr->ics, irq - spapr->ics->offset, lsi);
> }
>
> -int spapr_irq_alloc(sPAPRMachineState *spapr, int irq_hint, bool lsi,
> - Error **errp)
> +int spapr_irq_alloc(sPAPRMachineState *spapr, bool lsi, Error **errp)
> {
> ICSState *ics = spapr->ics;
> int irq;
>
> assert(ics);
>
> - if (irq_hint) {
> - if (!ICS_IRQ_FREE(ics, irq_hint - ics->offset)) {
> - error_setg(errp, "can't allocate IRQ %d: already in use", irq_hint);
> - return -1;
> - }
> - irq = irq_hint;
> - } else {
> - irq = ics_find_free_block(ics, 1, 1);
> - if (irq < 0) {
> - error_setg(errp, "can't allocate IRQ: no IRQ left");
> - return -1;
> - }
> - irq += ics->offset;
> + irq = ics_find_free_block(ics, 1, 1);
> + if (irq < 0) {
> + error_setg(errp, "can't allocate IRQ: no IRQ left");
> + return -1;
> }
> + irq += ics->offset;
>
> spapr_irq_set_lsi(spapr, irq, lsi);
> trace_spapr_irq_alloc(irq);
> diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
> index 86836f0626dc..64a67439beac 100644
> --- a/hw/ppc/spapr_events.c
> +++ b/hw/ppc/spapr_events.c
> @@ -712,8 +712,7 @@ void spapr_events_init(sPAPRMachineState *spapr)
> spapr->event_sources = spapr_event_sources_new();
>
> spapr_event_sources_register(spapr->event_sources, EVENT_CLASS_EPOW,
> - spapr_irq_alloc(spapr, 0, false,
> - &error_fatal));
> + spapr_irq_alloc(spapr, false, &error_fatal));
>
> /* NOTE: if machine supports modern/dedicated hotplug event source,
> * we add it to the device-tree unconditionally. This means we may
> @@ -725,8 +724,8 @@ void spapr_events_init(sPAPRMachineState *spapr)
> */
> if (spapr->use_hotplug_event_source) {
> spapr_event_sources_register(spapr->event_sources, EVENT_CLASS_HOT_PLUG,
> - spapr_irq_alloc(spapr, 0, false,
> - &error_fatal));
> + spapr_irq_alloc(spapr, false,
> + &error_fatal));
> }
>
> spapr->epow_notifier.notify = spapr_powerdown_req;
> diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c
> index 472dd6f33a96..cc064f64fccf 100644
> --- a/hw/ppc/spapr_vio.c
> +++ b/hw/ppc/spapr_vio.c
> @@ -455,7 +455,7 @@ static void spapr_vio_busdev_realize(DeviceState *qdev, Error **errp)
> dev->qdev.id = id;
> }
>
> - dev->irq = spapr_irq_alloc(spapr, dev->irq, false, &local_err);
> + dev->irq = spapr_irq_alloc(spapr, false, &local_err);
Silently breaking "irq" like this looks wrong. I'd rather officially remove
it first (ie, kill spapr_vio_props, -5 lines in spapr_vio.c).
Of course, this raises the question of interface deprecation, and it should
theoretically follow the process described at:
https://wiki.qemu.org/Features/LegacyRemoval#Rules_for_removing_an_interface
Cc'ing Thomas, our Chief Deprecation Officer, for insights :)
> if (local_err) {
> error_propagate(errp, local_err);
> return;
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Qemu-devel] [PATCH 2/4] sparp_pci: simplify how the PCI LSIs are allocated
2018-05-18 16:44 ` [Qemu-devel] [PATCH 2/4] sparp_pci: simplify how the PCI LSIs are allocated Cédric Le Goater
@ 2018-05-26 9:40 ` Greg Kurz
2018-06-05 3:44 ` David Gibson
0 siblings, 1 reply; 32+ messages in thread
From: Greg Kurz @ 2018-05-26 9:40 UTC (permalink / raw)
To: Cédric Le Goater
Cc: qemu-ppc, qemu-devel, David Gibson, Alexey Kardashevskiy
On Fri, 18 May 2018 18:44:03 +0200
Cédric Le Goater <clg@kaod.org> wrote:
> PCI LSIs are today allocated one by one using the IRQ alloc_block
> routine. Change the code sequence to first allocate a PCI_NUM_PINS
> block. It will help us providing a generic IRQ framework to the
> machine.
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
> hw/ppc/spapr_pci.c | 21 ++++++++++-----------
> 1 file changed, 10 insertions(+), 11 deletions(-)
>
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 39a14980d397..4fd97ffe4c6e 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1546,6 +1546,8 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
> sPAPRTCETable *tcet;
> const unsigned windows_supported =
> sphb->ddw_enabled ? SPAPR_PCI_DMA_MAX_WINDOWS : 1;
> + uint32_t irq;
> + Error *local_err = NULL;
>
> if (!spapr) {
> error_setg(errp, TYPE_SPAPR_PCI_HOST_BRIDGE " needs a pseries machine");
> @@ -1694,18 +1696,15 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
> QLIST_INSERT_HEAD(&spapr->phbs, sphb, list);
>
> /* Initialize the LSI table */
> - for (i = 0; i < PCI_NUM_PINS; i++) {
> - uint32_t irq;
> - Error *local_err = NULL;
> -
> - irq = spapr_irq_alloc_block(spapr, 1, true, false, &local_err);
> - if (local_err) {
> - error_propagate(errp, local_err);
> - error_prepend(errp, "can't allocate LSIs: ");
> - return;
> - }
> + irq = spapr_irq_alloc_block(spapr, PCI_NUM_PINS, true, false, &local_err);
> + if (local_err) {
> + error_propagate(errp, local_err);
> + error_prepend(errp, "can't allocate LSIs: ");
> + return;
> + }
>
It isn't strictly equivalent. The current code would be happy with
sparse IRQ numbers, while the proposed one wouldn't... Anyway, this
cannot happen since we don't have PHB hotplug.
Reviewed-by: Greg Kurz <groug@kaod.org>
> - sphb->lsi_table[i].irq = irq;
> + for (i = 0; i < PCI_NUM_PINS; i++) {
> + sphb->lsi_table[i].irq = irq + i;
> }
>
> /* allocate connectors for child PCI devices */
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Qemu-devel] [PATCH 1/4] spapr: remove irq_hint parameter from spapr_irq_alloc()
2018-05-25 14:02 ` Greg Kurz
@ 2018-05-28 6:17 ` Thomas Huth
2018-05-28 7:06 ` Cédric Le Goater
2018-06-02 9:19 ` Cédric Le Goater
0 siblings, 2 replies; 32+ messages in thread
From: Thomas Huth @ 2018-05-28 6:17 UTC (permalink / raw)
To: Greg Kurz, Cédric Le Goater
Cc: qemu-ppc, qemu-devel, David Gibson, Alexey Kardashevskiy
On 25.05.2018 16:02, Greg Kurz wrote:
> On Fri, 18 May 2018 18:44:02 +0200
> Cédric Le Goater <clg@kaod.org> wrote:
>
>> This IRQ number hint can possibly be used by the VIO devices if the
>> "irq" property is defined on the command line but it seems it is never
>> the case. It is not used in libvirt for instance. So, let's remove it
>> to simplify future changes.
>>
>
> Setting an irq manually looks a bit anachronistic. I doubt anyone would
> do that nowadays, and the patch does a nice cleanup. So this looks like
> a good idea.
[...]
>> diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c
>> index 472dd6f33a96..cc064f64fccf 100644
>> --- a/hw/ppc/spapr_vio.c
>> +++ b/hw/ppc/spapr_vio.c
>> @@ -455,7 +455,7 @@ static void spapr_vio_busdev_realize(DeviceState *qdev, Error **errp)
>> dev->qdev.id = id;
>> }
>>
>> - dev->irq = spapr_irq_alloc(spapr, dev->irq, false, &local_err);
>> + dev->irq = spapr_irq_alloc(spapr, false, &local_err);
>
> Silently breaking "irq" like this looks wrong. I'd rather officially remove
> it first (ie, kill spapr_vio_props, -5 lines in spapr_vio.c).
>
> Of course, this raises the question of interface deprecation, and it should
> theoretically follow the process described at:
>
> https://wiki.qemu.org/Features/LegacyRemoval#Rules_for_removing_an_interface
>
> Cc'ing Thomas, our Chief Deprecation Officer, for insights :)
The property is a public interface. Just because it's not used by
libvirt does not mean that nobody is using it. So yes, please follow the
rules and mark it as deprecated first for two release, before you really
remove it.
Thomas
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Qemu-devel] [PATCH 1/4] spapr: remove irq_hint parameter from spapr_irq_alloc()
2018-05-28 6:17 ` Thomas Huth
@ 2018-05-28 7:06 ` Cédric Le Goater
2018-05-28 7:18 ` [Qemu-devel] [Qemu-ppc] " Thomas Huth
2018-06-05 3:34 ` [Qemu-devel] " David Gibson
2018-06-02 9:19 ` Cédric Le Goater
1 sibling, 2 replies; 32+ messages in thread
From: Cédric Le Goater @ 2018-05-28 7:06 UTC (permalink / raw)
To: Thomas Huth, Greg Kurz
Cc: qemu-ppc, qemu-devel, David Gibson, Alexey Kardashevskiy
On 05/28/2018 08:17 AM, Thomas Huth wrote:
> On 25.05.2018 16:02, Greg Kurz wrote:
>> On Fri, 18 May 2018 18:44:02 +0200
>> Cédric Le Goater <clg@kaod.org> wrote:
>>
>>> This IRQ number hint can possibly be used by the VIO devices if the
>>> "irq" property is defined on the command line but it seems it is never
>>> the case. It is not used in libvirt for instance. So, let's remove it
>>> to simplify future changes.
>>>
>>
>> Setting an irq manually looks a bit anachronistic. I doubt anyone would
>> do that nowadays, and the patch does a nice cleanup. So this looks like
>> a good idea.
> [...]
>>> diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c
>>> index 472dd6f33a96..cc064f64fccf 100644
>>> --- a/hw/ppc/spapr_vio.c
>>> +++ b/hw/ppc/spapr_vio.c
>>> @@ -455,7 +455,7 @@ static void spapr_vio_busdev_realize(DeviceState *qdev, Error **errp)
>>> dev->qdev.id = id;
>>> }
>>>
>>> - dev->irq = spapr_irq_alloc(spapr, dev->irq, false, &local_err);
>>> + dev->irq = spapr_irq_alloc(spapr, false, &local_err);
>>
>> Silently breaking "irq" like this looks wrong. I'd rather officially remove
>> it first (ie, kill spapr_vio_props, -5 lines in spapr_vio.c).
>>
>> Of course, this raises the question of interface deprecation, and it should
>> theoretically follow the process described at:
>>
>> https://wiki.qemu.org/Features/LegacyRemoval#Rules_for_removing_an_interface
>>
>> Cc'ing Thomas, our Chief Deprecation Officer, for insights :)
>
> The property is a public interface. Just because it's not used by
> libvirt does not mean that nobody is using it. So yes, please follow the
> rules and mark it as deprecated first for two release, before you really
> remove it.
This "irq" property is a problem to introduce a new static layout of IRQ
numbers. It is in complete opposition.
Can we keep it as it is for old pseries machine (settable) and ignore it
for newer ? Would that be fine ?
Thanks,
C.
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Qemu-devel] [Qemu-ppc] [PATCH 1/4] spapr: remove irq_hint parameter from spapr_irq_alloc()
2018-05-28 7:06 ` Cédric Le Goater
@ 2018-05-28 7:18 ` Thomas Huth
2018-05-28 9:20 ` Cédric Le Goater
2018-06-05 3:34 ` [Qemu-devel] " David Gibson
1 sibling, 1 reply; 32+ messages in thread
From: Thomas Huth @ 2018-05-28 7:18 UTC (permalink / raw)
To: Cédric Le Goater, Greg Kurz; +Cc: qemu-ppc, qemu-devel, David Gibson
On 28.05.2018 09:06, Cédric Le Goater wrote:
> On 05/28/2018 08:17 AM, Thomas Huth wrote:
>> On 25.05.2018 16:02, Greg Kurz wrote:
>>> On Fri, 18 May 2018 18:44:02 +0200
>>> Cédric Le Goater <clg@kaod.org> wrote:
>>>
>>>> This IRQ number hint can possibly be used by the VIO devices if the
>>>> "irq" property is defined on the command line but it seems it is never
>>>> the case. It is not used in libvirt for instance. So, let's remove it
>>>> to simplify future changes.
>>>>
>>>
>>> Setting an irq manually looks a bit anachronistic. I doubt anyone would
>>> do that nowadays, and the patch does a nice cleanup. So this looks like
>>> a good idea.
>> [...]
>>>> diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c
>>>> index 472dd6f33a96..cc064f64fccf 100644
>>>> --- a/hw/ppc/spapr_vio.c
>>>> +++ b/hw/ppc/spapr_vio.c
>>>> @@ -455,7 +455,7 @@ static void spapr_vio_busdev_realize(DeviceState *qdev, Error **errp)
>>>> dev->qdev.id = id;
>>>> }
>>>>
>>>> - dev->irq = spapr_irq_alloc(spapr, dev->irq, false, &local_err);
>>>> + dev->irq = spapr_irq_alloc(spapr, false, &local_err);
>>>
>>> Silently breaking "irq" like this looks wrong. I'd rather officially remove
>>> it first (ie, kill spapr_vio_props, -5 lines in spapr_vio.c).
>>>
>>> Of course, this raises the question of interface deprecation, and it should
>>> theoretically follow the process described at:
>>>
>>> https://wiki.qemu.org/Features/LegacyRemoval#Rules_for_removing_an_interface
>>>
>>> Cc'ing Thomas, our Chief Deprecation Officer, for insights :)
>>
>> The property is a public interface. Just because it's not used by
>> libvirt does not mean that nobody is using it. So yes, please follow the
>> rules and mark it as deprecated first for two release, before you really
>> remove it.
>
> This "irq" property is a problem to introduce a new static layout of IRQ
> numbers. It is in complete opposition.
>
> Can we keep it as it is for old pseries machine (settable) and ignore it
> for newer ? Would that be fine ?
I think that would be fine, too. You likely need to keep the settable
IRQs around for the old machines anyway, to make sure that migration of
the old machine types still works, right?
Thomas
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Qemu-devel] [Qemu-ppc] [PATCH 1/4] spapr: remove irq_hint parameter from spapr_irq_alloc()
2018-05-28 7:18 ` [Qemu-devel] [Qemu-ppc] " Thomas Huth
@ 2018-05-28 9:20 ` Cédric Le Goater
2018-05-28 12:09 ` Greg Kurz
0 siblings, 1 reply; 32+ messages in thread
From: Cédric Le Goater @ 2018-05-28 9:20 UTC (permalink / raw)
To: Thomas Huth, Greg Kurz; +Cc: qemu-ppc, qemu-devel, David Gibson
On 05/28/2018 09:18 AM, Thomas Huth wrote:
> On 28.05.2018 09:06, Cédric Le Goater wrote:
>> On 05/28/2018 08:17 AM, Thomas Huth wrote:
>>> On 25.05.2018 16:02, Greg Kurz wrote:
>>>> On Fri, 18 May 2018 18:44:02 +0200
>>>> Cédric Le Goater <clg@kaod.org> wrote:
>>>>
>>>>> This IRQ number hint can possibly be used by the VIO devices if the
>>>>> "irq" property is defined on the command line but it seems it is never
>>>>> the case. It is not used in libvirt for instance. So, let's remove it
>>>>> to simplify future changes.
>>>>>
>>>>
>>>> Setting an irq manually looks a bit anachronistic. I doubt anyone would
>>>> do that nowadays, and the patch does a nice cleanup. So this looks like
>>>> a good idea.
>>> [...]
>>>>> diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c
>>>>> index 472dd6f33a96..cc064f64fccf 100644
>>>>> --- a/hw/ppc/spapr_vio.c
>>>>> +++ b/hw/ppc/spapr_vio.c
>>>>> @@ -455,7 +455,7 @@ static void spapr_vio_busdev_realize(DeviceState *qdev, Error **errp)
>>>>> dev->qdev.id = id;
>>>>> }
>>>>>
>>>>> - dev->irq = spapr_irq_alloc(spapr, dev->irq, false, &local_err);
>>>>> + dev->irq = spapr_irq_alloc(spapr, false, &local_err);
>>>>
>>>> Silently breaking "irq" like this looks wrong. I'd rather officially remove
>>>> it first (ie, kill spapr_vio_props, -5 lines in spapr_vio.c).
>>>>
>>>> Of course, this raises the question of interface deprecation, and it should
>>>> theoretically follow the process described at:
>>>>
>>>> https://wiki.qemu.org/Features/LegacyRemoval#Rules_for_removing_an_interface
>>>>
>>>> Cc'ing Thomas, our Chief Deprecation Officer, for insights :)
>>>
>>> The property is a public interface. Just because it's not used by
>>> libvirt does not mean that nobody is using it. So yes, please follow the
>>> rules and mark it as deprecated first for two release, before you really
>>> remove it.
>>
>> This "irq" property is a problem to introduce a new static layout of IRQ
>> numbers. It is in complete opposition.
>>
>> Can we keep it as it is for old pseries machine (settable) and ignore it
>> for newer ? Would that be fine ?
>
> I think that would be fine, too. You likely need to keep the settable
> IRQs around for the old machines anyway, to make sure that migration of
> the old machine types still works, right?
Yes, that is the goal of patch 3. It introduces a common sPAPR IRQ frontend,
the first backend being the current one.
C.
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Qemu-devel] [Qemu-ppc] [PATCH 1/4] spapr: remove irq_hint parameter from spapr_irq_alloc()
2018-05-28 9:20 ` Cédric Le Goater
@ 2018-05-28 12:09 ` Greg Kurz
2018-05-28 13:33 ` Cédric Le Goater
0 siblings, 1 reply; 32+ messages in thread
From: Greg Kurz @ 2018-05-28 12:09 UTC (permalink / raw)
To: Cédric Le Goater; +Cc: Thomas Huth, qemu-ppc, qemu-devel, David Gibson
On Mon, 28 May 2018 11:20:36 +0200
Cédric Le Goater <clg@kaod.org> wrote:
> On 05/28/2018 09:18 AM, Thomas Huth wrote:
> > On 28.05.2018 09:06, Cédric Le Goater wrote:
> >> On 05/28/2018 08:17 AM, Thomas Huth wrote:
> >>> On 25.05.2018 16:02, Greg Kurz wrote:
> >>>> On Fri, 18 May 2018 18:44:02 +0200
> >>>> Cédric Le Goater <clg@kaod.org> wrote:
> >>>>
> >>>>> This IRQ number hint can possibly be used by the VIO devices if the
> >>>>> "irq" property is defined on the command line but it seems it is never
> >>>>> the case. It is not used in libvirt for instance. So, let's remove it
> >>>>> to simplify future changes.
> >>>>>
> >>>>
> >>>> Setting an irq manually looks a bit anachronistic. I doubt anyone would
> >>>> do that nowadays, and the patch does a nice cleanup. So this looks like
> >>>> a good idea.
> >>> [...]
> >>>>> diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c
> >>>>> index 472dd6f33a96..cc064f64fccf 100644
> >>>>> --- a/hw/ppc/spapr_vio.c
> >>>>> +++ b/hw/ppc/spapr_vio.c
> >>>>> @@ -455,7 +455,7 @@ static void spapr_vio_busdev_realize(DeviceState *qdev, Error **errp)
> >>>>> dev->qdev.id = id;
> >>>>> }
> >>>>>
> >>>>> - dev->irq = spapr_irq_alloc(spapr, dev->irq, false, &local_err);
> >>>>> + dev->irq = spapr_irq_alloc(spapr, false, &local_err);
> >>>>
> >>>> Silently breaking "irq" like this looks wrong. I'd rather officially remove
> >>>> it first (ie, kill spapr_vio_props, -5 lines in spapr_vio.c).
> >>>>
> >>>> Of course, this raises the question of interface deprecation, and it should
> >>>> theoretically follow the process described at:
> >>>>
> >>>> https://wiki.qemu.org/Features/LegacyRemoval#Rules_for_removing_an_interface
> >>>>
> >>>> Cc'ing Thomas, our Chief Deprecation Officer, for insights :)
> >>>
> >>> The property is a public interface. Just because it's not used by
> >>> libvirt does not mean that nobody is using it. So yes, please follow the
> >>> rules and mark it as deprecated first for two release, before you really
> >>> remove it.
> >>
> >> This "irq" property is a problem to introduce a new static layout of IRQ
> >> numbers. It is in complete opposition.
> >>
> >> Can we keep it as it is for old pseries machine (settable) and ignore it
> >> for newer ? Would that be fine ?
> >
> > I think that would be fine, too. You likely need to keep the settable
> > IRQs around for the old machines anyway, to make sure that migration of
> > the old machine types still works, right?
>
> Yes, that is the goal of patch 3. It introduces a common sPAPR IRQ frontend,
> the first backend being the current one.
>
If we keep "irq" but we ignore it with newer machine types, we should at
least print a warning then IMHO.
> C.
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Qemu-devel] [Qemu-ppc] [PATCH 1/4] spapr: remove irq_hint parameter from spapr_irq_alloc()
2018-05-28 12:09 ` Greg Kurz
@ 2018-05-28 13:33 ` Cédric Le Goater
0 siblings, 0 replies; 32+ messages in thread
From: Cédric Le Goater @ 2018-05-28 13:33 UTC (permalink / raw)
To: Greg Kurz; +Cc: Thomas Huth, qemu-ppc, qemu-devel, David Gibson
On 05/28/2018 02:09 PM, Greg Kurz wrote:
> On Mon, 28 May 2018 11:20:36 +0200
> Cédric Le Goater <clg@kaod.org> wrote:
>
>> On 05/28/2018 09:18 AM, Thomas Huth wrote:
>>> On 28.05.2018 09:06, Cédric Le Goater wrote:
>>>> On 05/28/2018 08:17 AM, Thomas Huth wrote:
>>>>> On 25.05.2018 16:02, Greg Kurz wrote:
>>>>>> On Fri, 18 May 2018 18:44:02 +0200
>>>>>> Cédric Le Goater <clg@kaod.org> wrote:
>>>>>>
>>>>>>> This IRQ number hint can possibly be used by the VIO devices if the
>>>>>>> "irq" property is defined on the command line but it seems it is never
>>>>>>> the case. It is not used in libvirt for instance. So, let's remove it
>>>>>>> to simplify future changes.
>>>>>>>
>>>>>>
>>>>>> Setting an irq manually looks a bit anachronistic. I doubt anyone would
>>>>>> do that nowadays, and the patch does a nice cleanup. So this looks like
>>>>>> a good idea.
>>>>> [...]
>>>>>>> diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c
>>>>>>> index 472dd6f33a96..cc064f64fccf 100644
>>>>>>> --- a/hw/ppc/spapr_vio.c
>>>>>>> +++ b/hw/ppc/spapr_vio.c
>>>>>>> @@ -455,7 +455,7 @@ static void spapr_vio_busdev_realize(DeviceState *qdev, Error **errp)
>>>>>>> dev->qdev.id = id;
>>>>>>> }
>>>>>>>
>>>>>>> - dev->irq = spapr_irq_alloc(spapr, dev->irq, false, &local_err);
>>>>>>> + dev->irq = spapr_irq_alloc(spapr, false, &local_err);
>>>>>>
>>>>>> Silently breaking "irq" like this looks wrong. I'd rather officially remove
>>>>>> it first (ie, kill spapr_vio_props, -5 lines in spapr_vio.c).
>>>>>>
>>>>>> Of course, this raises the question of interface deprecation, and it should
>>>>>> theoretically follow the process described at:
>>>>>>
>>>>>> https://wiki.qemu.org/Features/LegacyRemoval#Rules_for_removing_an_interface
>>>>>>
>>>>>> Cc'ing Thomas, our Chief Deprecation Officer, for insights :)
>>>>>
>>>>> The property is a public interface. Just because it's not used by
>>>>> libvirt does not mean that nobody is using it. So yes, please follow the
>>>>> rules and mark it as deprecated first for two release, before you really
>>>>> remove it.
>>>>
>>>> This "irq" property is a problem to introduce a new static layout of IRQ
>>>> numbers. It is in complete opposition.
>>>>
>>>> Can we keep it as it is for old pseries machine (settable) and ignore it
>>>> for newer ? Would that be fine ?
>>>
>>> I think that would be fine, too. You likely need to keep the settable
>>> IRQs around for the old machines anyway, to make sure that migration of
>>> the old machine types still works, right?
>>
>> Yes, that is the goal of patch 3. It introduces a common sPAPR IRQ frontend,
>> the first backend being the current one.
>>
>
> If we keep "irq" but we ignore it with newer machine types, we should at
> least print a warning then IMHO.
May be we can deprecate at the same time. I will take a look.
Thanks,
C.
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Qemu-devel] [PATCH 3/4] spapr: introduce a generic IRQ frontend to the machine
2018-05-18 16:44 ` [Qemu-devel] [PATCH 3/4] spapr: introduce a generic IRQ frontend to the machine Cédric Le Goater
@ 2018-05-28 14:27 ` Greg Kurz
2018-06-13 5:00 ` David Gibson
1 sibling, 0 replies; 32+ messages in thread
From: Greg Kurz @ 2018-05-28 14:27 UTC (permalink / raw)
To: Cédric Le Goater
Cc: qemu-ppc, qemu-devel, David Gibson, Alexey Kardashevskiy
On Fri, 18 May 2018 18:44:04 +0200
Cédric Le Goater <clg@kaod.org> wrote:
> This proposal moves all the related IRQ routines of the sPAPR machine
> behind a class interface to prepare for future changes in the IRQ
> controller model. First of which is a reorganization of the IRQ number
> space layout and a second, coming later, will be to integrate the
> support for the new POWER9 XIVE IRQ controller.
>
> The new interface defines a set of fixed IRQ number ranges, for each
> IRQ type, in which devices allocate the IRQ numbers they need
> depending on a unique device index. Here is the layout :
>
> SPAPR_IRQ_IPI 0x0 /* 1 IRQ per CPU */
> SPAPR_IRQ_EPOW 0x1000 /* 1 IRQ per device */
> SPAPR_IRQ_HOTPLUG 0x1001 /* 1 IRQ per device */
> SPAPR_IRQ_VIO 0x1100 /* 1 IRQ per device */
> SPAPR_IRQ_PCI_LSI 0x1200 /* 4 IRQs per device */
> SPAPR_IRQ_PCI_MSI 0x1400 /* 1K IRQs per device */
>
> The IPI range is reserved for future use when XIVE support
> comes in.
>
> The routines of this interface encompass the previous needs and the
> new ones and seem complex but the provided IRQ backend should
> implement what we have today without any functional changes.
>
> Each device model is modified to take the new interface into account
> using the IRQ range/type definitions and a device index. A part from
> the VIO devices, lacking an id, the changes are relatively simple.
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
> include/hw/ppc/spapr.h | 10 +-
> include/hw/ppc/spapr_irq.h | 46 +++++++++
> hw/ppc/spapr.c | 177 +---------------------------------
> hw/ppc/spapr_events.c | 7 +-
> hw/ppc/spapr_irq.c | 233 +++++++++++++++++++++++++++++++++++++++++++++
> hw/ppc/spapr_pci.c | 21 +++-
> hw/ppc/spapr_vio.c | 5 +-
> hw/ppc/Makefile.objs | 2 +-
> 8 files changed, 308 insertions(+), 193 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 2cfdfdd67eaf..4eb212b16a51 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -3,10 +3,10 @@
>
> #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"
> +#include "hw/ppc/spapr_irq.h"
>
> struct VIOsPAPRBus;
> struct sPAPRPHBState;
> @@ -104,6 +104,7 @@ struct sPAPRMachineClass {
> unsigned n_dma, uint32_t *liobns, Error **errp);
> sPAPRResizeHPT resize_hpt_default;
> sPAPRCapabilities default_caps;
> + sPAPRIrq *irq;
> };
>
> /**
> @@ -773,13 +774,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_alloc(sPAPRMachineState *spapr, bool lsi, Error **errp);
> -int spapr_irq_alloc_block(sPAPRMachineState *spapr, int num, bool lsi,
> - bool align, 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
> new file mode 100644
> index 000000000000..caf4c33d4cec
> --- /dev/null
> +++ b/include/hw/ppc/spapr_irq.h
> @@ -0,0 +1,46 @@
> +/*
> + * 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
> +
> +#include "hw/ppc/xics.h"
> +
> +/*
> + * IRQ ranges
> + */
> +#define SPAPR_IRQ_IPI 0x0 /* 1 IRQ per CPU */
> +#define SPAPR_IRQ_EPOW 0x1000 /* 1 IRQ per device */
> +#define SPAPR_IRQ_HOTPLUG 0x1001 /* 1 IRQ per device */
> +#define SPAPR_IRQ_VIO 0x1100 /* 1 IRQ per device */
> +#define SPAPR_IRQ_PCI_LSI 0x1200 /* 4 IRQs per device */
> +#define SPAPR_IRQ_PCI_MSI 0x1400 /* 1K IRQs per device covered by
> + * a bitmap allocator */
> +
> +typedef struct sPAPRIrq {
> + uint32_t nr_irqs;
> +
> + void (*init)(sPAPRMachineState *spapr, Error **errp);
> + int (*alloc)(sPAPRMachineState *spapr, uint32_t range, uint32_t index,
> + Error **errp);
> + int (*alloc_block)(sPAPRMachineState *spapr, uint32_t range,
> + uint32_t index, int num, bool align, Error **errp);
> + void (*free)(sPAPRMachineState *spapr, int irq, int num, Error **errp);
> + qemu_irq (*qirq)(sPAPRMachineState *spapr, int irq);
> +} sPAPRIrq;
> +
> +extern sPAPRIrq spapr_irq_default;
> +
> +int spapr_irq_alloc(sPAPRMachineState *spapr, uint32_t range, uint32_t index,
> + Error **errp);
> +int spapr_irq_alloc_block(sPAPRMachineState *spapr, uint32_t range,
> + uint32_t index, int num, bool align, Error **errp);
> +void spapr_irq_free(sPAPRMachineState *spapr, int irq, int num, Error **errp);
> +qemu_irq spapr_qirq(sPAPRMachineState *spapr, int irq);
> +
> +#endif /* HW_SPAPR_IRQ_H */
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 05a924a5f2da..09f095d73eae 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,32 +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);
> -
> - if (kvm_enabled()) {
> - if (machine_kernel_irqchip_allowed(machine) &&
> - !xics_kvm_init(spapr, errp)) {
> - spapr->icp_type = TYPE_KVM_ICP;
> - spapr->ics = spapr_ics_create(spapr, TYPE_ICS_KVM, nr_irqs, errp);
> - }
> - if (machine_kernel_irqchip_required(machine) && !spapr->ics) {
> - error_prepend(errp, "kernel_irqchip requested but unavailable: ");
> - return;
> - }
> - }
> -
> - if (!spapr->ics) {
> - xics_spapr_init(spapr);
> - spapr->icp_type = TYPE_ICP;
> - spapr->ics = spapr_ics_create(spapr, TYPE_ICS_SIMPLE, nr_irqs, errp);
> - if (!spapr->ics) {
> - return;
> - }
> - }
> -}
> -
> static int spapr_fixup_cpu_smt_dt(void *fdt, int offset, PowerPCCPU *cpu,
> int smt_threads)
> {
> @@ -2580,7 +2527,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
> */
> @@ -3766,127 +3713,6 @@ 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;
> -}
> -
> -/*
> - * Allocate the IRQ number and set the IRQ type, LSI or MSI
> - */
> -static void spapr_irq_set_lsi(sPAPRMachineState *spapr, int irq, bool lsi)
> -{
> - ics_set_irq_type(spapr->ics, irq - spapr->ics->offset, lsi);
> -}
> -
> -int spapr_irq_alloc(sPAPRMachineState *spapr, bool lsi, Error **errp)
> -{
> - ICSState *ics = spapr->ics;
> - int irq;
> -
> - assert(ics);
> -
> - irq = ics_find_free_block(ics, 1, 1);
> - if (irq < 0) {
> - error_setg(errp, "can't allocate IRQ: no IRQ left");
> - return -1;
> - }
> - irq += ics->offset;
> -
> - spapr_irq_set_lsi(spapr, irq, lsi);
> - trace_spapr_irq_alloc(irq);
> -
> - return irq;
> -}
> -
> -/*
> - * Allocate block of consecutive IRQs, and return the number of the first IRQ in
> - * the block. If align==true, aligns the first IRQ number to num.
> - */
> -int spapr_irq_alloc_block(sPAPRMachineState *spapr, int num, bool lsi,
> - bool align, Error **errp)
> -{
> - ICSState *ics = spapr->ics;
> - int i, 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;
> - }
> -
> - first += ics->offset;
> - for (i = first; i < first + num; ++i) {
> - spapr_irq_set_lsi(spapr, i, lsi);
> - }
> -
> - trace_spapr_irq_alloc_block(first, num, lsi, align);
> -
> - return first;
> -}
> -
> -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)
> {
> @@ -4007,6 +3833,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_default;
> }
>
> static const TypeInfo spapr_machine_info = {
> diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
> index 64a67439beac..e457c5f18189 100644
> --- a/hw/ppc/spapr_events.c
> +++ b/hw/ppc/spapr_events.c
> @@ -712,7 +712,8 @@ void spapr_events_init(sPAPRMachineState *spapr)
> spapr->event_sources = spapr_event_sources_new();
>
> spapr_event_sources_register(spapr->event_sources, EVENT_CLASS_EPOW,
> - spapr_irq_alloc(spapr, false, &error_fatal));
> + spapr_irq_alloc(spapr, SPAPR_IRQ_EPOW, 0,
> + &error_fatal));
>
> /* NOTE: if machine supports modern/dedicated hotplug event source,
> * we add it to the device-tree unconditionally. This means we may
> @@ -724,8 +725,8 @@ void spapr_events_init(sPAPRMachineState *spapr)
> */
> if (spapr->use_hotplug_event_source) {
> spapr_event_sources_register(spapr->event_sources, EVENT_CLASS_HOT_PLUG,
> - spapr_irq_alloc(spapr, false,
> - &error_fatal));
> + spapr_irq_alloc(spapr, SPAPR_IRQ_HOTPLUG,
> + 0, &error_fatal));
> }
>
> spapr->epow_notifier.notify = spapr_powerdown_req;
> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> new file mode 100644
> index 000000000000..ff6cb1aafd25
> --- /dev/null
> +++ b/hw/ppc/spapr_irq.c
> @@ -0,0 +1,233 @@
> +/*
> + * QEMU PowerPC sPAPR IRQ backend
> + *
> + * 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 "qapi/error.h"
> +#include "hw/pci/pci.h"
> +#include "hw/ppc/spapr.h"
> +#include "sysemu/kvm.h"
> +#include "trace.h"
> +
> +/*
> + * Legacy XICS IRQ backend.
> + *
> + * The device IRQ 'range' is used to identify LSIs, and the device
> + * 'index' is unused
> + */
> +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_2_12(sPAPRMachineState *spapr, Error **errp)
> +{
> + MachineState *machine = MACHINE(spapr);
> + sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(machine);
> + uint32_t nr_irqs = smc->irq->nr_irqs;
> +
> + if (kvm_enabled()) {
> + if (machine_kernel_irqchip_allowed(machine) &&
> + !xics_kvm_init(spapr, errp)) {
> + spapr->icp_type = TYPE_KVM_ICP;
> + spapr->ics = spapr_ics_create(spapr, TYPE_ICS_KVM, nr_irqs, errp);
> + }
> + if (machine_kernel_irqchip_required(machine) && !spapr->ics) {
> + error_prepend(errp, "kernel_irqchip requested but unavailable: ");
> + return;
> + }
> + }
> +
> + if (!spapr->ics) {
> + xics_spapr_init(spapr);
> + spapr->icp_type = TYPE_ICP;
> + spapr->ics = spapr_ics_create(spapr, TYPE_ICS_SIMPLE, nr_irqs, errp);
> + if (!spapr->ics) {
> + return;
> + }
> + }
> +}
> +
> +#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;
> +}
> +
> +static int spapr_irq_alloc_2_12(sPAPRMachineState *spapr,
> + uint32_t range, uint32_t index, Error **errp)
> +{
> + ICSState *ics = spapr->ics;
> + bool lsi = (range == SPAPR_IRQ_PCI_LSI);
> + int srcno;
> +
> + assert(ics);
> +
> + srcno = ics_find_free_block(ics, 1, 1);
> + if (srcno < 0) {
> + error_setg(errp, "can't allocate IRQ: no IRQ left");
> + return -1;
> + }
> +
> + ics_set_irq_type(ics, srcno, lsi);
> + trace_spapr_irq_alloc(srcno);
> +
> + return ics->offset + srcno;
> +}
> +
> +/*
> + * Allocate block of consecutive IRQs, and return the number of the first IRQ in
> + * the block. If align==true, aligns the first IRQ number to num.
> + */
> +static int spapr_irq_alloc_block_2_12(sPAPRMachineState *spapr, uint32_t range,
> + uint32_t index, int num, bool align,
> + Error **errp)
> +{
> + ICSState *ics = spapr->ics;
> + bool lsi = (range == SPAPR_IRQ_PCI_LSI);
> + int i, srcno;
> +
> + assert(ics);
> +
> + /*
> + * MSIMessage::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.
> + */
> + if (align) {
> + assert((num == 1) || (num == 2) || (num == 4) ||
> + (num == 8) || (num == 16) || (num == 32));
> + srcno = ics_find_free_block(ics, num, num);
> + } else {
> + srcno = ics_find_free_block(ics, num, 1);
> + }
> +
> + if (srcno < 0) {
> + error_setg(errp, "can't find a free %d-IRQ block", num);
> + return -1;
> + }
> +
> + for (i = srcno; i < srcno + num; ++i) {
> + ics_set_irq_type(ics, i, lsi);
> + }
> +
> + trace_spapr_irq_alloc_block(srcno, num, lsi, align);
> +
> + return ics->offset + srcno;
> +}
> +
> +static void spapr_irq_free_2_12(sPAPRMachineState *spapr, int irq, int num,
> + Error **errp)
> +{
> + 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_2_12(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;
> +}
> +
> +sPAPRIrq spapr_irq_default = {
> + .nr_irqs = XICS_IRQS_SPAPR,
> + .init = spapr_irq_init_2_12,
> + .alloc = spapr_irq_alloc_2_12,
> + .alloc_block = spapr_irq_alloc_block_2_12,
> + .free = spapr_irq_free_2_12,
> + .qirq = spapr_qirq_2_12,
> +};
> +
> +int spapr_irq_alloc(sPAPRMachineState *spapr, uint32_t range, uint32_t index,
> + Error **errp)
> +{
> + sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
> +
> + return smc->irq->alloc(spapr, range, index, errp);
> +}
> +
> +int spapr_irq_alloc_block(sPAPRMachineState *spapr, uint32_t range,
> + uint32_t index, int num, bool align, Error **errp)
> +{
> + sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
> +
> + return smc->irq->alloc_block(spapr, range, index, num, align, errp);
> +}
> +
> +void spapr_irq_free(sPAPRMachineState *spapr, int irq, int num,
> + Error **errp)
> +{
> + sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
> +
> + smc->irq->free(spapr, irq, num, errp);
> +}
> +
> +qemu_irq spapr_qirq(sPAPRMachineState *spapr, int irq)
> +{
> + sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
> +
> + return smc->irq->qirq(spapr, irq);
> +}
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 4fd97ffe4c6e..cca4169fa10b 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -333,7 +333,12 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> return;
> }
>
> - spapr_irq_free(spapr, msi->first_irq, msi->num);
> + spapr_irq_free(spapr, msi->first_irq, msi->num, &err);
> + if (err) {
> + error_reportf_err(err, "Can't remove MSIs for device %x: ",
> + config_addr);
> + rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
> + }
> if (msi_present(pdev)) {
> spapr_msi_setmsg(pdev, 0, false, 0, 0);
> }
> @@ -371,8 +376,8 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> }
>
> /* Allocate MSIs */
> - irq = spapr_irq_alloc_block(spapr, req_num, false,
> - ret_intr_type == RTAS_TYPE_MSI, &err);
> + irq = spapr_irq_alloc_block(spapr, SPAPR_IRQ_PCI_MSI, phb->index, req_num,
> + ret_intr_type == RTAS_TYPE_MSI, &err);
> if (err) {
> error_reportf_err(err, "Can't allocate MSIs for device %x: ",
> config_addr);
> @@ -382,7 +387,11 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>
> /* Release previous MSIs */
> if (msi) {
> - spapr_irq_free(spapr, msi->first_irq, msi->num);
> + spapr_irq_free(spapr, msi->first_irq, msi->num, &err);
> + if (err) {
> + error_reportf_err(err, "Can't remove MSIs for device %x: ",
> + config_addr);
> + }
> g_hash_table_remove(phb->msi, &config_addr);
> }
>
> @@ -1696,7 +1705,8 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
> QLIST_INSERT_HEAD(&spapr->phbs, sphb, list);
>
> /* Initialize the LSI table */
> - irq = spapr_irq_alloc_block(spapr, PCI_NUM_PINS, true, false, &local_err);
> + irq = spapr_irq_alloc_block(spapr, SPAPR_IRQ_PCI_LSI, sphb->index,
> + PCI_NUM_PINS, false, &local_err);
> if (local_err) {
> error_propagate(errp, local_err);
> error_prepend(errp, "can't allocate LSIs: ");
> @@ -2112,6 +2122,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: fix the total count of allocatable MSIs per PHB */
> _FDT(fdt_setprop_cell(fdt, bus_off, "ibm,pe-total-#msi", XICS_IRQS_SPAPR));
I agree it is quite confusing that every PHB advertises the machine's total :-\
>
> /* Dynamic DMA window */
> diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c
> index cc064f64fccf..7ec69a29d806 100644
> --- a/hw/ppc/spapr_vio.c
> +++ b/hw/ppc/spapr_vio.c
> @@ -416,6 +416,9 @@ static void spapr_vio_busdev_reset(DeviceState *qdev)
> }
> }
>
> +/* TODO : poor VIO device indexing ... */
> +static uint32_t vio_index;
I guess we don't really care as we don't (and likely never will) support hotplug
of VIO devices.
This patch looks good for me.
Reviewed-by: Greg Kurz <groug@kaod.org>
> +
> static void spapr_vio_busdev_realize(DeviceState *qdev, Error **errp)
> {
> sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> @@ -455,7 +458,7 @@ static void spapr_vio_busdev_realize(DeviceState *qdev, Error **errp)
> dev->qdev.id = id;
> }
>
> - dev->irq = spapr_irq_alloc(spapr, false, &local_err);
> + dev->irq = spapr_irq_alloc(spapr, SPAPR_IRQ_VIO, vio_index++, &local_err);
> if (local_err) {
> error_propagate(errp, local_err);
> 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)
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Qemu-devel] [PATCH 4/4] spapr: introduce a new IRQ backend using fixed IRQ number ranges
2018-05-18 16:44 ` [Qemu-devel] [PATCH 4/4] spapr: introduce a new IRQ backend using fixed IRQ number ranges Cédric Le Goater
@ 2018-05-28 15:18 ` Greg Kurz
2018-05-28 15:42 ` Cédric Le Goater
2018-05-29 12:51 ` Cédric Le Goater
0 siblings, 2 replies; 32+ messages in thread
From: Greg Kurz @ 2018-05-28 15:18 UTC (permalink / raw)
To: Cédric Le Goater
Cc: qemu-ppc, qemu-devel, David Gibson, Alexey Kardashevskiy
On Fri, 18 May 2018 18:44:05 +0200
Cédric Le Goater <clg@kaod.org> wrote:
> The proposed layout of the IRQ number space is organized as follow :
>
> RANGES DEVICES
>
> 0x0000 - 0x0FFF Reserved for future use (IPI = 2)
> 0x1000 - 0x1000 1 EPOW
> 0x1001 - 0x1001 1 HOTPLUG
> 0x1002 - 0x10FF unused
> 0x1100 - 0x11FF 256 VIO devices (1 IRQ each)
> 0x1200 - 0x1283 32 PCI LSI devices (4 IRQs each)
> 0x1284 - 0x13FF unused
> 0x1400 - 0x17FF PCI MSI device 1 (1024 IRQs each)
> 0x1800 - 0x1BFF PCI MSI device 1
device 2 and...
> 0x1c00 - 0x1FFF PCI MSI device 2
device 3 ?
>
> 0x2000 .... not allocated. Need to increase NR_IRQS
>
What's NR_IRQS ? What's the goal to have several 1k ranges ?
So that each one may be affected to a single device ?
> The MSI range is a bit more complex to handle as the IRQS are dynamically
> allocated by the guest OS. In consequence, we use a bitmap allocator
> under the machine for these.
>
> The XICS IRQ number space is increased to 4K, which gives three MSI
> ranges of 1K for the PHBs. The XICS source IRQ numbers still have the
> 4K offset.
>
Why ?
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
> include/hw/ppc/spapr.h | 2 +
> include/hw/ppc/spapr_irq.h | 12 +++
> hw/ppc/spapr.c | 22 +++++
> hw/ppc/spapr_irq.c | 220 ++++++++++++++++++++++++++++++++++++++++++++-
> 4 files changed, 255 insertions(+), 1 deletion(-)
>
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 4eb212b16a51..fcc1b1c1451d 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -165,6 +165,8 @@ struct sPAPRMachineState {
> char *kvm_type;
> MemoryHotplugState hotplug_memory;
>
> + int32_t nr_irqs;
> + unsigned long *irq_map;
> const char *icp_type;
>
> bool cmd_line_caps[SPAPR_CAP_NUM];
> diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
> index caf4c33d4cec..d1af4c4d11ba 100644
> --- a/include/hw/ppc/spapr_irq.h
> +++ b/include/hw/ppc/spapr_irq.h
> @@ -22,8 +22,16 @@
> #define SPAPR_IRQ_PCI_MSI 0x1400 /* 1K IRQs per device covered by
> * a bitmap allocator */
>
> +typedef struct sPAPRPIrqRange {
> + const char *name;
> + uint32_t offset;
> + uint32_t width;
> + uint32_t max_index;
> +} sPAPRPIrqRange;
> +
> typedef struct sPAPRIrq {
> uint32_t nr_irqs;
> + const sPAPRPIrqRange *ranges;
>
> void (*init)(sPAPRMachineState *spapr, Error **errp);
> int (*alloc)(sPAPRMachineState *spapr, uint32_t range, uint32_t index,
> @@ -35,6 +43,10 @@ typedef struct sPAPRIrq {
> } sPAPRIrq;
>
> extern sPAPRIrq spapr_irq_default;
> +extern sPAPRIrq spapr_irq_2_12;
> +
> +const sPAPRPIrqRange *spapr_irq_get_range(sPAPRMachineState *spapr,
> + uint32_t offset);
>
> int spapr_irq_alloc(sPAPRMachineState *spapr, uint32_t range, uint32_t index,
> Error **errp);
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 09f095d73eae..f2ebd6f20414 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1848,6 +1848,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->nr_irqs);
> +}
> +
> +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, nr_irqs),
> + VMSTATE_END_OF_LIST()
> + },
> +};
> +
> static const VMStateDescription vmstate_spapr = {
> .name = "spapr",
> .version_id = 3,
> @@ -1875,6 +1893,7 @@ static const VMStateDescription vmstate_spapr = {
> &vmstate_spapr_cap_cfpc,
> &vmstate_spapr_cap_sbbc,
> &vmstate_spapr_cap_ibs,
> + &vmstate_spapr_irq_map,
> NULL
> }
> };
> @@ -3916,7 +3935,10 @@ 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_2_13_class_options(mc);
> + smc->irq = &spapr_irq_2_12;
> SET_MACHINE_COMPAT(mc, SPAPR_COMPAT_2_12);
> }
>
> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> index ff6cb1aafd25..bfffb1467336 100644
> --- a/hw/ppc/spapr_irq.c
> +++ b/hw/ppc/spapr_irq.c
> @@ -192,7 +192,7 @@ static qemu_irq spapr_qirq_2_12(sPAPRMachineState *spapr, int irq)
> return NULL;
> }
>
> -sPAPRIrq spapr_irq_default = {
> +sPAPRIrq spapr_irq_2_12 = {
> .nr_irqs = XICS_IRQS_SPAPR,
> .init = spapr_irq_init_2_12,
> .alloc = spapr_irq_alloc_2_12,
> @@ -201,6 +201,224 @@ sPAPRIrq spapr_irq_default = {
> .qirq = spapr_qirq_2_12,
> };
>
> +/*
> + * IRQ range helpers for new IRQ backends
> + */
> +const sPAPRPIrqRange *spapr_irq_get_range(sPAPRMachineState *spapr,
> + uint32_t offset)
> +{
> + sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
> + const sPAPRPIrqRange *range = smc->irq->ranges;
> +
> + if (range) {
> + while (range->name && range->offset != offset) {
> + range++;
> + }
> +
> + if (!range->name) {
> + return NULL;
> + }
> + }
> +
> + return range;
> +}
> +
> +static int spapr_irq_get_base(sPAPRMachineState *spapr, uint32_t offset,
> + uint32_t index, Error **errp)
> +{
> + const sPAPRPIrqRange *range = spapr_irq_get_range(spapr, offset);
> +
> + if (!range) {
> + error_setg(errp, "Invalid IRQ range %x", offset);
> + return -1;
> + }
> +
> + if (index > range->max_index) {
> + error_setg(errp, "Index %d too big for IRQ range %s", index,
> + range->name);
> + return -1;
> + }
> +
> + return range->offset + index * range->width;
> +}
> +
> +static int spapr_irq_range_alloc(sPAPRMachineState *spapr,
> + uint32_t range, uint32_t index, Error **errp)
> +{
> + return spapr_irq_get_base(spapr, range, index, errp);
> +}
> +
> +static int spapr_irq_range_alloc_msi(sPAPRMachineState *spapr, uint32_t range,
> + uint32_t index, int num, bool align,
> + Error **errp)
> +{
> + int msi_base = spapr_irq_get_base(spapr, SPAPR_IRQ_PCI_MSI, index, 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->nr_irqs,
> + msi_base, num, align);
> + if (irq == spapr->nr_irqs) {
> + error_setg(errp, "can't find a free MSI %d-IRQ block", num);
> + return -1;
> + }
> +
> + bitmap_set(spapr->irq_map, irq, num);
> + return irq;
> +}
> +
> +static void spapr_irq_range_free_msi(sPAPRMachineState *spapr, int irq, int num)
> +{
> + bitmap_clear(spapr->irq_map, irq, num);
> +}
> +
> +/*
> + * New XICS IRQ backend
> + *
> + * using the IRQ ranges and device indexes
> + */
> +static void spapr_irq_init_2_13(sPAPRMachineState *spapr, Error **errp)
FYI, next QEMU version will likely be 3.0:
https://lists.gnu.org/archive/html/qemu-devel/2018-05/msg04879.html
Also, it is useful to add version information to the name in case of
existing machine types. I guess _default or _new is a better choice
here.
> +{
> + MachineState *machine = MACHINE(spapr);
> + sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(machine);
> +
> + spapr_irq_init_2_12(spapr, errp);
Hmm... it's a bit confusing. I'd rather have each init function to call a
common helper.
> +
> + /*
> + * Initialize the MSI IRQ allocator. The full XICS IRQ number
> + * space is covered even though the bottow IRQ numbers below the
> + * XICS source number offset (4K) are unused and that only MSI IRQ
> + * numbers can be allocated. We does waste some bytes but it makes
> + * things easier. We will optimize later.
> + */
> + spapr->nr_irqs = smc->irq->nr_irqs + spapr->ics->offset;
> + spapr->irq_map = bitmap_new(spapr->nr_irqs);
> +}
> +
> +static int spapr_irq_alloc_2_13(sPAPRMachineState *spapr,
> + uint32_t range, uint32_t index, Error **errp)
> +{
> + ICSState *ics = spapr->ics;
> + bool lsi = (range == SPAPR_IRQ_PCI_LSI);
> + int irq = spapr_irq_range_alloc(spapr, range, index, errp);
> +
> + if (irq < 0) {
> + return irq;
> + }
> +
> + /* Update the IRQState in the XICS source */
> + ics_set_irq_type(ics, irq - ics->offset, lsi);
> +
> + return irq;
> +}
> +
> +static int spapr_irq_alloc_block_2_13(sPAPRMachineState *spapr, uint32_t range,
> + uint32_t index, int num, bool align,
> + Error **errp)
> +{
> + ICSState *ics = spapr->ics;
> + bool lsi = (range == SPAPR_IRQ_PCI_LSI);
> + int irq;
> + int i;
> +
> + if (range == SPAPR_IRQ_PCI_MSI) {
> + irq = spapr_irq_range_alloc_msi(spapr, range, index, num, align, errp);
> + } else {
> + /* TODO: check IRQ range width vs. required block size */
> + irq = spapr_irq_range_alloc(spapr, range, index, errp);
> + }
> +
> + if (irq < 0) {
> + return irq;
> + }
> +
> + /* Update the IRQState in the XICS source */
> + for (i = irq; i < irq + num; ++i) {
> + ics_set_irq_type(ics, i - ics->offset, lsi);
> + }
> +
> + return irq;
> +}
> +
> +static void spapr_irq_free_2_13(sPAPRMachineState *spapr, int irq, int num,
> + Error **errp)
> +{
> + ICSState *ics = spapr->ics;
> + int msi_base = spapr_irq_get_base(spapr, SPAPR_IRQ_PCI_MSI, 0, NULL);
> + int i;
> +
> + /* Any IRQ below MSI base should not be freed */
> + if (irq < msi_base) {
> + error_setg(errp, "IRQs %x-%x can not be freed", irq, irq + num);
> + return;
> + }
> +
> + spapr_irq_range_free_msi(spapr, irq, num);
> +
> + /* Clear out the IRQState from the XICS source */
> + for (i = irq; i < irq + num; ++i) {
> + if (ics_valid_irq(ics, i)) {
> + uint32_t srcno = i - ics->offset;
> + memset(&ics->irqs[srcno], 0, sizeof(ICSIRQState));
> + }
> + }
> +}
> +
> +static qemu_irq spapr_qirq_2_13(sPAPRMachineState *spapr, int irq)
> +{
> + return spapr_qirq_2_12(spapr, irq);
> +}
> +
> +/*
> + * RANGES DEVICES
> + *
> + * 0x0000 - 0x0FFF Reserved (IPI = 2)
> + *
> + * 0x1000 - 0x1000 1 EPOW
> + * 0x1001 - 0x1001 1 HOTPLUG
> + * 0x1002 - 0x10FF unused
> + * 0x1100 - 0x11FF 256 VIO devices (1 IRQ each)
> + * 0x1200 - 0x1283 32 PCI LSI devices (4 IRQs each)
> + * 0x1284 - 0x13FF unused
> + * 0x1400 - 0x17FF PCI MSI device 1 (1024 IRQs each)
> + * 0x1800 - 0x1BFF PCI MSI device 1
> + * 0x1c00 - 0x1FFF PCI MSI device 2
> + * 0x2000 .... not allocated. Need to increase NR_IRQS
> + */
> +static const sPAPRPIrqRange spapr_irq_ranges_2_13[] = {
> + /* "IPI" Not used */
> + { "EPOW", SPAPR_IRQ_EPOW, 1, 0 },
> + { "HOTPLUG", SPAPR_IRQ_HOTPLUG, 1, 0 },
> + { "VIO", SPAPR_IRQ_VIO, 1, 0xFF },
> + { "PCI LSI", SPAPR_IRQ_PCI_LSI, PCI_NUM_PINS, 0x1F },
> + { "PCI MSI", SPAPR_IRQ_PCI_MSI, 0x400, 0x1F },
> + { NULL, 0, 0, 0 },
> +};
> +
> +/*
> + * Increase the XICS IRQ number space to 4K. It gives us 3 possible
> + * MSI ranges for the PHBs. The XICS Source IRQ numbers still have the
> + * 4K offset.
> + */
> +#define SPAPR_NR_IRQS_2_13 0x1000
> +
> +sPAPRIrq spapr_irq_default = {
> + .nr_irqs = SPAPR_NR_IRQS_2_13,
Since there's only one user for this define, why not open-coding the value
here ?
> + .init = spapr_irq_init_2_13,
> + .ranges = spapr_irq_ranges_2_13,
> + .alloc = spapr_irq_alloc_2_13,
> + .alloc_block = spapr_irq_alloc_block_2_13,
> + .free = spapr_irq_free_2_13,
> + .qirq = spapr_qirq_2_13,
> +};
> +
> int spapr_irq_alloc(sPAPRMachineState *spapr, uint32_t range, uint32_t index,
> Error **errp)
> {
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Qemu-devel] [PATCH 4/4] spapr: introduce a new IRQ backend using fixed IRQ number ranges
2018-05-28 15:18 ` Greg Kurz
@ 2018-05-28 15:42 ` Cédric Le Goater
2018-05-29 12:51 ` Cédric Le Goater
1 sibling, 0 replies; 32+ messages in thread
From: Cédric Le Goater @ 2018-05-28 15:42 UTC (permalink / raw)
To: Greg Kurz; +Cc: qemu-ppc, qemu-devel, David Gibson, Alexey Kardashevskiy
On 05/28/2018 05:18 PM, Greg Kurz wrote:
> On Fri, 18 May 2018 18:44:05 +0200
> Cédric Le Goater <clg@kaod.org> wrote:
>
>> The proposed layout of the IRQ number space is organized as follow :
>>
>> RANGES DEVICES
>>
>> 0x0000 - 0x0FFF Reserved for future use (IPI = 2)
>> 0x1000 - 0x1000 1 EPOW
>> 0x1001 - 0x1001 1 HOTPLUG
>> 0x1002 - 0x10FF unused
>> 0x1100 - 0x11FF 256 VIO devices (1 IRQ each)
>> 0x1200 - 0x1283 32 PCI LSI devices (4 IRQs each)
>> 0x1284 - 0x13FF unused
>> 0x1400 - 0x17FF PCI MSI device 1 (1024 IRQs each)
>> 0x1800 - 0x1BFF PCI MSI device 1
>
> device 2 and...
>
>> 0x1c00 - 0x1FFF PCI MSI device 2
>
> device 3 ?
ah yes :)
>
>>
>> 0x2000 .... not allocated. Need to increase NR_IRQS
>>
>
> What's NR_IRQS ?
The total number of IRQs in the backend.
> What's the goal to have several 1k ranges ?
to support multiple PHBs with different allocatable MSI ranges,
which is not the case today.
> So that each one may be affected to a single device ?
yes.
>> The MSI range is a bit more complex to handle as the IRQS are dynamically
>> allocated by the guest OS. In consequence, we use a bitmap allocator
>> under the machine for these.
>>
>> The XICS IRQ number space is increased to 4K, which gives three MSI
>> ranges of 1K for the PHBs. The XICS source IRQ numbers still have the
>> 4K offset.
>>
>
> Why ?
It's the legacy IRQ offset value for the sPAPR sources (2 being
reserved for XICS IPIs) and because XIVE will use the range for
IPIs :
0x0000 - 0x0FFF Reserved for future use (IPI = 2)
So not changing the value serve our purpose.
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>> include/hw/ppc/spapr.h | 2 +
>> include/hw/ppc/spapr_irq.h | 12 +++
>> hw/ppc/spapr.c | 22 +++++
>> hw/ppc/spapr_irq.c | 220 ++++++++++++++++++++++++++++++++++++++++++++-
>> 4 files changed, 255 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
>> index 4eb212b16a51..fcc1b1c1451d 100644
>> --- a/include/hw/ppc/spapr.h
>> +++ b/include/hw/ppc/spapr.h
>> @@ -165,6 +165,8 @@ struct sPAPRMachineState {
>> char *kvm_type;
>> MemoryHotplugState hotplug_memory;
>>
>> + int32_t nr_irqs;
>> + unsigned long *irq_map;
>> const char *icp_type;
>>
>> bool cmd_line_caps[SPAPR_CAP_NUM];
>> diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
>> index caf4c33d4cec..d1af4c4d11ba 100644
>> --- a/include/hw/ppc/spapr_irq.h
>> +++ b/include/hw/ppc/spapr_irq.h
>> @@ -22,8 +22,16 @@
>> #define SPAPR_IRQ_PCI_MSI 0x1400 /* 1K IRQs per device covered by
>> * a bitmap allocator */
>>
>> +typedef struct sPAPRPIrqRange {
>> + const char *name;
>> + uint32_t offset;
>> + uint32_t width;
>> + uint32_t max_index;
>> +} sPAPRPIrqRange;
>> +
>> typedef struct sPAPRIrq {
>> uint32_t nr_irqs;
>> + const sPAPRPIrqRange *ranges;
>>
>> void (*init)(sPAPRMachineState *spapr, Error **errp);
>> int (*alloc)(sPAPRMachineState *spapr, uint32_t range, uint32_t index,
>> @@ -35,6 +43,10 @@ typedef struct sPAPRIrq {
>> } sPAPRIrq;
>>
>> extern sPAPRIrq spapr_irq_default;
>> +extern sPAPRIrq spapr_irq_2_12;
>> +
>> +const sPAPRPIrqRange *spapr_irq_get_range(sPAPRMachineState *spapr,
>> + uint32_t offset);
>>
>> int spapr_irq_alloc(sPAPRMachineState *spapr, uint32_t range, uint32_t index,
>> Error **errp);
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index 09f095d73eae..f2ebd6f20414 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -1848,6 +1848,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->nr_irqs);
>> +}
>> +
>> +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, nr_irqs),
>> + VMSTATE_END_OF_LIST()
>> + },
>> +};
>> +
>> static const VMStateDescription vmstate_spapr = {
>> .name = "spapr",
>> .version_id = 3,
>> @@ -1875,6 +1893,7 @@ static const VMStateDescription vmstate_spapr = {
>> &vmstate_spapr_cap_cfpc,
>> &vmstate_spapr_cap_sbbc,
>> &vmstate_spapr_cap_ibs,
>> + &vmstate_spapr_irq_map,
>> NULL
>> }
>> };
>> @@ -3916,7 +3935,10 @@ 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_2_13_class_options(mc);
>> + smc->irq = &spapr_irq_2_12;
>> SET_MACHINE_COMPAT(mc, SPAPR_COMPAT_2_12);
>> }
>>
>> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
>> index ff6cb1aafd25..bfffb1467336 100644
>> --- a/hw/ppc/spapr_irq.c
>> +++ b/hw/ppc/spapr_irq.c
>> @@ -192,7 +192,7 @@ static qemu_irq spapr_qirq_2_12(sPAPRMachineState *spapr, int irq)
>> return NULL;
>> }
>>
>> -sPAPRIrq spapr_irq_default = {
>> +sPAPRIrq spapr_irq_2_12 = {
>> .nr_irqs = XICS_IRQS_SPAPR,
>> .init = spapr_irq_init_2_12,
>> .alloc = spapr_irq_alloc_2_12,
>> @@ -201,6 +201,224 @@ sPAPRIrq spapr_irq_default = {
>> .qirq = spapr_qirq_2_12,
>> };
>>
>> +/*
>> + * IRQ range helpers for new IRQ backends
>> + */
>> +const sPAPRPIrqRange *spapr_irq_get_range(sPAPRMachineState *spapr,
>> + uint32_t offset)
>> +{
>> + sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
>> + const sPAPRPIrqRange *range = smc->irq->ranges;
>> +
>> + if (range) {
>> + while (range->name && range->offset != offset) {
>> + range++;
>> + }
>> +
>> + if (!range->name) {
>> + return NULL;
>> + }
>> + }
>> +
>> + return range;
>> +}
>> +
>> +static int spapr_irq_get_base(sPAPRMachineState *spapr, uint32_t offset,
>> + uint32_t index, Error **errp)
>> +{
>> + const sPAPRPIrqRange *range = spapr_irq_get_range(spapr, offset);
>> +
>> + if (!range) {
>> + error_setg(errp, "Invalid IRQ range %x", offset);
>> + return -1;
>> + }
>> +
>> + if (index > range->max_index) {
>> + error_setg(errp, "Index %d too big for IRQ range %s", index,
>> + range->name);
>> + return -1;
>> + }
>> +
>> + return range->offset + index * range->width;
>> +}
>> +
>> +static int spapr_irq_range_alloc(sPAPRMachineState *spapr,
>> + uint32_t range, uint32_t index, Error **errp)
>> +{
>> + return spapr_irq_get_base(spapr, range, index, errp);
>> +}
>> +
>> +static int spapr_irq_range_alloc_msi(sPAPRMachineState *spapr, uint32_t range,
>> + uint32_t index, int num, bool align,
>> + Error **errp)
>> +{
>> + int msi_base = spapr_irq_get_base(spapr, SPAPR_IRQ_PCI_MSI, index, 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->nr_irqs,
>> + msi_base, num, align);
>> + if (irq == spapr->nr_irqs) {
>> + error_setg(errp, "can't find a free MSI %d-IRQ block", num);
>> + return -1;
>> + }
>> +
>> + bitmap_set(spapr->irq_map, irq, num);
>> + return irq;
>> +}
>> +
>> +static void spapr_irq_range_free_msi(sPAPRMachineState *spapr, int irq, int num)
>> +{
>> + bitmap_clear(spapr->irq_map, irq, num);
>> +}
>> +
>> +/*
>> + * New XICS IRQ backend
>> + *
>> + * using the IRQ ranges and device indexes
>> + */
>> +static void spapr_irq_init_2_13(sPAPRMachineState *spapr, Error **errp)
>
> FYI, next QEMU version will likely be 3.0:
>
> https://lists.gnu.org/archive/html/qemu-devel/2018-05/msg04879.html
ah yes.
> Also, it is useful to add version information to the name in case of
> existing machine types. I guess _default or _new is a better choice
> here.
ok.
>> +{
>> + MachineState *machine = MACHINE(spapr);
>> + sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(machine);
>> +
>> + spapr_irq_init_2_12(spapr, errp);
>
> Hmm... it's a bit confusing. I'd rather have each init function to call a
> common helper.
well, I followed the machine class pattern.
>
>> +
>> + /*
>> + * Initialize the MSI IRQ allocator. The full XICS IRQ number
>> + * space is covered even though the bottow IRQ numbers below the
>> + * XICS source number offset (4K) are unused and that only MSI IRQ
>> + * numbers can be allocated. We does waste some bytes but it makes
>> + * things easier. We will optimize later.
>> + */
>> + spapr->nr_irqs = smc->irq->nr_irqs + spapr->ics->offset;
>> + spapr->irq_map = bitmap_new(spapr->nr_irqs);
>> +}
>> +
>> +static int spapr_irq_alloc_2_13(sPAPRMachineState *spapr,
>> + uint32_t range, uint32_t index, Error **errp)
>> +{
>> + ICSState *ics = spapr->ics;
>> + bool lsi = (range == SPAPR_IRQ_PCI_LSI);
>> + int irq = spapr_irq_range_alloc(spapr, range, index, errp);
>> +
>> + if (irq < 0) {
>> + return irq;
>> + }
>> +
>> + /* Update the IRQState in the XICS source */
>> + ics_set_irq_type(ics, irq - ics->offset, lsi);
>> +
>> + return irq;
>> +}
>> +
>> +static int spapr_irq_alloc_block_2_13(sPAPRMachineState *spapr, uint32_t range,
>> + uint32_t index, int num, bool align,
>> + Error **errp)
>> +{
>> + ICSState *ics = spapr->ics;
>> + bool lsi = (range == SPAPR_IRQ_PCI_LSI);
>> + int irq;
>> + int i;
>> +
>> + if (range == SPAPR_IRQ_PCI_MSI) {
>> + irq = spapr_irq_range_alloc_msi(spapr, range, index, num, align, errp);
>> + } else {
>> + /* TODO: check IRQ range width vs. required block size */
>> + irq = spapr_irq_range_alloc(spapr, range, index, errp);
>> + }
>> +
>> + if (irq < 0) {
>> + return irq;
>> + }
>> +
>> + /* Update the IRQState in the XICS source */
>> + for (i = irq; i < irq + num; ++i) {
>> + ics_set_irq_type(ics, i - ics->offset, lsi);
>> + }
>> +
>> + return irq;
>> +}
>> +
>> +static void spapr_irq_free_2_13(sPAPRMachineState *spapr, int irq, int num,
>> + Error **errp)
>> +{
>> + ICSState *ics = spapr->ics;
>> + int msi_base = spapr_irq_get_base(spapr, SPAPR_IRQ_PCI_MSI, 0, NULL);
>> + int i;
>> +
>> + /* Any IRQ below MSI base should not be freed */
>> + if (irq < msi_base) {
>> + error_setg(errp, "IRQs %x-%x can not be freed", irq, irq + num);
>> + return;
>> + }
>> +
>> + spapr_irq_range_free_msi(spapr, irq, num);
>> +
>> + /* Clear out the IRQState from the XICS source */
>> + for (i = irq; i < irq + num; ++i) {
>> + if (ics_valid_irq(ics, i)) {
>> + uint32_t srcno = i - ics->offset;
>> + memset(&ics->irqs[srcno], 0, sizeof(ICSIRQState));
>> + }
>> + }
>> +}
>> +
>> +static qemu_irq spapr_qirq_2_13(sPAPRMachineState *spapr, int irq)
>> +{
>> + return spapr_qirq_2_12(spapr, irq);
>> +}
>> +
>> +/*
>> + * RANGES DEVICES
>> + *
>> + * 0x0000 - 0x0FFF Reserved (IPI = 2)
>> + *
>> + * 0x1000 - 0x1000 1 EPOW
>> + * 0x1001 - 0x1001 1 HOTPLUG
>> + * 0x1002 - 0x10FF unused
>> + * 0x1100 - 0x11FF 256 VIO devices (1 IRQ each)
>> + * 0x1200 - 0x1283 32 PCI LSI devices (4 IRQs each)
>> + * 0x1284 - 0x13FF unused
>> + * 0x1400 - 0x17FF PCI MSI device 1 (1024 IRQs each)
>> + * 0x1800 - 0x1BFF PCI MSI device 1
>> + * 0x1c00 - 0x1FFF PCI MSI device 2
>> + * 0x2000 .... not allocated. Need to increase NR_IRQS
>> + */
>> +static const sPAPRPIrqRange spapr_irq_ranges_2_13[] = {
>> + /* "IPI" Not used */
>> + { "EPOW", SPAPR_IRQ_EPOW, 1, 0 },
>> + { "HOTPLUG", SPAPR_IRQ_HOTPLUG, 1, 0 },
>> + { "VIO", SPAPR_IRQ_VIO, 1, 0xFF },
>> + { "PCI LSI", SPAPR_IRQ_PCI_LSI, PCI_NUM_PINS, 0x1F },
>> + { "PCI MSI", SPAPR_IRQ_PCI_MSI, 0x400, 0x1F },
>> + { NULL, 0, 0, 0 },
>> +};
>> +
>> +/*
>> + * Increase the XICS IRQ number space to 4K. It gives us 3 possible
>> + * MSI ranges for the PHBs. The XICS Source IRQ numbers still have the
>> + * 4K offset.
>> + */
>> +#define SPAPR_NR_IRQS_2_13 0x1000
>> +
>> +sPAPRIrq spapr_irq_default = {
>> + .nr_irqs = SPAPR_NR_IRQS_2_13,
>
> Since there's only one user for this define, why not open-coding the value
> here ?
yes. I agree.
Thanks,
C.
>> + .init = spapr_irq_init_2_13,
>> + .ranges = spapr_irq_ranges_2_13,
>> + .alloc = spapr_irq_alloc_2_13,
>> + .alloc_block = spapr_irq_alloc_block_2_13,
>> + .free = spapr_irq_free_2_13,
>> + .qirq = spapr_qirq_2_13,
>> +};
>> +
>> int spapr_irq_alloc(sPAPRMachineState *spapr, uint32_t range, uint32_t index,
>> Error **errp)
>> {
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Qemu-devel] [PATCH 4/4] spapr: introduce a new IRQ backend using fixed IRQ number ranges
2018-05-28 15:18 ` Greg Kurz
2018-05-28 15:42 ` Cédric Le Goater
@ 2018-05-29 12:51 ` Cédric Le Goater
1 sibling, 0 replies; 32+ messages in thread
From: Cédric Le Goater @ 2018-05-29 12:51 UTC (permalink / raw)
To: Greg Kurz; +Cc: qemu-ppc, qemu-devel, David Gibson, Alexey Kardashevskiy
>> +static void spapr_irq_range_free_msi(sPAPRMachineState *spapr, int irq, int num)
>> +{
>> + bitmap_clear(spapr->irq_map, irq, num);
>> +}
>> +
>> +/*
>> + * New XICS IRQ backend
>> + *
>> + * using the IRQ ranges and device indexes
>> + */
>> +static void spapr_irq_init_2_13(sPAPRMachineState *spapr, Error **errp)
>
> FYI, next QEMU version will likely be 3.0:
>
> https://lists.gnu.org/archive/html/qemu-devel/2018-05/msg04879.html
>
> Also, it is useful to add version information to the name in case of
> existing machine types. I guess _default or _new is a better choice
> here.
What about :
_legacy for the former XICS backend
_xics for the new XICS backend
_xive for the coming XIVE backend
?
Thanks,
C.
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Qemu-devel] [PATCH 1/4] spapr: remove irq_hint parameter from spapr_irq_alloc()
2018-05-18 16:44 ` [Qemu-devel] [PATCH 1/4] spapr: remove irq_hint parameter from spapr_irq_alloc() Cédric Le Goater
2018-05-25 14:02 ` Greg Kurz
@ 2018-06-02 9:10 ` Cédric Le Goater
1 sibling, 0 replies; 32+ messages in thread
From: Cédric Le Goater @ 2018-06-02 9:10 UTC (permalink / raw)
To: qemu-ppc; +Cc: qemu-devel, David Gibson, Greg Kurz, Alexey Kardashevskiy
On 05/18/2018 06:44 PM, Cédric Le Goater wrote:
> This IRQ number hint can possibly be used by the VIO devices if the
> "irq" property is defined on the command line but it seems it is never
> the case. It is not used in libvirt for instance. So, let's remove it
> to simplify future changes.
>
> Nevertheless, this is a compatibbility breakage that will be addressed
> by the subsequent patches which introduce IRQ number allocator
> handlers per machine version.
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
> include/hw/ppc/spapr.h | 3 +--
> hw/ppc/spapr.c | 21 ++++++---------------
> hw/ppc/spapr_events.c | 7 +++----
> hw/ppc/spapr_vio.c | 2 +-
> 4 files changed, 11 insertions(+), 22 deletions(-)
>
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index d60b7c6d7a8b..2cfdfdd67eaf 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -773,8 +773,7 @@ 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_alloc(sPAPRMachineState *spapr, int irq_hint, bool lsi,
> - Error **errp);
> +int spapr_irq_alloc(sPAPRMachineState *spapr, bool lsi, Error **errp);
> int spapr_irq_alloc_block(sPAPRMachineState *spapr, int num, bool lsi,
> bool align, 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 32ab3c43b6c0..05a924a5f2da 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3798,28 +3798,19 @@ static void spapr_irq_set_lsi(sPAPRMachineState *spapr, int irq, bool lsi)
> ics_set_irq_type(spapr->ics, irq - spapr->ics->offset, lsi);
> }
>
> -int spapr_irq_alloc(sPAPRMachineState *spapr, int irq_hint, bool lsi,
> - Error **errp)
> +int spapr_irq_alloc(sPAPRMachineState *spapr, bool lsi, Error **errp)
> {
> ICSState *ics = spapr->ics;
> int irq;
>
> assert(ics);
>
> - if (irq_hint) {
> - if (!ICS_IRQ_FREE(ics, irq_hint - ics->offset)) {
Side note, this line is a promise for a nice segv if irq_hint < ics->offset.
I think it is time to deprecate the 'irq' property of the sPAPR VIO devices.
C.
> - error_setg(errp, "can't allocate IRQ %d: already in use", irq_hint);
> - return -1;
> - }
> - irq = irq_hint;
> - } else {
> - irq = ics_find_free_block(ics, 1, 1);
> - if (irq < 0) {
> - error_setg(errp, "can't allocate IRQ: no IRQ left");
> - return -1;
> - }
> - irq += ics->offset;
> + irq = ics_find_free_block(ics, 1, 1);
> + if (irq < 0) {
> + error_setg(errp, "can't allocate IRQ: no IRQ left");
> + return -1;
> }
> + irq += ics->offset;
>
> spapr_irq_set_lsi(spapr, irq, lsi);
> trace_spapr_irq_alloc(irq);
> diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
> index 86836f0626dc..64a67439beac 100644
> --- a/hw/ppc/spapr_events.c
> +++ b/hw/ppc/spapr_events.c
> @@ -712,8 +712,7 @@ void spapr_events_init(sPAPRMachineState *spapr)
> spapr->event_sources = spapr_event_sources_new();
>
> spapr_event_sources_register(spapr->event_sources, EVENT_CLASS_EPOW,
> - spapr_irq_alloc(spapr, 0, false,
> - &error_fatal));
> + spapr_irq_alloc(spapr, false, &error_fatal));
>
> /* NOTE: if machine supports modern/dedicated hotplug event source,
> * we add it to the device-tree unconditionally. This means we may
> @@ -725,8 +724,8 @@ void spapr_events_init(sPAPRMachineState *spapr)
> */
> if (spapr->use_hotplug_event_source) {
> spapr_event_sources_register(spapr->event_sources, EVENT_CLASS_HOT_PLUG,
> - spapr_irq_alloc(spapr, 0, false,
> - &error_fatal));
> + spapr_irq_alloc(spapr, false,
> + &error_fatal));
> }
>
> spapr->epow_notifier.notify = spapr_powerdown_req;
> diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c
> index 472dd6f33a96..cc064f64fccf 100644
> --- a/hw/ppc/spapr_vio.c
> +++ b/hw/ppc/spapr_vio.c
> @@ -455,7 +455,7 @@ static void spapr_vio_busdev_realize(DeviceState *qdev, Error **errp)
> dev->qdev.id = id;
> }
>
> - dev->irq = spapr_irq_alloc(spapr, dev->irq, false, &local_err);
> + dev->irq = spapr_irq_alloc(spapr, false, &local_err);
> if (local_err) {
> error_propagate(errp, local_err);
> return;
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Qemu-devel] [PATCH 1/4] spapr: remove irq_hint parameter from spapr_irq_alloc()
2018-05-28 6:17 ` Thomas Huth
2018-05-28 7:06 ` Cédric Le Goater
@ 2018-06-02 9:19 ` Cédric Le Goater
2018-06-04 6:05 ` Cédric Le Goater
1 sibling, 1 reply; 32+ messages in thread
From: Cédric Le Goater @ 2018-06-02 9:19 UTC (permalink / raw)
To: Thomas Huth, Greg Kurz
Cc: qemu-ppc, qemu-devel, David Gibson, Alexey Kardashevskiy
On 05/28/2018 08:17 AM, Thomas Huth wrote:
> On 25.05.2018 16:02, Greg Kurz wrote:
>> On Fri, 18 May 2018 18:44:02 +0200
>> Cédric Le Goater <clg@kaod.org> wrote:
>>
>>> This IRQ number hint can possibly be used by the VIO devices if the
>>> "irq" property is defined on the command line but it seems it is never
>>> the case. It is not used in libvirt for instance. So, let's remove it
>>> to simplify future changes.
>>>
>>
>> Setting an irq manually looks a bit anachronistic. I doubt anyone would
>> do that nowadays, and the patch does a nice cleanup. So this looks like
>> a good idea.
> [...]
>>> diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c
>>> index 472dd6f33a96..cc064f64fccf 100644
>>> --- a/hw/ppc/spapr_vio.c
>>> +++ b/hw/ppc/spapr_vio.c
>>> @@ -455,7 +455,7 @@ static void spapr_vio_busdev_realize(DeviceState *qdev, Error **errp)
>>> dev->qdev.id = id;
>>> }
>>>
>>> - dev->irq = spapr_irq_alloc(spapr, dev->irq, false, &local_err);
>>> + dev->irq = spapr_irq_alloc(spapr, false, &local_err);
>>
>> Silently breaking "irq" like this looks wrong. I'd rather officially remove
>> it first (ie, kill spapr_vio_props, -5 lines in spapr_vio.c).
>>
>> Of course, this raises the question of interface deprecation, and it should
>> theoretically follow the process described at:
>>
>> https://wiki.qemu.org/Features/LegacyRemoval#Rules_for_removing_an_interface
>>
>> Cc'ing Thomas, our Chief Deprecation Officer, for insights :)
>
> The property is a public interface. Just because it's not used by
> libvirt does not mean that nobody is using it. So yes, please follow the
> rules and mark it as deprecated first for two release, before you really
> remove it.
I don't see a section for 'Deprecated' properties in the qemu-doc files.
C.
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Qemu-devel] [PATCH 1/4] spapr: remove irq_hint parameter from spapr_irq_alloc()
2018-06-02 9:19 ` Cédric Le Goater
@ 2018-06-04 6:05 ` Cédric Le Goater
0 siblings, 0 replies; 32+ messages in thread
From: Cédric Le Goater @ 2018-06-04 6:05 UTC (permalink / raw)
To: Thomas Huth, Greg Kurz
Cc: qemu-ppc, qemu-devel, David Gibson, Alexey Kardashevskiy
>> The property is a public interface. Just because it's not used by
>> libvirt does not mean that nobody is using it. So yes, please follow the
>> rules and mark it as deprecated first for two release, before you really
>> remove it.
>
> I don't see a section for 'Deprecated' properties in the qemu-doc files.
So I added a new one, which gives us :
B.7 Device options
B.7.1 Block device options
B.7.1.1 "backing": "" (since 2.12.0)
B.7.2 vio-spapr-device device options
B.7.2.1 "irq": "" (since 2.13.0)
Cheers,
C.
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Qemu-devel] [PATCH 1/4] spapr: remove irq_hint parameter from spapr_irq_alloc()
2018-05-28 7:06 ` Cédric Le Goater
2018-05-28 7:18 ` [Qemu-devel] [Qemu-ppc] " Thomas Huth
@ 2018-06-05 3:34 ` David Gibson
2018-06-05 6:41 ` Cédric Le Goater
1 sibling, 1 reply; 32+ messages in thread
From: David Gibson @ 2018-06-05 3:34 UTC (permalink / raw)
To: Cédric Le Goater
Cc: Thomas Huth, Greg Kurz, qemu-ppc, qemu-devel, Alexey Kardashevskiy
[-- Attachment #1: Type: text/plain, Size: 3254 bytes --]
On Mon, May 28, 2018 at 09:06:12AM +0200, Cédric Le Goater wrote:
> On 05/28/2018 08:17 AM, Thomas Huth wrote:
> > On 25.05.2018 16:02, Greg Kurz wrote:
> >> On Fri, 18 May 2018 18:44:02 +0200
> >> Cédric Le Goater <clg@kaod.org> wrote:
> >>
> >>> This IRQ number hint can possibly be used by the VIO devices if the
> >>> "irq" property is defined on the command line but it seems it is never
> >>> the case. It is not used in libvirt for instance. So, let's remove it
> >>> to simplify future changes.
> >>>
> >>
> >> Setting an irq manually looks a bit anachronistic. I doubt anyone would
> >> do that nowadays, and the patch does a nice cleanup. So this looks like
> >> a good idea.
> > [...]
> >>> diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c
> >>> index 472dd6f33a96..cc064f64fccf 100644
> >>> --- a/hw/ppc/spapr_vio.c
> >>> +++ b/hw/ppc/spapr_vio.c
> >>> @@ -455,7 +455,7 @@ static void spapr_vio_busdev_realize(DeviceState *qdev, Error **errp)
> >>> dev->qdev.id = id;
> >>> }
> >>>
> >>> - dev->irq = spapr_irq_alloc(spapr, dev->irq, false, &local_err);
> >>> + dev->irq = spapr_irq_alloc(spapr, false, &local_err);
> >>
> >> Silently breaking "irq" like this looks wrong. I'd rather officially remove
> >> it first (ie, kill spapr_vio_props, -5 lines in spapr_vio.c).
> >>
> >> Of course, this raises the question of interface deprecation, and it should
> >> theoretically follow the process described at:
> >>
> >> https://wiki.qemu.org/Features/LegacyRemoval#Rules_for_removing_an_interface
> >>
> >> Cc'ing Thomas, our Chief Deprecation Officer, for insights :)
> >
> > The property is a public interface. Just because it's not used by
> > libvirt does not mean that nobody is using it. So yes, please follow the
> > rules and mark it as deprecated first for two release, before you really
> > remove it.
>
> This "irq" property is a problem to introduce a new static layout of IRQ
> numbers. It is in complete opposition.
>
> Can we keep it as it is for old pseries machine (settable) and ignore it
> for newer ? Would that be fine ?
So, Thomas is right that we need to keep the interface while we go
through the deprecation process, even though it's a bit of a pain
(like you, I seriously doubt anyone ever used it).
But, I think there's a way to avoid that getting in the way of your
cleanups too much.
A bunch of the current problems are caused because spapr_irq_alloc()
conflates two meanings of "allocate": 1) finding a free irq to use for
this device and 2) assigning that irq exclusively to this device.
I think the first thing to do is to split those two parts. (1) will
never take an irq parameter, (2) will always take an irq parameter.
To implement the (to be deprecated) "irq" property on vio devices you
should skip (1) and just call (2) with the given irq number.
The point of this series is to basically get rid of (1), but this
first step means we don't need to worry about the hint parameter as we
gradually remove it.
--
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 --]
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Qemu-devel] [PATCH 2/4] sparp_pci: simplify how the PCI LSIs are allocated
2018-05-26 9:40 ` Greg Kurz
@ 2018-06-05 3:44 ` David Gibson
2018-06-05 6:31 ` Cédric Le Goater
0 siblings, 1 reply; 32+ messages in thread
From: David Gibson @ 2018-06-05 3:44 UTC (permalink / raw)
To: Greg Kurz
Cc: Cédric Le Goater, qemu-ppc, qemu-devel, Alexey Kardashevskiy
[-- Attachment #1: Type: text/plain, Size: 3060 bytes --]
On Sat, May 26, 2018 at 11:40:23AM +0200, Greg Kurz wrote:
> On Fri, 18 May 2018 18:44:03 +0200
> Cédric Le Goater <clg@kaod.org> wrote:
>
> > PCI LSIs are today allocated one by one using the IRQ alloc_block
> > routine. Change the code sequence to first allocate a PCI_NUM_PINS
> > block. It will help us providing a generic IRQ framework to the
> > machine.
> >
> > Signed-off-by: Cédric Le Goater <clg@kaod.org>
> > ---
> > hw/ppc/spapr_pci.c | 21 ++++++++++-----------
> > 1 file changed, 10 insertions(+), 11 deletions(-)
> >
> > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> > index 39a14980d397..4fd97ffe4c6e 100644
> > --- a/hw/ppc/spapr_pci.c
> > +++ b/hw/ppc/spapr_pci.c
> > @@ -1546,6 +1546,8 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
> > sPAPRTCETable *tcet;
> > const unsigned windows_supported =
> > sphb->ddw_enabled ? SPAPR_PCI_DMA_MAX_WINDOWS : 1;
> > + uint32_t irq;
> > + Error *local_err = NULL;
> >
> > if (!spapr) {
> > error_setg(errp, TYPE_SPAPR_PCI_HOST_BRIDGE " needs a pseries machine");
> > @@ -1694,18 +1696,15 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
> > QLIST_INSERT_HEAD(&spapr->phbs, sphb, list);
> >
> > /* Initialize the LSI table */
> > - for (i = 0; i < PCI_NUM_PINS; i++) {
> > - uint32_t irq;
> > - Error *local_err = NULL;
> > -
> > - irq = spapr_irq_alloc_block(spapr, 1, true, false, &local_err);
> > - if (local_err) {
> > - error_propagate(errp, local_err);
> > - error_prepend(errp, "can't allocate LSIs: ");
> > - return;
> > - }
> > + irq = spapr_irq_alloc_block(spapr, PCI_NUM_PINS, true, false, &local_err);
> > + if (local_err) {
> > + error_propagate(errp, local_err);
> > + error_prepend(errp, "can't allocate LSIs: ");
> > + return;
> > + }
> >
>
> It isn't strictly equivalent. The current code would be happy with
> sparse IRQ numbers, while the proposed one wouldn't... Anyway, this
> cannot happen since we don't have PHB hotplug.
This makes me pretty nervous, because it's not obvious it will come up
with the same numbers in all circumstances, which we have to do for
existing machine types. It's also not obvious to me why it's useful
to go via this step before going straight to static allocation of the
irq numbers.
If you can convince me this will (in practice) return the same numbers
as the existing code for all valid setups, and that it's a useful
intermediate step, then I'll apply it.
>
> > - sphb->lsi_table[i].irq = irq;
> > + for (i = 0; i < PCI_NUM_PINS; i++) {
> > + sphb->lsi_table[i].irq = irq + i;
> > }
> >
> > /* allocate connectors for child PCI devices */
>
--
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 --]
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Qemu-devel] [PATCH 2/4] sparp_pci: simplify how the PCI LSIs are allocated
2018-06-05 3:44 ` David Gibson
@ 2018-06-05 6:31 ` Cédric Le Goater
2018-06-13 4:27 ` David Gibson
0 siblings, 1 reply; 32+ messages in thread
From: Cédric Le Goater @ 2018-06-05 6:31 UTC (permalink / raw)
To: David Gibson, Greg Kurz; +Cc: qemu-ppc, qemu-devel, Alexey Kardashevskiy
On 06/05/2018 05:44 AM, David Gibson wrote:
> On Sat, May 26, 2018 at 11:40:23AM +0200, Greg Kurz wrote:
>> On Fri, 18 May 2018 18:44:03 +0200
>> Cédric Le Goater <clg@kaod.org> wrote:
>>
>>> PCI LSIs are today allocated one by one using the IRQ alloc_block
>>> routine. Change the code sequence to first allocate a PCI_NUM_PINS
>>> block. It will help us providing a generic IRQ framework to the
>>> machine.
>>>
>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>> ---
>>> hw/ppc/spapr_pci.c | 21 ++++++++++-----------
>>> 1 file changed, 10 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
>>> index 39a14980d397..4fd97ffe4c6e 100644
>>> --- a/hw/ppc/spapr_pci.c
>>> +++ b/hw/ppc/spapr_pci.c
>>> @@ -1546,6 +1546,8 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
>>> sPAPRTCETable *tcet;
>>> const unsigned windows_supported =
>>> sphb->ddw_enabled ? SPAPR_PCI_DMA_MAX_WINDOWS : 1;
>>> + uint32_t irq;
>>> + Error *local_err = NULL;
>>>
>>> if (!spapr) {
>>> error_setg(errp, TYPE_SPAPR_PCI_HOST_BRIDGE " needs a pseries machine");
>>> @@ -1694,18 +1696,15 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
>>> QLIST_INSERT_HEAD(&spapr->phbs, sphb, list);
>>>
>>> /* Initialize the LSI table */
>>> - for (i = 0; i < PCI_NUM_PINS; i++) {
>>> - uint32_t irq;
>>> - Error *local_err = NULL;
>>> -
>>> - irq = spapr_irq_alloc_block(spapr, 1, true, false, &local_err);
>>> - if (local_err) {
>>> - error_propagate(errp, local_err);
>>> - error_prepend(errp, "can't allocate LSIs: ");
>>> - return;
>>> - }
>>> + irq = spapr_irq_alloc_block(spapr, PCI_NUM_PINS, true, false, &local_err);
>>> + if (local_err) {
>>> + error_propagate(errp, local_err);
>>> + error_prepend(errp, "can't allocate LSIs: ");
>>> + return;
>>> + }
>>>
>>
>> It isn't strictly equivalent. The current code would be happy with
>> sparse IRQ numbers, while the proposed one wouldn't... Anyway, this
>> cannot happen since we don't have PHB hotplug.
>
> This makes me pretty nervous, because it's not obvious it will come up
> with the same numbers in all circumstances, which we have to do for
> existing machine types.
Given that :
- irq_hint is "unused"
- all IRQs are allocated sequentially at machine init,
- spapr_pci is the only model using the block allocation for MSIs,
potentially fragmenting more the IRQ number space but done at
guest runtime.
- the PHB LSI are the allocated at realize time doing the loop above,
- we don't support PHB hotplug
- we do support PHB coldplug but then the IRQ allocation is done
at machine time,
it seems highly improbable that the IRQ number space is fragmented
to a point which would not allow the loop above to return four
contiguous IRQ numbers, always.
That is why I felt confident changing the loop to a single block
allocation.
> It's also not obvious to me why it's useful
> to go via this step before going straight to static allocation of the
> irq numbers.
It pollutes the new sPAPR IRQ interface API with an extra parameter
to support both underlying backend and it complexifies the code
to handle block allocation of a single IRQ (like above) within an
IRQ range (the PCI LSIs).
So you end up having a family, a device index, a count, an alignment,
and an index within the range. pffut.
Also, could we kill the alignment ?
C.
> If you can convince me this will (in practice) return the same numbers
> as the existing code for all valid setups, and that it's a useful
> intermediate step, then I'll apply it.
>
>>
>>> - sphb->lsi_table[i].irq = irq;
>>> + for (i = 0; i < PCI_NUM_PINS; i++) {
>>> + sphb->lsi_table[i].irq = irq + i;
>>> }
>>>
>>> /* allocate connectors for child PCI devices */
>>
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Qemu-devel] [PATCH 1/4] spapr: remove irq_hint parameter from spapr_irq_alloc()
2018-06-05 3:34 ` [Qemu-devel] " David Gibson
@ 2018-06-05 6:41 ` Cédric Le Goater
2018-06-13 4:22 ` David Gibson
0 siblings, 1 reply; 32+ messages in thread
From: Cédric Le Goater @ 2018-06-05 6:41 UTC (permalink / raw)
To: David Gibson
Cc: Thomas Huth, Greg Kurz, qemu-ppc, qemu-devel, Alexey Kardashevskiy
On 06/05/2018 05:34 AM, David Gibson wrote:
> On Mon, May 28, 2018 at 09:06:12AM +0200, Cédric Le Goater wrote:
>> On 05/28/2018 08:17 AM, Thomas Huth wrote:
>>> On 25.05.2018 16:02, Greg Kurz wrote:
>>>> On Fri, 18 May 2018 18:44:02 +0200
>>>> Cédric Le Goater <clg@kaod.org> wrote:
>>>>
>>>>> This IRQ number hint can possibly be used by the VIO devices if the
>>>>> "irq" property is defined on the command line but it seems it is never
>>>>> the case. It is not used in libvirt for instance. So, let's remove it
>>>>> to simplify future changes.
>>>>>
>>>>
>>>> Setting an irq manually looks a bit anachronistic. I doubt anyone would
>>>> do that nowadays, and the patch does a nice cleanup. So this looks like
>>>> a good idea.
>>> [...]
>>>>> diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c
>>>>> index 472dd6f33a96..cc064f64fccf 100644
>>>>> --- a/hw/ppc/spapr_vio.c
>>>>> +++ b/hw/ppc/spapr_vio.c
>>>>> @@ -455,7 +455,7 @@ static void spapr_vio_busdev_realize(DeviceState *qdev, Error **errp)
>>>>> dev->qdev.id = id;
>>>>> }
>>>>>
>>>>> - dev->irq = spapr_irq_alloc(spapr, dev->irq, false, &local_err);
>>>>> + dev->irq = spapr_irq_alloc(spapr, false, &local_err);
>>>>
>>>> Silently breaking "irq" like this looks wrong. I'd rather officially remove
>>>> it first (ie, kill spapr_vio_props, -5 lines in spapr_vio.c).
>>>>
>>>> Of course, this raises the question of interface deprecation, and it should
>>>> theoretically follow the process described at:
>>>>
>>>> https://wiki.qemu.org/Features/LegacyRemoval#Rules_for_removing_an_interface
>>>>
>>>> Cc'ing Thomas, our Chief Deprecation Officer, for insights :)
>>>
>>> The property is a public interface. Just because it's not used by
>>> libvirt does not mean that nobody is using it. So yes, please follow the
>>> rules and mark it as deprecated first for two release, before you really
>>> remove it.
>>
>> This "irq" property is a problem to introduce a new static layout of IRQ
>> numbers. It is in complete opposition.
>>
>> Can we keep it as it is for old pseries machine (settable) and ignore it
>> for newer ? Would that be fine ?
>
> So, Thomas is right that we need to keep the interface while we go
> through the deprecation process, even though it's a bit of a pain
> (like you, I seriously doubt anyone ever used it).
That's OK. The patch is simple. But it means that we have to keep the
irq_hint parameter for 2 QEMU versions.
> But, I think there's a way to avoid that getting in the way of your
> cleanups too much.
>
> A bunch of the current problems are caused because spapr_irq_alloc()
> conflates two meanings of "allocate": 1) finding a free irq to use for
> this device and 2) assigning that irq exclusively to this device.
>
> I think the first thing to do is to split those two parts. (1) will
> never take an irq parameter, (2) will always take an irq parameter.
> To implement the (to be deprecated) "irq" property on vio devices you
> should skip (1) and just call (2) with the given irq number.
well, we need to call both because if "irq" is zero then when we
fallback to "1) finding a free irq to use."
But we can move the exclusive IRQ assignment (2) under the VIO model
which is the only one using it and start deprecating the property.
> The point of this series is to basically get rid of (1), but this
> first step means we don't need to worry about the hint parameter as we
> gradually remove it.
OK. I think I got what you are asking for. (2) means adding an extra
handler to the sPAPR IRQ interface, which would always fail in the
new XICS sPAPR IRQ backend using static numbers.
Thanks,
C.
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Qemu-devel] [PATCH 1/4] spapr: remove irq_hint parameter from spapr_irq_alloc()
2018-06-05 6:41 ` Cédric Le Goater
@ 2018-06-13 4:22 ` David Gibson
2018-06-13 7:24 ` Cédric Le Goater
0 siblings, 1 reply; 32+ messages in thread
From: David Gibson @ 2018-06-13 4:22 UTC (permalink / raw)
To: Cédric Le Goater
Cc: Thomas Huth, Greg Kurz, qemu-ppc, qemu-devel, Alexey Kardashevskiy
[-- Attachment #1: Type: text/plain, Size: 4867 bytes --]
On Tue, Jun 05, 2018 at 08:41:13AM +0200, Cédric Le Goater wrote:
> On 06/05/2018 05:34 AM, David Gibson wrote:
> > On Mon, May 28, 2018 at 09:06:12AM +0200, Cédric Le Goater wrote:
> >> On 05/28/2018 08:17 AM, Thomas Huth wrote:
> >>> On 25.05.2018 16:02, Greg Kurz wrote:
> >>>> On Fri, 18 May 2018 18:44:02 +0200
> >>>> Cédric Le Goater <clg@kaod.org> wrote:
> >>>>
> >>>>> This IRQ number hint can possibly be used by the VIO devices if the
> >>>>> "irq" property is defined on the command line but it seems it is never
> >>>>> the case. It is not used in libvirt for instance. So, let's remove it
> >>>>> to simplify future changes.
> >>>>>
> >>>>
> >>>> Setting an irq manually looks a bit anachronistic. I doubt anyone would
> >>>> do that nowadays, and the patch does a nice cleanup. So this looks like
> >>>> a good idea.
> >>> [...]
> >>>>> diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c
> >>>>> index 472dd6f33a96..cc064f64fccf 100644
> >>>>> --- a/hw/ppc/spapr_vio.c
> >>>>> +++ b/hw/ppc/spapr_vio.c
> >>>>> @@ -455,7 +455,7 @@ static void spapr_vio_busdev_realize(DeviceState *qdev, Error **errp)
> >>>>> dev->qdev.id = id;
> >>>>> }
> >>>>>
> >>>>> - dev->irq = spapr_irq_alloc(spapr, dev->irq, false, &local_err);
> >>>>> + dev->irq = spapr_irq_alloc(spapr, false, &local_err);
> >>>>
> >>>> Silently breaking "irq" like this looks wrong. I'd rather officially remove
> >>>> it first (ie, kill spapr_vio_props, -5 lines in spapr_vio.c).
> >>>>
> >>>> Of course, this raises the question of interface deprecation, and it should
> >>>> theoretically follow the process described at:
> >>>>
> >>>> https://wiki.qemu.org/Features/LegacyRemoval#Rules_for_removing_an_interface
> >>>>
> >>>> Cc'ing Thomas, our Chief Deprecation Officer, for insights :)
> >>>
> >>> The property is a public interface. Just because it's not used by
> >>> libvirt does not mean that nobody is using it. So yes, please follow the
> >>> rules and mark it as deprecated first for two release, before you really
> >>> remove it.
> >>
> >> This "irq" property is a problem to introduce a new static layout of IRQ
> >> numbers. It is in complete opposition.
> >>
> >> Can we keep it as it is for old pseries machine (settable) and ignore it
> >> for newer ? Would that be fine ?
> >
> > So, Thomas is right that we need to keep the interface while we go
> > through the deprecation process, even though it's a bit of a pain
> > (like you, I seriously doubt anyone ever used it).
>
> That's OK. The patch is simple. But it means that we have to keep the
> irq_hint parameter for 2 QEMU versions.
No.. the suggestion below is designed to avoid that..
> > But, I think there's a way to avoid that getting in the way of your
> > cleanups too much.
> >
> > A bunch of the current problems are caused because spapr_irq_alloc()
> > conflates two meanings of "allocate": 1) finding a free irq to use for
> > this device and 2) assigning that irq exclusively to this device.
> >
> > I think the first thing to do is to split those two parts. (1) will
> > never take an irq parameter, (2) will always take an irq parameter.
> > To implement the (to be deprecated) "irq" property on vio devices you
> > should skip (1) and just call (2) with the given irq number.
>
> well, we need to call both because if "irq" is zero then when we
> fallback to "1) finding a free irq to use."
No, basically in the VIO code itself you'd have:
irq = <irq property value>;
if (!irq)
irq = find_irq()
claim_irq(irq);
find_irq() never takes a hint, claim_irq() always does (except it's
not really a hint).
> But we can move the exclusive IRQ assignment (2) under the VIO model
> which is the only one using it and start deprecating the property.
No.. the exclusive claim would be global - everything would use that.
> > The point of this series is to basically get rid of (1), but this
> > first step means we don't need to worry about the hint parameter as we
> > gradually remove it.
>
> OK. I think I got what you are asking for. (2) means adding an extra
> handler to the sPAPR IRQ interface, which would always fail in the
> new XICS sPAPR IRQ backend using static numbers.
No.. (2), "claim_irq()" as I called it above, would _always_ be used.
find_irq() would only be used to implement the legacy allocation.
In various places we'll have code like this:
if (legacy) {
irq = find_irq();
} else {
irq = <fixed value or formula>;
}
claim_irq(irq);
Where that fixed value could be something like:
irq = PCI_LSI_BASE + phb->index*4 + pin#;
--
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 --]
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Qemu-devel] [PATCH 2/4] sparp_pci: simplify how the PCI LSIs are allocated
2018-06-05 6:31 ` Cédric Le Goater
@ 2018-06-13 4:27 ` David Gibson
2018-06-13 7:26 ` Cédric Le Goater
0 siblings, 1 reply; 32+ messages in thread
From: David Gibson @ 2018-06-13 4:27 UTC (permalink / raw)
To: Cédric Le Goater
Cc: Greg Kurz, qemu-ppc, qemu-devel, Alexey Kardashevskiy
[-- Attachment #1: Type: text/plain, Size: 4575 bytes --]
On Tue, Jun 05, 2018 at 08:31:49AM +0200, Cédric Le Goater wrote:
> On 06/05/2018 05:44 AM, David Gibson wrote:
> > On Sat, May 26, 2018 at 11:40:23AM +0200, Greg Kurz wrote:
> >> On Fri, 18 May 2018 18:44:03 +0200
> >> Cédric Le Goater <clg@kaod.org> wrote:
> >>
> >>> PCI LSIs are today allocated one by one using the IRQ alloc_block
> >>> routine. Change the code sequence to first allocate a PCI_NUM_PINS
> >>> block. It will help us providing a generic IRQ framework to the
> >>> machine.
> >>>
> >>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> >>> ---
> >>> hw/ppc/spapr_pci.c | 21 ++++++++++-----------
> >>> 1 file changed, 10 insertions(+), 11 deletions(-)
> >>>
> >>> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> >>> index 39a14980d397..4fd97ffe4c6e 100644
> >>> --- a/hw/ppc/spapr_pci.c
> >>> +++ b/hw/ppc/spapr_pci.c
> >>> @@ -1546,6 +1546,8 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
> >>> sPAPRTCETable *tcet;
> >>> const unsigned windows_supported =
> >>> sphb->ddw_enabled ? SPAPR_PCI_DMA_MAX_WINDOWS : 1;
> >>> + uint32_t irq;
> >>> + Error *local_err = NULL;
> >>>
> >>> if (!spapr) {
> >>> error_setg(errp, TYPE_SPAPR_PCI_HOST_BRIDGE " needs a pseries machine");
> >>> @@ -1694,18 +1696,15 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
> >>> QLIST_INSERT_HEAD(&spapr->phbs, sphb, list);
> >>>
> >>> /* Initialize the LSI table */
> >>> - for (i = 0; i < PCI_NUM_PINS; i++) {
> >>> - uint32_t irq;
> >>> - Error *local_err = NULL;
> >>> -
> >>> - irq = spapr_irq_alloc_block(spapr, 1, true, false, &local_err);
> >>> - if (local_err) {
> >>> - error_propagate(errp, local_err);
> >>> - error_prepend(errp, "can't allocate LSIs: ");
> >>> - return;
> >>> - }
> >>> + irq = spapr_irq_alloc_block(spapr, PCI_NUM_PINS, true, false, &local_err);
> >>> + if (local_err) {
> >>> + error_propagate(errp, local_err);
> >>> + error_prepend(errp, "can't allocate LSIs: ");
> >>> + return;
> >>> + }
> >>>
> >>
> >> It isn't strictly equivalent. The current code would be happy with
> >> sparse IRQ numbers, while the proposed one wouldn't... Anyway, this
> >> cannot happen since we don't have PHB hotplug.
> >
> > This makes me pretty nervous, because it's not obvious it will come up
> > with the same numbers in all circumstances, which we have to do for
> > existing machine types.
>
> Given that :
>
> - irq_hint is "unused"
> - all IRQs are allocated sequentially at machine init,
> - spapr_pci is the only model using the block allocation for MSIs,
> potentially fragmenting more the IRQ number space but done at
> guest runtime.
> - the PHB LSI are the allocated at realize time doing the loop above,
> - we don't support PHB hotplug
> - we do support PHB coldplug but then the IRQ allocation is done
> at machine time,
>
> it seems highly improbable that the IRQ number space is fragmented
> to a point which would not allow the loop above to return four
> contiguous IRQ numbers, always.
Well, assuming irq_hint really is unused, that's right. But we can't
assume that - that's the whole point of the deprecation thing.
Given that, AIUI, just one vio device with irq= set to a value that
would be within an LSI block allocated under the old scheme would
result in the new scheme returning a non-contiguous set of LSIs -
i.e. a different result from what we used to have.
> That is why I felt confident changing the loop to a single block
> allocation.
>
> > It's also not obvious to me why it's useful
> > to go via this step before going straight to static allocation of the
> > irq numbers.
>
> It pollutes the new sPAPR IRQ interface API with an extra parameter
> to support both underlying backend and it complexifies the code
> to handle block allocation of a single IRQ (like above) within an
> IRQ range (the PCI LSIs).
>
> So you end up having a family, a device index, a count, an alignment,
> and an index within the range. pffut.
>
> Also, could we kill the alignment ?
Since we sometimes pass 'true', no, we can't, without changing the
existing pattern of allocations, which we can't do.
>
--
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 --]
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Qemu-devel] [PATCH 3/4] spapr: introduce a generic IRQ frontend to the machine
2018-05-18 16:44 ` [Qemu-devel] [PATCH 3/4] spapr: introduce a generic IRQ frontend to the machine Cédric Le Goater
2018-05-28 14:27 ` Greg Kurz
@ 2018-06-13 5:00 ` David Gibson
2018-06-13 7:44 ` Cédric Le Goater
1 sibling, 1 reply; 32+ messages in thread
From: David Gibson @ 2018-06-13 5:00 UTC (permalink / raw)
To: Cédric Le Goater
Cc: qemu-ppc, qemu-devel, Greg Kurz, Alexey Kardashevskiy
[-- Attachment #1: Type: text/plain, Size: 27542 bytes --]
On Fri, May 18, 2018 at 06:44:04PM +0200, Cédric Le Goater wrote:
> This proposal moves all the related IRQ routines of the sPAPR machine
> behind a class interface to prepare for future changes in the IRQ
> controller model. First of which is a reorganization of the IRQ number
> space layout and a second, coming later, will be to integrate the
> support for the new POWER9 XIVE IRQ controller.
>
> The new interface defines a set of fixed IRQ number ranges, for each
> IRQ type, in which devices allocate the IRQ numbers they need
> depending on a unique device index. Here is the layout :
>
> SPAPR_IRQ_IPI 0x0 /* 1 IRQ per CPU */
> SPAPR_IRQ_EPOW 0x1000 /* 1 IRQ per device */
> SPAPR_IRQ_HOTPLUG 0x1001 /* 1 IRQ per device */
> SPAPR_IRQ_VIO 0x1100 /* 1 IRQ per device */
> SPAPR_IRQ_PCI_LSI 0x1200 /* 4 IRQs per device */
> SPAPR_IRQ_PCI_MSI 0x1400 /* 1K IRQs per device */
>
> The IPI range is reserved for future use when XIVE support
> comes in.
>
> The routines of this interface encompass the previous needs and the
> new ones and seem complex but the provided IRQ backend should
> implement what we have today without any functional changes.
>
> Each device model is modified to take the new interface into account
> using the IRQ range/type definitions and a device index. A part from
> the VIO devices, lacking an id, the changes are relatively simple.
I find your use of "back end" vs. "front end" in this patch and the
next kind of confusing.
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
> include/hw/ppc/spapr.h | 10 +-
> include/hw/ppc/spapr_irq.h | 46 +++++++++
> hw/ppc/spapr.c | 177 +---------------------------------
> hw/ppc/spapr_events.c | 7 +-
> hw/ppc/spapr_irq.c | 233 +++++++++++++++++++++++++++++++++++++++++++++
> hw/ppc/spapr_pci.c | 21 +++-
> hw/ppc/spapr_vio.c | 5 +-
> hw/ppc/Makefile.objs | 2 +-
> 8 files changed, 308 insertions(+), 193 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 2cfdfdd67eaf..4eb212b16a51 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -3,10 +3,10 @@
>
> #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"
> +#include "hw/ppc/spapr_irq.h"
>
> struct VIOsPAPRBus;
> struct sPAPRPHBState;
> @@ -104,6 +104,7 @@ struct sPAPRMachineClass {
> unsigned n_dma, uint32_t *liobns, Error **errp);
> sPAPRResizeHPT resize_hpt_default;
> sPAPRCapabilities default_caps;
> + sPAPRIrq *irq;
> };
>
> /**
> @@ -773,13 +774,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_alloc(sPAPRMachineState *spapr, bool lsi, Error **errp);
> -int spapr_irq_alloc_block(sPAPRMachineState *spapr, int num, bool lsi,
> - bool align, 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
> new file mode 100644
> index 000000000000..caf4c33d4cec
> --- /dev/null
> +++ b/include/hw/ppc/spapr_irq.h
> @@ -0,0 +1,46 @@
> +/*
> + * 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
> +
> +#include "hw/ppc/xics.h"
> +
> +/*
> + * IRQ ranges
> + */
> +#define SPAPR_IRQ_IPI 0x0 /* 1 IRQ per CPU */
> +#define SPAPR_IRQ_EPOW 0x1000 /* 1 IRQ per device */
> +#define SPAPR_IRQ_HOTPLUG 0x1001 /* 1 IRQ per device */
> +#define SPAPR_IRQ_VIO 0x1100 /* 1 IRQ per device */
> +#define SPAPR_IRQ_PCI_LSI 0x1200 /* 4 IRQs per device */
> +#define SPAPR_IRQ_PCI_MSI 0x1400 /* 1K IRQs per device covered by
> + * a bitmap allocator */
These look like they belong in the next patch with the fixed allocations.
> +typedef struct sPAPRIrq {
Much of this structure doesn't make sense to me. AIUI, the idea is
that this method structure will vary with the xics vs. xive backend,
yes? Comments below are based on that assumption.
> + uint32_t nr_irqs;
> +
> + void (*init)(sPAPRMachineState *spapr, Error **errp);
> + int (*alloc)(sPAPRMachineState *spapr, uint32_t range, uint32_t index,
> + Error **errp);
'range' has no place here - working out the indices should be entirely
on the spapr side, only the final irq number should need to go to the
backend.
I'd also prefer to call it "claim" to distinguish it from the existing
"alloc" function which finds a free irq as well as setting it up for
our exclusive use.
> + int (*alloc_block)(sPAPRMachineState *spapr, uint32_t range,
> + uint32_t index, int num, bool align, Error **errp);
There should be no need for this. We needed an alloc_block routine
before, because we wanted to ensure that we got a contiguous (and
maybe aligned) block of irqs. By the time we go to the backend we
should already have absolute irq numbers, so we can just claim each of
them separately.
> + void (*free)(sPAPRMachineState *spapr, int irq, int num, Error **errp);
> + qemu_irq (*qirq)(sPAPRMachineState *spapr, int irq);
> +} sPAPRIrq;
> +
> +extern sPAPRIrq spapr_irq_default;
> +
> +int spapr_irq_alloc(sPAPRMachineState *spapr, uint32_t range, uint32_t index,
> + Error **errp);
> +int spapr_irq_alloc_block(sPAPRMachineState *spapr, uint32_t range,
> + uint32_t index, int num, bool align, Error **errp);
> +void spapr_irq_free(sPAPRMachineState *spapr, int irq, int num, Error **errp);
> +qemu_irq spapr_qirq(sPAPRMachineState *spapr, int irq);
> +
> +#endif /* HW_SPAPR_IRQ_H */
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 05a924a5f2da..09f095d73eae 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,32 +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);
> -
> - if (kvm_enabled()) {
> - if (machine_kernel_irqchip_allowed(machine) &&
> - !xics_kvm_init(spapr, errp)) {
> - spapr->icp_type = TYPE_KVM_ICP;
> - spapr->ics = spapr_ics_create(spapr, TYPE_ICS_KVM, nr_irqs, errp);
> - }
> - if (machine_kernel_irqchip_required(machine) && !spapr->ics) {
> - error_prepend(errp, "kernel_irqchip requested but unavailable: ");
> - return;
> - }
> - }
> -
> - if (!spapr->ics) {
> - xics_spapr_init(spapr);
> - spapr->icp_type = TYPE_ICP;
> - spapr->ics = spapr_ics_create(spapr, TYPE_ICS_SIMPLE, nr_irqs, errp);
> - if (!spapr->ics) {
> - return;
> - }
> - }
> -}
> -
> static int spapr_fixup_cpu_smt_dt(void *fdt, int offset, PowerPCCPU *cpu,
> int smt_threads)
> {
> @@ -2580,7 +2527,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
> */
> @@ -3766,127 +3713,6 @@ 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;
> -}
> -
> -/*
> - * Allocate the IRQ number and set the IRQ type, LSI or MSI
> - */
> -static void spapr_irq_set_lsi(sPAPRMachineState *spapr, int irq, bool lsi)
> -{
> - ics_set_irq_type(spapr->ics, irq - spapr->ics->offset, lsi);
> -}
> -
> -int spapr_irq_alloc(sPAPRMachineState *spapr, bool lsi, Error **errp)
> -{
> - ICSState *ics = spapr->ics;
> - int irq;
> -
> - assert(ics);
> -
> - irq = ics_find_free_block(ics, 1, 1);
> - if (irq < 0) {
> - error_setg(errp, "can't allocate IRQ: no IRQ left");
> - return -1;
> - }
> - irq += ics->offset;
> -
> - spapr_irq_set_lsi(spapr, irq, lsi);
> - trace_spapr_irq_alloc(irq);
> -
> - return irq;
> -}
> -
> -/*
> - * Allocate block of consecutive IRQs, and return the number of the first IRQ in
> - * the block. If align==true, aligns the first IRQ number to num.
> - */
> -int spapr_irq_alloc_block(sPAPRMachineState *spapr, int num, bool lsi,
> - bool align, Error **errp)
> -{
> - ICSState *ics = spapr->ics;
> - int i, 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;
> - }
> -
> - first += ics->offset;
> - for (i = first; i < first + num; ++i) {
> - spapr_irq_set_lsi(spapr, i, lsi);
> - }
> -
> - trace_spapr_irq_alloc_block(first, num, lsi, align);
> -
> - return first;
> -}
> -
> -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)
> {
> @@ -4007,6 +3833,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_default;
> }
>
> static const TypeInfo spapr_machine_info = {
> diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
> index 64a67439beac..e457c5f18189 100644
> --- a/hw/ppc/spapr_events.c
> +++ b/hw/ppc/spapr_events.c
> @@ -712,7 +712,8 @@ void spapr_events_init(sPAPRMachineState *spapr)
> spapr->event_sources = spapr_event_sources_new();
>
> spapr_event_sources_register(spapr->event_sources, EVENT_CLASS_EPOW,
> - spapr_irq_alloc(spapr, false, &error_fatal));
> + spapr_irq_alloc(spapr, SPAPR_IRQ_EPOW, 0,
> + &error_fatal));
>
> /* NOTE: if machine supports modern/dedicated hotplug event source,
> * we add it to the device-tree unconditionally. This means we may
> @@ -724,8 +725,8 @@ void spapr_events_init(sPAPRMachineState *spapr)
> */
> if (spapr->use_hotplug_event_source) {
> spapr_event_sources_register(spapr->event_sources, EVENT_CLASS_HOT_PLUG,
> - spapr_irq_alloc(spapr, false,
> - &error_fatal));
> + spapr_irq_alloc(spapr, SPAPR_IRQ_HOTPLUG,
> + 0, &error_fatal));
> }
>
> spapr->epow_notifier.notify = spapr_powerdown_req;
> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> new file mode 100644
> index 000000000000..ff6cb1aafd25
> --- /dev/null
> +++ b/hw/ppc/spapr_irq.c
> @@ -0,0 +1,233 @@
> +/*
> + * QEMU PowerPC sPAPR IRQ backend
> + *
> + * 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 "qapi/error.h"
> +#include "hw/pci/pci.h"
> +#include "hw/ppc/spapr.h"
> +#include "sysemu/kvm.h"
> +#include "trace.h"
> +
> +/*
> + * Legacy XICS IRQ backend.
> + *
> + * The device IRQ 'range' is used to identify LSIs, and the device
> + * 'index' is unused
> + */
Ugh.. ok. This backend/frontend interface seems to be conflating
things that vary with xics vs. xive (what I'd think of as the "back
end") and things that vary with our irq allocation model. Those
really need to be separate.
An ops structure might make sense for the xics vs. xive stuff. I'm
pretty sure it will be overkill for the allocation model.
> +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_2_12(sPAPRMachineState *spapr, Error **errp)
> +{
> + MachineState *machine = MACHINE(spapr);
> + sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(machine);
> + uint32_t nr_irqs = smc->irq->nr_irqs;
> +
> + if (kvm_enabled()) {
> + if (machine_kernel_irqchip_allowed(machine) &&
> + !xics_kvm_init(spapr, errp)) {
> + spapr->icp_type = TYPE_KVM_ICP;
> + spapr->ics = spapr_ics_create(spapr, TYPE_ICS_KVM, nr_irqs, errp);
> + }
> + if (machine_kernel_irqchip_required(machine) && !spapr->ics) {
> + error_prepend(errp, "kernel_irqchip requested but unavailable: ");
> + return;
> + }
> + }
> +
> + if (!spapr->ics) {
> + xics_spapr_init(spapr);
> + spapr->icp_type = TYPE_ICP;
> + spapr->ics = spapr_ics_create(spapr, TYPE_ICS_SIMPLE, nr_irqs, errp);
> + if (!spapr->ics) {
> + return;
> + }
> + }
> +}
> +
> +#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;
> +}
> +
> +static int spapr_irq_alloc_2_12(sPAPRMachineState *spapr,
> + uint32_t range, uint32_t index, Error **errp)
> +{
> + ICSState *ics = spapr->ics;
> + bool lsi = (range == SPAPR_IRQ_PCI_LSI);
> + int srcno;
> +
> + assert(ics);
> +
> + srcno = ics_find_free_block(ics, 1, 1);
> + if (srcno < 0) {
> + error_setg(errp, "can't allocate IRQ: no IRQ left");
> + return -1;
> + }
> +
> + ics_set_irq_type(ics, srcno, lsi);
> + trace_spapr_irq_alloc(srcno);
> +
> + return ics->offset + srcno;
> +}
> +
> +/*
> + * Allocate block of consecutive IRQs, and return the number of the first IRQ in
> + * the block. If align==true, aligns the first IRQ number to num.
> + */
> +static int spapr_irq_alloc_block_2_12(sPAPRMachineState *spapr, uint32_t range,
> + uint32_t index, int num, bool align,
> + Error **errp)
> +{
> + ICSState *ics = spapr->ics;
> + bool lsi = (range == SPAPR_IRQ_PCI_LSI);
> + int i, srcno;
> +
> + assert(ics);
> +
> + /*
> + * MSIMessage::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.
> + */
> + if (align) {
> + assert((num == 1) || (num == 2) || (num == 4) ||
> + (num == 8) || (num == 16) || (num == 32));
> + srcno = ics_find_free_block(ics, num, num);
> + } else {
> + srcno = ics_find_free_block(ics, num, 1);
> + }
> +
> + if (srcno < 0) {
> + error_setg(errp, "can't find a free %d-IRQ block", num);
> + return -1;
> + }
> +
> + for (i = srcno; i < srcno + num; ++i) {
> + ics_set_irq_type(ics, i, lsi);
> + }
> +
> + trace_spapr_irq_alloc_block(srcno, num, lsi, align);
> +
> + return ics->offset + srcno;
> +}
> +
> +static void spapr_irq_free_2_12(sPAPRMachineState *spapr, int irq, int num,
> + Error **errp)
> +{
> + 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_2_12(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;
> +}
> +
> +sPAPRIrq spapr_irq_default = {
> + .nr_irqs = XICS_IRQS_SPAPR,
> + .init = spapr_irq_init_2_12,
> + .alloc = spapr_irq_alloc_2_12,
> + .alloc_block = spapr_irq_alloc_block_2_12,
> + .free = spapr_irq_free_2_12,
> + .qirq = spapr_qirq_2_12,
> +};
> +
> +int spapr_irq_alloc(sPAPRMachineState *spapr, uint32_t range, uint32_t index,
> + Error **errp)
> +{
> + sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
> +
> + return smc->irq->alloc(spapr, range, index, errp);
> +}
> +
> +int spapr_irq_alloc_block(sPAPRMachineState *spapr, uint32_t range,
> + uint32_t index, int num, bool align, Error **errp)
> +{
> + sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
> +
> + return smc->irq->alloc_block(spapr, range, index, num, align, errp);
> +}
> +
> +void spapr_irq_free(sPAPRMachineState *spapr, int irq, int num,
> + Error **errp)
> +{
> + sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
> +
> + smc->irq->free(spapr, irq, num, errp);
> +}
> +
> +qemu_irq spapr_qirq(sPAPRMachineState *spapr, int irq)
> +{
> + sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
> +
> + return smc->irq->qirq(spapr, irq);
> +}
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 4fd97ffe4c6e..cca4169fa10b 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -333,7 +333,12 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> return;
> }
>
> - spapr_irq_free(spapr, msi->first_irq, msi->num);
> + spapr_irq_free(spapr, msi->first_irq, msi->num, &err);
> + if (err) {
> + error_reportf_err(err, "Can't remove MSIs for device %x: ",
> + config_addr);
> + rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
> + }
> if (msi_present(pdev)) {
> spapr_msi_setmsg(pdev, 0, false, 0, 0);
> }
> @@ -371,8 +376,8 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> }
>
> /* Allocate MSIs */
> - irq = spapr_irq_alloc_block(spapr, req_num, false,
> - ret_intr_type == RTAS_TYPE_MSI, &err);
> + irq = spapr_irq_alloc_block(spapr, SPAPR_IRQ_PCI_MSI, phb->index, req_num,
> + ret_intr_type == RTAS_TYPE_MSI, &err);
> if (err) {
> error_reportf_err(err, "Can't allocate MSIs for device %x: ",
> config_addr);
> @@ -382,7 +387,11 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>
> /* Release previous MSIs */
> if (msi) {
> - spapr_irq_free(spapr, msi->first_irq, msi->num);
> + spapr_irq_free(spapr, msi->first_irq, msi->num, &err);
> + if (err) {
> + error_reportf_err(err, "Can't remove MSIs for device %x: ",
> + config_addr);
> + }
> g_hash_table_remove(phb->msi, &config_addr);
> }
>
> @@ -1696,7 +1705,8 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
> QLIST_INSERT_HEAD(&spapr->phbs, sphb, list);
>
> /* Initialize the LSI table */
> - irq = spapr_irq_alloc_block(spapr, PCI_NUM_PINS, true, false, &local_err);
> + irq = spapr_irq_alloc_block(spapr, SPAPR_IRQ_PCI_LSI, sphb->index,
> + PCI_NUM_PINS, false, &local_err);
> if (local_err) {
> error_propagate(errp, local_err);
> error_prepend(errp, "can't allocate LSIs: ");
> @@ -2112,6 +2122,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: fix 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 cc064f64fccf..7ec69a29d806 100644
> --- a/hw/ppc/spapr_vio.c
> +++ b/hw/ppc/spapr_vio.c
> @@ -416,6 +416,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());
> @@ -455,7 +458,7 @@ static void spapr_vio_busdev_realize(DeviceState *qdev, Error **errp)
> dev->qdev.id = id;
> }
>
> - dev->irq = spapr_irq_alloc(spapr, false, &local_err);
> + dev->irq = spapr_irq_alloc(spapr, SPAPR_IRQ_VIO, vio_index++, &local_err);
> if (local_err) {
> error_propagate(errp, local_err);
> 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)
--
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 --]
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Qemu-devel] [PATCH 1/4] spapr: remove irq_hint parameter from spapr_irq_alloc()
2018-06-13 4:22 ` David Gibson
@ 2018-06-13 7:24 ` Cédric Le Goater
2018-06-14 3:46 ` David Gibson
0 siblings, 1 reply; 32+ messages in thread
From: Cédric Le Goater @ 2018-06-13 7:24 UTC (permalink / raw)
To: David Gibson
Cc: Thomas Huth, Greg Kurz, qemu-ppc, qemu-devel, Alexey Kardashevskiy
On 06/13/2018 06:22 AM, David Gibson wrote:
> On Tue, Jun 05, 2018 at 08:41:13AM +0200, Cédric Le Goater wrote:
>> On 06/05/2018 05:34 AM, David Gibson wrote:
>>> On Mon, May 28, 2018 at 09:06:12AM +0200, Cédric Le Goater wrote:
>>>> On 05/28/2018 08:17 AM, Thomas Huth wrote:
>>>>> On 25.05.2018 16:02, Greg Kurz wrote:
>>>>>> On Fri, 18 May 2018 18:44:02 +0200
>>>>>> Cédric Le Goater <clg@kaod.org> wrote:
>>>>>>
>>>>>>> This IRQ number hint can possibly be used by the VIO devices if the
>>>>>>> "irq" property is defined on the command line but it seems it is never
>>>>>>> the case. It is not used in libvirt for instance. So, let's remove it
>>>>>>> to simplify future changes.
>>>>>>>
>>>>>>
>>>>>> Setting an irq manually looks a bit anachronistic. I doubt anyone would
>>>>>> do that nowadays, and the patch does a nice cleanup. So this looks like
>>>>>> a good idea.
>>>>> [...]
>>>>>>> diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c
>>>>>>> index 472dd6f33a96..cc064f64fccf 100644
>>>>>>> --- a/hw/ppc/spapr_vio.c
>>>>>>> +++ b/hw/ppc/spapr_vio.c
>>>>>>> @@ -455,7 +455,7 @@ static void spapr_vio_busdev_realize(DeviceState *qdev, Error **errp)
>>>>>>> dev->qdev.id = id;
>>>>>>> }
>>>>>>>
>>>>>>> - dev->irq = spapr_irq_alloc(spapr, dev->irq, false, &local_err);
>>>>>>> + dev->irq = spapr_irq_alloc(spapr, false, &local_err);
>>>>>>
>>>>>> Silently breaking "irq" like this looks wrong. I'd rather officially remove
>>>>>> it first (ie, kill spapr_vio_props, -5 lines in spapr_vio.c).
>>>>>>
>>>>>> Of course, this raises the question of interface deprecation, and it should
>>>>>> theoretically follow the process described at:
>>>>>>
>>>>>> https://wiki.qemu.org/Features/LegacyRemoval#Rules_for_removing_an_interface
>>>>>>
>>>>>> Cc'ing Thomas, our Chief Deprecation Officer, for insights :)
>>>>>
>>>>> The property is a public interface. Just because it's not used by
>>>>> libvirt does not mean that nobody is using it. So yes, please follow the
>>>>> rules and mark it as deprecated first for two release, before you really
>>>>> remove it.
>>>>
>>>> This "irq" property is a problem to introduce a new static layout of IRQ
>>>> numbers. It is in complete opposition.
>>>>
>>>> Can we keep it as it is for old pseries machine (settable) and ignore it
>>>> for newer ? Would that be fine ?
>>>
>>> So, Thomas is right that we need to keep the interface while we go
>>> through the deprecation process, even though it's a bit of a pain
>>> (like you, I seriously doubt anyone ever used it).
>>
>> That's OK. The patch is simple. But it means that we have to keep the
>> irq_hint parameter for 2 QEMU versions.
>
> No.. the suggestion below is designed to avoid that..
>
>>> But, I think there's a way to avoid that getting in the way of your
>>> cleanups too much.
>>>
>>> A bunch of the current problems are caused because spapr_irq_alloc()
>>> conflates two meanings of "allocate": 1) finding a free irq to use for
>>> this device and 2) assigning that irq exclusively to this device.
>>>
>>> I think the first thing to do is to split those two parts. (1) will
>>> never take an irq parameter, (2) will always take an irq parameter.
>>> To implement the (to be deprecated) "irq" property on vio devices you
>>> should skip (1) and just call (2) with the given irq number.
>>
>> well, we need to call both because if "irq" is zero then when we
>> fallback to "1) finding a free irq to use."
>
> No, basically in the VIO code itself you'd have:
> irq = <irq property value>;
> if (!irq)
> irq = find_irq()
> claim_irq(irq);
>
> find_irq() never takes a hint, claim_irq() always does (except it's
> not really a hint).
ok. I add something like that in mind :
if (dev->irq) {
spapr_irq_assign(spapr, SPAPR_IRQ_VIO, dev->irq, &local_err);
if (local_err) {
error_propagate(errp, local_err);
return;
}
} else {
dev->irq = spapr_irq_alloc(spapr, SPAPR_IRQ_VIO, vio_index++,
&local_err);
if (local_err) {
error_propagate(errp, local_err);
return;
}
and spapr_irq_assign() would die when the vio "irq" property does.
>> But we can move the exclusive IRQ assignment (2) under the VIO model
>> which is the only one using it and start deprecating the property.
>
> No.. the exclusive claim would be global - everything would use that.
Yes, I see the model. I am not sure it's useful to have two routines
in the long term.
>>> The point of this series is to basically get rid of (1), but this
>>> first step means we don't need to worry about the hint parameter as we
>>> gradually remove it.
>>
>> OK. I think I got what you are asking for. (2) means adding an extra
>> handler to the sPAPR IRQ interface, which would always fail in the
>> new XICS sPAPR IRQ backend using static numbers.
>
> No.. (2), "claim_irq()" as I called it above, would _always_ be used.
> find_irq() would only be used to implement the legacy allocation.
> In various places we'll have code like this:
>
> if (legacy) {
> irq = find_irq();
> } else {
> irq = <fixed value or formula>;
> }
> claim_irq(irq);
I rather hide all this behind a class machine operation doing the
allocation, it will give us a clear view of the IRQ number space usage
instead of spreading the definitions in the code.
we will need something for XIVE any how.
> Where that fixed value could be something like:
> irq = PCI_LSI_BASE + phb->index*4 + pin#;
>
If you use a different class machine operation for allocation claim_irq()
is not needed at all. The only case to handle is the VIO "irq" property
which requires and extra operation.
C.
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Qemu-devel] [PATCH 2/4] sparp_pci: simplify how the PCI LSIs are allocated
2018-06-13 4:27 ` David Gibson
@ 2018-06-13 7:26 ` Cédric Le Goater
0 siblings, 0 replies; 32+ messages in thread
From: Cédric Le Goater @ 2018-06-13 7:26 UTC (permalink / raw)
To: David Gibson; +Cc: Greg Kurz, qemu-ppc, qemu-devel, Alexey Kardashevskiy
On 06/13/2018 06:27 AM, David Gibson wrote:
> On Tue, Jun 05, 2018 at 08:31:49AM +0200, Cédric Le Goater wrote:
>> On 06/05/2018 05:44 AM, David Gibson wrote:
>>> On Sat, May 26, 2018 at 11:40:23AM +0200, Greg Kurz wrote:
>>>> On Fri, 18 May 2018 18:44:03 +0200
>>>> Cédric Le Goater <clg@kaod.org> wrote:
>>>>
>>>>> PCI LSIs are today allocated one by one using the IRQ alloc_block
>>>>> routine. Change the code sequence to first allocate a PCI_NUM_PINS
>>>>> block. It will help us providing a generic IRQ framework to the
>>>>> machine.
>>>>>
>>>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>>>> ---
>>>>> hw/ppc/spapr_pci.c | 21 ++++++++++-----------
>>>>> 1 file changed, 10 insertions(+), 11 deletions(-)
>>>>>
>>>>> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
>>>>> index 39a14980d397..4fd97ffe4c6e 100644
>>>>> --- a/hw/ppc/spapr_pci.c
>>>>> +++ b/hw/ppc/spapr_pci.c
>>>>> @@ -1546,6 +1546,8 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
>>>>> sPAPRTCETable *tcet;
>>>>> const unsigned windows_supported =
>>>>> sphb->ddw_enabled ? SPAPR_PCI_DMA_MAX_WINDOWS : 1;
>>>>> + uint32_t irq;
>>>>> + Error *local_err = NULL;
>>>>>
>>>>> if (!spapr) {
>>>>> error_setg(errp, TYPE_SPAPR_PCI_HOST_BRIDGE " needs a pseries machine");
>>>>> @@ -1694,18 +1696,15 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
>>>>> QLIST_INSERT_HEAD(&spapr->phbs, sphb, list);
>>>>>
>>>>> /* Initialize the LSI table */
>>>>> - for (i = 0; i < PCI_NUM_PINS; i++) {
>>>>> - uint32_t irq;
>>>>> - Error *local_err = NULL;
>>>>> -
>>>>> - irq = spapr_irq_alloc_block(spapr, 1, true, false, &local_err);
>>>>> - if (local_err) {
>>>>> - error_propagate(errp, local_err);
>>>>> - error_prepend(errp, "can't allocate LSIs: ");
>>>>> - return;
>>>>> - }
>>>>> + irq = spapr_irq_alloc_block(spapr, PCI_NUM_PINS, true, false, &local_err);
>>>>> + if (local_err) {
>>>>> + error_propagate(errp, local_err);
>>>>> + error_prepend(errp, "can't allocate LSIs: ");
>>>>> + return;
>>>>> + }
>>>>>
>>>>
>>>> It isn't strictly equivalent. The current code would be happy with
>>>> sparse IRQ numbers, while the proposed one wouldn't... Anyway, this
>>>> cannot happen since we don't have PHB hotplug.
>>>
>>> This makes me pretty nervous, because it's not obvious it will come up
>>> with the same numbers in all circumstances, which we have to do for
>>> existing machine types.
>>
>> Given that :
>>
>> - irq_hint is "unused"
>> - all IRQs are allocated sequentially at machine init,
>> - spapr_pci is the only model using the block allocation for MSIs,
>> potentially fragmenting more the IRQ number space but done at
>> guest runtime.
>> - the PHB LSI are the allocated at realize time doing the loop above,
>> - we don't support PHB hotplug
>> - we do support PHB coldplug but then the IRQ allocation is done
>> at machine time,
>>
>> it seems highly improbable that the IRQ number space is fragmented
>> to a point which would not allow the loop above to return four
>> contiguous IRQ numbers, always.
>
> Well, assuming irq_hint really is unused, that's right. But we can't
> assume that - that's the whole point of the deprecation thing.
>
> Given that, AIUI, just one vio device with irq= set to a value that
> would be within an LSI block allocated under the old scheme would
> result in the new scheme returning a non-contiguous set of LSIs -
> i.e. a different result from what we used to have.
>
>> That is why I felt confident changing the loop to a single block
>> allocation.
>>
>>> It's also not obvious to me why it's useful
>>> to go via this step before going straight to static allocation of the
>>> irq numbers.
>>
>> It pollutes the new sPAPR IRQ interface API with an extra parameter
>> to support both underlying backend and it complexifies the code
>> to handle block allocation of a single IRQ (like above) within an
>> IRQ range (the PCI LSIs).
>>
>> So you end up having a family, a device index, a count, an alignment,
>> and an index within the range. pffut.
>>
>> Also, could we kill the alignment ?
>
> Since we sometimes pass 'true', no, we can't, without changing the
> existing pattern of allocations, which we can't do.
To be honest, this is very much discouraging.
C.
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Qemu-devel] [PATCH 3/4] spapr: introduce a generic IRQ frontend to the machine
2018-06-13 5:00 ` David Gibson
@ 2018-06-13 7:44 ` Cédric Le Goater
0 siblings, 0 replies; 32+ messages in thread
From: Cédric Le Goater @ 2018-06-13 7:44 UTC (permalink / raw)
To: David Gibson; +Cc: qemu-ppc, qemu-devel, Greg Kurz, Alexey Kardashevskiy
On 06/13/2018 07:00 AM, David Gibson wrote:
> On Fri, May 18, 2018 at 06:44:04PM +0200, Cédric Le Goater wrote:
>> This proposal moves all the related IRQ routines of the sPAPR machine
>> behind a class interface to prepare for future changes in the IRQ
>> controller model. First of which is a reorganization of the IRQ number
>> space layout and a second, coming later, will be to integrate the
>> support for the new POWER9 XIVE IRQ controller.
>>
>> The new interface defines a set of fixed IRQ number ranges, for each
>> IRQ type, in which devices allocate the IRQ numbers they need
>> depending on a unique device index. Here is the layout :
>>
>> SPAPR_IRQ_IPI 0x0 /* 1 IRQ per CPU */
>> SPAPR_IRQ_EPOW 0x1000 /* 1 IRQ per device */
>> SPAPR_IRQ_HOTPLUG 0x1001 /* 1 IRQ per device */
>> SPAPR_IRQ_VIO 0x1100 /* 1 IRQ per device */
>> SPAPR_IRQ_PCI_LSI 0x1200 /* 4 IRQs per device */
>> SPAPR_IRQ_PCI_MSI 0x1400 /* 1K IRQs per device */
>>
>> The IPI range is reserved for future use when XIVE support
>> comes in.
>>
>> The routines of this interface encompass the previous needs and the
>> new ones and seem complex but the provided IRQ backend should
>> implement what we have today without any functional changes.
>>
>> Each device model is modified to take the new interface into account
>> using the IRQ range/type definitions and a device index. A part from
>> the VIO devices, lacking an id, the changes are relatively simple.
>
> I find your use of "back end" vs. "front end" in this patch and the
> next kind of confusing.
This is the the front end, interface used by the machine and devices :
int spapr_irq_assign(sPAPRMachineState *spapr, uint32_t range, uint32_t irq,
Error **errp);
int spapr_irq_alloc(sPAPRMachineState *spapr, uint32_t range, uint32_t index,
Error **errp);
int spapr_irq_alloc_block(sPAPRMachineState *spapr, uint32_t range,
uint32_t index, int num, bool align, Error **errp);
void spapr_irq_free(sPAPRMachineState *spapr, int irq, int num, Error **errp);
qemu_irq spapr_qirq(sPAPRMachineState *spapr, int irq);
and the backend, which can be different depending on the machine level,
old vs. new layout, or on depending on the interrupt controller.
typedef struct sPAPRIrq {
uint32_t nr_irqs;
const sPAPRPIrqRange *ranges;
void (*init)(sPAPRMachineState *spapr, Error **errp);
int (*assign)(sPAPRMachineState *spapr, uint32_t range, uint32_t irq,
Error **errp);
int (*alloc)(sPAPRMachineState *spapr, uint32_t range, uint32_t index,
Error **errp);
int (*alloc_block)(sPAPRMachineState *spapr, uint32_t range,
uint32_t index, int num, bool align, Error **errp);
void (*free)(sPAPRMachineState *spapr, int irq, int num, Error **errp);
qemu_irq (*qirq)(sPAPRMachineState *spapr, int irq);
void (*print_info)(sPAPRMachineState *spapr, Monitor *mon);
} sPAPRIrq;
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>> include/hw/ppc/spapr.h | 10 +-
>> include/hw/ppc/spapr_irq.h | 46 +++++++++
>> hw/ppc/spapr.c | 177 +---------------------------------
>> hw/ppc/spapr_events.c | 7 +-
>> hw/ppc/spapr_irq.c | 233 +++++++++++++++++++++++++++++++++++++++++++++
>> hw/ppc/spapr_pci.c | 21 +++-
>> hw/ppc/spapr_vio.c | 5 +-
>> hw/ppc/Makefile.objs | 2 +-
>> 8 files changed, 308 insertions(+), 193 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 2cfdfdd67eaf..4eb212b16a51 100644
>> --- a/include/hw/ppc/spapr.h
>> +++ b/include/hw/ppc/spapr.h
>> @@ -3,10 +3,10 @@
>>
>> #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"
>> +#include "hw/ppc/spapr_irq.h"
>>
>> struct VIOsPAPRBus;
>> struct sPAPRPHBState;
>> @@ -104,6 +104,7 @@ struct sPAPRMachineClass {
>> unsigned n_dma, uint32_t *liobns, Error **errp);
>> sPAPRResizeHPT resize_hpt_default;
>> sPAPRCapabilities default_caps;
>> + sPAPRIrq *irq;
>> };
>>
>> /**
>> @@ -773,13 +774,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_alloc(sPAPRMachineState *spapr, bool lsi, Error **errp);
>> -int spapr_irq_alloc_block(sPAPRMachineState *spapr, int num, bool lsi,
>> - bool align, 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
>> new file mode 100644
>> index 000000000000..caf4c33d4cec
>> --- /dev/null
>> +++ b/include/hw/ppc/spapr_irq.h
>> @@ -0,0 +1,46 @@
>> +/*
>> + * 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
>> +
>> +#include "hw/ppc/xics.h"
>> +
>> +/*
>> + * IRQ ranges
>> + */
>> +#define SPAPR_IRQ_IPI 0x0 /* 1 IRQ per CPU */
>> +#define SPAPR_IRQ_EPOW 0x1000 /* 1 IRQ per device */
>> +#define SPAPR_IRQ_HOTPLUG 0x1001 /* 1 IRQ per device */
>> +#define SPAPR_IRQ_VIO 0x1100 /* 1 IRQ per device */
>> +#define SPAPR_IRQ_PCI_LSI 0x1200 /* 4 IRQs per device */
>> +#define SPAPR_IRQ_PCI_MSI 0x1400 /* 1K IRQs per device covered by
>> + * a bitmap allocator */
>
> These look like they belong in the next patch with the fixed allocations.
They act as ids also, can be discussed.
>> +typedef struct sPAPRIrq {
>
> Much of this structure doesn't make sense to me. AIUI, the idea is
> that this method structure will vary with the xics vs. xive backend,
> yes?
This is not only XIVE. the static allocation scheme changes all the
backend.
> Comments below are based on that assumption.
>
>> + uint32_t nr_irqs;
>> +
>> + void (*init)(sPAPRMachineState *spapr, Error **errp);
>> + int (*alloc)(sPAPRMachineState *spapr, uint32_t range, uint32_t index,
>> + Error **errp);
>
> 'range' has no place here - working out the indices should be entirely
> on the spapr side, only the final irq number should need to go to the
> backend.
RAnge identifies the device, see above. Maybe badly named.
> I'd also prefer to call it "claim" to distinguish it from the existing
> "alloc" function which finds a free irq as well as setting it up for
> our exclusive use.
...
>> + int (*alloc_block)(sPAPRMachineState *spapr, uint32_t range,
>> + uint32_t index, int num, bool align, Error **errp);
>
> There should be no need for this. We needed an alloc_block routine
> before, because we wanted to ensure that we got a contiguous (and
> maybe aligned) block of irqs. By the time we go to the backend we
> should already have absolute irq numbers, so we can just claim each of
> them separately.
...
So, David, let's stop wasting our time. please provide what you feel is
right and I will build on top of it.
For your information, please understand that I generally make sure that
a patchset fits older and newer machines, that migration is tested and
that the XIVE patchset is resynced. It takes a HUGE amount of time and
it's a not a ramdom idea that just looks nice.
Thanks,
C.
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Qemu-devel] [PATCH 1/4] spapr: remove irq_hint parameter from spapr_irq_alloc()
2018-06-13 7:24 ` Cédric Le Goater
@ 2018-06-14 3:46 ` David Gibson
2018-06-14 13:26 ` Cédric Le Goater
0 siblings, 1 reply; 32+ messages in thread
From: David Gibson @ 2018-06-14 3:46 UTC (permalink / raw)
To: Cédric Le Goater
Cc: Thomas Huth, Greg Kurz, qemu-ppc, qemu-devel, Alexey Kardashevskiy
[-- Attachment #1: Type: text/plain, Size: 7800 bytes --]
On Wed, Jun 13, 2018 at 09:24:12AM +0200, Cédric Le Goater wrote:
> On 06/13/2018 06:22 AM, David Gibson wrote:
> > On Tue, Jun 05, 2018 at 08:41:13AM +0200, Cédric Le Goater wrote:
> >> On 06/05/2018 05:34 AM, David Gibson wrote:
> >>> On Mon, May 28, 2018 at 09:06:12AM +0200, Cédric Le Goater wrote:
> >>>> On 05/28/2018 08:17 AM, Thomas Huth wrote:
> >>>>> On 25.05.2018 16:02, Greg Kurz wrote:
> >>>>>> On Fri, 18 May 2018 18:44:02 +0200
> >>>>>> Cédric Le Goater <clg@kaod.org> wrote:
> >>>>>>
> >>>>>>> This IRQ number hint can possibly be used by the VIO devices if the
> >>>>>>> "irq" property is defined on the command line but it seems it is never
> >>>>>>> the case. It is not used in libvirt for instance. So, let's remove it
> >>>>>>> to simplify future changes.
> >>>>>>>
> >>>>>>
> >>>>>> Setting an irq manually looks a bit anachronistic. I doubt anyone would
> >>>>>> do that nowadays, and the patch does a nice cleanup. So this looks like
> >>>>>> a good idea.
> >>>>> [...]
> >>>>>>> diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c
> >>>>>>> index 472dd6f33a96..cc064f64fccf 100644
> >>>>>>> --- a/hw/ppc/spapr_vio.c
> >>>>>>> +++ b/hw/ppc/spapr_vio.c
> >>>>>>> @@ -455,7 +455,7 @@ static void spapr_vio_busdev_realize(DeviceState *qdev, Error **errp)
> >>>>>>> dev->qdev.id = id;
> >>>>>>> }
> >>>>>>>
> >>>>>>> - dev->irq = spapr_irq_alloc(spapr, dev->irq, false, &local_err);
> >>>>>>> + dev->irq = spapr_irq_alloc(spapr, false, &local_err);
> >>>>>>
> >>>>>> Silently breaking "irq" like this looks wrong. I'd rather officially remove
> >>>>>> it first (ie, kill spapr_vio_props, -5 lines in spapr_vio.c).
> >>>>>>
> >>>>>> Of course, this raises the question of interface deprecation, and it should
> >>>>>> theoretically follow the process described at:
> >>>>>>
> >>>>>> https://wiki.qemu.org/Features/LegacyRemoval#Rules_for_removing_an_interface
> >>>>>>
> >>>>>> Cc'ing Thomas, our Chief Deprecation Officer, for insights :)
> >>>>>
> >>>>> The property is a public interface. Just because it's not used by
> >>>>> libvirt does not mean that nobody is using it. So yes, please follow the
> >>>>> rules and mark it as deprecated first for two release, before you really
> >>>>> remove it.
> >>>>
> >>>> This "irq" property is a problem to introduce a new static layout of IRQ
> >>>> numbers. It is in complete opposition.
> >>>>
> >>>> Can we keep it as it is for old pseries machine (settable) and ignore it
> >>>> for newer ? Would that be fine ?
> >>>
> >>> So, Thomas is right that we need to keep the interface while we go
> >>> through the deprecation process, even though it's a bit of a pain
> >>> (like you, I seriously doubt anyone ever used it).
> >>
> >> That's OK. The patch is simple. But it means that we have to keep the
> >> irq_hint parameter for 2 QEMU versions.
> >
> > No.. the suggestion below is designed to avoid that..
> >
> >>> But, I think there's a way to avoid that getting in the way of your
> >>> cleanups too much.
> >>>
> >>> A bunch of the current problems are caused because spapr_irq_alloc()
> >>> conflates two meanings of "allocate": 1) finding a free irq to use for
> >>> this device and 2) assigning that irq exclusively to this device.
> >>>
> >>> I think the first thing to do is to split those two parts. (1) will
> >>> never take an irq parameter, (2) will always take an irq parameter.
> >>> To implement the (to be deprecated) "irq" property on vio devices you
> >>> should skip (1) and just call (2) with the given irq number.
> >>
> >> well, we need to call both because if "irq" is zero then when we
> >> fallback to "1) finding a free irq to use."
> >
> > No, basically in the VIO code itself you'd have:
> > irq = <irq property value>;
> > if (!irq)
> > irq = find_irq()
> > claim_irq(irq);
> >
> > find_irq() never takes a hint, claim_irq() always does (except it's
> > not really a hint).
>
> ok. I add something like that in mind :
>
> if (dev->irq) {
> spapr_irq_assign(spapr, SPAPR_IRQ_VIO, dev->irq, &local_err);
> if (local_err) {
> error_propagate(errp, local_err);
> return;
> }
> } else {
> dev->irq = spapr_irq_alloc(spapr, SPAPR_IRQ_VIO, vio_index++,
> &local_err);
> if (local_err) {
> error_propagate(errp, local_err);
> return;
> }
>
> and spapr_irq_assign() would die when the vio "irq" property does.
Right, but this obscures the fact that the function of assign/claim is
also performed at the end of the alloc function.
> >> But we can move the exclusive IRQ assignment (2) under the VIO model
> >> which is the only one using it and start deprecating the property.
> >
> > No.. the exclusive claim would be global - everything would use that.
>
> Yes, I see the model. I am not sure it's useful to have two routines
> in the long term.
Well, we won't, in a sense, since we want to phase out the find()
part.
> >>> The point of this series is to basically get rid of (1), but this
> >>> first step means we don't need to worry about the hint parameter as we
> >>> gradually remove it.
> >>
> >> OK. I think I got what you are asking for. (2) means adding an extra
> >> handler to the sPAPR IRQ interface, which would always fail in the
> >> new XICS sPAPR IRQ backend using static numbers.
> >
> > No.. (2), "claim_irq()" as I called it above, would _always_ be used.
> > find_irq() would only be used to implement the legacy allocation.
> > In various places we'll have code like this:
> >
> > if (legacy) {
> > irq = find_irq();
> > } else {
> > irq = <fixed value or formula>;
> > }
> > claim_irq(irq);
>
> I rather hide all this behind a class machine operation doing the
> allocation,
I see that, but I think it's a bad idea. We have need for variants of
both the find() and claim() operations, but they vary based on
different things. Combining them into one abstraction point just
muddies that.
In particular, we're aiming to allow runtime switching of xics
vs. xive, for which we require the same irq allocations in both.
Combining the "claim" backend with the allocation makes it much less
obvious that we really do have the same allocations in both cases.
> it will give us a clear view of the IRQ number space usage
> instead of spreading the definitions in the code.
Yeah.. I really think centralizing the management of the IRQ space is
an anti goal here. Well, other than comments and #defines for the
sub-range bases.
Kind of the whole point of the fixed allocation scheme is so that we
don't need any centralized management, each device can just figure out
its own irq based only on static, read-only information.
To put the allocation back into a common routine requires explicit
enumeration of of the various subsystems, to be passed from the
subsystem into the central allocator. Thus we mirror the overall
structure of the code in the allocator - I think that's a worse
duplication that just setting the irq number in various places.
> we will need something for XIVE any how.
>
> > Where that fixed value could be something like:
> > irq = PCI_LSI_BASE + phb->index*4 + pin#;
> >
>
> If you use a different class machine operation for allocation claim_irq()
> is not needed at all. The only case to handle is the VIO "irq" property
> which requires and extra operation.
>
> C.
>
--
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 --]
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Qemu-devel] [PATCH 1/4] spapr: remove irq_hint parameter from spapr_irq_alloc()
2018-06-14 3:46 ` David Gibson
@ 2018-06-14 13:26 ` Cédric Le Goater
0 siblings, 0 replies; 32+ messages in thread
From: Cédric Le Goater @ 2018-06-14 13:26 UTC (permalink / raw)
To: David Gibson
Cc: Thomas Huth, Greg Kurz, qemu-ppc, qemu-devel, Alexey Kardashevskiy
On 06/14/2018 05:46 AM, David Gibson wrote:
> On Wed, Jun 13, 2018 at 09:24:12AM +0200, Cédric Le Goater wrote:
>> On 06/13/2018 06:22 AM, David Gibson wrote:
>>> On Tue, Jun 05, 2018 at 08:41:13AM +0200, Cédric Le Goater wrote:
>>>> On 06/05/2018 05:34 AM, David Gibson wrote:
>>>>> On Mon, May 28, 2018 at 09:06:12AM +0200, Cédric Le Goater wrote:
>>>>>> On 05/28/2018 08:17 AM, Thomas Huth wrote:
>>>>>>> On 25.05.2018 16:02, Greg Kurz wrote:
>>>>>>>> On Fri, 18 May 2018 18:44:02 +0200
>>>>>>>> Cédric Le Goater <clg@kaod.org> wrote:
>>>>>>>>
>>>>>>>>> This IRQ number hint can possibly be used by the VIO devices if the
>>>>>>>>> "irq" property is defined on the command line but it seems it is never
>>>>>>>>> the case. It is not used in libvirt for instance. So, let's remove it
>>>>>>>>> to simplify future changes.
>>>>>>>>>
>>>>>>>>
>>>>>>>> Setting an irq manually looks a bit anachronistic. I doubt anyone would
>>>>>>>> do that nowadays, and the patch does a nice cleanup. So this looks like
>>>>>>>> a good idea.
>>>>>>> [...]
>>>>>>>>> diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c
>>>>>>>>> index 472dd6f33a96..cc064f64fccf 100644
>>>>>>>>> --- a/hw/ppc/spapr_vio.c
>>>>>>>>> +++ b/hw/ppc/spapr_vio.c
>>>>>>>>> @@ -455,7 +455,7 @@ static void spapr_vio_busdev_realize(DeviceState *qdev, Error **errp)
>>>>>>>>> dev->qdev.id = id;
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>> - dev->irq = spapr_irq_alloc(spapr, dev->irq, false, &local_err);
>>>>>>>>> + dev->irq = spapr_irq_alloc(spapr, false, &local_err);
>>>>>>>>
>>>>>>>> Silently breaking "irq" like this looks wrong. I'd rather officially remove
>>>>>>>> it first (ie, kill spapr_vio_props, -5 lines in spapr_vio.c).
>>>>>>>>
>>>>>>>> Of course, this raises the question of interface deprecation, and it should
>>>>>>>> theoretically follow the process described at:
>>>>>>>>
>>>>>>>> https://wiki.qemu.org/Features/LegacyRemoval#Rules_for_removing_an_interface
>>>>>>>>
>>>>>>>> Cc'ing Thomas, our Chief Deprecation Officer, for insights :)
>>>>>>>
>>>>>>> The property is a public interface. Just because it's not used by
>>>>>>> libvirt does not mean that nobody is using it. So yes, please follow the
>>>>>>> rules and mark it as deprecated first for two release, before you really
>>>>>>> remove it.
>>>>>>
>>>>>> This "irq" property is a problem to introduce a new static layout of IRQ
>>>>>> numbers. It is in complete opposition.
>>>>>>
>>>>>> Can we keep it as it is for old pseries machine (settable) and ignore it
>>>>>> for newer ? Would that be fine ?
>>>>>
>>>>> So, Thomas is right that we need to keep the interface while we go
>>>>> through the deprecation process, even though it's a bit of a pain
>>>>> (like you, I seriously doubt anyone ever used it).
>>>>
>>>> That's OK. The patch is simple. But it means that we have to keep the
>>>> irq_hint parameter for 2 QEMU versions.
>>>
>>> No.. the suggestion below is designed to avoid that..
>>>
>>>>> But, I think there's a way to avoid that getting in the way of your
>>>>> cleanups too much.
>>>>>
>>>>> A bunch of the current problems are caused because spapr_irq_alloc()
>>>>> conflates two meanings of "allocate": 1) finding a free irq to use for
>>>>> this device and 2) assigning that irq exclusively to this device.
>>>>>
>>>>> I think the first thing to do is to split those two parts. (1) will
>>>>> never take an irq parameter, (2) will always take an irq parameter.
>>>>> To implement the (to be deprecated) "irq" property on vio devices you
>>>>> should skip (1) and just call (2) with the given irq number.
>>>>
>>>> well, we need to call both because if "irq" is zero then when we
>>>> fallback to "1) finding a free irq to use."
>>>
>>> No, basically in the VIO code itself you'd have:
>>> irq = <irq property value>;
>>> if (!irq)
>>> irq = find_irq()
>>> claim_irq(irq);
>>>
>>> find_irq() never takes a hint, claim_irq() always does (except it's
>>> not really a hint).
>>
>> ok. I add something like that in mind :
>>
>> if (dev->irq) {
>> spapr_irq_assign(spapr, SPAPR_IRQ_VIO, dev->irq, &local_err);
>> if (local_err) {
>> error_propagate(errp, local_err);
>> return;
>> }
>> } else {
>> dev->irq = spapr_irq_alloc(spapr, SPAPR_IRQ_VIO, vio_index++,
>> &local_err);
>> if (local_err) {
>> error_propagate(errp, local_err);
>> return;
>> }
>>
>> and spapr_irq_assign() would die when the vio "irq" property does.
>
> Right, but this obscures the fact that the function of assign/claim is
> also performed at the end of the alloc function.
>
>>>> But we can move the exclusive IRQ assignment (2) under the VIO model
>>>> which is the only one using it and start deprecating the property.
>>>
>>> No.. the exclusive claim would be global - everything would use that.
>>
>> Yes, I see the model. I am not sure it's useful to have two routines
>> in the long term.
>
> Well, we won't, in a sense, since we want to phase out the find()
> part.
>
>>>>> The point of this series is to basically get rid of (1), but this
>>>>> first step means we don't need to worry about the hint parameter as we
>>>>> gradually remove it.
>>>>
>>>> OK. I think I got what you are asking for. (2) means adding an extra
>>>> handler to the sPAPR IRQ interface, which would always fail in the
>>>> new XICS sPAPR IRQ backend using static numbers.
>>>
>>> No.. (2), "claim_irq()" as I called it above, would _always_ be used.
>>> find_irq() would only be used to implement the legacy allocation.
>>> In various places we'll have code like this:
>>>
>>> if (legacy) {
>>> irq = find_irq();
>>> } else {
>>> irq = <fixed value or formula>;
>>> }
>>> claim_irq(irq);
>>
>> I rather hide all this behind a class machine operation doing the
>> allocation,
>
> I see that, but I think it's a bad idea. We have need for variants of
> both the find() and claim() operations, but they vary based on
> different things. Combining them into one abstraction point just
> muddies that.
>
> In particular, we're aiming to allow runtime switching of xics
> vs. xive, for which we require the same irq allocations in both.
> Combining the "claim" backend with the allocation makes it much less
> obvious that we really do have the same allocations in both cases.
>
>> it will give us a clear view of the IRQ number space usage
>> instead of spreading the definitions in the code.
>
> Yeah.. I really think centralizing the management of the IRQ space is
> an anti goal here. Well, other than comments and #defines for the
> sub-range bases.
OK. so, we agree on these, at least.
> Kind of the whole point of the fixed allocation scheme is so that we
> don't need any centralized management, each device can just figure out
> its own irq based only on static, read-only information.
How do we know which IRQ number is being used or not ? The IRQ controller
needs to know that.
> To put the allocation back into a common routine requires explicit
> enumeration of of the various subsystems, to be passed from the
> subsystem into the central allocator.
yes.
> Thus we mirror the overall structure of the code in the allocator -
hmm, kind of, yes. You need to define the ranges somewhere anyhow.
> I think that's a worse
> duplication that just setting the irq number in various places.
but we need to do more than just that. The XICS, legacy and old, and
the XIVE interrupt controller need to declare those IRQ numbers in
their *status* tables or/and in their *routing* tables. I mean how
would the KVM device work for instance if it didn't know anything
about the layout ?
The machine IRQs and its device IRQs are linked to the IRQ controller,
we can not just ignore that. I really don't understand.
Please take a look at the resulting three backends :
https://github.com/legoater/qemu/blob/xive-3.0/hw/ppc/spapr_irq.c
They do much more, they
- initialize the IRQ controller (we could even imagine having a
new backend when KVM is in use.)
- define the ranges
- alloc/free IRQ numbers *from* an IRQ controller POV
- return the QEMU irq object
- handle 'info pic'
- populate the interrupt DT node
- allocated the CPU interrupt presenter
- handle IRQ migration quirks
- define IRQ OV5 bits
This needs a low latency discussion.
C.
^ permalink raw reply [flat|nested] 32+ messages in thread
end of thread, other threads:[~2018-06-14 13:27 UTC | newest]
Thread overview: 32+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-18 16:44 [Qemu-devel] [PATCH 0/4] spapr: generic IRQ frontend Cédric Le Goater
2018-05-18 16:44 ` [Qemu-devel] [PATCH 1/4] spapr: remove irq_hint parameter from spapr_irq_alloc() Cédric Le Goater
2018-05-25 14:02 ` Greg Kurz
2018-05-28 6:17 ` Thomas Huth
2018-05-28 7:06 ` Cédric Le Goater
2018-05-28 7:18 ` [Qemu-devel] [Qemu-ppc] " Thomas Huth
2018-05-28 9:20 ` Cédric Le Goater
2018-05-28 12:09 ` Greg Kurz
2018-05-28 13:33 ` Cédric Le Goater
2018-06-05 3:34 ` [Qemu-devel] " David Gibson
2018-06-05 6:41 ` Cédric Le Goater
2018-06-13 4:22 ` David Gibson
2018-06-13 7:24 ` Cédric Le Goater
2018-06-14 3:46 ` David Gibson
2018-06-14 13:26 ` Cédric Le Goater
2018-06-02 9:19 ` Cédric Le Goater
2018-06-04 6:05 ` Cédric Le Goater
2018-06-02 9:10 ` Cédric Le Goater
2018-05-18 16:44 ` [Qemu-devel] [PATCH 2/4] sparp_pci: simplify how the PCI LSIs are allocated Cédric Le Goater
2018-05-26 9:40 ` Greg Kurz
2018-06-05 3:44 ` David Gibson
2018-06-05 6:31 ` Cédric Le Goater
2018-06-13 4:27 ` David Gibson
2018-06-13 7:26 ` Cédric Le Goater
2018-05-18 16:44 ` [Qemu-devel] [PATCH 3/4] spapr: introduce a generic IRQ frontend to the machine Cédric Le Goater
2018-05-28 14:27 ` Greg Kurz
2018-06-13 5:00 ` David Gibson
2018-06-13 7:44 ` Cédric Le Goater
2018-05-18 16:44 ` [Qemu-devel] [PATCH 4/4] spapr: introduce a new IRQ backend using fixed IRQ number ranges Cédric Le Goater
2018-05-28 15:18 ` Greg Kurz
2018-05-28 15:42 ` Cédric Le Goater
2018-05-29 12:51 ` Cédric Le Goater
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.